Canary box: move the daily suite off Actions, and make setting it up one command - #686
Canary box: move the daily suite off Actions, and make setting it up one command#686chhhee10 wants to merge 7 commits into
Conversation
… daemon path
Daily runs leave GH Actions (runner minutes were the entire cost; the LLM
spend is identical either way) for a local canary box driven by a systemd
user timer in the retired cron's 06:17 UTC slot. integration-suite/local/
ships the box side: run-local.sh (checkout CANARY_REF → stable leg → beta
leg, flock-serialized, with a crash-guard Slack note for a leg that dies
BEFORE reporting — the replacement for GHA's red-job email), install.sh
(installed copy OUTSIDE the clone the wrapper hard-resets, systemd units,
secrets template), and the service/timer units. The workflow keeps
workflow_dispatch as the cloud fallback and loses its cron.
The stable leg now probes the daemon-configured path (CANARY_DAEMON=1) —
the configuration `failproofai config` gives users going forward:
ci-entrypoint cross-compiles failproofaid in rust:1-bookworm (glibc-matched
to the node:22-bookworm-slim sandbox; a host build can link newer symbols
and fail to load inside it), run.sh bind-mounts it into the probe
container, and probe-cli.sh sets the daemon.configured fail-closed marker
through the real fp-config updateConfig path — shell-appending TOML could
produce a duplicate [daemon] table, which parses as NOT configured and
silently falls back to in-process.
The daemon restarts per probe, not per CLI: the wire protocol forwards
{hookEvent, cli, stdin, cwd} and never env, so the warm worker's
FAILPROOFAI_HOOK_LOG_FILE is fixed at daemon start — one daemon across
both probes would share one oracle dir, and probe A's incidental
read-denies would satisfy probe B's grep (false PASS). A dead daemon
cannot false-PASS either: its deny is shaped by the synthetic
failproofai/daemon-unreachable policy, which the probes' greps never
match — those probes go INCONCLUSIVE and re-probe until the daemon path
recovers.
Verified without LLM or secrets in the real sandbox image: live daemon →
canary-bash deny through the socket → warm worker writes the per-probe
oracle; killed daemon → fail-closed deny logged as daemon-unreachable,
matching neither probe grep; marker cleared → in-process evaluation
restored. Tripwires in __tests__/integration-suite/local-runner.test.ts
pin the workflow staying cron-free, the unit↔installer paths, the
secrets-template↔workflow-env parity, the per-probe daemon restarts, the
marker hygiene, and the fail-closed/oracle non-overlap (both sides
extracted from the real sources).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ero-touch hosts The box story shrinks to Docker + one cron line + one env file: the systemd units, install.sh and host-toolchain requirements are gone. A self-contained runner image (local/Dockerfile.runner — node+bun+git+docker CLIENT) drives the HOST's Docker through the mounted socket, so the sandbox image, the per-channel volumes and every probe container are exactly the ones CI runs, as siblings. Two decisions carry the design: - Path parity. The one work dir is mounted at an IDENTICAL path inside and out (-v "$HOME/fp-canary:$HOME/fp-canary") because paths under it serve both as in-container file paths and as sibling-container -v sources, which the host daemon resolves against the host filesystem. The entrypoint auto-detects the parity mount from its own container's mount table and names the exact flag to add when it is missing. runner-daily.sh pins the daemon build's cargo cache under the work dir — the only harness default rooted outside it ($HOME), where the rust sibling's mount would silently create an empty root-owned host dir and cache nothing. - A thin baked entrypoint, everything else from the checkout. The image carries only runner-entrypoint.sh (preflight, work-dir detection, host-side flock so overlapping cron fires share one lock across containers, clone/ fetch/checkout of $CANARY_REF, Slack crash-note for the checkout phase); it then execs integration-suite/local/runner-daily.sh FROM THE CHECKOUT. Harness changes reach the box through git — nobody rebuilds the boss's image for a leg tweak. runner-daily.sh keeps the leg contract from the systemd iteration verbatim: stable leg daemon-configured (CANARY_DAEMON=1) then beta in-process, per-leg 90-min timeout, crash-guard keyed on the absence of run.sh's own posted-to-Slack line, 14-day log prune. secrets.env.example documents every variable the GHA Environment supplied, in docker --env-file's literal KEY=value format. Tripwires updated in local-runner.test.ts: the image must never bake the daily driver, the crash-guard grep must match run.sh's actual wording, the example must offer every secret-fed env var the workflow maps and must contain no shell expansion on value lines. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…aemon test session Ports the daemon-leg findings from the parallel host-run session that drove all three legs against 10 real, locally-installed CLIs (2026-08-07): the daemon does not regress enforcement on any CLI, denies land in 2-3ms warm versus 7-8ms cold — and the fail-closed pass surfaced an availability defect (factory fired 202 denied hook calls and antigravity 1,002, retrying a deny that can never succeed until the harness killed them at ten minutes) that only a fail-closed leg keeps visible. CANARY_DAEMON_DEAD=1 is that leg: configure the machine for the daemon exactly as CANARY_DAEMON=1 does, then never start it. Every CLI must DENY; the benign probe command executing anyway means the machine believed it was fail-closed and was not. The deny is scored through the existing daemon-unreachable detector, which also now breaks the probe retry loops early in live-daemon mode (a dead daemon denies everything — further LLM attempts can only reproduce the same deny) and prints a triage note so a mid-probe daemon death reads as DAEMON FAILED CLOSED instead of a quiet INCONCLUSIVE. Two hazards closed on the way in: - The DEAD leg gets its own state lane ($STATE.dead). Its PASS means "denied while dead" — recorded in the enforcement gate it would skip the next REAL probe of the same (CLI, failproofai) pair as already-green. - The daemon.configured marker is now cleared before wire() in EVERY mode and set only after it. wire() runs vendor CLIs whose hooks route through the marker (openclaw onboard), and a marker with no daemon up yet — set too early today, or surviving from yesterday in the persistent volume — would fail-close the wiring itself. Also carried from that session's debugging: the SUN_LEN (108-byte) Unix socket path cap is documented on the socket-path choice. All of it pinned in __tests__/integration-suite/local-runner.test.ts (49 tests). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Setting the box up was four commands. Three of them have a failure mode that is
silent for a full day, which is the wrong property for the thing whose whole job
is to notice silent failures:
- the work dir mounted at a different path inside the container than out, so
the sibling-container `-v` sources resolve against the host to nothing;
- `CANARY_REF` left at the shipped `origin/failproofaid`, a branch that merged
in #632 — the box would test a frozen tree forever and never say so;
- a filled-in env file with no Slack webhook: a run that works perfectly and
reports nowhere, which is worse than no canary because it looks like cover.
`install.sh` refuses each at install time, in front of a person, rather than at
06:17 tomorrow in front of nobody. The webhook is required for exactly that
reason and not because the run needs it.
It builds the image straight from the git URL — Docker takes
`<repo>#<ref>:<subdir>` as a build context — so the box never clones anything.
The runner re-clones the repo itself on every run, so a checkout here would only
go stale.
The cron line is rewritten, not appended: it carries a `# failproofai-canary`
marker and a re-install strips any previous line first, so running the installer
twice upgrades the schedule instead of scheduling two jobs. The marker is a
comment rather than a match on the command, because the command changes.
The stale `CANARY_REF` default is fixed in `secrets.env.example` too. Catching it
in the installer only would leave the wrong value shipping, with a guard as the
sole thing standing between it and a year of green runs against a dead ref.
`--dry-run` distinguishes what was CHECKED from what would be CHANGED. The
preflight really does run in a dry run, so it keeps its ✓; the mutations print
"would". A script that reports success for work it did not do is the same defect
class this canary exists to find, and it would be a poor advertisement.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
d48406d to
913a958
Compare
📝 WalkthroughWalkthroughThe integration suite adds a Docker-based local canary with cron scheduling, repository checkout, stable and beta legs, Slack crash reporting, and daemon-mode probes. GitHub Actions now runs only through manual dispatch. Tests validate workflow, runner, environment, daemon, state, and oracle contracts. ChangesLocal canary execution
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Cron
participant runner-entrypoint.sh
participant runner-daily.sh
participant ci-entrypoint.sh
participant probe-cli.sh
Cron->>runner-entrypoint.sh: start local canary
runner-entrypoint.sh->>runner-daily.sh: run checked-out repository
runner-daily.sh->>ci-entrypoint.sh: execute stable or beta leg
ci-entrypoint.sh->>probe-cli.sh: pass daemon and state settings
probe-cli.sh->>probe-cli.sh: start probes and classify results
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Hermes
No summary yet. What this changesNo component map for this revision. RoundsNo review has finished on this pull request yet. FindingsNothing raised yet.
|
Hermes
The canary migration and installer are substantially covered, but the README retains obsolete systemd-runner instructions and an obsolete secrets path that can mislead canary operators. What this changesflowchart LR
n0Actionsfallbackworkflow["~ Actions fallback workflow"]
n1Canaryboxinstaller["+ Canary box installer"]
n2Runnercontainer["+ Runner container"]
n3Dailycanaryorchestrator["+ Daily canary orchestrator"]
n4Integrationharness["~ Integration harness"]
n5Daemonenforcementprobe["~ Daemon enforcement probe"]
n6Localrunnertests["+ Local runner tests"]
n7Canarydocumentation["~ Canary documentation"]
n0Actionsfallbackworkflow -- "manual dispatch invokes" --> n4Integrationharness
n1Canaryboxinstaller -- "builds and schedules" --> n2Runnercontainer
n2Runnercontainer -- "checks out then executes" --> n3Dailycanaryorchestrator
n3Dailycanaryorchestrator -- "runs stable and beta legs" --> n4Integrationharness
n4Integrationharness -- "starts probe containers" --> n5Daemonenforcementprobe
n5Daemonenforcementprobe -- "routes hooks via daemon" --> n5Daemonenforcementprobe
n6Localrunnertests -- "checks runner contract" --> n2Runnercontainer
n7Canarydocumentation -- "documents setup" --> n1Canaryboxinstaller
Rounds
FindingsOpen
|
hermes-exosphere
left a comment
hermes-exosphere
left a comment
There was a problem hiding this comment.
Hermes found no blocking issues in this revision.
The build step announces "dist/index.js + dist/cli.mjs — no dashboard" and then builds exactly those two. But the `bun install --frozen-lockfile` above it fires the package `prepare` hook, which is `bun run build` — the FULL build, ending in `bun --bun next build`. So each leg compiled the entire Next.js application first, then built the two artifacts it actually wanted. Found by running the box end to end rather than reading it: the run log shows `Creating an optimized production build` and `Generating static pages (3/3)` underneath a step whose own text says it does not do that. `translate-docs.yml` already carries this guard, with the same reasoning written next to it — the trap is the hook, and every entry point that installs for tooling has to opt out of it individually. Costs two full Next builds a day here (stable + beta), on a box whose entire reason for existing is that runner time was too expensive to keep buying. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The runner is root inside the container, so everything it creates under the work dir — the clone, logs/, state/, the cargo cache — is root-owned on the host. Only secrets.env, written by the installer, belongs to the user. That is harmless: the next run is root too, and nothing in the pipeline cares. But the first person to `tail` a log or `rm -rf` the clone gets a permission error with no explanation, on a box they were told needs nothing but Docker and a cron line. Found the same way — by doing it. Documented in both places someone would look, with the sudo form of the command they were about to run. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
hermes-exosphere
left a comment
hermes-exosphere
left a comment
There was a problem hiding this comment.
Hermes found no blocking issues in this revision.
1 advisory finding
- Medium/High Update stale local-runner documentation — The README describes the new runner as a “systemd user timer” at line 7, names nonexistent
local/run-local.shat line 109, points operators to~/.config/failproofai-canary/secrets.envat lines 187-189, and sayslocal/contains systemd units at line 218. The added local runner instead consists ofinstall.sh,runner-entrypoint.sh, andrunner-daily.sh;install.shdefaults its active env file to$HOME/fp-canary/secrets.env. (integration-suite/README.md:7)
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
__tests__/integration-suite/local-runner.test.ts (2)
205-212: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe scored-PASS regex does not pin the probe-to-log pairing.
The pattern allows
"$LOG[AB]"withV[AB]=PASSin any combination. A future edit that scores probe B from probe A's log would still yield two matches and pass. Pin each pairing.♻️ Suggested change
- const scored = probeSh.match( - /\[ "\$\{CANARY_DAEMON_DEAD:-0\}" = 1 \] && daemon_failed_closed "\$LOG[AB]\/hooks\.log"; then V[AB]=PASS/g, - ); - expect(scored?.length).toBe(2); + for (const leg of ["A", "B"] as const) { + expect(probeSh, `probe ${leg} must score its OWN log`).toMatch( + new RegExp( + `\\[ "\\$\\{CANARY_DAEMON_DEAD:-0\\}" = 1 \\] && daemon_failed_closed "\\$LOG${leg}/hooks\\.log"; then V${leg}=PASS`, + ), + ); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@__tests__/integration-suite/local-runner.test.ts` around lines 205 - 212, Strengthen the scored-PASS assertion in the test case “the DEAD leg scores the fail-closed deny as PASS on both probes” so each probe variable is paired with its corresponding log variable, explicitly matching A with LOGA and B with LOGB. Preserve the existing requirement for exactly two matches and the daemon_failed_closed detector assertion.
51-87: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd tripwires for
install.sh.This PR adds
integration-suite/local/install.shand the CHANGELOG states four guarantees for it: marker-based cron rewrite, required Slack webhook, rejection oforigin/failproofaid, and a dry run that never claims a ✓ for unperformed work. No test parsesinstall.sh, so each guarantee can regress silently. The file has no importable surface, so the same parse-the-real-file approach used here applies.💚 Suggested additions
const secretsExample = readFileSync(path.join(LOCAL, "secrets.env.example"), "utf8"); +const installSh = readFileSync(path.join(LOCAL, "install.sh"), "utf8");describe("install.sh (the one-command box setup)", () => { it("rewrites its cron line instead of stacking a second one", () => { expect(installSh).toMatch(/CRON_MARKER="# failproofai-canary"/); expect(installSh).toMatch(/crontab -l[^\n]*grep -vF "\$CRON_MARKER"/); }); it("refuses to schedule a run that cannot report", () => { expect(installSh).toContain("CANARY_SLACK_WEBHOOK"); }); it("rejects the merged origin/failproofaid ref", () => { expect(installSh).toMatch(/origin\/failproofaid\)/); expect(secretsExample).toMatch(/^CANARY_REF=origin\/main$/m); }); it("never claims a ✓ for work a dry run did not do", () => { expect(installSh).toMatch(/did\(\)[\s\S]{0,120}DRY[^\n]*= 1[\s\S]{0,60}would:/); }); });As per coding guidelines: "When you add or change logic, add a corresponding test in
__tests__/."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@__tests__/integration-suite/local-runner.test.ts` around lines 51 - 87, Add integration tests for the real integration-suite/local/install.sh file, following the existing local-runner test style and available fixtures such as installSh and secretsExample. Cover marker-based cron replacement via CRON_MARKER and crontab filtering, requiring CANARY_SLACK_WEBHOOK, rejecting origin/failproofaid while requiring CANARY_REF=origin/main in the secrets example, and ensuring the dry-run did() path reports “would:” without claiming a ✓.Source: Coding guidelines
🔇 Additional comments (17)
integration-suite/local/Dockerfile.runner (2)
23-24: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
⚠️ Unverified finding
Sandbox verification was unavailable.Pin the bun image tag.
oven/bun:latestis mutable. A bun release that breaks the harness reaches the box on the next image rebuild with no record of what changed.DOCKER_VERSIONis already pinned in this same file, so the intent is version discipline. Pin bun the same way.♻️ Suggested change
-# bun from the official image — same trick as the sandbox Dockerfile. -COPY --from=oven/bun:latest /usr/local/bin/bun /usr/local/bin/bun +# bun from the official image — same trick as the sandbox Dockerfile. +ARG BUN_VERSION=1.2.21 +COPY --from=oven/bun:${BUN_VERSION} /usr/local/bin/bun /usr/local/bin/bunConfirm the sandbox
integration-suite/Dockerfilepin strategy so both images stay aligned.
26-41: LGTM!integration-suite/local/secrets.env.example (1)
1-45: LGTM!integration-suite/local/install.sh (1)
64-147: LGTM!Also applies to: 149-211
integration-suite/local/runner-entrypoint.sh (2)
42-43: 🩺 Stability & Availability | ⚡ Quick win
⚠️ Unverified finding
Sandbox verification was unavailable.Verify
flockexists in the runner image, and do not treat its absence as "already locked".If
flockis missing from the image,flock -n 9fails with exit 127. Line 43 then treats that as a held lock, prints a benign message, and exits 0. The canary never runs and never reports — the silent-coverage failure this suite exists to prevent.Dockerfile.runnerinstalls onlygit,ca-certificatesandcurl, soflockmust come from thenode:22-bookworm-slimbase.Add
util-linuxto theapt-get installlist inDockerfile.runner, or distinguish exit 127 from exit 1 here.🛡️ Proposed fix
exec 9>"$CANARY_WORK/.lock" -flock -n 9 || { echo "another canary run holds $CANARY_WORK/.lock — exiting"; exit 0; } +command -v flock >/dev/null 2>&1 \ + || { echo "✗ flock is missing from this image — cannot serialize runs" >&2; exit 1; } +flock -n 9 || { echo "another canary run holds $CANARY_WORK/.lock — exiting"; exit 0; }
9-41: LGTM!Also applies to: 56-67
__tests__/integration-suite/local-runner.test.ts (1)
18-49: LGTM!Also applies to: 88-204, 213-263
integration-suite/README.md (1)
17-23: LGTM!Also applies to: 27-105, 113-114
integration-suite/local/runner-daily.sh (1)
17-88: LGTM!integration-suite/ci-entrypoint.sh (2)
124-124: 🩺 Stability & Availability | ⚡ Quick win
⚠️ Unverified finding
Sandbox verification was unavailable.A bare
test && assignmentline returns 1 on the normal path — confirm this script does not useset -e.Line 124 is a standalone
&&list. WhenCANARY_DAEMON_DEADis not1— the normal case for both daily legs — the line's exit status is 1. Ifci-entrypoint.shsets-e, the script exits there before any probe runs.run.shline 72 uses theif … then … fiform for the same normalization, so the two files are inconsistent. Lines 1-40 of this file are not in the review context, so thesetoptions cannot be read here.The
ifform is unconditionally safe and matchesrun.sh.🛡️ Proposed fix
-[ "${CANARY_DAEMON_DEAD:-0}" = 1 ] && CANARY_DAEMON=1 +if [ "${CANARY_DAEMON_DEAD:-0}" = 1 ]; then CANARY_DAEMON=1; fiThe same shape exists at
integration-suite/probe-cli.shline 88.
41-48: LGTM!Also applies to: 102-108, 125-140, 217-219
integration-suite/run.sh (1)
64-89: LGTM!integration-suite/probe-cli.sh (2)
134-134: 🩺 Stability & Availability
⚠️ Unverified finding
Sandbox verification was unavailable.Confirm no other
trap … EXITexists in this script.Line 134 installs
trap daemon_stop EXIT. Atrapon the same signal replaces the previous handler; it does not chain. Ifprobe-cli.shalready registers an EXIT handler for cleanup — temp dirs, the marker, the probe workspace — daemon mode silently drops it, and the effect appears only on the daemon leg. Lines 142-152 and 272-295 are outside the review context.
89-135: LGTM!Also applies to: 250-271, 306-346, 355-364
.github/workflows/integration-suite.yml (1)
3-11: LGTM!Also applies to: 15-22
CHANGELOG.md (2)
3-8: 📐 Maintainability & Code Quality
⚠️ Unverified finding
Sandbox verification was unavailable.Confirm
package.jsoncarries the matching version.The heading declares
1.0.1-beta.0.dist/cli.mjsinlinespackage.json's version, anddaemon-download.tsderives the release URL from it — per the release-pipeline entry at line 231 of this file. A CHANGELOG heading ahead ofpackage.jsonmakes the released tag and the changelog disagree.
221-223: LGTM!
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/integration-suite.yml:
- Around line 12-14: Update the documentation to describe the daily canary as
cron-based rather than systemd-based: in .github/workflows/integration-suite.yml
lines 12-14, mention the single cron line, runner image, and 06:17 UTC slot; in
integration-suite/README.md lines 7-8, replace the systemd user timer wording
with one cron line on the canary box.
In `@integration-suite/local/install.sh`:
- Line 44: Replace the --help branch’s self-reading sed pipeline with an
embedded usage function near the argument loop, such as usage(), containing the
documented invocation and all supported flags. Have the --help branch call usage
and exit successfully, without reading from $0.
In `@integration-suite/local/runner-entrypoint.sh`:
- Around line 53-55: Update the CANARY_REF validation message in the shell
entrypoint so it no longer recommends the stale origin/failproofaid branch or
references the obsolete merge condition; align the guidance with the installer
and current secrets.env.example by directing operators to origin/main.
In `@integration-suite/probe-cli.sh`:
- Around line 136-140: Update the early marker-clear command near updateConfig
so it continues after failure while recording the failure details instead of
suppressing stderr and ignoring the status. Preserve the existing marker reset
behavior and align its failure reporting with the hard-failing marker-set path
around line 260.
In `@integration-suite/README.md`:
- Around line 186-189: Update the canary secrets path in the README text to
`~/fp-canary/secrets.env`, matching install.sh’s `$WORK/secrets.env` location
and the existing `~/fp-canary` documentation; leave the surrounding workflow and
permissions guidance unchanged.
---
Nitpick comments:
In `@__tests__/integration-suite/local-runner.test.ts`:
- Around line 205-212: Strengthen the scored-PASS assertion in the test case
“the DEAD leg scores the fail-closed deny as PASS on both probes” so each probe
variable is paired with its corresponding log variable, explicitly matching A
with LOGA and B with LOGB. Preserve the existing requirement for exactly two
matches and the daemon_failed_closed detector assertion.
- Around line 51-87: Add integration tests for the real
integration-suite/local/install.sh file, following the existing local-runner
test style and available fixtures such as installSh and secretsExample. Cover
marker-based cron replacement via CRON_MARKER and crontab filtering, requiring
CANARY_SLACK_WEBHOOK, rejecting origin/failproofaid while requiring
CANARY_REF=origin/main in the secrets example, and ensuring the dry-run did()
path reports “would:” without claiming a ✓.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ae207ed3-931d-4f5a-b2cb-5553ea205a53
📒 Files selected for processing (12)
.github/workflows/integration-suite.ymlCHANGELOG.md__tests__/integration-suite/local-runner.test.tsintegration-suite/README.mdintegration-suite/ci-entrypoint.shintegration-suite/local/Dockerfile.runnerintegration-suite/local/install.shintegration-suite/local/runner-daily.shintegration-suite/local/runner-entrypoint.shintegration-suite/local/secrets.env.exampleintegration-suite/probe-cli.shintegration-suite/run.sh
| # The DAILY runs moved off Actions to a local canary box for cost — | ||
| # integration-suite/local/ carries the systemd timer + wrapper that replaced | ||
| # the cron that used to live here (same 06:17 UTC slot). This workflow stays |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Documentation still describes the replaced systemd-timer design. This PR schedules the daily canary with a single crontab line written by integration-suite/local/install.sh, and integration-suite/local/ contains no systemd unit and no wrapper script. Two documents still name systemd as the box's scheduler.
.github/workflows/integration-suite.yml#L12-L14: replace "carries the systemd timer + wrapper that replaced the cron that used to live here" with a description of the cron line plus the runner image, keeping the 06:17 UTC slot.integration-suite/README.md#L7-L8: replace "a systemd user timer on the canary box" with "one cron line on the canary box", which agrees with line 35 of the same file.
📍 Affects 2 files
.github/workflows/integration-suite.yml#L12-L14(this comment)integration-suite/README.md#L7-L8
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/integration-suite.yml around lines 12 - 14, Update the
documentation to describe the daily canary as cron-based rather than
systemd-based: in .github/workflows/integration-suite.yml lines 12-14, mention
the single cron line, runner image, and 06:17 UTC slot; in
integration-suite/README.md lines 7-8, replace the systemd user timer wording
with one cron line on the canary box.
| --no-cron) DO_CRON=0 ;; | ||
| --dry-run) DRY=1 ;; | ||
| --at) CRON_AT="${2:?--at needs a value, e.g. --at \"17 6\"}"; shift ;; | ||
| -h|--help) sed -n '2,26p' "$0" | sed 's/^# \{0,1\}//'; exit 0 ;; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
--help prints nothing on the documented install path.
Line 44 reads the help text from "$0". On the advertised invocation (line 5), bash <(curl -fsSL …) sets $0 to a process-substitution file descriptor such as /dev/fd/63. That descriptor is a pipe, not a seekable file, and bash has already consumed it. sed -n '2,26p' "$0" then reads nothing, so --help exits 0 with no output.
Embed the usage text instead of re-reading the script.
🐛 Proposed fix
- -h|--help) sed -n '2,26p' "$0" | sed 's/^# \{0,1\}//'; exit 0 ;;
+ -h|--help) usage; exit 0 ;;Add above the argument loop:
usage() {
cat <<'USAGE'
install.sh — set the canary box up in ONE command.
bash <(curl -fsSL .../integration-suite/local/install.sh) ~/secrets.env
Flags:
--now run one canary immediately after installing (foreground)
--no-cron set everything up but do not touch the crontab
--dry-run print what would happen; touch nothing
--at "M H" cron minute and hour (default "17 6")
USAGE
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| -h|--help) sed -n '2,26p' "$0" | sed 's/^# \{0,1\}//'; exit 0 ;; | |
| usage() { | |
| cat <<'USAGE' | |
| install.sh — set the canary box up in ONE command. | |
| bash <(curl -fsSL .../integration-suite/local/install.sh) ~/secrets.env | |
| Flags: | |
| --now run one canary immediately after installing (foreground) | |
| --no-cron set everything up but do not touch the crontab | |
| --dry-run print what would happen; touch nothing | |
| --at "M H" cron minute and hour (default "17 6") | |
| USAGE | |
| } | |
| -h|--help) usage; exit 0 ;; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@integration-suite/local/install.sh` at line 44, Replace the --help branch’s
self-reading sed pipeline with an embedded usage function near the argument
loop, such as usage(), containing the documented invocation and all supported
flags. Have the --help branch call usage and exit successfully, without reading
from $0.
| # Required, no default ON PURPOSE: a baked-in ref would silently keep probing a | ||
| # stale branch after the daemon branch merges to main. The env file states it. | ||
| : "${CANARY_REF:?CANARY_REF missing from --env-file (origin/failproofaid until #632 merges, then origin/main)}" |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
The CANARY_REF error text is stale and contradicts the installer.
The message tells the operator to use origin/failproofaid "until #632 merges". install.sh line 141 now refuses that exact value because the branch merged, and secrets.env.example line 11 ships origin/main. A person who hits this error follows the message and is then rejected by the installer.
🐛 Proposed fix
-: "${CANARY_REF:?CANARY_REF missing from --env-file (origin/failproofaid until `#632` merges, then origin/main)}"
+: "${CANARY_REF:?CANARY_REF missing from --env-file (set CANARY_REF=origin/main in secrets.env)}"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Required, no default ON PURPOSE: a baked-in ref would silently keep probing a | |
| # stale branch after the daemon branch merges to main. The env file states it. | |
| : "${CANARY_REF:?CANARY_REF missing from --env-file (origin/failproofaid until #632 merges, then origin/main)}" | |
| # Required, no default ON PURPOSE: a baked-in ref would silently keep probing a | |
| # stale branch after the daemon branch merges to main. The env file states it. | |
| : "${CANARY_REF:?CANARY_REF missing from --env-file (set CANARY_REF=origin/main in secrets.env)}" |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@integration-suite/local/runner-entrypoint.sh` around lines 53 - 55, Update
the CANARY_REF validation message in the shell entrypoint so it no longer
recommends the stale origin/failproofaid branch or references the obsolete merge
condition; align the guidance with the installer and current secrets.env.example
by directing operators to origin/main.
| # The HOME volume persists across runs, so YESTERDAY's marker survives into | ||
| # today. Clear it EARLY in every mode — before install/wire — because wire() | ||
| # runs vendor CLIs (openclaw onboard fires plugin hooks) that would fail closed | ||
| # against a marker with no daemon up yet. Daemon mode re-sets it after wire. | ||
| bun -e 'const m=await import("/repo/src/hooks/fp-config.ts");m.updateConfig({daemon:{configured:false}})' 2>/dev/null || true |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
The marker clear swallows its own failure, while the marker set does not.
Line 140 ends with 2>/dev/null || true. The comment above it states the clear is required: a marker left by yesterday's daemon-mode run fail-closes the vendor CLI calls that wire() makes. If updateConfig fails — a corrupted config.toml in the persistent volume, a missing /repo/src/hooks/fp-config.ts on an older ref — the marker survives and every probe in an in-process leg denies. The result reads as INCONCLUSIVE with no cause recorded. Line 260 hard-fails for the opposite operation, so the two directions are inconsistent.
Keep the run going, but record the failure.
🛡️ Proposed fix
-bun -e 'const m=await import("/repo/src/hooks/fp-config.ts");m.updateConfig({daemon:{configured:false}})' 2>/dev/null || true
+bun -e 'const m=await import("/repo/src/hooks/fp-config.ts");m.updateConfig({daemon:{configured:false}})' \
+ || echo "⚠️ could not clear the daemon.configured marker — an inherited marker will fail-close wire() and every probe" >&2📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # The HOME volume persists across runs, so YESTERDAY's marker survives into | |
| # today. Clear it EARLY in every mode — before install/wire — because wire() | |
| # runs vendor CLIs (openclaw onboard fires plugin hooks) that would fail closed | |
| # against a marker with no daemon up yet. Daemon mode re-sets it after wire. | |
| bun -e 'const m=await import("/repo/src/hooks/fp-config.ts");m.updateConfig({daemon:{configured:false}})' 2>/dev/null || true | |
| # The HOME volume persists across runs, so YESTERDAY's marker survives into | |
| # today. Clear it EARLY in every mode — before install/wire — because wire() | |
| # runs vendor CLIs (openclaw onboard fires plugin hooks) that would fail closed | |
| # against a marker with no daemon up yet. Daemon mode re-sets it after wire. | |
| bun -e 'const m=await import("/repo/src/hooks/fp-config.ts");m.updateConfig({daemon:{configured:false}})' \ | |
| || echo "⚠️ could not clear the daemon.configured marker — an inherited marker will fail-close wire() and every probe" >&2 |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@integration-suite/probe-cli.sh` around lines 136 - 140, Update the early
marker-clear command near updateConfig so it continues after failure while
recording the failure details instead of suppressing stderr and ignoring the
status. Preserve the existing marker reset behavior and align its failure
reporting with the hard-failing marker-set path around line 260.
| the workflow triggers on `workflow_dispatch` **only**, so fork PRs can never | ||
| reach them. (The canary box keeps its own copy of the same variables in | ||
| `~/.config/failproofai-canary/secrets.env`, chmod 600 — updating one does not | ||
| update the other.) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the box secrets path.
Line 188 states the box keeps its variables in ~/.config/failproofai-canary/secrets.env. install.sh installs the file at $WORK/secrets.env, where WORK defaults to $HOME/fp-canary (lines 30 and 101), and line 49 of this README already says ~/fp-canary. An operator who follows line 188 edits a file no run ever reads.
🐛 Proposed fix
-reach them. (The canary box keeps its own copy of the same variables in
-`~/.config/failproofai-canary/secrets.env`, chmod 600 — updating one does not
-update the other.)
+reach them. (The canary box keeps its own copy of the same variables in
+`~/fp-canary/secrets.env`, chmod 600 — updating one does not
+update the other.)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| the workflow triggers on `workflow_dispatch` **only**, so fork PRs can never | |
| reach them. (The canary box keeps its own copy of the same variables in | |
| `~/.config/failproofai-canary/secrets.env`, chmod 600 — updating one does not | |
| update the other.) | |
| the workflow triggers on `workflow_dispatch` **only**, so fork PRs can never | |
| reach them. (The canary box keeps its own copy of the same variables in | |
| `~/fp-canary/secrets.env`, chmod 600 — updating one does not | |
| update the other.) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@integration-suite/README.md` around lines 186 - 189, Update the canary
secrets path in the README text to `~/fp-canary/secrets.env`, matching
install.sh’s `$WORK/secrets.env` location and the existing `~/fp-canary`
documentation; leave the surrounding workflow and permissions guidance
unchanged.
2 participants
Supersedes #656 — same four commits, cherry-picked onto current
main(thatbranch was based on the old
failproofaidline, so its history replayed ~160commits
mainalready has), plus a one-command installer.Why
The daily integration suite runs on GitHub Actions. Runner minutes were its
entire cost — the LLM spend is identical wherever it runs — so it moves to a box
whose whole contract is Docker + one cron line + one env file.
Setting it up used to be four commands. Three fail silently for a day.
That is the wrong property for the thing whose job is noticing silent failures:
host's Docker through the socket, so paths under it are used both for
in-container file ops and as sibling-container
-vsources the hostdaemon resolves. Get it wrong and the sandbox mounts nothing.
CANARY_REFleft at the shippedorigin/failproofaid. That branch mergedin [failproofaid] Split failproofai into a CLI + Rust background daemon #632, so the box tests a frozen tree forever and never says so.
reports nowhere — worse than no canary, because it looks like coverage.
integration-suite/local/install.shrefuses all three at install time, infront of a person, rather than at 06:17 tomorrow in front of nobody. The
webhook is required for that reason, not because the run needs it.
Whoever holds the credentials fills in
secrets.envand sends it; the personwith the machine runs one line and needs to know nothing about the canary.
Design points worth reviewing:
<repo>#<ref>:<subdir>as a buildcontext, so the image builds straight from the git URL. The runner re-clones
the repo itself each night anyway, so a checkout here would only go stale.
# failproofai-canarymarker and a re-install strips any previous line first,so running the installer twice upgrades the schedule instead of scheduling two
jobs. The marker is a comment rather than a match on the command, because the
command changes.
secrets.env.example), sothe installer's check is a backstop rather than the only thing between a wrong
default and a year of green runs against a dead ref.
--dry-runseparates CHECKED from CHANGED. The preflight genuinely runs ina dry run so it keeps its ✓; mutations print "would". A script that reports
success for work it did not do is the same defect class this canary exists to
find.
Testing
Not just "it parses" — the mechanisms were exercised:
docker buildfrom the git URL, no clonedocker --versionOK inside✗ docker socket not mounted — add: -v …✗ no work dir found — mount one at an identical pathCANARY_REF=origin/failproofaidorigin/main--dry-runbun run test:runon__tests__/integration-suite/— 49 passbash -non the installerNote
The one-command form only works once this is on
main— the installer curlsinstall.shandsecrets.env.examplefrom the default branch, and neitherexists there yet. Verified against this branch's ref in the meantime.
🤖 Generated with Claude Code
https://claude.ai/code/session_01HEwcerc9jE7ZBkfBiYRbep
Hermes review
e8de2272dc32ebce80ccb20e2092406ecab220741d8f31d926828f3bae215c58f5b35baa44acbff0gpt-5.6-terraSummary
The canary migration and installer are substantially covered, but the README retains obsolete systemd-runner instructions and an obsolete secrets path that can mislead canary operators.
Changes
Validation
Passeddocker run --rm -v /review/input/workspace:/repo:ro oven/bun:latest bash -lc 'bash -n /repo/integration-suite/local/install.sh /repo/integration-suite/local/runner-entrypoint.sh /repo/integration-suite/local/runner-daily.sh /repo/integration-suite/ci-entrypoint.sh /repo/integration-suite/probe-cli.sh /repo/integration-suite/run.sh'— All changed shell scripts passed Bash syntax validation in a nested container. (10s)Findings
No blocking findings.
1 advisory finding
local/run-local.shat line 109, points operators to~/.config/failproofai-canary/secrets.envat lines 187-189, and sayslocal/contains systemd units at line 218. The added local runner instead consists ofinstall.sh,runner-entrypoint.sh, andrunner-daily.sh;install.shdefaults its active env file to$HOME/fp-canary/secrets.env. (integration-suite/README.md:7)Open questions
None.
Policy overrides
None.
Summary by CodeRabbit
New Features
Documentation
Bug Fixes