diff --git a/.agents/AGENTS.md b/.agents/AGENTS.md index 65ffb6c0..4cf7ed7f 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. @@ -96,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 1b219141..e349cd94 100644 --- a/.agents/codex-qa.md +++ b/.agents/codex-qa.md @@ -22,19 +22,22 @@ 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 +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 @@ -48,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. @@ -63,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 @@ -95,6 +108,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 +271,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/developer.md b/.agents/developer.md index d085005e..f32a1b1d 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. @@ -136,8 +144,33 @@ 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. +- 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. @@ -151,12 +184,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/.agents/qa.md b/.agents/qa.md index f389b54a..efb66d6e 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 → `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 → + `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 @@ -94,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? @@ -143,6 +162,8 @@ Post PR review in this structure: ``` +## qa — PR #N at + ### spec coverage | criterion | provenance | covered | notes | |---|---|---|---| @@ -171,7 +192,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 @@ -186,12 +207,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 @@ -201,6 +226,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 @@ -262,11 +294,18 @@ 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 diff --git a/bin/codex-qa.sh b/bin/codex-qa.sh deleted file mode 100755 index e42886b3..00000000 --- a/bin/codex-qa.sh +++ /dev/null @@ -1,252 +0,0 @@ -#!/usr/bin/env bash -set -euo pipefail - -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 - echo "Usage: $0 [--daemon]" >&2 - exit 2 -fi - -source "$(dirname "$0")/common.sh" - -readonly CLAUDE_LABEL="claude-approved" -readonly CODEX_LABEL="codex-approved" - -# 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' and without '$CODEX_LABEL'..." - -# IMPORTANT: -# This is exactly: -# -# open -# AND claude-approved -# AND NOT codex-approved -# -# 'needs-work' intentionally does not participate in eligibility. -prs_output="$( - gh_retry gh pr list \ - --state open \ - --label "$CLAUDE_LABEL" \ - --limit 100 \ - --json number,labels \ - --jq ' - .[] - | select( - all( - .labels[]?; - .name != "codex-approved" - ) - ) - | .number - ' -)" - -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 "Codex QA: 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 - - for label in "${labels[@]}"; do - case "$label" in - "$CLAUDE_LABEL") - has_claude_approved=true - ;; - "$CODEX_LABEL") - has_codex_approved=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 - - 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..." - - # 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. -" - - echo "Codex finished PR #$pr." - cleanup_worktree -done - -echo -echo "Codex QA run complete." diff --git a/bin/common.sh b/bin/common.sh index 5a0005cf..8e95766d 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,56 @@ 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 + elif [[ $role == codex-qa ]]; then + value=codex + 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 @@ -257,27 +324,23 @@ 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 - # 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")" + local -a codex_write_dirs=(--add-dir "$AC_TARGET") + + # 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 +352,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 +376,24 @@ 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 \ + "${codex_write_dirs[@]}" \ + "${model_arg[@]}" "${extra[@]}" "$task_prompt" + fi return fi @@ -332,20 +419,20 @@ run() { 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, 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]; @@ -354,32 +441,60 @@ run() { | 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=() + [[ -n $model ]] && model_arg=(-m "$model") + codex exec -C "$PWD" -s "$sandbox" \ + -c 'approval_policy="never"' \ + -c "sandbox_${sandbox//-/_}.network_access=true" \ + "${codex_write_dirs[@]}" --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' \ + | sed -u "s|^|$prefix|" || 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..b1c8f560 100755 --- a/bin/implement.sh +++ b/bin/implement.sh @@ -1,29 +1,58 @@ #!/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 + +# 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 -# -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 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,14 +64,34 @@ 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: $(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." "$@" +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/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 348b0257..1bf7f70d 100755 --- a/bin/master.sh +++ b/bin/master.sh @@ -22,6 +22,15 @@ # 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. +# 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 @@ -34,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=() @@ -74,6 +83,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/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" + 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 @@ -144,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; } @@ -155,6 +190,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" @@ -171,20 +217,32 @@ 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 + 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 fi echo " #$n PR #$pr: qa review${force:+ (full — design changed since the last pass)}" @@ -217,12 +275,24 @@ 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 + elif has claude-approved "$ls"; then + 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 reviewed and raised nothing — yours to merge" - STATE=awaiting-merge + echo " #$n PR #$pr: qa posted no approval or routed finding — stopping" + STATE=needs-human fi return 0 fi @@ -235,7 +305,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 @@ -271,8 +341,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 @@ -288,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 @@ -302,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 @@ -331,23 +421,53 @@ 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; } + 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" @@ -398,8 +518,35 @@ is_epic() { [[ -n "$(children "$1")" ]] } +wait_for_merge() { + 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; } + 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 — integrating" + "$BIN/integrate.sh" "$pr" $fg || return 3 + return 4 + 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[@]}")" @@ -427,16 +574,39 @@ 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 + 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 + fi ;; needs-rig) echo " #$c needs a rig measurement — stopping epic." [[ -n ${KEEP_GOING:-} ]] || return 0 ;; 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" diff --git a/bin/review.sh b/bin/review.sh index b10aaf26..ccf9f3c5 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,82 @@ # 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 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"; } + 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)" + 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" + 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. + +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, +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="" @@ -71,12 +148,21 @@ 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 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. + +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 diff --git a/bin/revise.sh b/bin/revise.sh index a362283a..4dd6979a 100755 --- a/bin/revise.sh +++ b/bin/revise.sh @@ -19,14 +19,27 @@ 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 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 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. \ +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 \ 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