Add headless CLI for one-shot agent runs#3798
Conversation
Adds packages/cli (@posthog/code-cli, bin posthog-code-cli): run one agent turn against a local repo from the shell — prompt in, assistant output on stdout, exit code out. Supports --permission-mode auto (default) and bypassPermissions, --output text|json, --model, --system-prompt, and stdin prompts. Auth comes from ANTHROPIC_* env vars; question tool calls are parked with the same no-user-available contract as cloud background runs. ClaudeAcpAgent now accepts an injected logger (falling back to its console logger), letting hosts route adapter diagnostics off stdout. Generated-By: PostHog Code Task-Id: 97a6265f-b609-4775-9949-887aca895991
|
Merging to
After your PR is submitted to the merge queue, this comment will be automatically updated with its status. If the PR fails, failure details will also be posted here |
|
React Doctor found no issues in the changed files. 🎉 Reviewed by React Doctor for commit |
Review follow-ups: adapter error-level diagnostics now reach stderr without --debug (except expected teardown noise); SIGINT cancels the turn and exits 130 deterministically instead of racing the main path; stdout is flushed before the force-exit timer so piped JSON can't truncate; commander choices()/argParser replace hand-rolled validation (and the double error print); session updates and prompt results use the ACP SDK types instead of unknown casts; reject-only permission requests resolve to reject_once; bypassPermissions as non-sandboxed root fails fast with a clear message. Generated-By: PostHog Code Task-Id: 97a6265f-b609-4775-9949-887aca895991
The force-exit backstop now arms unconditionally so a stalled stdout consumer cannot hang the process, and a SIGINT that lands during session setup exits 130 instead of being reset when the turn activates. Generated-By: PostHog Code Task-Id: 97a6265f-b609-4775-9949-887aca895991
🦔 ReviewHog reviewed this pull requestFound 0 must fix, 4 should fix, 5 consider. Published 9 findings (view the review). |
|
ReviewHog Alpha 🦔 If you find any issues helpful - please reply "valid", "invalid", etc., for evaluation purposes 🙏 |
![posthog[bot]](https://avatars.githubusercontent.com/in/1203049?s=60&v=4){kind=small}
There was a problem hiding this comment.
ReviewHog Report
Business logic
Issues: 4 issues
Files (4)
packages/agent/src/adapters/acp-connection.tspackages/agent/src/adapters/claude/claude-agent.tspackages/cli/src/run.tspackages/cli/src/permission-policy.ts
What were the main changes
- ClaudeAcpAgent now accepts an injected logger (falling back to the console logger) instead of hardcoding one, wired through createAcpConnection so callers like the CLI can route diagnostics off stdout
- New run.ts drives the same in-process ACP connection the desktop app uses: spins up an Agent/ClientSideConnection, streams session updates into the output sink, handles SIGINT/cancel semantics, and maps ACP stop reasons to process exit codes (0/2/3/130)
- New permission-policy.ts implements the unattended permission policy: auto-approves reachable tool permissions (preferring allow_once so rules aren't persisted) and parks AskUserQuestion calls with the same no-user-available contract used by cloud background runs
Feature
Issues: 5 issues
Files (3)
packages/cli/src/cli.tspackages/cli/src/args.tspackages/cli/src/output.ts
What were the main changes
- New cli.ts entry point: reads the prompt from an argument or stdin, guards against bypassPermissions as root outside a sandbox, warns when no ANTHROPIC_* auth env var is set, and force-flushes stdout before exiting
- New args.ts: commander-based CLI parsing for prompt/--cwd/--permission-mode/--model/--system-prompt/--output/--debug, with cwd realpath validation and Claude model id validation
- New output.ts: OutputSink streams assistant text chunks to stdout in text mode, or buffers and emits a single {text, stopReason, usage, sessionId} JSON document in json mode
| this.logger = | ||
| options?.logger ?? | ||
| new Logger({ debug: true, prefix: "[ClaudeAcpAgent]" }); |
There was a problem hiding this comment.
Injecting the caller's logger reroutes ClaudeAcpAgent's internal debug logging into Desktop's persisted log sink and Cloud's SSE/log-writer stream, contradicting the PR's "no behavior change" claim
Why we think it's a valid issue
- Checked: the full logger chain —
logger.ts:38-42(emitLogskips the level/debugEnabledgate wheneveronLogis set) and72-79(child()copiesonLog); the PR diff (git show 988523a0) confirming acp-connection.ts:122 and claude-agent.ts:279-281 are both new; the Desktop caller chain (agent.ts:29-33 builds the logger withonLog:config.onLog, agent.ts:133 forwards it, workspace-server agent.ts:842-850 always setsonLog:this.onAgentLog, makeOnAgentLog:235-244 forwards unconditionally); the Cloud chain (agent-server.ts:1615 passesthis.logger, 1822-1828 reassigns it withonLog:emitConsoleLogafter the createAcpConnection call, cleanupSession:4452-4521 never resetsthis.logger, emitConsoleLog:455-482 broadcasts via SSE +logWriter.appendRawLinewith no level filter). - Found: The mechanism is genuine and the PR's "Desktop and cloud behavior is unchanged since they don't pass one" is factually false — both hosts pass a logger carrying an
onLog, and ClaudeAcpAgent's internal logs (previously console-only, discarded) now route to those sinks. Cloud is confirmed: on any session re-init on the same instance, the child logger inheritsemitConsoleLog, so all-level chatter (including debug) is rebroadcast over SSE and persisted. - Impact: Consequence is diagnostic log verbosity, not a functional defect — no wrong results, data loss, contract break, secret leak, or leaked resource. Two facts cut the severity below the reviewer's
should_fix: (1) the Desktop file transport level isisDev ? "debug" : "info"(apps/code/src/main/utils/logger.ts:63-64), so in production the 8debugsites — including the per-loop "Idle without an active turn" at claude-agent.ts:991 the finding relies on — are filtered; only ~4info+ warn/error persist at per-session frequency into a rotated 10MB×3 log, making "regardless of isDevBuild()" and "every consumer-loop iteration" overstated. (2) The AcpConnection-level child logger already routed info logs to these same sinks pre-PR, so the change is incremental. Cloud SSE/log noise is real but bounded and only on the resume path. - Priority: Kept (real, reproducible, unintended, contradicts an explicit PR claim, untested) but down-ranked should_fix → consider: it is an observability/verbosity regression, not a correctness/security/data/contract/perf-at-scale/reliability defect, and its worst-case Desktop framing is rebutted by production-level log filtering.
Issue description
ClaudeAcpAgent now uses options?.logger ?? new Logger({ debug: true, prefix: "[ClaudeAcpAgent]" }), and createClaudeConnection in acp-connection.ts (line 122) passes config.logger?.child("ClaudeAcpAgent") whenever the caller supplied a logger. The PR description asserts "Desktop and cloud behavior is unchanged since they don't pass one" — but both existing hosts DO pass a logger: Agent.run() (packages/agent/src/agent.ts:133) forwards logger: this.logger, and AgentServer._doInitializeSession (packages/agent/src/server/agent-server.ts:1614) forwards logger: this.logger too. Logger.emitLog (utils/logger.ts) forwards unconditionally to onLog when one is set, bypassing the debugEnabled check entirely, and Logger.child() copies the parent's onLog. Concretely: on Desktop, Agent is always constructed with onLog: this.onAgentLog (packages/workspace-server/src/services/agent/agent.ts:850), which forwards every call straight into RootLogger — so every one of ClaudeAcpAgent's ~30 internal debug/info call sites (fired on essentially every consumer-loop iteration: idle detection, compaction, MCP reconnects, turn activation, etc.) now gets persisted to the on-disk desktop log file on every single task run, regardless of isDevBuild(), where previously they only reached console.* and were discarded. On the cloud sandbox, AgentServer's own this.logger only gains an onLog of emitConsoleLog partway through _doInitializeSession (agent-server.ts:1822-1828, AFTER the createAcpConnection call at line 1609) — but that mutated field is never reset in cleanupSession(), so on any session re-init on the same instance (the code explicitly supports "warm reconnects... sandbox restart with snapshot resume"), the next createAcpConnection call reuses the already-mutated logger, and ClaudeAcpAgent's full internal debug chatter then gets broadcast as _posthog/console SSE notifications to the client AND persisted via session.logWriter.appendRawLine for every session after the first on that instance.
Suggested fix
Verify and, if unintended, avoid coupling ClaudeAcpAgent's own internal logger to the caller-supplied onLog sink. Either keep ClaudeAcpAgent's console-only isolation by default and have the CLI opt in with its own dedicated logger construction (not by threading the host's general-purpose logger through AcpConnectionConfig.logger), or explicitly measure/bound the log volume impact on Desktop (disk usage per run) and Cloud (SSE bandwidth + logWriter storage per session-resume) before merging, since neither is covered by the tests described in the PR ("39 unit tests... Full @posthog/agent suite" — none of which exercise Desktop's onAgentLog wiring or Cloud's session-resume createAcpConnection reuse path).
Prompt to fix with AI (copy-paste)
## Context
@packages/agent/src/adapters/claude/claude-agent.ts#L279-281
<issue_description>
`ClaudeAcpAgent` now uses `options?.logger ?? new Logger({ debug: true, prefix: "[ClaudeAcpAgent]" })`, and `createClaudeConnection` in acp-connection.ts (line 122) passes `config.logger?.child("ClaudeAcpAgent")` whenever the caller supplied a logger. The PR description asserts "Desktop and cloud behavior is unchanged since they don't pass one" — but both existing hosts DO pass a logger: `Agent.run()` (packages/agent/src/agent.ts:133) forwards `logger: this.logger`, and `AgentServer._doInitializeSession` (packages/agent/src/server/agent-server.ts:1614) forwards `logger: this.logger` too. `Logger.emitLog` (utils/logger.ts) forwards unconditionally to `onLog` when one is set, bypassing the `debugEnabled` check entirely, and `Logger.child()` copies the parent's `onLog`. Concretely: on Desktop, `Agent` is always constructed with `onLog: this.onAgentLog` (packages/workspace-server/src/services/agent/agent.ts:850), which forwards every call straight into RootLogger — so every one of ClaudeAcpAgent's ~30 internal debug/info call sites (fired on essentially every consumer-loop iteration: idle detection, compaction, MCP reconnects, turn activation, etc.) now gets persisted to the on-disk desktop log file on every single task run, regardless of `isDevBuild()`, where previously they only reached `console.*` and were discarded. On the cloud sandbox, `AgentServer`'s own `this.logger` only gains an `onLog` of `emitConsoleLog` partway through `_doInitializeSession` (agent-server.ts:1822-1828, AFTER the `createAcpConnection` call at line 1609) — but that mutated field is never reset in `cleanupSession()`, so on any session re-init on the same instance (the code explicitly supports "warm reconnects... sandbox restart with snapshot resume"), the *next* `createAcpConnection` call reuses the already-mutated logger, and ClaudeAcpAgent's full internal debug chatter then gets broadcast as `_posthog/console` SSE notifications to the client AND persisted via `session.logWriter.appendRawLine` for every session after the first on that instance.
</issue_description>
<issue_validation>
- **Checked:** the full logger chain — `logger.ts:38-42` (`emitLog` skips the level/`debugEnabled` gate whenever `onLog` is set) and `72-79` (`child()` copies `onLog`); the PR diff (`git show 988523a0`) confirming acp-connection.ts:122 and claude-agent.ts:279-281 are both new; the Desktop caller chain (agent.ts:29-33 builds the logger with `onLog:config.onLog`, agent.ts:133 forwards it, workspace-server agent.ts:842-850 always sets `onLog:this.onAgentLog`, makeOnAgentLog:235-244 forwards unconditionally); the Cloud chain (agent-server.ts:1615 passes `this.logger`, 1822-1828 reassigns it with `onLog:emitConsoleLog` *after* the createAcpConnection call, cleanupSession:4452-4521 never resets `this.logger`, emitConsoleLog:455-482 broadcasts via SSE + `logWriter.appendRawLine` with no level filter).
- **Found:** The mechanism is genuine and the PR's "Desktop and cloud behavior is unchanged since they don't pass one" is factually false — both hosts pass a logger carrying an `onLog`, and ClaudeAcpAgent's internal logs (previously console-only, discarded) now route to those sinks. Cloud is confirmed: on any session *re-init* on the same instance, the child logger inherits `emitConsoleLog`, so all-level chatter (including debug) is rebroadcast over SSE and persisted.
- **Impact:** Consequence is diagnostic log *verbosity*, not a functional defect — no wrong results, data loss, contract break, secret leak, or leaked resource. Two facts cut the severity below the reviewer's `should_fix`: (1) the Desktop file transport level is `isDev ? "debug" : "info"` (apps/code/src/main/utils/logger.ts:63-64), so in production the 8 `debug` sites — including the per-loop "Idle without an active turn" at claude-agent.ts:991 the finding relies on — are filtered; only ~4 `info` + warn/error persist at per-session frequency into a rotated 10MB×3 log, making "regardless of isDevBuild()" and "every consumer-loop iteration" overstated. (2) The AcpConnection-level child logger already routed info logs to these same sinks pre-PR, so the change is incremental. Cloud SSE/log noise is real but bounded and only on the resume path.
- **Priority:** Kept (real, reproducible, unintended, contradicts an explicit PR claim, untested) but down-ranked should_fix → consider: it is an observability/verbosity regression, not a correctness/security/data/contract/perf-at-scale/reliability defect, and its worst-case Desktop framing is rebutted by production-level log filtering.
</issue_validation>
## Task
Investigate the issue and solve it
<potential_solution>
Verify and, if unintended, avoid coupling ClaudeAcpAgent's own internal logger to the caller-supplied `onLog` sink. Either keep ClaudeAcpAgent's console-only isolation by default and have the CLI opt in with its own dedicated logger construction (not by threading the host's general-purpose logger through `AcpConnectionConfig.logger`), or explicitly measure/bound the log volume impact on Desktop (disk usage per run) and Cloud (SSE bandwidth + logWriter storage per session-resume) before merging, since neither is covered by the tests described in the PR ("39 unit tests... Full @posthog/agent suite" — none of which exercise Desktop's `onAgentLog` wiring or Cloud's session-resume `createAcpConnection` reuse path).
</potential_solution>
| let sessionId: string | undefined; | ||
| let interrupted = false; | ||
| const onSigint = () => { | ||
| interrupted = true; | ||
| if (sessionId) { | ||
| // Cancel resolves an in-flight prompt() with stopReason "cancelled"; | ||
| // the normal path then finishes output and exits 130. A second Ctrl-C | ||
| // (the handler is `once`) falls through to Node's default kill. | ||
| void conn.cancel({ sessionId }).catch(() => undefined); | ||
| } else { | ||
| // Nothing to cancel yet — exit directly. | ||
| void cleanup().then(() => process.exit(130)); | ||
| } | ||
| }; | ||
| process.once("SIGINT", onSigint); |
There was a problem hiding this comment.
No SIGTERM handler: the primary termination signal used by CI/cron/container supervisors skips cleanup entirely, orphaning the underlying claude subprocess
Why we think it's a valid issue
- Checked: run.ts:82-96 and the whole
packages/cli/srctree for signal handlers — onlyprocess.once("SIGINT", onSigint)(run.ts:96) exists; cli.ts registers none. Traced the subprocess lifecycle:buildSpawnWrapper(options.ts:343-347) spawnsclaudeas a direct child with piped stdio and nodetached, and the child is terminated only by the abort listenerspawnOpts.signal.addEventListener("abort", () => child.kill("SIGTERM"))(options.ts:373-377). That abort is fired byterminateQuery→controller.abort()(claude-agent.ts:1880-1881) reached throughagent.cleanup()(agent.ts:202-206), which the CLI calls only inrun()'sfinally(run.ts:132) or the SIGINT path. - Found: An unhandled SIGTERM uses Node's default disposition — immediate termination without unwinding — so the
try/finallyand the SIGINT handler are both skipped;cleanup()/controller.abort()/child.killnever run. Because the child has no parent-death protection, it is reparented to init and keeps running. The project already treats this as a real defect: the cloud agent-server binary installs a SIGTERM handler for cleanup (bin.ts:269) and agent-server.ts:3746 documents that missing it means teardown "never runs"; theterminateQuerycomment (claude-agent.ts:1876-1878) calls out the orphaned-claude-process failure mode explicitly. - Impact: Concrete trigger (SIGTERM from
timeout(1),kill, cron wrappers, or a supervisor signaling the leader PID) and concrete consequence (orphanedclaudesubprocess that keeps making paid LLM calls and mutating the target repo after the CLI is presumed dead) — a reliability/resource defect in the exact unattended CI/cron/pipeline use case the CLI targets, matching the 'missing handling for a failure mode that will occur' keep bar. Container/cgroup-managed runs (Docker/K8s) mitigate via namespace teardown, so it does not reach must_fix, but bare cron/shell/timeoutscenarios genuinely leak — should_fix is right, and the suggestedfor (const sig of ["SIGINT","SIGTERM"])fix is trivial and consistent with the existing cloud-host pattern.
Issue description
Only SIGINT is handled (process.once("SIGINT", onSigint)); there is no SIGTERM listener. The PR's own stated purpose is unattended use in "CI checks, shell pipelines, cron jobs" — exactly the environments where a supervising process (a CI job timeout, timeout(1)'s default signal, Docker/Kubernetes stop) sends SIGTERM, not SIGINT. Node's default disposition for an unhandled SIGTERM is immediate process termination, bypassing the try/finally in run() entirely — so cleanup() never runs, agent.abortController.abort() never fires, and the underlying claude subprocess (spawned via the Claude Agent SDK's query()) is left orphaned. This is the exact failure mode the codebase already calls out elsewhere: "a timed-out session leaks an orphaned claude process that the retry loop then multiplies" (claude-agent.ts, terminateQuery comment) — except here there is no retry loop to eventually clean it up, since each CLI invocation is a fresh, short-lived process.
Suggested fix
Register the same handler for SIGTERM (e.g. for (const sig of ["SIGINT", "SIGTERM"]) process.once(sig, onSigint)), and use the actual signal to pick between exit code 130 (SIGINT) and 143 (SIGTERM) if callers care about that distinction.
Prompt to fix with AI (copy-paste)
## Context
@packages/cli/src/run.ts#L82-96
<issue_description>
Only `SIGINT` is handled (`process.once("SIGINT", onSigint)`); there is no `SIGTERM` listener. The PR's own stated purpose is unattended use in "CI checks, shell pipelines, cron jobs" — exactly the environments where a supervising process (a CI job timeout, `timeout(1)`'s default signal, Docker/Kubernetes stop) sends `SIGTERM`, not `SIGINT`. Node's default disposition for an unhandled `SIGTERM` is immediate process termination, bypassing the `try/finally` in `run()` entirely — so `cleanup()` never runs, `agent.abortController.abort()` never fires, and the underlying `claude` subprocess (spawned via the Claude Agent SDK's `query()`) is left orphaned. This is the exact failure mode the codebase already calls out elsewhere: "a timed-out session leaks an orphaned `claude` process that the retry loop then multiplies" (claude-agent.ts, `terminateQuery` comment) — except here there is no retry loop to eventually clean it up, since each CLI invocation is a fresh, short-lived process.
</issue_description>
<issue_validation>
- **Checked:** run.ts:82-96 and the whole `packages/cli/src` tree for signal handlers — only `process.once("SIGINT", onSigint)` (run.ts:96) exists; cli.ts registers none. Traced the subprocess lifecycle: `buildSpawnWrapper` (options.ts:343-347) spawns `claude` as a direct child with piped stdio and no `detached`, and the child is terminated **only** by the abort listener `spawnOpts.signal.addEventListener("abort", () => child.kill("SIGTERM"))` (options.ts:373-377). That abort is fired by `terminateQuery` → `controller.abort()` (claude-agent.ts:1880-1881) reached through `agent.cleanup()` (agent.ts:202-206), which the CLI calls only in `run()`'s `finally` (run.ts:132) or the SIGINT path.
- **Found:** An unhandled SIGTERM uses Node's default disposition — immediate termination without unwinding — so the `try/finally` and the SIGINT handler are both skipped; `cleanup()`/`controller.abort()`/`child.kill` never run. Because the child has no parent-death protection, it is reparented to init and keeps running. The project already treats this as a real defect: the cloud agent-server binary installs a SIGTERM handler for cleanup (bin.ts:269) and agent-server.ts:3746 documents that missing it means teardown "never runs"; the `terminateQuery` comment (claude-agent.ts:1876-1878) calls out the orphaned-`claude`-process failure mode explicitly.
- **Impact:** Concrete trigger (SIGTERM from `timeout(1)`, `kill`, cron wrappers, or a supervisor signaling the leader PID) and concrete consequence (orphaned `claude` subprocess that keeps making paid LLM calls and mutating the target repo after the CLI is presumed dead) — a reliability/resource defect in the exact unattended CI/cron/pipeline use case the CLI targets, matching the 'missing handling for a failure mode that will occur' keep bar. Container/cgroup-managed runs (Docker/K8s) mitigate via namespace teardown, so it does not reach must_fix, but bare cron/shell/`timeout` scenarios genuinely leak — should_fix is right, and the suggested `for (const sig of ["SIGINT","SIGTERM"])` fix is trivial and consistent with the existing cloud-host pattern.
</issue_validation>
## Task
Investigate the issue and solve it
<potential_solution>
Register the same handler for `SIGTERM` (e.g. `for (const sig of ["SIGINT", "SIGTERM"]) process.once(sig, onSigint)`), and use the actual signal to pick between exit code 130 (SIGINT) and 143 (SIGTERM) if callers care about that distinction.
</potential_solution>
| main().then(exitAfterFlush, (err) => { | ||
| const message = err instanceof Error ? err.message : String(err); | ||
| process.stderr.write(`posthog-code-cli: ${message}\n`); | ||
| exitAfterFlush(1); | ||
| }); |
There was a problem hiding this comment.
Unexpected top-level errors always print message-only, discarding the stack even with --debug
Why we think it's a valid issue
- Checked:
packages/cli/src/cli.tsat the PR head (2cc41d3) — the top-level handlermain().then(exitAfterFlush, (err) => {...})(lines 63-67), the error-propagation path throughrun()inrun.ts, andCliOptions/parseCliArgsinargs.ts. - Found: The rejection handler prints only
err instanceof Error ? err.message : String(err)and nevererr.stack, with no reference to thedebugflag.run.tswrapsconn.initialize()/newSession()/prompt()in atry { ... } finally { cleanup() }with nocatch, so an ACP-handshake or subprocess-spawn failure re-throws out ofrun(), rejectsmain()'s promise, and lands in this message-only handler. Thedebug: booleanfield lives onCliOptions(args.ts:27) and is only in scope insidemain();parseCliArgsreturns aParseErrorobject rather than throwing, so the parsed flag is reliably available beforerun()executes but is not threaded to the outer catch. - Found: The flag is threaded carefully everywhere else —
run()gates all non-error diagnostics behindoptions.debugand even error-level adapter logs go to stderr — so the outermost handler for genuinely-unexpected failures is the one place--debugis ignored. - Impact: For a headless CI/cron tool, an operator running with
--debugspecifically to maximize diagnostics still gets a bare one-line message (e.g.posthog-code-cli: connection closed) with no stack when something unexpected throws, and cannot reproduce interactively. This is a real, if minor, diagnosability gap in a purpose-built debug flag; the trigger (unhandled throw fromrun()) and consequence (lost stack under--debug) are both concrete, and the fix is a small, low-risk change. Appropriately scoped atconsider.
Issue description
main().then(exitAfterFlush, (err) => {...}) prints only err.message unconditionally for any error that isn't one of the explicitly handled cases (ParseError, no-prompt, root-bypass). The parsed debug flag lives inside main()'s local scope and isn't available at this catch site, so even a --debug invocation gets no stack trace when something unexpected fails (e.g. an ACP handshake or subprocess-spawn error inside run()). For a CI/cron tool where there's no attached debugger and no way to reproduce interactively, this makes unexpected failures hard to diagnose from logs alone.
Suggested fix
Thread the parsed debug flag (or otherwise make it available at this outer catch) and print err.stack when set and available, in addition to the one-line message, so unattended failures remain diagnosable from CI output.
Prompt to fix with AI (copy-paste)
## Context
@packages/cli/src/cli.ts#L63-67
<issue_description>
`main().then(exitAfterFlush, (err) => {...})` prints only `err.message` unconditionally for any error that isn't one of the explicitly handled cases (ParseError, no-prompt, root-bypass). The parsed `debug` flag lives inside `main()`'s local scope and isn't available at this catch site, so even a `--debug` invocation gets no stack trace when something unexpected fails (e.g. an ACP handshake or subprocess-spawn error inside `run()`). For a CI/cron tool where there's no attached debugger and no way to reproduce interactively, this makes unexpected failures hard to diagnose from logs alone.
</issue_description>
<issue_validation>
- **Checked:** `packages/cli/src/cli.ts` at the PR head (2cc41d3) — the top-level handler `main().then(exitAfterFlush, (err) => {...})` (lines 63-67), the error-propagation path through `run()` in `run.ts`, and `CliOptions`/`parseCliArgs` in `args.ts`.
- **Found:** The rejection handler prints only `err instanceof Error ? err.message : String(err)` and never `err.stack`, with no reference to the `debug` flag. `run.ts` wraps `conn.initialize()`/`newSession()`/`prompt()` in a `try { ... } finally { cleanup() }` with **no `catch`**, so an ACP-handshake or subprocess-spawn failure re-throws out of `run()`, rejects `main()`'s promise, and lands in this message-only handler. The `debug: boolean` field lives on `CliOptions` (args.ts:27) and is only in scope inside `main()`; `parseCliArgs` returns a `ParseError` object rather than throwing, so the parsed flag is reliably available before `run()` executes but is not threaded to the outer catch.
- **Found:** The flag is threaded carefully everywhere else — `run()` gates all non-error diagnostics behind `options.debug` and even error-level adapter logs go to stderr — so the outermost handler for genuinely-unexpected failures is the one place `--debug` is ignored.
- **Impact:** For a headless CI/cron tool, an operator running with `--debug` specifically to maximize diagnostics still gets a bare one-line message (e.g. `posthog-code-cli: connection closed`) with no stack when something unexpected throws, and cannot reproduce interactively. This is a real, if minor, diagnosability gap in a purpose-built debug flag; the trigger (unhandled throw from `run()`) and consequence (lost stack under `--debug`) are both concrete, and the fix is a small, low-risk change. Appropriately scoped at `consider`.
</issue_validation>
## Task
Investigate the issue and solve it
<potential_solution>
Thread the parsed `debug` flag (or otherwise make it available at this outer catch) and print `err.stack` when set and available, in addition to the one-line message, so unattended failures remain diagnosable from CI output.
</potential_solution>
| /** | ||
| * Unattended permission policy. Question tool calls are parked (no user to | ||
| * answer); everything else that reaches the client is auto-approved. Prefers | ||
| * allow_once so a one-shot run never persists allow-always rules into the | ||
| * target repository's settings; likewise a reject-only request resolves to | ||
| * reject_once rather than whatever option happens to be listed first. | ||
| */ | ||
| export function resolvePermissionRequest( | ||
| params: RequestPermissionRequest, | ||
| ): RequestPermissionResponse { | ||
| const meta = params.toolCall._meta as { codeToolKind?: string } | undefined; | ||
| if (meta?.codeToolKind === "question") { | ||
| return { | ||
| outcome: { outcome: "cancelled" }, | ||
| _meta: { message: UNATTENDED_QUESTION_MESSAGE }, | ||
| }; | ||
| } |
There was a problem hiding this comment.
--permission-mode bypassPermissions never reaches the AskUserQuestion park logic, so the CLI's "no user available" contract silently doesn't apply to one of the only two supported modes
Why we think it's a valid issue
- Checked: args.ts:10-15,84 (only
auto/bypassPermissions, defaultauto); tools.ts:65-79 (isToolAllowedForMode→truefor all tools whenmode === "bypassPermissions"); permission-handlers.ts:734-832 (thecanUseToolordering — theisToolAllowedForModeallow at 815-819 precedes theAskUserQuestiondispatch at 830-831); handleAskUserQuestionTool:344-429 (the only caller ofrequestPermissionfor questions, injectinganswersor parking); permission-policy.ts:26-30 (the park branch); run.ts:104 (mode passthrough). - Found: The control-flow claim is correct — under
bypassPermissions,AskUserQuestionis allowed at permission-handlers.ts:815-819 with its original input (noanswers), so the park logic never runs and nopermission ... ->debug line is emitted. Verified accurate. - Impact: Real but far milder than must_fix. (1) The allow-everything path is intentional, pre-existing adapter design, not introduced here: tools.ts:69-71 and the comment at tools.ts:46-52 ("auto stays narrower than bypassPermissions") deliberately gate
AskUserQuestiononly inauto. A user selectingbypassPermissionsexplicitly opts into "bypass all permission checks," and the question gate is a permission mechanism — auto-allowing it is consistent with the mode's stated contract. (2) The CLI default isauto, where parking works correctly, so the common invocation is unaffected. (3) The harm is an edge case (model choosing to ask a clarifying question) within a non-default opt-in mode; the tool is allowed and the model continues rather than hanging or crashing — 'silent no-op' overstates it. The genuinely actionable, non-debatable core is documentation accuracy: the docstring and PR description claim parking applies unconditionally, which is false forbypassPermissions. - Priority: Lowered must_fix → consider. must_fix implies a serious functional/correctness or security defect; this is a subtle, debatable, edge-case behavioral discrepancy in an explicit 'bypass everything' mode, with the default path correct and no crash/data/security consequence. Worth the author's awareness (a docstring caveat, or special-casing
AskUserQuestion/plan-mode ahead of the short-circuit if they want the safety contract to hold regardless of mode), but not urgent.
Issue description
permission-policy.ts's docstring and the PR description both state, unconditionally, that AskUserQuestion tool calls are "parked" with the no-user-available message so the model restates the question as text and ends its turn. That's only true when the request actually reaches resolvePermissionRequest via client.requestPermission. Tracing how run.ts's client.requestPermission gets invoked: the agent-side canUseTool (packages/agent/src/adapters/claude/permissions/permission-handlers.ts, ~line 815) starts with if (isToolAllowedForMode(toolName, session.permissionMode)) { return { behavior: "allow", ... } }, evaluated before the toolName === "AskUserQuestion" branch. isToolAllowedForMode (packages/agent/src/adapters/claude/tools.ts) begins with if (mode === "bypassPermissions") { return true; } — unconditional on toolName. Since bypassPermissions is one of only two values packages/cli/src/args.ts's CLI_PERMISSION_MODES accepts (the other being auto), every AskUserQuestion call made by the model while the CLI runs with --permission-mode bypassPermissions is auto-"allow"ed with the model's original, unanswered tool input — handleAskUserQuestionTool is never invoked, client.requestPermission is never called, and therefore resolvePermissionRequest's question-park branch (lines 26-30 here) never runs. No answers field is ever injected into the tool input, and no trace of this occurs anywhere in the CLI's output — not even the permission "..." -> ... debug line in run.ts, since that line only executes inside the requestPermission handler this path never reaches. Contrast with --permission-mode auto, where AskUserQuestion is correctly not in AUTO_ALLOWED_TOOLS.auto and does fall through to the special-case branch and this module's park logic — so the break is specific to, and only surfaces in, bypassPermissions, exactly the second mode this PR documents as a supported unattended option. In the target use case (CI checks, cron jobs, shell pipelines with nobody watching), a model that asks a clarifying question under --permission-mode bypassPermissions gets a silent no-op instead of the intended "restate as text and stop" safety behavior, with no diagnostic signal available to the operator to even notice it happened.
Suggested fix
Special-case AskUserQuestion (and ideally document the same consideration for EnterPlanMode/ExitPlanMode, though bypassing those may be intentional under "bypass all permissions") ahead of the isToolAllowedForMode short-circuit in canUseTool, so it always routes through client.requestPermission regardless of session.permissionMode — mirroring how cloud background runs already guarantee the no-user-available contract independent of mode. At minimum, update this module's docstring and the CLI's --help/README to state plainly that AskUserQuestion parking does not apply under --permission-mode bypassPermissions, and consider having resolvePermissionRequest or run.ts log a warning when that mode is selected so the gap isn't entirely silent.
Prompt to fix with AI (copy-paste)
## Context
@packages/cli/src/permission-policy.ts#L15-31
<issue_description>
permission-policy.ts's docstring and the PR description both state, unconditionally, that AskUserQuestion tool calls are "parked" with the no-user-available message so the model restates the question as text and ends its turn. That's only true when the request actually reaches `resolvePermissionRequest` via `client.requestPermission`. Tracing how `run.ts`'s `client.requestPermission` gets invoked: the agent-side `canUseTool` (packages/agent/src/adapters/claude/permissions/permission-handlers.ts, ~line 815) starts with `if (isToolAllowedForMode(toolName, session.permissionMode)) { return { behavior: "allow", ... } }`, evaluated *before* the `toolName === "AskUserQuestion"` branch. `isToolAllowedForMode` (packages/agent/src/adapters/claude/tools.ts) begins with `if (mode === "bypassPermissions") { return true; }` — unconditional on `toolName`. Since `bypassPermissions` is one of only two values `packages/cli/src/args.ts`'s `CLI_PERMISSION_MODES` accepts (the other being `auto`), every `AskUserQuestion` call made by the model while the CLI runs with `--permission-mode bypassPermissions` is auto-"allow"ed with the model's original, unanswered tool input — `handleAskUserQuestionTool` is never invoked, `client.requestPermission` is never called, and therefore `resolvePermissionRequest`'s question-park branch (lines 26-30 here) never runs. No `answers` field is ever injected into the tool input, and no trace of this occurs anywhere in the CLI's output — not even the `permission "..." -> ...` debug line in run.ts, since that line only executes inside the `requestPermission` handler this path never reaches. Contrast with `--permission-mode auto`, where `AskUserQuestion` is correctly not in `AUTO_ALLOWED_TOOLS.auto` and does fall through to the special-case branch and this module's park logic — so the break is specific to, and only surfaces in, `bypassPermissions`, exactly the second mode this PR documents as a supported unattended option. In the target use case (CI checks, cron jobs, shell pipelines with nobody watching), a model that asks a clarifying question under `--permission-mode bypassPermissions` gets a silent no-op instead of the intended "restate as text and stop" safety behavior, with no diagnostic signal available to the operator to even notice it happened.
</issue_description>
<issue_validation>
- **Checked:** args.ts:10-15,84 (only `auto`/`bypassPermissions`, default `auto`); tools.ts:65-79 (`isToolAllowedForMode` → `true` for all tools when `mode === "bypassPermissions"`); permission-handlers.ts:734-832 (the `canUseTool` ordering — the `isToolAllowedForMode` allow at 815-819 precedes the `AskUserQuestion` dispatch at 830-831); handleAskUserQuestionTool:344-429 (the only caller of `requestPermission` for questions, injecting `answers` or parking); permission-policy.ts:26-30 (the park branch); run.ts:104 (mode passthrough).
- **Found:** The control-flow claim is correct — under `bypassPermissions`, `AskUserQuestion` is allowed at permission-handlers.ts:815-819 with its original input (no `answers`), so the park logic never runs and no `permission ... ->` debug line is emitted. Verified accurate.
- **Impact:** Real but far milder than must_fix. (1) The allow-everything path is intentional, pre-existing adapter design, not introduced here: tools.ts:69-71 and the comment at tools.ts:46-52 ("auto stays narrower than bypassPermissions") deliberately gate `AskUserQuestion` only in `auto`. A user selecting `bypassPermissions` explicitly opts into "bypass all permission checks," and the question gate is a permission mechanism — auto-allowing it is consistent with the mode's stated contract. (2) The CLI default is `auto`, where parking works correctly, so the common invocation is unaffected. (3) The harm is an edge case (model choosing to ask a clarifying question) within a non-default opt-in mode; the tool is allowed and the model continues rather than hanging or crashing — 'silent no-op' overstates it. The genuinely actionable, non-debatable core is documentation accuracy: the docstring and PR description claim parking applies unconditionally, which is false for `bypassPermissions`.
- **Priority:** Lowered must_fix → consider. must_fix implies a serious functional/correctness or security defect; this is a subtle, debatable, edge-case behavioral discrepancy in an explicit 'bypass everything' mode, with the default path correct and no crash/data/security consequence. Worth the author's awareness (a docstring caveat, or special-casing `AskUserQuestion`/plan-mode ahead of the short-circuit if they want the safety contract to hold regardless of mode), but not urgent.
</issue_validation>
## Task
Investigate the issue and solve it
<potential_solution>
Special-case `AskUserQuestion` (and ideally document the same consideration for `EnterPlanMode`/`ExitPlanMode`, though bypassing those may be intentional under "bypass all permissions") ahead of the `isToolAllowedForMode` short-circuit in `canUseTool`, so it always routes through `client.requestPermission` regardless of `session.permissionMode` — mirroring how cloud background runs already guarantee the no-user-available contract independent of mode. At minimum, update this module's docstring and the CLI's `--help`/README to state plainly that `AskUserQuestion` parking does not apply under `--permission-mode bypassPermissions`, and consider having `resolvePermissionRequest` or `run.ts` log a warning when that mode is selected so the gap isn't entirely silent.
</potential_solution>
| function parseModel(value: string): string { | ||
| // Non-claude ids are silently coerced to the default model downstream, so | ||
| // reject them here where the user can see why. | ||
| if (!value.startsWith("claude-")) { | ||
| throw new InvalidArgumentError( | ||
| 'Pass a full Claude model id starting with "claude-" (e.g. "claude-sonnet-4-5").', | ||
| ); | ||
| } | ||
| return value; | ||
| } |
There was a problem hiding this comment.
--model validation only checks the prefix; unrecognized ids are silently swapped for the default downstream
Why we think it's a valid issue
- Checked:
parseModelinpackages/cli/src/args.ts:38-47, the_meta.modelflow inrun.ts(...(options.model ? { model: options.model } : {})), and the downstream resolution inpackages/agent/src/adapters/claude/session/model-config.ts:46-58plus its only real callerclaude-agent.ts:2255. - Found:
parseModelonly enforces theclaude-prefix (args.ts:41) — its own comment (args.ts:39-40) names the exact hazard it's meant to prevent ("silently coerced to the default model downstream") yet leaves the more common case unguarded.resolveInitialModelId(model-config.ts:51-57) returns a candidate only ifallowedModelIds.has(trimmed); otherwise it falls through tomodelOptions.currentModelIdwith no throw, warning, or log. The CLI's--modelreaches this asmeta?.model(claude-agent.ts:2256-2257), so a well-formed-but-wrong id (typo, retired id) passes parse validation and is silently swapped. - Found: No feedback path exists in the CLI.
run.ts'sdebugLoglines cover session-start/cwd, permission decisions, and turn result — never the model; andclaude-agent.ts:2233"Session initialized"info log omits the resolved model id and never flags a substitution. So even--debugruns give the operator zero signal that the requested model was ignored. - Impact: In a headless CI/cron context with no model badge to eyeball, an operator's explicit
--modelselection can be silently replaced by the default — a real cost/quality behavior change with no error. Concrete trigger (--model claude-opus-4-8mistyped asclaude-opus-4.8) and concrete consequence (default model runs, no feedback) are both nameable; the lightweight remedy (warn inrun.tswhen resolved ≠ requested) is clearly feasible. Meets the correctness/contract bar.should_fixis appropriate — a silent input-honoring failure, not merely cosmetic.
Issue description
parseModel only rejects ids that don't start with "claude-"; its own comment says the goal is to catch what would otherwise be "silently coerced to the default model downstream." But a well-formed-looking, misspelled, or retired model id (e.g. --model claude-sonnet-4-5-typo) still passes this check. Downstream, packages/agent/src/adapters/claude/session/model-config.ts's resolveInitialModelId() only honors a candidate model id if it is present in the allowedModelIds set built from the gateway's model list; anything else falls through silently to modelOptions.currentModelId (the default) with no error, warning, or log line. In a headless CLI meant for CI/cron use, with no UI to notice the active model badge, the operator gets zero feedback that their explicitly requested model was ignored and a different model ran instead.
Suggested fix
Validate --model against the actual gateway/allowed-model list at parse time and fail loudly on an unrecognized id (matching the same friendly-error contract as --cwd/--permission-mode), or have run.ts surface a warning when the resolved model id differs from the requested one so silent substitution is never unnoticed in headless runs.
Prompt to fix with AI (copy-paste)
## Context
@packages/cli/src/args.ts#L38-47
<issue_description>
parseModel only rejects ids that don't start with "claude-"; its own comment says the goal is to catch what would otherwise be "silently coerced to the default model downstream." But a well-formed-looking, misspelled, or retired model id (e.g. --model claude-sonnet-4-5-typo) still passes this check. Downstream, packages/agent/src/adapters/claude/session/model-config.ts's resolveInitialModelId() only honors a candidate model id if it is present in the allowedModelIds set built from the gateway's model list; anything else falls through silently to modelOptions.currentModelId (the default) with no error, warning, or log line. In a headless CLI meant for CI/cron use, with no UI to notice the active model badge, the operator gets zero feedback that their explicitly requested model was ignored and a different model ran instead.
</issue_description>
<issue_validation>
- **Checked:** `parseModel` in `packages/cli/src/args.ts:38-47`, the `_meta.model` flow in `run.ts` (`...(options.model ? { model: options.model } : {})`), and the downstream resolution in `packages/agent/src/adapters/claude/session/model-config.ts:46-58` plus its only real caller `claude-agent.ts:2255`.
- **Found:** `parseModel` only enforces the `claude-` prefix (args.ts:41) — its own comment (args.ts:39-40) names the exact hazard it's meant to prevent ("silently coerced to the default model downstream") yet leaves the more common case unguarded. `resolveInitialModelId` (model-config.ts:51-57) returns a candidate only if `allowedModelIds.has(trimmed)`; otherwise it falls through to `modelOptions.currentModelId` with no throw, warning, or log. The CLI's `--model` reaches this as `meta?.model` (claude-agent.ts:2256-2257), so a well-formed-but-wrong id (typo, retired id) passes parse validation and is silently swapped.
- **Found:** No feedback path exists in the CLI. `run.ts`'s `debugLog` lines cover session-start/cwd, permission decisions, and turn result — never the model; and `claude-agent.ts:2233` `"Session initialized"` info log omits the resolved model id and never flags a substitution. So even `--debug` runs give the operator zero signal that the requested model was ignored.
- **Impact:** In a headless CI/cron context with no model badge to eyeball, an operator's explicit `--model` selection can be silently replaced by the default — a real cost/quality behavior change with no error. Concrete trigger (`--model claude-opus-4-8` mistyped as `claude-opus-4.8`) and concrete consequence (default model runs, no feedback) are both nameable; the lightweight remedy (warn in `run.ts` when resolved ≠ requested) is clearly feasible. Meets the correctness/contract bar. `should_fix` is appropriate — a silent input-honoring failure, not merely cosmetic.
</issue_validation>
## Task
Investigate the issue and solve it
<potential_solution>
Validate --model against the actual gateway/allowed-model list at parse time and fail loudly on an unrecognized id (matching the same friendly-error contract as --cwd/--permission-mode), or have run.ts surface a warning when the resolved model id differs from the requested one so silent substitution is never unnoticed in headless runs.
</potential_solution>
| const { options } = params; | ||
| const chosen = | ||
| options.find((o) => o.kind === "allow_once") ?? | ||
| options.find((o) => o.kind === "allow_always") ?? | ||
| options.find((o) => o.kind === "reject_once") ?? | ||
| options[0]; | ||
| if (!chosen) { | ||
| return { outcome: { outcome: "cancelled" } }; | ||
| } | ||
| return { outcome: { outcome: "selected", optionId: chosen.optionId } }; |
There was a problem hiding this comment.
Unattended permission policy silently downgrades an auto/bypass session to interactive "default" mode on plan approval (ExitPlanMode)
Why we think it's a valid issue
- Checked: the full ExitPlanMode path — tools.ts:30-57 (ExitPlanMode in no auto-allowed set), permission-handlers.ts:815-827 (canUseTool routes it to handleExitPlanModeTool since isToolAllowedForMode is false in auto), 234-256 (requestPlanApproval builds options via buildExitPlanModePermissionOptions), 259-284 (applyPlanApproval applies the selected mode + emits setMode/localSettings), permission-options.ts:107-162 (the only allow_once entry is optionId "default"), and permission-policy.ts:34-38 (kind-based allow_once-first selection). Also traced what happens after the switch: handleDefaultPermissionFlow → requestPermission → resolvePermissionRequest still returns allow_once.
- Found: The bug is real and reachable. Because resolvePermissionRequest selects by kind, not array position, it picks the plan-approval option "default" (the sole allow_once) over the continuation option "auto" (allow_always at index 0), so a plan approval in auto mode silently calls applySessionMode("default") + updateConfigOption("mode","default") and returns a setMode:localSettings update (permission-handlers.ts:271-283). The cloud reference (createCloudClient.requestPermission) avoids this by selecting the first allow_once/allow_always in array order, which lands on the front-loaded continuation option.
- Impact: Confirmed but milder than must_fix. Functional behavior is preserved — after the downgrade, Write/Edit/Bash calls still get auto-approved by resolvePermissionRequest (allow_once "Yes"), so the run completes; there is no block, hang, data loss, or privilege escalation ("default" is more restrictive than "auto"). The concrete consequences are extra in-process round trips (negligible) and a possibly-persisted benign "default" mode into localSettings (conventionally gitignored; a subsequent CLI run passes its own --permission-mode). It also only triggers when the model actually enters and exits plan mode in an unattended run, not on every run.
- Priority: Lowered must_fix → should_fix. It is a genuine correctness bug in the PR's core function with a clean, pattern-matching fix (mirror the cloud array-order selection / special-case the switch_mode request) and a real if-minor contract violation (silent mode change + repo-settings side effect for an unattended tool), but the absence of any functional breakage, data, or security consequence makes must_fix an overstatement.
Issue description
resolvePermissionRequest() always prefers the option with kind "allow_once" over "allow_always", regardless of what each option actually represents. That heuristic is correct for ordinary tool-permission requests built by buildPermissionOptions() (Bash/Write/etc.), where "allow_once" ('Yes') is genuinely the least-commitment choice. It breaks for the ExitPlanMode / plan-approval request built by buildExitPlanModePermissionOptions() (packages/agent/src/adapters/claude/permissions/permission-options.ts), which is reachable from the CLI's default --permission-mode auto (ExitPlanMode is not in AUTO_ALLOWED_TOOLS.auto, so canUseTool routes it through handleExitPlanModeTool -> requestPlanApproval -> client.requestPermission). In that option set, the entry that continues the session's current mode (e.g. optionId "auto", label 'Yes, continue in "auto" mode') is always kind "allow_always", while the only "allow_once" entry is optionId "default" ('Yes, and manually approve edits'). Because resolvePermissionRequest searches by kind rather than by array position, it will always pick "default" whenever the model plans and then calls ExitPlanMode in auto mode — silently switching the session from the unattended "auto" mode into the fully-interactive "default" mode (permission-handlers.ts applyPlanApproval then calls applySessionMode("default") and returns updatedPermissions with a setMode/localSettings update when the SDK supplies no other suggestions, which can persist "default" mode into the target repo's local settings for future runs, including the desktop app). This directly contradicts the CLI's stated contract that only "auto"/"bypassPermissions" are supported because 'there's no UI to answer prompts' — after this silent downgrade, every subsequent Write/Edit/Bash call generates a fresh permission request that previously would have been auto-allowed without a round trip. The reference implementation this PR claims to mirror (packages/agent/src/server/agent-server.ts createCloudClient.requestPermission, ~line 4003) avoids this exact problem: for background (unattended) runs it picks the first option matching either allow_once or allow_always in array order, and because buildExitPlanModePermissionOptions() always reorders the continuation option for previousMode to the front of the array, that array-order approach naturally selects 'continue in current mode' instead of 'default'. permission-policy.ts's kind-based preference does not have this property.
Suggested fix
Special-case switch_mode (plan-approval) permission requests before applying the generic allow_once-first heuristic, mirroring agent-server.ts's approach: e.g. if (params.toolCall.kind === "switch_mode") { const chosen = options.find(o => o.kind === "allow_once" || o.kind === "allow_always") ?? options[0]; ... } — picking the first eligible allow option in array order rather than searching by kind. Since buildExitPlanModePermissionOptions() always moves the entry matching session.modeBeforePlan to index 0, this reliably continues in the CLI's original --permission-mode instead of downgrading to "default". Add a unit test with the actual ExitPlanMode option shape (continuation option as allow_always at index 0, "default" as allow_once elsewhere) to lock in the expected optionId, since the existing tests in permission-policy.test.ts only exercise abstract option arrays and never catch this mismatch.
Prompt to fix with AI (copy-paste)
## Context
@packages/cli/src/permission-policy.ts#L33-42
<issue_description>
resolvePermissionRequest() always prefers the option with kind "allow_once" over "allow_always", regardless of what each option actually represents. That heuristic is correct for ordinary tool-permission requests built by buildPermissionOptions() (Bash/Write/etc.), where "allow_once" ('Yes') is genuinely the least-commitment choice. It breaks for the ExitPlanMode / plan-approval request built by buildExitPlanModePermissionOptions() (packages/agent/src/adapters/claude/permissions/permission-options.ts), which is reachable from the CLI's default `--permission-mode auto` (ExitPlanMode is not in AUTO_ALLOWED_TOOLS.auto, so canUseTool routes it through handleExitPlanModeTool -> requestPlanApproval -> client.requestPermission). In that option set, the entry that continues the session's current mode (e.g. optionId "auto", label 'Yes, continue in "auto" mode') is always kind "allow_always", while the only "allow_once" entry is optionId "default" ('Yes, and manually approve edits'). Because resolvePermissionRequest searches by kind rather than by array position, it will always pick "default" whenever the model plans and then calls ExitPlanMode in auto mode — silently switching the session from the unattended "auto" mode into the fully-interactive "default" mode (permission-handlers.ts applyPlanApproval then calls applySessionMode("default") and returns updatedPermissions with a setMode/localSettings update when the SDK supplies no other suggestions, which can persist "default" mode into the target repo's local settings for future runs, including the desktop app). This directly contradicts the CLI's stated contract that only "auto"/"bypassPermissions" are supported because 'there's no UI to answer prompts' — after this silent downgrade, every subsequent Write/Edit/Bash call generates a fresh permission request that previously would have been auto-allowed without a round trip. The reference implementation this PR claims to mirror (packages/agent/src/server/agent-server.ts createCloudClient.requestPermission, ~line 4003) avoids this exact problem: for background (unattended) runs it picks the *first* option matching either allow_once or allow_always in array order, and because buildExitPlanModePermissionOptions() always reorders the continuation option for `previousMode` to the front of the array, that array-order approach naturally selects 'continue in current mode' instead of 'default'. permission-policy.ts's kind-based preference does not have this property.
</issue_description>
<issue_validation>
- **Checked:** the full ExitPlanMode path — tools.ts:30-57 (ExitPlanMode in no auto-allowed set), permission-handlers.ts:815-827 (canUseTool routes it to handleExitPlanModeTool since isToolAllowedForMode is false in auto), 234-256 (requestPlanApproval builds options via buildExitPlanModePermissionOptions), 259-284 (applyPlanApproval applies the selected mode + emits setMode/localSettings), permission-options.ts:107-162 (the only allow_once entry is optionId "default"), and permission-policy.ts:34-38 (kind-based allow_once-first selection). Also traced what happens after the switch: handleDefaultPermissionFlow → requestPermission → resolvePermissionRequest still returns allow_once.
- **Found:** The bug is real and reachable. Because resolvePermissionRequest selects by kind, not array position, it picks the plan-approval option "default" (the sole allow_once) over the continuation option "auto" (allow_always at index 0), so a plan approval in auto mode silently calls applySessionMode("default") + updateConfigOption("mode","default") and returns a setMode:localSettings update (permission-handlers.ts:271-283). The cloud reference (createCloudClient.requestPermission) avoids this by selecting the first allow_once/allow_always in array order, which lands on the front-loaded continuation option.
- **Impact:** Confirmed but milder than must_fix. Functional behavior is preserved — after the downgrade, Write/Edit/Bash calls still get auto-approved by resolvePermissionRequest (allow_once "Yes"), so the run completes; there is no block, hang, data loss, or privilege escalation ("default" is more restrictive than "auto"). The concrete consequences are extra in-process round trips (negligible) and a possibly-persisted benign "default" mode into localSettings (conventionally gitignored; a subsequent CLI run passes its own --permission-mode). It also only triggers when the model actually enters and exits plan mode in an unattended run, not on every run.
- **Priority:** Lowered must_fix → should_fix. It is a genuine correctness bug in the PR's core function with a clean, pattern-matching fix (mirror the cloud array-order selection / special-case the switch_mode request) and a real if-minor contract violation (silent mode change + repo-settings side effect for an unattended tool), but the absence of any functional breakage, data, or security consequence makes must_fix an overstatement.
</issue_validation>
## Task
Investigate the issue and solve it
<potential_solution>
Special-case switch_mode (plan-approval) permission requests before applying the generic allow_once-first heuristic, mirroring agent-server.ts's approach: e.g. `if (params.toolCall.kind === "switch_mode") { const chosen = options.find(o => o.kind === "allow_once" || o.kind === "allow_always") ?? options[0]; ... }` — picking the first eligible allow option in array order rather than searching by kind. Since buildExitPlanModePermissionOptions() always moves the entry matching session.modeBeforePlan to index 0, this reliably continues in the CLI's original --permission-mode instead of downgrading to "default". Add a unit test with the actual ExitPlanMode option shape (continuation option as allow_always at index 0, "default" as allow_once elsewhere) to lock in the expected optionId, since the existing tests in permission-policy.test.ts only exercise abstract option arrays and never catch this mismatch.
</potential_solution>
| // The Claude subprocess refuses bypass for root outside a sandbox; fail | ||
| // here with a clear message instead of a cryptic session error. | ||
| if ( | ||
| parsed.permissionMode === "bypassPermissions" && | ||
| process.getuid?.() === 0 && | ||
| !process.env.IS_SANDBOX | ||
| ) { | ||
| process.stderr.write( | ||
| "--permission-mode bypassPermissions is unavailable when running as root unless IS_SANDBOX=1 is set.\n", | ||
| ); | ||
| return 1; | ||
| } |
There was a problem hiding this comment.
Root-bypass guard checks real UID, not effective UID — diverges from the canonical ALLOW_BYPASS/IS_ROOT check it's meant to preempt
Why we think it's a valid issue
- Checked: the CLI guard
process.getuid?.() === 0(cli.ts:27-33) against the canonicalIS_ROOTinpackages/agent/src/utils/common.ts:59-61((process.geteuid?.() ?? process.getuid?.()) === 0) andALLOW_BYPASS(common.ts:63). Also traced mode gating inexecution-mode.ts, where bothautoandbypassPermissionsare only pushed insideif (ALLOW_BYPASS). - Found: the divergence is real — the CLI checks real UID only, while the effective gate (
ALLOW_BYPASS, and the Claude subprocess's own refusal the comment cites) keys off effective UID. In an euid=0 / ruid≠0 environment (setuid-root wrapper, some su-exec/init setups) the CLI'sgetuid()===0is false, so the friendly early exit is skipped, yet the downstream layer still refuses — producing exactly the "cryptic session error" this block exists to preempt. - Impact: the consequence is confined to error-message quality: the security/functional outcome is unchanged (bypass is still correctly refused downstream, as the finding itself concedes), so there is no wrong-permission grant. It only manifests in a rare euid≠ruid configuration for a headless CLI. The real value is consistency — reusing the existing canonical
IS_ROOT/euid-preferring check instead of reimplementing a narrower privilege check, an area where subtle divergences are worth eliminating; the fix is one line. - Priority: lowered should_fix → consider. There is no correctness or security defect today (identical refusal outcome, only a less-friendly message) and the trigger is a rare privilege configuration, so should_fix overstates it; consider records the legitimate consistency nit at its true, low severity.
Issue description
This block exists specifically to short-circuit before the Claude subprocess's own root-outside-sandbox refusal, per the comment 'The Claude subprocess refuses bypass for root outside a sandbox; fail here with a clear message instead of a cryptic session error.' But the canonical check it is mirroring — IS_ROOT in packages/agent/src/utils/common.ts ((process.geteuid?.() ?? process.getuid?.()) === 0), which gates ALLOW_BYPASS — prefers the effective UID, falling back to real UID only if geteuid is unavailable. This CLI guard checks only process.getuid?.() (the real UID), never geteuid. In any environment where effective UID is 0 but real UID is not (e.g. a setuid-root wrapper, some container init/su-exec setups), this guard will not trigger, so bypassPermissions proceeds past this early check. The agent's own ALLOW_BYPASS (based on effective UID) will still correctly refuse downstream, but the user then gets exactly the 'cryptic session error' this code was written to avoid — defeating the stated purpose of the check in that specific edge case.
Suggested fix
Match the canonical check instead of reimplementing a narrower one, e.g. const isRoot = (process.geteuid?.() ?? process.getuid?.()) === 0; (or import/reuse IS_ROOT from @posthog/agent if it's exported) and use that in the condition instead of process.getuid?.() === 0.
Prompt to fix with AI (copy-paste)
## Context
@packages/cli/src/cli.ts#L26-37
<issue_description>
This block exists specifically to short-circuit before the Claude subprocess's own root-outside-sandbox refusal, per the comment 'The Claude subprocess refuses bypass for root outside a sandbox; fail here with a clear message instead of a cryptic session error.' But the canonical check it is mirroring — `IS_ROOT` in packages/agent/src/utils/common.ts (`(process.geteuid?.() ?? process.getuid?.()) === 0`), which gates `ALLOW_BYPASS` — prefers the *effective* UID, falling back to real UID only if `geteuid` is unavailable. This CLI guard checks only `process.getuid?.()` (the real UID), never `geteuid`. In any environment where effective UID is 0 but real UID is not (e.g. a setuid-root wrapper, some container init/su-exec setups), this guard will not trigger, so `bypassPermissions` proceeds past this early check. The agent's own `ALLOW_BYPASS` (based on effective UID) will still correctly refuse downstream, but the user then gets exactly the 'cryptic session error' this code was written to avoid — defeating the stated purpose of the check in that specific edge case.
</issue_description>
<issue_validation>
- **Checked:** the CLI guard `process.getuid?.() === 0` (cli.ts:27-33) against the canonical `IS_ROOT` in `packages/agent/src/utils/common.ts:59-61` (`(process.geteuid?.() ?? process.getuid?.()) === 0`) and `ALLOW_BYPASS` (common.ts:63). Also traced mode gating in `execution-mode.ts`, where both `auto` and `bypassPermissions` are only pushed inside `if (ALLOW_BYPASS)`.
- **Found:** the divergence is real — the CLI checks real UID only, while the effective gate (`ALLOW_BYPASS`, and the Claude subprocess's own refusal the comment cites) keys off effective UID. In an euid=0 / ruid≠0 environment (setuid-root wrapper, some su-exec/init setups) the CLI's `getuid()===0` is false, so the friendly early exit is skipped, yet the downstream layer still refuses — producing exactly the "cryptic session error" this block exists to preempt.
- **Impact:** the consequence is confined to error-message quality: the security/functional outcome is unchanged (bypass is still correctly refused downstream, as the finding itself concedes), so there is no wrong-permission grant. It only manifests in a rare euid≠ruid configuration for a headless CLI. The real value is consistency — reusing the existing canonical `IS_ROOT`/euid-preferring check instead of reimplementing a narrower privilege check, an area where subtle divergences are worth eliminating; the fix is one line.
- **Priority:** lowered should_fix → consider. There is no correctness or security defect today (identical refusal outcome, only a less-friendly message) and the trigger is a rare privilege configuration, so should_fix overstates it; consider records the legitimate consistency nit at its true, low severity.
</issue_validation>
## Task
Investigate the issue and solve it
<potential_solution>
Match the canonical check instead of reimplementing a narrower one, e.g. `const isRoot = (process.geteuid?.() ?? process.getuid?.()) === 0;` (or import/reuse `IS_ROOT` from `@posthog/agent` if it's exported) and use that in the condition instead of `process.getuid?.() === 0`.
</potential_solution>
| if (mode === "text") { | ||
| stdout.write(text); | ||
| streamedText = true; |
There was a problem hiding this comment.
No handling for EPIPE when stdout is piped into a reader that exits early
Why we think it's a valid issue
- Checked: the streaming write
stdout.write(text)(output.ts:42-44, fedprocess.stdoutfromrun.ts'screateOutputSink(options.output, process.stdout)), the flush write incli.ts'sexitAfterFlush, and every file inpackages/cli/srcfor aprocess.stdout.on("error")/uncaughtException/ EPIPE guard. - Found: no stdout error listener is registered anywhere in the package. I reproduced the failure on this environment's Node (v24.18.0): a program streaming lines to stdout piped into
head -1emits an unhandled'error'event once the reader closes → uncaught exception with a rawError: write EPIPEstack trace on stderr and exit code 1 (captured 25 stderr lines,PIPESTATUS[0]=1). The CLI writes one chunk per ACPsessionUpdate, so any multi-chunk response (the norm for a coding agent) hits a post-close write and triggers exactly this. - Impact: the PR explicitly targets "shell pipelines," and
| head,| grep -q,| less(quit early) are everyday idioms that WILL occur — this is a missing-handling reliability defect, not a speculative one. Concrete consequence: a stack-trace dump on stderr (directly breaking the PR's advertised "zero stderr bytes without --debug" contract) and a spurious exit 1 that collides with the tool's meaningful exit-code contract (0/2/3/130), so a pipeline consumer sees a false failure. The delivered output is intact (the crash is at the tail), which is why this isshould_fixrather than must_fix; the fix is the standard one-line EPIPE handler the reviewer names. Keep at should_fix.
Issue description
stdout.write(text) in text-streaming mode (and the flush write in cli.ts's exitAfterFlush) has no guard against EPIPE. Running the CLI in a normal shell pipeline like posthog-code-cli "..." | head -1 — exactly the use case the PR description calls out — closes the read end after one line. The next write to process.stdout then errors with EPIPE, and since nothing in the package registers an 'error' listener on process.stdout, Node's default behavior for an unhandled stream error is to throw, crashing the process with a raw stack trace instead of exiting cleanly.
Suggested fix
Install a process.stdout.on("error", (err) => { if (err.code === "EPIPE") process.exit(0); throw err; }) handler once at CLI startup (e.g. top of cli.ts) so a downstream reader closing the pipe early results in a clean exit rather than an uncaught-exception crash.
Prompt to fix with AI (copy-paste)
## Context
@packages/cli/src/output.ts#L42-44
<issue_description>
`stdout.write(text)` in text-streaming mode (and the flush write in `cli.ts`'s `exitAfterFlush`) has no guard against EPIPE. Running the CLI in a normal shell pipeline like `posthog-code-cli "..." | head -1` — exactly the use case the PR description calls out — closes the read end after one line. The next write to `process.stdout` then errors with EPIPE, and since nothing in the package registers an `'error'` listener on `process.stdout`, Node's default behavior for an unhandled stream error is to throw, crashing the process with a raw stack trace instead of exiting cleanly.
</issue_description>
<issue_validation>
- **Checked:** the streaming write `stdout.write(text)` (output.ts:42-44, fed `process.stdout` from `run.ts`'s `createOutputSink(options.output, process.stdout)`), the flush write in `cli.ts`'s `exitAfterFlush`, and every file in `packages/cli/src` for a `process.stdout.on("error")` / `uncaughtException` / EPIPE guard.
- **Found:** no stdout error listener is registered anywhere in the package. I reproduced the failure on this environment's Node (v24.18.0): a program streaming lines to stdout piped into `head -1` emits an unhandled `'error'` event once the reader closes → uncaught exception with a raw `Error: write EPIPE` stack trace on stderr and **exit code 1** (captured 25 stderr lines, `PIPESTATUS[0]=1`). The CLI writes one chunk per ACP `sessionUpdate`, so any multi-chunk response (the norm for a coding agent) hits a post-close write and triggers exactly this.
- **Impact:** the PR explicitly targets "shell pipelines," and `| head`, `| grep -q`, `| less`(quit early) are everyday idioms that WILL occur — this is a missing-handling reliability defect, not a speculative one. Concrete consequence: a stack-trace dump on stderr (directly breaking the PR's advertised "zero stderr bytes without --debug" contract) and a spurious exit 1 that collides with the tool's meaningful exit-code contract (0/2/3/130), so a pipeline consumer sees a false failure. The delivered output is intact (the crash is at the tail), which is why this is `should_fix` rather than must_fix; the fix is the standard one-line EPIPE handler the reviewer names. Keep at should_fix.
</issue_validation>
## Task
Investigate the issue and solve it
<potential_solution>
Install a `process.stdout.on("error", (err) => { if (err.code === "EPIPE") process.exit(0); throw err; })` handler once at CLI startup (e.g. top of `cli.ts`) so a downstream reader closing the pipe early results in a clean exit rather than an uncaught-exception crash.
</potential_solution>
| let prompt = parsed.prompt; | ||
| if (!prompt && !process.stdin.isTTY) { | ||
| prompt = (await text(process.stdin)).trim(); | ||
| } | ||
| if (!prompt) { | ||
| process.stderr.write( | ||
| "No prompt given. Pass one as an argument or pipe it on stdin.\n", | ||
| ); | ||
| return 1; | ||
| } |
There was a problem hiding this comment.
Positional prompt isn't trimmed like the stdin-sourced prompt, so a whitespace-only prompt slips past the "no prompt" guard
Why we think it's a valid issue
- Checked: the prompt-resolution flow at cli.ts:15-24 —
let prompt = parsed.prompt(positional, used verbatim), the stdin branchprompt = (await text(process.stdin)).trim()(trimmed), and theif (!prompt)rejection. - Found: the asymmetry is real and confirmed. A whitespace-only positional arg (
posthog-code-cli " ") leavesparsed.prompttruthy, so it skips the stdin branch AND passes!prompt, forwarding" "intorun(). The equivalent piped input is trimmed to"", fails!prompt, and is correctly rejected with "No prompt given" (exit 1). The author's stdin.trim()shows the intent is to reject empty/whitespace input — the positional path simply wasn't held to it. - Impact: beyond wasting a full agent turn (tokens + latency) on meaningless input, the exit codes diverge for equivalent input — whitespace positional runs and exits 0 (looks successful), whitespace pipe exits 1 (correctly flags no prompt). In the CI/cron contexts the PR targets, a templated
posthog-code-cli "$PROMPT"where$PROMPTresolved to whitespace would silently "succeed" instead of failing fast, masking the operator's empty-variable bug. Concrete trigger and concrete consequence, author-intended behavior bypassed on one path, trivial one-line fix — a real, actionable input-validation inconsistency. - Priority: kept at reviewer's
consider. The trigger is self-inflicted/low-frequency and the harm is a wasted turn plus a misleading exit code rather than data loss or wrong content, so consider (lowest, real-but-minor) is the honest severity.
Issue description
The prompt is trimmed only when read from stdin (prompt = (await text(process.stdin)).trim()); a prompt supplied as the positional argument is used exactly as typed. Running posthog-code-cli " " (or any whitespace-only string) leaves parsed.prompt as a non-empty, truthy string, so it skips both the stdin-read branch and the if (!prompt) "No prompt given" rejection, and gets forwarded straight into run() as if it were real content — burning a full agent turn on meaningless input instead of failing fast the way an equivalent whitespace-only piped prompt correctly does.
Suggested fix
Trim the resolved prompt before the emptiness check regardless of source, e.g. prompt = parsed.prompt?.trim() || (!process.stdin.isTTY ? (await text(process.stdin)).trim() : undefined), so both input paths are held to the same "non-empty after trimming" standard.
Prompt to fix with AI (copy-paste)
## Context
@packages/cli/src/cli.ts#L15-24
<issue_description>
The prompt is trimmed only when read from stdin (prompt = (await text(process.stdin)).trim()); a prompt supplied as the positional argument is used exactly as typed. Running posthog-code-cli " " (or any whitespace-only string) leaves parsed.prompt as a non-empty, truthy string, so it skips both the stdin-read branch and the if (!prompt) "No prompt given" rejection, and gets forwarded straight into run() as if it were real content — burning a full agent turn on meaningless input instead of failing fast the way an equivalent whitespace-only piped prompt correctly does.
</issue_description>
<issue_validation>
- **Checked:** the prompt-resolution flow at cli.ts:15-24 — `let prompt = parsed.prompt` (positional, used verbatim), the stdin branch `prompt = (await text(process.stdin)).trim()` (trimmed), and the `if (!prompt)` rejection.
- **Found:** the asymmetry is real and confirmed. A whitespace-only positional arg (`posthog-code-cli " "`) leaves `parsed.prompt` truthy, so it skips the stdin branch AND passes `!prompt`, forwarding `" "` into `run()`. The equivalent piped input is trimmed to `""`, fails `!prompt`, and is correctly rejected with "No prompt given" (exit 1). The author's stdin `.trim()` shows the intent is to reject empty/whitespace input — the positional path simply wasn't held to it.
- **Impact:** beyond wasting a full agent turn (tokens + latency) on meaningless input, the exit codes diverge for equivalent input — whitespace positional runs and exits 0 (looks successful), whitespace pipe exits 1 (correctly flags no prompt). In the CI/cron contexts the PR targets, a templated `posthog-code-cli "$PROMPT"` where `$PROMPT` resolved to whitespace would silently "succeed" instead of failing fast, masking the operator's empty-variable bug. Concrete trigger and concrete consequence, author-intended behavior bypassed on one path, trivial one-line fix — a real, actionable input-validation inconsistency.
- **Priority:** kept at reviewer's `consider`. The trigger is self-inflicted/low-frequency and the harm is a wasted turn plus a misleading exit code rather than data loss or wrong content, so consider (lowest, real-but-minor) is the honest severity.
</issue_validation>
## Task
Investigate the issue and solve it
<potential_solution>
Trim the resolved prompt before the emptiness check regardless of source, e.g. prompt = parsed.prompt?.trim() || (!process.stdin.isTTY ? (await text(process.stdin)).trim() : undefined), so both input paths are held to the same "non-empty after trimming" standard.
</potential_solution>
1 participant
Problem
There's no way to run the agent from a script. The desktop app needs a GUI, and cloud tasks need a sandbox; anyone who wants "prompt in, answer on stdout" (CI checks, shell pipelines, cron jobs) has to shell out to a different tool entirely.
Changes
Adds
packages/cli(@posthog/code-cli, binposthog-code-cli), a headless CLI host that drives the same in-process ACP connection the desktop app uses viaAgentfrom@posthog/agent:--permission-mode auto(default) orbypassPermissions. Interactive modes are rejected since there's no UI to answer prompts. Tool permissions that still reach the client are auto-approved (preferringallow_onceso one-shot runs don't persist allow rules);AskUserQuestioncalls are parked with the same no-user-available contract cloud background runs use, so the model restates the question as text and ends its turn.--output jsonbuffers and emits one{ text, stopReason, usage, sessionId }document for machine consumption.ANTHROPIC_API_KEY/ANTHROPIC_AUTH_TOKEN/ANTHROPIC_BASE_URLenv vars, falling back to a stored claude login credential.ClaudeAcpAgentpreviously hardcoded a debug console logger, ignoring the logger injected throughcreateAcpConnection; it now accepts the injected logger (keeping the console fallback), which lets the CLI route adapter diagnostics off stdout behind--debug. Desktop and cloud behavior is unchanged since they don't pass one.The new package lands in
packages/rather than theapps/clislot listed in AGENTS.md because repo settings deny writes there; the AGENTS.md hosts table now points atpackages/cli.How did you test this?
pnpm --filter @posthog/code-cli test).@posthog/agentsuite after the logger change (1625 passed; one pre-existing flake insettings.test.tsthat passes in isolation).--output jsonshape, clean stdout (zero stderr bytes without--debug, 15 diagnostic lines with it), and exit codes for bad flags/missing prompt.pnpm --filter @posthog/code-cli test:e2e) using the same gateway env contract aspackages/agent/e2e.Automatic notifications
Created with PostHog Code