From 5eb0a4b3bb7f837a6c9373b8f4b308ee367c1bf9 Mon Sep 17 00:00:00 2001 From: SSFSKIM Date: Mon, 10 Aug 2026 20:39:37 +0900 Subject: [PATCH 01/32] feat(reviewing-prs): route big diffs to the codex-companion review panel inside the engine MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 #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 #1148 stale-manifest lesson). - test-review-engine.sh: modernized — it still stubbed `codex` argv from the pre-companion engine and has been red since 89eec7ef ('node not found'); now stubs the companion layer via REVIEW_COMPANION_DIR and covers both routes, the auto threshold, and the interrupted verdict. --- skills/reviewing-prs/SKILL.md | 89 ++++---- .../references/operation-manual.md | 40 ++-- .../reviewing-prs/references/runner-setup.md | 6 +- .../scripts/render-panel-findings.mjs | 42 ++++ skills/reviewing-prs/scripts/review-engine.sh | 69 ++++++- tests/reviewing-prs/test-review-engine.sh | 192 +++++++++++++----- 6 files changed, 304 insertions(+), 134 deletions(-) create mode 100644 skills/reviewing-prs/scripts/render-panel-findings.mjs diff --git a/skills/reviewing-prs/SKILL.md b/skills/reviewing-prs/SKILL.md index 72aebd1002..8b4bbc8c4c 100644 --- a/skills/reviewing-prs/SKILL.md +++ b/skills/reviewing-prs/SKILL.md @@ -146,56 +146,38 @@ is a TOOL invocation, not a nested agent. Never add remove that directory before ending the turn — EXCEPT a needs-human park: wave boards live there and the resumed turn reads them. -2. Judge the diff shape and choose this round's engine-run count — most - PRs need exactly ONE run; a substantial diff may warrant 2–3 parallel - runs, whole-branch scale up to 4. From the worktree root, start each - run IN THE BACKGROUND (round N, run k uses findings-rN-k.txt; the - empty lens assignments are deliberate — they shield the plain run - from any inherited host value): +2. From the worktree root, start the round's ONE engine run IN THE + BACKGROUND (round N uses findings-rN.txt; the empty lens assignments + are deliberate — they shield the run from any inherited host value): CODEX_REVIEW_MODEL={{CODEX_REVIEW_MODEL}} \ CODEX_REVIEW_EFFORT={{CODEX_REVIEW_EFFORT}} \ CODEX_REVIEW_LENS= CODEX_REVIEW_LENS_FILE= \ {{REVIEW_ENGINE}} --base origin/{{BASE_REF}} \ - --out /findings-r1-1.txt - - A single run takes no lens. When fanning out, keep one run lens-free - as the broad sweep and give each other run a LENS: a structural focus - mandate you derive from the diff itself (e.g. actor/authz assumptions - in the changed routes; ordering/atomicity of the new writes; - consumers of a changed field) — never ticket/spec content. The repo's - risk-surface manifest (in your dispatch prompt) marks validated hot - paths: a diff touching one is a strong lens candidate. Write each - mandate to `/lens-.txt` with your file-writing tool - and set `CODEX_REVIEW_LENS_FILE=/lens-.txt` on that - run's command — never inline the mandate text into a shell command - (it is generated prose; interpolation is an injection surface). A - lensed run narrows hard — a scalpel beside the sweep, not a second - sweep; it runs the engine's challenge-review rubric along the lens, - so its findings may question structure and assumptions, not only - defects — triage them with the same judgment. Use your harness's background execution for these commands and - keep the task handles. Leave them running and the findings unread — - the protocol's COMPLIANCE AUDIT runs while the engine reviews, and - its JOIN step is the only place engine output is read. -3. At JOIN: wait for all of the round's background tasks. Bound the - wait — an engine task that has neither completed nor failed - 45 minutes after start is hung: kill it. The lens-free sweep is the - round's required whole-range review: if IT failed, the round failed - (the fallback below owns retries and the outage path) — only lensed - runs' failures are tolerable. When the sweep succeeded, proceed on - the successful outputs and record any failed lensed runs in the - review trail. -4. Read the findings file(s) — the round's findings are their union; - overlapping findings collapse into one triaged item (keep the - highest-priority duplicate as the anchor). + --out /findings-r1.txt + + Diff-size scaling is the ENGINE's, not yours: on a big diff (~20+ + files or a couple thousand changed lines) it runs the + codex-companion code-review panel — one lens-free sweep, diff-derived + scalpel lenses, one binding verifier — instead of a single native + review, and renders the verifier-confirmed findings into the same + --out file (raw panel result beside it as findings-rN.txt.panel.json; + a panel round can take ~20 minutes). Use your harness's background + execution and keep the task handle. Leave it running and the findings + unread — the protocol's COMPLIANCE AUDIT runs while the engine + reviews, and its JOIN step is the only place engine output is read. +3. At JOIN: wait for the round's background task. Bound the wait — an + engine task that has neither completed nor failed 45 minutes after + start is hung: kill it. If the run failed, the round failed (the + fallback below owns retries and the outage path). +4. Read the findings file. Correctness review of the whole range is the engine's job; your own reading serves the audit and the triage, not a second review. The verdict is YOURS, derived from the findings: approve when no -critical/high finding remains unresolved; needs-attention otherwise. On -RE-REVIEW rounds the same run-count judgment applies — after a small fix -wave a single plain run is the norm — with fresh --out files, again in -the background. +critical/high finding remains unresolved; needs-attention otherwise. +RE-REVIEW rounds run the same single command with a fresh --out file, +again in the background. ENGINE FALLBACK — there is no second engine; the reviewer is codex-only. If the engine script fails (codex missing — rc 127, auth failure, or @@ -295,11 +277,10 @@ itself a finding. ## JOIN -Wait for ALL of the round's background engine tasks per the engine -block's bound; a failed lens-free sweep fails the round (the fallback -block owns retries and the outage path — lensed-run failures alone do -not). Read every successful run's compact findings file — the round's -findings are their union — and your already-written audit together. +Wait for the round's background engine task per the engine block's +bound; a failed run fails the round (the fallback block owns retries +and the outage path). Read the round's compact findings file and your +already-written audit together. From here on, command-backed evidence checks may run whenever nothing else holds the worktree — never while an engine round or a fixer wave is live. @@ -351,9 +332,9 @@ Maximum 4 waves per review. ## RE-REVIEW -After a wave that fixed anything, rerun the engine — same run-count -judgment (a single plain run is the norm after a small wave), fresh ---out files, in the background again; max 5 engine rounds total. The +After a wave that fixed anything, rerun the engine — the same single +command with a fresh --out file, in the background again; max 5 engine +rounds total. The engine is stateless: it WILL re-flag findings you already routed. Match re-flags by file and substance against your tech-debt comments and wave dispositions (line numbers shift after fixes). A match against a LOGGED @@ -417,8 +398,9 @@ end your turn with the park intact. A `review-epic-` dispatch is the E2 scale review: the ticket is an EPIC in in-review whose `pr:` meta is a closure package, not a PR (your -`CLOSURE_PACKAGE` binding names it). Same engine machinery — whole-range -codex runs, lenses derived from the cross-child contracts: your worktree +`CLOSURE_PACKAGE` binding names it). Same engine machinery — a +whole-range codex run per range (an epic-scale diff routes to the panel +by size, as any diff does): your worktree sits at the epic's integration branch and START ENGINE's `--base origin/{{BASE_REF}}` reviews it against the branch it merges into. When your dispatch prompt instead says this epic has NO aggregate @@ -496,8 +478,9 @@ what the contract permits separately from what the evidence shows actually ran. ## REVIEW TRAIL The review-trail comment on the PR records: engine and rounds run — for -a fan-out round, every run (its lens mandate verbatim, or lens-free) with -the findings it contributed, written BEFORE `` cleanup; the +a panel round, the panel's verdict line and its lens/coverage summary +(from the findings file and the .panel.json beside it), written BEFORE +`` cleanup; the compliance-audit verdict with every AUDIT NOTE; every finding with its bin and a one-line disposition; each wave with its per-item board outcomes; deferred findings inline when the tech-debt issue is "none"; diff --git a/skills/reviewing-prs/references/operation-manual.md b/skills/reviewing-prs/references/operation-manual.md index bbac19a454..f659a642bf 100644 --- a/skills/reviewing-prs/references/operation-manual.md +++ b/skills/reviewing-prs/references/operation-manual.md @@ -77,9 +77,10 @@ human to merge" state. **Risk surfaces feed scrutiny, not a merge gate.** A repo may declare concrete hot paths in an optional `.doperpowers/risk-surfaces.md` — a plain list of globs and prose path/content rules the worker reads against -the diff; a diff touching one is a strong lens candidate for the engine -fan-out. The dispatch layer injects it from the PR's **base ref, never -HEAD**, so a PR cannot delist a surface it touches in the same commit. +the diff; a diff touching one earns the worker's sharpest audit and +triage attention. The dispatch layer injects it from the PR's **base ref, +never HEAD**, so a PR cannot delist a surface it touches in the same +commit. **Repo facts feed the cross-check.** The optional `.doperpowers/repo-facts.md` manifest (format: doperpowers:implementing) @@ -127,22 +128,22 @@ produce evidence, the review side verifies the claims were real. Review responsibility is split between two concurrent tracks with one owner each. The ENGINE — the native codex review run by -`scripts/review-engine.sh` through the doperpowers:codex-companion runtime -(plain run = the non-steerable `review` verb; lensed run = the -`adversarial-review` verb with the lens as focus) — receives no ticket, -spec, or policy input of +`scripts/review-engine.sh` through the doperpowers:codex-companion +runtime — receives no ticket, spec, or policy input of any kind: coupling spec policy into the native reviewer measurably weakened -its correctness review, so the interface is `--base` + `--out` plus the -optional `CODEX_REVIEW_LENS` env — a structural focus mandate the worker -derives from the diff itself (never from the ticket/spec) when it fans out -to 2–4 parallel runs on a large diff; a bench-validated lens recovered a -confirmed authz defect two plain runs had missed -(`tests/review-bench/results/2026-07-28-pr752-lenscell/`). The worker -starts the round's runs in the background, and each returns a compact -structured verdict file; the PR diff never enters the worker's own -context. A hung engine (no result within 45 minutes) is killed and treated -as a failure; a failed lens-free sweep fails the round (it is the required -whole-range review), while failed lensed runs are merely recorded. +its correctness review, so the interface is `--base` + `--out`. Diff-size +scaling lives inside the engine: a big diff (~20+ files or ~2k changed +lines — one reviewer's recall thins at that scale; on the PR752 benchmark +the best single run found 10 of 13 confirmed defects while a multi-run +union found all 13) routes to the companion's code-review panel — one +lens-free sweep, up to five diff-derived scalpel lenses, one binding +verifier — rendered into the same findings file (raw panel JSON beside it; +a `interrupted` panel verdict is an engine failure, never a findings +file). Smaller diffs run the single non-steerable `review` verb. The +worker starts the round's run in the background and gets a compact +findings file back; the PR diff never enters the worker's own context. A +hung engine (no result within 45 minutes) is killed and treated as a +failure; a failed run fails the round. The WORKER meanwhile audits implementer protocol/spec compliance itself, read-only, and records the audit BEFORE reading engine output: the issue @@ -212,7 +213,8 @@ self-review bias: the entity that grades the fixes never wrote them. `tech-debt` label). 5. (Optional) Add `.doperpowers/risk-surfaces.md` listing the repo's validated hot paths — auth files, migration dirs, privileged routes, - security-sensitive SQL; reviewers read it for lens derivation. Commit + security-sensitive SQL; reviewers read it to sharpen audit and triage + scrutiny. Commit it on the branch(es) reviewers target (it is read from the base). 6. Start in observation mode: leave `AUTO_MERGE_ENABLED` unset/false in the workflow env. Flip it to `true` only after the trail comments show the diff --git a/skills/reviewing-prs/references/runner-setup.md b/skills/reviewing-prs/references/runner-setup.md index f277ebd7eb..a36233b3ab 100644 --- a/skills/reviewing-prs/references/runner-setup.md +++ b/skills/reviewing-prs/references/runner-setup.md @@ -76,6 +76,6 @@ Declare the repo's validated hot paths in `.doperpowers/risk-surfaces.md` on the branch(es) reviewers target. It's a plain list of globs and prose path/content rules the review worker reads against the diff — auth files, migration dirs, privileged -routes, security-sensitive SQL — as lens-derivation input for the engine -fan-out. It is read from the PR's **base ref** (never HEAD). Absent file → -reviewers derive lenses from the diff alone. +routes, security-sensitive SQL — to sharpen its audit and triage +scrutiny; it never moves merge authority (surfaces are not a merge gate). +It is read from the PR's **base ref** (never HEAD). diff --git a/skills/reviewing-prs/scripts/render-panel-findings.mjs b/skills/reviewing-prs/scripts/render-panel-findings.mjs new file mode 100644 index 0000000000..ca67cfd222 --- /dev/null +++ b/skills/reviewing-prs/scripts/render-panel-findings.mjs @@ -0,0 +1,42 @@ +#!/usr/bin/env node +// render-panel-findings.mjs — flatten the code-review panel's stdout JSON +// ({runId, result: {verdict, findings, coverage, lenses, explanation}}) into +// the rendered-findings text the reviewing-prs worker reads at JOIN, so the +// panel route and the single-review route land in --out with one contract. +// +// `interrupted` exits 4: the panel withheld its verdict (lost lane, no +// contract-valid verifier set, or the reviewed head moved), so there is no +// findings file to write — the caller treats it like any other engine +// failure (retry, then the ENGINE-UNAVAILABLE fallback). Any partial +// evidence stays in the .panel.json beside --out for the review trail. +// +// Usage: render-panel-findings.mjs (findings text on stdout) +import { readFileSync } from "node:fs"; + +const raw = JSON.parse(readFileSync(process.argv[2], "utf8")); +const r = raw?.result ?? {}; +if (r.verdict === "interrupted") { + console.error(`panel interrupted: ${r.explanation ?? "no explanation"}`); + process.exit(4); +} +if (r.verdict !== "correct" && r.verdict !== "incorrect") { + console.error(`panel returned no verdict (got ${JSON.stringify(r.verdict)})`); + process.exit(4); +} + +const findings = r.findings ?? []; +const lenses = (r.lenses ?? []).length; +const out = []; +out.push( + `Panel verdict: ${r.verdict} — ${findings.length} verifier-confirmed ` + + `finding${findings.length === 1 ? "" : "s"} (sweep + ${lenses} lens${lenses === 1 ? "" : "es"} + verifier)` +); +if (r.explanation) out.push("", String(r.explanation).trim()); +for (const f of findings) { + const loc = f.lines ? `${f.file}:${f.lines}` : f.file; + out.push("", `- [${f.priority}] ${f.title} (${loc})`); + if (f.comment) out.push(` ${String(f.comment).trim().replace(/\n/g, "\n ")}`); + if (f.sources?.length > 1) out.push(` raised independently by: ${f.sources.join(", ")}`); +} +if (findings.length === 0) out.push("", "No verifier-confirmed findings."); +console.log(out.join("\n")); diff --git a/skills/reviewing-prs/scripts/review-engine.sh b/skills/reviewing-prs/scripts/review-engine.sh index b392775f98..708e4252ab 100755 --- a/skills/reviewing-prs/scripts/review-engine.sh +++ b/skills/reviewing-prs/scripts/review-engine.sh @@ -2,8 +2,11 @@ # review-engine.sh — the ONE review-engine invocation for the reviewing-prs # loop, driven by the doperpowers:codex-companion runtime (vendored codex # app-server client; sibling skill). PURE correctness review: a plain run is -# codex's native `review` verb with no ticket/spec input of any kind; the -# single optional modification is a diff-derived structural LENS +# codex's native `review` verb with no ticket/spec input of any kind — and +# on a big diff it routes instead to the companion's code-review PANEL +# (native sweep + diff-derived scalpel lenses + binding verifier; see the +# panel-routing block below), still behind the same --base/--out contract. +# The single optional modification is a diff-derived structural LENS # (CODEX_REVIEW_LENS_FILE / CODEX_REVIEW_LENS, see the lens block below), # which routes the run through the `adversarial-review` verb with the lens # as its focus mandate. Ticket/spec compliance is the REVIEW WORKER's own @@ -18,7 +21,8 @@ # Env: CODEX_REVIEW_MODEL (default gpt-5.6-sol), CODEX_REVIEW_EFFORT # (default xhigh), CODEX_REVIEW_LENS_FILE / CODEX_REVIEW_LENS (optional # diff-derived structural focus mandate — see the lens block below; both -# empty keeps the plain review). +# empty keeps the plain review), CODEX_REVIEW_PANEL (auto|always|never, +# default auto — see the panel-routing block below). # Run from the worktree root — the engine reviews $PWD. # Exits 127 when codex/node are missing, 2 on usage error, else the # runtime's rc. @@ -38,7 +42,7 @@ command -v codex >/dev/null 2>&1 || { echo "review-engine: codex CLI not found" command -v node >/dev/null 2>&1 || { echo "review-engine: node not found" >&2; exit 127; } script_dir="$(cd "$(dirname "$0")" && pwd)" -companion="$script_dir/../../codex-companion" +companion="${REVIEW_COMPANION_DIR:-$script_dir/../../codex-companion}" # override: tests [ -f "$companion/scripts/with-effort.mjs" ] || { echo "review-engine: codex-companion skill not found at $companion" >&2; exit 127; } model="${CODEX_REVIEW_MODEL:-gpt-5.6-sol}" @@ -107,13 +111,58 @@ if [ -n "$lens" ]; then verb_args=( adversarial-review --base "$base" --wait --model "$model" -- "$lens" ) fi -# with-effort.mjs serves the verb a private app-server carrying the effort -# override (the review protocol has no effort field) and provides its own -# live endpoint — no detached broker, nothing outlives this run. Findings -# render on stdout (--out), progress on stderr (.events.log). +# Panel routing: one reviewer's recall thins on a big diff — the multilens +# execplan's PR752 benchmark (22 files, +2,101 lines) had the best single +# engine find 10 of 13 confirmed defects while seven single runs' union +# found all 13. At the codex-companion rule-of-thumb threshold (~20+ files +# or ~2k changed lines) a plain run therefore routes to the bundled +# code-review panel — one native sweep, diff-derived scalpel lenses, one +# binding verifier — instead of a single native review. A lensed +# invocation is already a scalpel and never routes. CODEX_REVIEW_PANEL +# overrides the size gate: `always` / `never` (default `auto`). +panel="" +if [ -z "$lens" ]; then + case "${CODEX_REVIEW_PANEL:-auto}" in + always) panel=1 ;; + never) ;; + auto) + # --numstat: "added deleted path"; binary files carry "-" and count + # toward files only. A git failure (unfetched base, not a repo) counts + # nothing — the review verb below then surfaces the real error. + counts="$({ git diff --numstat "$base...HEAD" 2>/dev/null || true; } | \ + awk '{files++; if ($1 != "-") changed += $1 + $2} END {printf "%d %d", files+0, changed+0}')" + if [ "${counts% *}" -ge 20 ] || [ "${counts#* }" -ge 2000 ]; then panel=1; fi + ;; + *) echo "review-engine: CODEX_REVIEW_PANEL must be auto|always|never (got: $CODEX_REVIEW_PANEL)" >&2; exit 2 ;; + esac +fi + rc=0 -node "$companion/scripts/with-effort.mjs" --effort "$effort" -- \ - "${verb_args[@]}" > "$out" 2> "$out.events.log" || rc=$? +if [ -n "$panel" ]; then + # The workflow verb runs the panel as ONE foreground process; every worker + # spawns its own app-server (disableBroker), so with-effort's private + # endpoint has no role here — finder/verifier model and effort ride the + # script args instead. Args are built by node so no shell value is ever + # interpolated into JSON. The raw panel result stays in .panel.json + # beside the rendered findings for the review trail. + args_json="$(P_BASE="$base" P_MODEL="$model" P_EFFORT="$effort" node -e \ + 'console.log(JSON.stringify({base: process.env.P_BASE, finderModel: process.env.P_MODEL, finderEffort: process.env.P_EFFORT, verifierModel: process.env.P_MODEL}))')" + node "$companion/runtime/scripts/codex-companion.mjs" workflow \ + --script "$companion/workflows/code-review.mjs" \ + --args "$args_json" > "$out.panel.json" 2> "$out.events.log" || rc=$? + if [ "$rc" -eq 0 ]; then + # Exit 4 = panel verdict `interrupted` (lost lane or moved head): no + # findings file exists, the caller retries like any engine failure. + node "$script_dir/render-panel-findings.mjs" "$out.panel.json" > "$out" || rc=$? + fi +else + # with-effort.mjs serves the verb a private app-server carrying the effort + # override (the review protocol has no effort field) and provides its own + # live endpoint — no detached broker, nothing outlives this run. Findings + # render on stdout (--out), progress on stderr (.events.log). + node "$companion/scripts/with-effort.mjs" --effort "$effort" -- \ + "${verb_args[@]}" > "$out" 2> "$out.events.log" || rc=$? +fi # Fail closed when the engine's own fs sandbox never worked. Observed live # (ida-worker-1, 2026-08-09): a host that blocks unprivileged userns makes diff --git a/tests/reviewing-prs/test-review-engine.sh b/tests/reviewing-prs/test-review-engine.sh index 9ddfce0d11..dd74f7e2b9 100755 --- a/tests/reviewing-prs/test-review-engine.sh +++ b/tests/reviewing-prs/test-review-engine.sh @@ -1,10 +1,12 @@ #!/usr/bin/env bash # -# Hermetic tests for review-engine.sh — the single native-review invocation. -# The engine is PURE correctness review: no criteria file, no developer -# instructions — the worker owns ticket/spec compliance out-of-engine. -# `codex` is stubbed: it logs argv + the env recipe, honors -o, and exits -# with STUB_CODEX_RC. No network, no real codex. +# Hermetic tests for review-engine.sh — the single review-engine invocation. +# The engine is PURE correctness review driven through the codex-companion +# runtime; the companion layer is stubbed via REVIEW_COMPANION_DIR: stub +# with-effort.mjs / codex-companion.mjs log argv + the env recipe, emit +# canned output, and exit with STUB_RC. `codex` on PATH is a no-op (only +# its existence is probed). Real node runs the stubs and the engine's own +# render helper. No network, no real codex. set -euo pipefail SCRIPT_DIR="$(cd "$(dirname "$0")" && pwd)" @@ -23,11 +25,11 @@ assert_equals() { fail "$3"; echo " expected: $2"; echo " actual: $1"; fi } assert_contains() { - if printf '%s' "$1" | grep -Fq -- "$2"; then pass "$3"; else + if grep -Fq -- "$2" <<<"$1"; then pass "$3"; else fail "$3"; echo " expected to find: $2"; echo " in: $1"; fi } assert_not_contains() { - if printf '%s' "$1" | grep -Fq -- "$2"; then + if grep -Fq -- "$2" <<<"$1"; then fail "$3"; echo " expected NOT to find: $2"; echo " in: $1"; else pass "$3"; fi } @@ -38,44 +40,75 @@ mkdir -p "$HOME/.codex" "$HOME/.local/bin" "$TMPDIR" echo '{"token":"fake"}' > "$HOME/.codex/auth.json" : > "$HOME/.local/bin/codex-code-mode-host"; chmod +x "$HOME/.local/bin/codex-code-mode-host" +# codex exists on PATH (the engine probes it) but is never exercised — +# every invocation goes through the stubbed companion scripts below. STUB_BIN="$TEST_ROOT/bin"; mkdir -p "$STUB_BIN" -cat > "$STUB_BIN/codex" <<'STUB' -#!/usr/bin/env bash -set -euo pipefail -{ - printf 'ARGS:'; printf ' %q' "$@"; printf '\n' - echo "ENV_CODEX_HOME=${CODEX_HOME:-}" - echo "ENV_SSL_CERT_FILE=${SSL_CERT_FILE:-}" - echo "ENV_HOST_PATH=${CODEX_CODE_MODE_HOST_PATH:-}" - echo "AUTH_LINK=$([ -L "${CODEX_HOME:-/nonexistent}/auth.json" ] && echo yes || echo no)" - echo "AUTH_TARGET=$(readlink "${CODEX_HOME:-/nonexistent}/auth.json" 2>/dev/null || true)" -} >> "$ENGINE_LOG" -prev=""; out="" -for a in "$@"; do [ "$prev" = "-o" ] && out="$a"; prev="$a"; done -[ -n "$out" ] && echo "- [P2] stub finding (ratio.py:2)" > "$out" -exit "${STUB_CODEX_RC:-0}" +printf '#!/usr/bin/env bash\nexit 0\n' > "$STUB_BIN/codex"; chmod +x "$STUB_BIN/codex" +NODE_DIR="$(dirname "$(command -v node)")" +GIT_DIR_BIN="$(dirname "$(command -v git)")" +export PATH="$STUB_BIN:$NODE_DIR:$GIT_DIR_BIN:/usr/bin:/bin" + +# Stub companion: with-effort.mjs (single-review route) and +# codex-companion.mjs (panel route). Both log to ENGINE_LOG. +COMPANION="$TEST_ROOT/companion" +mkdir -p "$COMPANION/scripts" "$COMPANION/runtime/scripts" "$COMPANION/workflows" +: > "$COMPANION/workflows/code-review.mjs" +cat > "$COMPANION/scripts/with-effort.mjs" <<'STUB' +import { appendFileSync, lstatSync, readlinkSync } from "node:fs"; +const e = process.env; +let link = "no", target = ""; +try { + if (lstatSync(`${e.CODEX_HOME}/auth.json`).isSymbolicLink()) { + link = "yes"; target = readlinkSync(`${e.CODEX_HOME}/auth.json`); + } +} catch {} +appendFileSync(e.ENGINE_LOG, + `WITH_EFFORT ARGS: ${process.argv.slice(2).join(" ")}\n` + + `LENS_ARG=${JSON.stringify(process.argv[process.argv.length - 1])}\n` + + `ENV_CODEX_HOME=${e.CODEX_HOME ?? ""}\n` + + `ENV_SSL_CERT_FILE=${e.SSL_CERT_FILE ?? ""}\n` + + `ENV_HOST_PATH=${e.CODEX_CODE_MODE_HOST_PATH ?? ""}\n` + + `ENV_PLUGIN_DATA=${e.CLAUDE_PLUGIN_DATA ?? ""}\n` + + `AUTH_LINK=${link}\nAUTH_TARGET=${target}\n`); +if (e.STUB_EVENTS) process.stderr.write(e.STUB_EVENTS + "\n"); +console.log("- [P2] stub finding (ratio.py:2)"); +process.exit(Number(e.STUB_RC ?? 0)); STUB -chmod +x "$STUB_BIN/codex" -export PATH="$STUB_BIN:/usr/bin:/bin" +cat > "$COMPANION/runtime/scripts/codex-companion.mjs" <<'STUB' +import { appendFileSync, readFileSync } from "node:fs"; +const e = process.env; +appendFileSync(e.ENGINE_LOG, + `COMPANION ARGS: ${JSON.stringify(process.argv.slice(2))}\n` + + `ENV_CODEX_HOME=${e.CODEX_HOME ?? ""}\n` + + `ENV_PLUGIN_DATA=${e.CLAUDE_PLUGIN_DATA ?? ""}\n`); +if (e.STUB_EVENTS) process.stderr.write(e.STUB_EVENTS + "\n"); +if (e.STUB_PANEL_FILE) console.log(readFileSync(e.STUB_PANEL_FILE, "utf8")); +else console.log(JSON.stringify({ runId: "wf_stub", result: { + verdict: "incorrect", + findings: [{ id: "sweep#1", priority: "P1", title: "stub panel finding", + file: "a.ts", lines: "10-12", comment: "why it is wrong", + sources: ["sweep", "lens-1"] }], + coverage: [], lenses: ["l1", "l2"], explanation: "confirmed: stub panel finding" } })); +process.exit(Number(e.STUB_RC ?? 0)); +STUB +export REVIEW_COMPANION_DIR="$COMPANION" WT="$TEST_ROOT/wt"; mkdir -p "$WT"; cd "$WT" -reset() { : > "$ENGINE_LOG"; rm -f "$TEST_ROOT/out.txt" "$TEST_ROOT/out.txt.events.jsonl"; } +reset() { : > "$ENGINE_LOG"; rm -f "$TEST_ROOT"/out.txt*; } -echo "happy path (non-nested):" +echo "happy path (single review, non-nested):" reset env -u CODEX_HOME -u CODEX_SANDBOX -u CODEX_REVIEW_MODEL -u CODEX_REVIEW_EFFORT \ - -u SSL_CERT_FILE -u CODEX_CODE_MODE_HOST_PATH \ + -u CODEX_REVIEW_PANEL -u SSL_CERT_FILE -u CODEX_CODE_MODE_HOST_PATH \ "$ENGINE" --base origin/main --out "$TEST_ROOT/out.txt" LOG="$(cat "$ENGINE_LOG")" -assert_contains "$LOG" "exec review --base origin/main" "invokes the native review subcommand with the base" -assert_contains "$LOG" "gpt-5.6-sol" "default model applied" -assert_contains "$LOG" "xhigh" "default effort applied" +assert_contains "$LOG" "WITH_EFFORT ARGS: --effort xhigh -- review --base origin/main --wait --model gpt-5.6-sol" \ + "single route: with-effort wraps the native review verb, default model/effort" +assert_not_contains "$LOG" "COMPANION ARGS" "small/unmeasurable diff never routes to the panel" assert_not_contains "$LOG" "developer_instructions" "pure engine sends no developer instructions" -assert_not_contains "$LOG" "criteria" "pure engine has no criteria concept in its argv" -assert_not_contains "$LOG" "SPEC COMPLIANCE" "no spec policy rides the engine" -assert_not_contains "$LOG" "danger-full-access" "non-nested run never widens the sandbox" assert_contains "$LOG" "ENV_CODEX_HOME=$TMPDIR/review-engine-home." "temporary CODEX_HOME stays outside the reviewed tree" +assert_contains "$LOG" "ENV_PLUGIN_DATA=$TMPDIR/review-engine-home." "temporary CLAUDE_PLUGIN_DATA isolates the companion ledger" assert_contains "$LOG" "AUTH_LINK=yes" "auth.json symlinked into the engine home" assert_equals "$(find "$TMPDIR" -maxdepth 1 -name 'review-engine-home.*' | wc -l | tr -d ' ')" "0" "engine home removed after the run" assert_equals "$(cat "$TEST_ROOT/out.txt")" "- [P2] stub finding (ratio.py:2)" "findings land in --out" @@ -85,36 +118,91 @@ reset CUSTOM_CODEX_HOME="$TEST_ROOT/custom-codex" mkdir -p "$CUSTOM_CODEX_HOME" echo '{"token":"custom"}' > "$CUSTOM_CODEX_HOME/auth.json" -CODEX_HOME="$CUSTOM_CODEX_HOME" \ -env -u CODEX_SANDBOX -u SSL_CERT_FILE -u CODEX_CODE_MODE_HOST_PATH \ - "$ENGINE" --base origin/main --out "$TEST_ROOT/out.txt" +CODEX_HOME="$CUSTOM_CODEX_HOME" "$ENGINE" --base origin/main --out "$TEST_ROOT/out.txt" LOG="$(cat "$ENGINE_LOG")" assert_contains "$LOG" "AUTH_TARGET=$CUSTOM_CODEX_HOME/auth.json" "auth is inherited from a custom CODEX_HOME" -echo "nested:" +echo "only-if-unset env:" reset -CODEX_SANDBOX=seatbelt \ -env -u CODEX_HOME -u SSL_CERT_FILE -u CODEX_CODE_MODE_HOST_PATH \ +env -u CODEX_HOME SSL_CERT_FILE=/custom/pem CODEX_CODE_MODE_HOST_PATH=/custom/host \ "$ENGINE" --base origin/main --out "$TEST_ROOT/out.txt" LOG="$(cat "$ENGINE_LOG")" -assert_contains "$LOG" 'danger-full-access' "nested run skips self-profiling (outer profile confines)" -assert_contains "$LOG" "ENV_HOST_PATH=$HOME/.local/bin/codex-code-mode-host" "code-mode host path exported" -assert_contains "$LOG" "ENV_SSL_CERT_FILE=/etc/ssl/cert.pem" "TLS file bundle exported" +assert_contains "$LOG" "ENV_SSL_CERT_FILE=/custom/pem" "pre-set SSL_CERT_FILE preserved" +assert_contains "$LOG" "ENV_HOST_PATH=/custom/host" "pre-set host path preserved" -echo "only-if-unset env:" +echo "lens routes to adversarial-review:" reset -env -u CODEX_HOME CODEX_SANDBOX=seatbelt SSL_CERT_FILE=/custom/pem CODEX_CODE_MODE_HOST_PATH=/custom/host \ +printf 'watch the actor identity assumptions in the changed routes' > "$TEST_ROOT/lens.txt" +CODEX_REVIEW_LENS_FILE="$TEST_ROOT/lens.txt" CODEX_REVIEW_PANEL=always \ "$ENGINE" --base origin/main --out "$TEST_ROOT/out.txt" LOG="$(cat "$ENGINE_LOG")" -assert_contains "$LOG" "ENV_SSL_CERT_FILE=/custom/pem" "pre-set SSL_CERT_FILE preserved" -assert_contains "$LOG" "ENV_HOST_PATH=/custom/host" "pre-set host path preserved" +assert_contains "$LOG" "adversarial-review --base origin/main" "lens run uses the adversarial-review verb" +assert_contains "$LOG" 'LENS_ARG="watch the actor identity assumptions in the changed routes"' \ + "lens text travels as a single trailing argv element" +assert_not_contains "$LOG" "COMPANION ARGS" "a lensed run never routes to the panel, even under always" + +echo "panel route (CODEX_REVIEW_PANEL=always):" +reset +CODEX_REVIEW_PANEL=always "$ENGINE" --base origin/main --out "$TEST_ROOT/out.txt" +LOG="$(cat "$ENGINE_LOG")" +assert_contains "$LOG" "COMPANION ARGS" "always forces the panel route" +assert_contains "$LOG" '"workflow","--script"' "panel runs through the workflow verb" +assert_contains "$LOG" "workflows/code-review.mjs" "panel script is the bundled code-review panel" +assert_contains "$LOG" '\"base\":\"origin/main\"' "panel args carry the base" +assert_contains "$LOG" '\"finderModel\":\"gpt-5.6-sol\"' "panel args carry the finder model" +assert_contains "$LOG" '\"finderEffort\":\"xhigh\"' "panel args carry the finder effort" +assert_not_contains "$LOG" "WITH_EFFORT" "panel route does not also run a single review" +OUT="$(cat "$TEST_ROOT/out.txt")" +assert_contains "$OUT" "Panel verdict: incorrect" "panel verdict rendered into --out" +assert_contains "$OUT" "- [P1] stub panel finding (a.ts:10-12)" "confirmed finding rendered with priority and location" +assert_contains "$OUT" "raised independently by: sweep, lens-1" "multi-source findings carry their lanes" +assert_equals "$([ -s "$TEST_ROOT/out.txt.panel.json" ] && echo yes)" "yes" "raw panel result kept beside --out" + +echo "panel auto-threshold (real repo):" +REPO="$TEST_ROOT/repo"; mkdir -p "$REPO"; cd "$REPO" +git init -q -b main; git config user.email t@t; git config user.name t +echo base > seed.txt; git add .; git commit -qm seed +git checkout -qb feature +for i in $(seq 1 21); do echo "change $i" > "f$i.txt"; done +git add .; git commit -qm "21 files" +reset +"$ENGINE" --base main --out "$TEST_ROOT/out.txt" +LOG="$(cat "$ENGINE_LOG")" +assert_contains "$LOG" "COMPANION ARGS" "21 changed files crosses the file threshold → panel" +git checkout -q main; git checkout -qb small +echo tweak >> seed.txt; git add .; git commit -qm tweak +reset +"$ENGINE" --base main --out "$TEST_ROOT/out.txt" +LOG="$(cat "$ENGINE_LOG")" +assert_contains "$LOG" "WITH_EFFORT ARGS" "a small diff stays on the single review" +assert_not_contains "$LOG" "COMPANION ARGS" "a small diff never reaches the panel" +cd "$WT" + +echo "panel interrupted verdict:" +reset +cat > "$TEST_ROOT/interrupted.json" <<'JSON' +{"runId":"wf_stub","result":{"verdict":"interrupted","findings":[],"coverage":[],"lenses":[],"explanation":"the lens-free sweep did not complete"}} +JSON +rc=0; CODEX_REVIEW_PANEL=always STUB_PANEL_FILE="$TEST_ROOT/interrupted.json" \ + "$ENGINE" --base origin/main --out "$TEST_ROOT/out.txt" 2>"$TEST_ROOT/err.txt" || rc=$? +assert_equals "$rc" "4" "interrupted panel verdict is an engine failure (rc 4)" +assert_contains "$(cat "$TEST_ROOT/err.txt")" "panel interrupted: the lens-free sweep did not complete" \ + "interruption reason surfaces on stderr" echo "rc passthrough:" reset -rc=0; env -u CODEX_HOME STUB_CODEX_RC=3 "$ENGINE" --base origin/main --out "$TEST_ROOT/out.txt" || rc=$? -assert_equals "$rc" "3" "codex rc passes through" +rc=0; STUB_RC=3 "$ENGINE" --base origin/main --out "$TEST_ROOT/out.txt" || rc=$? +assert_equals "$rc" "3" "single-route rc passes through" +rc=0; CODEX_REVIEW_PANEL=always STUB_RC=3 "$ENGINE" --base origin/main --out "$TEST_ROOT/out.txt" || rc=$? +assert_equals "$rc" "3" "panel-route rc passes through" assert_equals "$(find "$TMPDIR" -maxdepth 1 -name 'review-engine-home.*' | wc -l | tr -d ' ')" "0" "engine home removed even on failure" +echo "sandbox fail-closed:" +reset +rc=0; STUB_EVENTS='bwrap: loopback: Failed RTM_NEWADDR: Operation not permitted' \ + env -u CODEX_SANDBOX "$ENGINE" --base origin/main --out "$TEST_ROOT/out.txt" 2>/dev/null || rc=$? +assert_equals "$rc" "3" "sandbox-failure markers in the events log fail the run closed" + echo "usage errors:" rc=0; "$ENGINE" --base origin/main 2>/dev/null || rc=$? assert_equals "$rc" "2" "missing --out is a usage error" @@ -122,9 +210,15 @@ rc=0; "$ENGINE" --out "$TEST_ROOT/out.txt" 2>/dev/null || rc=$? assert_equals "$rc" "2" "missing --base is a usage error" rc=0; "$ENGINE" --base origin/main --criteria "$TEST_ROOT/x.md" --out "$TEST_ROOT/out.txt" 2>/dev/null || rc=$? assert_equals "$rc" "2" "retired --criteria flag is a usage error" +rc=0; CODEX_REVIEW_PANEL=sometimes "$ENGINE" --base origin/main --out "$TEST_ROOT/out.txt" 2>/dev/null || rc=$? +assert_equals "$rc" "2" "unknown CODEX_REVIEW_PANEL value is a usage error" echo "codex missing:" -rc=0; PATH="/usr/bin:/bin" "$ENGINE" --base origin/main --out "$TEST_ROOT/out.txt" 2>/dev/null || rc=$? +# NODE_DIR may be a system bin dir that also carries a real codex — build a +# PATH with node alone so the probe genuinely misses. +NOCODEX="$TEST_ROOT/nocodex"; mkdir -p "$NOCODEX" +ln -s "$(command -v node)" "$NOCODEX/node" +rc=0; PATH="$NOCODEX:/usr/bin:/bin" "$ENGINE" --base origin/main --out "$TEST_ROOT/out.txt" 2>/dev/null || rc=$? assert_equals "$rc" "127" "missing codex CLI exits 127" echo From 5a96d36c47b5b2dac6b8483ff5374d6e2449c29e Mon Sep 17 00:00:00 2001 From: SSFSKIM Date: Mon, 10 Aug 2026 20:40:03 +0900 Subject: [PATCH 02/32] chore: bump plugin version to 7.46.0 (review-engine panel routing) --- .claude-plugin/marketplace.json | 2 +- .claude-plugin/plugin.json | 2 +- .codex-plugin/plugin.json | 2 +- package.json | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/.claude-plugin/marketplace.json b/.claude-plugin/marketplace.json index 49ffd3dd67..e704a5fb32 100644 --- a/.claude-plugin/marketplace.json +++ b/.claude-plugin/marketplace.json @@ -9,7 +9,7 @@ { "name": "doperpowers", "description": "Emerges by humans, Converges by Agent", - "version": "7.45.0", + "version": "7.46.0", "source": "./", "author": { "name": "SSFSKIM", diff --git a/.claude-plugin/plugin.json b/.claude-plugin/plugin.json index a70ecec392..ae11c7d440 100644 --- a/.claude-plugin/plugin.json +++ b/.claude-plugin/plugin.json @@ -1,7 +1,7 @@ { "name": "doperpowers", "description": "Emerges by humans, Converges by Agents", - "version": "7.45.0", + "version": "7.46.0", "author": { "name": "SSFSKIM", "email": "supremekim17@gmail.com" diff --git a/.codex-plugin/plugin.json b/.codex-plugin/plugin.json index a59cd80110..a2f0a815bd 100644 --- a/.codex-plugin/plugin.json +++ b/.codex-plugin/plugin.json @@ -1,6 +1,6 @@ { "name": "doperpowers", - "version": "7.45.0", + "version": "7.46.0", "description": "A two-track software-development methodology for coding agents: a human-gated controlled track (brainstorm, plan, TDD, review, ship) plus an autonomous board loop for unattended, well-scoped work.", "author": { "name": "SSFSKIM", diff --git a/package.json b/package.json index 0612799480..b9af416d3c 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "doperpowers", - "version": "7.44.0", + "version": "7.46.0", "description": "Doperpowers skills and runtime bootstrap for coding agents", "type": "module", "main": ".opencode/plugins/doperpowers.js", From def7635cfd0837cb5bea06fdf10d3d5fba948643 Mon Sep 17 00:00:00 2001 From: SSFSKIM Date: Mon, 10 Aug 2026 20:42:33 +0900 Subject: [PATCH 03/32] fix(reviewing-prs): keep the IN THE BACKGROUND phrase contract contiguous --- skills/reviewing-prs/SKILL.md | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/skills/reviewing-prs/SKILL.md b/skills/reviewing-prs/SKILL.md index 8b4bbc8c4c..8459e0428e 100644 --- a/skills/reviewing-prs/SKILL.md +++ b/skills/reviewing-prs/SKILL.md @@ -146,9 +146,10 @@ is a TOOL invocation, not a nested agent. Never add remove that directory before ending the turn — EXCEPT a needs-human park: wave boards live there and the resumed turn reads them. -2. From the worktree root, start the round's ONE engine run IN THE - BACKGROUND (round N uses findings-rN.txt; the empty lens assignments - are deliberate — they shield the run from any inherited host value): +2. From the worktree root, start the round's ONE engine run + IN THE BACKGROUND (round N uses findings-rN.txt; the empty lens + assignments are deliberate — they shield the run from any inherited + host value): CODEX_REVIEW_MODEL={{CODEX_REVIEW_MODEL}} \ CODEX_REVIEW_EFFORT={{CODEX_REVIEW_EFFORT}} \ From 226b10b75a89c02869356c8f537e1a54b7e1d1c0 Mon Sep 17 00:00:00 2001 From: SSFSKIM Date: Mon, 10 Aug 2026 20:55:42 +0900 Subject: [PATCH 04/32] =?UTF-8?q?fix(reviewing-prs):=20review=20round=20?= =?UTF-8?q?=E2=80=94=20forward=20sandbox=20diagnostics=20through=20workflo?= =?UTF-8?q?w=20leaves=20+=20doc/test=20hardening?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 .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). --- .../runtime/scripts/lib/workflow/engine.mjs | 19 ++++++++++++++ skills/reviewing-prs/SKILL.md | 5 +++- .../references/operation-manual.md | 2 +- tests/codex-companion/mock/codex | 9 ++++++- tests/codex-companion/test-engine-hooks.mjs | 25 +++++++++++++++++++ tests/reviewing-prs/test-review-engine.sh | 1 + 6 files changed, 58 insertions(+), 3 deletions(-) diff --git a/skills/codex-companion/runtime/scripts/lib/workflow/engine.mjs b/skills/codex-companion/runtime/scripts/lib/workflow/engine.mjs index 56f53a0840..1d4eadbefb 100644 --- a/skills/codex-companion/runtime/scripts/lib/workflow/engine.mjs +++ b/skills/codex-companion/runtime/scripts/lib/workflow/engine.mjs @@ -243,6 +243,22 @@ export async function runWorkflow(spec) { return { ...stamped, uncacheable: runNonce }; }; + // The app-server buffers its child's stderr and surfaces it only on a + // nonzero exit, but the observed fs-sandbox failure (bwrap RTM_NEWADDR — + // ida-worker-1, 2026-08-09) exits 0 and renders findings anyway. + // Consumers' fail-closed guards (reviewing-prs review-engine.sh) read this + // run's stderr stream for those markers, so forward them from every leaf's + // buffered stderr — dropping them here hides exactly the false-clean runs + // the guards exist to reject. Forwarded BEFORE the leaf's own success + // check, so a failing turn keeps its diagnostics too. + const SANDBOX_FAILURE_MARKERS = /RTM_NEWADDR|shell is unavailable|fs sandbox helper failed/; + const emitSandboxDiagnostics = (label, stderr) => { + if (!stderr) return; + for (const line of String(stderr).split("\n")) { + if (SANDBOX_FAILURE_MARKERS.test(line)) emit(`sandbox-diagnostic ${label ?? ""}: ${line}`); + } + }; + const hooks = { args: spec.args, log: (m) => { appendEvent(journalPath, { type: "log", message: String(m) }); emit(`log ${m}`); }, @@ -256,6 +272,7 @@ export async function runWorkflow(spec) { sandbox: "read-only", persistThread: true, outputSchema: opts.schema ?? null, connect }); + emitSandboxDiagnostics(opts.label, turn.stderr); assertTurnUsable(turn, "agent turn"); if (!opts.schema) { if (!turn.finalMessage?.trim()) throw new Error("agent turn returned no output"); @@ -270,6 +287,7 @@ export async function runWorkflow(spec) { model: opts.model, effort: opts.effort, sandbox: "read-only", outputSchema: opts.schema, connect }); + emitSandboxDiagnostics(opts.label, repair.stderr); assertTurnUsable(repair, "schema repair turn"); parsed = parseStructuredOutput(repair.finalMessage); errors = parsed.parseError ? [parsed.parseError] : validateSchema(parsed.parsed, opts.schema); @@ -316,6 +334,7 @@ export async function runWorkflow(spec) { model: opts.model, target, connect: { disableBroker: true, configOverrides: overrides, onSpawn } }); + emitSandboxDiagnostics(opts.label, res.stderr); assertTurnUsable(res, "review"); if (!res.reviewText?.trim()) throw new Error("review returned no output"); // review/start takes the SYMBOLIC target — it has no commit parameter — diff --git a/skills/reviewing-prs/SKILL.md b/skills/reviewing-prs/SKILL.md index 8459e0428e..d41ba3bf21 100644 --- a/skills/reviewing-prs/SKILL.md +++ b/skills/reviewing-prs/SKILL.md @@ -163,7 +163,10 @@ is a TOOL invocation, not a nested agent. Never add scalpel lenses, one binding verifier — instead of a single native review, and renders the verifier-confirmed findings into the same --out file (raw panel result beside it as findings-rN.txt.panel.json; - a panel round can take ~20 minutes). Use your harness's background + a panel round can take ~20 minutes). One judgment stays yours: a diff + below that size whose weight concentrates on declared risk surfaces + (your dispatch manifest) can warrant the panel anyway — add + CODEX_REVIEW_PANEL=always to the command to force it. Use your harness's background execution and keep the task handle. Leave it running and the findings unread — the protocol's COMPLIANCE AUDIT runs while the engine reviews, and its JOIN step is the only place engine output is read. diff --git a/skills/reviewing-prs/references/operation-manual.md b/skills/reviewing-prs/references/operation-manual.md index f659a642bf..2d6e03725e 100644 --- a/skills/reviewing-prs/references/operation-manual.md +++ b/skills/reviewing-prs/references/operation-manual.md @@ -27,7 +27,7 @@ Full design + rationale: `docs/doperpowers/specs/2026-07-08-pr-review-loop-desig | piece | what | |---|---| | `scripts/review-dispatch.sh \| --sweep` | mechanical trigger: dedupe → PR + ticket context → detached worktree at the PR head SHA → spawn a `review-pr-` daemon (`daemon-spawn.sh --no-wait`; default route is plain Claude models, `engine:codex` opts into the clodex gateway settings) → exclusively bind it to the primary ticket under the registry lock → complete a dispatcher-ready / worker-ack startup barrier so `board-answer.sh` reaches the parked reviewer and no review action races binding | -| `scripts/review-engine.sh` | the ONE native-review invocation, pure correctness: `--base` + `--out`, env recipe only — no ticket/spec input of any kind. Drives the doperpowers:codex-companion runtime (per-run effort via its with-effort wrapper). The worker may run it 1–4× in parallel per round (its judgment, by diff scale); extra runs carry `CODEX_REVIEW_LENS` — a diff-derived structural focus mandate that routes the run through the `adversarial-review` verb as its focus text | +| `scripts/review-engine.sh` | the ONE native-review invocation, pure correctness: `--base` + `--out`, env recipe only — no ticket/spec input of any kind. Drives the doperpowers:codex-companion runtime (per-run effort via its with-effort wrapper). The worker runs it once per round; diff-size scaling is the engine's own — a big diff routes internally to the companion's code-review panel (sweep + diff-derived scalpel lenses + binding verifier), smaller diffs run the single native `review` verb | | `SKILL.md` | the Review Worker Protocol — invoked by every review worker; the dispatch bootstrap supplies its `{{PLACEHOLDERS}}` as runtime bindings. The engine-start and engine-fallback text live in its START ENGINE section; the worker reads PR and ticket bodies live via gh (only the BASE-ref manifest snapshots ride the prompt) | | `references/wave-board.md` | runtime-opened fix-wave companion: board-file schema, the fixer's verify-then-fix contract, disposition grading | | `references/pr-review-dispatch.yml` | GH workflow template: PR events → self-hosted runner → dispatch script. No checkout, no token permissions | diff --git a/tests/codex-companion/mock/codex b/tests/codex-companion/mock/codex index 2fba32a90a..5c3468030c 100755 --- a/tests/codex-companion/mock/codex +++ b/tests/codex-companion/mock/codex @@ -2,7 +2,7 @@ // Mock codex app-server. Speaks the REAL JSON-RPC-over-stdio shapes // (derived from lib/codex.mjs captureTurn + app-server.mjs — see the // PROTOCOL notes below). Controlled by CODEX_MOCK_DIR: -// scenario.json { turns: [ {finalMessage|finalRaw|die|dieMidTurn|banner|hangMs|reviewText|turnStatus}... ], +// scenario.json { turns: [ {finalMessage|finalRaw|die|dieMidTurn|banner|stderrLine|hangMs|reviewText|turnStatus}... ], // byLens: { "": {same behavior} } } // `turns` is consumed in global order via a counter file (mkdir // lock); turnStatus: "failed" ⇒ emit turn/completed with the @@ -227,6 +227,13 @@ rl.on("line", (line) => { process.stdout.write(`${b.banner}\n`); } + // stderrLine: a line on the server's STDERR mid-turn — the real server's + // sandbox helper complains there (bwrap RTM_NEWADDR et al.) while the turn + // still exits 0; the client buffers it into client.stderr. + if (b.stderrLine) { + process.stderr.write(`${b.stderrLine}\n`); + } + const finish = () => { send({ method: "turn/started", params: { threadId, turn: turn(turnId, "inProgress") } }); if (isReview) { diff --git a/tests/codex-companion/test-engine-hooks.mjs b/tests/codex-companion/test-engine-hooks.mjs index 0421c8390a..b9faba621c 100644 --- a/tests/codex-companion/test-engine-hooks.mjs +++ b/tests/codex-companion/test-engine-hooks.mjs @@ -296,6 +296,31 @@ const spawnRecord = (mockDir, pid) => JSON.parse(fs.readFileSync(path.join(mockD assert.deepEqual(workers(c.runDir), [], "no pids left tracked"); } +// --- sandbox diagnostics on the server's stderr reach emit ------------------- +// The app-server buffers stderr and surfaces it only on a nonzero exit, but the +// observed fs-sandbox failure (bwrap RTM_NEWADDR) exits 0 and renders findings +// anyway; consumers' fail-closed guards grep the run's stderr stream for the +// markers, so the hooks must forward them from the buffered result. +{ + const c = newCase("sandbox-diag", [{ + reviewText: "# Sweep\n\nlooks clean", + stderrLine: "bwrap: loopback: Failed RTM_NEWADDR: Operation not permitted" + }]); + const lines = []; + const out = await runWorkflow({ + scriptPath: path.join(FIXTURES, "fx-review.mjs"), + args: {}, + cwd: c.repo, + runDir: c.runDir, + emit: (l) => lines.push(l) + }); + assert.equal(out.result.reviewText, "# Sweep\n\nlooks clean", "the leaf itself still succeeds"); + assert.ok( + lines.some((l) => l.startsWith("sandbox-diagnostic sweep:") && l.includes("RTM_NEWADDR")), + "the sandbox-failure marker from the worker's buffered stderr is forwarded to emit" + ); +} + // --- a review whose turn failed is not a review ------------------------------ { const c = newCase("review-fail", [ diff --git a/tests/reviewing-prs/test-review-engine.sh b/tests/reviewing-prs/test-review-engine.sh index dd74f7e2b9..5e118f140c 100755 --- a/tests/reviewing-prs/test-review-engine.sh +++ b/tests/reviewing-prs/test-review-engine.sh @@ -161,6 +161,7 @@ assert_equals "$([ -s "$TEST_ROOT/out.txt.panel.json" ] && echo yes)" "yes" "raw echo "panel auto-threshold (real repo):" REPO="$TEST_ROOT/repo"; mkdir -p "$REPO"; cd "$REPO" git init -q -b main; git config user.email t@t; git config user.name t +git config commit.gpgsign false # host git policy must not break the hermetic suite echo base > seed.txt; git add .; git commit -qm seed git checkout -qb feature for i in $(seq 1 21); do echo "change $i" > "f$i.txt"; done From 87b71ca4de8b5ea1bed7ca5a70f34ee35e623d18 Mon Sep 17 00:00:00 2001 From: SSFSKIM Date: Mon, 10 Aug 2026 21:16:50 +0900 Subject: [PATCH 05/32] =?UTF-8?q?fix(reviewing-prs):=20panel=20dogfood=20r?= =?UTF-8?q?ound=20=E2=80=94=20fail-closed=20renderer,=20throw-path=20sandb?= =?UTF-8?q?ox=20forwarding,=20honest=20lane=20rendering?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../runtime/scripts/lib/workflow/engine.mjs | 5 +++ .../scripts/render-panel-findings.mjs | 43 ++++++++++++++----- tests/codex-companion/mock/codex | 14 +++--- tests/codex-companion/test-engine-hooks.mjs | 24 +++++++++++ tests/reviewing-prs/test-review-engine.sh | 16 ++++++- 5 files changed, 84 insertions(+), 18 deletions(-) diff --git a/skills/codex-companion/runtime/scripts/lib/workflow/engine.mjs b/skills/codex-companion/runtime/scripts/lib/workflow/engine.mjs index 1d4eadbefb..1ff52bd7a4 100644 --- a/skills/codex-companion/runtime/scripts/lib/workflow/engine.mjs +++ b/skills/codex-companion/runtime/scripts/lib/workflow/engine.mjs @@ -181,6 +181,10 @@ export async function runWorkflow(spec) { out = await attempt(); } catch (e1) { if (e1?.terminal) throw e1; // schema-repair exhaustion etc: NEVER a third attempt + // A dead app-server's protocol error carries the child's buffered + // stderr — forward sandbox markers from it too, or a dying worker's + // diagnostics never reach the consumers' fail-closed guards. + emitSandboxDiagnostics(label, String(e1?.message ?? e1)); appendEvent(journalPath, { type: "retry", key, error: String(e1?.message ?? e1) }); emit(`retry ${kind}:${label ?? ""}`); out = await attempt(); // one automatic transport retry, fresh turn @@ -189,6 +193,7 @@ export async function runWorkflow(spec) { emit(`done ${kind}:${label ?? ""}`); return out; } catch (err) { + emitSandboxDiagnostics(label, String(err?.message ?? err)); appendEvent(journalPath, { type: "finished", key, error: String(err?.message ?? err) }); emit(`fail ${kind}:${label ?? ""}`); throw err; diff --git a/skills/reviewing-prs/scripts/render-panel-findings.mjs b/skills/reviewing-prs/scripts/render-panel-findings.mjs index ca67cfd222..31ca15f8c5 100644 --- a/skills/reviewing-prs/scripts/render-panel-findings.mjs +++ b/skills/reviewing-prs/scripts/render-panel-findings.mjs @@ -4,32 +4,55 @@ // the rendered-findings text the reviewing-prs worker reads at JOIN, so the // panel route and the single-review route land in --out with one contract. // -// `interrupted` exits 4: the panel withheld its verdict (lost lane, no -// contract-valid verifier set, or the reviewed head moved), so there is no -// findings file to write — the caller treats it like any other engine -// failure (retry, then the ENGINE-UNAVAILABLE fallback). Any partial +// FAIL CLOSED on anything but a well-formed verdict: the worker derives its +// approval from unresolved critical/high findings, so a malformed result +// rendered as "0 findings" would read as clean. `interrupted` and every +// malformed/inconsistent shape exit 4 — the caller treats that like any +// engine failure (retry, then the ENGINE-UNAVAILABLE fallback). Any partial // evidence stays in the .panel.json beside --out for the review trail. // // Usage: render-panel-findings.mjs (findings text on stdout) import { readFileSync } from "node:fs"; -const raw = JSON.parse(readFileSync(process.argv[2], "utf8")); +const refuse = (why) => { console.error(`panel result rejected: ${why}`); process.exit(4); }; + +let raw; +try { + raw = JSON.parse(readFileSync(process.argv[2], "utf8")); +} catch (e) { + refuse(`unreadable panel JSON (${e.message})`); +} const r = raw?.result ?? {}; if (r.verdict === "interrupted") { console.error(`panel interrupted: ${r.explanation ?? "no explanation"}`); process.exit(4); } if (r.verdict !== "correct" && r.verdict !== "incorrect") { - console.error(`panel returned no verdict (got ${JSON.stringify(r.verdict)})`); - process.exit(4); + refuse(`no verdict (got ${JSON.stringify(r.verdict)})`); +} +if (!Array.isArray(r.findings)) refuse("findings is not an array"); +for (const f of r.findings) { + if (typeof f?.priority !== "string" || typeof f?.title !== "string" || typeof f?.file !== "string") { + refuse(`malformed finding ${JSON.stringify(f).slice(0, 120)}`); + } +} +// The panel constructs the verdict FROM the findings (incorrect ⟺ confirmed +// findings exist), so a disagreement here is a mangled result, not a verdict. +if ((r.verdict === "incorrect") !== (r.findings.length > 0)) { + refuse(`verdict ${r.verdict} inconsistent with ${r.findings.length} findings`); } -const findings = r.findings ?? []; -const lenses = (r.lenses ?? []).length; +const findings = r.findings; const out = []; +// Participation from COVERAGE (what actually ran), never from the configured +// lens list — a dead lane must not be rendered as a participant in the trail. +const coverage = Array.isArray(r.coverage) ? r.coverage : []; +const lanes = coverage.length + ? ` (lanes: ${coverage.map((c) => `${c.finder} ${c.status}`).join(", ")})` + : ""; out.push( `Panel verdict: ${r.verdict} — ${findings.length} verifier-confirmed ` + - `finding${findings.length === 1 ? "" : "s"} (sweep + ${lenses} lens${lenses === 1 ? "" : "es"} + verifier)` + `finding${findings.length === 1 ? "" : "s"}${lanes}` ); if (r.explanation) out.push("", String(r.explanation).trim()); for (const f of findings) { diff --git a/tests/codex-companion/mock/codex b/tests/codex-companion/mock/codex index 5c3468030c..32cc594417 100755 --- a/tests/codex-companion/mock/codex +++ b/tests/codex-companion/mock/codex @@ -187,6 +187,13 @@ rl.on("line", (line) => { fs.appendFileSync(path.join(MOCK, "turns.jsonl"), JSON.stringify({ method: msg.method, params: msg.params, pid: process.pid }) + "\n"); const b = lensBehavior() ?? nextTurnBehavior(); + // stderrLine: a line on the server's STDERR — the real server's sandbox + // helper complains there (bwrap RTM_NEWADDR et al.); written before `die` + // so a dying server can leave the diagnostic behind, exactly like the + // real failure. The client buffers it into client.stderr either way. + if (b.stderrLine) { + fs.writeSync(2, `${b.stderrLine}\n`); + } if (b.die) process.exit(1); const isReview = msg.method === "review/start"; @@ -227,13 +234,6 @@ rl.on("line", (line) => { process.stdout.write(`${b.banner}\n`); } - // stderrLine: a line on the server's STDERR mid-turn — the real server's - // sandbox helper complains there (bwrap RTM_NEWADDR et al.) while the turn - // still exits 0; the client buffers it into client.stderr. - if (b.stderrLine) { - process.stderr.write(`${b.stderrLine}\n`); - } - const finish = () => { send({ method: "turn/started", params: { threadId, turn: turn(turnId, "inProgress") } }); if (isReview) { diff --git a/tests/codex-companion/test-engine-hooks.mjs b/tests/codex-companion/test-engine-hooks.mjs index b9faba621c..31e918bea4 100644 --- a/tests/codex-companion/test-engine-hooks.mjs +++ b/tests/codex-companion/test-engine-hooks.mjs @@ -321,6 +321,30 @@ const spawnRecord = (mockDir, pid) => JSON.parse(fs.readFileSync(path.join(mockD ); } +// --- sandbox diagnostics survive a leaf that THROWS -------------------------- +// A dying app-server never returns a result to inspect, but its protocol error +// carries the buffered stderr — the retry/fail paths must forward markers from +// the error text, or a dead scalpel hides exactly the diagnostic that matters. +{ + const c = newCase("sandbox-diag-throw", [ + { die: true, stderrLine: "bwrap: loopback: Failed RTM_NEWADDR: Operation not permitted" }, + { finalMessage: "recovered" } + ]); + const lines = []; + const out = await runWorkflow({ + scriptPath: path.join(FIXTURES, "fx-solo.mjs"), + args: {}, + cwd: c.repo, + runDir: c.runDir, + emit: (l) => lines.push(l) + }); + assert.equal(out.result.one, "recovered", "the transport retry still recovers the leaf"); + assert.ok( + lines.some((l) => l.startsWith("sandbox-diagnostic") && l.includes("RTM_NEWADDR")), + "the marker from the dying worker's protocol error is forwarded to emit" + ); +} + // --- a review whose turn failed is not a review ------------------------------ { const c = newCase("review-fail", [ diff --git a/tests/reviewing-prs/test-review-engine.sh b/tests/reviewing-prs/test-review-engine.sh index 5e118f140c..982c2e2e2b 100755 --- a/tests/reviewing-prs/test-review-engine.sh +++ b/tests/reviewing-prs/test-review-engine.sh @@ -88,7 +88,9 @@ else console.log(JSON.stringify({ runId: "wf_stub", result: { findings: [{ id: "sweep#1", priority: "P1", title: "stub panel finding", file: "a.ts", lines: "10-12", comment: "why it is wrong", sources: ["sweep", "lens-1"] }], - coverage: [], lenses: ["l1", "l2"], explanation: "confirmed: stub panel finding" } })); + coverage: [{ finder: "sweep", status: "ok" }, { finder: "lens-1", status: "ok" }, + { finder: "lens-2", status: "worker-failed" }], + lenses: ["l1", "l2"], explanation: "confirmed: stub panel finding" } })); process.exit(Number(e.STUB_RC ?? 0)); STUB export REVIEW_COMPANION_DIR="$COMPANION" @@ -154,6 +156,8 @@ assert_contains "$LOG" '\"finderEffort\":\"xhigh\"' "panel args carry the finder assert_not_contains "$LOG" "WITH_EFFORT" "panel route does not also run a single review" OUT="$(cat "$TEST_ROOT/out.txt")" assert_contains "$OUT" "Panel verdict: incorrect" "panel verdict rendered into --out" +assert_contains "$OUT" "lanes: sweep ok, lens-1 ok, lens-2 worker-failed" \ + "participation is rendered from coverage — a dead lane shows as dead" assert_contains "$OUT" "- [P1] stub panel finding (a.ts:10-12)" "confirmed finding rendered with priority and location" assert_contains "$OUT" "raised independently by: sweep, lens-1" "multi-source findings carry their lanes" assert_equals "$([ -s "$TEST_ROOT/out.txt.panel.json" ] && echo yes)" "yes" "raw panel result kept beside --out" @@ -190,6 +194,16 @@ assert_equals "$rc" "4" "interrupted panel verdict is an engine failure (rc 4)" assert_contains "$(cat "$TEST_ROOT/err.txt")" "panel interrupted: the lens-free sweep did not complete" \ "interruption reason surfaces on stderr" +echo "panel malformed result (fail closed):" +reset +cat > "$TEST_ROOT/malformed.json" <<'JSON' +{"runId":"wf_stub","result":{"verdict":"incorrect"}} +JSON +rc=0; CODEX_REVIEW_PANEL=always STUB_PANEL_FILE="$TEST_ROOT/malformed.json" \ + "$ENGINE" --base origin/main --out "$TEST_ROOT/out.txt" 2>"$TEST_ROOT/err.txt" || rc=$? +assert_equals "$rc" "4" "an incorrect verdict with no findings array is rejected, not rendered clean" +assert_contains "$(cat "$TEST_ROOT/err.txt")" "panel result rejected" "the rejection names itself" + echo "rc passthrough:" reset rc=0; STUB_RC=3 "$ENGINE" --base origin/main --out "$TEST_ROOT/out.txt" || rc=$? From b656b9dec0c66417cf55acb83d49ec0414b1f98b Mon Sep 17 00:00:00 2001 From: SSFSKIM Date: Tue, 11 Aug 2026 02:43:00 +0900 Subject: [PATCH 06/32] =?UTF-8?q?docs:=20ExecPlan=20=E2=80=94=20requesting?= =?UTF-8?q?-review=20skill=20split,=20runtime=20fail-closed,=20engine=20re?= =?UTF-8?q?tirement?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../2026-08-11-requesting-review-split.md | 145 ++++++++++++++++++ 1 file changed, 145 insertions(+) create mode 100644 docs/doperpowers/execplans/2026-08-11-requesting-review-split.md diff --git a/docs/doperpowers/execplans/2026-08-11-requesting-review-split.md b/docs/doperpowers/execplans/2026-08-11-requesting-review-split.md new file mode 100644 index 0000000000..73c76b8362 --- /dev/null +++ b/docs/doperpowers/execplans/2026-08-11-requesting-review-split.md @@ -0,0 +1,145 @@ +# Split codex-companion: a `requesting-review` skill, runtime fail-closed, and a script-free reviewing-prs review step + +This ExecPlan is a living document. The sections `Progress`, `Surprises & Discoveries`, `Decision Log`, and `Outcomes & Retrospective` must be kept up to date as work proceeds. It is maintained in accordance with the vendored PLANS.md contract of the doperpowers:execplan skill (the PLANS.md file itself is not checked into this repository; this document is self-contained and does not depend on it). + +## Purpose / Big Picture + +Today, an agent that wants an independent code review must know about two different layers: the `doperpowers:codex-companion` skill (which documents five verbs of a vendored OpenAI Codex runtime — reviews, delegation, orchestration, job plumbing — in one place) and, inside the autonomous PR-review loop, a bash script (`skills/reviewing-prs/scripts/review-engine.sh`) that wraps the runtime and decides by a hard-coded numstat heuristic whether a diff is big enough to deserve a multi-reviewer panel instead of a single review. That script accreted real machinery: a JSON-flattening renderer, exit-code conventions, environment isolation, and a fail-closed grep for sandbox failures. + +After this change, three things are true that were not before: + +1. There is a new skill, `doperpowers:requesting-review`, that owns the whole *independent quality review* job: when to review, how to choose between a single native review, a steered adversarial review, and the multi-lens panel (judgment guidance with a stated default and its reason — not a script heuristic), the exact invocations, and how to read the results. Any agent — an interactive session, a dispatched review daemon, another skill — reads one skill and can request a review. +2. The vendored runtime itself fails closed on sandbox failure: a review whose probing shell never worked errors out instead of rendering a hollow "clean" verdict. That guarantee used to exist only for callers who went through `review-engine.sh`; now every consumer has it. +3. The reviewing-prs review worker routes by its own judgment following requesting-review's doctrine, invoking the runtime directly. `review-engine.sh`, `render-panel-findings.mjs`, and their test suite are deleted. + +To see it working: run the test suites listed in Validation, and (the merge gate) dispatch one live review worker at a big-diff PR and observe it start the panel — not a single review, not two reviews — purely from reading its protocol. + +## Progress + +- [x] (2026-08-11 00:20Z) Grill complete (brainstorming session); design approved; track = autonomous. +- [x] (2026-08-11 00:40Z) ExecPlan authored and committed. +- [ ] Milestone 1: runtime fail-closed (sandbox.mjs, engine.mjs leaves, review/adversarial verb paths, CODEX_SANDBOX exemption) + tests. +- [ ] Milestone 2: skills/requesting-review/SKILL.md created; codex-companion slimmed (reviews.md deleted, verb list repointed, workflows.md panel section → pointer). +- [ ] Milestone 3: reviewing-prs — engine/renderer/tests deleted; SKILL.md START ENGINE/JOIN/RE-REVIEW/TRAIL rewritten; dispatcher binds COMPANION_DIR; operator docs updated; dispatch + entrypoint tests updated. +- [ ] Milestone 4: consumer repoints (execplan, subagent-driven-development, writing-plans, architecting); user-CLAUDE.md repoint flagged (not edited). +- [ ] Milestone 5: full validation suites + shell lint + residual-grep sweep; version stays 7.46.0. +- [ ] Milestone 6 (merge gate): live dogfood — one dispatched review worker routes a big diff to the panel by protocol alone; PR #55 description rewritten; push. + +## Surprises & Discoveries + +- Observation (planning stage): the incident's sandbox markers ride the app-server child's *buffered stderr* (surfaced per turn as `result.stderr`), while the streamed progress lines also carry a truncated copy of the model's final answer — making progress a false-positive channel (a diff quoting a marker string, e.g. this repo's own tests, would trip a naive scan). + Evidence: `skills/codex-companion/runtime/scripts/lib/workflow/engine.mjs` lines 251–265 (the buffering comment), and `executeReviewRun` in `codex-companion.mjs` returning `result.stderr` verbatim on the native-review branch. +- Observation (planning stage): `executeReviewRun`'s adversarial branch does not carry the turn's stderr into its payload today — fail-closed there needs the field plumbed through first. + Evidence: the adversarial branch builds its result from `runAppServerTurn` without a `stderr` field in the rendered payload (codex-companion.mjs, after line 416). + +## Decision Log + +- Decision: split codex-companion two ways (requesting-review + slimmed codex-companion), not three. + Rationale: `review`, `adversarial-review`, and the panel are one job — independent quality scrutiny of a diff — sharing target-selection flags and output contracts; a separate adversarial-review skill would have exactly two consumers and would split the shared plumbing doc. `task`/amigo is a genuinely different job (delegation/partnership). Named by job, not runtime, so the backing engine stays swappable. Rejected: three-way split (user's original sketch; declined by user after tradeoffs); no split (leaves the discoverability problem). + Date/Author: 2026-08-10, brainstorming session with human. +- Decision: big-diff routing moves from `review-engine.sh` to the review worker's judgment, guided by requesting-review. + Rationale: human directive (golden rule: trust agent judgment; the pre-#55 failure was doctrine living in a doc workers never read, not judgment failing). The worker sees risk surfaces and diff character that a numstat heuristic cannot. Rejected: keep engine-internal routing (PR #55 as built — human explicitly chose against); engine survives as thin executor (once the guard moved into the runtime, nothing load-bearing remained in the script). + Date/Author: 2026-08-10, human via AskUserQuestion. +- Decision: the fail-closed sandbox guard moves into the vendored runtime (workflow engine leaves AND the review/adversarial verb paths), exempted when `CODEX_SANDBOX` is set. + Rationale: a clean verdict from a reviewer whose shell was broken is garbage for every consumer, not just daemons; runtime-level failure protects interactive sessions that today have no guard, and lets the script layer retire. The `CODEX_SANDBOX` exemption carries over from the engine script: under an outer codex sandbox, probe confinement is expected and the degraded diff-only render is documented behavior. Rejected: guard stays script-side (leaves interactive consumers unguarded and blocks the retirement); protocol-level grep instructions (the incident was precisely that nobody looked). + Date/Author: 2026-08-10, human via AskUserQuestion. +- Decision: the runtime guard scans ONLY the app-server child's buffered stderr (`result.stderr` / `turn.stderr`), never `reviewText`/final-message/progress channels; ANY marker hit fails the run (no multi-hit threshold). + Rationale: model-authored channels can quote marker strings innocently (this repo's own tests contain them), so scanning them manufactures false positives; the buffered stderr is machine-emitted. The incident showed markers on every probe, so a single-hit trigger loses nothing there, and a threshold would be an invented knob with no observed failure to calibrate against. This resolves the "sustained threshold" delegated unknown from the design. + Date/Author: 2026-08-11, ExecPlan author. +- Decision: a marker-triggered leaf failure in the workflow engine is `terminal: true` (no transport retry). + Rationale: a host that blocks the sandbox (userns denied) does not heal between retries; the retry would spend a full review turn to reach the same failure. The panel's own composition then yields `interrupted` (lost lane), which is the correct verdict shape. + Date/Author: 2026-08-11, ExecPlan author. +- Decision: version stays 7.46.0; same branch, same PR (#55), description rewritten. + Rationale: 7.46.0 was bumped on this branch and never released; the rework replaces unmerged work. Rejected: merge #55 first then follow-up (human chose same-branch rework — no interim merge of machinery about to be deleted). + Date/Author: 2026-08-10, human via AskUserQuestion. +- Decision: the worker's per-round review environment preamble (temp `CODEX_HOME` + auth symlink, temp `CLAUDE_PLUGIN_DATA`, `SSL_CERT_FILE` bundle fallback, `CODEX_CODE_MODE_HOST_PATH` fallback) lives as an indented template inside reviewing-prs SKILL.md's START ENGINE step. + Rationale: it is daemon-environment knowledge (outer seatbelt makes `~/.codex` read-only; nested codex cannot reach trustd), not review doctrine — so it belongs to the loop's protocol, not to requesting-review, and a helper script would resurrect the layer being retired. Rejected: dispatcher-exported env (a resumed worker whose dispatcher is gone could not recreate it). + Date/Author: 2026-08-11, ExecPlan author. +- Decision: skill/protocol prose is authored to the root-CLAUDE.md "Authoring agent behavior" standard and the repo golden rule — generalized principles with reasons, fewest hard gates, every line earning its place; incident citations stay only where they license a hard constraint (the fail-closed guard). + Rationale: explicit human directive at track handoff. + Date/Author: 2026-08-11, human. + +## Outcomes & Retrospective + +Pending — written at finish. + +## Context and Orientation + +This repository (`doperpowers`) is a Claude Code plugin: a collection of *skills* — markdown protocol documents under `skills//SKILL.md` that agents load by name — plus supporting scripts and a vendored copy of OpenAI's Codex app-server client. Everything below is relative to the repository root (the git worktree you are in). + +Key parts: + +- `skills/codex-companion/` — the skill wrapping the vendored Codex runtime. `SKILL.md` lists verbs; `references/reviews.md` documents the `review` and `adversarial-review` verbs; `references/workflows.md` documents the `workflow` orchestration verb and its bundled code-review panel; `references/amigo.md` (the `task` verb) and `references/jobs.md` (backgrounding) stay untouched. The runtime lives in `runtime/scripts/` — entry point `codex-companion.mjs`, app-server client `lib/codex.mjs` and `lib/app-server.mjs`, workflow engine `lib/workflow/engine.mjs`. `scripts/with-effort.mjs` wraps a verb invocation with a private app-server carrying a reasoning-effort override. `workflows/code-review.mjs` is the bundled review panel (one lens-free sweep + up to five diff-derived scalpel lenses + one binding verifier; result `{verdict, findings, coverage, lenses, explanation}` where `verdict` ∈ `correct`/`incorrect`/`interrupted`). +- `skills/reviewing-prs/` — the autonomous PR-review loop. `SKILL.md` is the pinned protocol a dispatched review daemon follows; `scripts/review-dispatch.sh` spawns workers and macro-expands `{{...}}` bindings into the protocol; `scripts/review-engine.sh` (TO BE DELETED) wraps the runtime; `scripts/render-panel-findings.mjs` (TO BE DELETED) flattens panel JSON to text; `references/operation-manual.md` and `references/runner-setup.md` are operator docs. +- `tests/` — shell/node suites: `tests/codex-companion/` (runtime; `run-codex-companion-tests.sh` and `run-workflow-tests.sh` are the runners; `mock/codex` is a scenario-driven fake codex supporting a `stderrLine` behavior that writes a line to fd 2), `tests/reviewing-prs/` (`test-review-dispatch.sh`, `test-review-engine.sh` (TO BE DELETED), `test-skill-entrypoint.sh` which asserts protocol phrases — including that `IN THE BACKGROUND` appears contiguously in START ENGINE), `scripts/lint-shell.sh` (shellcheck baseline). +- Version manifests are bumped ONLY via `scripts/bump-version.sh`; this branch already carries 7.46.0 and keeps it. + +Terms: a *sandbox-failure marker* is any of the three substrings `RTM_NEWADDR`, `shell is unavailable`, `fs sandbox helper failed` appearing in the Codex app-server child's stderr — the observed signature (ida-worker-1, 2026-08-09) of a host whose filesystem sandbox never worked while codex still exited 0 and rendered findings (22 consecutive false-clean runs). *Fail closed* means: on that signature, produce an error, never a verdict. `CODEX_SANDBOX` set in the environment means we are already nested under an outer codex sandbox, where probe confinement is expected — the guard stands down there. + +The current state to build from: PR #55's branch, where `review-engine.sh` contains the numstat panel routing (lines 123–157), `engine.mjs` lines 251–265 forward sandbox markers from buffered leaf stderr as `sandbox-diagnostic` events (emission only — no failure), and reviewing-prs SKILL.md's START ENGINE step tells the worker "Diff-size scaling is the ENGINE's, not yours". + +## Plan of Work + +Milestone 1 — runtime fail-closed. Create `skills/codex-companion/runtime/scripts/lib/sandbox.mjs` exporting the marker regex and `assertSandboxUsable(label, stderr, {onDiagnostic})`: scans buffered stderr line-by-line; every hit line is reported through `onDiagnostic` first (so journaling survives the throw); on a hit, when `process.env.CODEX_SANDBOX` is set report only, otherwise throw an Error naming the guard (`sandbox unavailable during