fix: address PR #1 review findings (includes a kill(-1) guard now missing from main) - #2
Merged
Conversation
CodeRabbit review of #1 surfaced 13 findings; two were regressions from the prior review-fix commit. - stopProcessGroup passed an unvalidated manifest value to syscall.Kill(-pgid). Group id 1 means kill(-1, ...) — every process the operator can signal — and the identity check does not block it: pid 1 leads group 1, so only a start-time window stood in the way, and a boot-started worker satisfies it. Guarded in the predicate, the identity gate, the signal path, and manifest validation. - The direct run's Persist closure rode the cancellable context while completion used the detached one, so an interrupt dropped the unwinding events from the store. - Terminal phase exits (cancel, ceiling, send budget) skipped write boundary enforcement; two are agent-steerable, and a hook planted in the shared git dir outlives the attempt. - Prompt paths were read with IsLocal plus ReadFile, which follows symlinks the boundary permits; now opened under an OpenRoot. - diff_matches_claims passed vacuously when changed_files was absent. - rows.Err() unchecked in two claim loops; a truncated candidate list persists an empty claim the worker then replays. - enforceLimit could delete a concurrent in-flight clone; Result() raced the stream consumer; a manifest write failure dropped the attempt outcome; worktreeRegistered compared unresolved paths, so on macOS it always returned false.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe pull request hardens cancellation-aware persistence and server startup, engine write-boundary and prompt-path validation, CLI stream completion, worker lifecycle handling, process-group signaling, repository cache cleanup, and worktree registration. ChangesControl-plane lifecycle handling
Engine boundary and input validation
CLI stream completion
Worker state and workspace safety
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
The startup test asserted that resolving localhost consults the context. Linux's pure-Go resolver answers localhost from /etc/hosts without a context-aware lookup, so resolution legitimately succeeds on a dead context there while macOS's cgo path returns its error. Assert the bind, which honors the context on both.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/worker/repocache.go (1)
114-122: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winClose the window between
MkdirTempandclaimBuild.
os.MkdirTemppublishes the.clone-*directory on disk at line 114, but the claim is only recorded at line 118. A concurrentmaterializeLockedfor a different identity can runenforceLimitin that window. It then sees an unclaimed.clone-*directory and removes it. That is the exact failure this change targets, only with a smaller window.Create the directory and record the claim while holding
c.mutex.🔒️ Proposed fix
- temporary, err := os.MkdirTemp(c.root, ".clone-") - if err != nil { - return fmt.Errorf("create temporary clone directory: %w", err) - } - c.claimBuild(temporary) + temporary, err := c.newClaimedBuildDirectory() + if err != nil { + return err + } defer func() { c.releaseBuild(temporary) _ = os.RemoveAll(temporary) }()Add the helper next to
claimBuild:// newClaimedBuildDirectory creates a temporary clone directory and records the // claim under the same lock enforceLimit consults, so the directory is never // visible on disk as unclaimed. func (c *repoCache) newClaimedBuildDirectory() (string, error) { c.mutex.Lock() defer c.mutex.Unlock() temporary, err := os.MkdirTemp(c.root, ".clone-") if err != nil { return "", fmt.Errorf("create temporary clone directory: %w", err) } c.building[filepath.Base(temporary)] = true return temporary, nil }Note that
enforceLimitmust keep callingbuildInFlightwithout holdingc.mutex, which it does today.🤖 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/worker/repocache.go` around lines 114 - 122, Close the publication window in the temporary clone setup by adding a newClaimedBuildDirectory helper that holds c.mutex while calling os.MkdirTemp and recording the directory in c.building, then update the surrounding materialization flow to use it instead of separately calling MkdirTemp and claimBuild. Preserve the existing cleanup and error behavior, and leave enforceLimit’s buildInFlight usage unchanged.
🧹 Nitpick comments (3)
internal/engine/phase.go (1)
1325-1335: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winHandle deferred
Closeerrors explicitly.Wrap both calls in deferred closures with
_ = root.Close()and_ = file.Close()soerrcheckpasses. Go 1.26.4 providesos.OpenRootand(*os.Root).Open.🤖 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/engine/phase.go` around lines 1325 - 1335, Update the deferred cleanup in the prompt-reading flow around os.OpenRoot and root.Open so both Close calls use deferred closures that explicitly discard their returned errors with _ = root.Close() and _ = file.Close(), satisfying errcheck without changing the existing read behavior.Source: Linters/SAST tools
internal/worker/claiming.go (1)
235-246: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFix the garbled sentence in the comment.
Line 238-239 reads "a manifest we cannot write is a manifest disposal cannot trust." The clause is incomplete. State the rule directly.
📝 Proposed wording
- // terminal state is reported before returning, and the worktree is - // retained because a manifest we cannot write is a manifest disposal - // cannot trust. + // terminal state is reported before returning, and the worktree is + // retained because disposal cannot trust a manifest that this worker + // failed to write.🤖 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/worker/claiming.go` around lines 235 - 246, Fix the comment near completeAttempt to replace the garbled clause with a clear statement that the worktree is retained because a manifest that cannot be written cannot be trusted for disposal.internal/worker/claiming_test.go (1)
57-64: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSkip these tests when the process runs as root.
os.Chmod(attempts, 0o500)does not block writes for uid 0. Many CI containers run as root. In that case the manifest write succeeds,ClaimOncereturns no error, and both tests fail at thet.Fatal("a failed manifest write must still surface as an error")line for an environment reason, not a code defect. Add an explicit skip so the failure mode is clear.♻️ Proposed guard
func sealAttemptsDirectory(t *testing.T, dataDir string) { t.Helper() + if os.Geteuid() == 0 { + t.Skip("root ignores directory permissions, so the write path cannot be made to fail this way") + } attempts := filepath.Join(dataDir, "attempts")🤖 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/worker/claiming_test.go` around lines 57 - 64, Update sealAttemptsDirectory to detect whether the test process runs as root and call t.Skip with a clear reason before changing permissions when uid 0 is detected. Keep the existing chmod setup, cleanup, and test behavior unchanged for non-root users.
🤖 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 449-456: The Result flow around h.done, h.command.Wait, and
h.stopEverything must not depend on stdout EOF when a descendant inherits the
pipe. Record direct-process completion separately, trigger process-group
shutdown independently, and continue draining stdout before calling Cmd.Wait so
the terminal result is preserved; add a regression stub covering an
inherited-stdout child and asserting Result returns with the process group
terminated.
In `@internal/worker/reconcile_test.go`:
- Line 256: Update the test logic around the worktreePath extraction to assert
that raw["worktree_path"] exists and is a string, failing immediately with a
clear message when the field is missing or invalid; only then use the extracted
path for os.Stat.
In `@internal/worker/reconcile.go`:
- Around line 321-326: Update stopProcessGroup to validate that converting the
accepted groupID to int preserves its original int64 value before calling
syscall.Kill. Reject any value that fails this round-trip check, while retaining
the existing signallableProcessGroup validation and normal SIGTERM behavior for
valid IDs.
In `@internal/worker/worktree.go`:
- Around line 247-257: Update resolvedWorktreePath and worktreeRegistered to
propagate EvalSymlinks resolution failures instead of converting them into an
unmatched path and false result. Make removeWorktree distinguish a resolution
error from confirmed absence, so cleanup is not marked complete unless
worktreeRegistered proves the registration is gone.
---
Outside diff comments:
In `@internal/worker/repocache.go`:
- Around line 114-122: Close the publication window in the temporary clone setup
by adding a newClaimedBuildDirectory helper that holds c.mutex while calling
os.MkdirTemp and recording the directory in c.building, then update the
surrounding materialization flow to use it instead of separately calling
MkdirTemp and claimBuild. Preserve the existing cleanup and error behavior, and
leave enforceLimit’s buildInFlight usage unchanged.
---
Nitpick comments:
In `@internal/engine/phase.go`:
- Around line 1325-1335: Update the deferred cleanup in the prompt-reading flow
around os.OpenRoot and root.Open so both Close calls use deferred closures that
explicitly discard their returned errors with _ = root.Close() and _ =
file.Close(), satisfying errcheck without changing the existing read behavior.
In `@internal/worker/claiming_test.go`:
- Around line 57-64: Update sealAttemptsDirectory to detect whether the test
process runs as root and call t.Skip with a clear reason before changing
permissions when uid 0 is detected. Keep the existing chmod setup, cleanup, and
test behavior unchanged for non-root users.
In `@internal/worker/claiming.go`:
- Around line 235-246: Fix the comment near completeAttempt to replace the
garbled clause with a clear statement that the worktree is retained because a
manifest that cannot be written cannot be trusted for disposal.
🪄 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: 0b924a9e-5abc-4c38-896d-c1b0c9f7c42d
📒 Files selected for processing (19)
internal/controlplane/claim.gointernal/controlplane/embedded.gointernal/controlplane/embedded_test.gointernal/controlplane/server.gointernal/controlplane/store_test.gointernal/engine/gates.gointernal/engine/phase.gointernal/engine/phase_test.gointernal/runtime/claudecode/adapter.gointernal/runtime/claudecode/adapter_test.gointernal/worker/claiming.gointernal/worker/claiming_test.gointernal/worker/manifest.gointernal/worker/reconcile.gointernal/worker/reconcile_test.gointernal/worker/repocache.gointernal/worker/repocache_test.gointernal/worker/worktree.gointernal/worker/worktree_test.go
Comment on lines
+449
to
+456
| // Drain before Wait, never after: Cmd.Wait closes the read end of the | ||
| // stdout pipe as soon as the process exits, so waiting first can cut | ||
| // consume off mid-stream and lose the terminal `result` line still sitting | ||
| // in the pipe — reported downstream as "claude returned no terminal result | ||
| // event", a phase failure with no cause in the transcript. consume ends at | ||
| // EOF on its own, so this needs no help from Wait. | ||
| <-h.done | ||
| waitErr := h.command.Wait() |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- target outline ---'
ast-grep outline internal/runtime/claudecode/adapter.go --view compact || true
printf '%s\n' '--- target references ---'
rg -n -C 5 'h\.done|stopEverything|consume|command\.Wait|func .*Result|type .*handle|stdout|process group' internal/runtime/claudecode/adapter.go
printf '%s\n' '--- related files ---'
rg -n --glob '*.go' 'stopEverything|claudecode|no terminal result event|inherited stdout|Cmd\.Wait|command\.Wait' internal tests . 2>/dev/null | head -300
printf '%s\n' '--- candidate test files ---'
git ls-files | rg '(^|/)(.*claude.*|.*runtime.*test.*|.*adapter.*test.*)\.go$' | head -200Repository: StructuPath/jig
Length of output: 8597
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- adapter setup and handle lifecycle ---'
sed -n '1,340p' internal/runtime/claudecode/adapter.go
printf '%s\n' '--- consume and Result ---'
sed -n '320,510p' internal/runtime/claudecode/adapter.go
printf '%s\n' '--- relevant tests ---'
sed -n '1,500p' internal/runtime/claudecode/adapter_test.goRepository: StructuPath/jig
Length of output: 34074
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
import os
import select
import signal
import subprocess
import sys
import tempfile
import time
child_code = r'''
import os
import sys
import time
pid = os.fork()
if pid == 0:
# Keep the inherited stdout write end open after the direct process exits.
time.sleep(4)
os._exit(0)
os.write(1, b'{"type":"result","result":"terminal"}\n')
os._exit(0)
'''
proc = subprocess.Popen(
[sys.executable, "-c", child_code],
stdout=subprocess.PIPE,
stderr=subprocess.PIPE,
start_new_session=True,
)
line = proc.stdout.readline()
if line != b'{"type":"result","result":"terminal"}\n':
raise SystemExit(f"unexpected line: {line!r}")
# The direct process has exited, but EOF must not arrive while the descendant
# still owns the inherited stdout descriptor.
direct_exited = proc.poll() is not None
ready, _, _ = select.select([proc.stdout], [], [], 0.5)
eof_before_child_exit = bool(ready and proc.stdout.peek(1) == b"")
print(f"direct_exited={direct_exited}")
print(f"eof_before_child_exit={eof_before_child_exit}")
if not direct_exited or eof_before_child_exit:
raise SystemExit("inherited stdout did not keep the pipe open as expected")
# Group teardown is the operation that releases the inherited descriptor.
os.killpg(proc.pid, signal.SIGTERM)
try:
proc.wait(timeout=2)
except subprocess.TimeoutExpired:
os.killpg(proc.pid, signal.SIGKILL)
proc.wait(timeout=2)
print("group_teardown_completed=True")
PY
printf '%s\n' '--- local os/exec documentation ---'
go env GOROOT 2>/dev/null | xargs -r -I{} sh -c \
"grep -n -A12 -B4 'StdoutPipe' {}/src/os/exec/exec.go | head -80" || trueRepository: StructuPath/jig
Length of output: 257
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
import os
import select
import signal
import subprocess
import sys
import time
child_code = r'''
import os
import time
pid = os.fork()
if pid == 0:
time.sleep(4)
os._exit(0)
os.write(1, b'{"type":"result","result":"terminal"}\n')
os._exit(0)
'''
proc = subprocess.Popen(
[sys.executable, "-c", child_code],
stdout=subprocess.PIPE,
stderr=subprocess.PIPE,
start_new_session=True,
)
line = proc.stdout.readline()
if line != b'{"type":"result","result":"terminal"}\n':
raise SystemExit(f"unexpected line: {line!r}")
direct_exited = proc.poll() is not None
ready, _, _ = select.select([proc.stdout], [], [], 0.5)
eof_before_child_exit = bool(ready and proc.stdout.peek(1) == b"")
print(f"direct_exited={direct_exited}")
print(f"eof_before_child_exit={eof_before_child_exit}")
if not direct_exited or eof_before_child_exit:
raise SystemExit("inherited stdout did not keep the pipe open as expected")
os.killpg(proc.pid, signal.SIGTERM)
try:
proc.wait(timeout=2)
except subprocess.TimeoutExpired:
os.killpg(proc.pid, signal.SIGKILL)
proc.wait(timeout=2)
print("group_teardown_completed=True")
PY
printf '%s\n' '--- local os/exec documentation ---'
go env GOROOT 2>/dev/null | xargs -r -I{} sh -c \
"grep -n -A12 -B4 'StdoutPipe' {}/src/os/exec/exec.go | head -80" || trueRepository: StructuPath/jig
Length of output: 257
🏁 Script executed:
#!/bin/bash
set -u
python3 - <<'PY'
import os
import select
import signal
import subprocess
import sys
import tempfile
import time
with tempfile.TemporaryDirectory() as d:
marker = os.path.join(d, "parent-exited")
child_code = r'''
import os
import time
marker = os.environ["MARKER"]
pid = os.fork()
if pid == 0:
time.sleep(5)
os._exit(0)
with open(marker, "w") as f:
f.write("direct parent exited\n")
os.write(1, b'{"type":"result","result":"terminal"}\n')
os._exit(0)
'''
proc = subprocess.Popen(
[sys.executable, "-c", child_code],
stdout=subprocess.PIPE,
stderr=subprocess.PIPE,
env={**os.environ, "MARKER": marker},
start_new_session=True,
)
line = proc.stdout.readline()
if line != b'{"type":"result","result":"terminal"}\n':
raise SystemExit(f"unexpected line: {line!r}")
deadline = time.monotonic() + 2
while not os.path.exists(marker) and time.monotonic() < deadline:
time.sleep(0.01)
if not os.path.exists(marker):
raise SystemExit("direct parent did not exit")
# No EOF should be readable while the forked descendant owns stdout.
ready, _, _ = select.select([proc.stdout], [], [], 0.5)
data_before_teardown = os.read(proc.stdout.fileno(), 1) if ready else b""
print(f"direct_parent_exited=True")
print(f"pipe_ready_before_group_teardown={bool(ready)}")
print(f"read_before_group_teardown={data_before_teardown!r}")
if data_before_teardown == b"":
# select may report no readiness for an open pipe; that is also
# evidence that EOF has not arrived.
pass
elif data_before_teardown:
raise SystemExit("unexpected extra data before group teardown")
os.killpg(proc.pid, signal.SIGTERM)
proc.wait(timeout=2)
ready, _, _ = select.select([proc.stdout], [], [], 2)
data_after_teardown = os.read(proc.stdout.fileno(), 1) if ready else b""
print(f"pipe_ready_after_group_teardown={bool(ready)}")
print(f"read_after_group_teardown={data_after_teardown!r}")
if data_after_teardown != b"":
raise SystemExit("pipe did not reach EOF after group teardown")
PY
printf '%s\n' '--- local os/exec documentation ---'
if command -v go >/dev/null 2>&1; then
goroot="$(go env GOROOT)"
grep -n -A12 -B4 'StdoutPipe' "$goroot/src/os/exec/exec.go" | head -80
else
printf '%s\n' 'go is unavailable in the sandbox'
fiRepository: StructuPath/jig
Length of output: 2216
🏁 Script executed:
#!/bin/bash
set -u
python3 - <<'PY'
import os
import select
import signal
import subprocess
import sys
import tempfile
import time
with tempfile.TemporaryDirectory() as d:
marker = os.path.join(d, "parent-exited")
child_code = r'''
import os
import time
marker = os.environ["MARKER"]
pid = os.fork()
if pid == 0:
time.sleep(5)
os._exit(0)
with open(marker, "w") as f:
f.write("direct parent exited\n")
os.write(1, b'{"type":"result","result":"terminal"}\n')
os._exit(0)
'''
proc = subprocess.Popen(
[sys.executable, "-c", child_code],
stdout=subprocess.PIPE,
stderr=subprocess.PIPE,
env={**os.environ, "MARKER": marker},
start_new_session=True,
)
line = proc.stdout.readline()
if line != b'{"type":"result","result":"terminal"}\n':
raise SystemExit(f"unexpected line: {line!r}")
deadline = time.monotonic() + 2
while not os.path.exists(marker) and time.monotonic() < deadline:
time.sleep(0.01)
if not os.path.exists(marker):
raise SystemExit("direct parent did not exit")
ready, _, _ = select.select([proc.stdout], [], [], 0.5)
data_before_teardown = os.read(proc.stdout.fileno(), 1) if ready else b""
print(f"direct_parent_exited=True")
print(f"pipe_ready_before_group_teardown={bool(ready)}")
print(f"read_before_group_teardown={data_before_teardown!r}")
if data_before_teardown:
raise SystemExit("unexpected extra data before group teardown")
os.killpg(proc.pid, signal.SIGTERM)
proc.wait(timeout=2)
ready, _, _ = select.select([proc.stdout], [], [], 2)
data_after_teardown = os.read(proc.stdout.fileno(), 1) if ready else b""
print(f"pipe_ready_after_group_teardown={bool(ready)}")
print(f"read_after_group_teardown={data_after_teardown!r}")
if data_after_teardown != b"":
raise SystemExit("pipe did not reach EOF after group teardown")
PY
printf '%s\n' '--- local os/exec documentation ---'
if command -v go >/dev/null 2>&1; then
goroot="$(go env GOROOT)"
grep -n -A12 -B4 'StdoutPipe' "$goroot/src/os/exec/exec.go" | head -80
else
printf '%s\n' 'go is unavailable in the sandbox'
fiRepository: StructuPath/jig
Length of output: 2216
Decouple process-group shutdown from stdout EOF.
If the CLI exits while a descendant inherits stdout, consume cannot reach EOF. Result then blocks at line 455 and never calls h.stopEverything(0). Record direct-process completion and release the process group independently of stream EOF. Preserve terminal-output draining without calling Cmd.Wait before the reader finishes. Add a regression stub that spawns a child with inherited stdout, exits, and asserts that Result returns and the process group terminates.
🤖 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 449 - 456, The Result
flow around h.done, h.command.Wait, and h.stopEverything must not depend on
stdout EOF when a descendant inherits the pipe. Record direct-process completion
separately, trigger process-group shutdown independently, and continue draining
stdout before calling Cmd.Wait so the terminal result is preserved; add a
regression stub covering an inherited-stdout child and asserting Result returns
with the process group terminated.
| if err := json.Unmarshal(body, &raw); err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| worktreePath, _ := raw["worktree_path"].(string) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Fail the test when worktree_path is missing.
The discarded assertion result hides an absent or non-string field. worktreePath then holds "", and os.Stat("") fails at line 279 with the message "reconcile deleted the worktree behind an unreadable manifest". That message names the wrong cause.
💚 Proposed fix
- worktreePath, _ := raw["worktree_path"].(string)
+ worktreePath, ok := raw["worktree_path"].(string)
+ if !ok || worktreePath == "" {
+ t.Fatalf("manifest has no worktree_path to check: %v", raw["worktree_path"])
+ }📝 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
| worktreePath, _ := raw["worktree_path"].(string) | |
| worktreePath, ok := raw["worktree_path"].(string) | |
| if !ok || worktreePath == "" { | |
| t.Fatalf("manifest has no worktree_path to check: %v", raw["worktree_path"]) | |
| } |
🤖 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/worker/reconcile_test.go` at line 256, Update the test logic around
the worktreePath extraction to assert that raw["worktree_path"] exists and is a
string, failing immediately with a clear message when the field is missing or
invalid; only then use the extracted path for os.Stat.
Comment on lines
321
to
326
| func stopProcessGroup(groupID int64) { | ||
| if !signallableProcessGroup(groupID) { | ||
| return | ||
| } | ||
| pgid := int(groupID) | ||
| if err := syscall.Kill(-pgid, syscall.SIGTERM); err != nil { |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Also reject group IDs that do not survive the int conversion.
signallableProcessGroup validates the int64 value, but line 325 converts it with int(groupID). On a 32-bit build the conversion truncates. A corrupted manifest value such as 4294967298 passes the range gate and becomes group 2 at the syscall. The stated goal of this guard is that no bad integer reaches kill(2), so check the round trip.
🛡️ Proposed guard
func stopProcessGroup(groupID int64) {
if !signallableProcessGroup(groupID) {
return
}
pgid := int(groupID)
+ if int64(pgid) != groupID {
+ // The recorded value cannot be represented on this platform, so it
+ // cannot name a local process group.
+ return
+ }📝 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 stopProcessGroup(groupID int64) { | |
| if !signallableProcessGroup(groupID) { | |
| return | |
| } | |
| pgid := int(groupID) | |
| if err := syscall.Kill(-pgid, syscall.SIGTERM); err != nil { | |
| func stopProcessGroup(groupID int64) { | |
| if !signallableProcessGroup(groupID) { | |
| return | |
| } | |
| pgid := int(groupID) | |
| if int64(pgid) != groupID { | |
| // The recorded value cannot be represented on this platform, so it | |
| // cannot name a local process group. | |
| return | |
| } | |
| if err := syscall.Kill(-pgid, syscall.SIGTERM); err != nil { |
🤖 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/worker/reconcile.go` around lines 321 - 326, Update stopProcessGroup
to validate that converting the accepted groupID to int preserves its original
int64 value before calling syscall.Kill. Reject any value that fails this
round-trip check, while retaining the existing signallableProcessGroup
validation and normal SIGTERM behavior for valid IDs.
Comment on lines
+247
to
+257
| // resolvedWorktreePath canonicalizes a worktree path through its parent | ||
| // directory, so it still resolves after the worktree itself has been deleted | ||
| // — which is exactly when worktreeRegistered runs. | ||
| func resolvedWorktreePath(path string) string { | ||
| path = filepath.Clean(path) | ||
| parent, err := filepath.EvalSymlinks(filepath.Dir(path)) | ||
| if err != nil { | ||
| return path | ||
| } | ||
| return filepath.Join(parent, filepath.Base(path)) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Inspect how callers act on a false result from worktreeRegistered.
set -euo pipefail
rg -nP --type=go -C 12 '\bworktreeRegistered\s*\(' internal/workerRepository: StructuPath/jig
Length of output: 5642
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- worktree registration and removal logic ---'
sed -n '150,275p' internal/worker/worktree.go
printf '%s\n' '--- callers of worktree removal ---'
rg -nP --type=go -C 10 '\b(removeWorktree|worktreeRemove|RemoveWorktree)\s*\(' internalRepository: StructuPath/jig
Length of output: 8489
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
worktree = Path("internal/worker/worktree.go").read_text()
required = [
'want := resolvedWorktreePath(path)',
'if resolvedWorktreePath(absolute) == want {',
'return false, nil',
'if registered {',
'return errors.New("git reported removal success but the worktree registration remains")',
'return nil',
]
missing = [text for text in required if text not in worktree]
if missing:
raise SystemExit("missing expected control-flow text: " + repr(missing))
# Model the exact failure path: one canonicalization falls back to its
# unresolved input, while the other canonicalization succeeds.
want = "/var/tmp/worktrees/attempt-1"
listed = "/private/var/tmp/worktrees/attempt-1"
resolved_want = want # EvalSymlinks(parent) failed
resolved_listed = listed # EvalSymlinks(parent) succeeded
registered = resolved_listed == resolved_want
if registered:
raise SystemExit("model did not produce the claimed false result")
# The caller treats registered == false as successful removal.
if registered:
raise SystemExit("unexpected registered result")
caller_returns_error = False
if caller_returns_error:
raise SystemExit("model unexpectedly rejected false registration")
print("source control flow and one-sided resolution-failure model reproduce false registration")
PYRepository: StructuPath/jig
Length of output: 241
Return resolution failures from worktreeRegistered.
If EvalSymlinks fails for only one path, worktreeRegistered returns false even when Git still lists the worktree. removeWorktree treats false as successful removal and can mark cleanup complete without proving that the registration is gone.
🤖 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/worker/worktree.go` around lines 247 - 257, Update
resolvedWorktreePath and worktreeRegistered to propagate EvalSymlinks resolution
failures instead of converting them into an unmatched path and false result.
Make removeWorktree distinguish a resolution error from confirmed absence, so
cleanup is not marked complete unless worktreeRegistered proves the registration
is gone.
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.
Follow-up: PR review findings from #1
PR #1 merged at the commit immediately before this one, so these fixes did not make it into
main.maincurrently carries a machine-harming defect (first item below) — this branch closes it.CodeRabbit's review of #1 raised 13 findings. Two were regressions introduced by #1's own review-fix commit. Each was judged individually rather than applied on faith; verdicts and rejections are noted.
Critical — on
mainright nowstopProcessGrouppasses an unvalidated manifest value tosyscall.Kill(-pgid, …). A group id of1makes thatkill(-1, SIGTERM)— POSIX defines it as every process the caller may signal, i.e. the operator's entire session. A zeroed field makes itkill(0, …), jig's own group.CodeRabbit assumed the existing identity check incidentally blocked this. It does not:
ps -p 1 -o pgid=shows pid 1 leads group 1, so the group-match arm passes. The only barrier was the start-time window (CreatedAt-30s .. UpdatedAt+30s), which a boot-started worker under launchd/systemd satisfies. Reachable, not theoretical.Guarded in four places: a
signallableProcessGroup(id) = id >= 2predicate, the identity gate (refuses out-of-range before anypsinspection, still clears the flag),stopProcessGroupitself (deliberately duplicated, since that function is what negates the number), and manifest validation. Noted honestly:stopProcessGroup(1)cannot be called in a test — doing so is the harm — so the guard is asserted through the extracted predicate.Also fixed
Persistclosure rode the cancellable context while completion correctly used the detached one, so after Ctrl-C everyAppendEventsfailed for the rest of the attempt — dropping exactly the rollback and terminal events from theeventstable while they still reached the JSONL trace.deathrollback path, but those exits are terminal and never retried — a full rollback would destroy the authorized work the operator retained the worktree to inspect.IsLocal+ReadFile, which follows symlinks the boundary permits. Now opened under anos.OpenRoot. Pre-fix, the test attempt reachedaccepted_unpublishedwith a private key's contents in the system prompt.diff_matches_claimspassed vacuously whenchanged_fileswas absent, sincePassed()islen(Violations()) == 0and an empty check set is empty. Absent and non-list now fail; an empty list passes with an explicit check, because an empty list is an answer.rows.Err()unchecked in two claim loops. A mid-scan failure truncates the candidate list, so an empty claim is persisted under the request id and the worker replays that emptiness forEmptyClaimTTLwhile eligible work sits queued. Not practically testable without injecting a driver fault — fixed without a hollow test rather than inventing one.enforceLimitcould delete a concurrent in-flight clone (per-identity mutexes let two uncached repos materialize above capacity).Result()raced the stream consumer — reproduces deterministically 3/3 pre-fix as "claude returned no terminal result event".-racedoes not catch it; it is a file-descriptor lifetime bug, not a memory race.runninguntil the sweeper called it lost. Went beyond the suggested fix to record a terminal state on both branches.worktreeRegisteredcompared unresolved paths. macOS reports/private/varwhile the manifest holds/var, so it always returned false — the safety check that exists because "git reporting success is not the same as the worktree being gone" could never fire.NewServer/Starthave no non-test callers yet, so the signature change is in scope). Deviation: the context bounds the bind only; the sweeper runs onWithoutCancelso a request-scoped context cannot tear the server down.Testing
just checkandjust test-racegreen. Live smoke re-run against the real Claude Code CLI after these changes: stillaccepted_unpublishedin ~16s. New regression tests cover the interrupted-run persistence, both terminal-exit boundary cases, the prompt symlink refusal, the vacuous gate, the pgid guard, the in-flight clone, the adapter overlap, the manifest-failure outcome, and resolved-path comparison.Known residuals
Unchanged from #1 (the
phase.gosplit, cross-package helper dedup,git diff HEADon an empty repo, stat-based fingerprints for wholly-ignored directories,SQLITE_BUSYon concurrent first-run init,WorstCaseSendCountstill reading 6, trace redaction deferred to U8).Post-Deploy Monitoring & Validation
No production or runtime impact — local-first developer tool, no deployed surface. Validation is CI on both platforms plus the live smoke above.
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Security
Improvements