fix(llmadapters): fall back to foreground when a Claude background job fails - #577
Conversation
…b fails The Claude job-service transport is the default for reviewer tasks, and this file already documents it as the fragile one: sessions die mid-turn without writing a state file, and a job can enter a terminal state having never run the task at all. Observed in production: a job reported `blocked` because the prompt file it was told to read was gone, on four consecutive runs of the same review, always taking one reviewer with it. Losing a reviewer is worse than it looks from inside the adapter. A review that reports no findings because a reviewer never ran is not the same as a review that found nothing, and a caller gating on coverage cannot tell them apart, so one flaky job service withholds approval on an unrelated change. Background failures that mean "the transport never ran the task" are now marked with ErrClaudeBGTransport, and such a task is retried once on the foreground transport, which this file already describes as fully cr-owned. A job that DID complete and wrote nothing usable is a model failure, is not marked, and is not retried, since retrying would only repeat it. When the retry fails too, the original transport failure is what surfaces: the retry failing says nothing new, and burying the cause under it is how a job-service problem gets read as a reviewer problem. CR_CLAUDE_FOREGROUND is unchanged and still selects foreground outright.
monit-reviewer
left a comment
monit-reviewer
left a comment
There was a problem hiding this comment.
Automated PR Review
Reviewed commit: dc07ea7fdc6d
Profile: claude-monit-reviewer - Posting as: monit-reviewer
Summary
| Reviewer | Findings |
|---|---|
| go:implementation-tests | 1 |
| architecture:solid-reviewer-agnostic | 5 |
| security:code-auditor | 0 |
| structure:harness-engineering | 0 |
go:implementation-tests (1 finding)
Major - internal/llmadapters/subprocess_test.go:1635
TestSubprocessClaudeBackgroundStatesAndCleanup does not guard the new fallback behavior for the blocked/failed/waiting/stopped/stop-fails cases, even though those states are exactly the ones startClaude now retries on foreground (waitForClaudeBGState wraps them in ErrClaudeBGTransport). With CR_CLAUDE_FOREGROUND unset, each of those subtests currently launches a second (foreground) helper process via claudeFallbackStream.Wait, and that second launch happens to fail too because runClaudeForegroundHelper has no case for modes like "bg-blocked"/"bg-failed"/etc. -- so the original background error is what the test observes, purely because the fallback attempt errors out for an unrelated reason (unhandled mode string), not because the test asserts single-launch behavior. assertClaudeCleanup only scans records[1:] for stop/rm control calls matching job-1; it never asserts len(records) or that no foreground launch occurred. So a regression that makes the foreground fallback succeed for these states (or a regression that stops wrapping them in ErrClaudeBGTransport) would not necessarily be caught: the former could produce a passing test with the wrong final result if the substrings still line up, and the latter removes the retry with no test noticing the extra launch disappeared. Add an explicit assertion (e.g. len(readHelperRecords(...)) == 1, or a helper-record count keyed by CLAUDE_CONFIG_DIR presence indicating only the --bg transport ran) for the non-transport-failure states, and/or extend runClaudeForegroundHelper with real cases for these modes so the fallback path in this table is deliberately exercised rather than incidentally failing closed.
architecture:solid-reviewer-agnostic (5 findings)
Major - internal/llmadapters/subprocess.go:370
The launch path falls back on any error and drops it, which contradicts the classification this PR introduces.
ErrClaudeBGTransportexists precisely so that only "the transport never ran the task" failures are retried (see the sentinel's own doc at line 72), andWaitat line 401 honors that. This branch does not:erris discarded unexamined, so aErrUnsafeSubprocessConfigfromvalidateArgs, a workspace-validation failure frominvocationScratchDir, a non-empty scratch dir, or aclaudeBGWorkingDirmkdir failure all silently become a foreground attempt.The safety gate itself is not bypassed (
startClaudeForegroundre-runsvalidateArgs), but the error accounting is. When the foreground start also fails, the caller sees only the foreground error, so a genuine misconfiguration in the background arg construction is reported as a foreground problem, and a bug that only manifests on the bg path becomes invisible in production. That is a swallowed error with no explicit decision recorded (U-L2), and it makes the two retry decision points in the same file disagree about what is retryable (U-S1).Suggested fix: gate this branch the same way
Waitis gated — fall back only when the launch failure is transport-shaped (mark the launch-side failures withErrClaudeBGTransportand testerrors.Is, or at minimum return immediately onErrUnsafeSubprocessConfig). When the fallback start then fails too, join the discarded background error into what is returned (errors.Join(err, fgErr)) rather than returning only the second one.
Minor - internal/llmadapters/subprocess.go:396
SessionID()can report a session that does not correspond to the returned response. When the fallback succeeds but its print-mode output does not parse (line 573 logs exactly that and leaves the session id unset), this returns the background job's session id whileWaitreturns the foreground response.runOnceAttempt(internal/llm/adapter.go:347) pairs those two as one result, so a later resume targets a dead background session that the PR describes as the broken transport, and the foreground run's result is recorded against it.The PR body frames this as "session ids are preserved", but preservation is only correct while the primary's response is the one being returned; once the fallback's response wins, the primary's session id is stale state attached to someone else's output. The degradation is graceful (the resume fails with
ErrMissingProviderSessionand is handled at internal/llm/adapter.go:309), so the cost is a wasted attempt rather than a wrong review.Suggested fix: record which stream produced the returned response (set a
winner Streamunder the existing mutex inWait) and haveSessionID()report only that stream's id, letting empty be empty.
Major - internal/llmadapters/subprocess.go:404
The foreground retry destroys the evidence of the failure it is retrying.
startClaudeForegroundpasses the samereq.LogPathtolaunchProcess, andopenSubprocessLog(internal/llm/subprocess.go:294) opens it withos.Create, which truncates. The background stream has already flushed and closed that file by the timeWaitreturns, so the retry wipes it.Two consequences. First, the diagnostic the PR body itself relies on is gone: the
backgrounded · <id>banner and the blocked-state detail are exactly what identified this failure mode, and after a fallback the task log contains only the foreground attempt. Second, when the fallback also fails, line 418 deliberately reports the primary error, so the returned error and the on-disk artifact now describe different runs, with nothing in the log explaining the error the caller sees. A caller inspecting the log to explain a transport failure is silently handed the wrong run's output (U-L1: this Stream's observable side effects diverge from every other Stream, where the log atreq.LogPathcorresponds to the reported error).Suggested fix: give the fallback its own log destination, e.g. copy
s.reqand setLogPathto a sibling path (<name>.foreground.log, or the same stem with a suffix before the extension) before callingstartClaudeForeground, and note in the primary log that the task continued in the other file. Opening the retry's log in append mode instead would also preserve both, but a distinct path keeps the two transports' output separable.
Minor - internal/llmadapters/subprocess.go:413
The documented meaning of
SubprocessOptions.Timeoutno longer holds for Claude tasks. Lines 36-39 state it bounds "a single LLM task (one subprocess run, or one background job launch + result wait)", anddefaultLLMTaskTimeout(line 48) justifies its value by converting "one worker hung" into a timely failure. With the fallback, the foreground retry gets a fresh fulla.timeoutfromlaunchProcess, so a Claude task's worst case is now roughly twice the configured bound. It is reachable: a job that sitsworkingfor most of the window and then goesblocked, or the completed-without-session-id case above, both fail late and then start a fresh 14-minute attempt. Bg timeouts themselves do not fall back (they are not markedErrClaudeBGTransportandWaitchecksctx.Err()), which limits this to the late-failing transport states.A doc comment that states a bound the code no longer honors is worse than no comment, because callers size their own task deadlines from it (U-G1: the changed behavior is not reflected in the contract it changes).
Suggested fix: either derive one deadline in
startClaude(context.WithTimeout(ctx, a.timeout)passed to both attempts, so the retry inherits the remaining budget) or amend theTimeoutcomment to say it bounds each transport attempt and that a Claude task may make two.
Major - internal/llmadapters/subprocess.go:1393
Marking "job completed without session id" as
ErrClaudeBGTransportnow triggers a full second model run for a job that already produced a usable result.waitForClaudeBGResultchecks the session id (lines 1388-1394) beforereadFirstNonEmptyFile(line 1395), so a background job that ran to completion and wrote a validcr-result.jsonis discarded and re-run end to end whenever itsstate.jsonnever carried a session id. That contradicts the PR's own stated rule that a completed job is not retried, and the cost is not one wasted attempt but a whole duplicated reviewer task (time and tokens), for a job whose output was sitting on disk.The foreground path in this same file takes the opposite position on the identical condition: line 571 says "The session id is only needed for resume, so a parse failure must not fail the task" and proceeds to read the result file with an empty session id. Two sibling transports disagreeing on whether a missing session id is fatal is the substitutability problem (U-L1), and here the stricter of the two is the one being retried.
Suggested fix: read the result file first. If
readFirstNonEmptyFilesucceeds, return the response with whatever session id was found (empty is already a valid answer that callers handle, per line 571). Reserve theErrClaudeBGTransportmarking for the case where there is both no session id and no result.
Reviewer Coverage
go:implementation-tests— complete (broad); skipped: none; constraints: nonearchitecture:solid-reviewer-agnostic— complete (broad); inspected 1 assigned file (2 inspected across reviewers):internal/llmadapters/subprocess.go; skipped: none; constraints: Could not run the project's build/test/lint here (command execution was restricted), so no red-check claims are made either way. Reviewed the head file contents plus the PR description and change map; a line-level diff was not available in this sandbox, so new-vs-pre-existing attribution is inferred (ErrClaudeBGTransport, startClaude fallback, claudeFallbackStream, and the bg error classifications treated as changed). internal/llmadapters/subprocess_test.go was not in the assigned file set; test coverage is judged only from the PR description's test summary.security:code-auditor— complete (broad); inspected 1 assigned file (2 inspected across reviewers):internal/llmadapters/subprocess.go; skipped: none; constraints: nonestructure:harness-engineering— complete (broad); inspected 1 assigned file (2 inspected across reviewers):internal/llmadapters/subprocess.go; skipped: none; constraints: none
Inspected files (2)
internal/llmadapters/subprocess.gointernal/llmadapters/subprocess_test.go
0 PR discussion threads considered. 0 summarized; 0 resolved.
Completed in 5m 51s | $5.78 | claude-sonnet-5, claude-opus-5 | cr 0.10.294
| Field | Value |
|---|---|
| Model | claude-sonnet-5, claude-opus-5 |
| Reviewers | go:implementation-tests, architecture:solid-reviewer-agnostic, security:code-auditor, structure:harness-engineering |
| Engine | claude_cli · claude-sonnet-5, claude-opus-5 |
| Reviewed by | cr · monit-reviewer |
| Duration | 5m 51s wall · 11m 58s compute |
| Cost | $5.78 |
| Tokens | 142 in / 51.1k out |
Per-workstream usage
orchestrator-selection— claude-sonnet-5- In: 6
- Out: 2.0k
- Cache read: 115.4k
- Cache create: 65.3k
- Cost: $0.31
- Duration: 26s
go:implementation-tests— claude-sonnet-5- In: 44
- Out: 16.0k
- Cache read: 2.3M
- Cache create: 147.1k
- Cost: $1.21
- Duration: 3m 12s
architecture:solid-reviewer-agnostic— claude-opus-5- In: 40
- Out: 18.4k
- Cache read: 1.8M
- Cache create: 133.2k
- Cost: $2.68
- Duration: 4m 25s
security:code-auditor— claude-sonnet-5- In: 16
- Out: 2.0k
- Cache read: 535.6k
- Cache create: 89.6k
- Cost: $0.49
- Duration: 30s
structure:harness-engineering— claude-sonnet-5- In: 30
- Out: 9.4k
- Cache read: 1.2M
- Cache create: 108.3k
- Cost: $0.78
- Duration: 2m 41s
orchestrator-rollup— claude-sonnet-5- In: 6
- Out: 3.2k
- Cache read: 154.7k
- Cache create: 64.9k
- Cost: $0.32
- Duration: 42s
…t of evidence Review findings. A job that ran to completion and wrote its result is finished, whatever its state file says about session ids. The session-id check ran before the result read, so such a job was marked as a transport failure and re-run end to end, spending a second model run on output already on disk — the opposite of this change's own rule that a completed job is not retried. The result is read first now, and an empty session id is returned as empty, which is the position the foreground transport already takes on the same condition. The launch-side fallback retried on any error, so a configuration failure in background arg construction was reported as a foreground problem. It now applies the same ErrClaudeBGTransport test the wait side does, and joins the original error when the fallback launch fails too. The retry no longer truncates the log of the failure it is retrying. Task logs are opened with os.Create, so a shared path erased the background attempt — including the banner and blocked-state detail that identify this failure mode, and the only account of the error still reported when the retry also fails. The retry writes to a sibling path instead. Both attempts now share one task deadline rather than each starting a full window, so the Timeout contract still bounds a task rather than an attempt. SessionID reports the stream that produced the response and nothing else. Pairing a foreground response with the background job's session sent a later resume at a session the transport had already lost. Tests: the states table now asserts launch counts per case, so a retry that stops happening, or one that starts happening for a model failure, fails the test rather than passing on an unhandled helper mode; a completed job with a result and no session id is not retried; a fallback keeps both transports' logs.
monit-reviewer
left a comment
monit-reviewer
left a comment
There was a problem hiding this comment.
Automated PR Review
Reviewed commit: d3def239dd0f
Profile: claude-monit-reviewer - Posting as: monit-reviewer
Summary
| Reviewer | Findings |
|---|---|
| go:implementation-tests | 1 |
| architecture:solid-reviewer-agnostic | 3 |
| security:code-auditor | 0 |
| structure:harness-engineering | 0 |
go:implementation-tests (1 finding)
Minor - internal/llmadapters/subprocess.go:395
taskDeadline has no direct test, and no test exercises the behavior it exists for: that a retry runs under what remains of the task's original budget rather than a fresh window. The function has four real branches (zero timeout with no ctx deadline, timeout only, ctx deadline only, and picking the earlier of the two when both are set), and it is exactly the mechanism the earlier review round asked for ("one budget for the task, not one per attempt"). None of the current fallback tests (TestSubprocessClaudeBackgroundFailureFallsBackToForeground, TestSubprocessClaudeFallbackKeepsBothTransportLogs) constrain the adapter timeout tightly enough to prove the retry actually inherits the primary's remaining budget instead of a full new one -- they all use a generous 5s timeout where either behavior would pass. Add a table test on taskDeadline itself (zero timeout, ctx-only deadline, timeout-only deadline, earliest-of-both) plus one fallback test with a short adapter timeout that asserts the foreground retry's context is bounded by what's left of the original window, not a fresh a.timeout.
architecture:solid-reviewer-agnostic (3 findings)
Minor - internal/llmadapters/subprocess.go:372
This branch cannot be taken.
ErrClaudeBGTransportis only ever attached inside the wait path (lines 1095, 1474, 1501), all of which run after a successful launch; every return instartClaudeBG(lines 240-284) passes through a raw error frominvocationScratchDir,validateScratchDir,writeClaudeBGPromptFile,buildArgsForSession,validateArgs,claudeBGWorkingDir,processEnv, orlaunchProcess, none of which wrap the sentinel. Soerrors.Is(err, ErrClaudeBGTransport)is always false here and lines 375-379 are dead. Nothing in subprocess_test.go references the sentinel, and no test covers this branch, which is consistent with it being unreachable.That matters beyond tidiness for two reasons. A reader sees a handled case that is not handled, and the branch is where a future "mark this launch failure as transport" change would land silently. It also still passes
requnchanged tostartClaudeForeground, so the moment it did become reachable it would reuse the primaryLogPathand reintroduce the truncationforegroundRetryLogPathwas added to prevent (U-O1: an extension point with no reachable case; U-G1: new surface with no real consumer).Suggested fix: delete the branch and return
errdirectly, keeping the comment that a launch failure is configuration or a missing binary and would fail identically in foreground. If a launch failure is meant to be retryable, then wrap thelaunchProcesserror at line 281-283 withErrClaudeBGTransport, route it throughforegroundRetryLogPathasWaitdoes, and add a case to the existing table test so the branch is exercised.
Nits - internal/llmadapters/subprocess.go:424
fallbackis now write-only: it is assigned under the mutex at line 463 and never read anywhere in the package, sinceSessionIDreadswinnerinstead. The field's own doc at line 422 still describes it as carrying "the retry once started", a role nothing consumes.Suggested fix: drop the field and the lock section at lines 462-464, and trim the comment to describe
winneralone. Keep it only if something outside this file is expected to read it, in which case the comment should say what.
Minor - internal/llmadapters/subprocess.go:1471
An empty result file with no session id is marked as a transport failure and retried, which contradicts the rule this PR states for itself: "A job that completed and wrote an empty or missing result file is not marked, because retrying would only repeat it."
readFirstNonEmptyFilereturns an error for both "the file exists and is blank" and "no file at all" (lines 1528-1533 of the pre-existing helper), so this branch cannot distinguish them and treats the empty-file case as "the job left nothing behind". The comment at line 1472 asserts the stronger claim the code cannot check. The cost is a full duplicate model run for a job that did run and did write.The corner is narrow (it needs both an empty result and a missing session id), which is why this is minor rather than major, and the fix should not be message matching, since branching on error strings is what the typed-error rule exists to avoid (U-L2).
Suggested fix: give the helper two sentinels, e.g.
errClaudeBGEmptyResultanderrClaudeBGMissingResult, wrap its two returns accordingly, and gate this branch onerrors.Is(err, errClaudeBGMissingResult). An empty result then falls through to line 1476 and is reported as the model failure it is.
Reviewer Coverage
go:implementation-tests— complete (constrained); skipped: none; constraints: nonearchitecture:solid-reviewer-agnostic— complete (constrained); inspected 1 assigned file (2 inspected across reviewers):internal/llmadapters/subprocess.go; skipped: none; constraints: A line-level diff was not available in this sandbox; new-vs-pre-existing attribution is inferred from the head file, the change map, and the settled threads. Could not run the project's build/test/lint here (command execution was restricted), so no red-check claims are made either way. Re-review at d3def23. Verified the four settled threads against the head file: launch-path gating, single task deadline, sibling retry log path, and winner-based SessionID are all present and behave as described. The fifth thread (session id checked before the result file) is fixed at lines 1461-1476: the result is read first and the transport marking now applies only when neither a result nor a session exists. internal/llmadapters/subprocess_test.go was outside the allowed file set; it was read only as evidence about coverage of the paths below, not reviewed.security:code-auditor— complete (constrained); inspected 1 assigned file (2 inspected across reviewers):internal/llmadapters/subprocess.go; skipped: none; constraints: nonestructure:harness-engineering— complete (constrained); inspected 1 assigned file (2 inspected across reviewers):internal/llmadapters/subprocess.go; skipped: none; constraints: none
Inspected files (2)
internal/llmadapters/subprocess.gointernal/llmadapters/subprocess_test.go
0 PR discussion threads considered. 0 summarized; 0 resolved.
Completed in 3m 15s | $5.39 | claude-sonnet-5, claude-opus-5 | cr 0.10.294
| Field | Value |
|---|---|
| Model | claude-sonnet-5, claude-opus-5 |
| Reviewers | go:implementation-tests, architecture:solid-reviewer-agnostic, security:code-auditor, structure:harness-engineering |
| Engine | claude_cli · claude-sonnet-5, claude-opus-5 |
| Reviewed by | cr · monit-reviewer |
| Duration | 3m 15s wall · 5m 26s compute |
| Cost | $5.39 |
| Tokens | 84 in / 20.6k out |
Per-workstream usage
go:implementation-tests— claude-sonnet-5- In: 26
- Out: 5.5k
- Cache read: 1.9M
- Cache create: 170.5k
- Cost: $1.12
- Duration: 1m 16s
architecture:solid-reviewer-agnostic— claude-opus-5- In: 22
- Out: 10.2k
- Cache read: 1.5M
- Cache create: 162.8k
- Cost: $2.64
- Duration: 2m 36s
security:code-auditor— claude-sonnet-5- In: 12
- Out: 1.5k
- Cache read: 492.8k
- Cache create: 104.6k
- Cost: $0.53
- Duration: 27s
structure:harness-engineering— claude-sonnet-5- In: 18
- Out: 2.6k
- Cache read: 999.4k
- Cache create: 134.5k
- Cost: $0.76
- Duration: 52s
orchestrator-rollup— claude-sonnet-5- In: 6
- Out: 693
- Cache read: 177.0k
- Cache create: 72.1k
- Cost: $0.33
- Duration: 14s
…y result from a missing one Review findings. The launch-side fallback could never run: nothing on the launch path wraps ErrClaudeBGTransport, so the branch was dead, and a future change marking a launch failure would have landed there silently, reusing the primary log path and reintroducing the truncation the sibling path was added to prevent. A launch fails on configuration, a missing binary, or a scratch dir that could not be made, and foreground fails the same way, so the error is returned. An empty result file with no session id was being marked as a transport failure and retried, which the stated rule excludes: the job ran, and running it again repeats it. readFirstNonEmptyFile now returns errClaudeBGEmptyResult or errClaudeBGMissingResult, and only the missing case can mean the transport left nothing behind. The two are distinguished by sentinel rather than by message text. The fallback field was write-only once SessionID started reading the winning stream, so it is gone. Tests: taskDeadline covers its four branches (no bound, timeout only, context only, and the earlier of both), and a spent budget is shown to launch no retry at all rather than starting a fresh window.
monit-reviewer
left a comment
monit-reviewer
left a comment
There was a problem hiding this comment.
Automated PR Review
Reviewed commit: 2335eeeac765
Profile: claude-monit-reviewer - Posting as: monit-reviewer
Summary
| Reviewer | Findings |
|---|---|
| go:implementation-tests | 0 |
| architecture:solid-reviewer-agnostic | 1 |
| security:code-auditor | 0 |
| structure:harness-engineering | 0 |
architecture:solid-reviewer-agnostic (1 finding)
Nits - internal/llmadapters/subprocess.go:1589
These sentinels are shared by both transports but are named and documented for one.
readFirstNonEmptyFileis called from the foreground path at line 636 as well as the background path at line 1456, soerrClaudeBGMissingResultreaches a foreground caller that wraps it at line 645, producing "Claude foreground task completed without a result file: llm subprocess: Claude background job completed without writing result file". The docstring likewise explains the values purely in background terms ("Only the second can mean the transport never ran the task"), which is the background caller's rule rather than a property of the sentinel.The doubled wording is not a regression: the same strings were returned inline from this helper before the diff and the same call site wrapped them. What is new is that they are now a named contract, which is where the transport-specific framing gets locked in for whoever reads the symbol next (U-I1: a shared contract should be described in terms both consumers can hold).
Suggested fix: name them for what they describe rather than which transport noticed, e.g.
errClaudeEmptyResultFileanderrClaudeMissingResultFilewith messages like "result file is empty" / "no result file", and let each call site keep supplying its own transport prefix as both already do. Move the "only missing can mean the transport never ran the task" sentence to the branch at line 1460 that actually applies it.
Reviewer Coverage
go:implementation-tests— complete (constrained); skipped: none; constraints: nonearchitecture:solid-reviewer-agnostic— complete (constrained); inspected 1 assigned file (2 inspected across reviewers):internal/llmadapters/subprocess.go; skipped: none; constraints: A line-level diff was not available in this sandbox; new-vs-pre-existing attribution is inferred from the head file, the change map, and the settled threads. Could not run the project's build/test/lint here (command execution was restricted), so no red-check claims are made either way. Re-review at 2335eee. All three findings from the previous round are fixed in the head file: the unreachable launch-path branch is deleted (lines 366-373), the write-only fallback field is gone (lines 407-418), and the empty/missing result cases are separated by sentinels (lines 1456-1466, 1589-1... The earlier rounds' fixes still hold at this head: single task deadline (lines 382, 388-403, 440-445), sibling retry log path (lines 447, 468-479), winner-based SessionID (lines 420-432), result read before the session id (lines 1450-1458). internal/llmadapters/subprocess_test.go was outside the allowed file set and was not reviewed.security:code-auditor— complete (constrained); inspected 1 assigned file (2 inspected across reviewers):internal/llmadapters/subprocess.go; skipped: none; constraints: nonestructure:harness-engineering— complete (constrained); inspected 1 assigned file (2 inspected across reviewers):internal/llmadapters/subprocess.go; skipped: none; constraints: none
Inspected files (2)
internal/llmadapters/subprocess.gointernal/llmadapters/subprocess_test.go
0 PR discussion threads considered. 0 summarized; 0 resolved.
Completed in 2m 34s | $5.58 | claude-sonnet-5, claude-opus-5 | cr 0.10.294
| Field | Value |
|---|---|
| Model | claude-sonnet-5, claude-opus-5 |
| Reviewers | go:implementation-tests, architecture:solid-reviewer-agnostic, security:code-auditor, structure:harness-engineering |
| Engine | claude_cli · claude-sonnet-5, claude-opus-5 |
| Reviewed by | cr · monit-reviewer |
| Duration | 2m 34s wall · 4m 41s compute |
| Cost | $5.58 |
| Tokens | 86 in / 18.1k out |
Per-workstream usage
go:implementation-tests— claude-sonnet-5- In: 46
- Out: 9.4k
- Cache read: 4.2M
- Cache create: 187.0k
- Cost: $1.68
- Duration: 1m 59s
architecture:solid-reviewer-agnostic— claude-opus-5- In: 14
- Out: 4.9k
- Cache read: 1.1M
- Cache create: 172.3k
- Cost: $2.38
- Duration: 1m 20s
security:code-auditor— claude-sonnet-5- In: 8
- Out: 948
- Cache read: 352.1k
- Cache create: 98.0k
- Cost: $0.47
- Duration: 20s
structure:harness-engineering— claude-sonnet-5- In: 12
- Out: 2.3k
- Cache read: 735.5k
- Cache create: 132.2k
- Cost: $0.70
- Duration: 50s
orchestrator-rollup— claude-sonnet-5- In: 6
- Out: 466
- Cache read: 189.6k
- Cache create: 77.3k
- Cost: $0.35
- Duration: 11s
…e transport
Both transports read result files through readFirstNonEmptyFile, so sentinels
named and documented for the background one made the foreground path report
"foreground task completed without a result file: ...background job completed
without writing result file". They now describe the file ("no result file",
"result file is empty") and each call site supplies its own prefix, which is
what both already did. The rule about which case can mean the transport never
ran the task moved to the branch that applies it.
monit-reviewer
left a comment
monit-reviewer
left a comment
There was a problem hiding this comment.
Automated PR Review
Reviewed commit: 900c1ab92295
Profile: claude-monit-reviewer - Posting as: monit-reviewer
Summary
| Reviewer | Findings |
|---|---|
| go:implementation-tests | 1 |
| architecture:solid-reviewer-agnostic | 0 |
| security:code-auditor | 0 |
| structure:harness-engineering | 0 |
go:implementation-tests (1 finding)
Major - internal/llmadapters/subprocess_test.go:369
The one branch in waitForClaudeBGResult that actually returns ErrClaudeBGTransport for a missing-result job (subprocess.go:1460,
sessionID == "" && errors.Is(err, errClaudeMissingResultFile)) has no test. "missing result" (bg-missing-result, line 369) sets sessionId: "session-missing" in the helper state, so it takes the non-transport branch at subprocess.go:1468 and never exercises the retry condition; "empty result" carries the other sentinel; and TestSubprocessClaudeBackgroundResultWithoutSessionIsNotRetried (line 2258) covers the opposite combination (no session id, but a valid result). No case in either the table or a dedicated test drives a job that finishes with neither a session id nor a result file, which is the exact scenario this round's fix (900c1ab, and the thread at subprocess.go:1466 before it) was about: "the job left nothing behind, which is the only shape here that can mean the transport never ran the task." A regression that stopped marking this case as ErrClaudeBGTransport (or started marking bg-missing-result's case, which has a session id) would pass every existing test. Add a helper mode with state "done", no sessionId, and writeResult=false, and assert both errors.Is(err, ErrClaudeBGTransport) and assertClaudeLaunchCount(t, records, true). Note this path also falls through waitForClaudeBGSessionID's claudeBGSessionIDGrace (a hardcoded 10s const), so a naive test would block for the full grace window before failing closed; either inject/shorten the grace for this test or make it an adapter-configurable duration, which would also make the constant a properly testable seam per the reviewer's own preserve-testable-seams guidance.
Reviewer Coverage
go:implementation-tests— complete (constrained); skipped: none; constraints: nonearchitecture:solid-reviewer-agnostic— complete (constrained); inspected 1 assigned file (2 inspected across reviewers):internal/llmadapters/subprocess.go; skipped: none; constraints: A line-level diff was not available in this sandbox; new-vs-pre-existing attribution is inferred from the head file, the change map, and the settled threads. Also still intact: launch failure returned unchanged (366-373), single task deadline (382, 388-403, 440-445), sibling retry log path (447, 468-479), winner-based SessionID (414-432), result read before session id (1450-1468). Could not run the project's build/test/lint here (command execution was restricted), so no red-check claims are made either way. Not reported, as pre-existing and unchanged: the foreground wrapper at 645 reads "completed without a result file" even when the wrapped cause is an empty file. The base composed the same mismatch with different wording. Re-review at 900c1ab. No findings: every finding this reviewer raised across the earlier heads is fixed in the head file, and the remaining changed lines raise nothing new. Verified at this head: sentinels renamed to errClaudeEmptyResultFile / errClaudeMissingResultFile with file-describing messages (1590-1598), each call site supplying its own transport prefix (645, 1468), and errors.Is gating rather than message text (1460). internal/llmadapters/subprocess_test.go was outside the allowed file set and was not reviewed.security:code-auditor— complete (constrained); inspected 1 assigned file (2 inspected across reviewers):internal/llmadapters/subprocess.go; skipped: none; constraints: nonestructure:harness-engineering— complete (constrained); inspected 1 assigned file (2 inspected across reviewers):internal/llmadapters/subprocess.go; skipped: none; constraints: none
Inspected files (2)
internal/llmadapters/subprocess.gointernal/llmadapters/subprocess_test.go
0 PR discussion threads considered. 0 summarized; 0 resolved.
Completed in 1m 49s | $5.41 | claude-sonnet-5, claude-opus-5 | cr 0.10.294
| Field | Value |
|---|---|
| Model | claude-sonnet-5, claude-opus-5 |
| Reviewers | go:implementation-tests, architecture:solid-reviewer-agnostic, security:code-auditor, structure:harness-engineering |
| Engine | claude_cli · claude-sonnet-5, claude-opus-5 |
| Reviewed by | cr · monit-reviewer |
| Duration | 1m 49s wall · 3m 05s compute |
| Cost | $5.41 |
| Tokens | 56 in / 10.5k out |
Per-workstream usage
go:implementation-tests— claude-sonnet-5- In: 20
- Out: 5.1k
- Cache read: 2.0M
- Cache create: 206.5k
- Cost: $1.27
- Duration: 1m 11s
architecture:solid-reviewer-agnostic— claude-opus-5- In: 14
- Out: 3.0k
- Cache read: 1.2M
- Cache create: 190.0k
- Cost: $2.56
- Duration: 58s
security:code-auditor— claude-sonnet-5- In: 6
- Out: 651
- Cache read: 258.7k
- Cache create: 108.0k
- Cost: $0.49
- Duration: 12s
structure:harness-engineering— claude-sonnet-5- In: 10
- Out: 1.2k
- Cache read: 637.9k
- Cache create: 142.7k
- Cost: $0.71
- Duration: 30s
orchestrator-rollup— claude-sonnet-5- In: 6
- Out: 538
- Cache read: 200.5k
- Cache create: 82.9k
- Cost: $0.38
- Duration: 11s
The one branch that marks a background failure as ErrClaudeBGTransport had no test: the table's missing-result case carries a session id and takes the non-transport path, the empty-result case carries the other sentinel, and the result-without-session case is the opposite combination. A regression that stopped marking this case, or started marking one with a session id, passed everything. The new case is a job that finishes with neither a session id nor a result, and it asserts both the sentinel and that the retry was attempted. The session-id grace is an adapter field defaulting to the existing constant, so the test drives that path without waiting out a ten-second window. Verified the test fails with the branch disabled.
monit-reviewer
left a comment
monit-reviewer
left a comment
There was a problem hiding this comment.
Automated PR Review
Reviewed commit: 60ada627c9c7
Profile: claude-monit-reviewer - Posting as: monit-reviewer
Summary
| Reviewer | Findings |
|---|---|
| go:implementation-tests | 0 |
| architecture:solid-reviewer-agnostic | 1 |
| security:code-auditor | 0 |
| structure:harness-engineering | 0 |
architecture:solid-reviewer-agnostic (1 finding)
Nits - internal/llmadapters/subprocess.go:107
sessionIDGraceis the one adapter field no constructor sets.newSubprocessAdapter(lines 123-146) populates every other field, and this file already has a seam for exactly this kind of test-only injection:SubprocessOptions.commandArgsPrefixis unexported and threaded through at line 139. Adding the grace as a struct field written directly by tests routes around that seam, so the package now has two ways to inject a test value into the same adapter (U-D1: the dependency is reachable only by poking the struct rather than arriving through the constructor).The defaulting also differs from its nearest neighbor.
timeoutresolves zero todefaultLLMTaskTimeoutonce in the constructor (lines 132-135), so every reader sees a settled value;sessionIDGraceresolves at the use site (lines 1516-1519), so the field's stored value is not the value in effect. With one read site that is harmless, and it stops being harmless the first time a second reader forgets the fallback.Suggested fix: add an unexported
sessionIDGrace time.DurationtoSubprocessOptionsbesidecommandArgsPrefix, apply theclaudeBGSessionIDGracedefault innewSubprocessAdapterthe waytimeoutis applied, and havewaitForClaudeBGSessionIDreada.sessionIDGracedirectly.
Reviewer Coverage
go:implementation-tests— complete (constrained); skipped: none; constraints: nonearchitecture:solid-reviewer-agnostic— complete (constrained); inspected 1 assigned file (2 inspected across reviewers):internal/llmadapters/subprocess.go; skipped: none; constraints: A line-level diff was not available in this sandbox; new-vs-pre-existing attribution is inferred from the head file, the change map, and the settled threads. Could not run the project's build/test/lint here (command execution was restricted), so no red-check claims are made either way. Every finding this reviewer raised across the earlier heads remains fixed at this head. Re-review at 60ada62. The only production change since 900c1ab is the sessionIDGrace field (104-107) and its use-site default (1516-1519); everything else in the changed surface is unchanged and was verified in earlier rounds. Verified still intact: launch failure returned unchanged (366-377), single task deadline (386-407), sibling retry log path (451, 472-483), winner-based SessionID (418-436), result read before session id (1454-1472), file-describing result sentinels with per-caller prefixes (649, 1472). internal/llmadapters/subprocess_test.go was outside the allowed file set and was not reviewed, so the new bg-nothing-left-behind coverage is taken from the settled thread rather than verified.security:code-auditor— complete (constrained); inspected 1 assigned file (2 inspected across reviewers):internal/llmadapters/subprocess.go; skipped: none; constraints: nonestructure:harness-engineering— complete (constrained); inspected 1 assigned file (2 inspected across reviewers):internal/llmadapters/subprocess.go; skipped: none; constraints: none
Inspected files (2)
internal/llmadapters/subprocess.gointernal/llmadapters/subprocess_test.go
0 PR discussion threads considered. 0 summarized; 0 resolved.
Completed in 1m 36s | $5.76 | claude-sonnet-5, claude-opus-5 | cr 0.10.294
| Field | Value |
|---|---|
| Model | claude-sonnet-5, claude-opus-5 |
| Reviewers | go:implementation-tests, architecture:solid-reviewer-agnostic, security:code-auditor, structure:harness-engineering |
| Engine | claude_cli · claude-sonnet-5, claude-opus-5 |
| Reviewed by | cr · monit-reviewer |
| Duration | 1m 36s wall · 2m 40s compute |
| Cost | $5.76 |
| Tokens | 54 in / 8.8k out |
Per-workstream usage
go:implementation-tests— claude-sonnet-5- In: 22
- Out: 3.1k
- Cache read: 2.4M
- Cache create: 222.5k
- Cost: $1.39
- Duration: 49s
architecture:solid-reviewer-agnostic— claude-opus-5- In: 12
- Out: 3.5k
- Cache read: 1.1M
- Cache create: 207.9k
- Cost: $2.70
- Duration: 1m 04s
security:code-auditor— claude-sonnet-5- In: 6
- Out: 630
- Cache read: 278.8k
- Cache create: 118.1k
- Cost: $0.53
- Duration: 12s
structure:harness-engineering— claude-sonnet-5- In: 8
- Out: 1.0k
- Cache read: 518.1k
- Cache create: 154.0k
- Cost: $0.73
- Duration: 23s
orchestrator-rollup— claude-sonnet-5- In: 6
- Out: 442
- Cache read: 211.4k
- Cache create: 88.2k
- Cost: $0.40
- Duration: 9s
The field was the one adapter dependency written directly by tests, which routed around the unexported SubprocessOptions seam that commandArgsPrefix already uses, and it resolved its default at the use site rather than once in the constructor the way timeout does, so the stored value was not the value in effect. It is an unexported option now, defaulted in newSubprocessAdapter, and the wait reads the field directly.
monit-reviewer
left a comment
monit-reviewer
left a comment
There was a problem hiding this comment.
Automated PR Review
Reviewed commit: 5afb105ea4f7
Profile: claude-monit-reviewer - Posting as: monit-reviewer
Summary
| Reviewer | Findings |
|---|---|
| go:implementation-tests | 0 |
| architecture:solid-reviewer-agnostic | 0 |
| security:code-auditor | 0 |
| structure:harness-engineering | 0 |
Reviewer Coverage
go:implementation-tests— complete (constrained); skipped: none; constraints: nonearchitecture:solid-reviewer-agnostic— complete (constrained); inspected 1 assigned file (2 inspected across reviewers):internal/llmadapters/subprocess.go; skipped: none; constraints: A line-level diff was not available in this sandbox; new-vs-pre-existing attribution is inferred from the head file, the change map, and the settled threads. Could not run the project's build/test/lint here (command execution was restricted), so no red-check claims are made either way. Not reported: SubprocessOptions.sessionIDGrace treats a negative as "use the default" while the adjacent Timeout treats a negative as "disable the bound". The field is unexported and test-only, so no caller can reach the difference. Re-review at 5afb105. No findings: every finding this reviewer raised across the earlier heads is fixed in the head file, and the only change since 60ada62 resolves the last one. Verified still intact: launch failure returned unchanged (378-385), single task deadline (394, 400-415, 452-457), sibling retry log path (459, 480-491), winner-based SessionID (426-444), result read before session id with errors.Is gating (1462-1480). Verified the sessionIDGrace fix: unexported SubprocessOptions field beside commandArgsPrefix (44-48), default resolved once in newSubprocessAdapter like timeout (139-142, 152), and waitForClaudeBGSessionID reads a.sessionIDGrace directly (1524). internal/llmadapters/subprocess_test.go was outside the allowed file set and was not reviewed; the new coverage is taken from the settled threads rather than verified.security:code-auditor— complete (constrained); inspected 1 assigned file (2 inspected across reviewers):internal/llmadapters/subprocess.go; skipped: none; constraints: nonestructure:harness-engineering— complete (constrained); inspected 1 assigned file (2 inspected across reviewers):internal/llmadapters/subprocess.go; skipped: none; constraints: none
Inspected files (2)
internal/llmadapters/subprocess.gointernal/llmadapters/subprocess_test.go
0 PR discussion threads considered. 0 summarized; 0 resolved.
Completed in 1m 55s | $6.84 | claude-sonnet-5, claude-opus-5 | cr 0.10.294
| Field | Value |
|---|---|
| Model | claude-sonnet-5, claude-opus-5 |
| Reviewers | go:implementation-tests, architecture:solid-reviewer-agnostic, security:code-auditor, structure:harness-engineering |
| Engine | claude_cli · claude-sonnet-5, claude-opus-5 |
| Reviewed by | cr · monit-reviewer |
| Duration | 1m 55s wall · 3m 14s compute |
| Cost | $6.84 |
| Tokens | 72 in / 10.1k out |
Per-workstream usage
go:implementation-tests— claude-sonnet-5- In: 34
- Out: 4.9k
- Cache read: 4.1M
- Cache create: 249.3k
- Cost: $1.87
- Duration: 1m 17s
architecture:solid-reviewer-agnostic— claude-opus-5- In: 16
- Out: 2.9k
- Cache read: 1.6M
- Cache create: 227.8k
- Cost: $3.17
- Duration: 58s
security:code-auditor— claude-sonnet-5- In: 8
- Out: 818
- Cache read: 437.0k
- Cache create: 128.8k
- Cost: $0.61
- Duration: 19s
structure:harness-engineering— claude-sonnet-5- In: 8
- Out: 1.0k
- Cache read: 544.6k
- Cache create: 164.4k
- Cost: $0.78
- Duration: 29s
orchestrator-rollup— claude-sonnet-5- In: 6
- Out: 411
- Cache read: 221.5k
- Cache create: 92.9k
- Cost: $0.42
- Duration: 9s
2 participants
Why
The Claude job-service transport is the default for reviewer tasks, and
subprocess.goalready documents it as the fragile one:Observed failure, four consecutive runs of the same review: one reviewer's job entered
blockedbecause the prompt file it was told to read was gone.Its agent log held only the
backgrounded · <id>banner. Every other reviewer in the same run completed normally, and re-running reproduced it. Running the same review withCR_CLAUDE_FOREGROUND=1completed all reviewers on the first attempt.Losing one reviewer costs more than one reviewer's findings. A review that reports no findings because a reviewer never ran is not the same as a review that found nothing, and a caller gating on coverage cannot tell them apart from the outside, so a flaky job service withholds approval on a change that has nothing wrong with it.
What this changes
Background failures that mean the transport never ran the task are marked with
ErrClaudeBGTransport: the job blocked, stopped, failed, never registered an id, or completed without a session id. A task that fails that way is retried once on the foreground transport, which this file already describes as fully owned by cr.What it deliberately does not change
CR_CLAUDE_FOREGROUNDis unchanged and still selects foreground outright, without a background attempt first.Tests
make lintclean,go test ./...green apart fromTestPiRPCReviewerExtensionLoadsInInstalledPi, which fails on main here too (the locally installedpirejects--no-builtin-tools).Alternative considered
Flipping the default to foreground, which is what the existing comment argues for. That is a larger behavioral change and the background transport still has advantages this PR does not weigh, so this keeps the default and makes its failure recoverable instead.