feat(llm): foreground print-mode transport for Claude tasks#511
Conversation
…E_FOREGROUND) Claude tasks run through the CLI's background job service (--bg + job state polling). That layer has proven fragile in production: sessions intermittently die mid-turn without ever writing their state file, so the task starves until its deadline. Observed across CLI versions (2.1.210 and 2.1.211), at reviewer concurrency 1 and 3, on a task that failed 15 consecutive attempts while isolated bg probes succeeded. Add an opt-in foreground transport (CR_CLAUDE_FOREGROUND=1): claude -p --output-format json as a plain bounded child process. Same prompt-file / result-file contract, same flags, same scratch layout; session id parsed from the print-mode JSON. No job service, no state files, no detached workers — lifetime and teardown are fully owned by cr (task deadline + process-group kill + pipe-close teardown). Verification: the 15-consecutive-failure reproducer completed end-to-end in foreground mode (all 6 tasks, rust reviewer 75s). Unit tests cover arg shape, result-file contract, missing-result error, transient stderr classification, and opt-in gating. Background mode remains the default.
2ff4b92 to
62c30b8
Compare
piekstra-dev
left a comment
piekstra-dev
left a comment
There was a problem hiding this comment.
Automated PR Review
Reviewed commit: 62c30b8f8c89
Profile: reviewer - Posting as: piekstra-dev
Summary
| Reviewer | Findings |
|---|---|
| go:implementation-tests | 3 |
go:implementation-tests (3 findings)
Major - internal/llmadapters/subprocess.go:475
Resume() now dispatches to startClaudeForeground(ctx, req, sessionID) when CR_CLAUDE_FOREGROUND is set, but no test exercises the foreground resume path. buildClaudeForegroundArgs only appends --resume when resumeSessionID != "", and that branch is only reachable via Resume(), which none of the new tests call with foreground enabled (TestSubprocessClaudeForegroundMode/NoResultFile/TransientFailure all call Start(), never Resume()). This is exactly the multi-turn continuation path the PR is meant to make reliable; a regression here (wrong flag order, dropped session id, or Resume() silently falling back to --bg) would only surface in production. Add a test that calls adapter.Resume(ctx, "prior-session", req) with CR_CLAUDE_FOREGROUND=1 and asserts the recorded args contain "-p" and "--resume prior-session" (mirroring the existing bg Resume tests around subprocess_test.go:174/797).
Minor - internal/llmadapters/subprocess_test.go:1366
The new validateArgs guard (
containsFlag(checkedArgs, "--bg") == containsFlag(checkedArgs, "-p")) is the only safety net preventing a caller from launching claude_cli with neither or both transports, but it's untested. TestClaudeForegroundDisabledByDefault only unit-tests claudeForegroundEnabled() directly; none of the new tests hit validateArgs with a --bg+-p or neither-flag arg list to confirm it still returns ErrUnsafeSubprocessConfig with the updated message. Since this replaced the previous unconditionalmust use background jobscheck, a table test asserting both the XOR-violation and the passing XOR case would protect this safety-relevant validation from silent regression as the arg-building code evolves.
Minor - internal/llmadapters/subprocess.go:446
Session-id extraction locates the JSON blob via
bytes.IndexByte(outBytes, '{')over the entire captured stdout and then discards any json.Unmarshal error (_ = json.Unmarshal(...)). Print-mode Claude CLI output isn't guaranteed to be exactly one bare JSON object on stdout (banners/warnings before the trailing JSON, or a truncated write, could produce a leading '{' from unrelated content or a parse failure that never surfaces). Because the error is dropped, a malformed match silently yields an empty SessionID instead of a diagnosable error, and no test exercises stdout with any preamble before the JSON payload. Consider parsing from the last complete JSON object (e.g. scan for a matching final line) or logging when json.Unmarshal fails, and add a test with non-JSON preamble text on stdout to lock in the intended parsing contract.
Reviewer Coverage
| Reviewer | Status | Inspected | Skipped | Constraints |
|---|---|---|---|---|
| go:implementation-tests | complete_broad | internal/llmadapters/subprocess.go, internal/llmadapters/subprocess_test.go | unavailable | unavailable |
0 PR discussion threads considered. 0 summarized; 0 resolved.
Completed in 3m 22s | unavailable | claude-sonnet-5 | cr 0.10.257
| Field | Value |
|---|---|
| Model | claude-sonnet-5 |
| Reviewers | go:implementation-tests |
| Engine | claude_cli · claude-sonnet-5 |
| Reviewed by | cr · piekstra-dev |
| Duration | 3m 22s wall · 3m 09s compute |
| Cost | unavailable |
| Tokens | 38 in / 11.4k out |
Per-workstream usage
| Workstream | Model | In | Out | Cache read | Cache create | Cost | Duration |
|---|---|---|---|---|---|---|---|
| orchestrator-selection | claude-sonnet-5 | 6 | 1.1k | 30.9k | 19.0k | unavailable | 27s |
| go:implementation-tests | claude-sonnet-5 | 26 | 9.8k | 422.9k | 55.4k | unavailable | 2m 12s |
| orchestrator-rollup | claude-sonnet-5 | 6 | 527 | 39.3k | 37.2k | unavailable | 29s |
|
|
||
| if result.err == nil { | ||
| var parsed claudeForegroundOutput | ||
| if idx := bytes.IndexByte(outBytes, '{'); idx >= 0 { |
There was a problem hiding this comment.
Session-id extraction locates the JSON blob via bytes.IndexByte(outBytes, '{') over the entire captured stdout and then discards any json.Unmarshal error (_ = json.Unmarshal(...)). Print-mode Claude CLI output isn't guaranteed to be exactly one bare JSON object on stdout (banners/warnings before the trailing JSON, or a truncated write, could produce a leading '{' from unrelated content or a parse failure that never surfaces). Because the error is dropped, a malformed match silently yields an empty SessionID instead of a diagnosable error, and no test exercises stdout with any preamble before the JSON payload. Consider parsing from the last complete JSON object (e.g. scan for a matching final line) or logging when json.Unmarshal fails, and add a test with non-JSON preamble text on stdout to lock in the intended parsing contract.
Reply inline to this comment.
| @@ -1366,6 +1366,10 @@ func TestSubprocessHelperProcess(_ *testing.T) { | |||
| runClaudeBGHelper(os.Getenv("LLM_HELPER_MODE"), args) | |||
There was a problem hiding this comment.
File-level note: internal/llmadapters/subprocess_test.go
The new validateArgs guard (containsFlag(checkedArgs, "--bg") == containsFlag(checkedArgs, "-p")) is the only safety net preventing a caller from launching claude_cli with neither or both transports, but it's untested. TestClaudeForegroundDisabledByDefault only unit-tests claudeForegroundEnabled() directly; none of the new tests hit validateArgs with a --bg+-p or neither-flag arg list to confirm it still returns ErrUnsafeSubprocessConfig with the updated message. Since this replaced the previous unconditional must use background jobs check, a table test asserting both the XOR-violation and the passing XOR case would protect this safety-relevant validation from silent regression as the arg-building code evolves.
Reply inline to this comment.
| s.Finish(result.response, result.err) | ||
| } | ||
|
|
||
| func (a *SubprocessAdapter) processEnv(req Request, scratch string) ([]string, error) { |
There was a problem hiding this comment.
Resume() now dispatches to startClaudeForeground(ctx, req, sessionID) when CR_CLAUDE_FOREGROUND is set, but no test exercises the foreground resume path. buildClaudeForegroundArgs only appends --resume when resumeSessionID != "", and that branch is only reachable via Resume(), which none of the new tests call with foreground enabled (TestSubprocessClaudeForegroundMode/NoResultFile/TransientFailure all call Start(), never Resume()). This is exactly the multi-turn continuation path the PR is meant to make reliable; a regression here (wrong flag order, dropped session id, or Resume() silently falling back to --bg) would only surface in production. Add a test that calls adapter.Resume(ctx, "prior-session", req) with CR_CLAUDE_FOREGROUND=1 and asserts the recorded args contain "-p" and "--resume prior-session" (mirroring the existing bg Resume tests around subprocess_test.go:174/797).
Reply inline to this comment.
…e-tolerant output parsing Addresses review findings on #511: adds the foreground Resume path test (-p + --resume recorded, no --bg fallback), a table test for the exactly-one-transport validateArgs guard, and hardens print-mode output parsing to take the last parseable JSON line (banners/warnings before the payload no longer break session-id extraction; parse failures are logged).
piekstra-dev
left a comment
piekstra-dev
left a comment
There was a problem hiding this comment.
Automated PR Review
Reviewed commit: 90e0ed5bffc3
Profile: reviewer - Posting as: piekstra-dev
Summary
| Reviewer | Findings |
|---|---|
| go:implementation-tests | 0 |
| structure:repo-health | 1 |
| policies:conventions | 0 |
structure:repo-health (1 finding)
Minor - internal/llmadapters/subprocess.go:293
README.md (around line 628) is the documented source of truth for
claude_clitransport behavior: it explains the--bgjob-service flow and documentsCR_CLAUDE_BG_WORK_DIRas the override knob operators need to know about. This PR adds a second, mutually-exclusive transport gated by a new operator-facing env var,CR_CLAUDE_FOREGROUND, but only describes it in code comments (claudeForegroundEnabled) and the PR description — it isn't added to that README section. An operator debugging the exact production failure this PR fixes (bg jobs dying silently) has no versioned doc pointing them at the opt-in flag that fixes it. Add a short paragraph to README.md next to the existing--bg/CR_CLAUDE_BG_WORK_DIRdescription coveringCR_CLAUDE_FOREGROUNDand when to use it.
Reviewer Coverage
| Reviewer | Status | Inspected | Skipped | Constraints |
|---|---|---|---|---|
| go:implementation-tests | complete_broad | internal/llmadapters/subprocess.go, internal/llmadapters/subprocess_test.go | unavailable | unavailable |
| structure:repo-health | complete_broad | internal/llmadapters/subprocess.go | unavailable | unavailable |
| policies:conventions | complete_broad | internal/llmadapters/subprocess.go | unavailable | No local or sibling copies of cli-common/docs or .github were present in the workbench, so this review only checked repo-local conventions (README.md and existing subprocess.go patterns) rather than Open CLI Collective shared standards. |
0 PR discussion threads considered. 0 summarized; 0 resolved.
Completed in 3m 24s | unavailable | claude-sonnet-5 | cr dev
| Field | Value |
|---|---|
| Model | claude-sonnet-5 |
| Reviewers | go:implementation-tests, structure:repo-health, policies:conventions |
| Engine | claude_cli · claude-sonnet-5 |
| Reviewed by | cr · piekstra-dev |
| Duration | 3m 24s wall · 6m 09s compute |
| Cost | unavailable |
| Tokens | unavailable |
Per-workstream usage
| Workstream | Model | In | Out | Cache read | Cache create | Cost | Duration |
|---|---|---|---|---|---|---|---|
| orchestrator-selection | claude-sonnet-5 | unavailable | unavailable | unavailable | unavailable | unavailable | 17s |
| go:implementation-tests | claude-sonnet-5 | unavailable | unavailable | unavailable | unavailable | unavailable | 2m 01s |
| structure:repo-health | claude-sonnet-5 | unavailable | unavailable | unavailable | unavailable | unavailable | 1m 13s |
| policies:conventions | claude-sonnet-5 | unavailable | unavailable | unavailable | unavailable | unavailable | 2m 22s |
| orchestrator-rollup | claude-sonnet-5 | unavailable | unavailable | unavailable | unavailable | unavailable | 14s |
| // deadline. Foreground mode is a plain child process — its lifetime, output, | ||
| // and teardown are fully owned by cr (bounded by the task timeout, cleaned up | ||
| // by the process-group kill and pipe-close teardown). | ||
| func claudeForegroundEnabled(env []string) bool { |
There was a problem hiding this comment.
README.md (around line 628) is the documented source of truth for claude_cli transport behavior: it explains the --bg job-service flow and documents CR_CLAUDE_BG_WORK_DIR as the override knob operators need to know about. This PR adds a second, mutually-exclusive transport gated by a new operator-facing env var, CR_CLAUDE_FOREGROUND, but only describes it in code comments (claudeForegroundEnabled) and the PR description — it isn't added to that README section. An operator debugging the exact production failure this PR fixes (bg jobs dying silently) has no versioned doc pointing them at the opt-in flag that fixes it. Add a short paragraph to README.md next to the existing --bg/CR_CLAUDE_BG_WORK_DIR description covering CR_CLAUDE_FOREGROUND and when to use it.
Reply inline to this comment.
2 participants
Problem
Claude tasks run via the CLI's background job service (
--bg+jobs/<id>/state.jsonpolling). In production that layer intermittently kills sessions mid-turn without ever writing a state file — the task starves until its deadline. Reproduced across CLI versions (2.1.210 & 2.1.211) and reviewer concurrency levels; one reviewer task failed 15 consecutive attempts while isolated bg probes succeeded.Fix
Opt-in foreground transport (
CR_CLAUDE_FOREGROUND=1):claude -p --output-format jsonas a plain bounded child. Same prompt-file/result-file contract and flag safety validation; session id from the print-mode JSON. No job service, no state polling, no detached workers — lifetime fully owned by cr's existing task deadline + process-group + pipe teardown.Verification
-p/--output-format json, never--bg), result-file success path, missing-result error,ErrTransienton overloaded stderr, strict opt-in default.go test ./internal/llmadapters/ ./internal/llm/,make lint,go build ./...green.--bgXOR-p).