[beta.13] Extra capture paths per agent CLI, the deployment/version rename, and two silent enforcement gaps - #663
[beta.13] Extra capture paths per agent CLI, the deployment/version rename, and two silent enforcement gaps#663chhhee10 wants to merge 11 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe CLI adds ChangesFlush and interface updates
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@CHANGELOG.md`:
- Line 6: Replace the `#PR` placeholder in the changelog entry with `#663`,
preserving the existing dated, versioned entry and single-line description.
🪄 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: 46e07879-1743-42f6-86a2-69eaf27e55d0
📒 Files selected for processing (2)
CHANGELOG.mdsrc/hooks/configure-wizard.ts
bfd4dc3 to
01d4973
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/hooks/flush-cli.ts (1)
540-560: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winConsume and validate all flush arguments.
flush --wait unexpectedignoresunexpected.flush --timeout 5accepts a timeout that has no effect because--waitis absent. Repeated--timeoutflags silently use only the first value.Parse the argument list in one pass. Reject positional arguments, duplicate flags, and
--timeoutwithout--wait.🤖 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 `@src/hooks/flush-cli.ts` around lines 540 - 560, Update the flush CLI argument parsing in the relevant flow of flush-cli.ts to consume and validate the full argument list in one pass. Reject positional arguments such as unexpected, duplicate --wait or --timeout flags, and any --timeout usage without --wait; preserve the existing valid option behavior while ensuring all supplied arguments are accounted for.
🧹 Nitpick comments (1)
crates/fpai-collect/src/delivery.rs (1)
251-277: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a direct flush-sweep behavior test.
The new branch is not exercised by the current tests.
crates/fpai-collect/tests/delivery.rspasses anAtomicBoolthat remains false. The existingstale_batchestests do not verify thatsweepobserves a request and bypasses both the minimum age and the per-pass limit.Start a sweeper with fresh batches, set the shared flag, and assert that all batches drain without waiting for
SWEEP_MIN_AGE.🤖 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 `@crates/fpai-collect/src/delivery.rs` around lines 251 - 277, Add a delivery sweep test in the existing delivery test suite that starts the sweeper with fresh batches and a shared AtomicBool flush flag, sets the flag while the sweeper is running, and verifies all batches are delivered without waiting for SWEEP_MIN_AGE or being limited by SWEEP_MAX_FILES. Use the existing sweep and delivery test helpers, ensuring the test exercises the flush_now branch directly.
🤖 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 `@__tests__/hooks/tui.test.ts`:
- Around line 143-189: Add coverage to the promptText redraw tests for a failing
validate callback after pressing Enter, including the validation-error display
path. Update the measurement helper to split each ANSI-stripped write on newline
boundaries and measure the widest individual row rather than the complete write,
then assert every rendered row stays within the terminal width.
In `@CHANGELOG.md`:
- Line 6: Update the changelog entry for `failproofai flush --wait` to state
that it waits until the spool drains or the configured timeout expires, while
preserving the existing description of flush behavior.
In `@src/hooks/flush-cli.ts`:
- Around line 109-121: Update the flush flow around isDaemonSupportedPlatform()
to return the same error response before writing flush-request.json when the
platform is unsupported; preserve the existing daemon status handling for
supported platforms. Add a regression test for the flush entry point that mocks
an unsupported platform and verifies an error is returned and no request file is
created.
---
Outside diff comments:
In `@src/hooks/flush-cli.ts`:
- Around line 540-560: Update the flush CLI argument parsing in the relevant
flow of flush-cli.ts to consume and validate the full argument list in one pass.
Reject positional arguments such as unexpected, duplicate --wait or --timeout
flags, and any --timeout usage without --wait; preserve the existing valid
option behavior while ensuring all supplied arguments are accounted for.
---
Nitpick comments:
In `@crates/fpai-collect/src/delivery.rs`:
- Around line 251-277: Add a delivery sweep test in the existing delivery test
suite that starts the sweeper with fresh batches and a shared AtomicBool flush
flag, sets the flag while the sweeper is running, and verifies all batches are
delivered without waiting for SWEEP_MIN_AGE or being limited by SWEEP_MAX_FILES.
Use the existing sweep and delivery test helpers, ensuring the test exercises
the flush_now branch directly.
🪄 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: 0a8b3f07-7bb3-4cd4-b609-d7a175423fde
📒 Files selected for processing (12)
CHANGELOG.md__tests__/hooks/configure-wizard.test.ts__tests__/hooks/flush-cli.test.ts__tests__/hooks/tui.test.tsbin/failproofai.mjscrates/failproofaid/src/main.rscrates/failproofaid/src/paths.rscrates/fpai-collect/src/delivery.rscrates/fpai-collect/tests/delivery.rssrc/hooks/configure-wizard.tssrc/hooks/flush-cli.tssrc/hooks/tui.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@src/hooks/tui.ts`:
- Around line 509-512: Update the internal prompt result handling around
multiSelect and selectOne to recognize the Back result before array or choice
formatting, returning the “back” summary without passing the sentinel to
values.includes or rendering it as a choice. Widen the internal result type to
include Back, and add a direct left-arrow multiSelect test in the existing TUI
hook tests covering successful back navigation.
🪄 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: b45b7acb-e577-43a2-8388-89112d66bb61
📒 Files selected for processing (8)
CHANGELOG.md__tests__/hooks/configure-wizard.test.ts__tests__/hooks/daemon-service.test.ts__tests__/hooks/manager-cloud-listing.test.tssrc/hooks/configure-wizard.tssrc/hooks/daemon-service.tssrc/hooks/manager.tssrc/hooks/tui.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- CHANGELOG.md
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
__tests__/hooks/worker-server.test.ts (1)
247-247: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRename the stale directory variable.
generationDirnow points todeployments/42. Rename it todeploymentDirand update its local uses. This keeps the test aligned with thedeploymentterminology.Proposed rename
- const generationDir = join(managedRoot, "deployments", "42"); + const deploymentDir = join(managedRoot, "deployments", "42");🤖 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__/hooks/worker-server.test.ts` at line 247, Rename the local variable generationDir to deploymentDir in the affected test and update every local reference accordingly, preserving the existing deployments/42 path and test behavior.__tests__/hooks/policy-attribution.test.ts (1)
119-123: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAssert the active deployment in the plain-allow case.
evaluateHookEventrecordscloudDeploymentwhen an active cloud deployment exists, even when no policy decides the event. This test only checks thatcloudVersionis absent. Add an assertion forcloudDeploymentto protect this contract.Proposed assertion
expect(row().policySource).toBeUndefined(); expect(row().cloudVersion).toBeUndefined(); + expect(row().cloudDeployment).toBe(184);🤖 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__/hooks/policy-attribution.test.ts` around lines 119 - 123, Update the plain-allow test around evaluateHookEvent to also assert that row().cloudDeployment matches the active cloud deployment, while preserving the existing undefined assertions for policySource and cloudVersion.
🤖 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 `@__tests__/hooks/builtin-policies.test.ts`:
- Line 2756: Update the command-error mocks in the base-ref and related cases
around opts.baseRefExists, including the referenced cases, to use realistic Git
diagnostics such as “unknown revision or path not in the working tree,”
“ambiguous argument,” or “Needed a single revision” instead of “unknown
version”; ensure the production classifier recognizes those diagnostics if the
tests exercise it.
In `@app/policies/hooks-client.tsx`:
- Around line 399-400: Update the cloud policy detail template in the relevant
render expression to label item.cloudVersion as “version” instead of “rev”,
preserving the existing cloud metadata and policySource branches.
In `@CHANGELOG.md`:
- Line 8: Remove the duplicate “### Features” heading from the changelog while
preserving the cloud-policy entry under the existing Features section for
version 1.0.0-beta.13.
In `@crates/failproofaid/src/cloud_policies.rs`:
- Around line 30-44: Preserve schemaVersion 1 compatibility across the
cloud-policy migration: in crates/failproofaid/src/cloud_policies.rs:30-44,
update DesiredState deserialization to accept legacy generation and revision
fields; in crates/failproofaid/src/cloud_policies.rs:71-81, apply the same
compatibility to the active-manifest deserializer. In
src/hooks/cloud-managed-policies.ts:36-62, normalize legacy schemaVersion 1
fields, including generations/<id> paths, before validation. Add the
corresponding upgrade fixture in
__tests__/hooks/cloud-managed-policies.test.ts:19-28.
---
Nitpick comments:
In `@__tests__/hooks/policy-attribution.test.ts`:
- Around line 119-123: Update the plain-allow test around evaluateHookEvent to
also assert that row().cloudDeployment matches the active cloud deployment,
while preserving the existing undefined assertions for policySource and
cloudVersion.
In `@__tests__/hooks/worker-server.test.ts`:
- Line 247: Rename the local variable generationDir to deploymentDir in the
affected test and update every local reference accordingly, preserving the
existing deployments/42 path and test behavior.
🪄 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: b43d42a3-41d7-46ce-a1d7-bb6c91d01850
📒 Files selected for processing (34)
CHANGELOG.md__tests__/hooks/builtin-policies.test.ts__tests__/hooks/cloud-connect-permissions.test.ts__tests__/hooks/cloud-enrollment-cli.test.ts__tests__/hooks/cloud-enrollment.test.ts__tests__/hooks/cloud-managed-policies.test.ts__tests__/hooks/configure-wizard.test.ts__tests__/hooks/fail-closed-force-decision.test.ts__tests__/hooks/handler.test.ts__tests__/hooks/manager-cloud-listing.test.ts__tests__/hooks/policy-attribution.test.ts__tests__/hooks/session-pause-enforcement.test.ts__tests__/hooks/worker-server.test.ts__tests__/integration-suite/is-error.test.tsapp/policies/hooks-client.tsxcrates/failproofaid/src/cloud_client.rscrates/failproofaid/src/cloud_policies.rscrates/failproofaid/src/main.rscrates/failproofaid/src/paths.rscrates/failproofaid/src/telemetry.rscrates/failproofaid/tests/collector_reload_e2e.rscrates/fpai-collect/src/health.rscrates/fpai-collect/src/sources/hooks/transform.rscrates/fpai-collect/tests/hooks_source.rssrc/hooks/cloud-connection.tssrc/hooks/cloud-enrollment-cli.tssrc/hooks/cloud-enrollment.tssrc/hooks/cloud-managed-policies.tssrc/hooks/custom-hooks-loader.tssrc/hooks/daemon-service.tssrc/hooks/fp-home.tssrc/hooks/handler.tssrc/hooks/hook-activity-store.tssrc/hooks/manager.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- tests/hooks/manager-cloud-listing.test.ts
- tests/hooks/configure-wizard.test.ts
- src/hooks/manager.ts
- crates/failproofaid/src/paths.rs
- src/hooks/daemon-service.ts
- crates/failproofaid/src/main.rs
… a safety net "let's set up your safety net" was the first line of `failproofai config`, and it says nothing a first-time user can act on: it names no tool, describes no step, and leaves them guessing what the next four screens will change on their machine. The wizard installs hook entries across up to twelve agent CLIs and a root-owned system service — an opening line that reaches for a metaphor instead of naming the thing is at odds with what follows. "let's set up failproofai" is what the command actually does. This was the only occurrence in the wizard. The word survives in `src/audit/archetypes.ts` and `src/audit/findings.ts`, which are user-facing audit copy written in a deliberate persona voice — a separate call, left alone here rather than swept up mechanically. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015BDLTPtbQvUE62eCfQrUbf
Reported from a live run-through of `failproofai config`. **The API-key prompt printed one copy of itself per character.** `\r\x1b[2K` erases the row the cursor is on and nothing above it. `API key for <host>` plus the masked value plus the `needs events:add · policies:pull …` hint is past 80 columns before the key is half typed, so the line WRAPPED, the erase reached only its last row, and every keystroke left the previous row on screen. Pasting a 40-character key stacked 40 prompts down the terminal. The prompt now truncates to one physical row. The regression test is red-proven: without the fix it renders 88 columns into an 80-column terminal. **Cloud was offered second and "stay local" was preselected**, which is not what most people running the wizard came to do. The two options are swapped; neither option's copy changed, so staying local is still stated as plainly as it was and is one keystroke away. **"AI assistants" is a word no other surface uses.** The wizard protects agent CLIs — harnesses. Four user-facing strings, including the `12 assistants` count on the closing line. **`--help` did not mention `backfill` at all**, nor any of the `config` cloud flags (`--connect`, `--token`, `--machine-id`, `--machine-label`, `--no-transcripts`, `--disconnect`, `--status`, `--pause`, `--resume`). A flag nobody can discover is a flag that does not exist. **`failproofai flush` is new.** The collector is unhurried by design — swept once older than two minutes, at most 64 per pass, every 60 seconds — pacing that keeps a backlog from stampeding the server and is exactly wrong for somebody watching a dashboard for their own events, where "not delivered yet" and "not working" look identical. `flush` asks the daemon for a pass with no minimum age and no cap; `--wait` blocks until the spool drains so a script can flush and then assert. It hands off rather than doing the work, for the same reason `backfill` does: the uploader's concurrency limiter and in-flight set live in the running daemon, so a second uploader started by the CLI would POST every batch twice. It does NOT hand off the checking — collection off, no credential, daemon stopped are each verified synchronously, because a CLI that prints "requested" while the daemon is down has told the user the opposite of what happened. The daemon side is a flag rather than a channel: the sweeper is rebuilt whenever the collector cycles, and a receiver would go with it, dropping a request already taken off disk. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015BDLTPtbQvUE62eCfQrUbf
…nav, list cloud policies **The daemon install slept 750ms for no reason, twice.** Measured on a real machine: systemctl stop/start/enable/daemon-reload are 3-6ms each, the socket appears at 13ms, and a real hook is answered at 125ms. Then `waitForDaemonRunning` slept a flat `SERVICE_SETTLE_MS` and read the status once at the end — wrong in both directions. Healthy: setup sat out the remaining ~600ms with the answer already in hand, and the repair path does this twice (uninstall, reinstall). Broken: a daemon that died at 100ms was not noticed until 750ms, because nothing looked until the sleep was over. The window is now watched rather than slept through. Leaving `running` fails immediately, and the wait ends early once the daemon accepts a connection — a strictly stronger signal of "did not die at startup" than "still active after an arbitrary sleep", which is all the settle ever established. The connect check, not a hook evaluation: `probeDaemon` runs the end-to-end one moments later and paying twice is the thing being fixed. The timing is now injectable, so the tests assert the SHAPE of the wait against a virtual clock instead of spending it: early exit, startup-death detected on the first poll, full window still held when nothing answers, give-up when the unit never starts. **The wizard could not go back.** Cancel and "go back" were both `null`, so changing an earlier answer meant abandoning setup. `←` on the harness step now returns to the policy step with the previous selection still ticked. `BACK` is a symbol, not a sentinel string: a caller's value type is its own and `selectOne<string>` could legitimately carry "back" as a real choice. It reaches the return type through overloads, so the dozens of existing call sites are unchanged rather than widened to handle a value they can never receive. The policy step deliberately takes no `allowBack` — the scope question before it is often not asked at all, so `←` there would sometimes go nowhere. **Cloud-managed policies were invisible to `failproofai policies`.** They enforce on the machine exactly like the builtins and convention policies the command already lists, so it was answering "what is enforcing here?" with a subset — and the policies an operator pushed to a fleet were the ones the person standing at the machine could not see. Listed read-only, because they are owned by the deployment: `--uninstall` cannot switch one off, and printing them beside toggleable rows without saying so would imply it can. `observe` renders OBS rather than ON — its verdict is discarded, so a row claiming enforcement would be claiming something it deliberately is not doing. An unreadable manifest drops the section rather than breaking the listing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015BDLTPtbQvUE62eCfQrUbf
The daemon half of the AgentEye rename. Both words were product vocabulary nobody outside the codebase used: a customer reads "deployment 7" and "version 3" without a glossary. Renamed through the wire format and the on-disk manifest, not just the labels — `DesiredState`, `DesiredPolicy`, `ActiveDeployment` (was `ActiveGeneration`), `ActivePolicy`, and every path and identifier behind them. NO COMPATIBILITY ALIAS, deliberately. Nothing is deployed against the old names, so a `serde(alias)` would be dead code guarding a case that cannot occur. The consequence is that this must ship with the server change: a daemon reading `generation` from a server emitting `deployment` parses nothing and stops reconciling, and `ActiveDeployment` carries `deny_unknown_fields`, so it fails hard rather than degrading. Two boundary bugs the first sweep missed, both silent-failure class: 1. Snake_case identifiers — `generation_dir`, `generation_path`, `generation_valid` — survived `\bgeneration\b`, because `_` is a word character so there is no boundary to match. Four Rust files. 2. `cloudRevision` / `cloudGeneration` are `#[serde(rename)]` keys in the hook-activity JSONL that the TypeScript writes and the Rust collector reads. The TS side was renamed and the reader was not, which would have dropped cloud-policy attribution from every hook decision — no error, just an empty column. Verified: 218 Rust tests, 3217 TypeScript tests, clippy and fmt clean. The one TypeScript failure (`refuses to half-install when it cannot elevate`) is proven pre-existing — it fails identically with this entire change stashed, because this machine has passwordless sudo and the test asserts `canElevate()` is false. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015BDLTPtbQvUE62eCfQrUbf
…ng an unhandled rejection CI went red on a run where every test passed: `190 files passed, 3218 tests passed`, then `Errors 2 errors` and exit 1. `readline.emitKeypressEvents()` calls `listenerCount` on the stream it is given, and the mock in the new promptText tests did not have it. The call threw, and because the test deliberately does not await the prompt — `void promptText(...)`, since the prompt only resolves on a keypress — the throw surfaced as an UNHANDLED REJECTION rather than a failing assertion. That is why it passed locally and failed in CI: an unhandled rejection is a warning in a terminal and a job failure in the pipeline. The tests were testing the right thing; the mock was incomplete. Adds `listenerCount`, `once`, `off`, `emit` and `addListener` — the surface `emitKeypressEvents` actually touches. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015BDLTPtbQvUE62eCfQrUbf
aec23ce to
0e3045b
Compare
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
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
The v1 desired-state payload named its fields `generation` and `revision`. After the rename it carried neither, at the same version number — same endpoint, same version, different shape, which is the one thing a schema version exists to prevent. AgentEye#559 now emits 2; this accepts both. 1 is accepted ONLY for files already on disk. A machine that ran an earlier beta has a `desired-state.json` and an `active.json` written at version 1, and both structs carry `deny_unknown_fields` — so refusing that version would leave the daemon unable to read its own persisted state, silently not enforcing cloud policy until a poll re-materialised everything. That same asymmetry decides where the field aliases live. They stay on the persisted `ActiveDeployment`/`ActivePolicy`, whose bytes may have been written by an older daemon than the one now reading them. They are REMOVED from the wire `DesiredState`/`DesiredPolicy`, because no server can emit the old spelling — an alias there is dead code, and a silently-tolerated stale field is exactly how two sides drift back apart. A test pins each half, including that the wire now REFUSES the old spelling rather than quietly taking it. The TypeScript hook reader had the same constant and accepted only 1. That is the worst-shaped version of this bug: the daemon reconciles, writes a correct `active.json`, reports "deployment 1 active" — and the hook path alone refuses it, so cloud policy stops being enforced while every other signal says the machine is healthy. Reproduced exactly that way while syncing against a live #559 server, before this fix. Verified end to end against the real stack: publish a policy, deploy it, the daemon pulls schemaVersion 2, verifies every digest, activates, and a matching tool call is denied with that policy's reason. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CCSFM55BcUEAHECabpeSr4
Found by an adversarial audit of the rename, and confirmed by running the real reader against the byte-exact fixture the Rust test uses. `ACCEPTED_ACTIVE_SCHEMA_VERSIONS = [1, 2]` says 1 is accepted "for files a pre-rename beta daemon left behind". The parser then read only `deployment` and `version` — a genuine v1 file spells them `generation` and `revision` — so the acceptance was unreachable and every such file threw `active manifest deployment is invalid`. The Rust reader of this same file already handles it, via `#[serde(alias = "generation")]` and `#[serde(alias = "revision")]`. This is the TypeScript half of that hand-maintained pair, and it was the half missing. The failure is the exact shape the module header warns about: the daemon reconciles, writes a correct `active.json`, reports the deployment active — and the hook path alone refuses it, so cloud-managed policy silently stops being enforced while builtins keep working and every other signal says healthy. Recovery needs a successful poll to rewrite the file, so an offline machine or a revoked credential leaves the gap open indefinitely. The tests were hiding it. Every `schemaVersion: 1` fixture stamped version 1 on POST-rename field names — a combination no writer has ever produced — so v1 looked covered while the one migration path it exists for was broken. The fixture is now honestly labelled 2, and two new tests use the real v1 spelling. Red-proven: without the fallback the first one fails with that exact error. Also from the same audit: - `crates/CLOUD_POLICIES.md`, the daemon's own contract document, was never renamed. Its desired-state example is a payload the daemon now rejects. - `__tests__/e2e/layout/cloud-pairing.sh` — the live cross-repo harness — still spoke the pre-rename wire in nine places: it read `revision` off the publish response, sent `revision` in two deploy bodies, read `generation` off the deploy response, and asserted the ingested event carried `cloud_generation`. It would have failed against the renamed server, and its assertions proved nothing about the new contract. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015BDLTPtbQvUE62eCfQrUbf
c8fc648 to
39671df
Compare
Ten commits, and the original title (a one-line wizard copy change) is now the smallest thing in here. Retitled to match what it actually contains.
Lands with FailproofAI/agenteye#559. The two are one change split across two repos: #559 renames the cloud-policy vocabulary through AgentEye's schema, server and dashboard; this renames it through the daemon and moves the wire contract to
schemaVersion: 2. Verified together end to end — see Cross-repo verification below.1. Setup flow —
failproofai flush, back-navigation, five defects (fd9cf998,d0132a8e,15363627)failproofai flushdelivers what is already spooled instead of waiting out the sweeper's interval, with--waitblocking until the spool drains or its timeout expires. The install stopped waiting on a blind sleep; the wizard gained ← back-navigation; cloud-managed policies list with their deployment, version and status.2. The rename — a generation is a deployment, a revision is a version (
921b7f60)Words the product used nowhere else. Renamed through the daemon, its persisted state and every test name.
3. Capture sessions from more than one location per agent CLI (
3ecab827)Every source watched exactly the place its own installer chose. That is right for one laptop and wrong for a second profile, a mounted team share, a container's home bind-mounted beside the host's, or an agent an operator relocated — all of which hold real sessions that nothing collected.
Writes
[collector.sources.<harness>] extra_pathsinconfig.toml;FAILPROOFAI_<HARNESS>_EXTRA_PATHSoverrides for containers. 12 harnesses, 13 sources —claudecovers subagent transcripts, which share a root and would otherwise be dropped silently under an added path. Grammar, folder-name label fallback and<label>-<agentId>namespacing match AgentEye's own collector rather than inventing a second answer.Three things are structural, not cosmetic:
rootsis walked by one task with oneParams, and the label lives inParams— a second root there inherits the default's agent idroot_presentalternates, destroying the "absent root versus idle source" distinction. Hermes hit exactly this with per-profile databasesThe
<label>-prefix is applied inSpoolWriter::push— the one point every event passes through — and not in either engine. A SQLite format takesparams.agent_idonly as a fallback and derives the real id from the row (devin::agent_id→devin-<project>, which never consults the fallback for a session with a working directory, i.e. all of them). Prefixing in the engine would namespace only the sessions that failed to derive an id; a test fails on precisely that mistake.No restart and no sudo: the collector manager already re-reads
config.tomlon an interval and cycles on anyCollectorConfigchange, andsourcesrides that by living inside the compared value (79be966b).4. Upgrades silently emptied the policy set (
9c498e5b)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. 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.Measured in a container, identical starting state, two upgrade targets:
@latest→ published@next@latest→ this branchenabledPoliciesHook entries stayed installed in every agent CLI and kept firing — against an empty policy set. An allowlist, not a copy: layout 1's file also held a
collectorblock in camelCase, and layout 2 moved those to snake_case inconfig.toml, so carrying it would add a key nothing reads that looks preserved and behaves absent. Read-before/write-after, because its source and its destination's parent are both on the reset list.5. Review fixes (
18eaebb5) — 9 CodeRabbit threads, two of them real bugsBACKis a symbol the shared handler injects, andmultiSelect's summary callsvalues.includes(...)→TypeError, thrown insidefinishbeforeresolve(). The promise never settled and input stopped responding.selectOnerenderedSymbol(failproofai.back)as the user's answer. Handled once incollapse().deny_unknown_fields, so an upgraded daemon failed to parse its ownactive.jsonon three counts at once.flushreported success on platforms with no daemon; mocked git errors saidunknown version, a string git never produces;#PRplaceholders,--waitwording,rev→versionin the activity detail.6.
schemaVersion: 2(76acc68f)The v1 payload named its fields
generationandrevision; after the rename it carried neither, at the same version number. #559 now emits 2, and this accepts both — 1 only for files already on disk, never from a server, because a machine that ran an earlier beta has adesired-state.jsonandactive.jsonat version 1 anddeny_unknown_fieldswould make them unreadable.That asymmetry decides where the aliases live: kept on the persisted
ActiveDeployment/ActivePolicy(bytes possibly written by an older daemon), removed from the wireDesiredState/DesiredPolicy(no server can emit the old spelling, so an alias is dead code and a silently-tolerated stale field is how two sides drift apart). Tests pin each half, including that the wire now refuses the old spelling.The TypeScript hook reader had the same constant and accepted only 1 — the worst shape of this bug: the daemon reconciles, writes a correct
active.json, logs "deployment 1 active", and the hook path alone refuses it, so cloud policy stops being enforced while every other signal reads healthy. Reproduced exactly that way against a live #559 server before fixing it.Cross-repo verification
Against a real local stack (Postgres + ClickHouse + Redis + server + dashboard) running #559's branch, with a daemon built from this branch:
/enforcement/v1/desired-stateemitsdeployment/versionatschemaVersion: 2active.jsonat v2, and a matching tool call is denied with that policy's reasonMulti-path, five harnesses across both engines, default and extra path:
claude-work-defaultshare-claude-work-sharegoose-work-goosealt-goose-work-gooseopencode-work-ocoalt-opencode-work-ocoopenclaw-mainalt-openclaw-mainpi-work-pialt-pi-work-piclaude, goose and opencode were driven with a real LLM. OpenClaw is the strongest case — both sessions derive the same id, so only the label separates them. A path added to a running daemon was captured within one poll interval (task count 21 → 23), and its first transcript reached the server.
Tests
8 engine tests through the real
filetail/sqlitepollloops; 16 grammar unit tests; a cross-language contract test parsing the exactconfig.tomlbytes the TypeScript CLI emits; a test readingHARNESS_KEYSout ofmain.rsso the two hand-maintained lists cannot drift; 7 migration tests; 3 pre-rename compatibility tests; back-navigation and validation-row tests; and an e2e case driving the real binary against a hand-edited config. Two were explicitly proved non-vacuous by reverting the fix and watching them fail.cargo test --workspace624 passing, clippy + fmt clean;tsc --noEmitclean;bun run lint0 errors;bun run test:run3261 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 machine it ran on has a real one installed. It passes in CI, and fails identically on this branch with every change stashed.🤖 Generated with Claude Code
https://claude.ai/code/session_01CCSFM55BcUEAHECabpeSr4