feat(review-leg): put the build in front of the reviewer (#613) - #614
Merged
Conversation
PR #610 ran EIGHT review rounds with two independent reviewers while CI validate was red the entire time. Both reviewers ran the test suites directly and correctly reported them green — the guard that was failing was one nobody was asked about, and the merge gate is what finally caught it. That is "a gate step discovered mid-merge", which #593 Rung 1's stop condition names as a failure in so many words. Both reviewers then prescribed the same fix independently, and it is not more diff scrutiny: Seven rounds audited the diff; zero audited the build. So the launcher states the build where the reviewer reads, ahead of the model's own output, with the sentence that makes it binding: a certification verdict issued over a build that is not green, or not determinable, is only valid if it says so and says why. PREFLIGHT, NEVER A GATE A red build is REPORTED and the leg proceeds. That is deliberate and it is the harder half of the design: a leg must stay launchable against known-red CI when that is the point, and a preflight that can refuse to launch is a second way for a review not to happen — the exact failure #590 exists to prevent. Every probe failure mode is absorbed the same way: no gh, no auth, no network, no PR for this branch each record UNKNOWN, and the review runs. The probe gets `< /dev/null` for the same reason the exec line does. A preflight that can hang would have reintroduced #590 in the one place nobody would think to look. WHAT THE SUITE CAUGHT THAT REVIEW WOULD NOT HAVE The first version wrapped the probe in `timeout 30`. This machine has neither `timeout` nor `gtimeout` — they are GNU coreutils, not stock macOS. Every probe would have reported UNKNOWN, forever, silently, on the maintainer's own laptop: this issue's exact failure mode, reintroduced by its own fix. It is now used when present and skipped when not. Found by a test written RED before the code, not by a reviewer. That is the whole argument for the four assertions. test-run-review-leg 12 -> 16 rows, RED first on all four. shellcheck clean.
Round 1 returned NOT CERTIFIED 6/10 with one blocker, and it was a real reintroduction of the failure this launcher exists to prevent. THE CLAIM WAS MINE AND IT WAS WRONG The probe fell back to running `gh` bare when neither `timeout` nor `gtimeout` was present — stock macOS, this machine included — and a comment justified it: "gh carries its own network timeouts." I never checked. The reviewer did, and cited the source: gh 2.92 configures no overall HTTP timeout (cli/cli api/http_client.go, go-gh pkg/api/client_options.go). A stalled request blocks before `exec codex` ever runs. That is #590 — a review leg waited on instead of owned — reintroduced by the preflight added to prevent a different failure. The suite missed it because its stub always exited. So the deadline is now this script's own and depends on nothing. The probe runs in the background writing its exit status to a marker file; a bounded poll abandons it and records UNKNOWN. A marker rather than `kill -0` on the pid, because a background job of this shell stays a zombie until waited on and `kill -0` would report it alive after it finished; `wait -n` would do it but needs bash 4 and macOS ships 3.2. A SECOND DEFECT, FOUND WHILE FIXING THE FIRST The probe subshell inherits `set -e`. A non-zero `gh` — which is precisely what a RED BUILD is — killed it before it wrote its marker. Every red build would have looked stalled, burned the full deadline, and reported "abandoned": the one case this feature exists to report, reported wrong. Found by the red-build row regressing, not by reading the code back. `set +e` inside the subshell, and a comment saying why it is load-bearing. THE RED WAS THE LOUDEST KIND Running the new stalled-probe row against the unfixed launcher hung the test suite itself for 120 seconds. The defect reproduced rather than argued. The reviewer also answered the question the prompt asked it: the CI STATUS block is "useful, not noise... prominent, concise, includes the failing check's raw output, and correctly precedes the review verdict." test-run-review-leg 16 -> 20 rows, all green. shellcheck clean.
3 tasks
Round 2 returned NOT CERTIFIED 6/10 with one blocker, and it falsified a second unverified claim of mine in as many rounds. The timeout path killed CI_PROBE_PID. That is the SUBSHELL, not `gh`. Killing it left `gh` running, reparented to PID 1. My own known_limits had said the orphan "exits on its own" — I never checked, and a genuinely stalled request never does. The reviewer ran the real 120-second fixture and watched both the probe and its child survive the leg that started them. Repeated timeouts would leak processes, descriptors and connections until a later leg could not launch: the failure this launcher exists to prevent, arriving by a third route. THE FIX IS `set -m`, AND IT IS LOAD-BEARING Without job control a backgrounded subshell shares this shell's process group, so the only thing addressable by pid is the subshell itself. With `set -m` it leads its own group, and the timeout path can TERM then KILL the NEGATIVE pid — subshell, `gh`, and anything `gh` spawned — then `wait` to reap. If a future edit drops `set -m`, the negative-pid kill silently degrades to killing the subshell alone and the leak returns. That is now in known_limits, and the suite's new row is what would catch it. THE ROW PROVES DEATH, NOT ABSENCE The stub records its own pid; the test asserts that pid is gone afterwards. Run against the unfixed launcher it names the survivor by number. Asserting the leg finished quickly would have passed on the broken version — the leg did finish, that was never the defect. THREE ROUNDS, THREE CLAIMS I DID NOT CHECK Round 1: "gh carries its own network timeouts" — false, gh sets none. Round 2: the red-build path, where `set -e` killed the probe before it could report — caught by my own suite. Round 3: "the orphan exits on its own" — false. Each was a sentence written to justify a shortcut. Filed as #617. test-run-review-leg 20 -> 21 rows, all green. shellcheck clean.
Round 3 split 10/10 and 9/10-at-90%. The second reviewer ran its own stall
repro rather than reading the diff, and found two things neither the first
reviewer nor the suite had. Neither meets the blocking bar; both are defects in
the feature this PR exists to deliver, and 90% is below the merge gate's 95%.
THE NOTICE LANDED IN THE BLOCK IT WAS MAKING READABLE
On the abandoned-probe path, bash announces the kill:
./scripts/run-review-leg.sh: line 193: 34945 Terminated: 15 ( set +e; gh …
That goes to the reviewer's own output file, inside the CI STATUS block,
exposing this script's internal command line. The whole point of the block is
that a reviewer reads it first; shipping shell noise into it undercuts that.
`disown` after `set +m`. The group kill still works — it addresses the process
group, not the job table — and the existing `wait ... || true` absorbs the
disowned-job error.
THE SUITE WAS NOT HERMETIC, AND SAID NOTHING ABOUT IT
The `gh` stub was installed beside the tests that drive it, in section 7. Every
test before that ran the launcher against the REAL `gh`: live API calls from a
unit suite, with behaviour depending on the network and on whether the current
branch happens to have a PR. Tests 1-6 were measuring something other than what
they claimed, and passed anyway.
Stub hoisted above test 1.
Worth naming the pattern: this is the third defect on this PR found by running
the thing rather than reading it, and the second found only because a reviewer
built its own repro instead of trusting the suite. The suite is the artifact
that says whether the code works, and it was itself wrong about its own
isolation.
test-run-review-leg 21/0, hermetic throughout. shellcheck clean.
Round 4 cleared at 96%, above the gate bar, with one note the reviewer chose to
defer: the `disown` fix it had just verified carried no regression assertion.
Delete `disown` and the suite stayed 21/21 green, because nothing asserted on
the reviewer-facing output. The fix was guarded by a human running a repro.
Taken rather than deferred. A fix with no test is the pattern this repo's own
rules exist to stop, and the other reviewer's clearance was already stranded by
round 4's commit, so the round was owed whatever else changed — the assertion
costs one row on a leg that had to run anyway.
PROVEN BY MUTATION, not by reading:
with `disown` 22 passed, 0 failed
`disown` removed 21 passed, 1 failed <- exactly the new row
restored 22 passed, 0 failed
That is the difference between a test and a comment. The row greps the leg's
own output file for a shell job notice, which is what leaked: bash announcing
"Terminated: 15 ( set +e; gh pr checks …" inside the CI STATUS block, exposing
the launcher's internals in the one block this PR exists to make readable.
Also fixes the reviewer's cosmetic note: a comment introducing the gh stub as
though it were defined below it, 130 lines after the stub moved to the top of
the file to make the earlier tests hermetic.
test-run-review-leg 21 -> 22 rows. shellcheck clean.
Owner
Author
|
CROSS-MODEL-CLEARANCE {"confidence": 99, "reviewer": "sol", "sha": "7ed0aa7f2df851b0a0079898ddf2fb361166b65d", "verdict": "YES"}Round 5. CERTIFIED — 10/10 — confidence 99%. No findings.
|
Owner
Author
|
CROSS-MODEL-CLEARANCE {"confidence": 97, "reviewer": "fable", "sha": "7ed0aa7f2df851b0a0079898ddf2fb361166b65d", "verdict": "YES"}Round 5, independent leg. CERTIFIED — 9/10 — confidence 97%. Delta touches exactly one file ( Mutation reproduced by hand, not taken from the submitter's numbers: HEAD 22/0 → CI
|
5 tasks
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.
Closes #613.
The failure this closes
PR #610 ran eight review rounds with two independent reviewers while CI
validatewas red the entire time. Both ran the suites directly and correctly reported them green — the failing guard was one nobody was asked about.scripts/merge-pr.shcaught it at the merge.That is "a gate step discovered mid-merge", which #593 Rung 1's stop condition names in so many words. Both reviewers prescribed the same fix independently:
Both priority rulings then made this Rung 1 item 3, ahead of #521, as closing an observed stop-condition failure rather than discretionary re-planning.
What it does
The launcher writes a
## CI STATUSblock into the leg's output file before exec'ing codex, so the build is stated where the reviewer reads and ahead of the model's own output. The block carries the contract sentence: a certification verdict issued over a build that is not green, or not determinable, is only valid if it says so and says why.Preflight, never a gate
A red build is reported and the leg proceeds. Deliberate, and the harder half of the design: a leg must stay launchable against known-red CI when that is the point, and a preflight that can refuse to launch is a second way for a review not to happen — the failure #590 exists to prevent. Every probe failure mode (no
gh, no auth, no network, no PR for the branch) records UNKNOWN and the review runs. The probe gets< /dev/nullfor the same reason the exec line does.What the suite caught that review would not have
The first version wrapped the probe in
timeout 30. This machine has neithertimeoutnorgtimeout— GNU coreutils, not stock macOS. Every probe would have reported UNKNOWN, forever, silently, on the maintainer's own laptop: this issue's exact failure mode reintroduced by its own fix. Found by a test written RED before the code.No new guard
tests/test-workflow-triggers.shalready catches the drift that broke #610 and always did. The gap was invocation, not coverage — so this adds no guard, and modifies two existing surfaces (the launcher, its suite). That keeps it inside #593's "no new guard or tooling surface" standing ruling.Verification
tests/test-run-review-leg.sh16/0 (12 → 16 rows), RED first on all four new assertions