[piekstra]

Summary

TestPiRPCReviewerImageProcessingBounds failed with tool exited null whenever the full suite ran on a loaded Mac. The cause is a test bound plus two real defects in how the reviewer extension handles a killed helper. No timeout was raised and nothing is skipped or retried.

Cause

The extension kills a tool helper with SIGKILL at its deadline. The test set that deadline to 500 ms and shared it with starting a Node helper process, which is not what the output subtest measures and only part of what deadline measures.

Evidence (temporary instrumentation, not committed)	Result
Who closed the child across 10 loaded runs: every cr_read that failed	19 of 19 SIGKILL, from the deadline abort in readImage
No added load, helper delayed 0 / 300 / 700 ms	10 of 10 pass / 10 of 10 pass / 10 of 10 fail with tool exited null
Helper round trip under 120 CPU spinners, deadline lifted (80 calls)	p50 848 ms, p90 1138 ms, max 1355 ms; 75 of 80 over 500 ms, 19 of 80 over 1 s

The failure message also exposed two production defects:

1. A helper killed at the deadline returned tool exited null. In readImage the deadline had fired, but the early return on a tool error skipped the mapping to Image read timed out; keep this file skipped, so a reviewer lost that instruction. Ordinary tools had the same unexplained text.
2. A helper that dies with its request unread, when the request is larger than a pipe buffer, raised an unhandled EPIPE on child.stdin. That terminates the whole Node host, not one tool call. Reproduced with a 4 MB request against both a hung helper and one that exits at once.

Fix

• runTool reports Tool request timed out or Tool request aborted when it killed the child, and names the signal for any other kill (tool exited SIGTERM).
• readImage maps a killed helper to its timed-out message once its deadline has fired.
• child.stdin has an error handler; close still reports the outcome.
• output and the reader deadline case hold the extension's timers. deadline expires them only from inside the reader, so it always exercises the reader abort. Neither depends on wall-clock time.
• New helper deadline case: a hung helper and the real timer. Being killed at any stage gives the same result, so there is no race.
• New TestPiRPCReviewerToolReportsKilledHelper: deadline, abort, external signal and unread request.
• TestPiRPCReviewerExtensionRejectsInvalidImageTransport had the same flaw with a 1 s bound, and also holds its timers now.

Evidence

Condition	Before	After
go test -count=1 ./..., no added load (1-minute load average at end of run: 12 to 52 before, 17 to 83 after)	5 of 5 runs failed	5 of 5 runs passed
Target test, 120 spinners	55 of 60 subtests failed (20 + 10 repetitions)	0 of 150 failed (50 repetitions)
Target test, 240 spinners	40 of 40 subtests failed (20 repetitions)	0 of 24 failed (8 repetitions)
RejectsInvalidImageTransport, 240 spinners	8 of 8 failed	0 of 8 failed
ToolReportsKilledHelper, 120 spinners	new test	0 of 200 failed (50 repetitions)

Each production change was reverted on its own and a test failed each time: the readImage mapping (helper deadline), the stdin handler (unread request), the kill reason (deadline, abort, unread request), and the signal text (external signal). Removing the output budget check fails output, and removing the timed-out text fails deadline and helper deadline.

golangci-lint reports 0 issues on the package. TestPiRPCReviewerExtensionForwardsBinaryViewsAfterDiff (5 s) and TestPiRPCReviewerHelperStaysInParentProcessGroup passed 8 of 8 at 240 spinners, so they are unchanged.

To reproduce, run CPU spinners alongside the package:

go test -c -tags keyring_nopassage -o llm.test ./internal/llmadapters
for i in $(seq 120); do yes >/dev/null & done
./llm.test -test.run '^TestPiRPCReviewerImageProcessingBounds$' -test.count=20
kill $(jobs -p)

[piekstra-dev]

Automated PR Review

Reviewed commit: 7a5f8b778db0
Profile: reviewer - Posting as: piekstra-dev

Summary

Reviewer	Findings
documentation:drift	0
go:implementation-tests	0
privacy:captured-data	0
go:implementation	0

Reviewer Coverage

Complete coverage details are retained in the local coverage.json artifact.

• documentation:drift — complete (constrained); scope: 1 file; examples: internal/llmadapters/pi_rpc.go; body-inspected: 1 file; examples: internal/llmadapters/pi_rpc.go; skipped: none; constraints: Searched markdown docs for the old and new error texts, SIGKILL, and image read timeouts; found no document describing these messages.
• go:implementation-tests — complete (broad); scope: 2 files; examples: internal/llmadapters/pi_rpc.go, internal/llmadapters/pi_rpc_extension_unix_test.go; body-inspected: 2 files; examples: internal/llmadapters/pi_rpc.go, internal/llmadapters/pi_rpc_extension_unix_test.go; skipped: none; constraints: Static review of the diff only; tests were not executed and Node behavior under load was not re-measured.
• privacy:captured-data — complete (constrained); scope: 2 files; examples: internal/llmadapters/pi_rpc.go, internal/llmadapters/pi_rpc_extension_unix_test.go; body-inspected: 2 files; examples: internal/llmadapters/pi_rpc.go, internal/llmadapters/pi_rpc_extension_unix_test.go; skipped: none; constraints: none
• go:implementation — complete (broad); scope: 1 file; examples: internal/llmadapters/pi_rpc.go; body-inspected: 1 file; examples: internal/llmadapters/pi_rpc.go; skipped: none; constraints: Reviewed the diff of internal/llmadapters/pi_rpc.go only (embedded Node extension source); did not execute tests or the timing reproduction. Test files were not in the assigned scope and were not inspected.

0 PR discussion threads considered. 0 summarized; 0 resolved.

---

Completed in 1m 07s | $0.42 | claude-sonnet-5-5 | cr 0.10.365

Field	Value
Model	claude-sonnet-5-5
Reviewers	documentation:drift, go:implementation-tests, privacy:captured-data, go:implementation
Engine	claude_cli · claude-sonnet-5-5
Reviewed by	cr · piekstra-dev
Duration	1m 07s wall · 1m 46s compute
Cost	$0.42
Tokens	44 in / 6.1k out

Per-workstream usage

• orchestrator-selection — claude-sonnet-5-5
  • In: 6
  • Out: 971
  • Cache read: 21.1k
  • Cache create: 12.9k
  • Cost: $0.06
  • Duration: 16s
• documentation:drift — claude-sonnet-5-5
  • In: 8
  • Out: 1.2k
  • Cache read: 37.5k
  • Cache create: 11.6k
  • Cost: $0.06
  • Duration: 13s
• go:implementation-tests — claude-sonnet-5-5
  • In: 8
  • Out: 1.5k
  • Cache read: 40.8k
  • Cache create: 15.4k
  • Cost: $0.08
  • Duration: 23s
• privacy:captured-data — claude-sonnet-5-5
  • In: 8
  • Out: 1.0k
  • Cache read: 43.1k
  • Cache create: 15.7k
  • Cost: $0.08
  • Duration: 20s
• go:implementation — claude-sonnet-5-5
  • In: 8
  • Out: 1.1k
  • Cache read: 37.0k
  • Cache create: 11.1k
  • Cost: $0.06
  • Duration: 17s
• orchestrator-rollup — claude-sonnet-5-5
  • In: 6
  • Out: 306
  • Cache read: 49.4k
  • Cache create: 2.4k
  • Cost: $0.08
  • Duration: 15s