From ef688dcda63245c9c5f03c52b0cef5f4edbcf41e Mon Sep 17 00:00:00 2001 From: Oriol Torrent Florensa Date: Thu, 13 Aug 2026 23:14:03 +0200 Subject: [PATCH 1/2] feat(repo): make an /archon-pr-review run identifiable, and give it an unattended mode MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An /archon-pr-review run left no trace that a machine ran it, and it could not run without a human. Its comments arrive under the maintainer's own login, carry no marker, and are told apart only by a heading 24 shapes and 40 pull requests wide. So the night shift's first mutating gate could not be written, and the command could not be called with nobody present. Changes: - Add a "Two markers, written by two actors" section: the run's best-effort `comprehensive-review-report` marker and the command's own `archon-pr-review-verification` marker, sharing no leading token. - Record that the workflow posts PR comments and never a PR review, and give the first-line listing query that finds either marker, with the cmd.exe quoting caveat that query carries. - Step 3: name the human path as the default, the unattended path as the one exception, and stop a caller that cannot state an authorisation. - Step 4: ask the run to mark every comment it posts and to print a `dimensions-run:` / `dimensions-not-run:` roll call in five fixed spellings, on a produced-an-artifact definition. - Step 6: select the run's two comments by marker, five fixed strings for what the marker fails to deliver, and one verification comment carrying facts and labelled claims — never a verdict. - Allow the Write tool, which posting the comment by `--body-file` needs. Fixes #357 --- .claude/commands/archon-pr-review.md | 128 +++++++++++++++++++++++++-- 1 file changed, 120 insertions(+), 8 deletions(-) diff --git a/.claude/commands/archon-pr-review.md b/.claude/commands/archon-pr-review.md index 6ba37409..470e916a 100644 --- a/.claude/commands/archon-pr-review.md +++ b/.claude/commands/archon-pr-review.md @@ -1,7 +1,7 @@ --- description: Dispatch archon-comprehensive-pr-review for one or more open PRs, with intent from the linked issue, Gitflow guardrails and a deduped monitor. argument-hint: [pr-number ...] -allowed-tools: Bash(gh *), Bash(git *), Bash(archon *), Task +allowed-tools: Bash(gh *), Bash(git *), Bash(archon *), Task, Write --- # /archon-pr-review @@ -21,10 +21,39 @@ Dispatching is **not** read-only. Before you run anything, know that the workflo - **Force-pushes the PR branch** if it is behind its base. The `sync` node runs `archon-sync-pr-with-main`, which despite its name rebases onto `$PR_BASE` read from the scope node — not literally `main`. It rebases and resolves conflicts itself, including "auto-resolving" ones it judges SIMPLE. - **Commits and pushes fixes** for every CRITICAL/HIGH finding, in the `implement-fixes` node. The PR you review is not the PR you get back. - **Lets the five parallel review agents push too**, before `synthesize` and `implement-fixes` ever run. Observed on PR #307: the review phase committed and pushed a fix at 20:45, mid-review, with `git -C push origin ` — and it did so from the **implementation run's** worktree, which was still on disk, not from its own. Two consequences: the head SHA can move at any point after dispatch, and you must not remove an implementation worktree while a review run against its branch is in flight. -- **Posts a `gh pr comment`** with the synthesised review. +- **Posts a `gh pr comment`** with the synthesised review, and a second one with the auto-fix report. So a green PR can come back with new commits and a red build. That is why Step 1 records the pre-review head SHA and Step 6 re-verifies against it. +## Two markers, written by two actors + +The workflow is bundled inside the Archon binary and cannot be edited here, so this command cannot make a run mark its own output. It can only **ask**. That is why there are two markers, written by two different actors, sharing no leading token: a reader matching one never matches the other. + +**The run's markers.** Step 4 asks the run to open every comment it posts with one of these as its exact first line. Best-effort — a run may ignore the ask, and Step 6 records that outcome rather than papering over it: + +```text + + +``` + +**This command's marker.** Step 6 writes it itself, as the first line of the single verification comment it posts: + +```text + +``` + +**The workflow posts PR comments and never a PR review.** The string `gh pr review` appears nowhere in the Archon binary; `gh pr comment` appears eight times. So a run adds nothing to `gh pr view --json reviews`, and anything reading that field cannot detect the run at all. On PR #364, where a run completed, every review on the pull request is Copilot's. Read the comments instead. + +This query lists how every comment on the PR opens — one output line per comment, wide enough to carry a whole marker: + +```sh +gh pr view --repo unic/unic-agents-plugins --json comments --jq ".comments[] | (.body | split(\"\n\")[0])[0:100]" +``` + +In that output, find `archon-pr-review-verification` for this command's own comment and `comprehensive-review-report` for the run's. The width is 100 because the run's marker is 94 characters and ` part=` opens at column 79 — an 80-character slice shows the marker and hides which half of the run wrote it. + +**The quoting of this filter is not verified on `cmd.exe`.** It is the one snippet in this file allowed to hold an inner string literal, and it needs one: reaching a comment's _first_ line takes `split("\n")`, jq offers no literal-free way to split on a newline, and the first line is where both markers live. No other snippet added here may hold one, and the exemption does not extend to a filter that selects on the marker string. + ## Step 1 — Derive the plan from the PRs (do not ask for what you can infer) For each PR number: @@ -62,7 +91,15 @@ archon workflow status # is another run already touching this PR's br ## Step 3 — Present the dispatch plan and confirm -Dispatching spawns autonomous agents, consumes significant tokens, and writes to the PR. **Always show the plan and get explicit go-ahead before running anything.** Present, per PR: number, base, linked issue, CI state, pre-review head SHA, what the workflow may push, and the exact dispatch command. Then ask to proceed. +Dispatching spawns autonomous agents, consumes significant tokens, and writes to the PR. There are two paths, and the first is the default. + +**The human path is the default.** Show the plan, wait for an explicit go-ahead, then dispatch. Present, per PR: number, base, linked issue, CI state, pre-review head SHA, what the workflow may push, and the exact dispatch command. Then ask to proceed. Take this path unless the exception below applies. + +**The unattended path is the exception, and one caller may take it:** an unattended run of the `night-shift` skill, which by definition has no human to ask. Every other caller takes the human path above — including an interactive session where the maintainer has merely stepped away. + +A `--unattended` flag would record nothing, because an agent can always grant itself a flag. So this path asks for something a flag cannot fake: **the caller states the authorisation that covers this PR** — the date of the authorised run, and the issue numbers that run named. Step 6 copies that statement verbatim onto the `authorisation:` line of the verification comment, so the audit trail exists even where the permission was self-granted. + +**A caller that reaches this path with no authorisation to state stops, and dispatches nothing.** Not a weaker authorisation, not a default, not an assumption that last night's grant still holds. Stop, and report that the unattended path was reached with no authorisation to state. ## Step 4 — Dispatch @@ -76,11 +113,17 @@ Dispatch command shape (fill the bracketed clauses from Step 1): ```sh archon workflow run archon-comprehensive-pr-review --from develop \ - "Review the PR https://github.com/unic/unic-agents-plugins/pull/, which implements https://github.com/unic/unic-agents-plugins/issues/. Gather the intent from issue # — its acceptance criteria are the contract. Check that the PR actually satisfies EVERY acceptance criterion, then perform a full PR review. GITFLOW: the PR base is 'develop', never 'main'. Do NOT change the PR base. Do NOT merge the PR. If a rebase is needed, rebase onto origin/develop only. Scope: . Never create or delete LICENSE files. CONTEXT: CI is on this PR head, so if you push any fix commit, re-run 'pnpm --filter test', 'pnpm --filter typecheck' AND 'pnpm ci:check' and wait for 'gh pr checks ' to be green again before reporting. Pay particular attention to: ." + "Review the PR https://github.com/unic/unic-agents-plugins/pull/, which implements https://github.com/unic/unic-agents-plugins/issues/. Gather the intent from issue # — its acceptance criteria are the contract. Check that the PR actually satisfies EVERY acceptance criterion, then perform a full PR review. GITFLOW: the PR base is 'develop', never 'main'. Do NOT change the PR base. Do NOT merge the PR. If a rebase is needed, rebase onto origin/develop only. Scope: . Never create or delete LICENSE files. MARKERS: open EVERY comment you post on this PR with an exact first line. For the synthesised review comment that first line is and for the auto-fix report it is the same line with part=fixes in place of part=review. Copy the head value exactly as it appears here; do not substitute the current head, which moves during the run. ROLL CALL: in the part=review comment, print two further lines, each opening at the start of its own line. The first opens dimensions-run: and names every review dimension whose agent PRODUCED AN ARTIFACT, meaning it executed and wrote its findings, whatever those findings were. The second opens dimensions-not-run: and names every dimension that produced no artifact. A dimension that ran and found nothing produced an artifact and belongs in dimensions-run. A dimension whose agent crashed, was killed or timed out before writing produced no artifact and belongs in dimensions-not-run, as does a dimension that never executed. Write dimensions-not-run: none only when all five produced an artifact. In both lines, name a dimension by exactly one of these five spellings and by no other string: code-review, error-handling, test-coverage, comment-quality, docs-impact. No prose paraphrase, no title case, no count in place of a name. CONTEXT: CI is on this PR head, so if you push any fix commit, re-run 'pnpm --filter test', 'pnpm --filter typecheck' AND 'pnpm ci:check' and wait for 'gh pr checks ' to be green again before reporting. Pay particular attention to: ." ``` Do not write `$anything.output` in the prompt — see Standing rules #2. +`` is the `headRefOid` Step 1 recorded. Write it into the prompt as a literal, so both of the run's comments carry the same value even though the head moves during the run. + +**The marker and the roll call are an ask, not a guarantee.** The workflow is bundled in the Archon binary, so nothing here can make a run comply. A run may post its comments unmarked, or marked and silent on the roll call. Step 6 records each of those outcomes in fixed words rather than inferring a value. + +**The roll call is the only published source for the per-dimension result.** What the workflow prints instead is a template constant — `**Reviewed by**: 5 specialised agents` — and its `synthesize` node fires at `trigger_rule: one_success`, so that literal `5` survives a run in which four of the five agents died. Nothing else visible to a reader of the PR names which dimensions produced an artifact. If the run ignores the ask, the fact is unavailable, not inferable — do not reconstruct it from the run's working state or its artifacts directory. That directory lives outside the repository, no reader of the PR can check it, and `archon workflow run --resume` reuses it, so an earlier attempt's files read as this attempt's. + ## Step 5 — Arm the monitor Grab the 32-char run ID (`archon workflow status`, ~5–10s after dispatch), then launch one `Monitor` (persistent), keyed on that run ID — never on a global active count. @@ -109,18 +152,87 @@ Watch the git and PR writes against a **Bash-only view** of the log, not the who Monitor signals: `LIMIT` (Claude usage cap tripped — external, re-run after reset) · `NODE-FAIL` (read the log) · `GIT-WRITE` (the head moved — expect CI to re-run; confirm with `gh pr view --json headRefOid`) · `PR-WRITE` (a `gh pr merge` here is a bug, investigate immediately; a `gh pr edit --base` means the base moved) · `FINDING` (a CRITICAL/HIGH the auto-fix node will try to fix) · `[done]` (run left the active list). -## Step 6 — Verify what the review did +## Step 6 — Verify what the review did, and post the verification comment -When the run leaves the active list, do not trust its summary. Check the PR itself: +When the run leaves the active list, do not trust its summary. Read the PR itself: ```sh gh pr view --repo unic/unic-agents-plugins --json baseRefName,headRefOid,mergeable,mergeStateStatus,commits gh pr checks --repo unic/unic-agents-plugins git log --oneline .. # what the review pushed -gh pr view --repo unic/unic-agents-plugins --comments | tail -60 +gh pr view --repo unic/unic-agents-plugins --json comments --jq ".comments[] | (.body | split(\"\n\")[0])[0:100]" ``` -Report, per PR: base still `develop`, commits the review added, CI state after those commits, and the findings it left open. A review is complete only when the comment is posted **and** CI is green on the new head. If the auto-fix pushed a commit that broke CI, that is the review's fault and the PR is worse than before — say so plainly. +### Select this run's two comments by their marker + +Never by heading text, author or recency. A comment is this run's when its **first line** is a `comprehensive-review-report` marker whose `head=` equals the SHA Step 1 recorded and whose `part=` names the comment being read. Find them in the listing above, then open each one. Do not filter on the marker string inside a `--jq` expression. + +Heading text cannot do this job. `# 🔍 Comprehensive PR Review` spans 40 pull requests in this repository and only five ever carried an `archon-comprehensive-pr-review` run; PR #307 carries that heading twice, from two different workflows; and the fix report's heading reads `## Fixes Applied` on four PRs and `## Fixes applied` on the fifth. Copying rows verbatim off a marked comment makes both irrelevant. + +Where the marker does not deliver, the field takes one of these values, in these words and no others: + +| What you found | The field reads | +| --------------------------------------------------------------------------------- | ------------------------------------------------ | +| no comment matches a `part=` | `not published by the run` | +| more than one comment matches a `part=` | `ambiguous ( comments)`, and select none | +| a matching `part=review` comment that omits the roll-call lines | `roll-call lines absent from the marked comment` | +| a matching comment carrying no severity table at all | `no severity table in the marked comment` | +| a matching comment whose severity table carries neither a CRITICAL nor a HIGH row | `no CRITICAL or HIGH row in the marked comment` | + +Never write `unknown`, `0` or `none` in place of a missing source. `none` is a value the run may publish, not one this command substitutes: a run may print `0` as a real count, so an absent table wearing a `0` would read as a clean one. + +The last three strings exist rather than folding into the first because the comment _was_ posted and only a part of it is missing. Nothing posted means chase the run; posted-but-silent means the run ignored the ask in Step 4. A human meeting a stalled gate needs to know which. + +### Post exactly one verification comment + +Write the body to a file and post the file, so no shell quoting stands between you and the exact first line: + +```sh +gh pr comment --repo unic/unic-agents-plugins --body-file +``` + +The shape, first line first: + +```text + +run: +head-before: +head-after: +commits-pushed: + +dimensions-run: +source: the run's own claim +dimensions-not-run: +source: the run's own claim +findings-found: + +source: found by the run, before any fix +fixes-claimed: + + +source: the run's own claim +ci: +source: CI's word +authorisation: +``` + +`` is the 32-character ID Step 5 read from `archon workflow status`. `` is `headRefOid` read **after** the run left the active list. `authorisation:` appears on the unattended path only; every other field is always present, and where its source did not deliver it carries one of the five strings above. + +**Field format: name, colon, one space, then the value and nothing before it.** Every field opens at column 1, and the value starts immediately after that single space. Nothing may sit between the colon and the value — no source label, no parenthesis, no marker, no emoji. A gate reads a field's value as the text from immediately after `: ` to the end of that field's own line, and tests it as a prefix. Under a looser reading, `fixes-claimed: (the run's own claim) not published by the run` satisfies "labelled" and defeats every such test — which is fail-open on the one gate this marker exists to arm. + +Where a field is sourced, the source sits on its **own** line directly beneath it, reading `source: