fix: truncation guard, adapter drain bound, and fail-closed path resolution - #3
Merged
Conversation
Second CodeRabbit pass on the review fixes. - The process-group guard narrowed int64 to int without checking the conversion, so a value above MaxInt32 could truncate onto 0 or 1 on a 32-bit build — back onto the syscall the guard exists to prevent. The round-trip check now lives in the predicate, so the signal path, the identity gate, and manifest validation all inherit it. - Result() waited for stdout EOF before reaping, so a descendant holding the stream blocked it indefinitely on the normal-exit path; stderr was a buffer rather than a file, so Cmd.Wait blocked for the same reason and waiting on exit was not an available fallback. jig now owns both pipes, waits on process exit, releases the group, then drains under a bound. - worktreeRegistered swallowed resolution failures into false, making 'could not tell' indistinguishable from 'not registered' in a fail-closed path. - A reconcile assertion read a missing manifest field as an empty one.
📝 WalkthroughWalkthroughThe change hardens Claude subprocess stream handling, validates process-group IDs before signaling, and makes worktree verification report path-resolution failures instead of treating them as absence. ChangesClaude stream lifecycle
Worker reconciliation safety
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ClaudeSubprocess
participant Result
participant ProcessGroup
participant StreamCapture
ClaudeSubprocess-->>Result: process exits with terminal result
Result->>ProcessGroup: stop process group
Result->>StreamCapture: drain stdout and stderr
StreamCapture-->>Result: captured output or timeout
Result-->>ClaudeSubprocess: return terminal result
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
internal/runtime/claudecode/adapter_test.go (1)
47-52: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider a companion stub mode for a descendant that leaves the process group.
sleep 60 &stays in the CLI's process group, sostopEverythingdelivers EOF and the drain returns immediately. This case never spends thestreamDrainGracewindow. The forced-close branch indrainStreamis therefore untested.A second mode that backgrounds the sleeper under
setsidwould exercise the timeout path and the reader close.♻️ Sketch of an escaped-group stub mode
orphan) # A descendant that inherits stdout and outlives the CLI — an MCP server, # a backgrounded tool process. The write end of the stream stays open # after the CLI itself has exited and delivered its terminal result. sleep 60 & printf '{"type":"result","result":"the CLI exited first","is_error":false,"session_id":"%s"}\n' "$SESSION" ;; + escapee) + # The same descendant, but it leaves the group. Group teardown cannot + # reach it, so only the drain grace window ends the capture. + setsid sleep 60 & + printf '{"type":"result","result":"the CLI exited first","is_error":false,"session_id":"%s"}\n' "$SESSION" ;;Note that
setsidis not present on every platform the suite runs on. Verify availability before adopting this.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/runtime/claudecode/adapter_test.go` around lines 47 - 52, Add a companion stub mode alongside the existing orphan mode that backgrounds the sleeper in a separate process group, using setsid only when it is available and preserving portability when it is not. Ensure the escaped descendant keeps the stream open after CLI exit so stopEverything cannot deliver EOF, causing drainStream to exercise streamDrainGrace and its forced reader-close path. Update the relevant test coverage to invoke this mode and verify the timeout behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/runtime/claudecode/adapter.go`:
- Around line 552-563: Update claudeHandle.drainStream to wait for both h.done
and h.stderrDone within the existing streamDrainGrace timeout before closing
stdoutReader and stderrReader. Preserve the final completion waits after
closing, while ensuring stderr is not forcibly closed until its buffered output
has drained or the shared grace period expires.
- Around line 504-509: Update the Kill lifecycle around stopEverything and
drainStream so it also closes the owned stdoutReader and stderrReader when
Result is not called. Add or reuse an idempotent cleanup path shared by Kill and
drainStream, ensuring repeated cleanup is safe and the existing stream-draining
behavior remains intact.
---
Nitpick comments:
In `@internal/runtime/claudecode/adapter_test.go`:
- Around line 47-52: Add a companion stub mode alongside the existing orphan
mode that backgrounds the sleeper in a separate process group, using setsid only
when it is available and preserving portability when it is not. Ensure the
escaped descendant keeps the stream open after CLI exit so stopEverything cannot
deliver EOF, causing drainStream to exercise streamDrainGrace and its forced
reader-close path. Update the relevant test coverage to invoke this mode and
verify the timeout behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 73628ae6-fbe6-4ef2-bf21-6582d30f3f32
📒 Files selected for processing (6)
internal/runtime/claudecode/adapter.gointernal/runtime/claudecode/adapter_test.gointernal/worker/reconcile.gointernal/worker/reconcile_test.gointernal/worker/worktree.gointernal/worker/worktree_test.go
Comment on lines
504
to
+509
| waitErr := h.command.Wait() | ||
| // The CLI is gone; release the group. The anchor ignores TERM by design, | ||
| // so this is the one place it dies. | ||
| // so this is the one place it dies. This is also what frees the inherited | ||
| // descriptor: any descendant still holding stdout is in this group. | ||
| h.stopEverything(0) | ||
| h.drainStream() |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect the handle lifecycle methods around the changed shutdown path.
ast-grep outline internal/runtime/claudecode/adapter.go --items all
# Show stopEverything, Kill, Stop, and stopGroup implementations.
ast-grep run --pattern $'func (h *claudeHandle) $NAME($$$) {
$$$
}' --lang go internal/runtime/claudecode/adapter.go
# Confirm every other Wait/Close site on the command and owned readers.
rg -nP -C4 '\b(command\.Wait|stdoutReader|stderrReader|stopEverything|stopGroup)\b' internal/runtime/claudecode/Repository: StructuPath/jig
Length of output: 20027
🏁 Script executed:
#!/bin/bash
# Determine whether the runtime contract requires Result after StartOrContinue,
# and inspect call sites that can terminate a handle without calling Result.
rg -nP -C5 '\btype\s+(Handle|Runtime|Result)\b|\bResult\(\)\s*\(runtime\.Result|\.Kill\(\)|\.Result\(\)' internal --glob '*.go'Repository: StructuPath/jig
Length of output: 15681
Ensure Kill() releases owned stream readers
stopEverything only stops the process group and closes the watchdog. drainStream is the only function that closes stdoutReader and stderrReader. A caller that invokes Kill() without Result() therefore leaks both descriptors. Add an idempotent cleanup path for this lifecycle.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/runtime/claudecode/adapter.go` around lines 504 - 509, Update the
Kill lifecycle around stopEverything and drainStream so it also closes the owned
stdoutReader and stderrReader when Result is not called. Add or reuse an
idempotent cleanup path shared by Kill and drainStream, ensuring repeated
cleanup is safe and the existing stream-draining behavior remains intact.
Comment on lines
+552
to
+563
| func (h *claudeHandle) drainStream() { | ||
| timer := time.NewTimer(streamDrainGrace) | ||
| defer timer.Stop() | ||
| select { | ||
| case <-h.done: | ||
| case <-timer.C: | ||
| } | ||
| _ = h.stdoutReader.Close() | ||
| _ = h.stderrReader.Close() | ||
| <-h.done | ||
| <-h.stderrDone | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Wait for stderrDone as well before forcing the readers closed.
The select at Lines 555-558 only observes h.done. h.done closes when stdout capture ends. Stderr capture can still be draining buffered bytes at that moment. Line 560 then closes stderrReader under the reader, and io.Copy returns ErrFileClosed with the remaining tail lost.
The tail feeds the claude exited: %w: %s diagnostic at Line 522, so the loss lands in the failure path that needs the output. Wait for both completion channels under the same grace bound.
🐛 Proposed fix to bound both streams together
func (h *claudeHandle) drainStream() {
timer := time.NewTimer(streamDrainGrace)
defer timer.Stop()
- select {
- case <-h.done:
- case <-timer.C:
- }
+ captured := make(chan struct{})
+ go func() {
+ defer close(captured)
+ <-h.done
+ <-h.stderrDone
+ }()
+ select {
+ case <-captured:
+ case <-timer.C:
+ }
_ = h.stdoutReader.Close()
_ = h.stderrReader.Close()
<-h.done
<-h.stderrDone
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func (h *claudeHandle) drainStream() { | |
| timer := time.NewTimer(streamDrainGrace) | |
| defer timer.Stop() | |
| select { | |
| case <-h.done: | |
| case <-timer.C: | |
| } | |
| _ = h.stdoutReader.Close() | |
| _ = h.stderrReader.Close() | |
| <-h.done | |
| <-h.stderrDone | |
| } | |
| func (h *claudeHandle) drainStream() { | |
| timer := time.NewTimer(streamDrainGrace) | |
| defer timer.Stop() | |
| captured := make(chan struct{}) | |
| go func() { | |
| defer close(captured) | |
| <-h.done | |
| <-h.stderrDone | |
| }() | |
| select { | |
| case <-captured: | |
| case <-timer.C: | |
| } | |
| _ = h.stdoutReader.Close() | |
| _ = h.stderrReader.Close() | |
| <-h.done | |
| <-h.stderrDone | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/runtime/claudecode/adapter.go` around lines 552 - 563, Update
claudeHandle.drainStream to wait for both h.done and h.stderrDone within the
existing streamDrainGrace timeout before closing stdoutReader and stderrReader.
Preserve the final completion waits after closing, while ensuring stderr is not
forcibly closed until its buffered output has drained or the shared grace period
expires.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Round-two review fixes that missed the #2 merge by one commit. Lower stakes than #2 — the
kill(-1)guard itself landed there.int64tointwithout checking the conversion. A value aboveMaxInt32truncates on a 32-bit build, and a truncated value can land on 0 or 1 — back onto the syscall the guard exists to prevent. The round-trip check now lives in the predicate, so the signal path, the identity gate, and manifest validation all inherit it. Only reachable on 32-bit builds; jig ships amd64/arm64.Result()could block forever on the normal-exit path. It waited for stdout EOF before reaping, so a descendant inheriting the stream held it open indefinitely — and because stderr was a buffer rather than a file,Cmd.Waitblocked for the same reason, so "wait on process exit instead" was not an available fallback. jig now owns both pipes, waits on exit, releases the group, then drains under a 5s bound. Reproduced: the old ordering fails at 20s, the new one returns in 0.03s.worktreeRegisteredswallowed resolution failures intofalse, making "could not tell" indistinguishable from "not registered" in a fail-closed path.just checkandjust test-racegreen; live smoke against the real CLI re-run after the adapter rework.Post-Deploy Monitoring & Validation
No production or runtime impact — local-first developer tool, no deployed surface.
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests