[failproofaid] Split failproofai into a CLI + Rust background daemon - #632
[failproofaid] Split failproofai into a CLI + Rust background daemon#632NiveditJain wants to merge 149 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 change adds a persistent Rust daemon with Unix-socket IPC, supervised workers, daemon-aware hook dispatch, platform service installation, checksum-verified binary downloads, cross-platform release builds, and expanded Rust, TypeScript, and end-to-end tests. ChangesPersistent daemon architecture
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant DaemonClient
participant DaemonServer
participant Worker
participant ServiceManager
CLI->>DaemonClient: Submit hook event
DaemonClient->>DaemonServer: Send framed request
DaemonServer->>Worker: Forward hook request
Worker-->>DaemonServer: Return hook result
DaemonServer-->>DaemonClient: Return framed response
ServiceManager->>DaemonServer: Start and supervise daemon
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 |
Co-Authored-By: Claude Sonnet 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.
Ignoring alerts on:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (7)
__tests__/hooks/daemon-service.test.ts-77-84 (1)
77-84: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winTwo tests assume no
@failproofai/failproofaid-<platform>package resolves.resolveFailproofaidBinaryPathtries the per-platform npm package before the dev-build fallback. Another layer of this stack publishes those packages and adds them as optional dependencies of the root package. On a linux-x64 machine where the optional dependency installed, both tests take a path they were written to exclude.
__tests__/hooks/daemon-service.test.ts#L77-L84:setArch("x64")withsetPlatform("linux")makes@failproofai/failproofaid-linux-x64resolvable, soresolveFailproofaidBinaryPath()returns the shipped binary and thetoBeNull()assertion fails. Force an architecture with no published package, or mock the module resolution.__tests__/hooks/daemon-service.test.ts#L219-L228: with both environment overrides deleted and a resolvable platform package,installDaemonService()succeeds and installs a real systemd user service pointing at the real daemon binary. Isolate the resolution the same way so the test exercises the "no binary" path it names.🤖 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/daemon-service.test.ts` around lines 77 - 84, Update both __tests__/hooks/daemon-service.test.ts sites (lines 77-84 and 219-228) to isolate platform-binary resolution by forcing an architecture without a published package or mocking module resolution. Preserve each test’s intended no-binary behavior so resolveFailproofaidBinaryPath() returns null and installDaemonService() does not install a real service.src/hooks/configure-wizard.ts-644-647 (1)
644-647: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winThe outro can now exceed 80 columns and be hard-truncated.
The comment above this code records the constraint:
writeLinestruncates with a hard cut and no ellipsis, so an over-long line reads as broken output.Adding
daemonNotepushes the worst case past 80 characters:Setup complete — 13 policies + your custom policies · 12 assistants · background daemon enabledThat is about 95 characters.
daemonNoteis the last segment, so it is the part that gets cut, which is exactly the information this change adds. Shorten the note, or move it to its own line.♻️ Shorten the daemon note
- const daemonNote = daemonInstalled ? " · background daemon enabled" : ""; + // Keep the whole outro inside 80 columns — writeLines hard-cuts, and this + // note is the last segment, so it is the first thing to disappear. + const daemonNote = daemonInstalled ? " · daemon on" : "";🤖 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/configure-wizard.ts` around lines 644 - 647, Update the daemonNote text used by the outro call in configure wizard output so the complete worst-case setup message remains within the 80-column limit; keep the daemon-enabled status visible by shortening that note rather than allowing writeLines to hard-truncate it.src/hooks/configure-wizard.ts-613-620 (1)
613-620: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winReport a daemon install failure on the wizard's own output.
reviewLinesline 352 tells the user "failproofaid will be installed/started as a background service". When the install fails, the only report ishookLogWarn. That writes to stderr and to the log file, andshouldEmit("warn")can suppress it entirely (src/hooks/hook-logger.tslines 115-119). The wizard then prints "Setup complete" with no daemon note and no reason.The user was promised a background service, does not get one, and is given no explanation on the channel they are watching.
Write the failure to
stdout, which the wizard already owns, in addition tohookLogWarn.♻️ Surface the failure to the user
} else { hookLogWarn(`failproofaid was not installed as a service: ${daemonResult.reason}`); + // reviewLines() promised this install, so a silent stderr warning leaves + // the user with an unexplained gap between the review screen and the outro. + stdout.write( + `\n ! Background daemon not installed: ${daemonResult.reason}\n` + + ` Setup is otherwise complete; hooks run in-process as before.\n`, + ); }🤖 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/configure-wizard.ts` around lines 613 - 620, Update the failed-install branch in the configure wizard’s daemon setup flow to write daemonResult.reason to stdout in addition to the existing hookLogWarn call. Keep the warning log intact, and ensure the wizard’s own output clearly reports that failproofaid was not installed as a service and includes the failure reason.__tests__/hooks/worker-server.test.ts-89-89 (1)
89-89: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winKeep the test socket path short enough for the platform limit.
Unix socket paths are limited to about 104 bytes on macOS and 108 on Linux. On macOS
tmpdir()returns a long/var/folders/...path. Addingfpai-worker-server-test-<pid>-<timestamp>.sockpushes the total close to that limit and can make this suite fail withENAMETOOLONGon macOS runners. Use a short basename, for examplew-${process.pid}.sock.Also unlink
workerSocketPathinafterEachso repeated runs do not leave socket files in the temporary directory.🤖 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 89, Shorten the socket basename assigned to workerSocketPath in the test setup to avoid Unix path-length limits, using a compact process-specific name. Add cleanup in the test suite’s afterEach hook to unlink workerSocketPath after each test, while safely handling an already-absent socket.crates/PROTOCOL.md-10-13 (1)
10-13: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUpdate the stale Stage 2 statement.
crates/failproofaid/src/server.rsnow relayshookto the warm worker throughworker.call(). It no longer returns a "not implemented" stub. Lines 89-90 carry the same stale caveat inside theerrorexample.📝 Proposed documentation fix
Implemented in `crates/fpai-ipc` (framing + envelope + peer verification) and -`crates/failproofaid` (the socket server itself). As of Stage 2, the daemon -answers `ping` and rejects `hook` with a stub "not implemented" error — Stage 3 -wires `hook` up to a real warm Node/Bun worker. +`crates/failproofaid` (the socket server itself). The daemon answers `ping` +directly and relays `hook` to a warm Node/Bun worker process.// The daemon accepted the connection and parsed the request, but could not -// produce a verdict (worker down/hung, or — in Stage 2 — hook evaluation -// simply isn't wired up yet). Distinct from hookResult so the client can +// produce a verdict (worker down/hung, or a protocol version mismatch). +// Distinct from hookResult so the client can🤖 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/PROTOCOL.md` around lines 10 - 13, Update the Stage 2 documentation to state that the daemon relays hook requests to the warm worker via worker.call() instead of rejecting them with a “not implemented” stub, and revise the matching error example on lines 89-90 to remove the stale caveat.crates/failproofaid/src/paths.rs-16-27 (1)
16-27: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReject an override with an empty parent directory.
Path::parent()returnsSome("")for a bare filename such asdaemon.sock. Theok_or_elsebranch therefore never runs for that input.run_dir()then returns an empty path,ensure_run_dir()fails insidefs::create_dir_all("")with an unrelated ENOENT message, andlock_path()plusworker_socket_path()silently resolve against the process working directory.Treat an empty parent as the same error as a missing parent.
🐛 Proposed fix
if let Some(socket_override) = std::env::var_os("FAILPROOFAI_DAEMON_SOCKET") { let path = PathBuf::from(socket_override); return path .parent() + .filter(|parent| !parent.as_os_str().is_empty()) .map(PathBuf::from) - .ok_or_else(|| io::Error::other("FAILPROOFAI_DAEMON_SOCKET has no parent directory")); + .ok_or_else(|| { + io::Error::other( + "FAILPROOFAI_DAEMON_SOCKET must be an absolute path with a parent directory", + ) + }); }🤖 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/failproofaid/src/paths.rs` around lines 16 - 27, Update run_dir() to reject socket overrides whose parent path is empty, treating Path::parent() returning Some("") the same as None and returning the existing descriptive io::Error. Preserve valid non-empty parent directories and the HOME-based fallback behavior.crates/failproofaid/src/paths.rs-111-122 (1)
111-122: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRestore
HOMEand use RAII cleanup for the env mutations.This test overwrites
HOMEfor the whole test process and never restores it.run_dir()falls back toHOMEwheneverFAILPROOFAI_DAEMON_SOCKETis absent, so any later test in this binary that resolves a default path observes/home/example-user. The other tests in this module have the same weakness in the opposite direction: eachremove_varruns after the assertions, so a failed assertion leaksFAILPROOFAI_DAEMON_SOCKETinto every test that follows.Add a small guard type that captures the previous values and restores them on drop, including on panic.
💚 Proposed test guard
struct EnvGuard { _lock: std::sync::MutexGuard<'static, ()>, saved: Vec<(&'static str, Option<std::ffi::OsString>)>, } impl EnvGuard { fn new(keys: &[&'static str]) -> Self { let _lock = ENV_LOCK.lock().unwrap_or_else(|e| e.into_inner()); let saved = keys .iter() .map(|key| (*key, std::env::var_os(key))) .collect(); Self { _lock, saved } } fn set(&self, key: &str, value: impl AsRef<std::ffi::OsStr>) { unsafe { std::env::set_var(key, value) }; } fn unset(&self, key: &str) { unsafe { std::env::remove_var(key) }; } } impl Drop for EnvGuard { fn drop(&mut self) { for (key, value) in &self.saved { match value { Some(value) => unsafe { std::env::set_var(key, value) }, None => unsafe { std::env::remove_var(key) }, } } } }Then each test becomes, for example:
fn default_socket_path_lives_under_home_dot_failproofai_run() { - let _guard = ENV_LOCK.lock().unwrap(); - unsafe { - std::env::remove_var("FAILPROOFAI_DAEMON_SOCKET"); - std::env::set_var("HOME", "/home/example-user"); - } + let env = EnvGuard::new(&["FAILPROOFAI_DAEMON_SOCKET", "HOME"]); + env.unset("FAILPROOFAI_DAEMON_SOCKET"); + env.set("HOME", "/home/example-user");🤖 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/failproofaid/src/paths.rs` around lines 111 - 122, Introduce an RAII EnvGuard near ENV_LOCK that captures specified environment-variable values, holds the lock, provides set/unset helpers, and restores all saved values in Drop. Update default_socket_path_lives_under_home_dot_failproofai_run and the other environment-mutating tests to construct the guard with HOME and FAILPROOFAI_DAEMON_SOCKET, then use its helpers instead of direct mutations or manual cleanup so restoration also occurs during panics.
🧹 Nitpick comments (16)
__tests__/hooks/configure-wizard.test.ts (2)
469-496: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe service-path review line has no coverage.
daemonServiceFilePathis mocked to returnnullfor the whole file, soconfigure-wizard.tslines 381-382 never take theif (servicePath)branch. The review screen's "failproofaid service" file entry is new user-facing output in this PR, and no test asserts it appears.Mock the path for one case and assert it is listed under "This will update:".
💚 Cover the service-path line
expect(withDaemon).toContain("Daemon"); expect(withDaemon).toContain("failproofaid"); + + // The "This will update:" list must name the service file the install is + // about to write; the module-level mock returns null, so this branch was + // otherwise unreachable. + vi.mocked(daemonServiceFilePath).mockReturnValue( + "/home/tester/.config/systemd/user/failproofaid.service", + ); + const withServicePath = reviewLines({ + scope: "user", + clis: ["claude"], + policies: ["block-sudo"], + cwd: "/tmp/proj", + }).join("\n"); + expect(withServicePath).toContain("failproofaid.service"); + expect(withServicePath).toContain("failproofaid service"); + vi.mocked(daemonServiceFilePath).mockReturnValue(null);Add
daemonServiceFilePathto the import at line 37:-import { isDaemonSupportedPlatform, installDaemonService } from "../../src/hooks/daemon-service"; +import { + isDaemonSupportedPlatform, + installDaemonService, + daemonServiceFilePath, +} from "../../src/hooks/daemon-service";Note: lines 454 and 466 assert the literal
"background daemon enabled". I proposed shortening that outro string insrc/hooks/configure-wizard.tslines 644-647 to keep the line inside 80 columns. If you take that change, update both assertions with it. That is an intentional change to the message under test, which the test guidelines permit.As per coding guidelines: "Update a test only when intentionally changing the value or message it verifies."
🤖 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/configure-wizard.test.ts` around lines 469 - 496, Extend the reviewLines coverage in the existing test to mock daemonServiceFilePath with a non-null path for one user-scope case, then assert the resulting “This will update:” output includes the failproofaid service file entry. Import daemonServiceFilePath from the existing module alongside the other test dependencies, while preserving the current supported-platform and project-scope assertions.Source: Coding guidelines
405-415: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a case where
markDaemonConfiguredcannot persist the flag.
markDaemonConfiguredreturns early when the global config is malformed JSON, and it swallows write errors (src/hooks/configure-wizard.tslines 280-288). In both casesdaemonInstalledstill becomestrue, so the wizard prints "background daemon enabled" whiledaemonConfiguredwas never written. The next hook invocation then takes the in-process path, not the daemon path.That divergence between the reported state and the persisted state is untested. Write a malformed global config before running the wizard and pin the behavior you want.
The
configure_daemon_installtelemetry event at lines 621-625 is also unasserted, so neither theinstalledflag nor thereasonpayload is covered.💚 Pin the unpersisted-flag path
+ it("does not claim the daemon is enabled when the flag cannot be persisted", async () => { + // markDaemonConfigured bails out on a malformed global config. The install + // itself succeeded, so without this test nothing catches the wizard + // reporting a daemon that later hook runs will not use. + mkdirSync(resolve(fileHome, ".failproofai"), { recursive: true }); + writeFileSync(globalConfigPath(), "{ not valid json", "utf8"); + vi.mocked(isDaemonSupportedPlatform).mockReturnValue(true); + vi.mocked(installDaemonService).mockResolvedValue({ installed: true }); + vi.mocked(selectOne).mockResolvedValueOnce("user").mockResolvedValueOnce("apply"); + vi.mocked(multiSelect).mockResolvedValueOnce(["claude"]).mockResolvedValueOnce(["git"]); + + await runConfigureWizard(ttyIO()); + + expect(existsSync(globalConfigPath())).toBe(true); + expect(readFileSync(globalConfigPath(), "utf8")).not.toContain("daemonConfigured"); + const message = vi.mocked(outro).mock.calls[0]![0]; + expect(message).not.toContain("background daemon enabled"); + });🤖 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/configure-wizard.test.ts` around lines 405 - 415, Extend the configure wizard tests around runConfigureWizard with a malformed global configuration, then assert the expected unpersisted daemon state after markDaemonConfigured returns without writing. Also verify the configure_daemon_install telemetry event records the installed flag and reason payload for this failure path, while preserving the existing successful-install coverage.src/hooks/daemon-service.ts (3)
121-139: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winReject or escape systemd-hostile characters in
workerCmd.The launchd path escapes its interpolated value with
escapeXml. The systemd path interpolatesworkerCmdraw into a quotedEnvironment=line. A value that contains",\, or a newline produces a malformed unit, or adds an unintended directive.systemctl --user enable --nowthen fails with an opaque error, andinstallDaemonServicereturns that error as itsreason.The value comes from
FAILPROOFAI_WORKER_CMDor fromFAILPROOFAI_PACKAGE_ROOT, so this is robustness rather than a privilege boundary. Escaping the two characters systemd cares about keeps the failure mode out of the unit file.♻️ Escape the value before interpolation
+/** + * systemd's `Environment="…"` quoting understands `\"` and `\\`; a literal + * newline ends the directive outright, so it is dropped rather than escaped. + */ +function escapeSystemdEnvValue(s: string): string { + return s.replace(/\\/g, "\\\\").replace(/"/g, '\\"').replace(/[\r\n]/g, " "); +} + function systemdUnitContents(binaryPath: string, workerCmd: string | null): string { // Quoted because FAILPROOFAI_WORKER_CMD's value ("node /abs/path/worker.mjs") // contains a space — systemd's Environment= requires quoting whenever the // value does. - const envLine = workerCmd ? `Environment="FAILPROOFAI_WORKER_CMD=${workerCmd}"\n` : ""; + const envLine = workerCmd + ? `Environment="FAILPROOFAI_WORKER_CMD=${escapeSystemdEnvValue(workerCmd)}"\n` + : "";🤖 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/daemon-service.ts` around lines 121 - 139, Update systemdUnitContents to escape workerCmd before interpolating it into the quoted Environment= line, handling at least backslashes, double quotes, and newline characters according to systemd unit escaping rules. Preserve the existing omission of the environment line when workerCmd is null and use the escaped value for the generated unit.
164-167: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
KeepAliveandRestart=on-failureare not equivalent.The systemd unit uses
Restart=on-failure, so a clean exit stops the service. The plist usesKeepAlive=true, so launchd restarts the daemon after every exit, including exit code 0. The daemon implements graceful shutdown, so on macOS a graceful stop is immediately undone, and singleton locking then contends with the relaunched copy.If you want the two platforms to behave the same, use the dictionary form.
♻️ Mirror `Restart=on-failure` on launchd
<key>KeepAlive</key> - <true/> + <dict> + <key>SuccessfulExit</key> + <false/> + </dict>🤖 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/daemon-service.ts` around lines 164 - 167, Update the launchd plist’s KeepAlive configuration near RunAtLoad to use dictionary form matching systemd’s Restart=on-failure behavior, so clean exits are not restarted while unexpected failures still trigger relaunch. Preserve RunAtLoad and the existing daemon service configuration.
297-304: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
launchctl listreports loaded jobs, not running jobs.
launchctl listprints every registered job, including one whose PID column is-because it exited or never started. A substring match on the label therefore returns"running"for a crash-looping or throttled daemon. The systemd branch avoids this becausesystemctl is-activereports the actual unit state.Parse the PID column, or query the job directly.
♻️ Check the PID column instead of the label
const plistPath = launchdPlistPath(); if (!existsSync(plistPath)) return "not-installed"; try { const out = execFileSync("launchctl", ["list"], { stdio: ["ignore", "pipe", "ignore"] }).toString(); - return out.includes("ai.failproof.failproofaid") ? "running" : "stopped"; + // `launchctl list` columns are PID, LastExitStatus, Label. A job that is + // loaded but not running shows "-" for the PID, so matching the label + // alone reports a crash-looping daemon as running. + const row = out.split("\n").find((line) => line.endsWith("ai.failproof.failproofaid")); + if (!row) return "not-installed"; + return /^\d+\s/.test(row.trimStart()) ? "running" : "stopped"; } catch { return "stopped"; }🤖 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/daemon-service.ts` around lines 297 - 304, Update the launchd status logic in the daemon status function around launchdPlistPath and launchctl to determine whether the job is actually running, not merely loaded. Parse the launchctl list output for ai.failproof.failproofaid and require a valid non-dash PID (or query the job directly for its active state); return "stopped" when the job is loaded without a running process, while preserving "not-installed" and error handling.src/hooks/configure-wizard.ts (1)
275-289: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winReuse the existing config writer instead of a hand-rolled read-modify-write.
This function reads, mutates, and rewrites
~/.failproofai/policies-config.jsondirectly. Two concerns:
writeFileSynctruncates in place. If the process dies mid-write, the global policies config is left truncated, which disables every enabled policy on the machine.hooks-config.tsalready owns reading and writing this file, so it is the right place for adaemonConfiguredsetter, and it can write through a temp file andrename.- Duplicating the shape knowledge here means
markDaemonConfigureddoes not know about any normalization or migrationhooks-config.tsapplies.#!/bin/bash # Description: Look for an existing writer/setter for the hooks config file. set -euo pipefail ast-grep outline src/hooks/hooks-config.ts --items all rg -n -C4 'writeFileSync|renameSync|daemonConfigured' src/hooks/hooks-config.ts🤖 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/configure-wizard.ts` around lines 275 - 289, Replace the hand-rolled read/modify/write logic in markDaemonConfigured with a setter exposed by hooks-config.ts for daemonConfigured. Have that config-layer setter reuse its existing normalization and atomic temp-file/rename writing path, while preserving markDaemonConfigured’s best-effort behavior and early return on configuration read failure.__tests__/hooks/daemon-service.test.ts (1)
86-103: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThis test can pass without asserting anything.
Every assertion sits inside
if (result !== null). The comment states that whethertarget/is built depends on test execution order. When it is not built, the block never runs and the test passes while covering nothing. In CI, wherecargo buildmay never run before Vitest, the dev-build resolution branch ofresolveFailproofaidBinaryPathhas no coverage at all.Build the fixture instead of depending on repository state. A temp directory with
target/release/failproofaidmakes the assertion unconditional, and it also lets you cover thetarget/debugfallback, which is currently untested.💚 Deterministic fixture for the dev-build branch
- it("finds a locally-built dev binary under target/release relative to the package root", async () => { - delete process.env.FAILPROOFAI_DAEMON_BINARY; - // The real repo's own target/{release,debug}/failproofaid — built by - // the Rust test suite / a local `cargo build` earlier in this session. - process.env.FAILPROOFAI_PACKAGE_ROOT = resolve(__dirname, "..", ".."); - setPlatform("linux"); - const { resolveFailproofaidBinaryPath } = await import("../../src/hooks/daemon-service"); - const result = resolveFailproofaidBinaryPath(); - // Not asserting a specific outcome beyond "doesn't throw and returns a - // sensible type" here would be too weak — but whether target/ has been - // built depends on test execution order across files sharing state in - // this repo, so assert the *shape* of a real hit without depending on - // build state: either null, or an absolute path that actually exists. - if (result !== null) { - expect(existsSync(result)).toBe(true); - expect(result).toContain("failproofaid"); - } - }); + // A synthesized package root, not the repo's own target/ — the real + // directory's contents depend on whether cargo ran, which made the + // assertions conditional and the test vacuous in CI. + it.each(["release", "debug"])( + "finds a locally-built dev binary under target/%s relative to the package root", + async (profile) => { + delete process.env.FAILPROOFAI_DAEMON_BINARY; + const root = mkdtempSync(join(tmpdir(), "fpai-pkg-root-")); + const binDir = resolve(root, "target", profile); + mkdirSync(binDir, { recursive: true }); + const binary = resolve(binDir, "failproofaid"); + writeFileSync(binary, "", "utf8"); + process.env.FAILPROOFAI_PACKAGE_ROOT = root; + setPlatform("linux"); + setArch("arm64"); // no platform package resolves, so the dev branch runs + try { + const { resolveFailproofaidBinaryPath } = await import("../../src/hooks/daemon-service"); + expect(resolveFailproofaidBinaryPath()).toBe(binary); + } finally { + rmSync(root, { recursive: true, force: true }); + } + }, + );Add the imports this needs:
-import { existsSync, readFileSync, rmSync } from "node:fs"; -import { homedir } from "node:os"; -import { resolve } from "node:path"; +import { existsSync, readFileSync, rmSync, mkdtempSync, mkdirSync, writeFileSync } from "node:fs"; +import { homedir, tmpdir } from "node:os"; +import { resolve, join } from "node:path";Note:
releasemust win overdebugwhen both exist. That ordering is asserted by the loop only per-profile, so consider one extra case with both present.🤖 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/daemon-service.test.ts` around lines 86 - 103, Make the dev-build tests deterministic by creating a temporary package-root fixture containing target/release/failproofaid and asserting resolveFailproofaidBinaryPath always returns that existing absolute path. Add a separate fixture covering target/debug/failproofaid when release is absent, and verify release is selected when both profiles exist. Remove the conditional assertion and repository-state dependency from the test identified by resolveFailproofaidBinaryPath.__tests__/hooks/daemon-client.test.ts (1)
188-200: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a case for the oversized-frame rejection.
daemon-client.tslines 139-142 reject a frame whose declared length exceedsMAX_FRAME_LEN(16 MiB). No test covers that branch. It is cheap to cover deterministically: send only the 4-byte header with an oversized length and no body. The client must returnnullwithout buffering.This also proves the rejection happens at header-parse time rather than after accumulating the payload, which is the security-relevant part of the guard.
💚 Cover the oversized-frame guard
+ it("returns null on a frame that declares a length above the 16 MiB cap", async () => { + await startServer(async (socket) => { + await readFrame(socket); + // Header only, no body — the client must reject on the declared length + // alone rather than waiting to buffer 32 MiB. + const header = Buffer.alloc(4); + header.writeUInt32BE(32 * 1024 * 1024, 0); + socket.write(header); + }); + + const { tryDaemonHook } = await import("../../src/hooks/daemon-client"); + const start = Date.now(); + const result = await tryDaemonHook({ hookEvent: "PreToolUse", cli: "claude", stdin: "{}" }); + expect(result).toBeNull(); + // Rejected on the header, not by timing out while waiting for a body. + expect(Date.now() - start).toBeLessThan(140); + }); + it("skips the attempt entirely on win32, never touching the socket", async () => {🤖 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/daemon-client.test.ts` around lines 188 - 200, Add a test alongside the existing malformed-frame case that starts the test server, reads the request frame, sends only a 4-byte header declaring a length greater than MAX_FRAME_LEN, and closes the socket without a payload. Invoke tryDaemonHook with the same representative hook arguments and assert it returns null, covering rejection during header parsing without buffering the body.__tests__/hooks/handler.test.ts (1)
299-303: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace the 50 ms wall-clock budget with a deterministic ordering check.
This assertion depends on real elapsed time. If
readMergedHooksConfigorloadAllCustomHooksis not mocked in this file, the evaluation performs filesystem work and can exceed 50 ms on a loaded CI runner, which makes the test flaky. The property under test is that the outcome does not wait ontrackPromise, not that it completes in 50 ms.Await the outcome first and assert that it resolved while the telemetry promise was still pending, for example by recording a flag when
releaseTelemetryruns and asserting it is stillfalseafterawait outcomePromise.🤖 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/handler.test.ts` around lines 299 - 303, Replace the wall-clock Promise.race assertion around outcomePromise with a deterministic ordering check: await outcomePromise, record whether releaseTelemetry has run, and assert that flag remains false immediately after the outcome resolves. Preserve the test’s verification that the outcome does not wait on trackPromise, without relying on a timeout.bin/failproofai-worker.mjs (1)
47-51: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winBound the graceful shutdown so the worker cannot hang on an open connection.
server.close()stops accepting new connections and waits for every existing connection to end. The Rust supervisor may hold a persistent connection to this socket. In that case the callback never runs and the process stays alive until the supervisor escalates toSIGKILL, which delays every daemon restart.Close live connections and add a timeout fallback.
♻️ Proposed change
function shutdown() { - server.close(() => process.exit(0)); + server.close(() => process.exit(0)); + server.closeAllConnections?.(); + // Never let a stuck connection block the supervisor's restart. + setTimeout(() => process.exit(0), 2000).unref(); }🤖 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 `@bin/failproofai-worker.mjs` around lines 47 - 51, Update shutdown() to close or destroy active server connections before or during server.close(), then add a bounded timeout that forcefully exits if the graceful close callback does not run. Preserve immediate clean exit when server.close() completes, and ensure the fallback timer cannot keep the process alive after successful shutdown.src/hooks/handler.ts (1)
359-359: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAttach a rejection handler when the telemetry promise is not awaited.
When
awaitTelemetryFlushisfalse,trackPromiseis never awaited and never handled. The surroundingtry/catchcannot catch that rejection. In the warm worker this becomes an unhandled promise rejection, which Node terminates on by default.sendEventis documented as never rejecting, so this is defensive, but the worker is long-lived and a single unhandled rejection kills it.♻️ Proposed change
if (opts?.awaitTelemetryFlush ?? true) { await trackPromise; + } else { + void trackPromise.catch(() => {}); }Also applies to: 383-385
🤖 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/handler.ts` at line 359, Update the trackHookEvent call producing trackPromise in the hook handler so the non-awaited path attaches an explicit rejection handler, while preserving the existing awaited behavior when awaitTelemetryFlush is true. Ensure any rejection is consumed or logged defensively so the warm worker cannot receive an unhandled promise rejection.__tests__/hooks/worker-server.test.ts (1)
174-206: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a test that writes two frames in a single socket write.
The current tests always send one frame per connection, so they cannot detect the pipelining gap in
src/hooks/worker-server.tslines 72-125. Add a case that concatenates two encoded request frames into onesocket.writecall and asserts that the server returns twohookResultresponses. That test fails against the current handler and passes after the framing loop fix.As per path instructions: "Always add unit tests for new behaviour."
🤖 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` around lines 174 - 206, Add a worker-server test that encodes two valid hook requests, concatenates both frames, and sends them through one socket.write call; read and assert two hookResult responses with successful exit codes. Place it near the existing malformed-frame test and reuse the established request/frame helpers, ensuring the test verifies pipelined frame handling on a single connection.Source: Path instructions
crates/failproofaid/src/worker.rs (2)
213-216: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winSet a write timeout as well as a read timeout.
write_messageat Line 225 can send up to about 1 MiB. If the worker accepts the connection but stops reading, the write blocks with no deadline and the connection thread hangs. Addset_write_timeoutto bound both directions.🔧 Proposed fix
stream .set_read_timeout(Some(Duration::from_secs(30))) .map_err(WorkerError::Io)?; + stream + .set_write_timeout(Some(Duration::from_secs(30))) + .map_err(WorkerError::Io)?;🤖 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/failproofaid/src/worker.rs` around lines 213 - 216, Update the UnixStream setup in the worker connection flow to call set_write_timeout with the same 30-second duration as set_read_timeout, propagating failures through WorkerError::Io before write_message can run.
71-75: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winResolve the worker script against the binary location, not the cwd.
PathBuf::from("dist/worker.mjs")is relative. It resolves against the daemon's own cwd. A systemd user service or a launchd job does not run with the install directory as cwd, so this fallback fails in the packaged case. Resolve the script fromstd::env::current_exe()instead.🔧 Proposed fix
- // Packaging (Stage 5) lands dist/worker.mjs; until then this is only - // reachable via the explicit override above in dev/test. - WorkerCommand::Node { - script: PathBuf::from("dist/worker.mjs"), - } + // Packaging lands dist/worker.mjs next to the binary. Anchor on the + // binary path: the daemon's cwd is set by the service manager. + let script = std::env::current_exe() + .ok() + .and_then(|exe| exe.parent().map(|dir| dir.join("dist/worker.mjs"))) + .unwrap_or_else(|| PathBuf::from("dist/worker.mjs")); + WorkerCommand::Node { script }🤖 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/failproofaid/src/worker.rs` around lines 71 - 75, Update the packaged fallback in the WorkerCommand::Node branch to derive dist/worker.mjs relative to std::env::current_exe() rather than constructing a cwd-relative PathBuf. Preserve the explicit override behavior while ensuring the default script resolves from the daemon binary’s directory.crates/failproofaid/src/paths.rs (1)
80-81: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winCreate the run directory owner-only in one step.
fs::create_dir_allapplies the process umask first. The directory is group- and world-readable untilset_permissionsruns. The directory holds the socket that returns security decisions, so close that window by setting the mode at creation time withDirBuilderExt.Note that
DirBuilderapplies the mode to every component it creates, which is the desired behavior for~/.failproofai/run.🔒️ Proposed refactor
-use std::fs; +use std::fs::{self, DirBuilder}; use std::io; +use std::os::unix::fs::DirBuilderExt; use std::os::unix::fs::PermissionsExt;- fs::create_dir_all(&dir)?; - fs::set_permissions(&dir, fs::Permissions::from_mode(0o700))?; + DirBuilder::new().recursive(true).mode(0o700).create(&dir)?; Ok(dir)#!/bin/bash # Confirm no other code depends on the umask-created mode, and locate all run-dir consumers. rg -nP --type=rust -C3 'ensure_run_dir|set_permissions|from_mode|DirBuilder' crates🤖 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/failproofaid/src/paths.rs` around lines 80 - 81, Update the run-directory creation in ensure_run_dir to use DirBuilder with DirBuilderExt::mode(0o700) before create, rather than calling fs::create_dir_all followed by fs::set_permissions. Preserve recursive creation so every newly created component receives the owner-only mode, and remove the now-unnecessary post-creation permission update.crates/failproofaid/tests/daemon_e2e.rs (1)
23-36: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the per-test run directory after each test.
Each test creates a unique parent directory under the system temp directory and never removes it. Every
cargo testrun leaks three directories, each holding a stale socket or lock file. Add the cleanup to the same drop guard suggested for the child process.🤖 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/failproofaid/tests/daemon_e2e.rs` around lines 23 - 36, Update the test cleanup drop guard used with the child process to also remove the unique parent directory returned by unique_socket_path, not just terminate the process. Ensure cleanup runs after every test and removes the directory recursively so stale sockets and lock files do not remain.
🤖 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/daemon-service.test.ts`:
- Around line 206-217: Strengthen the re-installation test around
installDaemonService by using a genuinely different second binary path, then
assert the rewritten unit file contains the new path and no longer contains the
original path. Ensure the assertions distinguish the second installation from a
no-op.
In @.github/workflows/build-daemon.yml:
- Line 47: The actions/checkout steps used before compiling third-party Rust
dependencies must not persist the GITHUB_TOKEN. In
.github/workflows/build-daemon.yml lines 47-47 and .github/workflows/ci.yml
lines 96-149, update each actions/checkout@v7.0.1 step to set
persist-credentials to false while preserving the existing job behavior.
- Around line 51-59: Restrict caching in the build job so pull_request runs can
only restore existing Cargo caches and cannot write to shared keys. Replace the
current actions/cache@v6 usage with the restore-only action for untrusted
triggers, and add saving only for release or workflow_dispatch triggers,
preserving the existing cache paths and key.
- Around line 39-44: Update the darwin-x64 matrix entry for target
x86_64-apple-darwin in the workflow to replace the retired macos-13 runner label
with a supported Intel macOS runner, preferably macos-15-intel when available.
Leave the darwin-arm64 entry unchanged.
In `@bin/failproofai.mjs`:
- Around line 133-135: Replace the immediate process.exit(result.exitCode) in
the daemon hook path with process.exitCode assignment so stdout/stderr writes
and the asynchronous trackHookEvent call from evaluateHookEvent can drain before
termination. Verify handleHookEvent’s non-daemon path uses the same exit
discipline and align the daemon path without changing its exit-code behavior.
In `@crates/failproofaid/src/server.rs`:
- Around line 50-60: Update the accepted stream handling in the server accept
loop to explicitly restore blocking mode on each TcpStream before spawning the
worker thread or calling handle_connection. Propagate any set_nonblocking
failure consistently with the surrounding error handling, while leaving the
listener’s non-blocking polling behavior unchanged.
- Around line 84-103: Update handle_connection to configure finite read and
write timeouts on the accepted UnixStream before read_message and subsequent
response handling, propagating setup errors through its io::Result return. Also
update run_until to bound concurrent connection handling, using a worker pool or
maximum in-flight connection count so stalled clients cannot create unbounded
threads.
In `@crates/failproofaid/src/worker.rs`:
- Around line 150-177: Update Worker::ensure_started to remove any stale
socket_path before spawning the worker, then replace the socket_path.exists()
readiness check with an actual Unix-socket connection probe that confirms the
new worker is accepting connections. Preserve the existing child-exit and
timeout handling, and ensure shutdown cleanup in Worker::drop or
kill_process_group callers removes socket_path as well.
- Around line 95-117: Update the worker process setup in the command-spawning
flow to prevent piped stdout and stderr from filling: either discard both
streams with Stdio::null() or continuously drain each captured stream on
background threads. Preserve the existing process-group behavior and ensure both
worker output channels remain non-blocking for the worker's full lifetime.
In `@crates/failproofaid/tests/daemon_e2e.rs`:
- Around line 38-54: Update spawn_daemon and the shared test cleanup to prevent
daemon hangs and orphaned processes: drain or inherit the child’s output, poll
Child::try_wait() while waiting for the socket, and include captured output when
startup exits or times out. Add a guard type that kills and reaps the daemon on
Drop, then use it across all three tests so cleanup also occurs during assertion
panics.
In `@src/hooks/configure-wizard.ts`:
- Around line 621-625: Update the configure_daemon_install emission in the
daemon configuration flow to avoid sending daemonResult.reason verbatim in
telemetry. Replace it with a bounded, non-sensitive failure classification while
preserving null for successful installations, and retain the full failure
message only through the existing local hookLogWarn handling in
installDaemonService.
In `@src/hooks/daemon-client.ts`:
- Around line 23-33: The DAEMON_ATTEMPT_TIMEOUT_MS budget is too short for
legitimate serialized daemon evaluations and can deny valid tool calls. Update
tryDaemonHook’s timeout to exceed the slowest supported policy evaluation,
including the 10-second custom-policy limit and queueing/file-I/O overhead;
preserve the existing behavior for genuinely unreachable daemons.
In `@src/hooks/daemon-service.ts`:
- Around line 240-268: Update uninstallDaemonService to clear the persisted
daemonConfigured state after the platform-specific service removal completes,
using the existing configuration helper used by markDaemonConfigured. Ensure the
flag is cleared on successful uninstall so isDaemonConfigured returns false and
install/uninstall state remains symmetric.
- Around line 231-237: Update src/hooks/daemon-service.ts lines 231-237 to
verify the daemon is genuinely running by polling daemonServiceStatus() or
probing its socket after enable --now/load -w; return an install failure unless
it becomes reachable, preventing markDaemonConfigured() for failed startups.
Update src/hooks/daemon-service.ts lines 240-268 to clear daemonConfigured from
~/.failproofai/policies-config.json during uninstall so the in-process path is
restored.
- Around line 208-230: Set a finite timeout option on every execFileSync
invocation in installDaemonService, uninstallDaemonService, and
daemonServiceStatus, including systemctl and launchctl calls. Preserve the
existing stdio behavior and error-handling paths so a timeout throws and is
converted into the current failure result or status behavior rather than
blocking indefinitely.
In `@src/hooks/worker-server.ts`:
- Around line 72-125: Update the socket data handler around the frame-decoding
logic to loop while recvBuf contains a complete frame, allowing multiple
coalesced requests to be parsed and enqueued from one data event. Preserve
partial-frame buffering, MAX_FRAME_LEN validation, malformed-request handling,
and declaredLen reset behavior for each frame.
---
Minor comments:
In `@__tests__/hooks/daemon-service.test.ts`:
- Around line 77-84: Update both __tests__/hooks/daemon-service.test.ts sites
(lines 77-84 and 219-228) to isolate platform-binary resolution by forcing an
architecture without a published package or mocking module resolution. Preserve
each test’s intended no-binary behavior so resolveFailproofaidBinaryPath()
returns null and installDaemonService() does not install a real service.
In `@__tests__/hooks/worker-server.test.ts`:
- Line 89: Shorten the socket basename assigned to workerSocketPath in the test
setup to avoid Unix path-length limits, using a compact process-specific name.
Add cleanup in the test suite’s afterEach hook to unlink workerSocketPath after
each test, while safely handling an already-absent socket.
In `@crates/failproofaid/src/paths.rs`:
- Around line 16-27: Update run_dir() to reject socket overrides whose parent
path is empty, treating Path::parent() returning Some("") the same as None and
returning the existing descriptive io::Error. Preserve valid non-empty parent
directories and the HOME-based fallback behavior.
- Around line 111-122: Introduce an RAII EnvGuard near ENV_LOCK that captures
specified environment-variable values, holds the lock, provides set/unset
helpers, and restores all saved values in Drop. Update
default_socket_path_lives_under_home_dot_failproofai_run and the other
environment-mutating tests to construct the guard with HOME and
FAILPROOFAI_DAEMON_SOCKET, then use its helpers instead of direct mutations or
manual cleanup so restoration also occurs during panics.
In `@crates/PROTOCOL.md`:
- Around line 10-13: Update the Stage 2 documentation to state that the daemon
relays hook requests to the warm worker via worker.call() instead of rejecting
them with a “not implemented” stub, and revise the matching error example on
lines 89-90 to remove the stale caveat.
In `@src/hooks/configure-wizard.ts`:
- Around line 644-647: Update the daemonNote text used by the outro call in
configure wizard output so the complete worst-case setup message remains within
the 80-column limit; keep the daemon-enabled status visible by shortening that
note rather than allowing writeLines to hard-truncate it.
- Around line 613-620: Update the failed-install branch in the configure
wizard’s daemon setup flow to write daemonResult.reason to stdout in addition to
the existing hookLogWarn call. Keep the warning log intact, and ensure the
wizard’s own output clearly reports that failproofaid was not installed as a
service and includes the failure reason.
---
Nitpick comments:
In `@__tests__/hooks/configure-wizard.test.ts`:
- Around line 469-496: Extend the reviewLines coverage in the existing test to
mock daemonServiceFilePath with a non-null path for one user-scope case, then
assert the resulting “This will update:” output includes the failproofaid
service file entry. Import daemonServiceFilePath from the existing module
alongside the other test dependencies, while preserving the current
supported-platform and project-scope assertions.
- Around line 405-415: Extend the configure wizard tests around
runConfigureWizard with a malformed global configuration, then assert the
expected unpersisted daemon state after markDaemonConfigured returns without
writing. Also verify the configure_daemon_install telemetry event records the
installed flag and reason payload for this failure path, while preserving the
existing successful-install coverage.
In `@__tests__/hooks/daemon-client.test.ts`:
- Around line 188-200: Add a test alongside the existing malformed-frame case
that starts the test server, reads the request frame, sends only a 4-byte header
declaring a length greater than MAX_FRAME_LEN, and closes the socket without a
payload. Invoke tryDaemonHook with the same representative hook arguments and
assert it returns null, covering rejection during header parsing without
buffering the body.
In `@__tests__/hooks/daemon-service.test.ts`:
- Around line 86-103: Make the dev-build tests deterministic by creating a
temporary package-root fixture containing target/release/failproofaid and
asserting resolveFailproofaidBinaryPath always returns that existing absolute
path. Add a separate fixture covering target/debug/failproofaid when release is
absent, and verify release is selected when both profiles exist. Remove the
conditional assertion and repository-state dependency from the test identified
by resolveFailproofaidBinaryPath.
In `@__tests__/hooks/handler.test.ts`:
- Around line 299-303: Replace the wall-clock Promise.race assertion around
outcomePromise with a deterministic ordering check: await outcomePromise, record
whether releaseTelemetry has run, and assert that flag remains false immediately
after the outcome resolves. Preserve the test’s verification that the outcome
does not wait on trackPromise, without relying on a timeout.
In `@__tests__/hooks/worker-server.test.ts`:
- Around line 174-206: Add a worker-server test that encodes two valid hook
requests, concatenates both frames, and sends them through one socket.write
call; read and assert two hookResult responses with successful exit codes. Place
it near the existing malformed-frame test and reuse the established
request/frame helpers, ensuring the test verifies pipelined frame handling on a
single connection.
In `@bin/failproofai-worker.mjs`:
- Around line 47-51: Update shutdown() to close or destroy active server
connections before or during server.close(), then add a bounded timeout that
forcefully exits if the graceful close callback does not run. Preserve immediate
clean exit when server.close() completes, and ensure the fallback timer cannot
keep the process alive after successful shutdown.
In `@crates/failproofaid/src/paths.rs`:
- Around line 80-81: Update the run-directory creation in ensure_run_dir to use
DirBuilder with DirBuilderExt::mode(0o700) before create, rather than calling
fs::create_dir_all followed by fs::set_permissions. Preserve recursive creation
so every newly created component receives the owner-only mode, and remove the
now-unnecessary post-creation permission update.
In `@crates/failproofaid/src/worker.rs`:
- Around line 213-216: Update the UnixStream setup in the worker connection flow
to call set_write_timeout with the same 30-second duration as set_read_timeout,
propagating failures through WorkerError::Io before write_message can run.
- Around line 71-75: Update the packaged fallback in the WorkerCommand::Node
branch to derive dist/worker.mjs relative to std::env::current_exe() rather than
constructing a cwd-relative PathBuf. Preserve the explicit override behavior
while ensuring the default script resolves from the daemon binary’s directory.
In `@crates/failproofaid/tests/daemon_e2e.rs`:
- Around line 23-36: Update the test cleanup drop guard used with the child
process to also remove the unique parent directory returned by
unique_socket_path, not just terminate the process. Ensure cleanup runs after
every test and removes the directory recursively so stale sockets and lock files
do not remain.
In `@src/hooks/configure-wizard.ts`:
- Around line 275-289: Replace the hand-rolled read/modify/write logic in
markDaemonConfigured with a setter exposed by hooks-config.ts for
daemonConfigured. Have that config-layer setter reuse its existing normalization
and atomic temp-file/rename writing path, while preserving
markDaemonConfigured’s best-effort behavior and early return on configuration
read failure.
In `@src/hooks/daemon-service.ts`:
- Around line 121-139: Update systemdUnitContents to escape workerCmd before
interpolating it into the quoted Environment= line, handling at least
backslashes, double quotes, and newline characters according to systemd unit
escaping rules. Preserve the existing omission of the environment line when
workerCmd is null and use the escaped value for the generated unit.
- Around line 164-167: Update the launchd plist’s KeepAlive configuration near
RunAtLoad to use dictionary form matching systemd’s Restart=on-failure behavior,
so clean exits are not restarted while unexpected failures still trigger
relaunch. Preserve RunAtLoad and the existing daemon service configuration.
- Around line 297-304: Update the launchd status logic in the daemon status
function around launchdPlistPath and launchctl to determine whether the job is
actually running, not merely loaded. Parse the launchctl list output for
ai.failproof.failproofaid and require a valid non-dash PID (or query the job
directly for its active state); return "stopped" when the job is loaded without
a running process, while preserving "not-installed" and error handling.
In `@src/hooks/handler.ts`:
- Line 359: Update the trackHookEvent call producing trackPromise in the hook
handler so the non-awaited path attaches an explicit rejection handler, while
preserving the existing awaited behavior when awaitTelemetryFlush is true.
Ensure any rejection is consumed or logged defensively so the warm worker cannot
receive an unhandled promise rejection.
🪄 Autofix (Beta)
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: 6f897c49-5e02-45f8-99ac-6051103e5947
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lockbun.lockis excluded by!**/*.lock
📒 Files selected for processing (43)
.github/workflows/build-daemon.yml.github/workflows/ci.yml.gitignoreCHANGELOG.mdCargo.toml__tests__/hooks/builtin-policies.test.ts__tests__/hooks/configure-wizard.test.ts__tests__/hooks/daemon-client.test.ts__tests__/hooks/daemon-service.test.ts__tests__/hooks/handler.test.ts__tests__/hooks/worker-server.test.tsbin/failproofai-worker.mjsbin/failproofai.mjsbin/failproofaid-shim.mjscrates/.gitkeepcrates/PROTOCOL.mdcrates/failproofaid/Cargo.tomlcrates/failproofaid/src/lock.rscrates/failproofaid/src/main.rscrates/failproofaid/src/paths.rscrates/failproofaid/src/server.rscrates/failproofaid/src/worker.rscrates/failproofaid/tests/daemon_e2e.rscrates/fpai-ipc/Cargo.tomlcrates/fpai-ipc/src/envelope.rscrates/fpai-ipc/src/framing.rscrates/fpai-ipc/src/lib.rscrates/fpai-ipc/src/peer.rspackage.jsonpackages/failproofaid-darwin-arm64/package.jsonpackages/failproofaid-darwin-x64/package.jsonpackages/failproofaid-linux-arm64/package.jsonpackages/failproofaid-linux-x64/package.jsonrust-toolchain.tomlsrc/hooks/builtin-policies.tssrc/hooks/configure-wizard.tssrc/hooks/daemon-client.tssrc/hooks/daemon-service.tssrc/hooks/handler.tssrc/hooks/normalize-cli-payload.tssrc/hooks/policy-types.tssrc/hooks/read-stdin.tssrc/hooks/worker-server.ts
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@CLAUDE.md`:
- Line 874: Update the rust-quality entry in the CI quality table to remove the
obsolete “gated no-op” qualifier and document cargo fmt --check, cargo clippy,
and cargo test --workspace as active checks, reflecting the existing Rust
workspace manifests.
- Around line 870-871: Insert one blank line between the introductory paragraph
about .github/workflows/ci.yml and the CI jobs table in CLAUDE.md, preserving
the existing paragraph and table content.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
…cement-breaking CodeRabbit's review of #632 surfaced four bugs that each silently defeat enforcement, plus a set of smaller ones. Fixed with regression tests that fail against the old code. **macOS was broken outright.** `run_until` puts the listener in non-blocking mode. Linux discards that on accept (`accept4`); BSD-derived kernels inherit it. So on macOS `read_message` returned `WouldBlock` before the client's bytes landed, `handle_connection` read that as a malformed frame and answered with silence, and every hook call on macOS — a platform this PR ships launchd support for — fell through to the client's fail-closed deny. Accepted streams are now explicitly set blocking. **Worker restart never worked.** A Unix socket file outlives the process that bound it, so `socket_path.exists()` saw the *dead* worker's leftover file the instant a new one spawned, broke out of the wait loop, and handed `call()` a socket nothing was listening on — ECONNREFUSED on every request until the daemon restarted, which is precisely the crash-recovery path the loop exists to provide. Readiness is now a real `connect()`, the stale path is cleared before spawn, and `Drop` cleans up after itself. Both new tests fail on the old code with ECONNREFUSED. **`daemonConfigured` tracked the service manager, not a daemon.** It was granted the moment `systemctl enable --now` / `launchctl load` exited 0 — which a daemon that dies at startup also does — and never revoked on uninstall. Since `bin/failproofai.mjs` fails closed on that flag, either end of the lifecycle left the machine denying every hook event across all 11 CLIs, recoverable only by hand-editing `~/.failproofai/policies-config.json`. Install now waits for the service to reach *and hold* a running state (a `Type=simple` unit reports active the moment it forks, so a single reading waves through exactly the crash-at-startup case), and `uninstallDaemonService` clears the marker first and unconditionally. The marker write moves to `daemon-service.ts` as `setDaemonConfigured` so both ends share one implementation. **A 150ms budget covered policy evaluation.** On a daemon-configured machine a client timeout is a DENY, not a fallback — and that one budget had to cover the whole roundtrip, which `handler.ts` allows 10s per custom policy and `worker-server.ts` serializes. A slow-but-correct verdict produced the same block as a dead daemon, so users would see intermittent denials of legitimate tool calls. Split into a 150ms *connect* probe (a dead daemon still fails fast, adding no latency) and a 30s *response* budget matching worker.rs's own read timeout. Also: - `process.exit()` in the `--hook` path discarded unflushed stdout. Under every agent CLI that stdout is a pipe, so writes are async and exit drops what's buffered — measured: 2 MB written, 146 KB delivered. That truncates the decision payload the CLI parses, and on the fail-closed path drops the deny reason entirely. Both hook paths now drain first. - The worker's piped stdout/stderr were never read. The worker runs real policy code for the daemon's whole life, so a chatty custom policy eventually fills the pipe buffer and blocks it mid-write — every later hook call fails closed. Both pipes now drain on background threads into the daemon's own stderr, where systemd/launchd already capture it. - `worker-server.ts` decoded one frame per `data` event, stranding the second of two coalesced requests until a third write arrived. - Connections had no read/write deadline and no cap: a peer that connected and sent nothing held a thread for the daemon's lifetime. Now a 10s deadline plus a 64-connection ceiling. - Every `systemctl`/`launchctl` call was unbounded, so a wedged user session hung the wizard silently after the user pressed apply. - Daemon-install telemetry sent `err.message` verbatim — for writeFileSync/execFileSync failures that is an errno string carrying a `homedir()`-derived absolute path, i.e. the OS username. Only a bounded classification leaves the machine now; the full text stays in the local log. - The e2e harness piped the daemon's output without draining it (same buffer-fill hang) and orphaned a live daemon on any assertion panic. A `DaemonGuard` kills and reaps on drop, startup polls `try_wait()`, and failures report the daemon's own stderr. CI: - `darwin-x64` used the retired `macos-13` label. An unknown label doesn't fail — it never gets a runner, which is why that leg has been pending since the PR opened. Now `macos-15-intel`. - The release-artifact job shared a writable cargo cache between `pull_request` and `release` triggers, so a PR branch could seed an entry a later release run restores into a published binary. Restore-only on PRs, save on release/dispatch. - `persist-credentials: false` on the two checkouts that then compile third-party crates, so build scripts can't read GITHUB_TOKEN out of `.git/config`. Verified end to end against the real compiled daemon (deny, allow, worker-killed-mid-life restart, daemon-down fail-closed), plus `cargo test --workspace`, `bun run test:run`, `bun run test:e2e`, lint and tsc. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01B3D1CyJQDbxM25cNBgRrH7
|
@SocketSecurity ignore cargo/zerocopy@0.8.55 Verified false positive, and it never reaches users.
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/hooks/daemon-service.ts (1)
422-433: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winCheck the
PIDfield forlaunchctlstatus, not label presence.
launchctl liststill lists a loaded launchd job after it exits, and the first column is-rather than a running PID.daemonServiceStatus()currently reports"running"for that case, sowaitForDaemonRunning()can treat an immediately crashed daemon as healthy and setdaemonConfiguredtotrue, causing hook events to fail closed.Parse
ai.failproof.failproofaid’s own line and return"running"only when the PID field is not-; handle labels that include the service string as a prefix or substring.🤖 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/daemon-service.ts` around lines 422 - 433, Update daemonServiceStatus() to parse the ai.failproof.failproofaid launchctl list entry and inspect its first PID field, returning "running" only when that field is not "-". Match labels containing the service identifier as a prefix or substring, and return "stopped" when the entry is absent or has no running PID..github/workflows/build-daemon.yml (2)
97-101: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPreserve executable permissions before artifact upload.
actions/upload-artifact@v4does not preserve file modes through ZIP packaging, so later workflow consumers can unpack the binary without execute permission. Upload a preserved archive or reapply the execute bit before packaging/npm publish.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/build-daemon.yml around lines 97 - 101, Update the artifact upload step for the failproofaid binary in the workflow to preserve executable permissions, using a permissions-preserving archive before actions/upload-artifact@v4 or explicitly restoring the execute bit after extraction before packaging or npm publish. Keep the existing matrix-specific artifact naming and binary path behavior intact.
90-95: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winCompare
failproofaid --versionwith the package version before uploading.
packages/failproofaid-${{ matrix.platform }}has the release version in itspackage.json. A successfulfailproofaid --versioncall does not prove the staged artifact matches that same release version, so capture the command output and assert it beforeupload-artifact.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/build-daemon.yml around lines 90 - 95, Update the staging step for the failproofaid binary to read the release version from the platform package’s package.json, capture the output of failproofaid --version, and assert that it matches before the artifact upload step. Keep the existing executable check and staging paths unchanged.
♻️ Duplicate comments (1)
.github/workflows/build-daemon.yml (1)
44-46: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winMove the arm64 build off
macos-14.GitHub began deprecating the macOS 14 image on July 6, 2026. It becomes unsupported on November 2, 2026. A release after that date will not schedule this leg. Use the pinned
macos-15arm64 label instead. (docs.github.com)Suggested change
- os: macos-14 + os: macos-15🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/build-daemon.yml around lines 44 - 46, Update the arm64 build matrix entry for target aarch64-apple-darwin to use the pinned macos-15 runner instead of macos-14, while preserving its darwin-arm64 platform setting.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/build-daemon.yml:
- Around line 77-78: Update the release build step named “cargo build --release”
to pass Cargo’s --locked flag alongside the existing target and package options,
ensuring it uses the committed Cargo.lock without modifying dependency
resolution.
---
Outside diff comments:
In @.github/workflows/build-daemon.yml:
- Around line 97-101: Update the artifact upload step for the failproofaid
binary in the workflow to preserve executable permissions, using a
permissions-preserving archive before actions/upload-artifact@v4 or explicitly
restoring the execute bit after extraction before packaging or npm publish. Keep
the existing matrix-specific artifact naming and binary path behavior intact.
- Around line 90-95: Update the staging step for the failproofaid binary to read
the release version from the platform package’s package.json, capture the output
of failproofaid --version, and assert that it matches before the artifact upload
step. Keep the existing executable check and staging paths unchanged.
In `@src/hooks/daemon-service.ts`:
- Around line 422-433: Update daemonServiceStatus() to parse the
ai.failproof.failproofaid launchctl list entry and inspect its first PID field,
returning "running" only when that field is not "-". Match labels containing the
service identifier as a prefix or substring, and return "stopped" when the entry
is absent or has no running PID.
---
Duplicate comments:
In @.github/workflows/build-daemon.yml:
- Around line 44-46: Update the arm64 build matrix entry for target
aarch64-apple-darwin to use the pinned macos-15 runner instead of macos-14,
while preserving its darwin-arm64 platform setting.
🪄 Autofix (Beta)
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: d1072649-f686-4a61-9c87-e18c2d77ad30
📒 Files selected for processing (16)
.github/workflows/build-daemon.yml.github/workflows/ci.ymlCHANGELOG.mdCLAUDE.md__tests__/hooks/configure-wizard.test.ts__tests__/hooks/daemon-client.test.ts__tests__/hooks/daemon-service.test.ts__tests__/hooks/worker-server.test.tsbin/failproofai.mjscrates/failproofaid/src/server.rscrates/failproofaid/src/worker.rscrates/failproofaid/tests/daemon_e2e.rssrc/hooks/configure-wizard.tssrc/hooks/daemon-client.tssrc/hooks/daemon-service.tssrc/hooks/worker-server.ts
🚧 Files skipped from review as they are similar to previous changes (9)
- bin/failproofai.mjs
- CLAUDE.md
- CHANGELOG.md
- tests/hooks/configure-wizard.test.ts
- crates/failproofaid/tests/daemon_e2e.rs
- tests/hooks/daemon-client.test.ts
- .github/workflows/ci.yml
- src/hooks/worker-server.ts
- crates/failproofaid/src/worker.rs
…ef-aware (#634) * ci: publish the failproofaid binaries and make releases ref-aware The daemon split (#632) added every packaging input — the platform manifests, the pinned optional dependencies, and a 4-way cross-compile matrix — but never touched publish.yml, so every release built four binaries as Actions artifacts and threw them away with the runner. CI was green throughout, because nothing checks that what gets built also gets shipped. Rework publish.yml into four jobs: preflight (version/dist-tag resolution, npm credential check, daemon detection), daemon (calls build-daemon.yml, now a reusable workflow), release-assets (downloads the artifacts, assembles SHA256SUMS, attaches both to the release), and publish (npm + aliases). The assets land BEFORE the npm publish because the installed CLI downloads its daemon from that release tag — publish the package first and its binary does not exist yet. Two guards make a branch dispatch safe. The version bump checks main out and pushes to it, so it now runs only for a release or a dispatch from main; a dispatch from a feature branch would otherwise rewrite main's version line to whatever that branch carries. And `latest` is refused from a non-main dispatch, with `auto` resolving to `next` there, so a branch build cannot move a dist-tag that a later release from main would then move backwards. Everything binary-related is gated on the ref actually carrying a Rust workspace, so this is a no-op on main until #632 lands: both daemon jobs skip and the npm publish behaves exactly as before. build-daemon.yml comes along inert for the same reason — it gates its matrix on a detect job, which is also what stops its own path filter from running `cargo build` against a checkout with no crates. Also adds a dry-run input (builds, checksums and `npm publish --dry-run` while writing nothing), fixes publish.yml's bun cache key, which hashed a `bun.lockb` this repo does not track, and adds a tripwire test over both workflows, since a hand-maintained pipeline that silently drops its own build artifacts is exactly what just happened. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EbXKFnpRXBwsrvUJDdySky * docs(changelog): add the release-pipeline entry Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EbXKFnpRXBwsrvUJDdySky * fix: address release workflow review findings --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Stage 1 of the failproofai/failproofaid split: an empty Cargo workspace
plus a rust-quality CI job gated on crates/*/Cargo.toml existing, so it
goes green before any daemon code lands. Also fixes the bun cache key
(hashFiles('bun.lockb') has silently never matched anything since this
repo tracks bun.lock, not bun.lockb) and extends the version-consistency
check to cover the new Cargo workspace version.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Stage 2: crates/fpai-ipc (length-prefixed JSON framing, the ping/hook envelope, SO_PEERCRED/getpeereid peer verification) and crates/failproofaid (a Unix-socket server binding ~/.failproofai/run/failproofaid.sock at 0600 inside a 0700 dir, a flock-based singleton guard, and graceful SIGTERM shutdown). Hook requests get a stub "not implemented" error for now -- wiring them to a real warm Node/Bun worker is Stage 3. 28 Rust tests (unit + black-box binary integration tests spawning the real compiled binary), plus manual end-to-end verification against an independent Python client exercising ping/pong, the stub hook response, protocol-version mismatch, malformed frames, and an oversized length prefix. Testing caught two real bugs before they shipped: serde's rename_all on an enum only renames variant tags, not struct-variant field names (protocolVersion was silently serializing as protocol_version), and ensure_run_dir was unconditionally chmod-ing whatever directory FAILPROOFAI_DAEMON_SOCKET's parent resolved to -- now it refuses to touch a pre-existing directory it didn't create itself. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…d path
Stage 3: the full daemon-aware hook path, end to end.
TypeScript side:
- handler.ts: extract the core evaluation logic into evaluateHookEvent(),
which takes the stdin payload as a parameter and returns
{exitCode,stdout,stderr} instead of touching process.stdin/stdout
directly -- callable repeatedly inside a long-lived worker process.
handleHookEvent() keeps its exact existing signature/behavior as a thin
wrapper, so the 1300+ line existing test suite needed zero changes.
Adds a forceDecision option that registers a single synthetic policy and
runs it through the real, unmodified evaluatePolicies() -- the
fail-closed path gets correct per-CLI response shaping (Cursor's flat
continue:false, Factory's exit-2, ...) for free, with no duplicated
shaping logic. Also closes the process.cwd() hazard: a fallbackCwd
option lets a warm worker use the *originating* CLI's cwd instead of its
own fixed one when a payload omits cwd.
- worker-server.ts + bin/failproofai-worker.mjs: the Node/Bun process
failproofaid spawns. Listens on its own Unix socket (not stdio -- a
stray console.log from a user's custom policy must never desync a
shared framed channel), processing requests strictly sequentially so
the globalThis-backed policy registry stays correct with zero changes.
- daemon-client.ts: the thin client, real net.Server-tested framing
matching crates/PROTOCOL.md exactly. ~150ms connect+roundtrip budget,
null on any failure (no partial trust). isDaemonConfigured() gates the
whole path on a new HooksConfig.daemonConfigured marker (global scope
only) that nothing sets until Stage 4 -- inert on every machine today.
- bin/failproofai.mjs: daemon-attempt-then-fail-closed wiring, byte-for-
byte unchanged when not daemon-configured.
- builtin-policies.ts: fixes the git-branch cache for a warm process --
it previously never actually cached anything (one-shot process), and
reusing it unconditionally across many calls would silently serve a
stale branch after a checkout. Now gated on .git/HEAD's mtime.
Rust side:
- worker.rs: spawns and supervises the worker, relays Hook requests to
it, translating between the worker-facing protocol (no protocolVersion
-- this process always spawns a version-matched worker) and the
client-facing one (which does).
- server.rs: Hook requests now relay through the worker instead of
returning a stub error; any worker failure becomes a client Error
response, never a hang.
A live end-to-end test (real failproofaid, real bun-spawned worker, real
block-sudo policy) surfaced a genuine process-leak bug during development:
`sh -c "bun ..."` isn't guaranteed to exec(2) in place, so killing only
the tracked PID could leave a live grandchild worker process orphaned --
reproduced live as three orphaned failproofai-worker.mjs processes still
running minutes later. Fixed with process groups (spawn in a new group,
kill the whole group) and piped rather than inherited stdio (an inherited
fd on a worker that outlives its intended lifetime keeps a wrapping
shell's pipe from ever seeing EOF). Test infrastructure also gained an
RAII guard so a failing assertion cleans up its spawned worker exactly as
reliably as a passing one.
29 Rust tests, all passing worker-server.ts/daemon-client.ts tests against
real sockets (not mocks), full existing TS suite green.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The live end-to-end test in crates/failproofaid/src/server.rs spawns the
real TS worker via `bun bin/failproofai-worker.mjs`, but the rust-quality
job only ever set up the Rust toolchain -- bun was never on PATH there.
Failed on real CI with exit status 127 ("worker process exited before
creating its socket") even though it passed locally, where bun happens
to already be installed.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ofai config
Stage 4: no separate `failproofai daemon install` command (explicitly
rejected during planning) -- `daemon-service.ts` is a small install/
uninstall/status engine that `configure-wizard.ts` calls directly, the
same relationship `config` already has with manager.ts's installHooks().
- daemon-service.ts: writes + enables a systemd --user unit on Linux
(~/.config/systemd/user/failproofaid.service) or a launchd LaunchAgent
on macOS (~/Library/LaunchAgents/ai.failproof.failproofaid.plist).
resolveFailproofaidBinaryPath() points ExecStart/ProgramArguments at
the real compiled binary directly -- never the eventual JS bin shim,
which only exists for a user invoking `failproofaid` by hand, not for
what a service manager should supervise. Resolves via (in order) an
explicit test/dev override, the future @failproofai/failproofaid-<os>-
<arch> npm package, or a locally-built target/{release,debug}/
failproofaid -- so this already works against this session's own
cargo-built binary before Stage 5's packaging exists.
- configure-wizard.ts: when the global ("Everywhere I code") scope is
chosen on a supported platform, the wizard installs/starts the service
and writes the daemonConfigured marker unconditionally -- no separate
toggle, matching the product decision that this isn't an opt-in extra.
A failed install never fails the wizard: the rest of setup already
applied, and the machine simply stays on the in-process path since the
marker is only set on success. The review screen lists the exact
service file that's about to be written, alongside every other file
the wizard already shows.
Verified against a REAL systemd --user session (this sandbox has one) --
install, confirm `running`, uninstall, confirm fully removed, with the
test backing up and restoring any real pre-existing unit rather than
assuming a clean slate. macOS/launchd is unit-tested (plist content
generation, path resolution) but not live-verified -- no macOS available
here.
Also closes a real safety gap caught while adding these tests:
configure-wizard.test.ts already drives the wizard through scope "user"
in several existing cases, and without mocking daemon-service.ts those
tests would have shelled out to the real systemctl on whatever machine
runs them.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ound via Docker verification
Adds per-platform failproofaid binary packages distributed via
optionalDependencies (packages/failproofaid-{linux,darwin}-{x64,arm64}),
a failproofaid-shim.mjs bin entry that resolves and execs the right one,
and a build-daemon.yml cross-compile matrix (4 real targets, staged into
packages/*/bin/ for release). Bumps to 1.0.0-beta.0 across every
version-carrying file, per explicit instruction — this is the daemon
split, a major behavioral change on supported platforms.
Two real bugs surfaced by driving a real Docker clean-install + a live
daemon/worker relay end-to-end (not just unit tests):
- The worker was spawned lazily on the first real request, taking ~700ms
to cold-start — well past daemon-client.ts's 150ms fail-closed budget,
so the very first hook call after every daemon (re)start failed closed
even though the daemon was healthy. Fixed by pre-warming the worker in
a background thread right after the daemon binds its socket
(worker.rs, main.rs).
- Every deny/instruct decision unconditionally awaited a live PostHog
network POST before returning, regardless of opts.awaitTelemetryFlush
— the warm worker passes that flag specifically to avoid this, but the
inline telemetry call at handler.ts's "decisions that affect Claude's
behavior" block never checked it. This made every real policy block
through the daemon pay a live network round-trip (or up to 5s when
PostHog is unreachable), which is precisely the enforcement path that
most needs to be fast and reliable. Fixed to respect the same opt-out
flushHookTelemetry() already honors, with a regression test.
Also resolves the version-bumped Rust binary path (daemon-service.ts's
resolveWorkerCommand()/Environment= threading) once more against the new
version to confirm the fix still holds.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Corrects the CI job table (8 jobs across ci.yml, not the stale 4-job list, plus the separate path-filtered build-daemon.yml cross-compile matrix), adds the new Rust crates/daemon files to the project-structure cheatsheet, and records the deliberate decision to keep this repo's own dogfood hook configs on the in-process path rather than daemon-configured. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…cement-breaking CodeRabbit's review of #632 surfaced four bugs that each silently defeat enforcement, plus a set of smaller ones. Fixed with regression tests that fail against the old code. **macOS was broken outright.** `run_until` puts the listener in non-blocking mode. Linux discards that on accept (`accept4`); BSD-derived kernels inherit it. So on macOS `read_message` returned `WouldBlock` before the client's bytes landed, `handle_connection` read that as a malformed frame and answered with silence, and every hook call on macOS — a platform this PR ships launchd support for — fell through to the client's fail-closed deny. Accepted streams are now explicitly set blocking. **Worker restart never worked.** A Unix socket file outlives the process that bound it, so `socket_path.exists()` saw the *dead* worker's leftover file the instant a new one spawned, broke out of the wait loop, and handed `call()` a socket nothing was listening on — ECONNREFUSED on every request until the daemon restarted, which is precisely the crash-recovery path the loop exists to provide. Readiness is now a real `connect()`, the stale path is cleared before spawn, and `Drop` cleans up after itself. Both new tests fail on the old code with ECONNREFUSED. **`daemonConfigured` tracked the service manager, not a daemon.** It was granted the moment `systemctl enable --now` / `launchctl load` exited 0 — which a daemon that dies at startup also does — and never revoked on uninstall. Since `bin/failproofai.mjs` fails closed on that flag, either end of the lifecycle left the machine denying every hook event across all 11 CLIs, recoverable only by hand-editing `~/.failproofai/policies-config.json`. Install now waits for the service to reach *and hold* a running state (a `Type=simple` unit reports active the moment it forks, so a single reading waves through exactly the crash-at-startup case), and `uninstallDaemonService` clears the marker first and unconditionally. The marker write moves to `daemon-service.ts` as `setDaemonConfigured` so both ends share one implementation. **A 150ms budget covered policy evaluation.** On a daemon-configured machine a client timeout is a DENY, not a fallback — and that one budget had to cover the whole roundtrip, which `handler.ts` allows 10s per custom policy and `worker-server.ts` serializes. A slow-but-correct verdict produced the same block as a dead daemon, so users would see intermittent denials of legitimate tool calls. Split into a 150ms *connect* probe (a dead daemon still fails fast, adding no latency) and a 30s *response* budget matching worker.rs's own read timeout. Also: - `process.exit()` in the `--hook` path discarded unflushed stdout. Under every agent CLI that stdout is a pipe, so writes are async and exit drops what's buffered — measured: 2 MB written, 146 KB delivered. That truncates the decision payload the CLI parses, and on the fail-closed path drops the deny reason entirely. Both hook paths now drain first. - The worker's piped stdout/stderr were never read. The worker runs real policy code for the daemon's whole life, so a chatty custom policy eventually fills the pipe buffer and blocks it mid-write — every later hook call fails closed. Both pipes now drain on background threads into the daemon's own stderr, where systemd/launchd already capture it. - `worker-server.ts` decoded one frame per `data` event, stranding the second of two coalesced requests until a third write arrived. - Connections had no read/write deadline and no cap: a peer that connected and sent nothing held a thread for the daemon's lifetime. Now a 10s deadline plus a 64-connection ceiling. - Every `systemctl`/`launchctl` call was unbounded, so a wedged user session hung the wizard silently after the user pressed apply. - Daemon-install telemetry sent `err.message` verbatim — for writeFileSync/execFileSync failures that is an errno string carrying a `homedir()`-derived absolute path, i.e. the OS username. Only a bounded classification leaves the machine now; the full text stays in the local log. - The e2e harness piped the daemon's output without draining it (same buffer-fill hang) and orphaned a live daemon on any assertion panic. A `DaemonGuard` kills and reaps on drop, startup polls `try_wait()`, and failures report the daemon's own stderr. CI: - `darwin-x64` used the retired `macos-13` label. An unknown label doesn't fail — it never gets a runner, which is why that leg has been pending since the PR opened. Now `macos-15-intel`. - The release-artifact job shared a writable cargo cache between `pull_request` and `release` triggers, so a PR branch could seed an entry a later release run restores into a published binary. Restore-only on PRs, save on release/dispatch. - `persist-credentials: false` on the two checkouts that then compile third-party crates, so build scripts can't read GITHUB_TOKEN out of `.git/config`. Verified end to end against the real compiled daemon (deny, allow, worker-killed-mid-life restart, daemon-down fail-closed), plus `cargo test --workspace`, `bun run test:run`, `bun run test:e2e`, lint and tsc. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01B3D1CyJQDbxM25cNBgRrH7
Same subject as the surrounding sentence (hardening the job that produces the binary users install), so it belongs in that bullet rather than a new one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01B3D1CyJQDbxM25cNBgRrH7
… packages The four `@failproofai/failproofaid-<os>-<arch>` packages were the plan of record — declared as optionalDependencies, pinned to the root version, built by a 4-way cross-compile — but nothing ever published them. The workflow only uploaded the binaries as Actions artifacts and publish.yml was never touched at all, so all four names 404 on npm today and a released CLI would have resolved a daemon that does not exist. #634 fixes the pipeline either way; this removes the channel it would have had to publish, because the release assets have to exist regardless for anyone installing failproofaid on its own, and a second channel is a second thing to keep in step with the first. daemon-download.ts fetches `failproofaid-<os>-<arch>.gz` from the release tagged with this CLI's own version, verifies it against the published SHA256SUMS *before* decompressing, and installs it to `~/.failproofai/bin/failproofaid-<version>` by atomic rename, mode 0755. The URL is constructed from package.json's version rather than discovered through the API: no rate limit, no `releases/latest` redirect, and no way to run a daemon built from different source than the CLI talking to it. The versioned filename is what keeps an upgrade from overwriting a running binary (ETXTBSY) or silently repointing a live service unit. A bad checksum, a missing manifest entry and a failed fetch are refusals rather than warnings — what this writes is an executable a service manager runs at login. Only the install path downloads; resolveFailproofaidBinaryPath() stays a pure disk check, so the hook path can never block on the network. FAILPROOFAI_NO_DOWNLOAD=1 opts an air-gapped machine out while leaving an already-installed binary working, and FAILPROOFAI_DAEMON_BASE_URL points at an internal mirror (and at a local server in the tests). build-daemon.yml matches #634's copy so the rebase is a no-op: it gzips each binary and uploads that, which is also what makes the artifact's lost executable bit a non-issue. Verified: 13 new download tests against a real local HTTP server covering checksum mismatch, a manifest with no entry, a 404, the disabled-downloads opt-out and the atomic install; the daemon-service resolution tests now run against a scratch HOME so a developer machine with a real daemon cannot flake them; and a clean `npm pack` + container run confirms the shim reports the config hint with nothing installed and execs an overridden binary. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EbXKFnpRXBwsrvUJDdySky
a5003ef to
bd6ed8f
Compare
Two defects the 1.0.0-beta.0 release surfaced on real machines. The Linux binaries linked against the build runner's glibc, and `ubuntu-latest` is now 24.04 (glibc 2.39), so they refused to start on Ubuntu 22.04, Debian 12, RHEL 9 and Amazon Linux 2023 — measured against real containers, not predicted. Both Linux legs now target `*-unknown-linux-musl` and link statically, which has no glibc floor at all; an older runner would only move the floor (22.04 is 2.35, still above RHEL 9's 2.34). The build asserts staticness on the artifact itself, because a "static" build that came out dynamic still runs on the runner that made it and fails only on the users' distros. The service was a systemd --user unit, which only runs while its user manager does: without lingering that manager does not start at boot and stops with the last session. So the daemon died on logout — and on a daemon-configured machine an unreachable daemon fails closed, so any agent running without a login session (detached tmux, cron, a CI runner) then hit denials. It is now `/etc/systemd/system/failproofaid@<user>.service` with `User=<user>` and `WantedBy=multi-user.target`, enabled via `systemctl enable --now`; macOS moves from a LaunchAgent to a LaunchDaemon with `UserName`. Root-installed, never root-run: everything it touches still lives in one user's home and is peer-checked against that uid. The two costs of system scope are handled rather than assumed. Install needs root, so `canElevate()` probes `sudo -n` BEFORE writing anything and, failing that, returns the exact commands to run (classified as `needs_root`) instead of half-installing — never an interactive prompt, which would be unreadable under the wizard's TUI. And a system unit inherits no login environment, so `FAILPROOFAI_WORKER_CMD` now names an absolute runtime via `process.execPath`: a bare `node` resolves for the wizard and then fails inside the service on every nvm install, silently, which is the same class of bug CLAUDE.md documents for the dev hook. Any pre-existing user-scope daemon is stopped and removed on both install and uninstall. It holds the same singleton flock, so leaving one behind would make the new service lose the race and leave the machine fail-closed against a daemon that never came up. The unit is named per user so a second person on the same box cannot silently steal the first's service; every field in it is user-specific anyway. Status needs no privileges — `systemctl status failproofaid@<user>`, exposed as `daemonStatusCommand()`. No version bump here: `block-version-bumps` reserves that for a `luv-cut-*` branch, which is where 1.0.0-beta.1 gets cut. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EbXKFnpRXBwsrvUJDdySky
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
.github/workflows/ci.yml (1)
91-150: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAdd explicit least-privilege permissions to
rust-quality.This job runs
cargo clippy/cargo testover the full dependency tree, which executes third-party build scripts, as the comment at Line 96-99 itself notes. The job has nopermissions:block, so it falls back to whatever defaultGITHUB_TOKENscope the org/repo enforces, which can be broad.persist-credentials: falseprevents the checked-out credential from lingering in.git/config, but it does not scope the token the job's own steps receive from the runner environment.Add a job-level
permissions:block scoped to what this job actually needs (likely justcontents: read, since it doesn't push or open PRs).🔒️ Proposed fix: scope job permissions
rust-quality: runs-on: ubuntu-latest + permissions: + contents: read steps: - uses: actions/checkout@v7.0.1🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/ci.yml around lines 91 - 150, Add a job-level permissions block to the rust-quality job granting only contents: read. Keep persist-credentials: false and all existing checkout, setup, cache, formatting, lint, and test steps unchanged.Source: Linters/SAST tools
__tests__/hooks/daemon-download.test.ts (1)
224-233: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAssert that no fetch happens.
The test name states that the code does not reach the network, but the assertions only check the returned error text. A future reordering that fetches before the
FAILPROOFAI_NO_DOWNLOADcheck would still pass. Spy onfetchto assert the claim in the name.💚 Proposed assertion
process.env.FAILPROOFAI_DAEMON_BASE_URL = "http://127.0.0.1:1/never"; process.env.FAILPROOFAI_NO_DOWNLOAD = "1"; + const fetchSpy = vi.spyOn(globalThis, "fetch"); const { downloadFailproofaidBinary } = await import("../../src/hooks/daemon-download"); const result = await downloadFailproofaidBinary("linux-x64"); expect(result.error).toContain("downloads are disabled"); expect(result.path).toBeUndefined(); + expect(fetchSpy).not.toHaveBeenCalled(); + fetchSpy.mockRestore();🤖 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/daemon-download.test.ts` around lines 224 - 233, Update the “does not reach the network at all when downloads are disabled” test to spy on the global fetch implementation before calling downloadFailproofaidBinary, then assert it was not called while preserving the existing result assertions. Restore the spy afterward to avoid affecting other tests.src/hooks/daemon-service.ts (1)
114-118: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCheck that the override path exists.
FAILPROOFAI_DAEMON_BINARYis returned without anexistsSynccheck, while the other two candidates are checked. A stale override then produces a service unit that points at a missing file, and the failure surfaces later as a start timeout instead of a clear reason.♻️ Proposed check
- if (process.env.FAILPROOFAI_DAEMON_BINARY) return process.env.FAILPROOFAI_DAEMON_BINARY; + const override = process.env.FAILPROOFAI_DAEMON_BINARY; + if (override && existsSync(override)) return override;🤖 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/daemon-service.ts` around lines 114 - 118, Update resolveFailproofaidBinaryPath to validate FAILPROOFAI_DAEMON_BINARY with existsSync before returning it; only use the override when it points to an existing file, otherwise continue to the installedBinaryPath candidate and its existing check.src/hooks/daemon-download.ts (2)
85-89: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueBound the response size.
fetchBytesbuffers the whole body in memory with no size limit. The timeout limits the duration, not the volume, so a misconfigured mirror or a wrong URL can drive a large allocation inside the config wizard. The expected asset is about 2 MB.Reject a
content-lengthabove a ceiling, and stop reading once the ceiling is passed.♻️ Proposed size guard
+const MAX_DOWNLOAD_BYTES = 64 * 1024 * 1024; + async function fetchBytes(url: string): Promise<Buffer> { const response = await fetch(url, { signal: AbortSignal.timeout(DOWNLOAD_TIMEOUT_MS) }); if (!response.ok) throw new Error(`GET ${url} returned ${response.status}`); - return Buffer.from(await response.arrayBuffer()); + const declared = Number(response.headers.get("content-length") ?? Number.NaN); + if (Number.isFinite(declared) && declared > MAX_DOWNLOAD_BYTES) { + throw new Error(`GET ${url} declared ${declared} bytes, over the ${MAX_DOWNLOAD_BYTES} byte limit`); + } + const bytes = Buffer.from(await response.arrayBuffer()); + if (bytes.length > MAX_DOWNLOAD_BYTES) { + throw new Error(`GET ${url} returned ${bytes.length} bytes, over the ${MAX_DOWNLOAD_BYTES} byte limit`); + } + return bytes; }🤖 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/daemon-download.ts` around lines 85 - 89, Update fetchBytes to enforce a response-size ceiling appropriate for the expected roughly 2 MB asset: reject responses whose content-length exceeds the ceiling, and read the response incrementally while aborting or throwing as soon as accumulated bytes exceed it instead of buffering the entire body unbounded.
47-49: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winValidate the scheme of the mirror override.
FAILPROOFAI_DAEMON_BASE_URLis used without validation. A mirror set tohttp://downloads an executable over a channel with no transport integrity. The SHA-256 check does not close this gap, becauseSHA256SUMSis fetched from the same base URL, so anyone able to modify the asset can also modify the manifest.Restrict the override to
https:(andhttp://127.0.0.1/localhostfor tests), or require an explicit opt-in variable for plain HTTP.🔒 Proposed scheme check
function baseUrl(): string { - return (process.env.FAILPROOFAI_DAEMON_BASE_URL || DEFAULT_BASE_URL).replace(/\/+$/, ""); + const override = process.env.FAILPROOFAI_DAEMON_BASE_URL; + if (!override) return DEFAULT_BASE_URL; + const trimmed = override.replace(/\/+$/, ""); + try { + const url = new URL(trimmed); + const localhost = url.hostname === "127.0.0.1" || url.hostname === "localhost"; + if (url.protocol === "https:" || localhost) return trimmed; + } catch { + /* fall through to the default below */ + } + return DEFAULT_BASE_URL; }🤖 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/daemon-download.ts` around lines 47 - 49, Update baseUrl() to parse and validate FAILPROOFAI_DAEMON_BASE_URL before using it, permitting only https URLs plus http://127.0.0.1 and http://localhost for tests; reject other HTTP or unsupported schemes rather than silently accepting them. Preserve DEFAULT_BASE_URL behavior when no override is configured.
🤖 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/daemon-service.ts`:
- Around line 171-187: Update resolveWorkerCommand so the executable and
workerScript paths are safe when the command is launched through sh -c,
preserving each path as a single argument even when it contains spaces or
shell-special characters. Use the project’s existing argv or shell-escaping
approach before returning the command, while keeping the environment override
and missing-script behavior unchanged.
- Around line 227-236: Make the launchd service label and system plist path
user-specific, following the per-user naming approach in systemdUnitName(), so
installs from different users cannot overwrite or remove each other’s daemons.
Update launchdPlistPath(), daemonStatusCommand(), launchdPlistContents(), and
daemonServiceStatus() to use the namespaced label, while keeping
legacyLaunchAgentPlistPath() on the original unsuffixed LAUNCHD_LABEL for
migration.
- Around line 291-299: Update writePrivilegedFile to create a unique private
staging directory with mkdtempSync under tmpdir(), ensuring it has 0700
permissions, then write the temporary file inside that directory before invoking
runPrivileged. Keep cleanup in the finally block and remove the entire staging
directory after installation.
---
Nitpick comments:
In `@__tests__/hooks/daemon-download.test.ts`:
- Around line 224-233: Update the “does not reach the network at all when
downloads are disabled” test to spy on the global fetch implementation before
calling downloadFailproofaidBinary, then assert it was not called while
preserving the existing result assertions. Restore the spy afterward to avoid
affecting other tests.
In @.github/workflows/ci.yml:
- Around line 91-150: Add a job-level permissions block to the rust-quality job
granting only contents: read. Keep persist-credentials: false and all existing
checkout, setup, cache, formatting, lint, and test steps unchanged.
In `@src/hooks/daemon-download.ts`:
- Around line 85-89: Update fetchBytes to enforce a response-size ceiling
appropriate for the expected roughly 2 MB asset: reject responses whose
content-length exceeds the ceiling, and read the response incrementally while
aborting or throwing as soon as accumulated bytes exceed it instead of buffering
the entire body unbounded.
- Around line 47-49: Update baseUrl() to parse and validate
FAILPROOFAI_DAEMON_BASE_URL before using it, permitting only https URLs plus
http://127.0.0.1 and http://localhost for tests; reject other HTTP or
unsupported schemes rather than silently accepting them. Preserve
DEFAULT_BASE_URL behavior when no override is configured.
In `@src/hooks/daemon-service.ts`:
- Around line 114-118: Update resolveFailproofaidBinaryPath to validate
FAILPROOFAI_DAEMON_BINARY with existsSync before returning it; only use the
override when it points to an existing file, otherwise continue to the
installedBinaryPath candidate and its existing check.
🪄 Autofix (Beta)
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: 15d1ee1b-6176-4949-a75b-ae2def60f9b4
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (42)
.github/workflows/build-daemon.yml.github/workflows/ci.yml.gitignoreCHANGELOG.mdCLAUDE.mdCargo.toml__tests__/hooks/builtin-policies.test.ts__tests__/hooks/configure-wizard.test.ts__tests__/hooks/daemon-client.test.ts__tests__/hooks/daemon-download.test.ts__tests__/hooks/daemon-service.test.ts__tests__/hooks/handler.test.ts__tests__/hooks/worker-server.test.tsbin/failproofai-worker.mjsbin/failproofai.mjsbin/failproofaid-shim.mjscrates/.gitkeepcrates/PROTOCOL.mdcrates/failproofaid/Cargo.tomlcrates/failproofaid/src/lock.rscrates/failproofaid/src/main.rscrates/failproofaid/src/paths.rscrates/failproofaid/src/server.rscrates/failproofaid/src/worker.rscrates/failproofaid/tests/daemon_e2e.rscrates/fpai-ipc/Cargo.tomlcrates/fpai-ipc/src/envelope.rscrates/fpai-ipc/src/framing.rscrates/fpai-ipc/src/lib.rscrates/fpai-ipc/src/peer.rspackage.jsonrust-toolchain.tomlsrc/hooks/builtin-policies.tssrc/hooks/configure-wizard.tssrc/hooks/daemon-client.tssrc/hooks/daemon-download.tssrc/hooks/daemon-service.tssrc/hooks/handler.tssrc/hooks/normalize-cli-payload.tssrc/hooks/policy-types.tssrc/hooks/read-stdin.tssrc/hooks/worker-server.ts
🚧 Files skipped from review as they are similar to previous changes (31)
- .gitignore
- crates/failproofaid/src/main.rs
- src/hooks/builtin-policies.ts
- src/hooks/worker-server.ts
- src/hooks/read-stdin.ts
- src/hooks/configure-wizard.ts
- crates/fpai-ipc/src/lib.rs
- bin/failproofai-worker.mjs
- crates/fpai-ipc/src/peer.rs
- rust-toolchain.toml
- crates/failproofaid/src/paths.rs
- tests/hooks/handler.test.ts
- src/hooks/daemon-client.ts
- crates/failproofaid/Cargo.toml
- src/hooks/policy-types.ts
- crates/fpai-ipc/Cargo.toml
- Cargo.toml
- crates/failproofaid/src/worker.rs
- bin/failproofai.mjs
- package.json
- tests/hooks/daemon-client.test.ts
- bin/failproofaid-shim.mjs
- crates/failproofaid/src/lock.rs
- crates/fpai-ipc/src/envelope.rs
- crates/fpai-ipc/src/framing.rs
- src/hooks/normalize-cli-payload.ts
- tests/hooks/worker-server.test.ts
- crates/failproofaid/tests/daemon_e2e.rs
- tests/hooks/configure-wizard.test.ts
- crates/failproofaid/src/server.rs
- src/hooks/handler.ts
CI caught what a local run could not. `installDaemonService()` downloads the daemon when nothing is resolvable, so the test asserting "fails cleanly when the binary cannot be resolved" actually fetched the real 1.0.0-beta.0 asset from GitHub Releases and installed it — the install then succeeded, failing that assertion, and the binary it left in the runner's home broke a later test that expects win32 to resolve nothing. It passed locally only because this sandbox has no network in the test environment. Downloads are now off for the whole file (the download path itself is covered in daemon-download.test.ts against a local HTTP server), and the two tests that assert "nothing is installed" run against a scratch HOME so a machine that really has a daemon — a CI runner that just ran the lifecycle tests, a developer laptop — cannot flake them. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EbXKFnpRXBwsrvUJDdySky
`sudo failproofai config` was the advice beta.1 printed when it could not elevate, and it is actively wrong. Under sudo `homedir()` is /root: the hooks land in root's settings, `daemonConfigured` is set for root, the binary downloads to /root/.failproofai/bin, and the unit is generated with `User=root` — the entire user-scope design undone silently, on the one path a user follows when something already went wrong. The wizard now refuses to run under sudo when SUDO_USER shows a real user behind it, and names the account to re-run as. A genuinely root-only environment, which has no SUDO_USER, still works. Service installation becomes step 0. It is the only step that needs a password, so asking there means `sudo -v` prompts on a clean terminal instead of firing from underneath a drawn TUI screen, where the prompt is invisible and the typed characters land in a redrawn frame. That single prompt caches the credential for the run, which is what keeps the install itself non-interactive — no sudo -n failure, no half-written unit. The daemon is no longer inferred from the scope either. It is machine-level — one service for every project on the box — so step 0 is where the user consents to it, and a project-scope setup can have one too. Declining, or failing to authenticate, never costs the rest of the setup: the wizard says so and applies everything else, exactly as a machine with no daemon behaved before. Six existing tests asserted the old scope-gated flow — the behaviour this changes — so they move with it, and three new ones pin what actually matters: that sudo is primed BEFORE any other question (ordering is the whole fix), that declining never touches sudo or the service, and that a refused password still applies the rest. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EbXKFnpRXBwsrvUJDdySky
…starts The daemon unit ships Restart=on-failure with RestartSec=2, so a rewrite systemd accepts but cannot run (the poisoned User= the refresh test injects; a genuinely un-startable regenerated unit in the field) does not fail once — it cycles, and within DefaultStartLimitIntervalSec trips DefaultStartLimitBurst and latches into "start request repeated too quickly". On systemd 255 (ubuntu-24.04, the CI runners) that latch is sticky at the unit level: the rollback restores a runnable definition, `systemctl restart` is refused anyway, and ensureDaemonServiceCurrent returns daemonRunning:false on a machine whose only safety net just failed — which on a daemonConfigured box denies every tool call across all 12 CLIs. restartSystemdUnit() now runs `systemctl reset-failed` (best-effort; a no-op on a healthy unit) before every refresh/rollback restart, making the restart deterministic. The behaviour is load-dependent — an idle machine accumulates too few cycles before the 5s wait ends, which is why it surfaced only under CI's parallel-suite load — and was proven both ways against real systemd 255: original code returns daemonRunning:false, the fix returns daemonRunning:true. This is what made the `test` matrix jobs red on the "puts the old definition back" case. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015BDLTPtbQvUE62eCfQrUbf
… regress `--connect` grew a third network call — `/v1/auth/introspect` — and its test seam was not threaded through with it. `ConnectOptions` already carried `verify` and `verifyIngest` precisely so nothing reaches the network from a unit test; `introspect` was added to `connectToCloud` without the matching option, so every test in `cloud-enrollment-cli.test.ts` silently began making a real request to be.failproof.ai. It USUALLY resolved fast enough to pass, which is the worst available outcome: the suite went intermittently red on network weather rather than on anything a change had broken, so a green run stopped being evidence of anything. Measured at 2 failures in 4 consecutive runs on this machine, and it passed on the machine that introduced it — the reason it shipped. Two parts: - Thread `introspect` through `runConnectCommand` alongside the other two, and stub it in the suite's shared fixture. 6 consecutive runs green afterwards. - Block non-loopback fetch outright in `__tests__/setup.ts`, so the next call added without a seam fails immediately and by name instead of degrading into flake. Loopback stays allowed: six suites stand up a local HTTP server and talk to it, which is a dependency under the test's own control rather than the network. The wrapper is async so a block arrives as a REJECTED PROMISE exactly as a real network failure does — throwing synchronously would escape `fetch(…).catch(…)` and crash the caller, making the guard behave unlike the thing it stands in for. E2E runs under `vitest.config.e2e.mts`, which does not load this file, because those tests are supposed to talk to real infra. The guard has its own tests, since one that nobody exercises is one that quietly stops working: a dropped `await` or an edited loopback list would disarm it, every suite would still pass, and the flake would come straight back. The whole unit suite passes with the guard active, which also establishes that no other test was reaching the network. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01F9eufSorsg96cwpfSdamAk
Two audit adapters were discarding data their agents do emit — pi contributed zero tool events (only `text`/`thinking` blocks were handled, so `toolCall` blocks fell to the generic system branch and `role: "toolResult"` records attached to nothing), and the hermes adapter returned nothing at all for `audit --project <cwd>` on the premise that gateway sessions have no working directory, which its own `sessions` table contradicts. One conflict, in CHANGELOG.md, and it was structural rather than semantic: the PR branched when the top section was `## 0.0.16-beta.0 — 2026-07-31` and adds a single line beneath it. That heading no longer exists — the changelog was renumbered to 1.0.0-beta.x — so git could not place the line and marked the whole top of the file conflicted. Resolved by re-placing that one line under the current `## 1.0.0-beta.5` → `### Fixes`, verified absent beforehand so the merge cannot duplicate it. Nothing else conflicted. No branch here has ever touched `lib/pi-sessions.ts`, `src/audit/cli-adapters/goose.ts` or `src/audit/cli-adapters/hermes.ts`, and main has not moved them since the PR's base, so the five code files merged clean. tsc clean; 3095 unit tests pass, including the 19 the PR adds. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01F9eufSorsg96cwpfSdamAk
NiveditJain
left a comment
NiveditJain
left a comment
There was a problem hiding this comment.
Multi-agent review (ultracode)
Reviewed the full diff (origin/main...HEAD, 243 files / ~60K lines) across 7 subsystem-focused passes (Rust daemon core, Rust cloud-policy security, Rust telemetry/collect pipeline, TS daemon lifecycle, TS hooks core, TS config/onboarding/cloud, CI release pipeline), each finding independently re-verified against the live code by a separate adversarial pass. Also ran the full test/validation protocol from CLAUDE.md.
Validation results
cargo test --workspace: PASS — 26 "test result: ok" blocks, 0 failed, 0 ignored.- Docker clean-install smoke test: PASS — packed tgz installed clean in a fresh
oven/bun:latestcontainer,failproofai --versionandfailproofai p -i -cboth matched the expected output/exit code fromCLAUDE.md's protocol. bun run test:run: 3071/3095 passed (2 files / 24 tests failing). Both failing files are unrelated to this PR's diff (git diff origin/main --statis empty for both):__tests__/hooks/dogfood-configs.test.tsfails onENOENT .claude/settings.jsonbecause that file is locally deleted in this working tree (not part of the PR's commits), and__tests__/components/project-list.test.tsxfails on ajsdom/window.localStorage.clear()sandbox artifact in an unchanged test file. Not regressions from this PR.
Findings
Ranked most-severe first. Every item below was independently re-derived and confirmed against the current code by a second, adversarial agent pass (not just the first pass's claim).
1. [Blocking / security] Systemd unit file directive injection — src/hooks/daemon-service.ts:508-541
systemdUnitContents() interpolates workerCmd/cliCmd/binaryPath/homedir() into Environment="..." and ExecStart= lines with no escaping for systemd's unit-file grammar — only shellQuote() (for the later sh -c split) is applied, which does nothing for an embedded " or newline breaking out of a directive. This unit is installed root-owned (install -m 0644) and loaded at boot via systemctl daemon-reload && systemctl enable --now.
The PR's own test proves the mechanism live: __tests__/hooks/daemon-service.test.ts ("puts the old definition back when the rewritten one will not start") sets FAILPROOFAI_CLI_CMD = '/usr/bin/true"\nUser=failproofai-no-such-user', calls ensureDaemonServiceCurrent(), and confirms systemd parses and honors the injected User= directive — the test only passes because the invalid user makes systemd refuse to start; a valid injected value (e.g. User=root, a new ExecStartPre=) would silently succeed. Any path that lets process.execPath / FAILPROOFAI_PACKAGE_ROOT / the resolved dist path contain " or a newline can inject arbitrary directives into a unit that runs at every boot — undermining the documented "root-installed but never root-run" invariant.
Suggest: escape (or reject) " and newlines when composing Environment=/ExecStart= values, or use EnvironmentFile= with values written via a properly escaping serializer instead of hand-built template strings.
2. [Blocking] Hook evaluation aborts uncaught on a corrupted cloud-managed-policy manifest — src/hooks/handler.ts:297
readActiveCloudManagedPolicies() is called inside evaluateHookEvent's try block with no local try/catch, and that try runs straight into a finally with no catch at all. readActiveCloudManagedPolicies() is documented/unit-tested to throw on a corrupted/tampered active.json or referenced artifact (bad JSON, unsafe id, integrity mismatch, unknown effect). Every sibling loader in the same hot path (readConfigAt, readActivePause) explicitly catches and fails open per-item — this call breaks that established convention.
The throw propagates past handleHookEvent (no catch) to bin/failproofai.mjs's outer catch, which does console.error(...); process.exit(2) with no stdout write. Per this repo's own CLAUDE.md, Hermes "reads a {decision:block,reason} JSON object on stdout and ignores exit codes" — so on a Hermes-gated session hitting this state, every tool call (including something like sudo rm -rf /) is silently allowed until the manifest is repaired. (Note: the daemon-routed path is not vulnerable — worker-server.ts catches the same throw and daemon-client.ts maps it to a fail-closed retry that skips cloud-policy loading.)
Suggest: wrap readActiveCloudManagedPolicies() the same way readConfigAt/readActivePause are wrapped — fail open (skip cloud-managed policies, keep builtins) rather than aborting the whole evaluation.
3. [Blocking] Truncation/reset in file-tail cursor leaves first_line_len stale, causing byte-offset corruption on rewritten files — crates/fpai-collect/src/filetail.rs:236
The file-shrank/truncation branch resets only cursor.offset and cursor.state, leaving first_line_len (and session_id/agent_start_emitted/ended/last_ts) stale from the pre-truncation content. For RereadPolicy::ValidatePrefix sources (e.g. Factory/droid), rebase_on_first_line() then computes delta = new_first_line_len - stale_first_line_len and applies it to the freshly-reset offset=0 — silently skipping bytes (or mis-offsetting) on the very first read of the replaced file. agent_start_emitted staying true also means the replaced content may never get a fresh agent_start, making it (per this module's own docs) invisible downstream even though bytes were spooled.
Suggest: reset the full cursor state (including first_line_len, session_id, agent_start_emitted, ended, last_ts) on truncation detection, not just offset/state.
4. [Blocking] Both-scope + "Everything available" wizard combo crashes mid-apply, leaving the machine half-configured — src/hooks/configure-wizard.ts:1129
Choosing target "Both" (scopes = [user, project]) with the "Everything available" assistants option builds clis as the union across scopes (clisSupportingScope per scope, flattened), which includes hermes/openclaw — CLIs that only support HookScope: "user". The apply loop then reuses that same unfiltered clis array for every scope, contrary to the adjacent code comment claiming installHooks "already skips CLIs a given scope cannot take." installHooksImpl (src/hooks/manager.ts) does not skip — it throws CliError for any CLI unsupported at a given scope.
By the time the project-scope call throws, the daemon has already been installed and daemonConfigured set true, and the user-scope hooks/policies are already written. There's no try/catch around the loop, so the CliError crashes the wizard mid-apply with the daemon enforcing but only one scope fully configured. __tests__/hooks/configure-wizard.test.ts fully mocks installHooks, so this real per-scope validation path is never exercised.
Suggest: filter clis per-scope inside the apply loop (reuse clisSupportingScope(scope) per iteration) instead of passing the cross-scope union to every call.
5. [Major] Worker::call() has a read timeout but no write timeout — can wedge the daemon's whole connection budget — crates/failproofaid/src/worker.rs:288-302
stream.set_read_timeout(Some(30s)) is set, but set_write_timeout is never called anywhere in the file, and write_message() (fpai-ipc/src/framing.rs) does a plain blocking write_all. If the worker's single-threaded event loop stalls (e.g. a custom policy running synchronous CPU-bound code), it stops draining its socket's receive buffer, and a large-enough request can block the writer indefinitely once the kernel send buffer fills. MAX_INFLIGHT_CONNECTIONS = 64 has no mechanism to reclaim a thread stuck in this state, so a single hung worker can exhaust the daemon's inflight budget within 64 requests — the opposite of the fail-fast behavior server.rs/worker.rs are otherwise built around.
Suggest: set a write timeout on the worker-facing stream symmetric with the read timeout.
6. [Major] SIGTERM racing worker cold-start can orphan the worker subprocess — crates/failproofaid/src/main.rs:80-82
worker.warm() is spawned on a detached thread with its JoinHandle discarded, and that thread holds its own Arc<Worker> clone. If SIGTERM arrives while it's still inside ensure_started() (cold start can take hundreds of ms), the accept loop returns within ~20-70ms, run() drops its own Arc<Worker> reference and returns — but the warm-up thread's independent reference keeps the refcount above zero, so Worker::drop() (which SIGKILLs the worker's process group) never fires, and the process exits without joining that thread. This is exercised by the repo's own daemon_e2e.rs test (spawn_daemon → immediate terminate()), and repeats on every fast systemctl restart.
Suggest: join the warm-up thread (with a bounded timeout) during shutdown before the process exits, or have shutdown explicitly kill the worker rather than relying solely on Drop.
7. [Major] repair_active_from_cache() has no fallback path when desired-state.json is corrupted, unlike the symmetric case for active.json — crates/failproofaid/src/cloud_policies.rs:384
self.read_desired()? at the top of the function propagates any JSON parse error straight out, short-circuiting before the branch that repairs a tampered generations/<n>/<id>.mjs copy from the still-valid, content-addressed artifacts/<sha>.mjs copy using only active.json. reconcile() explicitly tolerates a corrupted active.json (Err(ReconcileError::Json(_)) => None), but there's no equivalent tolerance here — so a corrupted desired-state.json permanently disables generation-copy self-healing until the next successful cloud poll rewrites it, which per CLOUD_POLICIES.md never happens on an unenrolled/unreachable-cloud machine. No test covers a corrupted desired-state.json (only a corrupted active.json is tested).
Suggest: make the desired-state read tolerant the same way the active-state read is, falling back to the active.json-driven repair path.
8. [Major] [collector] redact config knob is parsed and tested but never wired to any real source — crates/fpai-collect/src/config.rs:127
Redact::Off/Minimal parses correctly from config.toml (proven by its own unit test), but filetail::Params, sqlitepoll::Params, and sources::hooks::run() have no redact field at all, and collector_tasks() in crates/failproofaid/src/main.rs never reads cfg.settings.redact when constructing any of them. SpoolWriter::with_redact() is invoked only from a #[test] — every real SpoolWriter::new() call site keeps the hardcoded Redact::Minimal default regardless of config. Setting redact = "off" in config.toml has zero observable effect.
Suggest: thread cfg.settings.redact through to each source's SpoolWriter construction, or remove the config knob until it's wired up (a silently-ignored setting is worse than none).
9. [Major] publish.yml has no concurrency: group, enabling a TOCTOU on the "already published" preflight and an unguarded version-bump push race
Unlike ci.yml (concurrency: group: ci-${{ github.ref }}) and bump-platform-submodule.yml (explicitly commented "not a race that loses one of them"), publish.yml has no concurrency: block at all, despite being triggered by both release: published and workflow_dispatch. Two overlapping runs for the same version can both pass the new "verify the version is unpublished" npm view preflight before either has actually published — most of the downstream npm-level races resolve safely, but the "Bump version for next development cycle" step's unguarded git fetch → checkout → commit → push origin main (using the same version-bot bypass mechanism bump-platform-submodule.yml deliberately serializes) means the losing concurrent run's push genuinely fails outright.
Suggest: add a concurrency: group: publish-${{ github.ref }} (or version-keyed) block, matching the pattern already used elsewhere in this repo's own workflows.
10. [Minor] policyModuleCache has no size bound, unlike the sibling cache added in the same diff — src/hooks/custom-hooks-loader.ts:48
gitBranchCache (added alongside this in builtin-policies.ts) is explicitly capped at 500 entries with the stated rationale "so a warm worker touching many projects over its lifetime doesn't grow this unboundedly." policyModuleCache, keyed by absolute custom-policy file path and holding cloned hook closures, has no equivalent cap, eviction, or clear — the same rationale applies but wasn't carried over. Low severity since it requires long daemon-worker uptime across many distinct policy-file paths to matter in practice.
Suggest: apply the same cap/clear pattern used for gitBranchCache.
Summary
Test/build validation is clean (Rust suite green, Docker install/smoke-test green, TS unit-test failures traced to pre-existing local-environment issues unrelated to this diff). The findings above cluster around two themes worth prioritizing before merge: fail-open gaps in hot enforcement paths (#2 handler.ts, #7 cloud_policies.rs) that can silently weaken enforcement rather than loudly fail, and unescaped interpolation into a root-owned, boot-loaded systemd unit (#1), which is the highest-severity item here. #3 and #4 are both concrete correctness bugs with clear repro paths. #5/#6 are daemon-lifecycle robustness gaps under adversarial-policy or fast-restart conditions rather than everyday-path bugs.
Generated by a bounded multi-agent review (7 review agents across Rust daemon/cloud/telemetry and TS hooks/daemon/config/CI, each finding independently re-verified by a second adversarial pass, plus a dedicated Docker/test-validation agent) run against origin/main...HEAD.
…d redactor
Nine findings, each verified against the code before being changed. Three were
already fixed on this branch and are not repeated here; what remained is below.
**The reset deleted hand-written policies and then hid it.**
`resettablePaths()` listed `at("policies")`, an unconditional recursive remove —
and on layout 1 that directory IS the documented home for personal convention
policies (`docs/configuration.mdx`: "User | ~/.failproofai/policies/"). Nothing
migrated them, nothing backed them up, and the printed message named only
"policy config, activity history and audit cache". Three things compounded it
into a silent enforcement gap: it fired from `failproofai policies --help`
(the help block skips subcommands, so help fell through to the layout check
while the adjacent first-run gate exempted it); afterwards `isConfigured()`
still read true off the agent CLIs' untouched settings files, so the wizard was
skipped and `markLauncherSeen()` back-filled the marker so every later run
skipped it too. The reset now enumerates the machine-owned children
(`local-policies`, `cloud-policies`, and layout 1's `cloud-managed`), moves
top-level policy sources into `custom-policies/` where layout 2's loader reads
them, names each moved file, exempts help and `--version`, and reports
`didReset` so the caller forces setup.
**paths.rs and fp-home.ts both cited a guard that did not exist.**
The three divergences themselves were fixed earlier on this branch, but
`fp-home.ts` named a test with no reference to `crates/` and `paths.rs` named
`crates/failproofaid/tests/layout.rs`, which was never created. Replaced with
`every_mirrored_path_agrees_with_fp_home_ts`, which imports the TypeScript
module in a child process and compares all ten mirrored paths rather than
restating their values in Rust — the reason the existing hand-written
assertions passed while all three rows were wrong. Verified to fail when the
`cloud-managed` bug is reintroduced.
**An installed-but-broken daemon locked the machine out with no way back.**
`ExecStart` bakes in `process.execPath`, so an `nvm uninstall 20` leaves a unit
systemd reports as active whose worker dies on every spawn. Every check passed
that machine: `waitForDaemonRunning()` asks the service manager, `Ping` is
answered without touching the worker, and the wizard's "already running —
leaving it alone" branch skipped the documented repair. `daemonConfigured` then
denies every tool call including `UserPromptSubmit`. Adds
`probeDaemonEndToEnd()` (a real `SessionStart` hook, 5s budget), gates install
on it, extends `healDaemonFlag` from not-installed to running-but-broken, and
gives `uninstallDaemonService` its first production caller: the wizard tears a
wedged unit down before reinstalling, since it holds the singleton flock the
replacement needs. Adds the first `forceDecision` test — the fail-closed path
had none.
**A never-settling policy load wedged the daemon permanently.** `enqueue`'s
`.catch()` covers a task that rejects and does nothing for one that never
settles, which a bare `await import()` of a module with a hanging top-level
`await` produces. Every subsequent hook on the machine queued behind it forever
and fail-closed denied, across all 12 CLIs. The import is now bounded at 10s
(matching the per-policy budget), with a 60s backstop on the queue that exits
rather than continuing — the orphan still holds the globalThis registry the
chain exists to serialize.
**block-self-pause denied `grep`.** `SELF_PAUSE_RE` had no command-position
anchor, so any command merely containing the string matched — including this
repo's own CHANGELOG and docs. It is `defaultEnabled`. Now anchored
structurally: segments split on shell operators, runners and their flags walked
off, and the binary required in command position. All fourteen existing
red-team cases still deny.
**The wizard crashed mid-apply, and put a bearer token on cleartext http.**
"Both" + "Everything available" passed the cross-scope union to every scope, and
`installHooksImpl` throws rather than skips — after the daemon was installed and
user hooks written. Now filtered per scope. Separately, `validateCloudUrl` had
one caller (`--connect`); the wizard matched `/^https?:\/\//` and handed the raw
string to `validateIngestKey` and `connectToCloud`. The check now runs inside
`connectToCloud`, so no path can skip it, and at the prompt so it fails fast.
**Redaction leaked the tail of every non-ASCII secret.** `match_bearer` and
`match_assignment` returned `.chars().count()` where `scrub_str` uses the value
as a byte offset. Both predicates accept non-ASCII, so the cursor landed back
inside the secret. Eleven two-byte characters leak eleven bytes — enough for a
whole readable tail. Invisible to every existing test because every token in
them was ASCII; the new test asserts exact equality, since a
"does the tail survive" check passes while the bug is fully present.
**Every release turned main red.** The next-dev-version bump moved
`package.json` only, while CI compares it against `Cargo.toml`'s workspace
version, and the bump commit carries `[skip ci]` — so the mismatch surfaced on
the next unrelated PR. Now rewrites `[workspace.package]` and refreshes
`Cargo.lock` in the same step. Adds the `concurrency` group `publish.yml` was
missing, without `cancel-in-progress`: the assets attach before the npm publish,
so a cancelled run leaves a tag with binaries and no package.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… sweep **Redaction, cloud-policy loading and the daemon's I/O budgets.** `readActiveCloudManagedPolicies()` sat bare inside `evaluateHookEvent`'s `try`, whose only handler is a `finally`. Fourteen throw sites, and what any of them cost depended on where the hook ran: on a daemon machine the client fail-closed denies everything; off it the throw reaches the CLI's outer catch, which exits 2 with nothing on stdout — a deny on Claude and Factory, but a logged warning followed by an ALLOW on the five CLIs that read a decision off stdout and ignore the exit code. So one corrupt byte was either a machine-wide lockout or silent non-enforcement. Now wrapped like its siblings: the cloud layer degrades alone, loudly, while builtins and local custom policies keep enforcing. `CONNECTION_IO_TIMEOUT` did not bound a connection. `set_read_timeout` is `SO_RCVTIMEO`, which bounds one `read(2)`, and `read_message` reads through `read_exact`, so every byte that arrived reset the clock — a peer dribbling one byte every nine seconds held its handler thread forever while satisfying the timeout, and 64 of them fill `MAX_INFLIGHT_CONNECTIONS`. Replaced with a `Deadline` wrapper enforcing one wall-clock budget across every read and write. The worker stream also gained the write timeout it never had: `write_message` is a blocking `write_all`, so a worker whose event loop stalls stops draining its socket and blocks the writer once the send buffer fills. `Server::bind` used `Path::exists()`, which follows symlinks — so a DANGLING symlink at the socket path reported false, was never unlinked, and `UnixListener::bind` failed `EADDRINUSE`. With `Restart=on-failure` that is a crash loop, and a crash-looping daemon on a `daemonConfigured` machine denies every tool call. One `ln -s` away. **Collector correctness.** The truncation branch reset `offset` and `state` and left everything else, so for a `ValidatePrefix` format `rebase_on_first_line` applied a delta computed against the OLD first line to the freshly-zeroed offset and skipped bytes — and `agent_start_emitted` staying true meant the replacement content never announced a session, which the server selects on, so it was spooled and then absent from the product. Now re-derives the whole cursor from the file as it is. Inode reuse resumed a dead file's cursor: the guard only fires when the recorded path still EXISTS and still holds the inode, and in a real reuse the old file was unlinked — which is how its inode came to be free. Cursors now carry a fingerprint of the file's first bytes, checked when a resumed cursor's path has changed, so rotation still resumes and reuse does not. `CursorStore::save()` fsynced the entire map every 2s per source whether or not anything moved, and the map grows monotonically because `retain_existing` only drops cursors for deleted files and transcripts are never deleted. Now gated on a dirty flag, cleared only after a successful rename. A file tailer could not report an error: `poll_once` warned per file and returned `Ok(0)` even when every file failed, and `record_poll` then unconditionally cleared `last_error` — so a source whose root is unreadable was indistinguishable from an idle one, the exact distinction per-source health exists to draw. And every Hermes profile reported under the bare key `"hermes"`, so two profiles with one database missing alternated `root_present` every five seconds; each now reports under its own key, matching the cursor directory it already had. The aggregate `hook_id` omitted the attribution its own `BucketKey` splits on, so two buckets the key had just separated carried byte-identical ids — and per that file's header the server dedups on `hook_id`, undoing the split downstream in exactly the two cases it was built for: the minute a pause starts, and the minute a cloud generation flips during a rollout. `tests/hooks_source.rs` constructed that collision and never compared the ids. **Pause.** The 8h ceiling was measured from `pausedAt`, which every renewal reset to now — so `--pause 8h` re-issued every seven hours suspended enforcement indefinitely, one individually-legal command at a time. Pauses now carry `firstPausedAt`, clamped at write AND at read, so a hand-edited state file cannot buy an unbounded pause either. `maxPauseMs` is removed rather than wired up: the merge in `hooks-config.ts` never emitted it, so the lookup could only read `undefined`, and its two tests `vi.mock`ed that function to return a field it cannot produce. **`--disconnect` now disconnects.** It cleared the credential, which stops POLLING — every artifact already on disk stayed referenced by `active.json` and kept being enforced on every tool call, so a machine that had left its organisation went on being governed by whatever generation was current when it left. It also claimed "hook activity and transcripts stop being sent", which was not true of the running daemon: the collector manager starts once and the uploader caches its key at construction. Both fixed — the manifest is cleared, and the message names the restart instead of asserting something false. **Layout-2 stragglers.** `install-check.ts` read layout 1's `policies-config.json`, so `package_installed` telemetry reported every layout-2 machine as unconfigured with zero policies; `manager.ts` printed that path in all three render states; the wizard's convention scan read the layout-1 global directory while the loader reads `customPoliciesDir()`. `last-version` moved under `state/`, which also fixes the reset banner every fresh install saw: the CLI wrote that file and then read it back as one of `detectLayout()`'s layout-1 landmarks. The install report now runs after the layout check for the same reason. And `cli_configure_invoked` sent `result.scope`, replaced by `target`/`scopes` in the wizard rework — always null since; `.mjs` is outside the tsconfig include, so `tsc` could not catch it. The same call site discarded `result.abort`, so `failproofai config` exited 0 even when the machine was left unconfigured. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…config **A value with a newline could inject directives into a root-owned unit.** `systemdUnitContents` interpolated `workerCmd`, `cliCmd`, `binaryPath`, `homedir()` and `User=` into a file installed at /etc/systemd/system and loaded at every boot, with no escaping for systemd's grammar — and a newline ENDS a directive. This repo's own refresh test demonstrates the mechanism: it sets `FAILPROOFAI_CLI_CMD` to `/usr/bin/true"\nUser=failproofai-no-such-user` and passes only because systemd HONOURS the injected `User=` and that account does not exist. `User=root`, or an added `ExecStartPre=`, would have succeeded silently and undone the "root-installed but never root-run" invariant the whole design rests on. These values are resolved paths and commands, so this rejects rather than inventing an escaping scheme for systemd's grammar, and it refuses before anything is written or stopped — so the refresh path now reports `daemonRunning` truthfully instead of leaving the caller to assume the worst about a machine it never touched. **The worker outlived the daemon when SIGTERM raced its cold start.** `main.rs` pre-warmed on a detached thread holding its own `Arc<Worker>` whose `JoinHandle` was discarded, so a signal arriving while that thread was still inside `ensure_started()` — hundreds of milliseconds, against an accept loop that returns in tens — left the refcount above zero when `run()` dropped its reference. `Worker::drop` never fired and the process group was never killed. Adds an explicit `Worker::shutdown()` whose flag is checked under the same lock `ensure_started` takes, so a warm-up that had not yet spawned refuses rather than installing a worker after the kill; `Drop` stays as the backstop. **Generated policy modules accumulated forever and were blamed on the user.** The temporary tree is written beside the sources — the only place a rewritten relative import resolves — and its name now carries a pid and sequence number, so unlike the old fixed name each killed load leaks a file permanently instead of leaving one the next load overwrites. `findSkippedPolicyFiles` then reported every leftover as a policy file that would not load, which is an accusation about a file failproofai wrote itself. They are now excluded from that scan and swept on load, age-gated so a sweep can never remove a tree another process is still importing. `policyModuleCache` gained the cap its sibling `gitBranchCache` documents the rationale for and it never carried over. **A corrupt `desired-state.json` disabled cloud-policy self-healing forever.** `repair_active_from_cache` propagated the parse error straight out, short- circuiting before the branch that rebuilds a tampered generation copy from the content-addressed artifact using `active.json` alone — a file that branch does not even need. Per CLOUD_POLICIES.md the only thing that rewrites it is a successful cloud poll, which never happens on an unenrolled or unreachable machine. `reconcile()` already tolerated exactly this for `active.json`; the two are now symmetric. **`[collector] redact` configured nothing.** It parsed correctly and reached no source: `SpoolWriter::with_redact` had exactly two references — its definition and its own unit test — and every real writer kept the hardcoded `Redact::Minimal`, so `redact = "off"` had no observable effect anywhere. Now threaded from config through all three source shapes, with an integration test that runs a real filetail pass both ways over a transcript containing a token: before this, the two outputs were byte-identical. **`Cargo.lock` was scanned by nothing** — OSV-Scanner was only ever given `bun.lock`, and Dependabot had no `cargo` ecosystem — for a TLS stack that compiles into a root-installed system service. Validation: 3126 unit tests, 318 e2e, 26 cargo test blocks, clippy -D warnings, cargo fmt, tsc, next build, and the Docker clean-install smoke test from CLAUDE.md (`Validated 1 custom hook(s)`, exit 0). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
CI caught this: gating `installDaemonService()` on `probeDaemonEndToEnd()` broke
six sudo-gated tests that install `/usr/bin/sleep infinity` as a stub to verify
unit-file mechanics. That binary starts and stays running — satisfying
`waitForDaemonRunning()` — and of course cannot answer a hook, so the probe
correctly turned `{installed: true}` into `{installed: false}`.
The tests were right and the placement was wrong. `installDaemonService()` can
only honestly report on the SERVICE: the unit was written, systemd accepted it,
the process is up. Whether the daemon can evaluate anything is a separate
question, and the finding put it in the right place to begin with — before
`daemonConfigured` is set, because that flag is what turns an unanswering daemon
into every tool call denied across all twelve CLIs.
So the probe moves to `configure-wizard.ts`, immediately after a successful
install and before the flag. A failure takes the existing daemon abort path,
which is correct and unchanged: the daemon step runs before anything
user-facing is written, so setup stops with the machine exactly as it was found.
Keeping the two apart is also what lets the install mechanics stay testable
against a stub, which is worth preserving.
Adds a test for the new branch — install succeeds, probe fails, nothing is
written and `daemonConfigured` is never set.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…t door Three removals asked for, plus one thing the first of them made newly wrong. **Nothing about a scheduled scan leaves the machine any more.** The scan reads the CONTENTS of every agent session transcript on disk — prompts, file contents, pasted credentials, command output — and POSTed a counters-only projection of that to /enforcement/v1/machine-scans. The projection was built defensively: an additive whitelist rather than a subtractive filter, rule ids checked against our own catalog so a customer's policy name could not ride along, a project COUNT rather than names, `deny_unknown_fields` on the receiving side, and three gates that all had to open. It never carried a path, a command, or a line of prose. It is still gone. An audit of what is on someone's laptop is a local tool, and the safest version of a network call it does not need to make is not making it. `machine-scan-payload.ts`, `machine-scan-report.ts` and `harmful.ts` are deleted, and `runScheduledAudit` now ends at the dashboard cache — the channel the user actually sees. The scheduled audit itself is untouched: the daemon still runs it, `audit --scheduled` still works, and the /settings controls still schedule it. A regression test pins that a completed scan makes no `fetch` call at all, spying on the global rather than on the deleted module, so the upload cannot come back by accident and a POST to loopback would fail it too. The receiving route in AgentEye is deliberately untouched and simply stops being called. Removing it there is a separate decision in a separate repo. **Emailed scan reports go with it**, because they existed only to deliver that upload: `config --email` / `--no-email`, `email-reports-cli.ts`, the `[email]` block in config.toml, the `[email] verified_for` record in credentials.toml, and the email section of /settings. The server picks recipients from org membership, so with nothing uploaded there is nothing that could produce a report — and an opt-in switch for something that cannot happen is worse than no switch at all. **`failproofai auth login` / `logout` / `whoami` are removed.** Sign-in has not gone anywhere: the local dashboard's re-audit reminder and invite-a-friend both still require it and both still work, through the dashboard's own sign-in dialog, which has always had its own login-request / login-verify routes and writes the same auth.json. What is removed is the redundant second front door. `lib/auth/` is untouched because those routes are built on it. The 15 `cli/auth` doc pages and their nav entries go too, and every cross-reference in all 15 languages was rewritten rather than left pointing at a deleted page — including the two where the connective follows the link (ja, ko) and the three where removing the term stranded a leading conjunction (ja, zh, ko). **And the unknown-subcommand hint is now a nearest match** instead of the literal string "policies" for every input. That was only ever right when the typo happened to be a typo of that word, and this change made it concrete: `auth` was a real subcommand until now, so an old script or plain muscle memory lands on that path and was answered with the one command that has nothing to do with what was typed. `failproofai auth` points at `audit`; `confg` points at `config`. The Levenshtein helper the flag guard already had is hoisted so both guards use it. Validation: 3068 unit, 320 e2e, 26 cargo test blocks, clippy -D warnings, cargo fmt, tsc, eslint (4 warnings, all pre-existing and in files not touched here), `bun run build`, and 705 MDX pages parsing with every nav target resolving. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01F9eufSorsg96cwpfSdamAk
The ESM shim that makes a policy file's `import ... from 'failproofai'` resolve was written into the installed package's own directory. That directory belongs to whoever installed failproofai, and on a system-wide install (`sudo npm i -g`, a container image, a shared build host, a CI runner) it is root-owned — so every hook run by a non-root user failed with EACCES, the policy never loaded, and the hook exited 0. The tool call was allowed. Builtin policies need no file loading and kept firing, so the machine looked protected: denies appeared, the dashboard showed activity, `--status` reported connected and pulling, while the organisation's centrally-managed policy did nothing. The only signal was one line on stderr. Found in a container test and confirmed by toggling nothing but that directory's mode — chmod 777 and the policy denies, chmod 755 and the same call is allowed. A single-user machine never saw it because npm's global prefix there is owned by the user running the hook. The shim now goes to `<home>/state/shims`, which is ours by construction, and — unlike a shared /tmp — no other local user can pre-plant a file at a path we are about to import. The order of operations carries as much weight as the location: - The write sits inside the same guard as the mkdir. `mkdir` with `recursive` RESOLVES on a directory that already exists whatever its mode, so a `state/shims` left behind unwritable (a container that ran the CLI as root and then dropped to a non-root user — a chain this very function creates) would sail past a mkdir-only guard and throw on the write, reproducing the exact fail-open being fixed here. - The directory is lstat-checked to be a real, private, self-owned one. `mkdir` accepts a pre-existing symlink and a plain write would follow it out of the home, into a file we then execute. - The shim is written 0600 rather than inheriting `0666 & ~umask`; `umask 000` is routine in containers, and on the fallback that is a world-writable file in shared /tmp that this process imports. - Any failure degrades to os.tmpdir() LOUDLY. A silent slide into the weaker path is this bug's own shape. - The per-load suffix now carries a random id, so a leftover file cannot fail a legitimate load through the fallback's O_EXCL, and two pid namespaces sharing a home cannot collide. `fingerprintTemporaryTree` normalises the whole suffix away, so the policy module cache still hits — verified, because a name it could not normalise would force a cold module load on every hook call. - The shim directory is swept on the same age-gated terms as the policy directories. It is ours alone and outside the policy tree, so nothing else would ever reap a shim left behind by an abnormal termination. Six real-filesystem tests; a mocked fs would happily "write" to a root-owned path and prove nothing. Verified live in containers against all four shapes: root-owned package dir, root-owned unwritable state/shims, state/shims symlinked out of the home, and a harmless command that must still pass. This does not close the whole class: rewritten policy copies are still written beside their source, so a read-only policy directory (a root-owned org policy pack, a :ro mount) still fails the same way. Untouched here, and called out in the changelog rather than left to look solved. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015BDLTPtbQvUE62eCfQrUbf
1.0.0-beta.5 went to the registry on 2026-08-04, and the branch has been sitting on that version since — so the publish preflight refuses to start, which is what it is there for: the root package publishes LAST, so without the check a release runs the whole 4-way cross-compile, attaches the assets, publishes the four platform packages, and only then takes E403, stranding orphan platform versions that nothing pins and that npm's 72-hour window is the only way to remove. Four places, because the version is not only an npm fact: - package.json — the published version. - Cargo.toml — the CI quality job compares it against root. The release tag the CLI builds its daemon download URL from is the npm version, and the binary at that URL reports the Cargo one; letting them drift means a CLI that downloads a daemon claiming to be a different build. - Cargo.lock — the three workspace crates pin it. Version lines only; no dependency churn. - CHANGELOG.md — the top heading, which the repo requires to match package.json. Everything under that heading is unreleased work: it is dated 2026-08-06, two days after beta.5 actually shipped. NOTE for whoever cuts the release: a SECOND `## 1.0.0-beta.5` heading remains further down (dated 2026-08-05, ~32 entries). It also postdates the beta.5 publish, so those entries ship in beta.6 while appearing under a version that is already on the registry. Left alone deliberately rather than restructuring someone else's entries mid-release. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015BDLTPtbQvUE62eCfQrUbf
…rever Two changes answering the same question — what is true when the daemon cannot answer — plus the one place the answer is not literally true, written down. **A daemon-configured machine now has exactly one evaluator.** A protocol-version mismatch used to fall back to in-process evaluation, on the reasoning that a daemon which answered is demonstrably alive and denying would take a working machine offline to protect nothing. That reasoning was about availability and it ignored what the fallback IS: a second policy engine, reachable by breaking the first. So a mismatch now denies like every other way of not getting an answer, and in-process is never reached from that branch. The two failures are still told apart, in the MESSAGE and nowhere else, because the remedies differ: a mismatch names the version and `failproofai config`, where an unreachable socket cannot honestly say more than that it could not be reached. A stopped service, a deleted socket and deliberate tampering are indistinguishable from the client, and a machine where stopping one service silently disables every guardrail is not a guarded machine. The cost is accepted deliberately rather than overlooked: both sides hardcode PROTOCOL_VERSION, so the first time it is bumped, a machine whose CLI updated via npm before its daemon did denies until `failproofai config` runs. publish.yml ships both from one commit and daemonVersionSkew() already hints on every CLI command, so the window is bounded and announces itself. In-process survives only where there is no daemon to route to — this repo's dogfood configs (standing decision: a flaky dev daemon must not block contributors in the loop where the daemon is developed), unsupported platforms, and machines that were never set up, which have no hooks either. `protocol-fallback.sh` asserted the opposite and now asserts this, against a real CLI process talking to a real daemon answering protocol 99. It also checks that a command a local policy WOULD allow is still denied — reaching a verdict of its own would prove a second evaluator ran. **An aborted setup is now remembered.** Every abort path writes nothing at all. That is deliberate and unchanged: a `daemonConfigured` flag with no daemon behind it denies every tool call across twelve CLIs, so a machine that could not be configured is left exactly as found. But it left onboarding with no memory — isConfigured() reads three signals and an abort sets none of them, so "never tried" and "tried twenty times and could not finish" were identical. On a box without passwordless sudo the wizard relaunched on literally every command, forever. `state/onboarding-attempt.json` records the reason; the next command prints one line instead of redrawing the wizard. It is emphatically NOT a fourth "configured" signal — a failed attempt still reads as unconfigured to --status, to the hook path, and to `failproofai config`, which always gets the wizard (exempt from the gate AND passes force). A test pins that invariant directly. A hint that never became an offer again would be its own failure, so each reason carries a cheap local check for whether its blocker has cleared: needs_root re-checks elevation, daemon_failed re-checks the service manager (any movement, not just `running`, so a partially-repaired machine is not stranded), and a deliberate `cancelled` waits for a version change rather than re-nagging. An unrecognised reason re-offers — a blocker we cannot prove is still present must not silently withhold setup. None of these probes runs on the hook path, which never reaches this gate. **And the one place the rule is not literally true.** Windows: isDaemonSupportedPlatform() is linux + darwin, and the wizard skips the daemon requirement there rather than refusing setup, so that machine completes setup, reads as configured, and enforces in-process with no fail-closed guarantee. The policies are identical and do enforce; the guarantee is what is missing. Dropping the platform was judged worse. Now documented in CLAUDE.md alongside the routing rules rather than left to be rediscovered. Validation: 3093 unit (+19), 320 e2e, 9/9 protocol harness against a real daemon, tsc, eslint (1 pre-existing warning), cargo fmt, clippy -D warnings. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01F9eufSorsg96cwpfSdamAk
1.0.0-beta.6 was tagged and released while this branch was in flight, so the branch was carrying a version that is already on the registry. publish.yml's preflight refuses to start in exactly that case — a workflow_dispatch has no version input, so it publishes whatever package.json carries, and the root package publishes LAST, meaning a burned version would run the whole 4-way cross-compile and attach release assets before taking E403. package.json, Cargo.toml and Cargo.lock move together because CI's version consistency check compares the Cargo workspace version against the root package.json: the CLI builds its daemon download URL from the npm version, and the binary at that URL reports the Cargo one, so a drift between them ships a CLI that downloads a daemon claiming to be something else. The CHANGELOG heading moves with it, per the rule that entries always sit under a dated, versioned heading matching package.json. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01F9eufSorsg96cwpfSdamAk
….0.0-beta.8
`failproofai config` could not complete against a healthy daemon. The setup
health probe sends no `cwd`. The daemon deserialises that as `None` and forwards
it with `json!({ "cwd": cwd })`, which serialises to an explicit **null** rather
than omitting the key — and the worker's request validator accepted only
`undefined`. So the worker answered "unrecognized request shape", the probe
failed, and setup aborted telling the operator "its worker process could not be
run" against a worker that had already logged `listening on worker.sock`.
It was not intermittent. The probe never sends a cwd, so this failed on every
machine, every time, and setup could never install a daemon. The validator now
treats null and absent alike — a wrong TYPE is still refused — and normalises to
`undefined` at the boundary so nothing downstream learns how the wire spells
"absent".
The same mismatch sat under real enforcement, not just setup: on a machine with
`[daemon] configured = true`, any hook payload carrying no cwd took the identical
path, and the fail-closed contract turns that into a denied tool call.
Two faults behind it are fixed as well:
- The probe raced the socket. It runs moments after `systemctl enable --now`, and
a `Type=simple` unit is reported ACTIVE the instant systemd forks it, before the
daemon has bound. The probe got the hook path's deliberately-tight 150ms connect
budget and exactly one attempt, so on a loaded machine it lost that race and
called a healthy daemon broken. It now retries for up to 10s. The 150ms itself is
untouched: that budget is what keeps a dead daemon from adding latency to every
tool call, and it is the wrong knob to loosen for a once-per-setup probe.
- The abort named the wrong fault. `DaemonFailure` reports `unreachable` for BOTH
a refused connection and a request accepted but never answered, so setup told
people their worker would not start when nothing was listening at all — sending
them to inspect a process that was fine. `probeDaemon` now separates "never
accepted a connection" from "accepted, could not answer", and the wizard prints
the remedy that matches.
Tests: 4 race tests drive a real Unix socket server rather than a mock, because
the bug lives in the timing between connect() and listen(); 3 cover the null/
absent/wrong-type validator split. Each was red-proven against the unfixed code.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…rashing npm runs no uninstall script — this repo proved that empirically and deleted the dead `preuninstall` that assumed otherwise — so `npm rm -g failproofai` deletes the package and leaves behind every durable thing it installed: hook entries in up to twelve agent CLIs' settings files, and a root-owned systemd unit. Those leftovers are not inert. The hook entries invoke `npx -y failproofai`, which re-downloads the package from the registry, so a "removed" failproofai keeps running on every tool call. And on a machine with `[daemon] configured = true`, the surviving unit points at a worker script npm just deleted, which under fail-closed semantics denies EVERY tool call across every agent CLI with nothing on screen naming the cause. 1. `failproofai uninstall` — the sanctioned way off a machine. ORDER IS THE SAFETY PROPERTY, and it is not the intuitive one. `daemonConfigured` comes down FIRST — before hooks, before the service, before anything that can fail or need a password. From that instant the machine can only fail OPEN and every later step is best-effort cleanup. The intuitive order (tear the service down, then update config) leaves a window where the flag demands a daemon that is already gone, and that window is a total lockout of the user's agent — the exact failure `healDaemonFlag` exists to repair after it bricked a machine during development. If the flag CANNOT be cleared the command stops there rather than pressing on. `--purge` also deletes ~/.failproofai; `--dry-run` prints the plan; `--yes` skips the prompt and is REQUIRED rather than assumed when there is no TTY, so a prompt that cannot be answered never reads as consent to delete a root-owned service. Incomplete cleanup exits non-zero and prints the exact sudo commands — the one thing worse than a leftover unit is a leftover unit the operator was told did not exist. The survey walks every INSTALLABLE cli, not every detected one: hook entries outlive the CLI that owned them, and those orphans are the point. `--purge` also suppresses the command's own telemetry. Resolving an instance id lazily WRITES state/telemetry-id, which re-created the entire directory seconds after the purge deleted it — leaving a just-wiped machine holding a brand-new tracking identifier and making the command's own "✓ deleted" line false. Caught by the container test, which checked the filesystem rather than the output. 2. The unit is now gated on the files it cannot run without. `ConditionPathExists=` covers the daemon binary and the worker script. A deleted worker previously left systemd running a daemon that could only deny; a deleted BINARY was worse, because ExecStart fails 203/EXEC under `Restart=on-failure` and cycles until it trips the start-limit and latches into "start request repeated too quickly", which then refuses a legitimate restart later. A failed condition is not a failure: systemd SKIPS the job and names the exact missing path. Verified against real systemd — with the worker deleted the unit goes inactive with ConditionResult=no and is-failed=inactive, restoring the file brings it straight back, and the same unit WITHOUT the condition reproduces the restart cycle it prevents. `daemonServiceStatus()` gained `condition-failed`, read from systemd's own ConditionResult, and `healDaemonFlag` treats it like "not-installed" — clearing `daemonConfigured` so the machine stops denying every tool call. "Stopped" still never qualifies, deliberately: a restart in flight looks identical, and clearing there would silently downgrade a healthy machine to the in-process path. Only a literal `no` counts, so this under-reports rather than over-reports. None of this softens the hook path's fail-closed deny. A machine configured to require the daemon still denies while the daemon is absent; being skipped by systemd is not consent to stop enforcing. It shortens how long that lasts and explains why. `uninstall` is exempt from the first-run wizard for the sharpest version of the existing reason: it states the exact opposite intent, and offering to set a machine up on the way to tearing it down would install hooks and a root-owned unit seconds before the command removes them. Tests: 12 for the command (the ordering ones red-proven by moving the flag-clear last, which fails 3), 6 for the unit conditions, plus a real-systemd container run that removed a live unit and live hook entries end to end. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… path
Both found by installing the packed tarball and driving the product as a new
user would, against a live AgentEye stack.
**Setup could not complete on any machine that requires the daemon.**
probeDaemonEndToEnd() — the gate added so a daemon that cannot evaluate never
gets daemonConfigured set — failed against a perfectly healthy daemon. The
wizard read that as "installed but cannot evaluate", aborted, and wrote nothing.
Every other signal said the machine was fine: the service installed, systemd
reported it active, the journal showed the socket bound and the worker
listening, and a hand-fired hook routed and enforced correctly.
The cause is a null/undefined mismatch across the Rust->TS boundary. The daemon
builds its request with `json!({… "cwd": cwd})` where cwd is an Option<String>;
serde renders None as JSON null; the worker's validator accepted only undefined
or a string, and answered "unrecognized request shape". The probe is the ONLY
caller that omits a cwd, so the defect lived exclusively on the health-check
path and nothing else could surface it.
Fixed on both sides on purpose. The validator accepts null, because JSON has no
way to spell undefined and null IS the wire's representation of an absent
optional — any other Rust caller omitting a field would have hit the same wall.
And the probe now sends a real cwd, so it genuinely traverses the path its
docstring claims: a probe that sends a shape no real caller sends can only ever
test something else. worker-request-shape.test.ts drives the real worker socket
and still rejects genuinely malformed shapes.
**An absolute path defeated block-sudo.**
It matched the literal binary name at a command boundary, so a fully-qualified
path to the elevation binary was ALLOWED — a direct invocation, no obfuscation,
on a defaultEnabled guard whose only job is stopping root. block-self-pause had
already been hardened against exactly this; the two had drifted apart.
Elevation is now anchored structurally the way that sibling is: prefix
assignments, redirections, runners and their flags are walked off, and the
comparison is on the BASENAME. doas is included, because a machine with it
installed and only the other name blocked is not blocked.
Two mistakes on the way, both recorded in the tests. Stripping quotes from the
WHOLE command before segmenting turned an escaped pipe inside a grep
alternation into a separator, so an ordinary search was denied — a policy that
fires on grep gets switched off, and a policy that is off protects nothing.
Tokens are therefore unquoted individually, and quote removal can never move a
boundary. A quoted argument is re-examined only when a shell runner was invoked
with an eval flag, since a runner evaluating its argument and a search string
containing the same text are indistinguishable from outside; what separates
them is whether the receiving binary evaluates it.
The second mistake is worth naming: the new helper was called shellSegments,
which already existed in that file. The duplicate shadowed the original and
silently broke every recursive-delete check as well as this matcher — caught
only because tsc reports duplicate implementations, not by any test.
block-sudo-anchoring.test.ts covers both directions and states, as an explicit
test, what static inspection genuinely cannot reach (a variable, base64 through
a pipe, a wrapper script on disk), so the honest claim stays "stops the obvious
attempt" rather than "prevents elevation".
**Also**
scripts/repro-npm-install.sh reproduces a real install in the shape that breaks
— npm i -g into a ROOT-owned prefix, then the CLI run by an unprivileged user,
with real systemd. A single-user laptop cannot exercise that split, which is how
the root-owned policy-shim fail-open shipped. It guards two traps found while
writing it: cgroup v2 needs --cgroupns=host plus tmpfs mounts or the container
exits 255 with an empty `docker logs`, and `npm pack --ignore-scripts` skips the
prepare rebuild, so it asserts the version INSIDE the tarball.
onboarding-attempt.test.ts now pins hasGlobalHooks rather than reading it.
detectSetupState takes an injectable home and promises "every path is derived
from an injectable home/cwd", but hasGlobalHooksInstalled() takes no home and
walks the real user's settings files — so the assertion flipped the moment a
developer had failproofai installed on their own machine. The gap is noted, not
papered over.
Validation: 3108 unit, 320 e2e, 26 cargo blocks, clippy -D warnings, cargo fmt,
tsc. The one failing unit test asserts /etc/systemd/system holds no failproofaid
unit, which is false on a machine where the product is genuinely installed —
verified environmental: 35/35 with that unit moved aside.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01F9eufSorsg96cwpfSdamAk
The page said it "blocks invocations that include the sudo keyword" and that matching is token-based to prevent operator injection. The first half described a substring check that a path prefix defeated; the second described a property only the allowPatterns comparison had. It now states the real rule — an elevation binary in COMMAND POSITION, compared by basename after runners and prefixes are walked off — names doas, and gives the forms that are caught. And it carries a warning that was missing: an agent with arbitrary shell can still reach elevation through a variable, a base64 pipe, or a wrapper script, because no inspection of one command string can follow those. A guardrail against mistakes is worth having; letting someone believe it is a security boundary is not. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01F9eufSorsg96cwpfSdamAk
CI caught what the container test could not: the systemd lifecycle suite sets `FAILPROOFAI_DAEMON_BINARY` to `/usr/bin/sleep infinity`, and `binaryPath` is an ExecStart value — systemd accepts arguments there, and the env var is documented as "someone named a binary explicitly". `ConditionPathExists=` takes a PATH, so gating on that whole string looked for a file literally named "sleep infinity", never found it, and skipped a unit that would have run perfectly. Every real systemd lifecycle test went red. Splitting on whitespace to recover the binary is not the fix, because a path may legally contain spaces and there is no way to tell the two apart from here. Existence at render time answers it without guessing: a real binary is on disk when its unit is written (`installDaemonService` has just put it there), and a command-with-arguments is not. That is the same rule the worker script already followed — never gate on a path that is not there, or the freshly installed unit skips on its very first start. Verified by running the real-systemd suite as root in a privileged container, where all 7 lifecycle tests now pass. (The 2 that still fail there assert `Environment="HOME=..."` against a tmp dir and fail on `homedir()` under docker root; both pass on the host and neither goes through this code.) Also stops pinning an incidental tie-break in the CLI suggestion e2e. "unknowncommand" is a typo of nothing, so which subcommand comes out nearest is an artefact of the command LIST: it read "policies" only because three names tied at distance 12 and `SUBCOMMANDS[0]` broke the tie. `uninstall` lands at 10 and wins outright. The test now asserts the actual contract — a suggestion is offered and names a real subcommand — while the nearest-match behaviour stays covered by the neighbouring tests, which use inputs that ARE typos of something. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…faid-daemon-ux # Conflicts: # src/hooks/daemon-service.ts # src/hooks/worker-server.ts
`cache/hook-activity` is every decision the machine has ever recorded — the
data the dashboard's activity tab exists to show — and it sat inside `cache/`,
which the reset removed as a unit. Upgrading therefore discarded it. The message
even said so ("removed … activity history") while offering no alternative, which
made a silent data loss read as an intended one.
The log is now MOVED into layout 2's `hook-activity/`, and move-not-copy is the
design rather than an implementation detail.
The collector keys its cursors on `(device, inode)`, deliberately: the store
rotates by RENAMING `current.jsonl`, and a path-keyed cursor would get both
halves of that wrong at once — re-shipping the rotated file and carrying its
offset onto the fresh one. `rename()` preserves the inode, so every carried page
is still the file its cursor belongs to and resumes at the right offset. A copy
gives each page a NEW inode, which reads as never-seen and re-ships the entire
history. That difference is invisible on disk, which is why the e2e test checks
the inode number rather than the file contents.
`head_fingerprint` is what makes this safe rather than lucky. It was added to
defend against inode REUSE, and it verifies a file's first bytes exactly when a
resumed cursor's path has changed — which is precisely a migration. The same
guard answers both questions.
`EXDEV` — a `cache/` on a different filesystem, possible with bind mounts —
falls back to copy and accepts the re-ship, because a page left behind is data
lost and ingest dedups on a content hash. The legacy `current.jsonl` is carried
under a PAGE name: the destination has its own, possibly mid-write, and a
rotated page is exactly what the store would have made of it. `current.count`
and `stats.json` are dropped rather than merged — two derived counters cannot be
reconciled without inventing a number, and the store rebuilds them.
Two things had to change with it, or the move would have been pointless:
- `at("cursors")` was removed on the explicit principle of "one rule, no
exceptions", accepting a one-off re-ship. That was a reasonable call when
nothing was preserved and is the wrong one now: keeping the log while dropping
the watermarks is half a feature, since every carried page would re-ship
anyway. The test pinning that decision warned against a later "kindness"
quietly reintroducing an exception; this one was asked for, and the test now
records why it flipped rather than just flipping.
- `hookActivityDir()` was still in `resettablePaths()`, and the reset runs that
list AFTER the migrations — so the log was moved and then deleted moments
later. Caught by the e2e test, not by reading the code.
`cache/` is no longer removed wholesale either; its other children
(`cache/audit`, `cache/codex-session-paths.json`) are named individually so
nothing else quietly outlives the reset.
The reset message now says what was KEPT as well as what went, and counts the
carried pages rather than listing them — machine-generated names tell a reader
nothing they can act on, where the count answers the only question they have.
Tests: 11 unit (inode preservation, byte-identical records, the current.jsonl
rename, name collisions, derived counters, the reset no longer eating its own
output) plus an e2e harness that builds a real layout-1 home, upgrades it with
the real CLI, and compares the inode before and after.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01F9eufSorsg96cwpfSdamAk
…youts old `~/.failproofai/hook-activity.jsonl` is named in three places as the activity log. There is no such file. It is a DIRECTORY of paged JSONL — `current.jsonl` rotating into `page-<timestamp>-<seq>.jsonl` — and has been since the store gained paging, which predates the layout-2 move out of `cache/`. The same table also gave layout 1's `policies-config.json` and `hook.log` at the home root; both moved (`policies/local-policies/` and `logs/`). Someone following these docs looks for files that are not there and concludes nothing is being recorded. Noticed while carrying that directory across the layout upgrade — the migration is only worth anything if someone can find what it preserved. English only. The translations regenerate from it nightly. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01F9eufSorsg96cwpfSdamAk
Two changes that are really one: a machine should report to the origin its owner already has in their browser, and setup should not ask them which. **The default endpoint moves to the dashboard hostname.** `DEFAULT_INGEST_URL` was `https://server.befailproof.ai/v1/events` — the API server's own hostname, which therefore had to be a public name. It does not. The reverse proxy in front of the hosted deployment already routes `/v1/*` and `/enforcement/v1/*` straight to the server: `deploy/base/ingress/ dashboard-ingressroute.yaml` in agenteye matches `PathPrefix(/v1)` at priority 100 against the catch-all's 1. So `https://app.befailproof.ai/v1/events` reaches exactly the same handler, and the server needs no hostname of its own. That is also what makes ONE origin sufficient. Ingest, `/v1/auth/introspect` and `/enforcement/v1/*` all hang off the origin someone pastes, so `--connect <origin>` configures every capability from a single URL — and that URL is now the one already in their address bar rather than a second name they have to be told about. Nothing breaks: a machine with a URL recorded in `credentials.toml` keeps using it. This is only the value used when none was recorded. **Setup no longer asks for the URL.** There is one right answer for everyone on the hosted product, and asking made it look like a decision. Two failures came out of that within ten minutes of each other, and neither is one the person making it could have avoided: • the API key pasted into the URL field — the key prompt came second, so the first thing asked for was the thing nobody has a value for; • `http://localhost:3000` typed at it, which is the DASHBOARD. It 404s, correctly, because ingest is the API server. "Failproof Cloud URL" has no knowable answer other than the default already showing on screen. The two audiences that genuinely need another endpoint keep an explicit way to say so, neither of them a prompt: `FAILPROOFAI_CLOUD_URL` for local development and self-hosting, and `--connect <url> --token <key>` for scripted installs. `FAILPROOFAI_CLOUD_URL` deliberately, because the DAEMON already reads that exact variable for cloud-managed policy (crates/failproofaid/src/cloud_client.rs:85). One export now points the whole machine at one place, instead of the wizard and the daemon disagreeing about where this machine reports. The env value goes through the SAME `validateCloudUrl` a typed one did — it is not a trusted back door. http stays loopback-only, so a bearer token cannot be put on the wire in clear by setting a variable. And an unusable value CANCELS rather than falling back to the hosted endpoint: quietly reporting a machine to the hosted service is the one outcome someone who exported that variable did not ask for, and they would only find out by going looking for data that never came. The destination moved into the key prompt (`API key for app.befailproof.ai`), which is now the only place a key about to go somewhere unintended can be seen. **And the byte-identity is finally enforced.** Both copies of `DEFAULT_INGEST_URL` said "MUST stay byte-identical" and nothing checked. The CLI resolves a credential to VERIFY the endpoint at setup; the daemon resolves one independently to POST to it; neither reads the other's constant. A divergence therefore fails in the worst available way — the wizard reports success, the daemon reports healthy, and nothing arrives, invisibly from both ends. `default-ingest-url.test.ts` reads the Rust literal out of source (no cargo, so it fails in the quality job before anything is compiled) and also pins the versioned path, https, and that it stays a complete endpoint. Red-proven: diverging the Rust copy fails the identity test; re-introducing the URL prompt fails "never asks for the endpoint". Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
beta.8 is published, and its section had grown six entries that are not in it. Verified against the installed tarball rather than assumed from git history: the published build already carries `failproofai uninstall`, `ConditionPathExists`, `ELEVATION_RE` and the cwd-null fix, and still carries `server.befailproof.ai` — so the cut landed after the daemon work and before the ingest-origin change. Moved to a new beta.9 section: the two layout-upgrade fixes, the hook-activity docs correction, and the three ingest-origin changes. beta.8's ten shipped entries are untouched, so a reader of that section still sees exactly what that release contains. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…-beta.10
The daemon reads its ingest credential ONCE, at collector start. `main.rs`
starts the collector manager once and never tears it down, and the uploader
caches its bearer key at construction. Config is re-read on a 5s tick; the
CREDENTIAL is not.
So rotating a key left the file correct and the process wrong — and the failure
is invisible from every angle a person can check. `--connect` verifies the NEW
key itself and prints success. `systemctl` reports the service healthy.
`credentials.toml` holds a key that works when you curl it. And every batch 401s
and parks. The only symptom is data that never arrives.
Observed live, which is how it was found: a key revoked at 13:05:37 UTC and
replaced 37 seconds later still produced 401s twenty minutes on, with 26 batches
parked in `state/failed/` and a CLI insisting the machine was connected.
This was ALREADY KNOWN. `--disconnect` printed a line telling the user to
restart by hand. Printing the remedy is strictly worse than applying it: it
relies on somebody reading past a success message to discover the success is
conditional, and nobody does.
`reloadDaemonAfterConfigChange()` now runs from `--connect`, `--disconnect` and
the wizard's apply. Four things it deliberately does:
• verifies with `probeDaemon()`, not the exit code. `systemctl restart`
returning 0 proves the fork happened; a `Type=simple` unit is reported
active the instant it is forked. Trusting that is how a daemon that comes
back unable to evaluate gets reported as a successful reload.
• resets a latched start-limit first. `Restart=on-failure` plus a definition
that cannot start trips systemd's limit, and a latched unit then REFUSES a
legitimate restart.
• only touches a RUNNING service, and only when a connection actually
changed. On a `daemonConfigured` machine a restart is a window in which
tool calls fail closed — not a price to pay for a run that only picked
policies, and not this function's business for a service deliberately down.
• when it cannot restart, says so and gives the exact command instead of
printing success over a machine still shipping with the old key.
Dependencies are injected the way `connectionStatusLines` already takes
`daemonStatus`. That is not stylistic: these are module-internal calls, and ESM
binds them at module scope, so `vi.spyOn(mod, …)` leaves the function calling
the original. I wrote the tests that way first and three of them passed against
a stub that was never consulted — the most misleading failure available.
`cloud-enrollment-cli.test.ts` also had NO mocks, so an unmocked restart would
have bounced the developer's own daemon once per assertion during `test:run`.
Only the restart is stubbed there; the rest of the module stays real.
Red-proven: skipping the restart fails 3 tests, trusting the exit code instead
of probing fails 1.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Replaces the CLI-side restart shipped earlier in beta.10. That version only
covered changes our own CLI made, which is being told rather than reloading —
`config.toml` says "Safe to edit by hand" and means it, and a fleet tool, an
editor or a `sed` are all legitimate ways to change it. None of them run our
code, so none of them took effect.
The collector resolves its ingest credential once, when it starts, and the
uploader caches the bearer key at construction. Rotating a key therefore left
the file correct and the process wrong, and the failure is invisible from every
angle a person can check: `--connect` verifies the NEW key itself and reports
success, the service stays healthy, `credentials.toml` holds a key that works
when you curl it — and every batch 401s and parks. Observed live: a key revoked
at 13:05:37 and replaced 37 seconds later was still producing 401s twenty
minutes on, with 26 parked batches and a CLI insisting the machine was
connected. The only symptom was data that never arrived.
`spawn_collector_manager` now compares the whole on-disk `CollectorConfig` each
tick and cycles the collector when it differs. The whole config, not just the
credential: a stream switched off, a verbosity change and a redaction change are
all baked into the tasks at build time and none of them took effect either.
Four decisions worth stating:
• it cycles the COLLECTOR, not the daemon. A daemon restart is a window in
which a `daemonConfigured` machine denies every tool call, and re-reading a
credential does not justify that. The enforcement socket keeps serving.
• an unreadable config is "wait", not "disabled". A file caught mid-save
would otherwise tear down a healthy collector and rebuild it from the same
bytes a tick later — on a fleet tool, on every rewrite.
• the old generation is drained BEFORE the new one starts. Two collectors
sharing a spool directory would both claim the same batch files.
• disabling stops it and re-enabling starts it again, both without a restart.
`--disconnect` needing one is why a machine that had left its organisation
went on shipping; the reverse needing one would be half a fix.
`COLLECTOR_METRICS` had to stop being a `OnceLock`. Every publish after the
first was silently dropped, so a cycled collector left the telemetry lane
polling a generation that had already been joined and reporting its totals as
current — nothing errors, the numbers just stop moving, which looks exactly like
a healthy idle machine.
A race the tests caught: the manager recorded `current_collector_config()` after
spawning rather than the config it had just built from, so an edit landing in
that window was recorded as already-applied and never seen again. It now records
`next_cfg`.
Tests drive the REAL binary against real files on disk, because the property is
"an edit somebody else made is noticed" — a test calling an internal reload
function would prove something else. Red-proven: making the comparison always
report "unchanged" fails 4 of the 5. Verified stable across three consecutive
runs, and live against a real daemon: a hand-edited credential cycled it, the
socket stayed up throughout, and disable/re-enable both worked with no restart.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The collector never re-reads a file it has a cursor for. That is right for steady state and wrong exactly twice: when the dashboard's data was cleared or a machine re-enrolled, and when cursors advanced before there was anywhere to send. Both leave a machine whose transcripts exist locally and nowhere else, with no way to ask for them again short of deleting files by hand and hoping. Re-sending is safe by construction, not by luck. Re-reading is already the documented recovery path for a damaged cursor store — "starting over re-ships records the server already dedups" — and `redaction_is_deterministic` is an existing test, so a re-sent event hashes identically to its first send and collapses into the row that is already there. Shape follows what was asked for: 30 days by default (`--since 30d | 6m | YYYY-MM-DD`, `--dry-run` to look first), every agent CLI with sessions on disk, and only the streams `[collector]` enables — so a backfill can never ship transcripts on a machine that deliberately set `sessions = false`. It HANDS OFF via a request file rather than doing the work. The cursors it rewinds are held in memory by the running collector, which would write them straight back over; only the daemon can stop the collector first. A file, not an IPC call, because the CLI returns immediately so nothing is holding a connection to answer on — and a request that outlives a daemon restart is the one a person expects. The daemon deletes it BEFORE acting: a backfill that panicked mid-rewind must not be retried forever, because the cursors it already forgot would be forgotten again and the machine would sit re-shipping its history in a loop. What it does NOT hand off is the checking. No home, no credential, collection switched off — all verified synchronously before returning. Handing off an impossible request and printing success is the failure that already cost twenty minutes on a live machine while batches parked. THE BUG THIS ALSO FIXES, which the design work surfaced and which would have made the whole feature a lie: `new_cursor` refuses any file whose mtime is older than `since_days`, hardcoded to 7. Forgetting cursors for a 30-day window would therefore have re-read a WEEK, given the older files no cursor at all, and re-skipped them on every poll after. Silent, and the dashboard would look complete. The window is now widened for the rebuild a backfill triggers, and needs no clearing: `since_days` is only consulted for a file with no cursor, and after the rebuild those files all have one. Verified end to end against the real binary: a 30-day request forgot 241 cursors at `window_days=31`, then read 3,364 files older than the 7-day gate, the oldest 28.3 days — that count is 0 without the fix. Sizing worth knowing: that backfill produced 3,838 spool batches / 256 MiB (avg 66 KiB, max 8 MiB) on one developer machine. They drain through the existing 8-way uploader and park on failure like anything else, but it is not a small operation and `--dry-run` reports the file count before you ask for it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`rust-quality` runs BOTH `cargo fmt --all -- --check` and clippy. I ran clippy and not fmt, so three line-wrapping differences reached CI — the one gate I had not actually run locally. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Overall Verdict16 findings confirmed after adversarial re-verification (2 Critical, 6 High, 7 Medium, 1 Low). This is not safe to ship as-is — the two Critical findings are both silent-failure modes in core daemon lifecycle management (a crash-loop trigger and a stale-daemon-masquerading-as-current bug) that defeat the daemon's own restart/upgrade guarantees. Notably, most of these are not novel one-off bugs but gaps in fix patterns already applied elsewhere in this same PR: the panic-safe thread-spawn pattern from commit FindingsCritical: Unguarded
|
3 participants
Summary
Splits
failproofaiinto two binaries shipped from the same npm package:the existing
failproofaiCLI (unchanged entrypoint for every hook configacross all 11 supported agent CLIs) and a new
failproofaid, a persistentRust daemon that keeps policy evaluation warm instead of paying a full
cold-start cost (bundle parse, custom-policy temp-file dance, config reads)
on every single hook call.
Landing in stages on this one branch/PR, per the approved plan
(
/home/legion/.claude/plans/we-want-to-change-foamy-raven.mdin theoriginating session):
rust-qualityCI job,green before any daemon code exists. Also fixes a pre-existing bug
where the bun cache key (
hashFiles('bun.lockb')) never matchedanything since this repo tracks
bun.lock.failproofaidskeleton: Unix-socket server(
~/.failproofai/run/failproofaid.sock,0600inside a0700dir), length-prefixed JSON framing,
SO_PEERCRED/getpeereidpeerverification, a flock-based singleton guard, graceful SIGTERM
shutdown.
crates/PROTOCOL.mddocuments the wire contract.Node/Bun worker the daemon spawns and supervises, a TS thin client
(
daemon-client.ts) with a ~150ms connect+roundtrip budget, and afail-closed path for daemon-configured machines that reuses the
real per-CLI response shaping (not a hand-rolled generic denial).
Currently inert on every machine — gated behind a
daemonConfiguredmarker nothing sets until Stage 4.failproofai configinstalls/starts the daemon(systemd
--user/ launchd) and writes the marker. No separatefailproofai daemon installcommand, per explicit productdirection during planning.
GitHub Release assets, SHA-256 verified and downloaded by
failproofai config— see the update below), CI cross-compilematrix, expanded Docker clean-install test.
Key design points
machine has been daemon-configured (Stage 4), an unreachable daemon
denies the tool call rather than silently falling back to full
in-process evaluation — a loud, correctly-shaped deny instead of a
silent enforcement gap. A machine that's never been daemon-configured
(or is on Windows, out of scope for now) is completely unaffected: no
socket attempt, byte-for-byte the same behavior as today.
failproofaistays the single entrypoint every hook config invokes;daemon-awareness is entirely internal to it.
(socket + process lifecycle) that relays to a warm worker running the
existing, unmodified TypeScript evaluation engine.
Review round 1 — 11 findings fixed (d9b8eb3)
CodeRabbit's review surfaced four bugs that each silently defeat enforcement,
plus seven smaller ones. Each fix landed with a regression test that fails
against the old code.
Enforcement-breaking:
run_untilputs the listener in non-blockingmode; Linux discards that on accept (
accept4) but BSD-derived kernelsinherit it. So on macOS
read_messagereturnedWouldBlockbefore theclient's bytes landed,
handle_connectionread that as a malformed frame andanswered with silence, and every hook call on macOS — a platform this PR ships
launchd support for — fell through to the fail-closed deny.
bound it, so
socket_path.exists()saw the dead worker's leftover file theinstant a new one spawned and handed
call()a socket nothing was listeningon. Readiness is now a real
connect(); the stale path is cleared beforespawn;
Dropcleans up after itself. Both new tests fail on the old code withECONNREFUSED.daemonConfiguredtracked the service manager, not a daemon. Granted themoment
systemctl enable --nowexited 0 — which a daemon that dies at startupalso does — and never revoked on uninstall. Since the CLI fails closed on that
flag, either end of the lifecycle left the machine denying every hook event
across all 11 CLIs. Install now waits for the service to reach and hold a
running state (a
Type=simpleunit reports active the moment it forks, so onereading waves through exactly the crash-at-startup case); uninstall clears the
marker first and unconditionally.
client timeout is a deny, not a fallback — and that budget had to cover the
whole roundtrip, which
handler.tsallows 10 s per custom policy andworker-server.tsserializes. Split into a 150 ms connect probe (a deaddaemon still fails fast) and a 30 s response budget matching
worker.rs'sown read timeout.
Also fixed:
process.exit()discarding unflushed stdout on the--hookpath(measured: 2 MB written, 146 KB delivered — it truncates the decision payload the
CLI parses); the worker's piped stdout/stderr never being drained (a chatty custom
policy fills the pipe buffer and blocks the worker mid-write);
worker-server.tsdecoding only one frame per
dataevent; connections having no read/writedeadline and no cap; unbounded
systemctl/launchctlcalls hanging the wizard;daemon-install telemetry sending an errno string containing a
homedir()-derivedpath (i.e. the OS username); and an e2e harness that orphaned a live daemon on any
assertion panic.
CI:
darwin-x64used the retiredmacos-13label — an unknown label doesn'tfail, it just never gets a runner, which is why that leg sat pending from the
moment this PR opened. Now
macos-15-intel. The release-artifact job also shareda writable cargo cache between
pull_requestandreleasetriggers (restore-onlyon PRs now, save on release/dispatch), and the two checkouts that compile
third-party crates set
persist-credentials: false.Round 2 (88ee006): the release build now passes
--locked, so the binary users install comes from the committedCargo.lockrather than whatever Cargoresolves in the runner.
Socket: the one alert is
cargo/zerocopy@0.8.55flagged as likely obfuscated.It reaches us only through
proptest, a dev-dependency ofcrates/fpai-ipc, andis absent from
cargo tree -p failproofaid -e normal— nothing published containsit. Ignored with justification in a PR comment.
Testing
cargo test --workspace), including a live end-to-endtest that spawns the real compiled
failproofaidbinary, which spawnsthe real
bun-run TS worker, which runs a realblock-sudopolicyevaluation — not mocked at any layer.
daemon-client.ts/worker-server.tstested against realnet.Server/Unix sockets, not mocks, per the plan's verificationsection.
bun run lint,tsc --noEmit,bun run test:run) with zero regressions —handleHookEvent's publiccontract is byte-for-byte unchanged, so the existing 1300+ line
handler.test.tssuite needed no edits.subprocess when
sh -c "bun ..."forked rather than exec'd) and wasfixed with process groups + piped stdio; see the Stage 3 commit message
for the full story.
🤖 Generated with Claude Code
Summary by CodeRabbit
Update — the daemon now ships from GitHub Releases, not npm platform packages
Stage 5 originally shipped four
@failproofai/failproofaid-<os>-<arch>packages, declared as
optionalDependenciesand pinned to the root version.Nothing ever published them.
build-daemon.ymlonly uploaded the binariesas Actions artifacts, and this branch never touched
publish.ymlat all — soall four names 404 on npm today, and a released CLI would have resolved a
daemon that does not exist. #634 fixes the pipeline; this branch removes the
channel it would have had to publish, because the release assets have to exist
regardless for anyone installing failproofaid on its own, and a second channel
is a second thing to keep in step with the first.
How it works now.
src/hooks/daemon-download.tsfetchesfailproofaid-<os>-<arch>.gzfrom the release tagged with this CLI's ownversion, verifies it against the published
SHA256SUMSbefore decompressing,and installs it to
~/.failproofai/bin/failproofaid-<version>by atomic rename,mode 0755.
package.json's version, never discovered —no API call, no rate limit, no
releases/latestredirect, and no way to run adaemon built from different source than the CLI talking to it.
binary (
ETXTBSY) or silently repointing a live service unit.not warnings — what this writes is an executable a service manager runs at
login.
resolveFailproofaidBinaryPath()stays apure disk check, so the hook path can never block on the network.
FAILPROOFAI_NO_DOWNLOAD=1opts an air-gapped machine out (analready-installed binary keeps working);
FAILPROOFAI_DAEMON_BASE_URLpointsat an internal mirror, and at a local HTTP server in the tests.
build-daemon.ymlnow gzips each binary and uploads that — which also makesupload-artifactdropping the executable bit a non-issue — and matches #634'scopy so the rebase onto main is a no-op.
Verified: 13 new download tests against a real local HTTP server (checksum
mismatch, manifest with no entry, 404, disabled-downloads opt-out, atomic
install, mode 0755); daemon-service resolution tests now run against a scratch
HOME; a cleannpm pack+ container run confirms the shim reports the confighint with nothing installed and execs an overridden binary; all four
cross-compile legs green.
Update — OSV-Scanner green (
brace-expansion5.0.8 → 5.0.9)The Supply Chain gate was failing:
brace-expansion@5.0.8is affected byGHSA-rgw5-rvv9-x895 (high, CVSS 7.5) — a
DoS via unbounded intermediate arrays that bypasses the CVE-2026-14257
mitigation — fixed in 5.0.9 on the 4.x/5.x line. OSV-Scanner blocks on any
finding, so the check went red the moment the advisory published.
The finding isn't introduced by this branch (
mainresolves the same 5.0.8),but it blocks this PR, and
osv-scanner.tomlstates the preference plainly:fix (bump, or pin via
overrides) rather than allow-list. The pin alreadyexisted, so this is a one-line bump of that override. Because
overridesapplies tree-wide it covers every consumer at once —
minimatch@10undereslint/next, plus the
^1.1.7requests from the oldereslint-plugin-*minimatches — and
brace-expansionis the only entry the resolved lockfilemoves (2-line
bun.lockdiff).Verified by replaying the CI scan's own inputs: batch-querying
api.osv.devwith every package
bun.lockresolves reproduces the exact CI failure againstthe pre-fix lockfile (1 finding,
brace-expansion@5.0.8→GHSA-rgw5-rvv9-x895) and reports 0 findings across all 644 packages after.bun run lint,tsc --noEmitand the unit suite are unchanged.Also merged
mainin (askillssubmodule bump) so the branch contains everycommit on
mainbefore pushing — merged rather than rebased, since rewriting 61published commits to absorb a one-line submodule pointer costs more than it buys.
Update — the daemon binary now ships through npm too (dcf13d7)
Stage 5 shipped one channel: a fetch from the GitHub Release at
failproofai configtime. That leaves a proxy, an air-gapped box or arate-limited runner with a CLI and no daemon, and gives no way to install the
CLI itself without the npm registry. Both are closed here, without removing
the release assets — they stay the standalone channel.
npm platform packages. The four binaries publish as
@failproofai/failproofaid-<os>-<arch>with theosandcpufields set,pinned as
optionalDependenciesof the root package, sonpm install failproofaialready brought down the one matching the machine.ensureFailproofaidBinary()copies it in before it considers the download — nonetwork, so
FAILPROOFAI_NO_DOWNLOAD=1deliberately does not gate it (thatflag exists so an air-gapped machine does not reach out, and on exactly those
machines npm is the only channel that can supply a daemon).
Both channels land the binary at the same versioned path under the user's
.failproofaibin directory through oneinstallBinaryBytes()—ExecStartnever points intonode_modules, becausenpm i -g failproofai@nextwould swap the binary under a running service and anuninstall would delete it out from under an enabled unit that then crash-loops
at every boot.
The first attempt at this is what shapes the second. The pins shipped once
before with nothing published behind them, so every install resolved four 404s
(removed in 1.0.0-beta.3). So
scripts/build-daemon-packages.mjspublishes thefour packages before the root package that pins them, fails the release
rather than warning when one cannot be published, and writes the pins in the
same invocation that publishes them — injected at publish time, never
committed, so a pin cannot name a version that was not published and this
repo's own
bun install --frozen-lockfilekeeps working. Both orderings areasserted in
__tests__/ci/release-pipeline.test.ts.The CLI tarball is a release asset now.
failproofai-<version>.tgz, packedby a new
cli-tarballjob at the version being published and covered by thesame
SHA256SUMS, so the CLI installs with no registry at all. Not gated onhas_daemon, and its failure blocks the npm publish since it runs the samebuild.
The 14 typo-squat alias stubs pin the same four packages.
Verification. Unit + e2e green.
npm publish --dry-runfor all fourplatform packages against the real
v1.0.0-beta.3artifacts, with the packedtarball confirmed to preserve
-rwxr-xr-xon the binary. End to end in asystemd container: the npm-installed binary copied into place with downloads
disabled and the base URL pointed at a dead port, the service installed from
it, and the daemon still answering socket pings and live hook events after a
reboot.
Prerequisite before the first release that carries this: the
@failproofainpm scope must exist and
NPM_TOKENmust own it. Nothing is published under ittoday. If it is missing, the platform publish fails the job and nothing is
published at all — deliberately, since a root package pinning names that do not
resolve is the failure this replaces.
Two follow-ups the dry run and a second read caught (4bcc2f6, ec75874)
the exact version, but a workspace holding two
failproofaiversions canhoist the other one's platform package to the top of the tree — and
installing that binary under this version's filename would put a daemon
built from different source behind a CLI that believes it matches. The
candidate's manifest version is checked before its binary is used; a
mismatch falls through to the download, whose URL is pinned to this version.
by the first pipeline dry run, not by reasoning:
npm publishre-runsprepare, so the Next build happens again after the platform-package step,and Next's tracing sweeps the whole project root into
.next/standalone(
scripts/prune-standalone.mjsalready calls this out as "over-tracedproject artifacts"). Downloading the build matrix's output into the
workspace therefore shipped 16 MB of daemon
.gzassets inside thefailproofaipackage — every platform's binary, in the one package thatdeliberately carries none. The publish job now downloads into
RUNNER_TEMP,the platform packages stage in the temp dir rather than the checkout, and
both names joined the standalone prune list so a local
npm packcannotreintroduce it. Both are asserted in
__tests__/ci.The publish pipeline can no longer strand orphan platform packages (0681e79)
A
workflow_dispatchof this branch at1.0.0-beta.0failed (Actions run30906933501), and the way it failed is the point. A dispatch has no versioninput — the publish version is whatever
package.jsoncarries, and a featurebranch's is routinely a version that shipped long ago — while the root package
publishes last. So the run went through the full 4-way cross-compile,
attached the release assets, published all four
@failproofai/failproofaid-<os>-<arch>packages, and only then tookE403 You cannot publish over the previously published versionson the rootpackage. Those four platform packages are now on the registry at
1.0.0-beta.0,a version whose CLI is published with no
optionalDependenciespinning them, andnpm's 72-hour window is the only way to remove them.
one
npm view, ahead of every other job, so a burned version costs secondsinstead of a full matrix plus four irreversible publishes. Deliberately ungated
on
dry_run: a dry run that validated a release which cannot happen is not auseful dry run. Asserted in
__tests__/ci/release-pipeline.test.tsalongsidethe rest of the release wiring.
1.0.0-beta.5so this branch carries somethingpublishable —
1.0.0-beta.0through.4are all on npm.block-version-bumpspolicy, which reservedpackage.jsonversion edits forluv-cut-X.Y.Zbranches. It blocks the onlyfix for a burned publish version, and the preflight check now catches the drift
it was guarding against at the point where it actually matters.
For the record, the npm/release-asset split that prompted the dig:
failproofaipublished at beta.0-beta.4, but the platform packages exist only at beta.0 and
beta.4 — beta.1-beta.3 predate the
build-daemon-packagesstep landing inpublish.yml (dcf13d7), so they shipped with the release
.gzassets but no npmpackages and empty pins.
A release now proves it reached users, on every platform (3671a0c)
Two checks after the publish, because "the run went green" has twice not meant
"users got a release".
Registry check (in the
publishjob). Every published name must resolve atthe publish version, and the published root package's
optionalDependenciesmust pin that same version. Construction already guarantees lockstep — the root
package, the four platform packages and the aliases all take one
PUBLISH_VERSION— but it cannot cover a partial run, and both halves ofthat split have shipped once each: beta.1-3 published the CLI with no platform
packages behind it, and beta.0 published four platform packages whose CLI was
already on the registry without pins to them. Both runs reported success.
Install check (new
verify-installjob). A realnpm install -gfrom theregistry on a clean runner, then both binaries actually invoked. The CLI must
report the published version; the daemon must resolve the way the CLI resolves
it at runtime (through the installed package, not by path), be executable, and
report the same version.
npm viewproves a manifest is queryable — it does notprove the tarball is fetchable, that the platform filters resolve the right
package on the machine it is meant for, that the executable bit survived the
publish-then-install roundtrip, or that the binary matches the CLI beside it.
Each of those fails while every manifest query still reads as healthy.
It is a matrix rather than one runner because npm installs the one platform
package matching the runner's os and cpu, silently skipping the other three, so
a single leg can only ever verify a quarter of what shipped. Same four legs as
the build matrix, each native to its own target.
Both checks retry immediately, then back off at ten seconds, thirty seconds, one
minute, two minutes — long enough that read-through-cache propagation is not
mistaken for a failed publish, short enough that a genuinely failed one is
reported in the same run rather than hours later by a user.
Absorbed dependency bumps
react19.2.8,react-dom19.2.8,@types/react19.2.18 (commit 4cd9ad0).These land here rather than as their own PRs. Dependabot opened #652 (react +
@types/react) and #607 (react-dom) as two separate PRs, but React refuses to
render when
reactandreact-domare not the exact same version, so neitheris green on its own — #652 applied alone fails 15 component test files with
Incompatible React versions. #652 is closed in favour of this commit.