redesign(reviewing-prs): orchestrator worker with fix waves on one harness - #16
Merged
Conversation
…emons DAEMON_CLAUDE_SETTINGS/DAEMON_CLAUDE_EFFORT ride the spawn argv, persist in the registry meta, and are reconstructed on every resume fork - without the round-trip a gateway daemon silently reverts to plain models on its first resume.
review-engine.sh is pure correctness review: --base + --out only. Criteria parsing/validation and the developer_instructions construction are gone; the env recipe (temp CODEX_HOME, auth link, TLS, code-mode host, nested sandbox detection) is byte-preserved. The engine block now starts the engine in the background with a 45-minute bounded JOIN; compliance review moves to the worker protocol (next commit).
The review worker never fixes: it orients read-only, starts the pure engine in the background, records its compliance audit (three classes: PROTOCOL BLOCKER / SPEC FINDING / AUDIT NOTE; timestamp-anchored drift via GitHub edit history; gate-comment-keyed evidence rule) before JOIN, triages on native severity without reading code, and delegates fixing to one fixer subagent per wave through a .doperpowers/qa wave board (references/wave-board.md: strict-JSON schema, verify-then-fix contract, disposition grading, empty-slot = failed item). Max 2 waves inside the 3-round cap; push strips confident-ready in-loop; ticketless TOO BIG, TECH_DEBT_ISSUE=none, and secondary-issue dead ends get explicit routes. IMPLEMENT_PROTOCOL_FILE joins SKILL_FILE as a dispatcher-owned binding. Entrypoint tests rewritten structure-asserting (headings, tokens, exact placeholder set) instead of sentence-pinning.
…e cap codex-spawn.sh no longer spawns review workers: the default codex engine is a GATEWAY route — the same Claude-harness daemon with the clodex settings (DAEMON_CLAUDE_SETTINGS/EFFORT; model fable) — and engine:claude spawns plain Claude models. Old codex registry metas stay readable in dedupe. The cron sweep stops respawning a PR after 3 consecutive ENGINE-UNAVAILABLE reviewers; an explicit PR event always re-dispatches.
…rebuild Operation manual: split-responsibility engine/audit section, new orchestrator-and-fix-waves section, sweep outage cap, one-harness pieces table and adoption checklist. Dated Revision Notes appended to the 2026-07-08 loop design and 2026-07-12 native-review recovery specs; the loop design's Purpose paragraph now describes the orchestrator worker. Implement-worker gateway migration stays deferred, recorded in the ExecPlan (fork has GitHub issues disabled).
The retired codex-CLI species was the only worker that ever finalized its registry meta; a --no-wait claude-harness worker stayed status=working forever and _is_live's presence-only check made every finished reviewer read as active — triggered re-dispatch, the ENGINE-UNAVAILABLE retry, and the sweep outage cap were all unreachable on the new default path. New daemon-finalize.sh records a finished --bg turn's reply + terminal status (noop/live/absent/idle/error contract); dispatch _decide finalizes working/blocked claude metas before judging, then routes through the shared _finished_verdict (triggered respawn / outage retry + cap / skip). Found by the exit-review subagent (P1).
From the two independent exit reviews (Fable subagent + direct codex): - PARKED tier in ESCALATE — the human-tier catch-all could overwrite a needs-human park with confident-ready; parks are now terminal. - Re-flag dedupe scoped: a re-flag matching a FIXED disposition means the fix did not hold — it re-waves as a live blocker instead of escaping the no-NEW-blocker exit as a dupe. - Evidence-only SPEC FINDINGs resolve by verification (no actor may edit the PR body): orchestrator runs the checks post-JOIN; pass → trail evidence, fail → correctness wave. - Wave push gate: pushes only when every FIXED item passed grading; rejected-FIXED route (fix-forward re-wave); commit-content scan for the board file with an unpushed-local-only removal exception; fixer stages only the files its fix touches.
…n completed turns Re-ran Milestone 1 after the codex quota reset. Gateway spawn, subagent inheritance, in-session codex, and the settings-carrying resume fork all confirmed with completed turns; the without-settings fork's silent revert proven by env probe. Recorded the stale cliproxy in-memory cooldown (restart to clear), two evidence hazards (context-echo self-reports on forks; unrepassed argv dimensions reverting), and a timestamp correction note for the execution-run Progress stamps.
…e marker A worker that dies before it can speak — the gateway refusing its very first turn is the live-observed case — finalizes status=error with an empty reply: no assistant message exists to carry ENGINE-UNAVAILABLE. The sweep's finished-reviewer branch keyed retry on the reply marker alone, so such a PR was skipped indefinitely until an explicit event. Treat terminal worker errors as retryable in sweep mode, sharing the existing 3-consecutive failure cap with marker outages (one streak, so interleaved failure kinds cannot reset the count — a reset the RED run demonstrated). Cleanly finished reviewers still break the streak and still stay finished.
The board lived at .doperpowers/qa/ inside the PR worktree — PR-controlled content. A PR that pre-creates a board path component as a symlink would redirect the unattended worker's board write anywhere on the runner before any push gate runs. Boards now live in <review-tmp> (the dispatcher-session mktemp dir the engine findings and protocol audit already use), so no PR-controlled component ever sits on the write path. Park durability moves to the trail: the review-trail comment is the durable per-wave record; the engine block keeps <review-tmp> across a needs-human park and a long-parked board rebuilds from the trail. The push gate stays as defense-in-depth against a fixer copying wave state into the worktree.
…state lies for lingering sessions
Observed live on the first dog-food review worker: a finished --bg daemon
stays listed in `claude agents` with state=working while its harness
process lingers; the turn signal is `status` (busy while a turn runs,
idle after). Three consumers keyed on state alone:
- daemon-finalize.sh read a finished worker as live forever, so dedupe
skipped even an explicit PR event's re-dispatch ("skip active
reviewer") — reproduced on the real worker before this fix.
- _poll_until_done/_poll_uuid made blocking spawn/resume poll a finished
turn to timeout.
- _wt_occupied let the finished session's lingering row occupy the
review worktree forever, blocking the retire+respawn path.
All three now normalize the lingering shape (state=working, status=idle)
to done; busy turns and prompt-blocked sessions keep their live/occupied
semantics, and unmanaged rows stay conservatively occupying.
Audit of ticket obra#567 (ida-solution): an implement worker registered a follow-up ticket with the pre-spec skeleton body and the DEFAULT birth state ready-for-agent; label automation dispatched an implementer 45 seconds later, the spec was never written, and the implementer ended up deciding a security API contract itself. The review loop caught it two days later as a PROTOCOL BLOCKER — the most expensive place to catch it. The board doc already defined ready-for-agent as 'pre-spec complete; dispatchable'; the scripts never enforced it. Mechanical guard keyed on the skeleton marker the register script itself plants: explicit --state ready-for-agent with a skeleton body is refused; a default birth with a skeleton demotes to needs-info with a spec-pending note; board-transition re-checks the body on promotion to ready-for-agent. Real spec bodies are unaffected.
The first live dog-food fixer read 'read-only helper subagents may be used' and delegated implementation to a nested writer. The orchestrator correctly rejected the wave and re-waved it, but the prose should shape the right behavior before grading has to recover it. State the positive contract: the fixer personally performs every code edit and commit; helpers are read-only only. Delegating to a nested writer makes the affected item FAILED and none of that writer's commits acceptable.
…ed at register time, routed through one contract
A constraint-minimization audit over every worker protocol (review, fixer, land, implement, spike, triage). Prohibitions that banned a failure's means — reading code, dispatching subagents, item ordering, serial execution — are removed or rewritten as ownership statements and accountability contracts; hard rules survive only where the banned act is itself the failure state. Notably: triage now routes on the worker's own judgment with native severity as the starting rank, and the fixer may delegate freely but answers for its whole task tree (every commit claimed by exactly one item, backed by the state-defense layer).
Sweep for leftover means-bans and DO-mandates: drop the codex block's solo-execution emphasis, replace the spike decompose exploration ban with its deliverable statement, and align the operation manual with the first pass (ownership statement, no serial mandate, judgment-based routing instead of severity-class definitions). Five new test pins enforce the removals.
…-first golden rule Human rulings on the second constraint-minimization pass: how a wave is crewed is the orchestrator's call — the protocol dispatches the wave's fixer and grades its task tree, but no longer mandates ONE (the accountability contract and quiescence gate bind whatever it dispatches, and a hard count is pure friction for the planned second-order-orchestrator fixer). The INVALID-only-via-graded-REFUTED rule is confirmed kept. The authoring principle lands in CLAUDE.md as a golden rule: no restriction or process enforcement beyond necessary; hard gates only for truly validated failure states.
… becomes guidance Where the enforcement framing exceeded the validated failure state, the clause keeps its content but becomes a statement of the fact or bound: verify-then-fix grounds in the cited code without a prohibition; the staging mandate leaves the fixer contract (grading and the push-gate scan already carry the failure state); the one-shell-command packaging mandate yields to the load-bearing expiry-before-push order; the re-review dedupe tail, the cap exit, the engine block's double DO-NOT, the land conflict-entry choreography, and the hunks-only prohibition list are each restated as the fact they protected. All hard gates with validated failure states stay hard.
SSFSKIM
marked this pull request as ready for review
July 15, 2026 21:36
SSFSKIM
added a commit
that referenced
this pull request
Jul 15, 2026
…e, constraint-minimized protocols (PR #16)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Rebuilds the PR-review loop around one orchestrating worker architecture (ExecPlan:
docs/doperpowers/execplans/2026-07-15-reviewing-prs-orchestrator-rebuild.md).review-engine.shis pure correctness review:--base+--out, with no ticket/spec criteria ordeveloper_instructions. The worker starts it in the background under a 45-minute bound and concurrently audits implementer protocol/spec compliance. The issue body is the canonical primary specification; post-gate drift resolves through GitHub edit history; the worker recordsPROTOCOL BLOCKER,SPEC FINDING, andAUDIT NOTEresults to<review-tmp>/protocol-audit.mdbefore reading native findings.<review-tmp>/pr-<n>-fix-wave-<k>.md, outside the PR-controlled worktree. One direct fixer per wave works the batch sequentially under a verify-then-fix contract. The worker waits for the whole task tree and all tracked control artifacts to become content-stable, snapshots the submitted board, grades only that immutable snapshot, and validates the full unpushed range against an orchestrator-only accepted-commit ledger before removing stale confidence and pushing. Maximum: two fix waves inside three engine rounds, with whole-range re-review after accepted fixes.engine:claudeopts into plain Claude models. Daemon settings/model/effort survive resume forks. The nativecodex exec reviewcall remains only inside the correctness engine.board-answer.shnormalizes lingering sessions and resumes only an idle or awaiting-human owner; absent, error, retired, active, or unknown owners cannot move a parked ticket intoin-progress.Additional failure routes are explicit: terminal workers that cannot emit
ENGINE-UNAVAILABLEshare the same capped sweep failure streak; ticketless oversized findings become structured PR comments;TECH_DEBT_ISSUE=noneroutes non-blockers into the trail; secondary issues are named but not mutated; unexpected remote movement parks without rebase/salvage; a confirmed protocol blocker parks confidence immediately while correctness fixing may continue.First live dog-food
The rebuilt loop ran end-to-end on
ida-solutionPR obra#570 in observation mode./%74a-files/bucket marker could bypass the attachment ownership guard.c10c6226, then re-reviewed the whole PR range clean.needs-humanbecause the security behavior began from an empty, ungated issue body.That failure drove the direct-fixer rule, transitive task-tree containment, content-sensitive quiescence, immutable submitted-board snapshots, clean rollback boundaries, full-range commit provenance, and the review/land ready-ack barriers in this diff.
The related issue-tracker audit found 29 empty or pre-spec tickets in
ida-solution: 25 closed and 4 open; 26 have PR cross-references, of which 23 are closed; only one carries a[gate] pass. The remaining open tickets are obra#567 (needs-human) and obra#568/obra#281/obra#208 (deferred). The tracker now refuses an explicit skeletonready-for-agent, demotes a default skeleton toneeds-info, and rechecks the body before promotion, containing current risk without inventing historical specifications.Verification
All behavior changes were driven through focused RED→GREEN regression tests. The final post-hardening command ran:
tests/reviewing-prs/test-skill-entrypoint.shtests/reviewing-prs/test-review-dispatch.shtests/reviewing-prs/test-review-engine.shtests/reviewing-prs/test-land-dispatch.shtests/orchestrating-daemons/test-daemon-scripts.shtests/issue-tracker/test-board-scripts.shtests/implementing-tickets/test-protocol-content.shtests/codex-plugin-sync/test-sync-to-codex-plugin.shscripts/lint-shell.shover all 13 changed shell filesgit diff --checkEvery command exited 0. The Codex-plugin command above is the hermetic sync regression. The outward
scripts/sync-to-codex-plugin.shdeployment helper was intentionally not run because it targets a separate repository and correctly refuses this feature branch.The architecture received the original two independent exit reviews plus repeated post-dog-food skill reviews. The final independent review included the latest review/land/answer lifecycle and returned Ready, with no important finding; the three remaining comments were non-blocking wording or maintenance nits.
Rollout posture
AUTO_MERGE_ENABLEDremains off. This PR keeps the loop in observation mode: it performs the full judgment and records what it would do, but routes eligible results to human review rather than merging. One successful dog-food plus the resulting hardening is evidence to continue observing, not authorization to enable automatic merge.This remains a draft for complete human diff review. Do not merge it as part of this change.
Provenance
Agent-generated on Claude Code using Fable 5, directed by the fork owner through an interactive design grill and live dog-food review. Design decisions, rejected alternatives, runtime evidence, and retrospective are recorded in the ExecPlan. Fork-local change; not intended for upstream submission.