An unsafe session could report fewer refusals than it made - #762
Conversation
…it ends the turn The policy check runs on a goroutine of its own, and it is the one thing that ends a turn without being the reader. It terminated the worker, waited for the worker to go, and then asked the turn what its refusals were. But the refusals Codex puts on its JSON stream are the reader's to record, and a worker that is gone is not a stream that has been read: a turn ended at the instant the worker died hands back a result short of the refusals the ledger goes on to hold. For an ending whose whole point is to report what an unsafe session refused, that is the wrong answer. The verdict now gives the reader the worker's output — the same wait Close makes, now shared — before finishing the turn. In almost every run the reader has by then ended the turn itself, as unsafe and with every refusal; what is left is the turn it did not end. Closing a session under a turn dropped them outright. A worker that has stopped reading its input leaves the prompt's write blocked, and closing the session fails that write, so the prompt's own writer ends the turn — with an empty result, over a turn that had refusals on it. It carries them now, as every other ending does. Two tests hold these, both by ordering rather than by waiting: the ledger is held over its first write, so the reader is stopped on the first of two refusals with the second unread, and the ending under test lands while it is there. The first fails 25 runs of 25 without the change, the second 10 of 10. The comments that claimed a turn's result carries what the ledger carries said it of the stream as well as the stderr, and it was true only of the stderr. They now say which is which, and where an ending that is not the reader's has to wait.
There was a problem hiding this comment.
🟡 Changes recommended
A blocked prompt writer can still race the unsafe path and return partial refusals with the wrong error.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Improves Codex refusal reporting when sessions terminate asynchronously.
Changes:
- Waits for worker output before completing unsafe turns.
- Preserves refusals on writer-ended turns.
- Adds deterministic concurrency regressions.
[!TIP]
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or rungh pr ready --undo.
Click "Ready for review" or rungh pr readyto reengage.
File summaries
| File | Description |
|---|---|
internal/connector/driver/codex/codex.go |
Coordinates reader completion and refusal results. |
internal/connector/driver/codex/codex_test.go |
Tests unsafe and close ordering. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Ending the worker closes the worker's stdin, and a prompt still blocked writing that stdin — a worker that stopped reading its input while it went on writing to its stream — wakes on that close and finishes the turn itself. It reports the session as ended, over a session already known to be unsafe, with whatever refusals the reader had reached. An unsafe session is reported as unsafe (invariant 3), so that is the wrong answer twice over, and giving the reader time to finish the stream first only widened the window it had to be wrong in. The verdict now takes the turn before it touches the worker. A turn an ending has claimed stays the session's — a refusal read while it settles still lands on it, which is why the claim is a flag and not a removal — and every other ending leaves it alone. That leaves the prompt's writer as the only ending that is not the reader's and is not claimed against. It needs a worker that has stopped reading its input, and it can still be short of a refusal the reader has not reached; it no longer reports over an unsafe verdict, which was the part that mattered. The combined case is held by a test: a deaf worker under an unsafe policy, with the ledger held so the reader is stopped on the first of two refusals. Without the claim it fails 20 runs of 20, reporting the session as ended and carrying one refusal of the two.
There was a problem hiding this comment.
🟡 Changes recommended
Worker stdout can still close before the slower refusal recorder finishes draining it.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Balanced
There was a problem hiding this comment.
🟡 Changes recommended
The new API breaks non-Unix compilation, and the reader-state race can still discard unread output.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 2
- Review effort level: Balanced
There was a problem hiding this comment.
🟡 Changes recommended
Repeated stop requests can prematurely end draining, and the delayed-event fixture can bypass its synchronization condition.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
internal/connector/driver/codex/fake_test.go:205
- If
LateAfternever appears, this loop times out and still emitsLateEvents. That lets the ordering tests continue—and potentially pass—without ever establishing the synchronization point they claim to test. Make the fake fail when the trigger deadline expires instead of publishing the events anyway.
- Files reviewed: 6/6 changed files
- Comments generated: 1
- Review effort level: Balanced
There was a problem hiding this comment.
🟡 Changes recommended
A first-stop race can still prematurely end output draining, and its regression tests do not reliably establish their claimed ordering.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (1)
internal/connector/driver/driver_test.go:414
- The sleeps do not guarantee that the goroutine has opened its drain window. If it starts late, the line is queued before
Readand the test can pass even when a redundant stop incorrectly resets the deadline; under load, either sleep can also overshoot the 50 ms window and make the test fail spuriously. Synchronize on the reader opening its window, then issue the second stop and write without relying on scheduler timing.
time.Sleep(drainWindow / 10)
out.stop()
time.Sleep(drainWindow / 10)
_, err = writeEnd.Write([]byte("still in the window\n"))
- Files reviewed: 6/6 changed files
- Comments generated: 2
- Review effort level: Balanced
There was a problem hiding this comment.
🟡 Changes recommended
Cancellation and continuous escaped output can still leave turn completion or shutdown blocked indefinitely.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (2)
Previously missed (1) — in code that hasn't changed since the last review.
internal/connector/driver/worker.go:279
- This says the waiter ends the reader with
CloseStdout, but the new protocol usesStopReading;CloseStdoutis explicitly destructive and reserved for after reading has ended. Naming the destructive method here misdocuments the ownership contract callers must follow.
internal/connector/driver/codex/codex.go:639
- This second wait is unbounded, but
output.Readstarts a freshdrainWindowafter every successful read. An escaped descendant that continuously writes stdout therefore prevents any empty window and can holdClose(or an unsafe verdict) forever, regressing the bounded shutdown guarantee this path handles. Add an absolute post-stop bound or another mechanism that terminates ongoing descendant output without cutting off a reader that is processing already-read worker data.
s.worker.StopReading()
<-s.readerEnd
- Files reviewed: 6/6 changed files
- Comments generated: 1
- Review effort level: Balanced
There was a problem hiding this comment.
🟡 Changes recommended
The stop budget can expire during ledger processing and still discard unread worker refusals.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 1
- Review effort level: Balanced
There was a problem hiding this comment.
🟡 Changes recommended
Post-stop processing remains effectively unbounded, and several new concurrency tests have uncontrolled startup or failure paths.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (2)
Previously missed (1) — in code that hasn't changed since the last review.
internal/connector/driver/codex/codex_test.go:1246
require.NoErroris unsafe in this helper goroutine: on a filesystem error it callsFailNow, which exits only this goroutine beforeledger.releaseis closed. The reader andgot := <-answersthen remain blocked until the global test timeout, hiding the original failure. Report the write error to the test goroutine and always release the ledger.
internal/connector/driver/codex/codex_test.go:1285
- This test has the same deaf-worker startup race: the fake may emit and record the refusal before the asynchronous
Prompthas installed its turn. In that schedule the refusal is never added tot.refusals, so the test either flakes or fails to exercise the claimed writer-ending path. Gate the fake's events until the test has observedsession.turn != nil.
answers := make(chan answer, 1)
go func() {
result, err := s.Prompt(context.Background(), strings.Repeat("Event 1. ", 200_000))
answers <- answer{result, err}
- Files reviewed: 6/6 changed files
- Comments generated: 2
- Review effort level: Balanced
Both tests let the fake say its refusals at once, while the prompt was still installing the turn on another goroutine. A refusal read before there is a turn is attached to no turn — refused only puts it on one that exists — so the test could be asking about something that never happened, and reaching for the turn to watch it could find nothing there at all. The fake holds them behind a barrier now, and the test releases it only once it has seen the turn in flight. The barrier fails loudly if its trigger never comes, rather than saying the events anyway: a barrier that degrades into a wait is a test that passes on timing while claiming to prove an ordering, which is the thing this harness is for catching. Both still fail 10 runs of 10 against main with the barriers in place.
7587a73 to
34a0717
Compare
A Codex session stopped for running under a policy it was not asked to run under is supposed to report what it refused before it was stopped — that is the whole point of that ending. It could hand back a turn with fewer refusals than the ledger holds, and in one case with none at all.
Found under the last fix in Four driver failures on CI: a crash in the connector, and three checks that read too early, and reported rather than fixed there, because it is a change to when a turn finishes and not a flake. Originally tracked in Main is red: five tests failing across the drivers and intake.
What was wrong
The policy check runs on a goroutine of its own. It is the one thing that ends a turn without being the reader — every other ending is the reader finishing a turn after it has parsed the stream up to that point. It terminated the worker, waited for the worker to go, and then asked the turn what its refusals were. But the refusals Codex puts on its JSON stream are the reader's to record, and a worker that is gone is not a stream that has been read.
Ending the worker is also what wakes another ending. Terminating closes the worker's stdin, and a prompt still blocked writing it — a worker that stopped reading its input while it went on writing — wakes and finishes the turn itself, reporting the session as ended over a session already known to be unsafe. An unsafe session is reported as unsafe (invariant 3), so that was wrong twice over.
And a turn ended that way handed back an empty result, discarding refusals the turn already held. That one was not a race: the result was constructed empty.
What changes
The verdict claims the turn before it touches the worker, so every other ending finds it claimed and leaves it alone. The claim is a flag rather than taking the turn out of the session, because a refusal read while the turn settles still has to land on it. Then it waits for the reader before asking the turn what it refused — the same wait
Closealready made, now shared.A turn ended by the prompt's writer carries the refusals it has, like every other ending.
Every path that can end a turn
turn.completedturn.failedAn ordinary cancellation and a lost worker are not separate entries: once the prompt is written that goroutine is gone, so both are finished by the reader. The two that remain need a worker that stopped reading its input while still writing, so the write is still blocked when a cancel or a close fails it. Neither reports over an unsafe verdict now, and neither hands back an empty result over a turn with refusals; each can still be short of a refusal the reader has not reached. Left alone: making them wait changes which goroutine finishes the turn and so what its error says, which is worth deciding on its own.
What is not here, and why
Underneath all of this sits a second question — who owns the worker's output pipe, and until when — and it is not settled by this change. The read end is closed two seconds after the worker is terminated, whatever the reader is doing, while a refusal's write is allowed ten seconds on that same goroutine; a refusal arriving in that gap is thrown away, not merely missing from a result.
I had a fix for it here and have taken it out. Every way of bounding it that keeps ingestion and ledger work on one goroutine fails one way or the other: bound the wall clock and a busy reader loses refusals, bound only the waiting and a descendant writing without end holds
Closeopen forever. Those are not two bugs to balance, they are one structure — the pipe read and the ledger write on the same goroutine — making the bound impossible to place. The real answer is to separate them, and that is a change to the driver's shape rather than a fix to this ending.So it is carded rather than carried here, as The worker's output pipe is closed on a clock, and a refusal can be lost with it, with the work done so far parked as patches on
pipe-ownership-parked. Shipping a shutdown that can hang would be a worse trade than leaving a reporting gap the ledger already covers durably.Evidence
Every behaviour change red first. The tests make their ordering rather than waiting for one: the fake holds its refusals behind a barrier the test releases only once it has seen the turn in flight, and the ledger is held over its first write, so the reader is provably stopped on the first of two refusals with the second still unread. The barrier fails loudly if its trigger never comes, rather than saying the events anyway.
go build ./...,go vet ./...,gofmt -l .,golangci-lint run --build-tags dev ./...andBASECAMP_NO_KEYRING=1 go test -tags dev ./...are all clean, as isgo test -tags dev -race ./internal/connector/..., and so areGOOS=windowsandGOOS=darwinbuilds.