From c69ccfb8939cd21fab63dd7a2c496e85ebccff2c Mon Sep 17 00:00:00 2001 From: Markus Kovero Date: Tue, 1 Sep 2026 20:06:15 +0000 Subject: [PATCH 01/22] spec: route rig-only evidence around developer --- .agents/qa.md | 26 +++++++++++++++++++++----- 1 file changed, 21 insertions(+), 5 deletions(-) diff --git a/.agents/qa.md b/.agents/qa.md index f389b54a..42ce03ff 100644 --- a/.agents/qa.md +++ b/.agents/qa.md @@ -68,8 +68,7 @@ memory, no redefinition here): `AGENTS.md` states for an agent asserting a mechanism, applied here to a reviewer inheriting one. -A flagged `derived`/`assumed` criterion withholds `in-review` — apply -`needs-work` — unless one of: +A flagged `derived`/`assumed` criterion needs one of: - the gap is closed with evidence in the PR (a measurement, a rig run, a cited derivation the review comment can point to directly), or - a human has already posted an explicit comment on the issue accepting the @@ -77,9 +76,23 @@ A flagged `derived`/`assumed` criterion withholds `in-review` — apply gates: merge is human-only because agent review is not independent, and neither is an agent's acceptance of another agent's assumption). -This blocks only the flagged criterion. A `measured` criterion failing spec -coverage is a correctness issue, reported in that section below, not folded -into this one. +Route an unresolved gap by what can close it: + +- Evidence the developer can add in the tree or derive from already-known + quantities → `request-changes` and `needs-work`. Name the missing artifact or + derivation. This is developer work. +- Evidence only a physical rig can produce, while the implementation correctly + enforces the specified value → `approve` and `requires-rig`, as specified in + the dedicated section below. Do **not** apply `needs-work`: sending this to a + developer cannot produce the measurement and creates a dev→QA loop with no + possible source change. +- The implementation does not enforce the specified value correctly → + `request-changes` and `needs-work`, independently of provenance. This is a + code defect, not an evidence gap. + +In every case, flag the unresolved criterion in the spec-coverage table. A +`measured` criterion failing spec coverage is a correctness issue, reported in +that section below, not folded into this one. ### step 2 — review the diff @@ -267,6 +280,9 @@ Verdict and `requires-rig` are separate axes. A PR can be `approve` + measurement remain before it should land. Say that plainly in the justification — do not downgrade to `request-changes` to express it, because that send the PR back to a developer who cannot take the measurement either. +This includes an unresolved `derived`/`assumed` acceptance criterion when the +implementation correctly enforces the specified value and only physical rig +evidence can validate that value. Step 1 routes that case here explicitly. Where the measurement is one the rig role would take, say which block of `rig/rig-verify-queue.md` it belong to, or that it needs a new one. The From 649657568af4b966078c2075689ccc298ae35495 Mon Sep 17 00:00:00 2001 From: Markus Kovero Date: Tue, 1 Sep 2026 20:07:34 +0000 Subject: [PATCH 02/22] fix(qa): allow same-tip verdict reclassification --- .agents/qa.md | 7 +++++++ bin/review.sh | 6 ++++++ 2 files changed, 13 insertions(+) diff --git a/.agents/qa.md b/.agents/qa.md index 42ce03ff..4563049b 100644 --- a/.agents/qa.md +++ b/.agents/qa.md @@ -214,6 +214,13 @@ would falsify the claim. See step 5.} `claude-approved` is not a merge signal. It puts the PR in the Codex queue merge needs a human. You never set or clear `codex-approved` — if you disagree with a Codex finding, say so in your review comment and leave the label alone. +An explicit full re-review at the same commit is a new review pass when the +governing agent spec, issue decision, or human evidence changed after the old +verdict. Re-evaluate under the current inputs, post a superseding review +comment, and update labels to the new verdict. "No new commit" is not a reason +to preserve a verdict whose governing rule changed; the commit is the subject +of review, not the only review input. + ### sending it back to architect or ux — the design is wrong, not the code Most findings are "the implementation does not do what the spec says". Some are diff --git a/bin/review.sh b/bin/review.sh index b10aaf26..deeded13 100755 --- a/bin/review.sh +++ b/bin/review.sh @@ -77,6 +77,12 @@ Standards PDFs are NOT in this checkout — they are licence-restricted and giti else prompt="Review PR #$n in $AC_REPO. +This is an explicit full review. Even if this commit already has an earlier QA +comment, this invocation is a new review pass: governing specs, issue decisions, +or human evidence may have changed without a code push. Apply the current QA +spec, post a new superseding QA comment, and update labels to its verdict. Do +not decline to post merely because the PR tip is unchanged. + Standards PDFs are NOT in this checkout — they are licence-restricted and gitignored. They are at $AC_STDDOCS. Each PDF has a .txt sibling extracted with pdftotext -layout: Grep that to find the clause, then Read the PDF at that region only. Do not page through a PDF looking for a clause. Extraction is lossy for equations, figures and some tables — where the clause turns on one of those, open the PDF itself. A citation you did not verify against the primary text is not a verified citation; if a document you need is genuinely missing from that directory, say which one rather than carrying the gap forward silently." fi From 70931c6f3672ccf8b4a358211f28624bf4a9dfa2 Mon Sep 17 00:00:00 2001 From: Markus Kovero Date: Tue, 1 Sep 2026 20:50:18 +0000 Subject: [PATCH 03/22] spec: require rig evidence before QA approval --- .agents/AGENTS.md | 10 +++++++--- .agents/codex-qa.md | 21 +++++++++++++++------ .agents/qa.md | 28 ++++++++++++++++++---------- bin/codex-qa.sh | 31 +++++++++++++++++++++++++++++-- bin/master.sh | 21 +++++++++++++++------ 5 files changed, 84 insertions(+), 27 deletions(-) diff --git a/.agents/AGENTS.md b/.agents/AGENTS.md index 65ffb6c0..e033970d 100644 --- a/.agents/AGENTS.md +++ b/.agents/AGENTS.md @@ -50,13 +50,13 @@ Always human-only: | `ready-to-implement` | triage, architect or ux | developer can pick up | | `tier-1` `tier-2` `scene` `view` `scope-none` | triage, architect corrects, qa raises | exactly one. `tier-1` = a standard in the document map governs correctness, so qa runs the standards check. Unlabelled is a triage gap and reads as `tier-1`. qa may raise a label to `tier-1`, never lower one | | `in-review` | developer (via PR) | PR open | -| `claude-approved` | qa (step 5, approve verdict) | Claude QA passed **at the commit it reviewed** | -| `codex-approved` | codex-qa (pass verdict) | independent Codex QA passed at the commit it reviewed | +| `claude-approved` | qa (step 5, approve verdict) | Claude QA passed **at the commit it reviewed**, with no pending rig gate | +| `codex-approved` | codex-qa (pass verdict) | independent Codex QA passed at the commit it reviewed, with no pending rig gate | | `needs-work` | qa **or** codex-qa | PR has issues, developer must revise | | `blocked` | any agent | this issue waits on something else — see below | | `blocks-others` | any agent | other work waits on **this** issue | | `epic` | triage | contains sub-issues | -| `requires-rig` | qa | correctness rests on a measurement only the rig can make — human clears it after the measurement exists | +| `requires-rig` | qa | correctness rests on a measurement only the rig can make — blocks both approval labels; human clears it after the measurement exists | | `agent:triage` | triage | audit trail | | `agent:architect` | architect | audit trail | | `agent:dev` | developer | audit trail | @@ -68,6 +68,10 @@ Always human-only: under different models, and both must be present for a human to merge (see human gates). Neither agent may set the other's label. +Neither approval label may be applied while `requires-rig` is present. Tree QA +defines the measurement and stops at the rig gate; after the measurement is +recorded and a human clears `requires-rig`, QA runs again at the same commit. + **Whoever applies `blocked` names the exact condition that lifts it**, in the comment that applies it: *"#180 merged → remove `blocked`"*. #181 and #182 are the established form. diff --git a/.agents/codex-qa.md b/.agents/codex-qa.md index 1b219141..bb05b78c 100644 --- a/.agents/codex-qa.md +++ b/.agents/codex-qa.md @@ -22,17 +22,20 @@ Review-only. No fixes, no test edits, no merges, no branch pushes. ## queue -Open PRs with `claude-approved` and without `codex-approved`. +Open PRs with `claude-approved`, without `codex-approved`, `needs-work`, or +`requires-rig`. ```bash gh pr list --state open \ - --search 'label:claude-approved -label:codex-approved' \ + --search 'label:claude-approved -label:codex-approved -label:needs-work -label:requires-rig' \ --json number --jq '.[].number' ``` -`needs-work` does **not** exclude a PR from the queue. `claude-approved` plus -`needs-work` means you failed it previously and Claude QA has since re-passed -it; that is a PR to review again, not one to skip. +`claude-approved` plus `needs-work` is the state immediately after a Codex +failure. Exclude it so an unattended runner does not review the same rejected +tip on every poll. The developer removes both labels when picking up the +finding. After the revision, Claude QA re-reviews the new tip and restores +`claude-approved`; that puts the PR back in this queue. There is no queue state anywhere but GitHub. `bin/codex-qa.sh` walks this list and holds nothing. @@ -95,6 +98,10 @@ measurement that would separate it from an equally plausible alternative — you inherit the assumption the same way the first reviewer did, and it is no more verified for having survived one review. +`requires-rig` present, or the required measurement record absent, is not a +pass. Do not apply `codex-approved`; the PR must return through full Claude QA +after a human records the measurement and clears the rig gate. + ### step 2 — the diff - **correctness** — does the implementation do what the spec says? - **numerical correctness** — estimator and measurement code: window sizes, @@ -254,7 +261,9 @@ it. - Do not merge. Merge to main is a human gate, and both approvals plus a human reading the timestamps is what that gate means (`AGENTS.md`). - Never set or clear `claude-approved`. Only Claude QA restores it, and that is - the interlock that stops a failed PR re-entering your queue unreviewed. + how a revised PR re-enters your queue. `needs-work` is the interlock that + keeps the rejected tip out until the developer picks it up; the developer + removes `claude-approved` before changing that tip. - Never remove `requires-rig`. Human-only, after the measurement exists. - No citing a location you have not opened. A `Grep` hit, or any summary of the tree, is a candidate — not a verified read. diff --git a/.agents/qa.md b/.agents/qa.md index 4563049b..ba7535fb 100644 --- a/.agents/qa.md +++ b/.agents/qa.md @@ -82,8 +82,8 @@ Route an unresolved gap by what can close it: quantities → `request-changes` and `needs-work`. Name the missing artifact or derivation. This is developer work. - Evidence only a physical rig can produce, while the implementation correctly - enforces the specified value → `approve` and `requires-rig`, as specified in - the dedicated section below. Do **not** apply `needs-work`: sending this to a + enforces the specified value → `rig-pending` and `requires-rig`, as specified + in the dedicated section below. Do **not** apply `needs-work`: sending this to a developer cannot produce the measurement and creates a dev→QA loop with no possible source change. - The implementation does not enforce the specified value correctly → @@ -184,7 +184,7 @@ Post PR review in this structure: {Any files touched outside spec scope. If none: "none."} ### verdict -{approve | request-changes | request-changes: design} +{approve | rig-pending | request-changes | request-changes: design} {One sentence justification.} ### sent back to @@ -199,12 +199,16 @@ would falsify the claim. See step 5.} ### step 5 — apply label - Approving → apply `claude-approved`, leave `in-review` in place +- Rig pending → apply `requires-rig`, remove `claude-approved` and + `needs-work`, and leave `in-review` in place. The human clears + `requires-rig` after recording the measurement; the next full QA pass can + then approve at the same commit. - Requesting changes → apply `needs-work`, remove `in-review`. Do **not** apply `claude-approved`; the pairing of `claude-approved` with a request-changes verdict is what tells a reader the finding came from Codex, so never produce it here. -- Correctness turn on a physical measurement you cannot make from the tree → - apply `requires-rig` **in addition to** whichever of the above applies +- Correctness turns on a physical measurement you cannot make from the tree → + use the `rig-pending` verdict above, not an approval or request-changes - The defect is in the **design**, not the implementation → apply `needs-work` as above, and additionally apply `needs-design` **on the issue** (or `needs-ux` on the issue, where the thing that is wrong is what the operator @@ -282,15 +286,19 @@ Apply it when the PR change or depend on: - timing that depend on real device or driver behaviour rather than the test clock -Verdict and `requires-rig` are separate axes. A PR can be `approve` + -`requires-rig`: the code is right as far as the tree can show, and one -measurement remain before it should land. Say that plainly in the -justification — do not downgrade to `request-changes` to express it, because -that send the PR back to a developer who cannot take the measurement either. +`requires-rig` is a pre-approval state. The code may be right as far as the +tree can show, but QA has not established the acceptance criterion until the +measurement exists. Use `rig-pending`; do not apply `claude-approved` and do +not downgrade to `request-changes`, because that sends the PR back to a +developer who cannot take the measurement either. This includes an unresolved `derived`/`assumed` acceptance criterion when the implementation correctly enforces the specified value and only physical rig evidence can validate that value. Step 1 routes that case here explicitly. +After the measurement record exists and a human clears `requires-rig`, run a +full QA pass even when the commit is unchanged. Verify the record closes the +named falsification test before applying `claude-approved`. + Where the measurement is one the rig role would take, say which block of `rig/rig-verify-queue.md` it belong to, or that it needs a new one. The rig role produce the measurement record; you do not run the session and you do diff --git a/bin/codex-qa.sh b/bin/codex-qa.sh index e42886b3..d79ed054 100755 --- a/bin/codex-qa.sh +++ b/bin/codex-qa.sh @@ -32,6 +32,8 @@ source "$(dirname "$0")/common.sh" readonly CLAUDE_LABEL="claude-approved" readonly CODEX_LABEL="codex-approved" +readonly NEEDS_WORK_LABEL="needs-work" +readonly REQUIRES_RIG_LABEL="requires-rig" # The active review worktree is removed on both success and failure. Refuse to # reuse an existing path: it may contain evidence or edits from an interrupted @@ -65,7 +67,7 @@ if ! gh_retry gh auth status >/dev/null 2>&1; then exit 1 fi -echo "Checking for open PRs with '$CLAUDE_LABEL' and without '$CODEX_LABEL'..." +echo "Checking for open PRs with '$CLAUDE_LABEL', without '$CODEX_LABEL', '$NEEDS_WORK_LABEL', or '$REQUIRES_RIG_LABEL'..." # IMPORTANT: # This is exactly: @@ -73,8 +75,13 @@ echo "Checking for open PRs with '$CLAUDE_LABEL' and without '$CODEX_LABEL'..." # open # AND claude-approved # AND NOT codex-approved +# AND NOT needs-work +# AND NOT requires-rig # -# 'needs-work' intentionally does not participate in eligibility. +# A Codex failure leaves claude-approved in place because that label belongs +# to Claude QA. Excluding needs-work prevents the daemon from reviewing the +# same rejected tip again on every poll. A revised tip re-enters only after +# Claude QA has reviewed it and restored claude-approved. prs_output="$( gh_retry gh pr list \ --state open \ @@ -87,6 +94,8 @@ prs_output="$( all( .labels[]?; .name != "codex-approved" + and .name != "needs-work" + and .name != "requires-rig" ) ) | .number @@ -126,6 +135,8 @@ for pr in "${prs[@]}"; do has_claude_approved=false has_codex_approved=false + has_needs_work=false + has_requires_rig=false for label in "${labels[@]}"; do case "$label" in @@ -135,6 +146,12 @@ for pr in "${prs[@]}"; do "$CODEX_LABEL") has_codex_approved=true ;; + "$NEEDS_WORK_LABEL") + has_needs_work=true + ;; + "$REQUIRES_RIG_LABEL") + has_requires_rig=true + ;; esac done @@ -148,6 +165,16 @@ for pr in "${prs[@]}"; do continue fi + if [[ "$has_needs_work" == true ]]; then + echo "Skipping PR #$pr: '$NEEDS_WORK_LABEL' is present." + continue + fi + + if [[ "$has_requires_rig" == true ]]; then + echo "Skipping PR #$pr: '$REQUIRES_RIG_LABEL' is present." + continue + fi + pr_url="$( gh_retry gh pr view "$pr" \ --json url \ diff --git a/bin/master.sh b/bin/master.sh index 348b0257..209dec0d 100755 --- a/bin/master.sh +++ b/bin/master.sh @@ -171,20 +171,26 @@ qa_loop() { head="$(gh_retry gh pr view "$pr" -R "$AC_REPO" --json headRefOid --jq .headRefOid)" mark="$AC_LOG_DIR/reviewed-pr-$pr.sha" - # Already reviewed at this exact tip and qa raised nothing: that is a pass. + # Already reviewed at this exact tip. requires-rig is a pre-approval stop; + # after a human clears it, absence of claude-approved forces a full same-tip + # QA pass so the measurement record becomes part of the approval evidence. # Unless force is set — then the tip is unchanged but the design under it # is not, and the cached approval is an approval of a superseded spec. ev="$(qa_evidence "$pr")" || { echo " #$n: cannot count qa output — stopping"; return 1; } if [[ -z $force && -f $mark && "$(cat "$mark")" == "$head" ]] && (( ev > 0 )); then if has requires-rig "$ls"; then - echo " #$n PR #$pr: qa reviewed $head — approved, but REQUIRES RIG" + echo " #$n PR #$pr: tree QA complete — REQUIRES RIG before approval" echo " a measurement is outstanding. the label is human-clear only:" echo " read the review's 'rig verification required' field, run the" echo " session, then: gh pr edit $pr -R $AC_REPO --remove-label requires-rig" STATE=needs-rig; return 0 fi - echo " #$n PR #$pr: qa reviewed $head and raised nothing — yours to merge" - STATE=awaiting-merge; return 0 + if has claude-approved "$ls"; then + echo " #$n PR #$pr: qa approved $head — yours to merge" + STATE=awaiting-merge; return 0 + fi + echo " #$n PR #$pr: rig gate cleared — full QA must incorporate its evidence" + force=full fi echo " #$n PR #$pr: qa review${force:+ (full — design changed since the last pass)}" @@ -217,12 +223,15 @@ qa_loop() { if ! has needs-work "$ls"; then if has requires-rig "$ls"; then - echo " #$n PR #$pr: approved, but REQUIRES RIG — measurement outstanding" + echo " #$n PR #$pr: tree QA complete — REQUIRES RIG before approval" echo " see the review's 'rig verification required' field." STATE=needs-rig - else + elif has claude-approved "$ls"; then echo " #$n PR #$pr: qa reviewed and raised nothing — yours to merge" STATE=awaiting-merge + else + echo " #$n PR #$pr: qa posted no approval or routed finding — stopping" + STATE=needs-human fi return 0 fi From d4f58979d5d8ce7b12cf898fa1329400d375b17e Mon Sep 17 00:00:00 2001 From: Markus Kovero Date: Wed, 2 Sep 2026 17:07:46 +0000 Subject: [PATCH 04/22] feat(bin): resume work and select agent providers --- bin/common.sh | 208 +++++++++++++++++++++++++++++++++++------------ bin/implement.sh | 60 +++++++++++--- bin/master.sh | 33 ++++++-- bin/session.sh | 81 ++++++++++++++++-- 4 files changed, 303 insertions(+), 79 deletions(-) diff --git a/bin/common.sh b/bin/common.sh index 5a0005cf..269286df 100755 --- a/bin/common.sh +++ b/bin/common.sh @@ -224,11 +224,32 @@ qa_evidence() { # The architect's file manifest for an issue: repo-relative paths, one per line. # Empty output means no manifest — the caller decides whether that is fatal. manifest_of() { - gh_retry gh issue view "$1" -R "$AC_REPO" --json comments \ - --jq '[.comments[] | select(.body | test(""))] | last | .body // ""' \ - | sed -n '/^```files[[:space:]]*$/,/^```[[:space:]]*$/p' \ - | sed '1d;$d; s/^[[:space:]]*//; s/[[:space:]]*$//' \ - | grep -v '^$' || true + local body out + body=$(gh_retry gh issue view "$1" -R "$AC_REPO" --json comments \ + --jq '[.comments[] | select(.body | test(""))] | last | .body // ""') \ + || return 1 + + # Newer comments may use an explicit files fence. Prefer it because its end + # marker is unambiguous. + out=$(printf '%s\n' "$body" \ + | sed -n '/^```files[[:space:]]*$/,/^```[[:space:]]*$/p' \ + | sed '1d;$d; s/^[[:space:]]*//; s/[[:space:]]*$//' \ + | grep -v '^$' || true) + if [[ -n $out ]]; then printf '%s\n' "$out"; return 0; fi + + # The architect template in existing issues uses a Markdown section with + # one bare path per line. Stop at the next bold field and emit paths only; + # prose such as "(none — coordination-only epic)" is not a manifest. + printf '%s\n' "$body" | awk ' + /^\*\*file manifest\*\*[[:space:]]*$/ { in_manifest=1; next } + in_manifest && /^\*\*/ { exit } + in_manifest { + line=$0 + sub(/^[[:space:]]*[-*][[:space:]]*/, "", line) + gsub(/`/, "", line) + sub(/[[:space:]]*$/, "", line) + if (line ~ /^[[:alnum:]_.-]+\//) print line + }' } # Extract the session's final message from a finished transcript. # Prefer the result event; fall back to the last assistant text block, because @@ -242,10 +263,54 @@ distill() { | select(.type=="text") | .text] | last // empty' "$raw" 2>/dev/null || true } -# run [--fg] [--read] [extra claude args...] -# --fg drops into interactive Claude Code: you see everything and can steer, -# but the allowlist is not enforced — you are prompted instead, and nothing -# is written to $AC_SESSION_DIR. +# Provider selection, in descending precedence: +# AC__PROVIDER=codex one role (AC_DEVELOPER_PROVIDER, AC_QA_PROVIDER...) +# AC_PROVIDER=codex every non-QA role in this invocation +# claude backwards-compatible default +provider_for() { + local role="$1" key value + key="AC_${role^^}_PROVIDER" + key="${key//-/_}" + if [[ -n ${!key:-} ]]; then + value="${!key}" + elif [[ $role == qa ]]; then + value=claude + else + value="${AC_PROVIDER:-claude}" + fi + case "$value" in + claude|codex) printf '%s\n' "$value" ;; + *) echo "unsupported provider '$value' for $role (expected claude or codex)" >&2; return 2 ;; + esac +} + +# A role-specific model wins, followed by the old global AC_MODEL. Provider +# defaults are deliberately separate: Claude aliases are not Codex model IDs. +model_for() { + local role="$1" provider="$2" key value + key="AC_${role^^}_MODEL"; key="${key//-/_}" + value="${!key:-${AC_MODEL:-}}" + if [[ -z $value ]]; then + case "$provider:$role" in + claude:architect|claude:ux|claude:triage) value="${AC_CLAUDE_MODEL:-opus}" ;; + claude:*) value="${AC_CLAUDE_MODEL:-sonnet}" ;; + codex:*) value="${AC_CODEX_MODEL:-}" ;; + esac + fi + printf '%s\n' "$value" +} + +distill_codex() { + local raw="$1" last="$2" + if [[ -s $last ]]; then cat "$last"; return; fi + jq -rs '[.[] | select(.type=="item.completed") | .item + | select(.type=="agent_message") | .text] | last // empty' \ + "$raw" 2>/dev/null || true +} + +# run [--fg] [--read] [extra provider args...] +# --fg drops into the selected provider's interactive CLI: you see everything +# and can steer, and nothing is written to $AC_SESSION_DIR. run() { local role="$1" prompt="$2"; shift 2 @@ -261,23 +326,18 @@ run() { *) turns=80 ;; esac fi - # Opus for the roles whose output is a judgement nobody re-derives: a design - # decision, an output-surface decision, and a spec's acceptance criteria are - # all read as settled by every role downstream, so a weak one propagates - # instead of failing. Triage belongs here for a second reason — it also sets - # the scope label that decides whether qa runs the standards check at all. - # The rest produce something a later step re-checks: the developer's code - # meets the test suite, qa's findings meet the diff. - # - # An AC_MODEL from the environment wins. It used to be overwritten here - # unconditionally, so `AC_MODEL=opus bin/review.sh 384` silently ran sonnet - # and the `:-sonnet` fallback below was unreachable. - local model="${AC_MODEL:-}" - if [[ -z $model ]]; then - case "$role" in - architect|ux|triage) model=opus ;; - *) model=sonnet ;; - esac + local provider model + provider="$(provider_for "$role")" || return + model="$(model_for "$role" "$provider")" + + # The current approval labels are reviewer identities, not generic slots: + # qa owns claude-approved and codex-qa owns codex-approved. Until those specs + # and labels are migrated together, letting Codex occupy qa would make both + # supposedly independent gates Codex reviews. + if [[ $role == qa && $provider != claude ]]; then + echo "qa provider is fixed to claude by the current two-review gate" >&2 + echo "migrate claude-approved/codex-approved to provider-neutral review slots first" >&2 + return 2 fi local fg="" tools="$TOOLS_WRITE" deny="$DENY_ASYNC" mode="acceptEdits" arg local -a extra=() @@ -289,6 +349,18 @@ run() { esac done + command -v "$provider" >/dev/null 2>&1 \ + || { echo "provider CLI not found: $provider" >&2; return 127; } + + # Codex has no --system-prompt-file equivalent. Make reading the same role + # spec the first task instruction; AGENTS.md is loaded by Codex itself. + local task_prompt="$prompt" + if [[ $provider == codex ]]; then + task_prompt="Read $(spec "$role") fully before doing anything else. It is your role specification and is binding. + +$prompt" + fi + if [[ -n $fg ]]; then # Same options as the -p run below, minus only the three that are about # being non-interactive: -p itself, the stream-json plumbing, and @@ -301,12 +373,23 @@ run() { # # --permission-mode still differs in effect, not in value: interactively # it prompts where -p auto-approves, which is the point of --fg. - claude --system-prompt-file "$(spec "$role")" \ - --model "$model" \ - --allowedTools "$tools${GH_TOOLS:+,$GH_TOOLS}" \ - ${deny:+--disallowedTools "$deny"} \ - --permission-mode "$mode" \ - "${extra[@]}" "$prompt" + if [[ $provider == claude ]]; then + claude --system-prompt-file "$(spec "$role")" \ + --model "$model" \ + --allowedTools "$tools${GH_TOOLS:+,$GH_TOOLS}" \ + ${deny:+--disallowedTools "$deny"} \ + --permission-mode "$mode" \ + "${extra[@]}" "$task_prompt" + else + # Read-only roles still run tests and gh label/comment operations. Codex + # therefore needs a writable sandbox; the binding role spec forbids + # source edits, like the existing Claude review convention around Bash. + local sandbox=workspace-write + local -a model_arg=() + [[ -n $model ]] && model_arg=(-m "$model") + codex -C "$PWD" -s "$sandbox" -a on-request \ + "${model_arg[@]}" "${extra[@]}" "$task_prompt" + fi return fi @@ -332,20 +415,19 @@ run() { local raw="$AC_LOG_DIR/$stamp.jsonl" local out="$AC_SESSION_DIR/$stamp.md" + local last="$AC_LOG_DIR/$stamp.last.md" - # Stream to the terminal, keep the raw transcript. Distillation happens after - # the run, not inside the pipe — a process-substitution tee races the - # pipeline's exit and truncates exactly the long sessions worth reading. -# CLAUDE_AGENT_SDK_DISABLE_BUILTIN_AGENTS=1 \ - claude -p --system-prompt-file "$(spec "$role")" "$prompt" \ - --model "$model" \ - --allowedTools "$tools${GH_TOOLS:+,$GH_TOOLS}" \ - ${deny:+--disallowedTools "$deny"} \ - --permission-mode "$mode" \ - --max-turns "$turns" \ - --output-format stream-json --verbose "${extra[@]}" \ - | tee "$raw" \ - | jq -r --unbuffered ' + # Stream to the terminal and retain the provider's native JSONL transcript. + if [[ $provider == claude ]]; then + claude -p --system-prompt-file "$(spec "$role")" "$task_prompt" \ + --model "$model" \ + --allowedTools "$tools${GH_TOOLS:+,$GH_TOOLS}" \ + ${deny:+--disallowedTools "$deny"} \ + --permission-mode "$mode" \ + --max-turns "$turns" \ + --output-format stream-json --verbose "${extra[@]}" \ + | tee "$raw" \ + | jq -r --unbuffered ' def arg: (.input.file_path // .input.pattern // .input.command // .input.description // "") | tostring | gsub("[\r\n]+"; " ") | .[0:100]; @@ -355,31 +437,57 @@ run() { elif .type=="tool_use" then " → \(.name) \(arg)" else empty end) else empty end' || status=$? + else + local sandbox=workspace-write + local -a model_arg=() + [[ -n $model ]] && model_arg=(-m "$model") + codex exec -C "$PWD" -s "$sandbox" \ + -c 'approval_policy="never"' \ + -c "sandbox_${sandbox//-/_}.network_access=true" \ + --add-dir "$AC_TARGET" --json -o "$last" \ + "${model_arg[@]}" "${extra[@]}" "$task_prompt" \ + | tee "$raw" \ + | jq -r --unbuffered ' + if .type=="item.completed" and .item.type=="agent_message" then .item.text + elif .type=="item.started" and .item.type=="command_execution" then + " → Bash " + ((.item.command // "") | gsub("[\\r\\n]+"; " ") | .[0:100]) + else empty end' || status=$? + fi # Header says what this file is: a point-in-time record of one run, not # state. The tracker still owns whether the issue or PR is open. local sid - sid=$(jq -r 'select(.type=="system") | .session_id // empty' "$raw" 2>/dev/null | head -1 || true) + if [[ $provider == claude ]]; then + sid=$(jq -r 'select(.type=="system") | .session_id // empty' "$raw" 2>/dev/null | head -1 || true) + else + sid=$(jq -r 'select(.type=="thread.started") | .thread_id // empty' "$raw" 2>/dev/null | head -1 || true) + fi { printf '\n' \ "$role" "$tag" "$(date -Iminutes)" "$status" printf '\n' "${raw/#$HOME/\~}" - printf '\n\n' "${sid:-unknown}" - distill "$raw" + printf '\n' "$provider" "${model:-default}" + if [[ $provider == claude ]]; then + printf '\n\n' "${sid:-unknown}" + distill "$raw" + else + printf '\n\n' "${sid:-unknown}" + distill_codex "$raw" "$last" + fi } > "$out" # A capped run ends mid-task with a final message that reads like progress, # not like failure. Say so plainly rather than leaving it to be inferred. local used used=$(jq -r 'select(.type=="result") | .num_turns // empty' "$raw" 2>/dev/null | tail -1 || true) - if [[ -n $used ]] && (( used >= turns )); then + if [[ $provider == claude && -n $used ]] && (( used >= turns )); then echo "WARNING: hit the $turns-turn cap (used $used) — this run was cut off." >&2 echo " work is uncommitted in the worktree. resume:" >&2 echo " cd \$(git rev-parse --show-toplevel) && claude --resume ${sid:-}" >&2 echo " or raise it: AC_MAX_TURNS=$(( turns * 2 )) ..." >&2 fi - [[ -s $raw ]] || echo "warning: empty transcript — check claude exited cleanly" >&2 + [[ -s $raw ]] || echo "warning: empty transcript — check $provider exited cleanly" >&2 echo "session: $out" >&2 echo "raw: $raw" >&2 return "$status" diff --git a/bin/implement.sh b/bin/implement.sh index cdb66685..a9587bfb 100755 --- a/bin/implement.sh +++ b/bin/implement.sh @@ -1,29 +1,53 @@ #!/usr/bin/env bash -# implement.sh [--fg] +# implement.sh [--continue] [--fg] # # Developer role against one issue, in its own worktree so several can run at # once. For a PR already sent back by QA use revise.sh — this script creates # branches and would reset an existing one. source "$(dirname "$0")/common.sh" -n="${1:?usage: implement.sh [--fg]}"; shift || true +n="${1:?usage: implement.sh [--continue] [--fg]}"; shift || true wt="$WT_BASE/issue-$n" +continue_mode="${AC_CONTINUE:-}" +declare -a run_args=() +for arg in "$@"; do + case "$arg" in + --continue) continue_mode=1 ;; + *) run_args+=("$arg") ;; + esac +done -# -B force-resets, so an existing branch would lose its commits and with them -# the PR's history. Refuse instead. +# Reuse only the exact empty branch/worktree shape left by a failed preflight. +# Anything dirty or ahead may contain a cut-off implementation and is refused. if git show-ref -q "refs/heads/issue-$n"; then - echo "branch issue-$n exists — use revise.sh for QA feedback, or delete it first" >&2 - exit 1 + existing="$(worktree_of_branch "issue-$n")" + dirty=""; ahead="$(git rev-list --count "origin/main..issue-$n" 2>/dev/null || echo 0)" + [[ -n $existing && -d $existing ]] && dirty="$(git -C "$existing" status --porcelain)" + if [[ -n $dirty || $ahead != 0 ]] && [[ -z $continue_mode ]]; then + echo "branch issue-$n contains or may contain work — use revise.sh or inspect it" >&2 + exit 1 + fi + if [[ -n $existing ]]; then + wt="$existing" + else + require_space "$wt" || exit 1 + git worktree add "$wt" "issue-$n" >/dev/null + fi + [[ -n $dirty || $ahead != 0 ]] || git -C "$wt" merge -q --ff-only origin/main +else + require_space "$wt" || exit 1 + git fetch -q origin main + git worktree add -B "issue-$n" "$wt" origin/main >/dev/null fi -require_space "$wt" || exit 1 -git fetch -q origin main -git worktree add -B "issue-$n" "$wt" origin/main >/dev/null sparse_trim "$wt" cd "$wt" link_support "$wt" -mapfile -t files < <(manifest_of "$n") +manifest="$(manifest_of "$n")" \ + || { echo "cannot read architect manifest for #$n" >&2; exit 1; } +files=() +[[ -n $manifest ]] && mapfile -t files <<< "$manifest" if (( ${#files[@]} == 0 )); then echo "no file manifest on #$n — run bin/design.sh $n first" >&2 @@ -35,7 +59,19 @@ for f in "${files[@]}"; do [[ -e $f ]] || echo "note: manifest names $f — not in tree, assuming new file" >&2 done -AC_TAG="issue-$n" run developer "Implement issue #$n in $AC_REPO. +if [[ -n $continue_mode ]]; then + task="Continue the interrupted implementation of issue #$n in $AC_REPO. + +This branch already contains uncommitted or committed work from an earlier +developer session that ended before opening a PR. Inspect git status and the +existing diff first. Preserve correct work, finish the remaining implementation +and verification, then commit, push, and open the PR. Do not restart from a +clean tree and do not discard work merely because another provider produced it." +else + task="Implement issue #$n in $AC_REPO." +fi + +AC_TAG="issue-$n${continue_mode:+-continue}" run developer "$task The architect's design comment names the files this change touches. That list is your scope: @@ -43,6 +79,6 @@ $(printf '%s\n' "${files[@]}") Read those files first, in that order, before anything else. Do not sweep the tree. Do not Glob or Grep to find what to work on — the search has already been done and its result is above. Grep is for locating a symbol inside a file already on this list. -A file you need that is not on the list is a finding about the design, not a gap for you to fill. Stop, comment on the issue with the path and why it is needed, apply needs-design, and end the run. Adding it silently is the exact failure this list exists to prevent." "$@" +A file you need that is not on the list is a finding about the design, not a gap for you to fill. Stop, comment on the issue with the path and why it is needed, apply needs-design, and end the run. Adding it silently is the exact failure this list exists to prevent." "${run_args[@]}" echo "worktree: $wt branch: issue-$n" >&2 diff --git a/bin/master.sh b/bin/master.sh index 209dec0d..2e9415c0 100755 --- a/bin/master.sh +++ b/bin/master.sh @@ -22,6 +22,12 @@ # AC_DESIGN_PASSES=2 max architect passes per issue per run (default 2) # AC_UX_PASSES=2 max ux passes per issue per run (default 2) # AC_NO_TRIAGE=1 never triage; an unrouted issue is nothing to do +# AC_PROVIDER=codex use Codex for ordinary roles except the model-bound +# QA gates (default: claude) +# AC_DEVELOPER_PROVIDER=codex +# override one role; likewise TRIAGE, ARCHITECT, and UX. +# Per-role settings win over AC_PROVIDER. QA remains +# model-bound until its two approval labels are migrated. # # Verify label names first — a wrong one makes this do nothing while looking # like it worked: gh label list -R mkovero/ac @@ -244,7 +250,7 @@ qa_loop() { } drive() { - local n="$1" step=0 ls pr tc st force="" + local n="$1" step=0 ls pr tc st force="" continue_arg="" mf="" local ran_design=0 ran_ux=0 ran_triage=0 local design_passes=0 ux_passes=0 qa_round=0 @@ -340,21 +346,30 @@ drive() { # Work is present. Deleting here throws away a whole run. echo " it has work: ${dirty:-0} uncommitted file(s), $ahead commit(s) ahead of main." echo " an earlier run was probably cut off. do NOT delete it. resume:" - echo " jq -r 'select(.type==\"system\") | .session_id // empty' \\" - echo " ${AC_LOG_DIR}/*developer-issue-$n.jsonl | head -1" - echo " cd $wt && claude --resume " + echo " continuing in the same worktree with the currently selected provider" + continue_arg=--continue else - echo " it is empty — no commits, nothing uncommitted. safe to clear:" - echo " git worktree remove --force $wt; git branch -D issue-$n" + echo " it is empty — reusing it for this implementation attempt." fi - return 0 fi has ready-to-implement "$ls" \ || { echo " #$n: no PR, not ready-to-implement — nothing to do"; return 0; } - echo " #$n: implementing" - "$BIN/implement.sh" "$n" $fg || { echo " #$n: implement failed"; return 1; } + # Every developer invocation needs a hard file boundary. Issues triaged + # straight to ready-to-implement have no architect comment, so create that + # boundary before opening a worktree instead of asking dev to implement + # from triage's non-exhaustive "files likely affected" list. + mf="$(manifest_of "$n")" \ + || { echo " #$n: cannot read architect manifest — stopping"; return 1; } + if [[ -z $mf ]]; then + echo " #$n: no architect manifest — running design preflight" + "$BIN/design.sh" "$n" $fg || { echo " #$n: design preflight failed"; return 1; } + continue + fi + + echo " #$n: implementing${continue_arg:+ (continuation)}" + "$BIN/implement.sh" "$n" $continue_arg $fg || { echo " #$n: implement failed"; return 1; } pr="$(pr_for "$n")" [[ -n $pr ]] || { echo " #$n: no PR opened — check the session log"; return 1; } echo " #$n: opened PR #$pr" diff --git a/bin/session.sh b/bin/session.sh index bfcac44d..5e07fd21 100755 --- a/bin/session.sh +++ b/bin/session.sh @@ -17,9 +17,25 @@ set -euo pipefail f="${1:?usage: session.sh [summary|tools|text|files|delegate|final|errors|weight|cost|types]}" view="${2:-summary}" +provider() { + jq -e 'select(.type=="thread.started")' "$f" >/dev/null 2>&1 \ + && echo codex || echo claude +} + # Collapse newlines: a heredoc in a Bash command otherwise becomes several # rows, and every downstream cut/sort counts its lines as tool names. tools() { + if [[ $(provider) == codex ]]; then + jq -r 'select((.type=="item.started" and .item.type=="command_execution") + or (.type=="item.completed" and .item.type=="file_change")) + | .item + | if .type=="command_execution" then + "Bash\t" + ((.command // "") | gsub("[\\r\\n]+"; " ") | .[0:120]) + elif .type=="file_change" then + "Edit\t" + ((.changes // []) | tostring | .[0:120]) + else empty end' "$f" + return + fi jq -r 'select(.type=="assistant") | .message.content[]? | select(.type=="tool_use") | .name + "\t" + @@ -28,12 +44,22 @@ tools() { } files() { + if [[ $(provider) == codex ]]; then + jq -r 'select(.type=="item.completed" and .item.type=="file_change") + | .item.changes[]?.path // empty' "$f" | sort -u + return + fi jq -r 'select(.type=="assistant") | .message.content[]? | select(.type=="tool_use" and (.name=="Edit" or .name=="Write" or .name=="NotebookEdit")) | .input.file_path // empty' "$f" | sort -u } text() { + if [[ $(provider) == codex ]]; then + jq -r 'select(.type=="item.completed" and .item.type=="agent_message") + | .item.text // empty' "$f" + return + fi jq -r 'select(.type=="assistant") | .message.content[]? | select(.type=="text") | .text' "$f" } @@ -43,6 +69,11 @@ text() { # not every version emits result into the stream. final() { local r + if [[ $(provider) == codex ]]; then + jq -rs '[.[] | select(.type=="item.completed" and .item.type=="agent_message") + | .item.text] | last // "(no agent message found)"' "$f" + return + fi r=$(jq -r 'select(.type=="result") | .result // empty' "$f") [[ -n $r ]] && { printf '%s\n' "$r"; return; } jq -rs '[.[] | select(.type=="assistant") | .message.content[]? @@ -50,6 +81,11 @@ final() { } errors() { + if [[ $(provider) == codex ]]; then + jq -r 'select(.type=="error" or .type=="turn.failed") + | "ERROR " + (.message // .error.message // tostring)' "$f" + return + fi jq -r 'select(.type=="user") | .message.content[]? | select(.type=="tool_result" and (.is_error == true)) | "ERROR " + (.content | tostring | .[0:300])' "$f" @@ -63,6 +99,10 @@ errors() { # that settles "is earning its place" — it is a measurement, not an # impression, and it is the only one that separates a locator from a payload. weight() { + if [[ $(provider) == codex ]]; then + echo "weight view is not available in Codex JSONL" >&2 + return 2 + fi jq -rs ' ( [ .[] | select(.type=="assistant") | .message.content[]? | select(.type=="tool_use") | {key: .id, value: .name} ] @@ -81,6 +121,11 @@ weight() { # One line per session, for aggregating across a whole log directory: # for j in ~/src/ac-wt/log/*.jsonl; do bin/session.sh "$j" cost; done | sort -k3 -rn cost() { + if [[ $(provider) == codex ]]; then + jq -r --arg f "$(basename "$f")" ' + select(.type=="turn.completed") | [$f, "?", "?", "?"] | @tsv' "$f" + return + fi jq -r --arg f "$(basename "$f")" ' select(.type=="result") | [$f, (.num_turns // "?"), (.total_cost_usd // "?"), @@ -97,10 +142,20 @@ case "$view" in weight) weight ;; cost) cost ;; types) jq -r '.type' "$f" | sort | uniq -c | sort -rn ;; - delegate) jq -r 'select(.type=="assistant") | .message.content[]? - | select(.type=="tool_use" and (.name|test("Agent|Task";"i"))) - | "\(.name): \(.input | tostring | .[0:400])"' "$f" ;; + delegate) + if [[ $(provider) == codex ]]; then + jq -r 'select(.type=="item.completed" and .item.type=="mcp_tool_call") + | select((.item.server // "") | test("collaboration"; "i")) + | "\(.item.tool): \(.item.arguments | tostring | .[0:400])"' "$f" + else + jq -r 'select(.type=="assistant") | .message.content[]? + | select(.type=="tool_use" and (.name|test("Agent|Task";"i"))) + | "\(.name): \(.input | tostring | .[0:400])"' "$f" + fi ;; summary) + echo "== provider ==" + echo " $(provider)" + echo echo "== tool counts ==" tools | cut -f1 | sort | uniq -c | sort -rn echo @@ -108,14 +163,24 @@ case "$view" in files | sed 's/^/ /' || true echo echo "== delegation ==" - n=$(jq -r 'select(.type=="assistant") | .message.content[]? - | select(.type=="tool_use" and (.name|test("Agent|Task";"i"))) | .name' "$f" | wc -l) + if [[ $(provider) == codex ]]; then + n=$(jq -r 'select(.type=="item.completed" and .item.type=="mcp_tool_call") + | select((.item.server // "") | test("collaboration"; "i")) + | .item.tool' "$f" | wc -l) + else + n=$(jq -r 'select(.type=="assistant") | .message.content[]? + | select(.type=="tool_use" and (.name|test("Agent|Task";"i"))) | .name' "$f" | wc -l) + fi echo " subagent calls: $n" echo echo "== cost ==" - jq -r 'select(.type=="result") - | " turns=\(.num_turns // "?") cost=\(.total_cost_usd // "?") dur=\((.duration_ms // 0)/1000|floor)s"' "$f" \ - | grep . || echo " no result event — session incomplete, or this version does not emit one" + if [[ $(provider) == codex ]]; then + echo " Codex JSONL does not report USD cost or elapsed time" + else + jq -r 'select(.type=="result") + | " turns=\(.num_turns // "?") cost=\(.total_cost_usd // "?") dur=\((.duration_ms // 0)/1000|floor)s"' "$f" \ + | grep . || echo " no result event — session incomplete, or this version does not emit one" + fi echo echo "== final ==" final | head -40 From f728f84f8d3d3a01acbc03876d1ae9ca9460404d Mon Sep 17 00:00:00 2001 From: Markus Kovero Date: Wed, 2 Sep 2026 17:26:27 +0000 Subject: [PATCH 05/22] fix(master): route developer handbacks before PR lookup --- bin/master.sh | 23 ++++++++++++++++++++++- 1 file changed, 22 insertions(+), 1 deletion(-) diff --git a/bin/master.sh b/bin/master.sh index 2e9415c0..37c62567 100755 --- a/bin/master.sh +++ b/bin/master.sh @@ -371,7 +371,28 @@ drive() { echo " #$n: implementing${continue_arg:+ (continuation)}" "$BIN/implement.sh" "$n" $continue_arg $fg || { echo " #$n: implement failed"; return 1; } pr="$(pr_for "$n")" - [[ -n $pr ]] || { echo " #$n: no PR opened — check the session log"; return 1; } + if [[ -z $pr ]]; then + # A developer may discover an out-of-manifest dependency and correctly + # hand the issue back to design without committing or opening a PR. Read + # the issue labels before calling that a failed implementation; the next + # loop must drive the design gate against the preserved worktree. + ls="$(labels "$n")" || { echo " #$n: cannot read post-implementation labels"; return 1; } + if has needs-design "$ls"; then + echo " #$n: implementation handed back to architect — routing design" + continue + fi + if has needs-ux "$ls"; then + echo " #$n: implementation handed back to UX — routing design" + continue + fi + if has blocked "$ls" || has needs-discussion "$ls"; then + echo " #$n: implementation stopped on a human/design gate" + STATE=needs-human + return 0 + fi + echo " #$n: no PR opened — check the session log" + return 1 + fi echo " #$n: opened PR #$pr" st=0; qa_loop "$n" "$pr" || st=$? (( st == 0 )) || return "$st" From 5c0bfab71656f8e447a605bb5b44780da2c76b3e Mon Sep 17 00:00:00 2001 From: Markus Kovero Date: Wed, 2 Sep 2026 17:26:49 +0000 Subject: [PATCH 06/22] fix(master): stop epic on nonterminal child state --- bin/master.sh | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/bin/master.sh b/bin/master.sh index 37c62567..2afd7013 100755 --- a/bin/master.sh +++ b/bin/master.sh @@ -482,6 +482,10 @@ drive_epic() { needs-human|blocked) echo " #$c needs you — stopping epic." [[ -n ${KEEP_GOING:-} ]] || return 0 ;; + *) + echo " #$c did not reach a terminal workflow state — stopping epic." + echo " Inspect its labels and session log before continuing." + [[ -n ${KEEP_GOING:-} ]] || return 0 ;; esac done echo " #$e: all children processed" From ad85a575dc8f0ba6d381bcd43e8cb23c4183e4b7 Mon Sep 17 00:00:00 2001 From: Markus Kovero Date: Wed, 2 Sep 2026 18:18:26 +0000 Subject: [PATCH 07/22] feat(bin): prefix agent output with provider and role --- bin/codex-qa.sh | 28 ++++++++++++++-------------- bin/common.sh | 7 +++++-- 2 files changed, 19 insertions(+), 16 deletions(-) diff --git a/bin/codex-qa.sh b/bin/codex-qa.sh index d79ed054..1a6d1091 100755 --- a/bin/codex-qa.sh +++ b/bin/codex-qa.sh @@ -67,7 +67,7 @@ if ! gh_retry gh auth status >/dev/null 2>&1; then exit 1 fi -echo "Checking for open PRs with '$CLAUDE_LABEL', without '$CODEX_LABEL', '$NEEDS_WORK_LABEL', or '$REQUIRES_RIG_LABEL'..." +echo " Checking for open PRs with '$CLAUDE_LABEL', without '$CODEX_LABEL', '$NEEDS_WORK_LABEL', or '$REQUIRES_RIG_LABEL'..." # IMPORTANT: # This is exactly: @@ -108,16 +108,16 @@ if [[ -n "$prs_output" ]]; then fi if ((${#prs[@]} == 0)); then - echo "No PRs require Codex QA." + echo " No PRs require Codex QA." exit 0 fi -echo "Found ${#prs[@]} PR(s): ${prs[*]}" +echo " Found ${#prs[@]} PR(s): ${prs[*]}" for pr in "${prs[@]}"; do echo echo "============================================================" - echo "Codex QA: PR #$pr" + echo " Reviewing PR #$pr" echo "============================================================" # Re-check state immediately before handing the PR to Codex. @@ -156,22 +156,22 @@ for pr in "${prs[@]}"; do done if [[ "$has_claude_approved" != true ]]; then - echo "Skipping PR #$pr: '$CLAUDE_LABEL' is no longer present." + echo " Skipping PR #$pr: '$CLAUDE_LABEL' is no longer present." continue fi if [[ "$has_codex_approved" == true ]]; then - echo "Skipping PR #$pr: '$CODEX_LABEL' is already present." + echo " Skipping PR #$pr: '$CODEX_LABEL' is already present." continue fi if [[ "$has_needs_work" == true ]]; then - echo "Skipping PR #$pr: '$NEEDS_WORK_LABEL' is present." + echo " Skipping PR #$pr: '$NEEDS_WORK_LABEL' is present." continue fi if [[ "$has_requires_rig" == true ]]; then - echo "Skipping PR #$pr: '$REQUIRES_RIG_LABEL' is present." + echo " Skipping PR #$pr: '$REQUIRES_RIG_LABEL' is present." continue fi @@ -187,8 +187,8 @@ for pr in "${prs[@]}"; do --jq '.title' )" - echo "Title: $pr_title" - echo "URL: $pr_url" + echo " Title: $pr_title" + echo " URL: $pr_url" # Review the exact GitHub tip in an isolated, disposable worktree. Using # refs/pull/N/head also works when the PR branch originates from a fork. @@ -222,8 +222,8 @@ for pr in "${prs[@]}"; do active_worktree="$review_worktree" link_support "$active_worktree" - echo "Review worktree: $active_worktree [$pr_head]" - echo "Starting Codex..." + echo " Review worktree: $active_worktree [$pr_head]" + echo " Starting Codex..." # Codex owns the actual review and GitHub state transition. # The wrapper intentionally does not parse a PASS/FAIL file and does @@ -271,9 +271,9 @@ and do not add '$CODEX_LABEL'. The GitHub PR is the persistent record of your review. " - echo "Codex finished PR #$pr." + echo " Done. Review posted for PR #$pr." cleanup_worktree done echo -echo "Codex QA run complete." +echo " Codex QA run complete." diff --git a/bin/common.sh b/bin/common.sh index 269286df..9332c517 100755 --- a/bin/common.sh +++ b/bin/common.sh @@ -416,6 +416,7 @@ $prompt" local raw="$AC_LOG_DIR/$stamp.jsonl" local out="$AC_SESSION_DIR/$stamp.md" local last="$AC_LOG_DIR/$stamp.last.md" + local prefix="<${provider}/${role}> " # Stream to the terminal and retain the provider's native JSONL transcript. if [[ $provider == claude ]]; then @@ -436,7 +437,8 @@ $prompt" | if .type=="text" then .text elif .type=="tool_use" then " → \(.name) \(arg)" else empty end) - else empty end' || status=$? + else empty end' \ + | sed -u "s|^|$prefix|" || status=$? else local sandbox=workspace-write local -a model_arg=() @@ -451,7 +453,8 @@ $prompt" if .type=="item.completed" and .item.type=="agent_message" then .item.text elif .type=="item.started" and .item.type=="command_execution" then " → Bash " + ((.item.command // "") | gsub("[\\r\\n]+"; " ") | .[0:100]) - else empty end' || status=$? + else empty end' \ + | sed -u "s|^|$prefix|" || status=$? fi # Header says what this file is: a point-in-time record of one run, not From 67c2fa8c90de878054deda2eda2108a20c5664a2 Mon Sep 17 00:00:00 2001 From: Markus Kovero Date: Wed, 2 Sep 2026 18:23:31 +0000 Subject: [PATCH 08/22] refactor(qa): use shared provider runner for independent review --- bin/codex-qa.sh | 68 +++++++++++++++++-------------------------------- bin/common.sh | 4 ++- 2 files changed, 26 insertions(+), 46 deletions(-) diff --git a/bin/codex-qa.sh b/bin/codex-qa.sh index 1a6d1091..64f53895 100755 --- a/bin/codex-qa.sh +++ b/bin/codex-qa.sh @@ -225,51 +225,29 @@ for pr in "${prs[@]}"; do echo " Review worktree: $active_worktree [$pr_head]" echo " Starting Codex..." - # Codex owns the actual review and GitHub state transition. - # The wrapper intentionally does not parse a PASS/FAIL file and does - # not maintain local workflow state. - CARGO_TARGET_DIR="$AC_TARGET" codex exec \ - -c 'approval_policy="never"' \ - -s workspace-write \ - -c 'sandbox_workspace_write.network_access=true' \ - -C "$active_worktree" \ - --add-dir "$AC_TARGET" " -You are the independent Codex QA worker for GitHub PR #$pr. - -Read .agents/codex-qa.md before doing anything else. - -Also follow the repository's normal AGENTS.md instructions. - -Source bin/common.sh and run every GitHub CLI operation through gh_retry. -Inspect the PR, linked issue, PR discussion, -commits, complete diff, checks, relevant source, tests, configuration, -documentation, and git history as needed. - -You are responsible for the Codex QA labels and the PR comment. - -If the reviewed PR passes: - add '$CODEX_LABEL' - remove 'needs-work' if present - post the QA result as a PR comment - -If the reviewed PR has blocking defects: - add 'needs-work' - remove '$CODEX_LABEL' if present - post detailed findings as a PR comment - -Do not modify source code, tests, configuration, or git history. - -Do not create local workflow state files. - -Do not touch '$CLAUDE_LABEL' unless the repository's explicit workflow -instructions require it; normally that label belongs to Claude. - -Before applying the final label decision, re-check the PR's current HEAD -on GitHub. If the PR changed during your review, treat the review as stale -and do not add '$CODEX_LABEL'. - -The GitHub PR is the persistent record of your review. -" + # The shared runner owns provider invocation, transcript capture, model + # metadata, and prefixed terminal output. This role still owns the + # independent queue, disposable worktree, and Codex label contract. + cd "$active_worktree" + AC_TAG="pr-$pr" run codex-qa "Review PR #$pr in $AC_REPO as the independent Codex QA worker. + +Inspect the linked issue, triage/design/UX comments, PR discussion, commits, +complete diff, checks, relevant source, tests, configuration, documentation, +and git history as needed. You own the independent Codex QA labels and PR +comment: on pass add '$CODEX_LABEL' and remove 'needs-work'; on blocking +defects add 'needs-work' and remove '$CODEX_LABEL'. Never touch +'$CLAUDE_LABEL', 'in-review', 'requires-rig', or any agent label. + +Before applying the final label decision, re-check the PR's current HEAD. If +it changed since this review began, treat the review as stale and do not add +'$CODEX_LABEL'. The GitHub PR is the persistent review record." --read + + current_head="$(gh_retry gh pr view "$pr" --json headRefOid --jq '.headRefOid')" + if [[ "$current_head" != "$pr_head" ]]; then + echo " PR #$pr changed during review; result is stale and no approval was accepted." + cleanup_worktree + continue + fi echo " Done. Review posted for PR #$pr." cleanup_worktree diff --git a/bin/common.sh b/bin/common.sh index 9332c517..0aa57048 100755 --- a/bin/common.sh +++ b/bin/common.sh @@ -275,6 +275,8 @@ provider_for() { value="${!key}" elif [[ $role == qa ]]; then value=claude + elif [[ $role == codex-qa ]]; then + value=codex else value="${AC_PROVIDER:-claude}" fi @@ -322,7 +324,7 @@ run() { if [[ -z $turns ]]; then case "$role" in developer) turns=160 ;; # implementation across crates is long - qa|architect|audit) turns=120 ;; + qa|codex-qa|architect|audit) turns=120 ;; *) turns=80 ;; esac fi From f0d26b065463c972a721e8c496c10047a1c9af25 Mon Sep 17 00:00:00 2001 From: Markus Kovero Date: Wed, 2 Sep 2026 18:26:42 +0000 Subject: [PATCH 09/22] fix(master): recover unlabeled interrupted child --- bin/master.sh | 22 ++++++++++++++++++++-- 1 file changed, 20 insertions(+), 2 deletions(-) diff --git a/bin/master.sh b/bin/master.sh index 2afd7013..8547ed85 100755 --- a/bin/master.sh +++ b/bin/master.sh @@ -286,8 +286,26 @@ drive() { fi # Spec comment but no routing label: triage stopped mid-way, or a label # was removed by hand. Either way the next step is a decision, not a run. - echo " #$n: triage spec present but no routing label — yours to set" - STATE=needs-human; return 0 + # If a real implementation branch already contains work, however, an + # architect handback may have removed needs-design without restoring the + # ready label. Preserve the work and treat the existing branch as an + # implicit continuation; do not invent a fresh implementation. + if stale_branch "$n"; then + local unlabeled_wt="$WT_BASE/issue-$n" unlabeled_dirty=0 unlabeled_ahead=0 + [[ -d $unlabeled_wt ]] && unlabeled_dirty="$(git -C "$unlabeled_wt" status --porcelain | wc -l)" + unlabeled_ahead="$(git rev-list --count "origin/main..issue-$n" 2>/dev/null || echo 0)" + if (( unlabeled_dirty > 0 || unlabeled_ahead > 0 )); then + echo " #$n: routing label missing but existing implementation work is present — continuing" + ls+=$'\nready-to-implement' + continue_arg=--continue + else + echo " #$n: triage spec present but no routing label — yours to set" + STATE=needs-human; return 0 + fi + else + echo " #$n: triage spec present but no routing label — yours to set" + STATE=needs-human; return 0 + fi fi # ux step 6 runs first: it clears needs-ux but defers ready-to-implement to From 9a22c0083cb39f1ca9a4d75ea213ab8b93a359d6 Mon Sep 17 00:00:00 2001 From: Markus Kovero Date: Wed, 2 Sep 2026 20:49:08 +0000 Subject: [PATCH 10/22] feat(master): run independent QA before merge gate --- bin/codex-qa.sh | 14 +++++++++++--- bin/master.sh | 39 +++++++++++++++++++++++++++++++++++---- 2 files changed, 46 insertions(+), 7 deletions(-) diff --git a/bin/codex-qa.sh b/bin/codex-qa.sh index 64f53895..1e1818da 100755 --- a/bin/codex-qa.sh +++ b/bin/codex-qa.sh @@ -1,6 +1,7 @@ #!/usr/bin/env bash set -euo pipefail +requested_prs=() if [[ "${1:-}" == "--daemon" ]]; then if (($# != 1)); then echo "Usage: $0 [--daemon]" >&2 @@ -23,9 +24,11 @@ if [[ "${1:-}" == "--daemon" ]]; then fi sleep "$POLL_SECONDS" done -elif (($# != 0)); then - echo "Usage: $0 [--daemon]" >&2 - exit 2 +elif (($# > 0)); then + for arg in "$@"; do + [[ "$arg" =~ ^[0-9]+$ ]] || { echo "Usage: $0 [--daemon|...]" >&2; exit 2; } + requested_prs+=("$arg") + done fi source "$(dirname "$0")/common.sh" @@ -82,6 +85,8 @@ echo " Checking for open PRs with '$CLAUDE_LABEL', without '$CODEX_LAB # to Claude QA. Excluding needs-work prevents the daemon from reviewing the # same rejected tip again on every poll. A revised tip re-enters only after # Claude QA has reviewed it and restored claude-approved. +prs_output="" +if ((${#requested_prs[@]} == 0)); then prs_output="$( gh_retry gh pr list \ --state open \ @@ -101,6 +106,9 @@ prs_output="$( | .number ' )" +else + prs_output="$(printf '%s\n' "${requested_prs[@]}")" +fi prs=() if [[ -n "$prs_output" ]]; then diff --git a/bin/master.sh b/bin/master.sh index 8547ed85..0aeb6951 100755 --- a/bin/master.sh +++ b/bin/master.sh @@ -80,6 +80,22 @@ stale_branch() { # does. Require positive evidence that QA spoke. qa_evidence() is in common.sh. qa_comments() { qa_evidence "$1"; } +codex_gate() { + local pr="$1" pls + echo " PR #$pr: independent Codex QA" + "$BIN/codex-qa.sh" "$pr" || return 1 + pls="$(pr_labels "$pr")" || return 1 + if has needs-work "$pls"; then + echo " PR #$pr: Codex QA requested changes" + return 2 + fi + if ! has codex-approved "$pls"; then + echo " PR #$pr: Codex QA posted no approval — stopping" + return 1 + fi + echo " PR #$pr: independent Codex QA approved" +} + # Has anything routed this issue? Any one of these labels means triage, # architect, ux or you already decided where it goes. blocked, needs-discussion # and needs-clarification are not listed: drive() returns on them earlier, so @@ -192,8 +208,14 @@ qa_loop() { STATE=needs-rig; return 0 fi if has claude-approved "$ls"; then - echo " #$n PR #$pr: qa approved $head — yours to merge" - STATE=awaiting-merge; return 0 + if ! has codex-approved "$ls"; then + codex_gate "$pr" || { rc=$?; (( rc == 2 )) && continue; return "$rc"; } + ls="$(pr_labels "$pr")" + fi + if has codex-approved "$ls" && ! has needs-work "$ls"; then + echo " #$n PR #$pr: both QA gates passed — yours to merge" + STATE=awaiting-merge; return 0 + fi fi echo " #$n PR #$pr: rig gate cleared — full QA must incorporate its evidence" force=full @@ -233,8 +255,17 @@ qa_loop() { echo " see the review's 'rig verification required' field." STATE=needs-rig elif has claude-approved "$ls"; then - echo " #$n PR #$pr: qa reviewed and raised nothing — yours to merge" - STATE=awaiting-merge + if ! has codex-approved "$ls"; then + codex_gate "$pr" || { rc=$?; (( rc == 2 )) && continue; return "$rc"; } + ls="$(pr_labels "$pr")" + fi + if has codex-approved "$ls" && ! has needs-work "$ls"; then + echo " #$n PR #$pr: both QA gates passed — yours to merge" + STATE=awaiting-merge + else + echo " #$n PR #$pr: independent QA did not approve — stopping" + STATE=needs-human + fi else echo " #$n PR #$pr: qa posted no approval or routed finding — stopping" STATE=needs-human From 3a112a25fae2238b1fab12766809e3420ecff760 Mon Sep 17 00:00:00 2001 From: Markus Kovero Date: Wed, 2 Sep 2026 21:09:10 +0000 Subject: [PATCH 11/22] refactor(review): consolidate independent Codex QA --- .agents/codex-qa.md | 2 +- bin/codex-qa.sh | 265 -------------------------------------------- bin/master.sh | 2 +- bin/review.sh | 61 ++++++++++ 4 files changed, 63 insertions(+), 267 deletions(-) delete mode 100755 bin/codex-qa.sh diff --git a/.agents/codex-qa.md b/.agents/codex-qa.md index bb05b78c..a1a00b6a 100644 --- a/.agents/codex-qa.md +++ b/.agents/codex-qa.md @@ -37,7 +37,7 @@ tip on every poll. The developer removes both labels when picking up the finding. After the revision, Claude QA re-reviews the new tip and restores `claude-approved`; that puts the PR back in this queue. -There is no queue state anywhere but GitHub. `bin/codex-qa.sh` walks this list +There is no queue state anywhere but GitHub. `bin/review.sh --independent` walks this list and holds nothing. ## read order — this order is the mechanism, not a preference diff --git a/bin/codex-qa.sh b/bin/codex-qa.sh deleted file mode 100755 index 1e1818da..00000000 --- a/bin/codex-qa.sh +++ /dev/null @@ -1,265 +0,0 @@ -#!/usr/bin/env bash -set -euo pipefail - -requested_prs=() -if [[ "${1:-}" == "--daemon" ]]; then - if (($# != 1)); then - echo "Usage: $0 [--daemon]" >&2 - exit 2 - fi - - readonly POLL_SECONDS="${CODEX_QA_POLL_SECONDS:-300}" - if [[ ! "$POLL_SECONDS" =~ ^[1-9][0-9]*$ ]]; then - echo "ERROR: CODEX_QA_POLL_SECONDS must be a positive integer." >&2 - exit 2 - fi - - SCRIPT_PATH="$(realpath "$0")" - readonly SCRIPT_PATH - echo "Codex QA daemon started; polling every ${POLL_SECONDS}s." - - while true; do - if ! "$SCRIPT_PATH"; then - echo "Codex QA pass failed; retrying in ${POLL_SECONDS}s." >&2 - fi - sleep "$POLL_SECONDS" - done -elif (($# > 0)); then - for arg in "$@"; do - [[ "$arg" =~ ^[0-9]+$ ]] || { echo "Usage: $0 [--daemon|...]" >&2; exit 2; } - requested_prs+=("$arg") - done -fi - -source "$(dirname "$0")/common.sh" - -readonly CLAUDE_LABEL="claude-approved" -readonly CODEX_LABEL="codex-approved" -readonly NEEDS_WORK_LABEL="needs-work" -readonly REQUIRES_RIG_LABEL="requires-rig" - -# The active review worktree is removed on both success and failure. Refuse to -# reuse an existing path: it may contain evidence or edits from an interrupted -# run, and silently replacing those would be destructive. -active_worktree="" -cleanup_worktree() { - if [[ -n "$active_worktree" ]]; then - git -C "$ROOT" worktree remove --force "$active_worktree" || { - echo "WARNING: could not remove review worktree: $active_worktree" >&2 - return 1 - } - active_worktree="" - fi -} -trap cleanup_worktree EXIT - -# Run from the repository containing this script. -cd "$HERE" - -# Verify required commands before doing anything. -for command in gh codex git jq; do - if ! command -v "$command" >/dev/null 2>&1; then - echo "ERROR: required command not found: $command" >&2 - exit 1 - fi -done - -# Verify GitHub authentication up front. -if ! gh_retry gh auth status >/dev/null 2>&1; then - echo "ERROR: gh is not authenticated." >&2 - exit 1 -fi - -echo " Checking for open PRs with '$CLAUDE_LABEL', without '$CODEX_LABEL', '$NEEDS_WORK_LABEL', or '$REQUIRES_RIG_LABEL'..." - -# IMPORTANT: -# This is exactly: -# -# open -# AND claude-approved -# AND NOT codex-approved -# AND NOT needs-work -# AND NOT requires-rig -# -# A Codex failure leaves claude-approved in place because that label belongs -# to Claude QA. Excluding needs-work prevents the daemon from reviewing the -# same rejected tip again on every poll. A revised tip re-enters only after -# Claude QA has reviewed it and restored claude-approved. -prs_output="" -if ((${#requested_prs[@]} == 0)); then -prs_output="$( - gh_retry gh pr list \ - --state open \ - --label "$CLAUDE_LABEL" \ - --limit 100 \ - --json number,labels \ - --jq ' - .[] - | select( - all( - .labels[]?; - .name != "codex-approved" - and .name != "needs-work" - and .name != "requires-rig" - ) - ) - | .number - ' -)" -else - prs_output="$(printf '%s\n' "${requested_prs[@]}")" -fi - -prs=() -if [[ -n "$prs_output" ]]; then - mapfile -t prs <<<"$prs_output" -fi - -if ((${#prs[@]} == 0)); then - echo " No PRs require Codex QA." - exit 0 -fi - -echo " Found ${#prs[@]} PR(s): ${prs[*]}" - -for pr in "${prs[@]}"; do - echo - echo "============================================================" - echo " Reviewing PR #$pr" - echo "============================================================" - - # Re-check state immediately before handing the PR to Codex. - # Another process may have changed labels since gh pr list ran. - labels_output="$( - gh_retry gh pr view "$pr" \ - --json labels \ - --jq '.labels[].name' - )" - - labels=() - if [[ -n "$labels_output" ]]; then - mapfile -t labels <<<"$labels_output" - fi - - has_claude_approved=false - has_codex_approved=false - has_needs_work=false - has_requires_rig=false - - for label in "${labels[@]}"; do - case "$label" in - "$CLAUDE_LABEL") - has_claude_approved=true - ;; - "$CODEX_LABEL") - has_codex_approved=true - ;; - "$NEEDS_WORK_LABEL") - has_needs_work=true - ;; - "$REQUIRES_RIG_LABEL") - has_requires_rig=true - ;; - esac - done - - if [[ "$has_claude_approved" != true ]]; then - echo " Skipping PR #$pr: '$CLAUDE_LABEL' is no longer present." - continue - fi - - if [[ "$has_codex_approved" == true ]]; then - echo " Skipping PR #$pr: '$CODEX_LABEL' is already present." - continue - fi - - if [[ "$has_needs_work" == true ]]; then - echo " Skipping PR #$pr: '$NEEDS_WORK_LABEL' is present." - continue - fi - - if [[ "$has_requires_rig" == true ]]; then - echo " Skipping PR #$pr: '$REQUIRES_RIG_LABEL' is present." - continue - fi - - pr_url="$( - gh_retry gh pr view "$pr" \ - --json url \ - --jq '.url' - )" - - pr_title="$( - gh_retry gh pr view "$pr" \ - --json title \ - --jq '.title' - )" - - echo " Title: $pr_title" - echo " URL: $pr_url" - - # Review the exact GitHub tip in an isolated, disposable worktree. Using - # refs/pull/N/head also works when the PR branch originates from a fork. - pr_head="$( - gh_retry gh pr view "$pr" \ - --json headRefOid \ - --jq '.headRefOid' - )" - [[ -n "$pr_head" ]] || { - echo "ERROR: PR #$pr has no head commit." >&2 - exit 1 - } - - review_worktree="$WT_BASE/codex-pr-$pr" - if [[ -e "$review_worktree" ]]; then - echo "ERROR: review worktree path already exists: $review_worktree" >&2 - echo "Inspect it, then remove it with: git worktree remove '$review_worktree'" >&2 - exit 1 - fi - - require_space "$review_worktree" - mkdir -p "$WT_BASE" "$AC_TARGET" - git fetch -q origin "pull/$pr/head" - fetched_head="$(git rev-parse FETCH_HEAD)" - if [[ "$fetched_head" != "$pr_head" ]]; then - echo "ERROR: PR #$pr moved while preparing the review." >&2 - echo "Expected $pr_head, fetched $fetched_head; retry the run." >&2 - exit 1 - fi - git worktree add --detach "$review_worktree" "$pr_head" >/dev/null - active_worktree="$review_worktree" - link_support "$active_worktree" - - echo " Review worktree: $active_worktree [$pr_head]" - echo " Starting Codex..." - - # The shared runner owns provider invocation, transcript capture, model - # metadata, and prefixed terminal output. This role still owns the - # independent queue, disposable worktree, and Codex label contract. - cd "$active_worktree" - AC_TAG="pr-$pr" run codex-qa "Review PR #$pr in $AC_REPO as the independent Codex QA worker. - -Inspect the linked issue, triage/design/UX comments, PR discussion, commits, -complete diff, checks, relevant source, tests, configuration, documentation, -and git history as needed. You own the independent Codex QA labels and PR -comment: on pass add '$CODEX_LABEL' and remove 'needs-work'; on blocking -defects add 'needs-work' and remove '$CODEX_LABEL'. Never touch -'$CLAUDE_LABEL', 'in-review', 'requires-rig', or any agent label. - -Before applying the final label decision, re-check the PR's current HEAD. If -it changed since this review began, treat the review as stale and do not add -'$CODEX_LABEL'. The GitHub PR is the persistent review record." --read - - current_head="$(gh_retry gh pr view "$pr" --json headRefOid --jq '.headRefOid')" - if [[ "$current_head" != "$pr_head" ]]; then - echo " PR #$pr changed during review; result is stale and no approval was accepted." - cleanup_worktree - continue - fi - - echo " Done. Review posted for PR #$pr." - cleanup_worktree -done - -echo -echo " Codex QA run complete." diff --git a/bin/master.sh b/bin/master.sh index 0aeb6951..6f76c78b 100755 --- a/bin/master.sh +++ b/bin/master.sh @@ -83,7 +83,7 @@ qa_comments() { qa_evidence "$1"; } codex_gate() { local pr="$1" pls echo " PR #$pr: independent Codex QA" - "$BIN/codex-qa.sh" "$pr" || return 1 + "$BIN/review.sh" --independent "$pr" || return 1 pls="$(pr_labels "$pr")" || return 1 if has needs-work "$pls"; then echo " PR #$pr: Codex QA requested changes" diff --git a/bin/review.sh b/bin/review.sh index deeded13..bdfe9312 100755 --- a/bin/review.sh +++ b/bin/review.sh @@ -1,5 +1,6 @@ #!/usr/bin/env bash # review.sh [--full] [--since ] [--fg] +# review.sh --independent [--daemon|...] # # QA role. No Edit/Write against the tree — a reviewer that can fix what it # finds will fix it, and the finding never reaches you as a finding. @@ -12,6 +13,66 @@ # is the right way to fail: toward more scrutiny, not less. source "$(dirname "$0")/common.sh" + +# Independent review slot. This used to live in codex-qa.sh; keeping it here +# makes the review interface one command while preserving the separate label +# and model gate. +independent_review() { + local -a prs=() + if (($#)); then + local p + for p in "$@"; do + [[ $p =~ ^[0-9]+$ ]] || { echo "usage: review.sh --independent [...]" >&2; return 2; } + prs+=("$p") + done + else + mapfile -t prs < <(gh_retry gh pr list -R "$AC_REPO" --state open --label claude-approved \ + --limit 100 --json number,labels --jq '.[] | select(all(.labels[]?; .name != "codex-approved" and .name != "needs-work" and .name != "requires-rig")) | .number') + fi + ((${#prs[@]})) || { echo " No PRs require independent QA."; return 0; } + + local pr head wt labels + for pr in "${prs[@]}"; do + labels="$(gh_retry gh pr view "$pr" -R "$AC_REPO" --json labels --jq '.labels[].name')" + has_label() { printf '%s\n' "$labels" | grep -qx "$1"; } + has_label claude-approved || { echo " Skipping PR #$pr: claude-approved absent."; continue; } + has_label codex-approved && continue + has_label needs-work && continue + has_label requires-rig && continue + head="$(gh_retry gh pr view "$pr" -R "$AC_REPO" --json headRefOid --jq .headRefOid)" + wt="$WT_BASE/codex-pr-$pr" + [[ ! -e $wt ]] || { echo "review worktree already exists: $wt" >&2; return 1; } + require_space "$wt"; mkdir -p "$WT_BASE" "$AC_TARGET" + git fetch -q origin "pull/$pr/head" + [[ $(git rev-parse FETCH_HEAD) == "$head" ]] || { echo "PR #$pr changed while preparing review" >&2; return 1; } + git worktree add --detach "$wt" "$head" >/dev/null + link_support "$wt" + local rc=0 + ( cd "$wt" && AC_TAG="pr-$pr" run codex-qa "Review PR #$pr in $AC_REPO as the independent Codex QA worker. + +Inspect the linked issue, decisions, complete diff, checks, tests, and relevant +history. On pass add codex-approved and remove needs-work; on blocking defects +add needs-work and remove codex-approved. Never touch claude-approved, +in-review, requires-rig, or agent labels. Re-check the PR HEAD before applying +the final decision; if it changed, do not approve." --read ) || rc=$? + git worktree remove --force "$wt" || true + ((rc == 0)) || return "$rc" + echo " Done. Review posted for PR #$pr." + done +} + +if [[ ${1:-} == --independent ]]; then + shift + if [[ ${1:-} == --daemon ]]; then + shift; (($# == 0)) || { echo "usage: review.sh --independent --daemon" >&2; exit 2; } + poll="${CODEX_QA_POLL_SECONDS:-300}" + [[ $poll =~ ^[1-9][0-9]*$ ]] || { echo "CODEX_QA_POLL_SECONDS must be positive" >&2; exit 2; } + while true; do independent_review || echo " pass failed; retrying in ${poll}s" >&2; sleep "$poll"; done + fi + independent_review "$@" + exit $? +fi + n="${1:?usage: review.sh [--full] [--since ] [--fg]}"; shift || true mode=auto since="" From 65ad7dd0bc52496ce313ff156b0ee5943b6002a9 Mon Sep 17 00:00:00 2001 From: Markus Kovero Date: Wed, 2 Sep 2026 21:45:31 +0000 Subject: [PATCH 12/22] fix(master): route developer design handbacks --- bin/master.sh | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/bin/master.sh b/bin/master.sh index 6f76c78b..fc20f562 100755 --- a/bin/master.sh +++ b/bin/master.sh @@ -177,6 +177,17 @@ qa_loop() { # report "raised nothing", which is how a request-changes verdict turns # into "yours to merge". Leave the label. Stop. if [[ $pre == "$post" ]]; then + ils="$(labels "$n")" || { echo " #$n: cannot read issue labels — stopping"; return 1; } + if has needs-design "$ils"; then + echo " #$n PR #$pr: developer handed back to architect (needs-design)" + echo " the PR stays unchanged; architect must amend the design/manifest" + STATE=needs-design; return 0 + fi + if has needs-ux "$ils"; then + echo " #$n PR #$pr: developer handed back to ux (needs-ux)" + echo " the PR stays unchanged; ux must resolve the output decision" + STATE=needs-ux; return 0 + fi echo " #$n PR #$pr: revise pushed nothing — tip is still $post" echo " needs-work stays. re-reviewing an identical tip cannot change" echo " the verdict, so the block is one only you can clear: a rig" From 8d3e0a4421e6bc92d2f1739b1a8f38e9d45c034e Mon Sep 17 00:00:00 2001 From: Markus Kovero Date: Wed, 2 Sep 2026 21:48:24 +0000 Subject: [PATCH 13/22] fix(revise): allow issue design handbacks --- bin/revise.sh | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/bin/revise.sh b/bin/revise.sh index a362283a..85b8bf07 100755 --- a/bin/revise.sh +++ b/bin/revise.sh @@ -25,8 +25,11 @@ link_support "$wt" AC_TAG="pr-$n-rev" run developer "PR #$n in $AC_REPO is labelled needs-work. \ Read the agent:qa review comment on it and address every point raised. This is \ a revision: the branch and the PR already exist — commit and push to this \ -branch, do not open a new PR and do not change labels. Reply to the review \ -points in a PR comment so the next QA pass can see what you did and why. Any \ +branch, do not open a new PR. Preserve PR labels except the mandatory removal \ +of claude-approved when the branch changes. If the finding is outside the \ +manifest or requires a design/UX decision, apply needs-design or needs-ux on \ +the ISSUE as required by your role spec, leave the PR unchanged, and stop. Reply \ +to the review points in a PR comment so the next QA pass can see what you did and why. Any \ point you disagree with, say so there rather than silently leaving it. Before you start: check the linked issue for an architect or ux comment newer \ From 7960a4ee14e19413f270e6a991e33a26ddb7032a Mon Sep 17 00:00:00 2001 From: Markus Kovero Date: Thu, 3 Sep 2026 03:58:29 +0000 Subject: [PATCH 14/22] feat(master): wait for epic child merges --- bin/master.sh | 30 +++++++++++++++++++++++++++--- 1 file changed, 27 insertions(+), 3 deletions(-) diff --git a/bin/master.sh b/bin/master.sh index fc20f562..78c6ac3a 100755 --- a/bin/master.sh +++ b/bin/master.sh @@ -28,6 +28,9 @@ # override one role; likewise TRIAGE, ARCHITECT, and UX. # Per-role settings win over AC_PROVIDER. QA remains # model-bound until its two approval labels are migrated. +# AC_WAIT_MERGE=1 wait at an epic child until its human merge, then +# continue with the next child (default: stop and return) +# AC_MERGE_POLL_SECONDS=60 polling interval for AC_WAIT_MERGE # # Verify label names first — a wrong one makes this do nothing while looking # like it worked: gh label list -R mkovero/ac @@ -503,6 +506,23 @@ is_epic() { [[ -n "$(children "$1")" ]] } +wait_for_merge() { + local child="$1" pr state poll="${AC_MERGE_POLL_SECONDS:-60}" + [[ $poll =~ ^[1-9][0-9]*$ ]] || { echo " invalid AC_MERGE_POLL_SECONDS: $poll" >&2; return 2; } + while true; do + state="$(gh_retry gh issue view "$child" -R "$AC_REPO" --json state --jq .state 2>/dev/null || echo UNKNOWN)" + [[ $state == CLOSED ]] && { echo " #$child merged; continuing epic"; return 0; } + pr="$(pr_for "$child" || true)" + if [[ -n $pr ]] && gh_retry gh pr view "$pr" -R "$AC_REPO" --json state,mergedAt \ + --jq 'select(.state == "MERGED" or .mergedAt != null) | .number' >/dev/null 2>&1; then + echo " #$child PR merged; continuing epic" + return 0 + fi + echo " #$child awaiting your merge — checking again in ${poll}s" + sleep "$poll" + done +} + drive_epic() { local e="$1" kids c st mapfile -t kids < <(children "$e") @@ -532,10 +552,14 @@ drive_epic() { case "$STATE" in awaiting-merge) echo - echo " #$c is ready for your merge. Stopping here." + echo " #$c is ready for your merge." echo " Later children branch from main and would not see #$c's work." - echo " Merge it, then rerun: master.sh $e" - [[ -n ${KEEP_GOING:-} ]] || return 0 ;; + if [[ -n ${AC_WAIT_MERGE:-} ]]; then + wait_for_merge "$c" || return $? + else + echo " Merge it, then rerun: master.sh $e" + [[ -n ${KEEP_GOING:-} ]] || return 0 + fi ;; needs-rig) echo " #$c needs a rig measurement — stopping epic." [[ -n ${KEEP_GOING:-} ]] || return 0 ;; From cf1ed8fa1779e7560d504e2f17df190d7652946f Mon Sep 17 00:00:00 2001 From: Markus Kovero Date: Thu, 3 Sep 2026 04:01:57 +0000 Subject: [PATCH 15/22] fix(master): require actual merge before advancing --- bin/master.sh | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/bin/master.sh b/bin/master.sh index 78c6ac3a..c6a7f515 100755 --- a/bin/master.sh +++ b/bin/master.sh @@ -507,14 +507,15 @@ is_epic() { } wait_for_merge() { - local child="$1" pr state poll="${AC_MERGE_POLL_SECONDS:-60}" + local child="$1" pr="$2" state merged_at poll="${AC_MERGE_POLL_SECONDS:-60}" [[ $poll =~ ^[1-9][0-9]*$ ]] || { echo " invalid AC_MERGE_POLL_SECONDS: $poll" >&2; return 2; } + [[ -n $pr ]] || { echo " cannot identify the open PR for #$child" >&2; return 1; } while true; do state="$(gh_retry gh issue view "$child" -R "$AC_REPO" --json state --jq .state 2>/dev/null || echo UNKNOWN)" [[ $state == CLOSED ]] && { echo " #$child merged; continuing epic"; return 0; } - pr="$(pr_for "$child" || true)" - if [[ -n $pr ]] && gh_retry gh pr view "$pr" -R "$AC_REPO" --json state,mergedAt \ - --jq 'select(.state == "MERGED" or .mergedAt != null) | .number' >/dev/null 2>&1; then + merged_at="$(gh_retry gh pr view "$pr" -R "$AC_REPO" --json mergedAt \ + --jq '.mergedAt // empty' 2>/dev/null || true)" + if [[ -n $merged_at ]]; then echo " #$child PR merged; continuing epic" return 0 fi @@ -555,7 +556,7 @@ drive_epic() { echo " #$c is ready for your merge." echo " Later children branch from main and would not see #$c's work." if [[ -n ${AC_WAIT_MERGE:-} ]]; then - wait_for_merge "$c" || return $? + wait_for_merge "$c" "$(pr_for "$c" || true)" || return $? else echo " Merge it, then rerun: master.sh $e" [[ -n ${KEEP_GOING:-} ]] || return 0 From 70cc0372890232622cf564017dba3da2e957df45 Mon Sep 17 00:00:00 2001 From: Markus Kovero Date: Thu, 3 Sep 2026 04:21:50 +0000 Subject: [PATCH 16/22] spec: bound agent file reading --- .agents/AGENTS.md | 19 +++++++++++++++++++ .agents/architect.md | 4 +++- .agents/codex-qa.md | 6 ++++++ .agents/developer.md | 12 ++++++++++-- .agents/qa.md | 6 ++++++ bin/implement.sh | 10 +++++++++- 6 files changed, 53 insertions(+), 4 deletions(-) diff --git a/.agents/AGENTS.md b/.agents/AGENTS.md index e033970d..4cf7ed7f 100644 --- a/.agents/AGENTS.md +++ b/.agents/AGENTS.md @@ -100,5 +100,24 @@ acceptance criterion instead of living only in this section.** triage and archit An untagged numeric criterion defaults to `assumed` — the default fails toward more scrutiny, not less. +## bounded reading discipline — every role + +Reading a file means obtaining enough direct evidence for the decision at +hand; it does not mean printing every byte of every named file. Start with the +diff, named symbols, headings, or a search restricted to the already-authorised +paths, then open the surrounding region. Read a small file in full when that is +cheaper. Expand into callers, adjacent sections, or the full file only when the +local context leaves a concrete question unanswered. + +Do not batch-dump large files. Tool-output truncation in one batch does not +invalidate files or regions that were returned successfully, and is not a +reason to reread them. Continue only from the missing region. A required read +order governs the first inspection of each source; it does not require an +exhaustive linear scan before useful work begins. + +This rule narrows reading cost, not evidence. A location cited in a durable +comment must still have been opened, and a scope manifest remains the boundary +for what may be changed. + ## updating specs Agent specs are code. Change via PR like anything else. Spec make bad output → fix live in spec: tighten constraints, or add concrete example of bad behavior to relevant section. diff --git a/.agents/architect.md b/.agents/architect.md index f082a460..6ba85375 100644 --- a/.agents/architect.md +++ b/.agents/architect.md @@ -74,7 +74,9 @@ Cargo feature flags. Write "none" if there are none.} **implementation notes for developer** {Concrete pointers: which function to extend, which struct to modify, which test -to look at as a model. Not pseudocode — just orientation.} +to look at as a model. For every large manifest file, name the relevant symbol, +test, or document heading so the developer can begin with a bounded read rather +than scanning the full file. Not pseudocode — just orientation.} **scope** {tier-1 | tier-2 | scene | view | scope-none} diff --git a/.agents/codex-qa.md b/.agents/codex-qa.md index a1a00b6a..4ae64c22 100644 --- a/.agents/codex-qa.md +++ b/.agents/codex-qa.md @@ -51,6 +51,12 @@ and holds nothing. 4. The architect design comment, if the issue carries `design-approved`. 5. The diff, and the tree it applies to. +For steps 3–5, follow the shared bounded-reading rule: inspect changed hunks, +named symbols, and relevant document headings first. The order establishes +independence; it does not require dumping whole large files. If a combined tool +result truncates, continue only with the missing source or region and do not +reread completed ones. + **Then, and only then:** the `` and `` comments on the PR and the issue. diff --git a/.agents/developer.md b/.agents/developer.md index d085005e..9d13f528 100644 --- a/.agents/developer.md +++ b/.agents/developer.md @@ -38,10 +38,18 @@ for a cargo build. ## what you must do, in order ### step 1 — read -Read full triage spec comment + architect comment (if present). +Read the full triage spec comment and architect comment (if present) first. List files you intend to touch before writing code. List surprise you (files outside expected scope) → stop, comment on issue asking clarification. -Your prompt contains a file manifest from the architect. It is the output of a search that has already happened. Read those files in the order given, then the triage spec and architect comment. Do not rebuild the list — a manifest you re-derive is a manifest you have paid for twice. +Your prompt contains a file manifest from the architect. It is the output of a +search that has already happened. After reading the issue and design, inspect +every listed file once, in the order given. For a large source or +document, locate the symbol, changed hunk, or heading named by the issue and +open its surrounding region; do not `cat` the whole file merely to satisfy the +ordering rule. Expand only when that region leaves a concrete question. Do not +reread successful output because another file in the same tool batch was +truncated. Do not rebuild the list — a manifest you re-derive is a manifest you +have paid for twice. A search hit inside a manifest file is a locator. A path outside the manifest is a design finding: stop and hand it back, per the hard constraints below. diff --git a/.agents/qa.md b/.agents/qa.md index ba7535fb..dc3ee83b 100644 --- a/.agents/qa.md +++ b/.agents/qa.md @@ -107,6 +107,12 @@ file opens, and both are cheap: Both are leads, not findings. The checklist below runs in full either way. +Open changed hunks and their enclosing symbols first. Do not read an entire +large file merely because it appears in the diff or manifest; expand outward +only to answer a concrete caller, invariant, or compatibility question. If a +batched tool result truncates, continue from the missing region without +replaying regions already returned. + Check: - **correctness** — implementation do what spec says? - **numerical correctness** — estimator/measurement code: window sizes, normalization factors, array indices correct? diff --git a/bin/implement.sh b/bin/implement.sh index a9587bfb..820db1c7 100755 --- a/bin/implement.sh +++ b/bin/implement.sh @@ -77,7 +77,15 @@ The architect's design comment names the files this change touches. That list is $(printf '%s\n' "${files[@]}") -Read those files first, in that order, before anything else. Do not sweep the tree. Do not Glob or Grep to find what to work on — the search has already been done and its result is above. Grep is for locating a symbol inside a file already on this list. +Read the issue's triage spec and architect decision first, then inspect these +files in the listed order. For large files, use the issue, architect pointers, +and searches restricted to that file to open +the relevant symbols or headings; do not dump the entire file. The order means +first inspection, not exhaustive linear reading. If batched output truncates, +continue from the missing region without rereading successful output. Do not +sweep the tree or use Glob/Grep to rebuild the file list — that search has +already happened. Grep is for locating a symbol inside a file already on this +list. A file you need that is not on the list is a finding about the design, not a gap for you to fill. Stop, comment on the issue with the path and why it is needed, apply needs-design, and end the run. Adding it silently is the exact failure this list exists to prevent." "${run_args[@]}" From 094e4a5582019d6f45c8ca33126ea1795c15f965 Mon Sep 17 00:00:00 2001 From: Markus Kovero Date: Thu, 3 Sep 2026 06:42:46 +0000 Subject: [PATCH 17/22] fix(qa): bind approvals to reviewed head SHA --- .agents/codex-qa.md | 24 ++++++++++++++---------- .agents/qa.md | 2 ++ bin/review.sh | 23 +++++++++++++++++++++-- 3 files changed, 37 insertions(+), 12 deletions(-) diff --git a/.agents/codex-qa.md b/.agents/codex-qa.md index 4ae64c22..e349cd94 100644 --- a/.agents/codex-qa.md +++ b/.agents/codex-qa.md @@ -72,24 +72,28 @@ Your own comment carries `` as its first line. ## pre-check — stale approval -Before reviewing anything, compare the timestamp of the last commit on the -branch against the timestamp of the QA comment that applied `claude-approved`. +Before reviewing anything, establish that the newest `` +record explicitly names the current full PR head SHA as the reviewed tip (or as +the endpoint of its reviewed range). Search both GitHub PR comments and review +bodies: `gh pr view --json comments` does not include review bodies. -```bash -gh pr view N --json commits,comments,labels -``` +The runner performs this identity check before invoking you and supplies the +verified head SHA in the task prompt. You may confirm it, but do not replace it +with timestamp inference. Git commit timestamps are author-controlled and can +postdate a review that actually inspected that exact commit. -**Commits postdate the approval → the label is stale. Do not review.** Post a -short comment saying the approval predates commit `` and that a fresh -Claude QA pass is needed, and stop. Apply no labels. +**Newest QA record does not name the current head → the approval is stale or +unverifiable. Do not review.** Post a short comment requesting a fresh Claude +QA pass that names the full current SHA, and stop. Apply no labels. Reviewing past a stale label produces an independent review of a tree that Claude QA never approved, presented as the second half of a two-review gate. That is worse than no review, because the merge gate reads as satisfied. `developer.md` requires the pusher to remove `claude-approved`, and `qa.md` -removes it at re-review. This is the third place, and it is the only one that -catches a label that survived both. +removes it at re-review. This identity check is the third guard and catches a +label that survived both without rejecting a valid review because of clock +metadata. ## what you must do diff --git a/.agents/qa.md b/.agents/qa.md index dc3ee83b..efb66d6e 100644 --- a/.agents/qa.md +++ b/.agents/qa.md @@ -162,6 +162,8 @@ Post PR review in this structure: ``` +## qa — PR #N at + ### spec coverage | criterion | provenance | covered | notes | |---|---|---|---| diff --git a/bin/review.sh b/bin/review.sh index bdfe9312..ccf9f3c5 100755 --- a/bin/review.sh +++ b/bin/review.sh @@ -31,7 +31,7 @@ independent_review() { fi ((${#prs[@]})) || { echo " No PRs require independent QA."; return 0; } - local pr head wt labels + local pr head wt labels qa_record for pr in "${prs[@]}"; do labels="$(gh_retry gh pr view "$pr" -R "$AC_REPO" --json labels --jq '.labels[].name')" has_label() { printf '%s\n' "$labels" | grep -qx "$1"; } @@ -40,6 +40,16 @@ independent_review() { has_label needs-work && continue has_label requires-rig && continue head="$(gh_retry gh pr view "$pr" -R "$AC_REPO" --json headRefOid --jq .headRefOid)" + qa_record="$(gh_retry gh pr view "$pr" -R "$AC_REPO" --json comments,reviews --jq ' + ([.comments[] | {at: .createdAt, body: .body}] + + [.reviews[] | {at: .submittedAt, body: .body}]) + | map(select(.body | startswith(""))) + | sort_by(.at) | last | .body // empty')" + if [[ $qa_record != *"$head"* ]]; then + echo " PR #$pr: newest Claude QA record does not name current tip $head." >&2 + echo " A fresh Claude QA pass with explicit SHA evidence is required." >&2 + return 1 + fi wt="$WT_BASE/codex-pr-$pr" [[ ! -e $wt ]] || { echo "review worktree already exists: $wt" >&2; return 1; } require_space "$wt"; mkdir -p "$WT_BASE" "$AC_TARGET" @@ -50,6 +60,12 @@ independent_review() { local rc=0 ( cd "$wt" && AC_TAG="pr-$pr" run codex-qa "Review PR #$pr in $AC_REPO as the independent Codex QA worker. +The runner combined GitHub PR comments and reviews and verified that the newest + record explicitly covers current tip $head. Treat that SHA +identity as the fresh-Claude-approval pre-check. Do not compare commit and +comment timestamps; commit timestamps are author-controlled and are not +workflow chronology. + Inspect the linked issue, decisions, complete diff, checks, tests, and relevant history. On pass add codex-approved and remove needs-work; on blocking defects add needs-work and remove codex-approved. Never touch claude-approved, @@ -132,7 +148,8 @@ Scope of this pass: - The delta may break something outside itself. Where it plausibly does, say where you looked. -State at the top of your comment which commit range you reviewed. +State at the top of your comment which commit range you reviewed, including +the full $head_sha SHA as the range endpoint. Standards PDFs are NOT in this checkout — they are licence-restricted and gitignored. They are at $AC_STDDOCS. Each PDF has a .txt sibling extracted with pdftotext -layout: Grep that to find the clause, then Read the PDF at that region only. Do not page through a PDF looking for a clause. Extraction is lossy for equations, figures and some tables — where the clause turns on one of those, open the PDF itself. A citation you did not verify against the primary text is not a verified citation; if a document you need is genuinely missing from that directory, say which one rather than carrying the gap forward silently." else @@ -144,6 +161,8 @@ or human evidence may have changed without a code push. Apply the current QA spec, post a new superseding QA comment, and update labels to its verdict. Do not decline to post merely because the PR tip is unchanged. +State the full current head SHA $head_sha at the top of the review comment. + Standards PDFs are NOT in this checkout — they are licence-restricted and gitignored. They are at $AC_STDDOCS. Each PDF has a .txt sibling extracted with pdftotext -layout: Grep that to find the clause, then Read the PDF at that region only. Do not page through a PDF looking for a clause. Extraction is lossy for equations, figures and some tables — where the clause turns on one of those, open the PDF itself. A citation you did not verify against the primary text is not a verified citation; if a document you need is genuinely missing from that directory, say which one rather than carrying the gap forward silently." fi From 0def79920293a767f176868935f16b4bdd35183a Mon Sep 17 00:00:00 2001 From: Markus Kovero Date: Thu, 3 Sep 2026 07:09:25 +0000 Subject: [PATCH 18/22] fix(master): stop merge wait on conflicts --- bin/master.sh | 23 +++++++++++++++++------ 1 file changed, 17 insertions(+), 6 deletions(-) diff --git a/bin/master.sh b/bin/master.sh index c6a7f515..3add7437 100755 --- a/bin/master.sh +++ b/bin/master.sh @@ -507,25 +507,34 @@ is_epic() { } wait_for_merge() { - local child="$1" pr="$2" state merged_at poll="${AC_MERGE_POLL_SECONDS:-60}" + local child="$1" pr="$2" state pr_state merged_at mergeable merge_status + local poll="${AC_MERGE_POLL_SECONDS:-60}" [[ $poll =~ ^[1-9][0-9]*$ ]] || { echo " invalid AC_MERGE_POLL_SECONDS: $poll" >&2; return 2; } [[ -n $pr ]] || { echo " cannot identify the open PR for #$child" >&2; return 1; } while true; do state="$(gh_retry gh issue view "$child" -R "$AC_REPO" --json state --jq .state 2>/dev/null || echo UNKNOWN)" [[ $state == CLOSED ]] && { echo " #$child merged; continuing epic"; return 0; } - merged_at="$(gh_retry gh pr view "$pr" -R "$AC_REPO" --json mergedAt \ - --jq '.mergedAt // empty' 2>/dev/null || true)" - if [[ -n $merged_at ]]; then + pr_state="$(gh_retry gh pr view "$pr" -R "$AC_REPO" \ + --json mergedAt,mergeable,mergeStateStatus \ + --jq '[.mergedAt // "-", .mergeable // "UNKNOWN", .mergeStateStatus // "UNKNOWN"] | join("|")' \ + 2>/dev/null || true)" + IFS='|' read -r merged_at mergeable merge_status <<< "$pr_state" + if [[ -n $merged_at && $merged_at != - ]]; then echo " #$child PR merged; continuing epic" return 0 fi + if [[ $mergeable == CONFLICTING || $merge_status == DIRTY ]]; then + echo " #$child PR #$pr has merge conflicts with main — stopping wait." + echo " Resolve the conflicts and push; both QA approvals must then be refreshed." + return 3 + fi echo " #$child awaiting your merge — checking again in ${poll}s" sleep "$poll" done } drive_epic() { - local e="$1" kids c st + local e="$1" kids c st wait_rc mapfile -t kids < <(children "$e") (( ${#kids[@]} )) || { echo " #$e: no sub-issues or task-list refs found"; return 0; } echo " #$e: epic with ${#kids[@]} children — $(printf '#%s ' "${kids[@]}")" @@ -556,7 +565,9 @@ drive_epic() { echo " #$c is ready for your merge." echo " Later children branch from main and would not see #$c's work." if [[ -n ${AC_WAIT_MERGE:-} ]]; then - wait_for_merge "$c" "$(pr_for "$c" || true)" || return $? + wait_rc=0 + wait_for_merge "$c" "$(pr_for "$c" || true)" || wait_rc=$? + (( wait_rc == 0 )) || return 0 else echo " Merge it, then rerun: master.sh $e" [[ -n ${KEEP_GOING:-} ]] || return 0 From 5c3851db492080c676c5b55da416ee82da056769 Mon Sep 17 00:00:00 2001 From: Markus Kovero Date: Thu, 3 Sep 2026 07:35:50 +0000 Subject: [PATCH 19/22] feat(master): integrate conflicted epic PRs --- .agents/developer.md | 33 +++++++++++++++---- bin/implement.sh | 7 +++- bin/integrate.sh | 78 ++++++++++++++++++++++++++++++++++++++++++++ bin/master.sh | 25 ++++++++++---- 4 files changed, 130 insertions(+), 13 deletions(-) create mode 100755 bin/integrate.sh diff --git a/.agents/developer.md b/.agents/developer.md index 9d13f528..4fde062c 100644 --- a/.agents/developer.md +++ b/.agents/developer.md @@ -144,8 +144,29 @@ way to agreement is precisely the failure mode an independent second review exists to prevent, and it is the one outcome that would make the whole arrangement worthless while looking like it worked. +## integration mode + +Invoked as `"integrate main into PR #N"` when an approved PR conflicts after +another epic child merged. This is not a new feature pass. + +1. The runner has fetched the current PR head and `origin/main` into a + disposable worktree and attempted the merge. +2. Inspect every unmerged path and the two sides of each conflict. Conflict + paths are authorised integration scope even when only one side appeared in + the original architect manifest; do not edit unrelated paths. +3. Preserve both already-reviewed intents. If they cannot coexist without a + new design decision, abort the merge, apply `needs-design` on the issue, and + stop without pushing. +4. Resolve all conflicts, run the full workspace verification gate, commit the + merge, and push `HEAD` to the existing PR branch. Do not open another PR. + +Any integration push invalidates both commit-bound approvals. The runner +removes `claude-approved` and `codex-approved` after confirming the remote head +changed; do not restore either label. + ## hard constraints -- Touch only files justified by spec + listed in step 1. +- Touch only files justified by spec + listed in step 1. Integration mode also + authorises the merge's unmerged paths, and only for resolving those conflicts. - Search result is evidence about location, not licence to widen scope. Turn up file outside step 1 list → same rule: stop, comment on issue. - No reformat or style cleanup outside scope. `cargo fmt --check` must pass, but run `cargo fmt` only on files you edited. - No TODO comments. Implement it or open follow-up issue. @@ -159,12 +180,12 @@ arrangement worthless while looking like it worked. Stopping without a label is stopping with nothing to route on: the issue sit at `ready-to-implement` looking dispatchable, and the next run pick it up and hit the same ambiguity. -- **Pushing to a PR branch that carries `claude-approved` → remove - `claude-approved` in the same action.** Applies to every push in every mode: +- **Pushing to a PR branch that carries either approval → invalidate both + `claude-approved` and `codex-approved`.** Applies to every push in every mode: the issue flow, codex-finding mode, a one-line fixup, a `cargo fmt` reflow. - The label attests to a specific commit (`qa.md`, post-approval rule) and the - human merge gate reads it, so a push that leaves it standing hands a reviewer - an approval of a tree that no longer exists. Whether the commit "looks + Each label attests to a specific commit (`qa.md`, post-approval rule) and the + human merge gate reads them, so a push that leaves either standing hands a + reviewer an approval of a tree that no longer exists. Whether the commit "looks harmless" is not a criterion — the gate cannot distinguish a whitespace change from a logic change by trust, only by running. - Do not merge. Do not close issue. PR closes it automatically on merge. diff --git a/bin/implement.sh b/bin/implement.sh index 820db1c7..b1c8f560 100755 --- a/bin/implement.sh +++ b/bin/implement.sh @@ -17,6 +17,12 @@ for arg in "$@"; do esac done +# Epic children may have been planned before an earlier child merged. Always +# refresh the base before deciding whether an existing empty branch can be +# reused; an ff-only merge against a stale origin/main merely preserves the +# stale base while appearing to synchronize it. +git fetch -q origin main + # Reuse only the exact empty branch/worktree shape left by a failed preflight. # Anything dirty or ahead may contain a cut-off implementation and is refused. if git show-ref -q "refs/heads/issue-$n"; then @@ -36,7 +42,6 @@ if git show-ref -q "refs/heads/issue-$n"; then [[ -n $dirty || $ahead != 0 ]] || git -C "$wt" merge -q --ff-only origin/main else require_space "$wt" || exit 1 - git fetch -q origin main git worktree add -B "issue-$n" "$wt" origin/main >/dev/null fi diff --git a/bin/integrate.sh b/bin/integrate.sh new file mode 100755 index 00000000..58c4d42f --- /dev/null +++ b/bin/integrate.sh @@ -0,0 +1,78 @@ +#!/usr/bin/env bash +# integrate.sh [--fg] +# +# Bring an open PR up to current main after an earlier epic child merges. +# Conflict resolution is a developer task; the runner owns the disposable +# worktree, push, and invalidation of commit-bound approvals. + +source "$(dirname "$0")/common.sh" +n="${1:?usage: integrate.sh [--fg]}"; shift || true + +branch="$(gh_retry gh pr view "$n" -R "$AC_REPO" --json headRefName --jq .headRefName)" +old_head="$(gh_retry gh pr view "$n" -R "$AC_REPO" --json headRefOid --jq .headRefOid)" +[[ -n $branch && -n $old_head ]] || { echo "cannot resolve PR #$n branch" >&2; exit 1; } + +wt="$WT_BASE/integrate-pr-$n" +[[ ! -e $wt ]] || { + echo "integration worktree already exists: $wt" >&2 + echo "inspect or resume it; it may contain unresolved conflict work" >&2 + exit 1 +} + +require_space "$wt" +git fetch -q origin main "$branch" +[[ $(git rev-parse "origin/$branch") == "$old_head" ]] || { + echo "PR #$n moved while preparing integration; retry" >&2 + exit 1 +} +git worktree add -B "integrate-pr-$n" "$wt" "origin/$branch" >/dev/null +cd "$wt" +link_support "$wt" + +if git merge --no-edit origin/main; then + echo " PR #$n merged current main without conflicts" + ( cd ac-rs && cargo test --workspace ) + ( cd ac-rs && cargo clippy -- -D warnings ) + ( cd ac-rs && cargo fmt --check ) +else + echo " PR #$n requires developer conflict resolution" + AC_TAG="pr-$n-integrate" run developer "Integrate current origin/main into PR #$n in $AC_REPO. + +This is integration mode. A merge is already in progress in this disposable +worktree. Inspect every unmerged path and both sides of each conflict. Preserve +the reviewed intent of the PR and main, resolve only integration conflicts, +run the full workspace verification gate, and commit the merge. Do not push; +the runner verifies and pushes the result. If the two designs cannot coexist, +abort the merge, apply needs-design on the linked issue, and stop." "$@" +fi + +[[ -z $(git diff --name-only --diff-filter=U) ]] || { + echo "PR #$n still has unresolved paths; preserving $wt" >&2 + exit 1 +} +[[ -z $(git status --porcelain) ]] || { + echo "PR #$n integration left uncommitted work; preserving $wt" >&2 + exit 1 +} +new_head="$(git rev-parse HEAD)" +[[ $new_head != "$old_head" ]] || { + echo "PR #$n integration produced no commit; preserving $wt" >&2 + exit 1 +} + +git push origin "HEAD:$branch" +remote_head="" +for attempt in {1..10}; do + remote_head="$(gh_retry gh pr view "$n" -R "$AC_REPO" --json headRefOid --jq .headRefOid)" + [[ $remote_head == "$new_head" ]] && break + (( attempt == 10 )) || sleep 2 +done +[[ $remote_head == "$new_head" ]] || { + echo "PR #$n remote head did not reach integration commit $new_head" >&2 + exit 1 +} + +gh_retry gh pr edit "$n" -R "$AC_REPO" --remove-label claude-approved \ + --remove-label codex-approved --remove-label needs-work --add-label in-review >/dev/null +git worktree remove --force "$wt" +echo " PR #$n integrated main at $new_head; QA approvals invalidated" diff --git a/bin/master.sh b/bin/master.sh index 3add7437..f44735f0 100755 --- a/bin/master.sh +++ b/bin/master.sh @@ -524,9 +524,9 @@ wait_for_merge() { return 0 fi if [[ $mergeable == CONFLICTING || $merge_status == DIRTY ]]; then - echo " #$child PR #$pr has merge conflicts with main — stopping wait." - echo " Resolve the conflicts and push; both QA approvals must then be refreshed." - return 3 + echo " #$child PR #$pr has merge conflicts with main — integrating" + "$BIN/integrate.sh" "$pr" $fg || return 3 + return 4 fi echo " #$child awaiting your merge — checking again in ${poll}s" sleep "$poll" @@ -565,9 +565,22 @@ drive_epic() { echo " #$c is ready for your merge." echo " Later children branch from main and would not see #$c's work." if [[ -n ${AC_WAIT_MERGE:-} ]]; then - wait_rc=0 - wait_for_merge "$c" "$(pr_for "$c" || true)" || wait_rc=$? - (( wait_rc == 0 )) || return 0 + while true; do + wait_rc=0 + wait_for_merge "$c" "$(pr_for "$c" || true)" || wait_rc=$? + if (( wait_rc == 4 )); then + echo " #$c: integration pushed — rerunning both QA gates" + STATE="" + drive "$c" || { echo " #$c: post-integration QA aborted"; return 1; } + [[ $STATE == awaiting-merge ]] || { + echo " #$c stopped after integration in state: ${STATE:-unknown}" + return 0 + } + continue + fi + (( wait_rc == 0 )) || return 0 + break + done else echo " Merge it, then rerun: master.sh $e" [[ -n ${KEEP_GOING:-} ]] || return 0 From 18876391b49c716ae9f1ecf50e7a435bc8e225ea Mon Sep 17 00:00:00 2001 From: Markus Kovero Date: Thu, 3 Sep 2026 09:33:14 +0000 Subject: [PATCH 20/22] fix(runner): recover interrupted Codex revisions --- bin/common.sh | 11 ++++++++++- bin/master.sh | 12 +++++++++++- bin/revise.sh | 12 ++++++++++-- 3 files changed, 31 insertions(+), 4 deletions(-) diff --git a/bin/common.sh b/bin/common.sh index 0aa57048..fd457482 100755 --- a/bin/common.sh +++ b/bin/common.sh @@ -331,6 +331,14 @@ run() { local provider model provider="$(provider_for "$role")" || return model="$(model_for "$role" "$provider")" + local -a codex_write_dirs=(--add-dir "$AC_TARGET") + if [[ $role == developer ]]; then + # A linked worktree's .git file points back into the main repository. + # Without this grant Codex can edit source but cannot update the index, + # commit, or advance the local branch, which leaves a dirty projection of + # a commit it may still manage to create remotely. + codex_write_dirs+=(--add-dir "$(git rev-parse --path-format=absolute --git-common-dir)") + fi # The current approval labels are reviewer identities, not generic slots: # qa owns claude-approved and codex-qa owns codex-approved. Until those specs @@ -390,6 +398,7 @@ $prompt" local -a model_arg=() [[ -n $model ]] && model_arg=(-m "$model") codex -C "$PWD" -s "$sandbox" -a on-request \ + "${codex_write_dirs[@]}" \ "${model_arg[@]}" "${extra[@]}" "$task_prompt" fi return @@ -448,7 +457,7 @@ $prompt" codex exec -C "$PWD" -s "$sandbox" \ -c 'approval_policy="never"' \ -c "sandbox_${sandbox//-/_}.network_access=true" \ - --add-dir "$AC_TARGET" --json -o "$last" \ + "${codex_write_dirs[@]}" --json -o "$last" \ "${model_arg[@]}" "${extra[@]}" "$task_prompt" \ | tee "$raw" \ | jq -r --unbuffered ' diff --git a/bin/master.sh b/bin/master.sh index f44735f0..e9c23a54 100755 --- a/bin/master.sh +++ b/bin/master.sh @@ -169,7 +169,17 @@ qa_loop() { echo " #$n PR #$pr: revising (round $qa_round)" pre="$(gh_retry gh pr view "$pr" -R "$AC_REPO" --json headRefOid --jq .headRefOid)" \ || { echo " #$n: cannot read the tip — not starting a revise"; return 1; } - "$BIN/revise.sh" "$pr" $fg || { echo " #$n: revise failed"; return 1; } + local revise_rc=0 retry_head + "$BIN/revise.sh" "$pr" $fg || revise_rc=$? + if (( revise_rc != 0 && revise_rc != 130 && revise_rc != 143 )); then + retry_head="$(gh_retry gh pr view "$pr" -R "$AC_REPO" --json headRefOid --jq .headRefOid)" || return 1 + if [[ $retry_head == "$pre" ]]; then + echo " #$n: revision worker exited before pushing — retrying once in the preserved worktree" + revise_rc=0 + "$BIN/revise.sh" "$pr" $fg || revise_rc=$? + fi + fi + (( revise_rc == 0 )) || { echo " #$n: revise failed"; return 1; } post="$(gh_retry gh pr view "$pr" -R "$AC_REPO" --json headRefOid --jq .headRefOid)" \ || { echo " #$n: cannot read the tip — check the PR by hand"; return 1; } diff --git a/bin/revise.sh b/bin/revise.sh index 85b8bf07..78835b64 100755 --- a/bin/revise.sh +++ b/bin/revise.sh @@ -19,14 +19,22 @@ wt="$(ensure_worktree "$branch" "$WT_BASE/$branch")" \ || { echo "cannot get a worktree for $branch" >&2; exit 1; } cd "$wt" sparse_trim "$wt" -git pull -q --ff-only 2>/dev/null || true +git fetch -q origin "$branch" +# A cut-off Codex run may have pushed the PR commit while its sandbox was +# unable to advance this linked worktree's Git metadata. If the local tip is a +# strict ancestor of the remote PR tip, advance only HEAD/index and preserve +# every working-tree edit for the resumed revision. +if [[ $(git rev-parse HEAD) != $(git rev-parse "origin/$branch") ]] \ + && git merge-base --is-ancestor HEAD "origin/$branch"; then + git reset --mixed "origin/$branch" +fi link_support "$wt" AC_TAG="pr-$n-rev" run developer "PR #$n in $AC_REPO is labelled needs-work. \ Read the agent:qa review comment on it and address every point raised. This is \ a revision: the branch and the PR already exist — commit and push to this \ branch, do not open a new PR. Preserve PR labels except the mandatory removal \ -of claude-approved when the branch changes. If the finding is outside the \ +of claude-approved and codex-approved when the branch changes. If the finding is outside the \ manifest or requires a design/UX decision, apply needs-design or needs-ux on \ the ISSUE as required by your role spec, leave the PR unchanged, and stop. Reply \ to the review points in a PR comment so the next QA pass can see what you did and why. Any \ From a4dbb6ff6014a3ef92a54244e423276ebbd54840 Mon Sep 17 00:00:00 2001 From: Markus Kovero Date: Thu, 3 Sep 2026 10:18:38 +0000 Subject: [PATCH 21/22] fix(master): reset rounds after design handback --- bin/common.sh | 7 ------- bin/master.sh | 8 +++++--- 2 files changed, 5 insertions(+), 10 deletions(-) diff --git a/bin/common.sh b/bin/common.sh index fd457482..8e95766d 100755 --- a/bin/common.sh +++ b/bin/common.sh @@ -332,13 +332,6 @@ run() { provider="$(provider_for "$role")" || return model="$(model_for "$role" "$provider")" local -a codex_write_dirs=(--add-dir "$AC_TARGET") - if [[ $role == developer ]]; then - # A linked worktree's .git file points back into the main repository. - # Without this grant Codex can edit source but cannot update the index, - # commit, or advance the local branch, which leaves a dirty projection of - # a commit it may still manage to create remotely. - codex_write_dirs+=(--add-dir "$(git rev-parse --path-format=absolute --git-common-dir)") - fi # The current approval labels are reviewer identities, not generic slots: # qa owns claude-approved and codex-qa owns codex-approved. Until those specs diff --git a/bin/master.sh b/bin/master.sh index e9c23a54..1bf7f70d 100755 --- a/bin/master.sh +++ b/bin/master.sh @@ -43,9 +43,9 @@ STEPS="${AC_STEPS:-8}" DESIGN_PASSES="${AC_DESIGN_PASSES:-2}" UX_PASSES="${AC_UX_PASSES:-2}" -# dev→qa rounds, counted per ISSUE rather than per qa_loop() call. A design -# handback re-enters qa_loop, and a counter local to it would reset there — -# turning ROUNDS from a bound into a suggestion. +# dev→qa rounds are reset at the start of each issue and after an authoritative +# design/UX handback. DESIGN_PASSES, UX_PASSES, and STEPS independently bound +# cross-role loops. qa_round=0 fg=""; ids=() @@ -376,6 +376,7 @@ drive() { fi ran_ux=1; (( ++ux_passes )); echo " #$n: ux (pass $ux_passes)" "$BIN/ux.sh" "$n" $fg || { echo " #$n: ux failed"; return 1; } + qa_round=0 continue fi @@ -390,6 +391,7 @@ drive() { fi ran_design=1; (( ++design_passes )); echo " #$n: architect (pass $design_passes)" "$BIN/design.sh" "$n" $fg || { echo " #$n: design failed"; return 1; } + qa_round=0 continue fi From 8d223e9856a90419ff2fe7bbfa5ee9540eaf48ff Mon Sep 17 00:00:00 2001 From: Markus Kovero Date: Thu, 3 Sep 2026 10:23:02 +0000 Subject: [PATCH 22/22] spec: keep UX changes within architect manifest --- .agents/developer.md | 4 ++++ bin/revise.sh | 6 ++++-- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/.agents/developer.md b/.agents/developer.md index 4fde062c..f32a1b1d 100644 --- a/.agents/developer.md +++ b/.agents/developer.md @@ -167,6 +167,10 @@ changed; do not restore either label. ## hard constraints - Touch only files justified by spec + listed in step 1. Integration mode also authorises the merge's unmerged paths, and only for resolving those conflicts. +- Only architect amends the file manifest. A newer UX decision supersedes the + presentation requirement, but paths UX names are orientation, not write + authority. If implementing that decision needs an out-of-manifest file, + apply `needs-design` on the issue and stop so architect can amend the scope. - Search result is evidence about location, not licence to widen scope. Turn up file outside step 1 list → same rule: stop, comment on issue. - No reformat or style cleanup outside scope. `cargo fmt --check` must pass, but run `cargo fmt` only on files you edited. - No TODO comments. Implement it or open follow-up issue. diff --git a/bin/revise.sh b/bin/revise.sh index 78835b64..4dd6979a 100755 --- a/bin/revise.sh +++ b/bin/revise.sh @@ -36,8 +36,10 @@ a revision: the branch and the PR already exist — commit and push to this \ branch, do not open a new PR. Preserve PR labels except the mandatory removal \ of claude-approved and codex-approved when the branch changes. If the finding is outside the \ manifest or requires a design/UX decision, apply needs-design or needs-ux on \ -the ISSUE as required by your role spec, leave the PR unchanged, and stop. Reply \ -to the review points in a PR comment so the next QA pass can see what you did and why. Any \ +the ISSUE as required by your role spec, leave the PR unchanged, and stop. \ +Only architect may expand the file manifest: files named by a newer UX comment \ +remain out of scope until architect adds them. Reply to the review points in a \ +PR comment so the next QA pass can see what you did and why. Any \ point you disagree with, say so there rather than silently leaving it. Before you start: check the linked issue for an architect or ux comment newer \