requesting-review: split the review job out of codex-companion, runtime fail-closed, engine script retired - #55
requesting-review: split the review job out of codex-companion, runtime fail-closed, engine script retired#55SSFSKIM wants to merge 33 commits into
Conversation
…nel inside the engine The big-diff routing doctrine shipped in codex-companion (v7.37.2 panel, 2026-08-04 reviews.md size rule) but the reviewing-prs loop never adopted it: the worker still followed the 2026-07-28 multilens protocol — manual 1–4 parallel single-agent runs (the panel's precursor) — so a big PR got a sweep + hand-rolled lens runs instead of the panel (observed live, ida-solution PR obra#1255). - review-engine.sh: a plain run measures the merge-base diff and at the panel threshold (~20+ files / ~2k changed lines) runs the bundled code-review panel (sweep + diff-derived scalpels + binding verifier) through the workflow verb, behind the unchanged --base/--out contract. CODEX_REVIEW_PANEL=auto|always|never overrides the size gate. A lensed invocation stays adversarial-review and never routes. - render-panel-findings.mjs: flattens the panel's stdout JSON into the findings text the worker reads at JOIN; verdict `interrupted` exits 4 so the protocol's existing retry → ENGINE-UNAVAILABLE path owns it; raw panel JSON stays beside --out for the review trail. - SKILL.md / operation-manual: the manual run-count judgment and lens authoring retire — one engine run per round, diff-size scaling is the engine's. Risk surfaces now feed audit/triage scrutiny (their lens role moved into the panel's own deriver). - runner-setup.md: manifest paragraph also states surfaces never move merge authority (the ida-solution obra#1148 stale-manifest lesson). - test-review-engine.sh: modernized — it still stubbed `codex` argv from the pre-companion engine and has been red since 89eec7e ('node not found'); now stubs the companion layer via REVIEW_COMPANION_DIR and covers both routes, the auto threshold, and the interrupted verdict.
…h workflow leaves + doc/test hardening External codex review (gpt-5.6-sol) + the panel's own partial-evidence findings on this branch, both independently flagged the same P1: on the panel route, per-worker app-server stderr is buffered by the client and dropped on exit 0, so the RTM_NEWADDR/fs-sandbox markers never reach <out>.events.log and the fail-closed guard (ida-worker-1 incident, 2026-08-09) is blind. Fix: workflow engine hooks forward marker lines from every leaf's buffered stderr to emit (-> [workflow] stderr -> the events log), before the leaf's own success check. Mock gains stderrLine; engine-hooks test covers it. Also from the round: - operation-manual pieces table still described the retired 1-4x manual fan-out — now states the engine-internal panel routing. - SKILL.md: worker may force the panel (CODEX_REVIEW_PANEL=always) on a sub-threshold diff concentrated on declared risk surfaces — restores the scrutiny-escalation judgment the manual fan-out removal took away. - test repo sets commit.gpgsign=false so host git policy cannot break the hermetic suite. Not fixed, accepted: artifact truncation on failed runs (rc-gated caller contract never reads them) and route-time size measurement racing a moving HEAD (the loop reviews a detached worktree; the panel pins and interrupts on drift — proven live by this branch's own dogfood run).
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 226b10b75a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| 2. From the worktree root, start the round's ONE engine run | ||
| IN THE BACKGROUND (round N uses findings-rN.txt; the empty lens |
There was a problem hiding this comment.
This replaces the dispatched worker’s judged 1–4-run behavior with one automatically routed engine run, but the validation only exercises the engine mechanically; the documented live dogfood ended as interrupted, with its clean run still pending. Without a before/after pressure test of the actual skill consumer, there is no evidence that workers reliably follow the new panel-forcing, JOIN, and review-trail behavior or that this behavioral rewrite preserves review outcomes. Complete and report that evaluation before shipping the protocol change. CLAUDE.mdL34-L39
Useful? React with 👍 / 👎.
| if [ -z "$lens" ]; then | ||
| case "${CODEX_REVIEW_PANEL:-auto}" in | ||
| always) panel=1 ;; |
There was a problem hiding this comment.
When CODEX_SANDBOX is set and the outer Codex sandbox blocks nested shell probes, this still auto-routes large diffs—and permits always to route any diff—to the panel. Unlike the native sweep, the panel’s lens deriver and binding verifier are generic agent turns that must run git diff and re-inspect the repository themselves; with their shell unavailable they can derive and verify from no code, yet the fail-closed marker check is explicitly disabled for CODEX_SANDBOX, allowing an ungrounded panel verdict to exit 0. Keep nested runs on the native review route, or fail the panel when these worker probes are unavailable.
Useful? React with 👍 / 👎.
…-path sandbox forwarding, honest lane rendering The clean dogfood panel run on this branch (sweep + 4 scalpels + verifier, gpt-5.6-sol) returned verdict incorrect with 4 confirmed findings on the change itself; this round applies three of them: - render-panel-findings.mjs fails CLOSED: malformed shapes (findings not an array, malformed entries, verdict/findings inconsistency) exit 4 instead of rendering an empty-findings file the worker would read as clean; participation in the verdict line now comes from coverage (what actually ran), never the configured lens list, so a dead lane can't be rendered as a participant in the review trail. - workflow engine: leafCall's retry and fail paths forward sandbox markers from the error text — a dying app-server's protocol error carries the buffered stderr, and dropping it hid exactly the diagnostic the fail-closed guard greps for. Mock writes stderrLine before die (fs.writeSync) so a dying server leaves the marker behind. - tests: malformed-result and die-with-marker cases; stub panel JSON carries coverage so lane rendering is exercised. Accepted, not fixed (4th finding): the stderr snapshot race — a marker written in the child's final moments can miss the buffered copy read before close. The guard targets sustained sandbox failure (markers on every probe), so a last-instant miss cannot produce the 22-run false-clean shape; a real fix means assembling results after client close, filed in the PR discussion.
Live dogfood — two real panel runs through the modified engine (
|
…engine retirement
Milestones 1-3 of the requesting-review ExecPlan: - lib/sandbox.mjs: shared fail-closed guard (buffered-stderr markers, terminal, CODEX_SANDBOX stands down); wired into workflow engine leaves, review/adversarial verb branches, and with-effort.mjs (which owns the socket path's stderr and exits 3 on a marker) - new skills/requesting-review: owns routing doctrine (single review vs adversarial vs code-review panel), invocations, output contracts; codex-companion slimmed (reviews.md removed, panel section pointed) - reviewing-prs: review-engine.sh, render-panel-findings.mjs and their test suite deleted; the worker routes per requesting-review from an env-preamble command template; dispatcher binds COMPANION_DIR; interrupted panel retries once; operator docs updated - tests: engine-hooks sandbox cases now assert fail-closed; fake codex gains sandbox-broken behavior and -c tolerance; 4 new runtime cases; dispatch/entrypoint suites updated — all green
execplan exit gate, SDD final review, writing-plans and architecting plan reviews now invoke requesting-review; review-bench's codex engine runs the with-effort single-review path directly; stale review-engine.sh mentions scrubbed from orchestrating-daemons comment and bench README.
…he panel's base-only target
…l-closed guarantee
…for the sandbox marker
… is executable The per-round engine home was created by a `mktemp -d` inside the background review shell and named only by that shell's `eng_home` variable. The prose telling the worker to remove it "once its findings are read" was unexecutable: at JOIN — a separate step in a shell that no longer exists — the worker has no handle on the path, and an early failure, the 45-minute hung-engine kill, or the ENGINE-UNAVAILABLE exit skipped the instruction entirely. Create the home under `<review-tmp>` instead. Step 1 already mandates removing that directory before the turn ends (a park excepted), so one cleanup rule now reclaims every round's engine state on every exit path, with no cross-step handle to carry. Findings are written to `<review-tmp>` by redirection and never to the engine home, so nothing downstream loses an artifact.
…s a shell word
START ENGINE's two commands interpolate `{{BASE_REF}}` — a branch name
the dispatcher renders from the PR, which the worker cannot sanitize
because it never sees the unexpanded form. Git accepts refs containing a
single quote, `$`, or a backtick, so a base branch named e.g.
`release/'$(id)'` closes the panel's single-quoted `--args` JSON and runs
a command substitution, or breaks the JSON before Node parses it. The
loop runs unattended, which is what makes an otherwise-unlikely ref name
worth closing.
State the constraint where the commands are, with the hard stop the
protocol already uses for an unusable base: a ref that is not plain is
never pasted into a shell — the round does not start and the ticket
parks needs-human naming it. No quoting apparatus; the worker checks the
one value it was handed.
…tage path START ENGINE step 4 already spends the panel's one retry on an `interrupted` verdict — doperpowers:requesting-review's contract is "retry once, then treat the round as an outage". ENGINE FALLBACK then listed a twice-interrupted panel among the transient failures worth "retry twice with a short backoff", which permitted two further panel executions (more if each attempt re-entered step 4) at roughly 20 minutes and ~8 workers apiece. Drop the already-retried panel out of the generic retry list and say where it goes instead. The generic retry stays as-is for the transient failures it is right for — codex missing, auth, API errors.
…h's codex path The bench used to invoke the (now retired) review-engine.sh, which set up a throwaway CODEX_HOME with auth.json symlinked over, an isolated CLAUDE_PLUGIN_DATA, a TLS trust-anchor file bundle, and the code-mode host path a nested codex cannot resolve. Its replacement called with-effort.mjs bare, so a nested or read-only host — exactly where those fallbacks matter — could fail auth or state writes, while the README claimed the bench runs the loop's single-review path. Reproduce the live loop's START ENGINE preamble around the codex invocation (no new isolation invented), remove the engine home in the existing EXIT cleanup, and say in the README what the codex path now matches. The retired script's fail-closed sandbox check needs no restoring here: it now lives inside with-effort.mjs.
The Milestone 4 consumer repoint moved architecting's plan review from codex-companion's adversarial-review verb to doperpowers:requesting-review, leaving this assert pinning a literal the skill no longer contains. Repoint it, and pin the writing-plans side of the same sentence so the drift the assert exists to prevent is guarded from both ends.
Moving the engine home inside <review-tmp> made the turn-end removal reclaim it, but the needs-human carve-out preserves that whole directory — and observation mode parks needs-human on success, so codex session state and the symlink to the real auth.json survived the most common successful path with no reaper. Scope the carve-out: the boards stay, the engine homes go by glob on every exit.
The fallback retried the runtime's sandbox-unavailable rejection twice, though the guard throws terminal precisely because a blocked sandbox does not heal between attempts — two wasted panel rounds before the outage is reported. And exempting only a twice-INTERRUPTED panel left a panel whose step 4 retry died with a nonzero exit falling into the generic budget, so one interrupted round could still buy two more panels. Generic retry-twice stays for codex missing, auth, and transient API errors.
…he verdict The panel promotes to interrupted only when a lost lane would otherwise have made the answer correct (or when the sweep itself died); a lost lane beside a confirmed finding keeps incorrect and appends a coverage-partial note. The flat "a lane was lost" reading told a caller to retry rounds the panel never interrupted, and hid where partial coverage is reported.
A leaf whose app-server died carries the child's buffered stderr in the protocol error. That path only forwarded the sandbox marker as a diagnostic and then took its transport retry, so a second attempt that happened to succeed was journaled as a result even though the sandbox was already observed broken. Reuse guardSandbox there, as every other leaf site does — CODEX_SANDBOX still stands the guard down.
This reverts commit 1b06ac6. That change assumed a leaf's protocol error is the app-server child's buffered stderr, the machine channel the fail-closed guard is scoped to. It is not exclusively that: a rejected request quotes back the command the model asked to run, so a leaf reviewing this repository can put a marker in the error text just by reading its own test fixtures. That is what happened on the first live panel round with the change in — a command-policy rejection carrying `bwrap: ... RTM_NEWADDR` out of tests/codex-companion/ killed the scalpel-1 lane on a host whose sandbox was fine, and the round returned partial coverage. Restore the emit-only diagnostic and its transport retry, and record at both the call site and the test why this channel stays emit-only. The guard keeps its five `*.stderr` sites, all of which read cleanCodexStderr(client.stderr).
Review trail — automated review worker
|
| round | verdict | coverage | findings | outcome |
|---|---|---|---|---|
| 1 | incorrect |
sweep + 4 scalpels, all ok | 14 | wave 1 (8 items) |
| 2 | incorrect |
all ok | 8 | wave 2 (4 items) |
| 3 | incorrect |
all ok | 8 | wave 3 (4 items) — incl. a red test |
| 4 | incorrect |
all ok | 7 | wave 4 (4 items) |
| 5 | incorrect |
scalpel-1 dead, partial | 7 | at round cap; see the correction below |
Round 3's first attempt was killed by an external SIGTERM ~14 minutes in with no verdict written; it was retried per the fallback's budget and the retry completed normally.
Compliance audit — SKIPPED (ticketless PR)
ISSUE_NUMBER binds none and the PR closes no issue, so there is no canonical issue body to audit against and no [gate] pass or Architect handoff to anchor timestamps on. No PROTOCOL BLOCKER or SPEC FINDING class can apply here. Board writes are suppressed for the same reason, which is why the deferred findings below are inline rather than on the standing tech-debt issue (#26 was bound; move them there if you prefer).
The correction I have to lead with
Wave 4 introduced a regression, and I accepted it. Round 5 caught it live, and it is now reverted.
1b06ac69 changed the workflow engine so that a sandbox-failure marker found in a dead app-server's protocol error became a terminal failure instead of a diagnostic. It looked right, the tests passed, and I graded it PASS. On the very next panel round the scalpel-1 lane died — not because any sandbox was broken, but because the lane composed a shell command quoting this repository's own test fixtures (RTM_NEWADDR appears nine times in tests/codex-companion/test-engine-hooks.mjs), codex's command policy rejected the command, and the rejection error embeds the model-authored command text. The guard matched a string a model had innocently quoted.
lib/sandbox.mjs warns about precisely this in its own header — "model-authored channels … can quote marker strings innocently" — and the fixer, re-verifying independently, found the structural reason my reasoning missed: lib/app-server.mjs:314 builds an error from this.stderr (machine channel), but :165 rejects a pending RPC with arbitrary server text. The throw path's error is a union of the two and can never be treated as machine-emitted.
Evidence across rounds, same repo and same panel: rounds 1–4 (original emit-only behaviour) produced 0 sandbox diagnostics and lost 0 lanes; round 5 produced 3 and lost one. 79a69dad reverts it. The only surviving difference from what you wrote is a comment at that line recording why it must stay emit-only, so the next agent does not repeat my mistake.
This is also why round 5's coverage is partial. Its findings are still verifier-confirmed and are triaged below; its completeness claim is not.
What the waves fixed — 20 items, all graded and pushed
Wave 1 — d58d85a5..689fed97
- P1 START ENGINE's prose promised "an explicit code-mode host path" the command template never set. Four lanes flagged it; your ExecPlan required it carried over verbatim from the deleted engine (lines 57, 89).
82793c52 - P1 RE-REVIEW mandated "a fresh
--outfile"; neither engine command accepts--out— it was the retired script's flag, and the nativereviewverb rejects trailing text, so a worker following the step literally would break.9c7a11bb - P1 "Target selection (all shapes)" documented
--scope working-treefor the panel, which is base-only.548828a6 - P1 The fail-closed guarantee was stated without its
CODEX_SANDBOXexception.a831ba90 - P2 "Every invocation carries the shared env contract" contradicted codex-companion's rule that
setupmust omit the dead endpoint — following the new skill literally makessetupmisreport a valid login as unauthenticated.034cb0ab - P2 The per-round temporary
CODEX_HOMEwas never removed (the retired script had atrap … EXIT). Flagged by all five lanes.0ad54533 - P2
infra/worker-host/README.mdstill told operators to run the deletedreview-engine.sh.4c7f1903 - P2
with-effort.mjsmarker-tested only newline-completed lines, so a marker arriving as the child's last unterminated write stayed buffered.689fed97
Wave 2 — 689fed97..69aae205
- Wave 1's engine-home fix did not hold:
eng_homeis a shell variable local to the background command, so at JOIN the worker has no handle on the path it was told to remove. I had to record it manually myself to execute the cleanup at all. Re-waved; the home now lives inside<review-tmp>, which one existing rule already reclaims — and the fixer correctly rejected my suggestedtrap, since a trap does not run when the shell is killed.4862ed8b - The twice-interrupted panel came out of the generic retry-twice budget.
a6144f89 - The base ref is rendered into a single-quoted JSON shell word; git accepts refs with quotes and
$, and this loop runs unattended.19b416bd - The benchmark's codex path lost the isolated environment the retired script supplied while its README claimed production parity.
69aae205
Wave 3 — 69aae205..8b81b162
- RED TEST.
tests/implementing/test-protocol-content.shexited 1 on this branch: the Milestone 4 consumer repoint changed architecting's call-site, and that suite asserts the old literal. It appears in neither the PR's Validation list nor the ExecPlan's Concrete Steps, which is why nobody ran it. The assertion was repointed (not deleted) and a companion assert added so the drift it guards is pinned from both ends.68e59e7e - Wave 2's own fix regressed:
<review-tmp>is preserved on a needs-human park, and observation-mode success is a park — so on the most common successful path the engine home and its symlink to the realauth.jsonpersisted.675bd269 - ENGINE FALLBACK still retried the terminal sandbox rejection twice.
e8e12b75 requesting-reviewoverstated when the panel returnsinterrupted.8b81b162
Wave 4 — 8b81b162..27a585fd (plus the revert)
- An interrupted attempt's confirmed findings had no rule carrying them into triage, though the panel attaches them deliberately as partial evidence.
dad53436 - Wave 3's lost-lane wording was unqualified; a dead sweep does overturn
incorrect.b4b75aa4 - The operator manual still promised retry-twice for every engine failure.
27a585fd 1b06ac69— reverted, see above.
Grading: every commit touches only its item's file plus that item's test; none mixes items; no board file or attribution line rides the range. Skill assertions that could be pinned were, each confirmed failing against the pre-fix text. Two claims I re-verified independently rather than taking on trust — with-effort's new test (isolated copy: pre-fix exits 0, a false clean) and the red test above.
Deferred — valid, not blocking (LOG)
- [P1] The fail-closed guarantee is incomplete on the broker path.
BrokerCodexAppServerClientnever populates stderr (app-server.mjs:64sets""; only the direct client appends at:302), and the broker forwards RPC traffic only, so a review served over a live broker reaches the guard with an empty string. Flagged in all five rounds. Unreachable through the documented contract —requesting-reviewmandates a dead endpoint, which forces the direct path, and the workflow verb always setsdisableBroker— but the PR's claim that "now every consumer has it" is not yet true. Closing it is either broker-transport plumbing or a decision to fail closed when the guard cannot observe its channel; that is a design fork, not a reviewer's call. - [P1] Direct-path stderr is snapshotted before the transport closes.
runAppServerReview/runAppServerTurncopyclient.stderrinside the callback; stdout completion and stderr are independent pipes. Same class as the "stderr snapshot race" you already recorded as accepted-not-fixed, and I agree with your reasoning: the guard targets sustained failure, so a last-instant miss cannot reproduce the 22-run incident shape. - [P1]
with-effortdecides at the verb's exit without awaiting the private app-server's stderr close. Wave 1 fixed the buffered-fragment half; I deliberately scoped out the await, because an unconditional wait risks hanging a production engine call and this is the same accepted-risk class. Flagged in rounds 2, 3, 4 and 5. - [P2] The base-ref allow-list is ordered too late. Wave 2 put the check in START ENGINE, but ORIENT already runs
git diff --stat origin/{{BASE_REF}}...HEADunquoted before it. My fix, incompletely placed; the wave cap was spent by the time round 5 surfaced it. - [P2] The outage branch does not preserve partial findings. Wave 4 promised an interrupted attempt's confirmed findings reach TRIAGE, but a failed retry routes straight to the outage comment and
ENGINE-UNAVAILABLE, which never reaches TRIAGE. Also my fix, also caught after the cap. - [P2] Command templates leave
{{COMPANION_DIR}}, the findings redirections, and the cleanup target unquoted — a path with whitespace or glob characters splits. - [P2] The engine home still leaks if the worker itself dies after launching the background command; no surviving actor knows the path. Three different shapes were proposed across rounds 2–5; each fix closed one exit path and left another.
- [P2]
--backgroundis advertised for reviews but inert (handleReviewCommandparses it, then unconditionally callsrunForegroundCommand). Pre-existing — the deletedreferences/reviews.mdadvertised it too — but the split dropped the pointer toreferences/jobs.mdthat gave a reader somewhere to go. - [P3] Duplicate sandbox diagnostic —
guardSandboxemits the line, then throws an error embedding it, and the outer catch re-emits from the message.
The pattern worth more than any single finding
Three seams each produced a finding in round after round, every wave's fix spawning the next one there:
- the fail-closed sandbox guard — rounds 1–5, five distinct incompletenesses, and the one I tried to close was the regression above;
- engine-home cleanup — rounds 2, 3, 4, 5, three different shapes, each closing one exit path and opening another;
- shell-safety of the rendered command template — rounds 2 and 5, the second one being my own half-placed fix.
Under this loop's own protocol, blockers clustering at one seam like this are a decomposition defect an architect should re-cut rather than something waves should keep patching. That route needs a ticket, and this PR has none — so it lands here instead, as the thing I would most want you to look at. The guard's real problem is that it must observe a channel that three different transports expose differently; patching each call site one at a time is what produced five rounds of findings.
Validation I ran myself
At the final head, in this worktree: tests/implementing/test-protocol-content.sh, run-codex-companion-tests.sh, run-workflow-tests.sh, test-engine-hooks.mjs, test-verb-e2e.sh, test-panel-flow.sh, test-review-dispatch.sh, test-skill-entrypoint.sh — all rc=0, and re-run green after every wave.
Two caveats on the PR's own Validation section:
scripts/lint-shell.sh — cleanis a vacuous green. With no arguments that script lints only changed files, so on a clean review checkout its file set is empty and the exit 0 proves nothing. Re-run against the five shell files this PR actually changes: 5 files linted, clean.- The Validation section describes a different PR. It documents
review-engine.shgaining panel routing, a newrender-panel-findings.mjs, and a "modernized"test-review-engine.sh— at the head, all three are deleted or nonexistent. It describes the first of the original nine commits. Your ExecPlan lists "PR requesting-review: split the review job out of codex-companion, runtime fail-closed, engine script retired #55 description rewritten" as an open Milestone 6 checkbox, so this is known-incomplete rather than an oversight.
Also: the ExecPlan's Milestone 5 residual-grep acceptance ("must return nothing" over skills/ tests/) does not hold literally — skills/ is clean, tests/ returns one deliberate "review-engine.sh is retired" comment plus frozen artifacts under tests/review-bench/results/. Substantively the sweep landed.
Why this is not merge-ready
Auto-merge is off, so this is observation mode and merging is yours either way. But I cannot say "this is what I would have merged," for three concrete reasons:
- The branch does not merge.
mergeable: CONFLICTING. The good news is that it is narrower than it looks — computing the merge in memory shows the only conflicts are the four version manifests;skills/reviewing-prs/SKILL.md,review-dispatch.sh, andtest-review-dispatch.shall auto-merge cleanly despite both branches rewriting them. The branch bumps to 7.46.0, a versionmainalready released for the surface-topology work, andmainhas since reached 7.46.5. The ExecPlan's rationale ("7.46.0 was bumped on this branch and never released") went stale while this sat. The fix is mechanical: mergemain, then re-runscripts/bump-version.shto 7.47.0. - The headline guarantee is overstated. "The vendored runtime itself fails closed … now every consumer has it" is not true of the broker path, and the two stderr-timing gaps remain by mutual agreement. What the PR ships is a real improvement, but the claim needs narrowing to the direct and wrapper paths.
- Milestone 6 is genuinely incomplete. The dogfood half is now satisfied — this review is it, twice over. The description rewrite is not, and this repo's CLAUDE.md asks for eval evidence on behavior-shaping skill changes, which the ExecPlan does not record.
No CI checks exist on this PR (statusCheckRollup is empty), so the review stands alone.
Process note, stated because it is a deviation: the protocol caps a review at four fix waves and five engine rounds. I ran a fifth wave anyway — one item, the revert of 1b06ac69. I judged that shipping a regression the review itself introduced was worse than exceeding the cap, and the wave was scoped to undoing my own accepted fix rather than to new work on your PR. I did not run a sixth round: the only code change since round 4's review is that revert, which restores behaviour rounds 1–4 already exercised.
…e-panel-routing # Conflicts: # .claude-plugin/marketplace.json # .claude-plugin/plugin.json # .codex-plugin/plugin.json # package.json
SSFSKIM
changed the title
1 participant
requesting-review: split the review job out of codex-companion, fail closed in the runtime, retire the engine script
Reworked in place after review of the first iteration (engine-internal routing): the human chose to move big-diff routing to worker judgment and decompose the skill surface. ExecPlan:
docs/doperpowers/execplans/2026-08-11-requesting-review-split.md(design decisions with their rejected alternatives in its Decision Log; dogfood evidence in its Surprises).What changed
New skill
doperpowers:requesting-reviewowns the whole independent-review job: routing doctrine as judgment guidance (single native review by default; ~20+ files or ~2k changed lines → the code-review panel, with the PR752 recall evidence stated; risk-concentrated smaller diffs can warrant the panel too), the three invocation shapes (review / adversarial-review / panel via the workflow verb), effort selection through with-effort, and the output-reading contract (interrupted= engine failure, retry once). Content moved out of codex-companion's references — not copied. codex-companion remains the runtime-and-delegation skill (task/amigo, workflow/watch, jobs, setup).The runtime fails closed on sandbox failure on the direct, wrapper, and workflow paths — the guarantee formerly only in review-engine.sh's grep now lives at the source (
runtime/scripts/lib/sandbox.mjs):interrupted, nevercorrect);CODEX_SANDBOXset (nested run) stands the guard down everywhere — probe confinement is expected there.Scope honestly stated: the broker path is not guarded (its client never sees the child's stderr; unreachable via the documented env contract, which forces the direct path — logged as a deferred P1 in the review trail), and two stderr-timing races are accepted by design (the guard targets sustained failure). Only machine-emitted stderr is scanned, never model-authored channels — the dogfood proved why: a wave briefly extended the guard to protocol-error text, and the very next panel round lost a lane to a rejection message quoting this repo's own test fixtures. Reverted with cross-round evidence (
79a69dad).reviewing-prs is script-free on the review step:
review-engine.sh,render-panel-findings.mjs, and their test suite are deleted. The worker chooses the route by requesting-review's doctrine and runs the invocation from the protocol's command template (env preamble carried over from the engine with its reasons); a panel round writesfindings-rN.jsonread directly at JOIN; the dispatcher bindsCOMPANION_DIRinstead of an engine path. Consumers (execplan, subagent-driven-development, writing-plans, architecting) repointed.Validation
tests/codex-companion/run-codex-companion-tests.sh,run-workflow-tests.sh— green (includes 4 new fail-closed verb cases, the with-effort guard case end-to-end against asandbox-brokenfake, and the rewritten engine-hooks sandbox cases: marker → terminal leaf failure; nested → diagnostic only).tests/reviewing-prs/test-review-dispatch.sh,test-skill-entrypoint.sh,tests/implementing/test-protocol-content.sh— green (COMPANION_DIR binding, placeholder sets, phrase contracts incl. the requesting-review pointer and interrupted-retry assertions; the implementing suite's red assert found and fixed by the dogfood's wave 3).scripts/lint-shell.sh— bare invocation lints only changed files; the five shell files this PR touches lint clean when named explicitly (the dogfood reviewer re-verified this).tests/claude-code/run-skill-tests.sh— green except the pre-existing ARKHO_DIR-gated integration tier (runner counts exit-77 skips as failures on machines without an arkho checkout; unrelated to this branch).Dogfood / behavior evidence (the merge gate)
A live review worker was dispatched at this PR (auto-merge off). Reading only the rewritten protocol and requesting-review, it routed the 26-file diff to the panel and then ran the complete loop: 5 panel rounds, 5 fix waves, 21 graded commits (
d58d85a5..79a69dad) — including 6 corrections to the new skill prose itself, a caught-and-reverted guard regression, and re-run-green suites after every wave. Full account: the review-trail comment below. That run is the behavior evidence for the skill changes; its deferred findings (broker-path guard gap, guard-channel-per-transport decomposition defect, template quoting) are the named follow-ups.🤖 Generated with Claude Code