diff --git a/.claude-plugin/marketplace.json b/.claude-plugin/marketplace.json index 45c86435d1..32df8df657 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.46.6", + "version": "7.47.0", "source": "./", "author": { "name": "SSFSKIM", diff --git a/.claude-plugin/plugin.json b/.claude-plugin/plugin.json index 1740b2ff6f..bb1c6f1fd3 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.46.6", + "version": "7.47.0", "author": { "name": "SSFSKIM", "email": "supremekim17@gmail.com" diff --git a/.codex-plugin/plugin.json b/.codex-plugin/plugin.json index d39031fe06..62e12ee4c3 100644 --- a/.codex-plugin/plugin.json +++ b/.codex-plugin/plugin.json @@ -1,6 +1,6 @@ { "name": "doperpowers", - "version": "7.46.6", + "version": "7.47.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/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..a973f4c1de --- /dev/null +++ b/docs/doperpowers/execplans/2026-08-11-requesting-review-split.md @@ -0,0 +1,161 @@ +# 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-10 16:40Z) Grill complete (brainstorming session); design approved; track = autonomous. +- [x] (2026-08-10 16:55Z) ExecPlan authored and committed (b656b9de). +- [x] (2026-08-10 17:25Z) Milestone 1: runtime fail-closed — lib/sandbox.mjs; engine.mjs leaves guard via assertSandboxUsable (terminal); review AND adversarial verb branches assert on result.stderr; with-effort.mjs guards its private server's piped stderr (exit 3) since the socket path hides stderr from the verb's client; CODEX_SANDBOX stands the guard down everywhere. Tests: engine-hooks sandbox cases rewritten (fail-closed + nested-OK), fake codex gained `sandbox-broken` behavior + `-c` tolerance, 4 new runtime.test.mjs cases — all green; full companion + workflow suites green. +- [x] (2026-08-10 17:40Z) Milestone 2: skills/requesting-review/SKILL.md created; codex-companion SKILL.md verb list repointed; references/reviews.md deleted; workflows.md panel section → pointer. +- [x] (2026-08-10 17:56Z) Milestone 3: review-engine.sh, render-panel-findings.mjs, test-review-engine.sh deleted; SKILL.md START ENGINE rewritten (worker routes per requesting-review; env preamble template; findings-rN.txt/.json; interrupted retries once); ENGINE FALLBACK covers sandbox rejection + twice-interrupted panel; dispatcher binds COMPANION_DIR (3 sites); bootstrap binding renamed; operation-manual rewritten; dispatch + entrypoint suites updated and green. +- [x] (2026-08-10 18:10Z) Milestone 4: consumer repoints (execplan, subagent-driven-development, writing-plans, architecting) committed 214a496f; review-bench's codex path moved to the with-effort invocation; user-CLAUDE.md repoint flagged for the final report, not edited. +- [x] (2026-08-10 18:40Z) Milestone 5: companion, workflow, dispatch, entrypoint, shell-lint suites all rc=0; residual grep clean (one intentional "is retired" note in review-bench); run-skill-tests.sh shows 6 pre-existing ARKHO_DIR-gated integration skips the runner counts as failures (unrelated to this branch — runner has no exit-77 accounting) plus one LLM-phrasing flake in the SDD test that passed on rerun. Version stays 7.46.0. +- [x] (2026-08-10 22:10Z) Milestone 6 (merge gate): PASSED — worker `review-pr-55` (dispatched 18:20Z, auto-merge off) chose the panel from protocol reading alone on the 26-file diff and composed the documented invocation exactly; it then ran the FULL loop: 5 panel rounds, 5 waves, 21 graded commits pushed (d58d85a5→79a69dad), trail comment posted 21:34Z. Review commits pulled; origin/main merged (4 version-manifest conflicts only, resolved to main); version re-bumped 7.47.0 per the reviewer's staleness finding; PR #55 description rewritten; pushed. + +## 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, CORRECTED at M1): I first noted the adversarial branch drops the turn's stderr — wrong on re-read: its payload carries `codex.stderr` (codex-companion.mjs line ~440). Both verb branches only needed the assert added. + Evidence: `payload.codex.stderr` present in both branches; M1 diff adds only the two `assertSandboxUsable` calls. +- Observation (M1): the verb's default path and the with-effort path talk to the app-server over a socket, where the child's stderr never reaches the verb's client — `result.stderr` is populated only on the direct (dead-endpoint kill-switch) path. The guard therefore lives in THREE places: the workflow engine's leaves (disableBroker → direct), the verb branches (direct path), and with-effort.mjs itself, which pipes and scans its private server's stderr and exits 3 on a marker after a clean verb exit. + Evidence: first new runtime test failed with exit 0 until the kill-switch env was added; with-effort test passes end-to-end against the `sandbox-broken` fake. + +- Observation (M6 dogfood): the stderr-only scanning decision was validated LIVE, in the negative — a wave-4 fixer extended the guard to the workflow engine's throw path (error text), the very next panel round's scalpel-1 lane died because codex's command-policy rejection embedded model-authored text quoting this repo's own `RTM_NEWADDR` test fixtures, and the worker reverted it with cross-round evidence (rounds 1–4: 0 diagnostics, 0 lost lanes; round 5: 3 and 1). The throw path's error is a union of machine stderr and arbitrary server text (`app-server.mjs:314` vs `:165`) and can never be treated as machine-emitted. + Evidence: PR #55 trail comment ("The correction I have to lead with"), revert 79a69dad. +- Observation (M6 dogfood): three seams produced findings in round after round — the guard's per-transport observation channels, engine-home cleanup, and template shell-safety. The trail names this a decomposition defect: the guard must observe a channel three transports expose differently, and patching call sites one at a time is what produced five rounds. A follow-up design pass should unify the observation channel. Deferred findings (broker-path guard gap, stderr snapshot races, unquoted template paths, inert `--background`) are LOGGED inline in the trail. + Evidence: trail "The pattern worth more than any single finding" + Deferred section. +- Observation (M6 dogfood): behavior evidence for the skill rewrite — the dispatched worker followed the new protocol end to end with zero protocol errors attributable to the prose: correct route choice, correct env preamble, correct JSON reading at JOIN, interrupted-retry honored (round 3's first attempt was SIGTERMed and retried), observation-mode close. The 5-round loop also pressure-tested requesting-review's own text: 6 of the 20 wave fixes were to MY skill prose (promised-but-unset env var, `--out` flag that no longer exists, over-broad target-selection claim, missing CODEX_SANDBOX caveat, setup-verb contradiction, overstated interrupted conditions) — all caught by the panel reading the docs against the code. + Evidence: trail Waves 1–4 tables; commits 82793c52, 9c7a11bb, 548828a6, a831ba90, 034cb0ab, 8b81b162. + +## 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 + +Completed 2026-08-10 ~22:20Z (calendar 2026-08-11 local). All six milestones landed. The review job now lives in one skill (`requesting-review`, 120 lines) that every consumer — interactive session, daemon worker, other skills — reads directly; the engine script layer (182-line engine + 74-line renderer + 384-line test suite) is deleted; the fail-closed sandbox guarantee moved to the runtime and got *stronger* (stderr-only scanning removed the quoted-marker false-positive class the old events-log grep carried — proven live when the dogfood's own wave briefly reintroduced the class and the next round caught it). + +The merge-gate dogfood exceeded its brief: the dispatched worker not only routed to the panel by protocol alone but ran the full 5-round/5-wave loop against this branch, contributing 21 graded commits — including 6 corrections to the new skill prose itself and a caught-and-reverted regression. That run IS the behavior evidence CLAUDE.md's skill-change bar asks for. + +Gaps, stated plainly: the "every consumer" fail-closed claim holds for the direct, wrapper, and workflow paths, NOT the broker path (unreachable via the documented contract but real — logged as a deferred P1); two stderr-timing races remain accepted-by-design; and the guard-channel-per-transport pattern is a decomposition defect deserving its own design pass (the trail's strongest recommendation). Lessons: (1) moving a guard from wrapper to source is what frees the wrapper's deletion; (2) a protocol template's env preamble must be carried over *verbatim with its reasons* — every line I paraphrased or dropped (CODE_MODE_HOST_PATH, cleanup trap, isolated bench env) came back as a finding; (3) the panel reviewing prose-against-code catches skill-doc lies a suite cannot. + +## 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