[desgin-docs] Phase 1: design set, implementation plan, and Stage 0 + Stage 1 of failproofaid - #630
Merged
Merged
Conversation
Grounded in a full survey of the current code, not the design docs alone. The sequencing is built around one idea: the existing TypeScript implementation is not replaced, it is promoted to oracle. It stays in the tree, keeps its tests, answers when the daemon cannot, and is what every new implementation is diffed against byte-for-byte. Also lands the per-user agent section in 03-daemon-architecture.md, which closes the question of how a daemon running as _failproofai reads session transcripts under 0700 homes: it does not. Capture and user-context evaluation both run in one per-user agent at the requesting UID, because both want the same thing and residency is already required by the enforcement deadline. Granting the service account ACLs into every enrolled user's home was considered and rejected — a chmod by the harness silently caps the ACL mask, the SQLite sources need write rather than read, and it would make a daemon compromise a read of every developer's transcripts while buying no tamper-resistance. Three decisions settled: - The sealed tier runs QuickJS-ng, ~1 MB against V8's +30-45 MB on each of four tarballs npx downloads, with deny-by-default structural rather than a syscall filter. Gated by a Stage-0 spike against the real regex corpus. - The 39 builtins are not ported to Rust. The boundary is the process, UID and absent bindings, not the language; porting buys no security, does nothing for user-authored policies, and the regex crate cannot express extractAbsolutePaths' lookbehind. - Protected policy enablement moves to a root-owned machine.json. It currently lives in a user-writable file, so an agent deletes block-sudo from a JSON array and the unforgeable verdict never runs — the tamper-proof claim was not true as written. The survey turned up four live bugs Stage 0 fixes: prepare is the only thing populating dist/ before npm publish and publish.yml has no build step, so removing it alone ships an empty package; prune-standalone.mjs prunes "design-docs" while the directory is desgin-docs; every CI cache key hashes a bun.lockb that does not exist, so the bun cache has never invalidated; and the packages/wrapper version check has no-opped since the initial import. It also found the least visible failure mode in the project: all 39 builtins share a module importing child_process, so import-graph tier derivation would route every one of them to user-context and leave the sealed tier empty — an architecture that looks implemented and delivers no verdict integrity. Verification is seven layers ending in a full-stack Docker gate: a contract-faithful events:add stub plus a debian:12 machine running systemd as PID 1 with node and npm only, installed through the real npx setup. Twelve assertions cover install, enforcement across 12 CLIs, both tiers, the no-agent path, protected-vs-mutable, capture through spool to delivery, crash durability with strace-verified fsync ordering, dashboard access control, catalog rollback and uninstall. Two legs must never be skipped: the no-init preflight refusal, and tamper refusing before anything is written under /opt. Six design-doc amendments are recorded. macOS codesigning and notarization is the most urgent — absent from all eight documents, and an unsigned LaunchDaemon is killed on macOS 15, which blocks two of the four targets. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XjFAYdzWkGUPE2sMWWBC6E
|
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 |
The enforcement plane now has a working privileged evaluator behind a Unix socket, off unless FAILPROOFAI_DAEMON_MODE is set. Stage 0's four prerequisite refactors come first, and P1 is the one that mattered most. Execution tiers are derived from a policy's resolved import graph, and all 39 builtins shared one module importing child_process — so derivation would have routed every one of them to user-context and left the sealed tier empty. An architecture that looks implemented and delivers no verdict integrity. The 39 now split 32/7 by capability, and a test walks the real transitive graph rather than trusting the file layout. That test caught itself. Its specifier regex used a newline-excluding character class, which matches no multi-line import in this codebase, so the walk stopped at the entry file and the suite went green while payload-only transitively imported node:fs. Found by injecting node:fs and watching nothing fail. It now asserts the exact module set it visited, so "the walk found nothing" is a failure rather than a pass. P2 threads home and projectDir through the request. The daemon evaluates on behalf of another UID, so its own homedir belongs to the service account — and isAgentInternalPath and block-read-outside-cwd both WIDEN the allow set, which is the direction that does not announce itself. P3 splits evaluatePolicies into evaluateVerdicts + encodeResponse, moving the twelve-vendor response matrix character-identically rather than rewriting it. P4 adds the canonical request envelope and bounds resolveCodexMode's unbounded transcript scan to a 256 KiB head window: measured, first turn_context at line 8 in every real transcript sampled, 3.07ms to 0.54ms on a 2.7 MB file. The sealed runtime bundles the 32 payload-only builtins, the registry, and the full response encoder into one file evaluated inside QuickJS-ng with no bindings registered. node:path is replaced by a port proven equivalent to node:path.posix differentially over 8,000+ generated cases; the host modules by stubs that throw a named capability; hook-logger, hook-telemetry and telemetry-id by inert no-ops, which removed a fetch to PostHog from a tier the design requires to perform no unbounded I/O. Three Rust crates. fpai-ipc carries framing (1 MiB cap validated before allocation, asserted with a counting global allocator), the envelope, peer credentials, and the combination lattice — 70,656 generated property cases proving that adding any number of user_context results never lowers a sealed deny, which is the formal statement of the entire two-tier argument. fpai-canon embeds the generated canonicalization tables. failproofaid is the daemon: one warm sealed worker, one enforcement lane, an interrupt handler that makes a runaway policy a deadline miss rather than a hung daemon. home is derived at the socket from getpwuid_r(peer_uid) and a client-supplied one is REJECTED, not overwritten. Overwriting makes the attack a no-op but leaves the protocol looking like it accepts the field, so the next client implementation and the next reviewer both reasonably conclude it is meaningful. cwd, project_dir and env_facts genuinely cannot be derived, so they ride as client-asserted and any decision reading one is reported sealed_unattested. That is the honest version of unforgeable, and it beats a claim that quietly is not true. Verification, in increasing strength: the sealed bundle runs in a node:vm context with no host globals at all; a 5,220-row corpus goes twice through one warm worker, then shuffled, then against a fresh context per row — the last being the assertion that pins the resident worker to per-event-process semantics, since self-consistency alone is satisfied by a worker that is consistently wrong; all 5,220 rows are byte-identical to the legacy evaluator; and the real TypeScript client drives the real Rust daemon, which is the only test that catches two independent implementations of one prose spec disagreeing. Four live CI and packaging bugs fixed, all found by survey rather than invented. Removing the prepare script alone would have published an empty package. prune-standalone pruned "design-docs" while the directory is "desgin-docs", so 17 design-doc files were shipping to npm on every publish — and unpacking the tarball showed Next's tracer was about to ship the new Cargo.toml and rust-toolchain.toml too. Every CI cache key hashed a bun.lockb that does not exist, so the bun cache has never invalidated since the initial import. And the version-consistency check looped over a packages directory that does not exist, passing vacuously the whole time. Plus one real bug the bench harness surfaced: handler.ts armed a 10-second timeout per custom hook and never cleared it. Invisible today only because the bin entry point calls process.exit on return — and about to stop being invisible in the resident worker and the per-user agent, exactly the processes this change introduces. Measured at a 10,088ms p95 in a harness without the hard exit. Tests: 2,920 unit (+450), 330 e2e, 117 Rust. The 15 remaining unit failures are pre-existing jsdom localStorage failures in project-list.test.tsx, unchanged from before this branch. tsc clean, eslint 0 errors, cargo fmt and clippy with -D warnings clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
|
Warning Review the following alerts detected in dependencies. According to your organization's Security Policy, it is recommended to resolve "Warn" alerts. Learn more about Socket for GitHub.
|
NiveditJain
changed the title
…t set Two problems with the previous commit. The bench baseline it committed is not trustworthy. Another agent rebuilt dist/ while the matrix run was in flight, and dist/ is a shared gitignored build output the harness was reading live — 27,345 of 36,888 iterations failed and the custom variant produced zero usable samples. The script wrote the artifact anyway, which is the worse half of the bug: a biased survivor set masquerading as a measurement. The artifacts are withdrawn rather than left in place, because a baseline nobody can trust is worse than no baseline — the whole point of the file is to be the number Stage 4 is judged against. scripts/bench-hook.ts is hardened three ways: it snapshots dist/ into .bench-hook-tmp/ at startup and points FAILPROOFAI_DIST_PATH at the snapshot, so nothing a concurrent build does can reach a run; it aborts non-zero and writes nothing above a 1% failure rate; and it records totalIterations, failedIterations and failureRate in the artifact, so a degraded run is visible in the committed file rather than only in a terminal nobody kept. Second: the daemon hardcodes DEFAULT_SEALED_POLICIES, and nothing checked it against the TypeScript default-enabled set. Hardcoding is deliberate — reading the user's policies-config.json would make the sealed tier's enabled set come from a user-writable file, so an agent could delete block-sudo from a JSON array and the unforgeable verdict would never run. But a second copy drifts, and this one would drift invisibly: the cross-implementation test feeds the same list to both sides, so it compares encoders, not enabled sets. Add a default-enabled builtin in TypeScript without touching server.rs and every existing test still passes while the daemon quietly enforces one policy fewer than the legacy path — a silent allow, on the exact upgrade path this project exists to protect. __tests__/parity/daemon-default-policies.test.ts parses the list out of server.rs and asserts it equals BUILTIN_POLICIES.filter(defaultEnabled), in order, that every entry is sealed-eligible, and that every entry names a policy that exists. Mutation-checked: adding block-rm-rf to the Rust list fails it. The two sets agree today, at 11 policies. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two were severe and reproduced end-to-end against the running daemon. ## The enforcement deadline could not fire The interrupt flag that turns a runaway policy into a deadline miss was only ever set from inside the microtask pump loop — and a policy body is synchronous JavaScript, so the pump does not get another turn until the call returns. There was no watchdog. Any single policy that outran its deadline blocked the one enforcement lane forever. Not hypothetical, and not exotic. CURL_PIPE_SH_RE in block-curl-pipe-sh is /(?:curl|wget)\s.*\|\s*(?:sh|bash|…)\b/ — the .* backtracks quadratically, and "curl ".repeat(n) drives it. Measured against a 200 ms deadline with no watchdog: 40 KB took 7.1 s and 80 KB took 30 s, both returning Ok, never DeadlineExceeded. The frame cap is 1 MiB, so a *legal* request extrapolates past an hour. block-curl-pipe-sh is default-enabled and sealed; protect-env-vars and block-failproofai-commands have the same regex shape and are also default-enabled. Against the live daemon, one 200 KB request left every subsequent client falling back for the rest of the process's life. Now a long-lived watchdog thread arms before entering QuickJS and disarms after. An interrupt unwinds as an ordinary exception, so the two catch sites check the flag and report DeadlineExceeded rather than an evaluation failure — otherwise a merely slow policy would trip its circuit breaker. My own test is why this survived: it armed the interrupt *before* calling eval, which proves the handler works and says nothing about whether anything arms it. The replacement does not pre-arm, and asserts the lane still answers afterward. ## Connection threads parked forever, then the listener died Every connection thread waited on a bare reply_rx.recv(). The client gives up at its own 800 ms budget and falls back, but the thread stayed blocked for an answer nobody would read — one leaked thread per hook event, unbounded. serve() propagates a failed accept or spawn, so the first EMFILE/EAGAIN returned Err out of the listener and main exited: "no daemon answers" escalating into "the daemon is gone". Now recv_timeout, bounded by the request's deadline plus a lane-queue grace. ## The daemon enforced a policy set the user never configured handle_evaluate hardcoded the 11 defaultEnabled builtins and ignored what the client had resolved. Reproduced against this repo's own policies-config.json, which enables 30: rm -rf / daemon: allow legacy: deny (block-rm-rf) cat /etc/passwd daemon: allow legacy: deny (block-read-outside-cwd) 19 of 30 enabled builtins plus 100% of custom and convention policies stopped enforcing the moment the daemon answered. The documented safety net could not fire: the sealed worker computes needsUserContext by partitioning the list it was handed, and a daemon-supplied list is all-sealed by construction, so it was always empty. And since FAILPROOFAI_DAEMON_MODE is read from the hook process's environment — which the governed agent inherits — an agent could have set one variable to downgrade itself to eleven policies. EvaluateHook now carries enabled_policies and the daemon evaluates that. An empty list is refused rather than treated as "enforce nothing" or backfilled with defaults; the first turns a client bug into a silent allow and the second reinstates the defect. handler.ts reads config *before* the daemon call, and skips the daemon outright when custom policies are configured, since those can never be sealed-eligible — enforcing a subset would be worse than not using the daemon at all. The expensive half of the legacy work, loadAllCustomHooks and its per-tool-call temp files, still only runs on the fallback path. DEFAULT_SEALED_POLICIES is deleted rather than kept in sync. Removing the duplication beats testing it. ## The protocol documented a check that did not exist PROTOCOL.md claimed fpai-canon re-derived canonicalization and rejected a mismatch. It never did, and it is not implementable from this envelope — re-deriving needs the raw vendor payload and only the canonicalized one is sent. Believing the claim was worse than not making it: a reviewer would reasonably assume something was catching a hostile client. The doc now says what is true, explains that the exposure is bounded (the client runs as the user whose events these are, and the one field that would be dangerous to accept is the one the daemon derives), and defers the check to Stage 2. The unused fpai-canon dependency is dropped so the crate graph stops implying otherwise. Tests: 2,934 unit, 116 Rust, 16 cross-implementation. The 15 remaining unit failures are the pre-existing jsdom localStorage failures. clippy -D warnings, cargo fmt, tsc and eslint all clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The hardened failure-rate guard added after the first corrupt capture was itself unsound, and the regenerated artifact proved it: `failedIterations: 636` sitting beside `failureRate: 0`, written by a run that exited 0. 636 of 37,524 is 1.69%, well past the 1% abort threshold. The rate was computed once, before the calibration and repeatability phases — which run below that point and share the same counters. So the guard saw a clean matrix, passed, and then the later phases failed 636 times and inflated the count while the rate stayed frozen at zero. A stale rate next to a live count is worse than either alone: it reads as reassurance while contradicting itself on the same screen. Now the rate is a function of the current counters rather than a captured value, the artifact recomputes it at write time, and the threshold is checked again after every phase has run. The early check stays, so a doomed run still fails fast instead of spending minutes on calibration first. Second gate: refuse to write when the repeatability probe produced no rows. That is what happened here — `rows: []` with `maxAbsDeltaMs: 0` and `maxRelDelta: 0`, which renders as "no run-to-run drift observed" when the truth is that nothing was observed at all. Zeros from an empty sample are not a measurement. This is the same failure shape as the import-graph tripwire earlier in this branch and the deadline test after it: a guard that exists, looks like coverage, and asserts nothing. Worth noting the pattern rather than just the instance — all three needed a deliberate attempt to break them before they became real. No artifact is committed. The harness is the deliverable; the number needs one clean capture on a quiet machine. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Three of the four failed captures on this branch had the same cause, and I
misdiagnosed it twice before the real one turned up: two instances of
scripts/bench-hook.ts running at once. They share one working directory, so the
second one's buildWorkerBundle rewrites the first one's worker bundle and dist
snapshot mid-flight, and the first dies on a harness that was replaced under
it. The earlier explanations — a concurrent `bun run build:cli`, then an
external `dist/` rebuild — were both wrong; the rebuild was the *other
instance's* own ensureDistFresh.
The gates added after the first two failures do their job: a corrupt run now
writes nothing. But refusing to publish garbage is not the same as being able
to take a measurement, and nothing stopped the collision itself.
So: an exclusive lockfile at .bench-hook.lock, taken with O_EXCL before
anything else happens. A second capture is refused with the holder's pid and
start time rather than proceeding. A lock whose holder is no longer alive
(tested with signal 0) is taken over automatically, so a killed run leaves
nothing to clean up by hand.
The ordering matters more than the lock. It is acquired *before*
`process.on("exit", cleanup)` is registered, because cleanup removes the
working directory — so a refused run that had registered cleanup first would
delete the harness out from under the run it just deferred to. That is exactly
how one of the captures died.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A stacked PR — one opened against another feature branch — ran no CI at all. Not a reduced set: none. quality, test, build, docs, test-e2e and rust-quality were all silently skipped, while the third-party checks that do run on any PR still painted a green checkmark. That is the worst possible presentation of 'nothing was verified'. Found the hard way. Phase 1 Stage 0 and Stage 1 — several thousand tests, three Rust crates, a daemon — were pushed to a PR stacked on the design-doc branch, and CI had not executed once. Retargeting that PR at main would have dragged 22 unrelated commits into its diff, so the trigger filter is what needed fixing. The push trigger keeps its main-only filter; this only widens pull_request, which by definition already has a review gate attached. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ng main" This reverts commit 912ff1b.
Member
Author
|
Consolidated into #625 — that branch is now fast-forwarded to Closing this rather than retargeting it at The CI-trigger change pushed here earlier (widening |
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.
Phase 1 of
failproofaid: the design set, the implementation plan, and Stage 0 + Stage 1 implemented.The enforcement plane now has a working privileged evaluator behind a Unix socket. It is dead code unless
FAILPROOFAI_DAEMON_MODEis set — no configuration default, no wizard default. A revert is onegit revert; a field rollback is one environment variable.The thing that would have been silently broken
Execution tiers are derived from a policy's resolved import graph. All 39 builtins shared one module importing
child_process, so derivation would have routed every one of them touser-contextand left the sealed tier empty — an architecture that looks implemented and delivers no verdict integrity at all. The 39 now split 32 / 7 by capability, and__tests__/hooks/builtin-tier-split.test.tswalks the real transitive graph rather than trusting the file layout.That test caught itself. Its import-specifier regex used a newline-excluding character class, which matches none of this codebase's multi-line imports — so the walk stopped at the entry file and the suite went green while
payload-only.tstransitively importednode:fs. Found by injectingnode:fsintoshared.tsand watching nothing fail. It now asserts the exact module set it visited, so "the walk found nothing" is a failure rather than a pass.Stage 0
builtin/payload-only.ts(32) +builtin/host-access.ts(7). Registry order, categories and defaults snapshot-identical.home/projectDirtravel as request data. The daemon evaluates for another UID, so its ownhomedir()is the service account's — andisAgentInternalPath/block-read-outside-cwdboth widen the allow set, the direction that does not announce itself.evaluatePolicies→evaluateVerdicts+encodeResponse. The twelve-vendor response matrix moved character-identically; a "semantically equivalent" rewrite is a silent-allow generator.resolveCodexMode's unbounded scan of~/.codex/sessionsbounded to a 256 KiB head window — measured: firstturn_contextat line 8 in every real transcript sampled, 3.07 ms → 0.54 ms on a 2.7 MB file.Plus: 5,568 parity fixtures across every CLI × event × decision × tool-presence × policy-count, a coverage tripwire over all 348
(cli, event)cells, generated canonicalization tables with a drift gate, and a cold-start latency harness (scripts/bench-hook.ts).Stage 1
The sealed runtime bundles the 32 payload-only builtins, the registry and the full response encoder into one file evaluated inside QuickJS-ng with no bindings registered — no
require, no module loader, noprocess, no filesystem, no sockets.node:pathis replaced by a port proven equivalent tonode:path.posixdifferentially over 8,000+ generated cases; host modules by stubs that throw a named capability;hook-logger/hook-telemetry/telemetry-idby inert no-ops, which removed afetch()to PostHog from a tier the design requires to perform no unbounded I/O.Three Rust crates:
fpai-ipc(framing with a 1 MiB cap validated before allocation, envelope, peer credentials, and the combination lattice),fpai-canon(generated tables),failproofaid(one warm sealed worker, one enforcement lane, an interrupt handler that makes a runaway policy a deadline miss rather than a hung daemon).homeis derived at the socket fromgetpwuid_r(peer_uid)and a client-supplied one is rejected, not overwritten. Overwriting makes the attack a no-op but leaves the protocol looking like it accepts the field, so the next client implementation and the next reviewer both reasonably conclude it is meaningful.cwd/project_dir/env_factsgenuinely cannot be derived, so they ride as client-asserted and any decision reading one is reportedsealed_unattested— the honest version of "unforgeable".How it is verified
In increasing strength:
node:vmcontext with no host globals at all — whatever is not in the context does not exist, exactly as in QuickJS.deny > instruct > allowis associative, commutative, idempotent, equals the maximum, and that adding any number ofuser_contextresults never lowers asealeddeny — the formal statement of the entire two-tier argument.homerefused, unknown env fact refused, oversize frame refused without allocating,homegenuinely derived from the peer UID.Live bugs found and fixed
All four found by survey, none invented by the plan:
"prepare"alone would have published an empty package.publish.ymlhad no build step and depended entirely on that lifecycle script to populate the gitignoreddist/and.next/standalone/, both in thefilesallowlist. Now an explicit build plus a step that fails on a missing or zero-length artifact,--ignore-scriptson publish, and a version check that fails CI if any lifecycle script is re-added.prune-standalone.mjspruned"design-docs"; the directory isdesgin-docs. Unpacking the tarball also showed Next's tracer was about to ship the newCargo.tomlandrust-toolchain.toml.bun.lockbthat does not exist.packages/directory that does not exist.Plus one the bench harness surfaced:
handler.tsarmed a 10-second timeout per custom hook and never cleared it. Invisible today only because the bin entry point callsprocess.exit()on return — and about to stop being invisible in the resident worker and the per-user agent, exactly the processes this PR introduces. Measured at a 10,088 ms p95 in a harness without the hard exit.Test status
cargo fmt/clippy -D warningsThe 15 remaining unit failures are pre-existing
window.localStoragefailures inproject-list.test.tsx(a jsdom/env issue), unchanged from before this branch.What is NOT in this PR
Stages 2–5 of Phase 1 remain. Stage 2 is full twelve-CLI shadow-mode parity against real vendor CLIs; Stage 3 the privileged install,
_failproofaiaccount, systemd/launchd registration and journal-backed transactional setup; Stage 4 the native client, per-user agent and signed schema catalog; Stage 5 collector convergence, which is blocked on an external dependency —agenteye-collectoris a separate repository and its conformance corpus has to be vendored first.Also outstanding and on the critical path: macOS codesigning and notarization, absent from all eight design documents. An unsigned
failproofaidregistered as a LaunchDaemon is killed on macOS 15, which blocks two of the four release targets.🤖 Generated with Claude Code