[failproofaid] Split failproofai into a CLI + Rust background daemon - #632
Open
NiveditJain wants to merge 15 commits into
Open
[failproofaid] Split failproofai into a CLI + Rust background daemon#632NiveditJain wants to merge 15 commits into
NiveditJain wants to merge 15 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 |
NiveditJain
added a commit
that referenced
this pull request
Jul 31, 2026
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
|
All alerts resolved. Learn more about Socket for GitHub. This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored. 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
NiveditJain
added a commit
that referenced
this pull request
Jul 31, 2026
…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
Member
Author
|
@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
NiveditJain
added a commit
that referenced
this pull request
Jul 31, 2026
…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
NiveditJain
force-pushed
the
failproofaid
branch
from
July 31, 2026 13:02
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
All three were introduced with the system-scope service, and all three are real. The staging file for the privileged write was named from pid + timestamp in the shared temp dir. That is guessable, and writeFileSync follows a symlink already sitting at the path — so on a multi-user box another local user could pre-create it and have `install` copy content they control into /etc/systemd/system as root. Staging now happens inside a mkdtempSync 0700 directory, whose name cannot be pre-created. FAILPROOFAI_WORKER_CMD joined two paths with a space and no quoting, and the daemon runs that value through `sh -c` (WorkerCommand::Shell in crates/failproofaid/src/worker.rs). Any path containing a space split into fragments and the worker never started — ordinary on macOS (/Users/First Last/…), and newly likely because the absolute process.execPath replaced a bare `node`. Both halves are shell-quoted now; systemd's Environment="…" quoting does not help, because that protects the unit parse, not the later shell split. The launchd label was still a fixed string while the systemd unit was already per-user — so a second Mac user's install overwrote the first's daemon (UserName, the ExecStart path under their own ~/.failproofai/bin, their log paths) and their uninstall deleted it. Label and plist path are namespaced per user, and the shared 1.0.0-beta.1 LaunchDaemon is stopped and removed on install like the legacy LaunchAgent, since it holds the same singleton flock. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EbXKFnpRXBwsrvUJDdySky
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.