[failproofaid] Capture sessions from more than one location per agent CLI, and stop upgrades emptying the policy set - #667
Merged
Conversation
Every source watched exactly the place its own installer puts it — ~/.claude/projects, ~/.hermes/state.db. That is right for one machine and wrong for every other arrangement: a second profile, a mounted team share, a container's home beside the host's, an agent an operator relocated. Those hold real sessions and nothing collected them. `failproofai harness add-path <harness> [<label>=]<path>` (plus list / remove-path) writes [collector.sources.<harness>] extra_paths in config.toml; FAILPROOFAI_<HARNESS>_EXTRA_PATHS overrides it for containers. All twelve harnesses — thirteen sources, since `claude` carries its subagent transcripts, which share a root and would otherwise be silently dropped under an added path. Grammar, folder-name label fallback and <label>-<agentId> namespacing match AgentEye's collector (--openclaw-extra-path, --hermes-extra-path) rather than inventing a second answer to what an extra path is. Three things are structural rather than incidental: * Each labelled path is its OWN task. `roots` is walked by one task with one Params, and the label lives in Params — so a second root added there would be captured under the same agent id as the default. Two copies of one project derive the SAME id (from the cwd inside the transcript, identical in both), which is precisely what the label exists to separate. * Each gets its own cursor directory. The store writes its whole map atomically, so two instances sharing one clobber each other's watermark and both re-read from zero after every restart. * Each gets its own health key, or they overwrite each other's record and root_present alternates — destroying the "absent root versus idle source" distinction that file exists to draw. Hermes hit exactly this with its per-profile databases; that is the shape every source now takes. The label is applied in SpoolWriter::push — the one point every event from every source passes through, where machine_id and user are already stamped for the same reason — and NOT in either engine. A SQLite format is handed params.agent_id only as a fallback and derives the real id from the row: devin::agent_id returns devin-<project> and never consults the fallback for a session that has a working directory, which is all of them. Prefixing there would namespace only the sessions that failed to derive an id. A path overlapping one already captured is refused at startup rather than collected twice under two ids, and an unrecognised [collector.sources.*] table is reported instead of silently capturing nothing. A machine with nothing configured is unchanged, byte for byte, including its config.toml. Tests: 8 engine tests through the real filetail and sqlitepoll loops (multi-path steady state, backfill from a newly added path, cursor resume, and a default-only regression); 16 grammar unit tests; a cross-language contract test parsing the exact config.toml bytes the TypeScript CLI emits, because that seam has nothing else holding it together; and a test that reads HARNESS_KEYS out of main.rs so the two hand-maintained copies of that list cannot drift. Found and fixed while testing: `harness list` returned "No extra capture paths configured" before it checked for unknown tables, so the typo warning was unreachable in exactly the case it exists for. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CCSFM55BcUEAHECabpeSr4
Layout 1 kept it at ~/.failproofai/policies-config.json; layout 2 keeps it at policies/local-policies/policies-config.json — and BOTH were on the reset list. So moving between layouts discarded every builtin the user had enabled, every explicit customPoliciesPaths entry and every policyParams value. The reset runs unattended from checkLayoutForCli on the first command after an upgrade, and the machine still read as configured afterwards (isConfigured() is a union that sees the agent CLIs' untouched settings files) — so the wizard never re-asked and hooks kept firing against a policy set that had quietly become the default one. Same silent enforcement gap migrateConventionPolicies() and migrateHookActivity() already close, by a third route. Deliberately narrow, and inside the standing wipe-and-re-setup decision: everything derived (cursors, spool, health, audit cache) still goes and is rebuilt. What moves is only what a person typed and nothing regenerates — the same test migrateHookActivity() applies to the decision log. An ALLOWLIST rather than a copy. Layout 1's file also carried a `collector` block in camelCase; layout 2 moved those settings to [collector] in config.toml in snake_case, where fpai-collect's Settings deserializes them. Carrying it would put a key into the new file that nothing reads — looking preserved and behaving absent, which is the exact bug that Settings' own doc comment records. Two phases, because the source AND the destination's parent are both on the reset list: read before the deletions, write after. Writing first would have the reset delete the carry moments after it happened — what the note on hookActivityDir() in resettablePaths() records happening once already. A reset FROM the current layout no longer clears local-policies/. That is not a layout migration, and there it would discard a current, valid selection, including one just carried. The single production caller always passes a DETECTED stale layout, so this only changes the forced same-layout case. An existing layout-2 config is never overwritten by the layout-1 file beside it — a stale file winning would undo configuration rather than preserve it. The reset still clears a stale layout-2 config on a layout migration, which the pre-existing "still clears the machine-owned children of policies/" test pins and which this change leaves intact. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CCSFM55BcUEAHECabpeSr4
`harness add-path` told the user to run
`sudo systemctl restart failproofaid@$USER`. That was wrong twice over.
The daemon's collector manager already re-reads config.toml on an interval
and cycles the collector whenever the resolved CollectorConfig differs from
the one the live generation was built from — the same path that picks up
`--connect`, a stream being switched off and a verbosity change. `sources`
lives inside `Settings`, which is part of that compared value, so extra
paths ride it for free.
Verified live against a running daemon: adding a path logged "collector
configuration changed; cycling the collector", took the task count 21 -> 23
(claude's extra path adds two, since the main and subagent formats share a
root), and the first transcript written under the new path reached the
server.
So the restart was unnecessary — and asking for root from a command that
writes one file in the user's own home and needs none is the worse half of
the mistake.
Two tests, because nothing registers `sources` with the reload machinery
explicitly and moving it anywhere resolved later breaks the promise
SILENTLY: the CLI keeps reporting success, the config keeps parsing, and the
daemon just never captures the path until an unrelated restart.
- collector_config_change_cycles_the_collector (unit) pins that adding an
extra path changes the compared value at all.
- an_extra_capture_path_added_by_hand_is_registered_without_a_restart
(e2e) drives the REAL binary against a hand-edited config, alongside the
credential-rotation cases already there and for the same stated reason —
the property is "an edit somebody else made is noticed", and a fleet
tool or a sed is a legitimate way to make it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CCSFM55BcUEAHECabpeSr4
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
SiddarthAA
added a commit
that referenced
this pull request
Aug 7, 2026
Nine findings; eight fixed, one already handled during the #667 merge. **Back navigation was broken, and one half of it hung the wizard.** `BACK` is a symbol the shared key handler injects on ←, so it is not a value of any prompt's own result type — and both callers mishandled it differently. `multiSelect`'s summary calls `values.includes(...)`, which throws TypeError on a symbol, and it throws INSIDE `finish` before `resolve()`: pressing ← never settled the promise and the wizard stopped responding to input at all. `selectOne` fell through to `String(value)` and rendered the literal text `Symbol(failproofai.back)` as the user's answer. Handled centrally in `collapse()` rather than in each `summaryFor`, so a prompt never has to know about a symbol it did not declare. Verified non-vacuous: reverting the fix reproduces `TypeError: values.includes is not a function`. **Pre-rename cloud-policy state on disk became unreadable.** The generation→deployment / revision→version rename left `ActiveDeployment` and `ActivePolicy` carrying `deny_unknown_fields`, so an upgraded daemon failed to parse its OWN `active.json` on three counts at once — `generation` unrecognised, `deployment` missing, and the same again per policy. The machine would silently lose the deployment it was enforcing until a poll succeeded, which on a fail-closed machine is the gap this subsystem exists to close. `serde(alias)` on all four fields, covering `desired-state.json` too since it is both received and persisted. The regression test uses a byte-exact `active.json` captured from a live pre-rename daemon; without the aliases two of its three cases fail. **`failproofai flush` reported success on unsupported platforms.** It skipped the daemon check, wrote `flush-request.json` and exited 0 — but nothing on Windows ever reads it, so the only symptom is data that never arrives. Now refuses before writing, matching what `failproofai config` on this same branch already does rather than the two commands disagreeing about whether the machine has a daemon. **The rename reached mocked git output.** Four `builtin-policies` mocks threw `unknown version`, which no git ever produces; restored to the real `ambiguous argument: unknown revision or path not in the working tree`. Production classifies on none of it, hence minor — but the string was invented by a sweep. Also: `#PR` placeholders resolved in the 1.0.0-beta.13 section only (the 72 in older released sections are left alone), with the two entries from #667 attributed there rather than to #663; `--wait` documented as waiting until the spool drains OR its timeout expires, which is what `runFlushCommand` does; the activity detail says `version` rather than `rev`, matching the rename. Tests: back navigation for both prompt kinds plus an inert-← case, and validation-error row widths for `promptText` — measured per WRITE and per newline, since each write is one redraw and only the error path adds a row. The duplicate `### Features` heading was already collapsed while merging #667. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CCSFM55BcUEAHECabpeSr4
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Targets #663 (
chore/wizard-copy-no-safety-net), notmain, so it lands on top of the beta.13 work already accumulating there.What this adds
1. Extra capture paths per harness. Every source watched exactly the place its own installer puts it —
~/.claude/projects,~/.hermes/state.db. That is right for one machine and wrong for every other arrangement: a second profile, a mounted team share, a container's home beside the host's, an agent an operator relocated. Those hold real sessions and nothing collected them.Writes
[collector.sources.<harness>] extra_pathsinconfig.toml;FAILPROOFAI_<HARNESS>_EXTRA_PATHSoverrides it for containers. All twelve harnesses — thirteen sources, sinceclaudecarries its subagent transcripts, which share a root and would otherwise be silently dropped under an added path. Grammar, folder-name label fallback and<label>-<agentId>namespacing match AgentEye's collector (--openclaw-extra-path,--hermes-extra-path) rather than inventing a second answer to what an extra path is.Three things are structural rather than incidental:
rootsis walked by one task with oneParams, and the label lives there — so a second root added there would be captured under the same agent id as the default. Two copies of one project derive the same id (from thecwdinside the transcript, identical in both), which is precisely what the label exists to separate.root_presentalternates — destroying the "absent root versus idle source" distinction that file exists to draw. Hermes hit exactly this with its per-profile databases; that is the shape every source now takes.The label is applied in
SpoolWriter::push— the one point every event from every source passes through, wheremachine_idanduserare already stamped for the same reason — and not in either engine. A SQLite format is handedparams.agent_idonly as a fallback and derives the real id from the row:devin::agent_idreturnsdevin-<project>and never consults the fallback for a session that has a working directory, which is all of them. Prefixing there would namespace only the sessions that failed to derive an id. There is a test that fails on exactly that mistake.No restart and no sudo: the collector manager already re-reads
config.tomlon an interval and cycles the collector whenever the resolvedCollectorConfigchanges, andsourcesrides that by living inside the compared value.2. A silent enforcement gap on upgrade, fixed. Layout 1 kept the user's policy selection at
~/.failproofai/policies-config.json; layout 2 keeps it atpolicies/local-policies/policies-config.json— and both were on the reset list. So an upgrade discarded every builtin the user had enabled, every explicitcustomPoliciesPathsentry and everypolicyParamsvalue. The reset runs unattended on the first command after an upgrade, and the machine still read as configured afterwards (isConfigured()is a union that sees the agent CLIs' untouched settings files), so the wizard never re-asked and hooks kept firing against a policy set that had quietly become the default one.Measured in a container, identical starting state, two upgrade targets:
@latest→ published@next(beta.12)@latest→ this branchenabledPoliciesThe carry is an allowlist, not a copy: layout 1's file also held a
collectorblock in camelCase, and layout 2 moved those to[collector]inconfig.tomlin snake_case wherefpai-collect'sSettingsdeserializes them — carrying it would put a key into the new file that nothing reads, looking preserved and behaving absent. It runs read-before/write-after because its source and its destination's parent are both on the reset list.Verification
End to end against a real local stack (Postgres + ClickHouse + Redis + server + dashboard), daemon built from this branch, 21 collector tasks:
claude-work-defaultshare-claude-work-shareopenclaw-mainalt-openclaw-mainClaude Code driven live on both paths. OpenClaw is the stronger case — both sessions derive the same id, so only the label separates them. Health keys came out distinct (
claude,claude:share,claude-subagent,claude-subagent:share).Two-way channel confirmed: policy authored via
POST /enforcement/policies→ deployed → daemon pulled/enforcement/v1/desired-state→ SHA-256 verified → activated → a real hook denied → the decision shipped back ashook_triggered/hook_completed.Restart-free pickup verified live against a running daemon: task count 21 → 23 within one poll interval, and the first transcript under the new path reached the server.
Tests: 8 engine tests through the real
filetail/sqlitepollloops (multi-path steady state, backfill from a newly added path, cursor resume, default-only regression); 16 grammar unit tests; a cross-language contract test parsing the exactconfig.tomlbytes the TypeScript CLI emits, because that seam has nothing else holding it together; a test that readsHARNESS_KEYSout ofmain.rsso the two hand-maintained copies cannot drift; 7 migration tests; and an e2e case on the real binary against a hand-edited config, alongside the credential-rotation cases already in that file.cargo test --workspace620 passing, clippy and fmt clean,tsc --noEmitclean,bun run lint0 errors,bun run test:run3255 passing.One pre-existing failure, not from this branch:
daemon-service.test.ts > refuses to half-install when it cannot elevateasserts/etc/systemd/system/failproofaid@<user>.serviceis absent, and the dev box it ran on has a real one installed. Verified it fails identically on #663's branch untouched.Notes for review
harness listreturned "No extra capture paths configured" before it checked for unknown tables, so the typo warning was unreachable in exactly the case it existed for. Fixed.TaskSpec.nameis nowStringrather than&'static str— one source can have several live instances and their names are built at runtime (claude:work). Only ever used for logging, never as a key.### Featuresheading in [beta.13] Extra capture paths per agent CLI, the deployment/version rename, and two silent enforcement gaps #663's beta.13 CHANGELOG section while resolving the merge — two same-level headings under one version looked like a slip. Happy to drop that hunk if it was deliberate.failproofaid's collector has no page in either doc set (enterprise-docs/covers only its enforcement half;failproofai backfillshipped the same way in beta.10).harness --helpcarries the full contract; the broader gap is worth its own piece of work.🤖 Generated with Claude Code
https://claude.ai/code/session_01CCSFM55BcUEAHECabpeSr4