Conversation
lsm
left a comment
There was a problem hiding this comment.
🤖 Review by ox-alpha (Stealth)
Model: ox-alpha | Client: HyperNeo | Provider: Stealth
Diff: +1071/−86 across 12 files — code 391 | tests 680 | comments 0 | other 0
Round-1 review of the full PR against head 6d483445b. A test-heavy slice (~64% of added lines are tests) wiring workspace-scoped safe-fs read/write and terminal host callbacks (AskUserQuestion-gated), command-identity digest stamping, the SIGTERM → group-probe → SIGKILL close ladder, and the lifecycle abort-before-stop fix into the ACP runtime. Task #1204 scope is fully covered: every listed race fix is present and correctly implemented; both prior inline threads (win32 direct-pid probe, raw-command persistence) are verified fixed in e52c66bf/6d483445 and their resolutions are accepted; CI is fully green on head; the descope drops are enumerated in the PR body as the task requires.
The core engineering is sound. Path confinement survived a dedicated adversarial pass — no escape was constructible (lexical + realpath dual assertion in front of an O_NOFOLLOW per-component openat walk; traversal, in-workspace symlinks, symlinked workspaces, null bytes, FIFOs all fail closed). Approval gating is fail-closed at three layers; the terminal env is a strict allowlist; basicAcpProcessTreeOwner.terminate preserves every old kill fallback including win32 non-detached children; updateSession metadata merge semantics (undefined deletes keys) match every new clear.
1 blocking (P1) and 6 additional findings (P2) below — mostly a CI-execution gap for the retained confinement coverage and small guards on teardown races.
Findings
- P1 — acp-query-runner.test.ts:615 — the workspace-confinement test is silently skipped in every CI run (CI runs daemon units under vitest, so process.versions.bun is undefined → test.skip). The only coverage of read ranges, escape rejections, nested-write creation, and the realpath fallback never executes. Fix: split rejection-only assertions (pure node:path logic, no bun:ffi) into an ungated test.
- P2 — acp-query-runner.ts:1438 — post-abort drain while(true) has no internal bound and no continuing-producer test; bounded today only by external force-kill schedules.
- P2 — acp-transport.ts:380 — group-gone short-circuit abandons a live child that left its original process group (setsid); the old code's direct proc.kill fallback always signaled the child.
- P2 — acp-query-runner.ts:388 — permission prompt is created even when the signal is already aborted → transient ghost approval card.
- P2 — query-lifecycle-manager.ts:193 — the new queryAbortController.abort() is the only unguarded teardown step in stop() and can abort a newer run's controller in a concurrent stop/start race.
- P2 — acp-query-runner.test.ts:1106 (+ 1-core/acp-transport.test.ts:344) — two new timing-dependent tests have thin margins (exit-before-flush JSON.parse('') race; 200 ms sleep asserting a 50 ms timer).
- P2 — acp-query-runner.ts:573 — the identity digest covers raw argv including potential inline secrets and is broadcast to every web client; digest-of-a-low-entropy-token is offline-brute-forceable. Salt/HMAC or exclude secret-shaped args.
Details in the inline comments.
Verified OK (highlights)
- All task-scope race fixes traced end-to-end: pendingNext preservation + interrupted-tool-result flush (translator state clears on flush, so no double-delivery); startup-timeout cancel()/closeSession() before close; stale-startup aborts at both expensive windows; usage clearing on reset and identity change; identity-change session reset with the legacy preserve path documented as dropped.
- Security: no P0/P1. Confinement, approval gating, terminal scoping (per-run manager, random ids, 1:1 pipe), and env allowlist all held; deny/cancel/malformed outcomes default-deny; no shell interpolation in terminal spawn.
- Compatibility: additive options only; the one other production AcpClient site (acp-provider.ts) is unaffected via the default tree owner; upgrade-safe for existing sessions (no mass ACP resets); knip @public removals valid; acpSessionCommand has zero remaining references.
- Tests pin most reintroduced-bug classes (lost tool results on abort, hung stop, missing startup teardown, identity regressions, ladder regressions).
Observations (non-blocking)
- Probe-on-recorded-pid has a theoretical pid-reuse window that could signal an unrelated group — standard limitation without pidfd; the old code's alternative was orphan leaks, and EPERM-as-alive is conservative-correct.
- The unref'd escalation timer may not fire during daemon quit (matches house style in acp-terminal-manager).
- A throwing custom processTreeOwner mid-spawnProcess would orphan the spawned child — unreachable today and the throw path is deliberately tested.
- Craft follow-ups: dynamic imports of statically-importable builtins in resolveWorkspaceSegments (+ fs/promises vs node:fs/promises specifier inconsistency); closeProxyBridge param now disposes two resources; hostCallbacks IIFE and the identity-reconciliation block could be private methods; agent-chosen permission labels could spoof semantics (mitigated by rendering kind); fs-write approval shows path but not a content preview.
- The pre-existing test name "close sends SIGKILL after timeout" never reaches SIGKILL; the newer escalation test is the real pin.
Verdict: REQUEST_CHANGES — 1×P1, 6×P2. Everything else reviewed clean; the fixes are all small and localized.
Recommendation: REQUEST_CHANGES
- split workspace-escape rejections into an ungated test so confinement coverage executes under CI's vitest runner (P1) - bound the post-abort iterator drain by message cap and deadline, with a continuing-producer test - keep signaling the leader when its process group is gone (setsid): direct SIGTERM plus leader-based SIGKILL escalation - skip permission prompts when the query signal is already aborted - snapshot the abort controller in stop() so teardown cannot abort a newer run's controller - redact secret-shaped argument values from the command identity digest so persisted digests cannot be brute-forced against low-entropy tokens - replace timing-thin sleeps with deadline polling in the terminal env and SIGKILL escalation tests
|
Review round 2 addressed in b459143 — all 7 findings fixed: P1 — vitest-skip of confinement coverage: split the rejection-only assertions (absolute/relative escapes, workspace-as-file) into the ungated P2s:
Typecheck, lint, format, knip, and no-comments all green locally; CI will run the suites on this head. |
…eat-daemon-wire-acp-runtime-query-runner
|
Head updated to |
- deliver only tool results from the post-abort drain so ordinary agent output stops at interruption, keeping the message cap and deadline bounds - orphan the pending AskUserQuestion card when an ACP permission request is abandoned by abort, so the handler resolver does not linger for same-id replays - rework two permission tests onto the held-client pattern so the deny and prompt-before-create paths run against a live signal; the runner aborts its controller after every query, which now short-circuits prompts without leaving a ghost card
…rminals - anchor secret-argument matching to credential-shaped flag names so non-secret flags like --max-tokens or --password-policy still differentiate the command identity digest - build the ACP terminal environment from the merged agent/session environment instead of ambient process.env so session-configured allowlisted variables (PATH, HOME, proxies) reach approved terminal commands
…limit AskUserQuestionHandler truncates displayed questions at 2000 characters, so a longer terminal command (or fs-write path) could be approved with its tail invisible. Permission requests whose question exceeds the handler's display limit now fail closed before any prompt is shown.
A throwing processTreeOwner after a successful spawn previously left the subprocess running with no transport handle to close it; kill the child with the basic owner before rethrowing, mirroring AcpTerminalManager.
… secrets - await the in-flight iterator result before flushing pending messages so a tool_call_update completing during abort is delivered once instead of being followed by a synthetic duplicate - redact secret-shaped argument values from the terminal approval question (which AskUserQuestionHandler persists into resolvedQuestions metadata) while still executing the original arguments
The reordered abort drain awaited pendingNext unconditionally before any deadline existed, so a producer that ignores cancellation could stall query teardown until the transport lifecycle timeout. The drain deadline now also races the in-flight read and the trailing iterator.return(), settling the query within the drain budget instead.
- treat -H/--header/--oauth2-bearer style flags as credential carriers and always redact the value of secret-named flags, including values that begin with a dash - apply the deadline-bounded iterator.return() to every aborted path, not only after the first delivered message
…iring - redact -H/--header values only when the header name itself is credential-bearing, so ordinary headers stay visible in approvals and differentiate the command identity - re-check startup activity after spawning the client and before the prompt loop, and close the run-local client when a stale query's cleanup is skipped, so a stop during startup cannot orphan the child - deliver the in-flight post-abort result unfiltered and let tool_use messages through the drain so synthesized results keep their corresponding tool invocations
… output - secret-named flags only consume a value that is not itself a long flag, so a valueless --token no longer swallows the next argument in the approval display - recurse redaction into -c shell command strings so nested credential flags and secret headers are redacted in persisted approval text - apply the post-abort relevance filter to the in-flight iterator result as well, keeping only tool_use/tool-result messages
- positionals no longer steal command attribution, so curl keeps -H/-u handling after URL arguments - separators glued to adjacent words (ok;, ok&&) restart assignment tracking like standalone operators - everything after -- is positional: flags there stay visible in approvals and differentiate the identity digest
…late processes - shell scripts beyond the recursion depth are replaced wholesale with a redacted placeholder when they contain credential-shaped flags, instead of being returned verbatim - fs-write approval titles disclose the content size in characters so benign and destructive replacements are distinguishable - stop() terminates agent processes that appear in the tracked snapshot during the stop window, reaping a child spawned after the entry snapshot while its query was being stopped
- attribute flags to the command launched through env wrappers - redact secret assignments embedded between shell separators inside a single token (echo ok;API_TOKEN=x curl) - recognize credential carriers in clustered curl short options (-sH/-sU) with attached or separated values - apply userinfo redaction to non-secret option values (--url=…) - terminate late processes during stop only while the entry query still owns the context, sparing a replacement query's child - pass a short stop timeout in the replacement-reference test and assert startup liveness after authenticate so stale runs cannot persist an ACP session id
|
Work concluded for scope review — head For the breakdown review — suggested follow-up slices from what grew here:
All ~30 bot-review rounds' findings are either fixed with pinned tests or declined with documented rationale on-thread. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ace08b11b7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| await iterator.return?.(); | ||
| } catch {} | ||
| pendingNext?.catch(() => {}); | ||
| if (signal.aborted && messageDelivered) { |
There was a problem hiding this comment.
Drain the first in-flight message after abort
When interruption happens before any message has been yielded, messageDelivered is false, so this skips both the pending iterator.next() result and flushPendingMessages(). If the first ACP update arriving during that race is a tool call, its tool-use/result pair is discarded even though the new cleanup is intended to preserve in-flight tool activity, leaving the persisted transcript inconsistent; drain whenever a prompt has an in-flight iterator result rather than requiring an earlier delivered message.
Useful? React with 👍 / 👎.
|
|
||
| function redactShellCommand(script: string, depth: number): string { | ||
| if (depth <= 0) { | ||
| return SCRIPT_SENSITIVITY_PATTERN.test(script) ? '[redacted script]' : script; |
There was a problem hiding this comment.
Detect secret assignments at the recursion cutoff
When shell nesting reaches the cutoff, the sensitivity pattern recognizes credential flags but not secret-shaped environment assignments. For example, sufficiently nesting API_TOKEN=topsecret curl https://a under sh -c returns the remaining script verbatim, exposing the token in the terminal approval title and persisted resolved question. Fresh evidence after the prior cutoff fix is that assignment-only scripts still bypass its conservative fallback; include credential assignments in the cutoff detection.
Useful? React with 👍 / 👎.
| const redacted: string[] = []; | ||
| for (let index = 0; index < args.length; index++) { | ||
| const arg = args[index]; | ||
| if (options.shellScript === true && (SHELL_OPERATORS.has(arg) || /[;&|]$/.test(arg))) { |
There was a problem hiding this comment.
Reset command tracking across all shell boundaries
The parsed token stream discards newlines and this condition recognizes only standalone or trailing operator tokens, so valid scripts such as echo ok\ncurl -H 'Authorization: Bearer topsecret' and echo ok;curl -u alice:topsecret never attribute the later flags to curl. Their credentials remain in the approval title and persisted resolved question. Fresh evidence beyond the prior adjacent-separator fix is that multiline boundaries and a command word attached after the separator are still not tracked.
Useful? React with 👍 / 👎.
| let inLeadingAssignments = | ||
| options.shellScript === true ? command.indexOf('=') > 0 || isEnv : true; | ||
| let currentCommand = command; | ||
| let awaitingCommandWord = options.shellScript === true || isEnv; |
There was a problem hiding this comment.
Follow credential consumers launched through command wrappers
For wrapper executables other than env, awaitingCommandWord starts false and currentCommand remains the wrapper forever. Thus common requests such as sudo curl -H 'Authorization: Bearer topsecret' /a or command curl -u alice:topsecret /a retain the credential in the approval title and persisted resolved question. Fresh evidence after the env wrapper fix is that the same bypass remains for other command-launching wrappers; attribute their wrapped executable before applying tool-specific redaction.
Useful? React with 👍 / 👎.
| } | ||
| if (!arg.startsWith('-')) { | ||
| const looksLikeUrl = /^[a-z][a-z0-9+.-]*:/i.test(arg); | ||
| if (options.shellScript === true && !looksLikeUrl && /[;&|]/.test(arg)) { |
There was a problem hiding this comment.
Preserve quoted assignment-shaped command data
This textual operator scan has no quote context, so a semicolon inside an ordinary quoted argument reopens assignment handling. For example, sh -c "rm 'x;API_TOKEN=one'" is displayed as deleting x;API_TOKEN=[redacted], and changing one to two yields the same command digest. Fresh evidence after assignment-position scoping is that quoted filenames containing a separator still collapse, hiding behavior-changing input from the approval; only treat unquoted shell operators as command boundaries.
Useful? React with 👍 / 👎.
| if (staleTimer && staleTimer === startupTimeoutTimer) { | ||
| clearTimeout(staleTimer); | ||
| this.ctx.startupTimeoutTimer = null; |
There was a problem hiding this comment.
Clear startup timers installed by the stopped query
When the stopped query installs its startup timer after stop() takes the entry snapshot, this identity check preserves that late timer even when this.ctx.queryPromise still belongs to the stopped query. After restart launches a replacement, the old timer can fire and execute this.ctx.queryObject?.close(), which now refers to the replacement adapter, aborting the new turn. Fresh evidence after preserving replacement timers is that timers created later by the same stopped query need the complementary ownership cleanup.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eb4bb02518
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } catch { | ||
| return script; |
There was a problem hiding this comment.
Redact shell scripts when parsing fails
When a valid shell script contains an unmatched quote inside a trailing comment, such as sh -c 'some-agent --token topsecret # "', the shell executes the command because the quote is commented out, but this parser still treats it as unmatched and the catch branch returns the complete script verbatim. The credential is therefore included in the terminal approval title and persisted in resolvedQuestions; parsing failures should use the conservative sensitivity fallback rather than expose the raw script.
Useful? React with 👍 / 👎.
| const next = args[index + 1]; | ||
| if (isSecretArgName(arg) && next !== undefined && !next.startsWith('--')) { |
There was a problem hiding this comment.
Redact inline curl cookie arguments
When curl receives inline cookies through -b session=topsecret or --cookie session=topsecret, neither the generic secret-name check nor the curl-specific header/user checks consume the cookie value, so it remains in the approval title and persisted resolved question. The inspected curl 8.5.0 curl --help all documents -b, --cookie <data|filename> Send cookies from string/file; detect inline cookie data and redact it while retaining behavior-changing filename operands.
Useful? React with 👍 / 👎.
| const SECRET_ARG_NAME_PATTERN = | ||
| /(?:^|[-_])(?:token|secret|password|passphrase|credential|api[-_]?key|bearer)$/i; |
There was a problem hiding this comment.
Redact compound secret environment names
When a terminal command uses a common compound credential name such as env AWS_SECRET_ACCESS_KEY=topsecret agent, the pattern does not match because secret is not the final name component and api-key is the only recognized key suffix. The complete assignment consequently appears in the approval title and persisted resolvedQuestions; environment assignments should recognize secret-bearing components such as SECRET_ACCESS_KEY rather than requiring the credential word at the end.
Useful? React with 👍 / 👎.
| storedIdentity !== undefined && | ||
| storedIdentity !== commandIdentity | ||
| ) { | ||
| session.acpSessionId = undefined; |
There was a problem hiding this comment.
Preserve the prior session until the new command starts
When the configured ACP command changes but the replacement cannot start—for example, its executable name is mistyped—this clears and persists the previous acpSessionId before the client is spawned or initialized. Reverting to the working command then cannot load the prior ACP conversation and creates a fresh session instead; keep the old ID/identity until replacement session creation succeeds, or restore them on startup failure.
Useful? React with 👍 / 👎.
Persist a digest of the ACP launch command in session metadata and start a fresh ACP conversation when it changes, so switching the agent command no longer resumes a session created by a different agent. Only the digest is stored — the raw command line never reaches session metadata. Extracted from #2711 (ACP split 8/10).
* feat(daemon): stamp ACP command-identity digest on sessions Persist a digest of the ACP launch command in session metadata and start a fresh ACP conversation when it changes, so switching the agent command no longer resumes a session created by a different agent. Only the digest is stored — the raw command line never reaches session metadata. Extracted from #2711 (ACP split 8/10). * style: wrap stamping condition to satisfy biome width * fix(daemon): defer ACP identity rotation persist until the new session exists Keep the old persisted ACP session id until the replacement command actually establishes a session, so a command change whose agent fails to start can be reverted and the previous conversation resumed. The stamped identity metadata now rides the first session-id persist instead of an eager update that erased the resume handle up front.
* feat(daemon): wire ACP host callbacks in bypass mode Wire the ACP terminal manager and safe-filesystem host callbacks into query startup when the session has a workspace: terminals run with an allowlisted environment and inherit the process-tree owner, filesystem reads and writes are confined to the workspace with size and range limits, and cwd or env overrides on terminal creation are rejected. Permission requests from the agent are auto-allowed, matching the bypass-permissions trust model the SDK path already uses; the AskUserQuestion approval plumbing for ACP moves to the deferred unified permission UX (#2782). Extracted from #2711 (ACP split 8/10). * test(daemon): skip ACP fs-callback tests when the safe-fs backend cannot load The safe-fs FFI backend fails to load inside some CI shard processes where a native-library sibling test poisons the dynamic loader; the backend itself stays covered by its dedicated suite. Probe the backend before the two wiring tests instead of failing the shard, and apply canonical import formatting.
… ladder (#2790) Route transport and terminal kills through the process-tree owner so the whole child group receives signals, track group liveness with a shared probe helper, and escalate close() from SIGTERM to SIGKILL only while the group is still alive — cancelling the escalation timer when exit is detected. Child spawn errors now surface through a dedicated handler that also settles close waiters, and a process-tree owner failure kills the freshly spawned child before propagating. Extracted from #2711 (ACP split 8/10).
* fix(daemon): harden ACP query abort and interrupt semantics Track the in-flight iterator promise so an abort no longer drops the message it was fetching: after an abort, tool-result and tool-use messages still in flight or queued are drained (bounded by message count and a deadline) and delivered before the iterator settles, so interrupted tool calls end with synthesized results instead of hanging the next turn. Startup now aborts as soon as cleanup begins instead of after the handshake, timed-out sessions are cancelled and closed before the retry, stale queries close a leftover client in finally, and the lifecycle manager aborts the active query before waiting on stop, terminates processes spawned during the stop window, and only clears context references a replacement query has not already replaced. Extracted from #2711 (ACP split 8/10). * test(daemon): drop duplicated abort test blocks
|
Closing as superseded: the ACP runtime slice from this PR landed on dev as a five-PR stack, re-cut for focused review:
Deliberately not extracted, per the security-hardening decision tracked in #2782: approval-gated permission prompts (AskUserQuestion wiring) and the command-secret redaction engine (preserved on #2781 and Branch retained for reference. |
ACP split 8/10 (from #2599). The query runner now registers workspace-scoped safe-fs read/write and terminal host callbacks (each gated by AskUserQuestion approval), stamps a command-identity digest on the session so changing the ACP command starts a fresh conversation, and passes process-tree ownership into the transport/client for group-wide termination. Transport close runs a SIGTERM → group-probe → SIGKILL ladder that cancels escalation once the process group is gone (probing the direct pid on win32, where process groups don't exist), and QueryLifecycleManager aborts the active query before waiting on stop plus clears cached ACP context usage on reset.
Retained race fixes: stale startup aborts, timed-out sessions are cancelled/closed before the retry re-spawns, interrupting a run preserves the in-flight iterator result and flushes pending tool results, and pending approvals cancel promptly when the query ends.
Intentionally dropped (follow-ups):
buildAcpProcessEnv(speculative portability; cleanup is POSIX-only today)acpCommandIdentitycomparison fallback in the query runneracpSessionCommandmetadata) — dropped per review; it would store inline credentials in the DB and session-metadata APIs, and the identity digest already covers change detectionrecordProcessGroupGone()re-probe after arming the close timer