Repository navigation
Harden local Guardian findings integration - #3
Conversation
Co-authored-by: gagan114662 <[email protected]> Signed-off-by: gagan114662 <[email protected]>
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
![xhawk-ai[bot]](https://avatars.githubusercontent.com/in/2457056?s=60&v=4){kind=small}
There was a problem hiding this comment.
📋 Review Summary
I reviewed the Numbat/Guardian integration that installs monitor-only hooks, stores local findings, and projects session-scoped findings into the agent panel. The direction matches the safety boundaries, but two reliability issues should be addressed: the hook installer can indefinitely block managed-agent startup, and the UI filters by the currently active turn so valid findings disappear or never appear once a turn completes.
🔍 Findings
🟠 High 1 · 🟡 Medium 1
| let output = Command::new(binary) | ||
| .args([ | ||
| "hook", | ||
| "install", | ||
| "--agent", | ||
| runtime, | ||
| "--emit", | ||
| "findings", | ||
| "--output", | ||
| "file", | ||
| "--output-file", | ||
| ]) | ||
| .arg(&findings) | ||
| .output() |
There was a problem hiding this comment.
🟠 High
This subprocess runs synchronously in the managed-agent spawn path, and Command::output() has no timeout. A broken or hanging numbat hook install will block agent start/restore indefinitely even though monitoring is best-effort, and the manual start path holds the managed-agent store/runtime locks while this executes. Run this off the critical path or enforce a short timeout and record disconnected on timeout.
| agent.pubkey, | ||
| sessionChannelId, | ||
| activeSessionId, | ||
| activeTurnId, |
There was a problem hiding this comment.
🟡 Medium
Using activeTurnId as the required projection scope drops valid findings as soon as isWorking becomes false. Guardian records can be flushed at or just after turn completion with exact session/channel/turn provenance, but the hook then passes null for turnId, causing the Rust projection to return no findings and the panel to clear them. Scope to the latest/displayed turn id rather than only the currently working turn so completed-turn findings remain visible.
gagan114662
left a comment
gagan114662
left a comment
There was a problem hiding this comment.
Blocking review — do not merge yet.
-
Agent isolation is still not enforced.
numbat_findings_pathvalidates the requested pubkey but returns the same globallive.ndjsonfor every agent (numbat_findings.rs:82-85).project_findingrequires session/channel/turn but never comparesrecord.source_agentwith the requested managed agent (:131-170), and the Tauri command accepts any syntactically valid pubkey (:405-424). Therefore the API has no structural proof that a displayed record belongs to the viewed agent. Exact context matching prevents relabeling, but it does not establish agent ownership. Require a trustworthy record identity bound to the managed agent, or keep agent-scoped findings hidden until that identity exists. Add a negative cross-agent test. -
Private storage fails open on non-Unix platforms.
set_private_permissionsreturnsOk(())without applying any protection undercfg(not(unix))(:312-315), while the code proceeds to create/write the raw evidence and health files (:336-364). Raw Numbat evidence can contain commands, paths, endpoint data, hashes, and event IDs. Implement a Windows ACL/private-file path or explicitly disable the sink where owner-only storage cannot be guaranteed; add platform coverage. -
Agent startup can hang on Numbat configuration. Every supported managed-agent spawn synchronously calls
prepare_numbat_monitoring(runtime.rs:614-621), which runsnumbat hook installvia blocking.output()with no timeout (numbat_findings.rs:366-380). A stuck or slow external binary now blocks the normal spawn critical path. Reconcile configuration outside that path, or use a bounded child process and make Guardian failure non-blocking.
The provenance rewrite and wrong-turn Cancel vulnerabilities are fixed, the health wording is more honest, and the privacy projection is appropriately narrow. Those improvements are not enough to merge while the ownership, storage, and startup boundaries above remain open.
Verification context: PR head caca6401c85c567bdee13525bc472173ae3e21a5, base 9bee25bef600f8e13b0f75e3a438a0995a1606a1; GitHub reports mergeable/clean but no CI checks are configured.
Co-authored-by: gagan114662 <[email protected]> Signed-off-by: gagan114662 <[email protected]>
|
Re-review at head Verification at this exact local head: TypeScript passed; 3,752/3,752 desktop tests passed; full unrestricted serialized Rust suite passed (1,855 library + 3 diagnostics, 14 ignored); E2E production build passed. |
Summary
configured/unverified, clear stale health on scope changes and polling failures, and protect/rotate local evidence storageSafety boundaries
Verification
cargo fmt --all -- --checkcargo fmt --manifest-path desktop/src-tauri/Cargo.toml -- --checkpnpm typecheckFollow-up
Agent-bound display and safe cancellation should only be added after Numbat records carry a Buzz-verifiable managed-agent identity and the cancellation API accepts an atomic expected session + turn.