Skip to content

docs: document the automated PR review and fix the decision count - #43

Open
bokelley wants to merge 5 commits into
mainfrom
docs/argus-review-expectations
Open

bokelley wants to merge 5 commits into
mainfrom
docs/argus-review-expectations

Conversation

@bokelley

@bokelley bokelley commented Oct 16, 2026 •

Copy link
Copy Markdown
Contributor

Follow-up to #42. Two real documentation gaps, and the live CI proof that the repaired Argus workflow actually reviews a PR.

What this fixes

The automated review is undocumented. Every contributor gets a review from aao-ipr-bot[bot] and nothing in the repo explains it. CONTRIBUTING.md gains a short section covering the parts that actually surprise people:

  • the reviewer and the IPR signature check share one GitHub App identity, which is why the "IPR bot" appears to review Java;
  • it runs read-only — no shell, no writes, no network fetches;
  • its approval does not satisfy branch protection (D21 requires a human code owner, and the bot is not one);
  • a review that cannot complete is a red build plus a comment, never a silent pass — the Argus AI PR review crashes in claude-code-action before reading the diff #41 behaviour, now inverted;
  • PRs that change the review system are deliberately skipped, because Argus reads those files from the base branch and would be judging a change to its own gates using the version that change replaces.

The confirmed-decision count is stale. ROADMAP.md has carried D1–D24 since D22–D24 landed, but CONTRIBUTING.md and CLAUDE.md both still said 21 — in the exact lines that tell a new contributor or coding agent what to read first.

CLAUDE.md also gains a short Argus section pointing at scripts/check-ai-review-workflow.cjs and listing the guarded paths, so an agent editing the workflow finds the validator before CI does.

Known gap left for a separate PR

.github/ai-review/expert-adcp-reviewer.md carries the same stale D1–D21 reference on lines 9 and 138. That file is a self-modification guarded path: fixing it here would pause Argus and defeat the purpose of this PR. It needs its own PR with a human reviewer.

Why this PR exists

#41 is not provable on #42 itself — that PR changes ai-review.yml, so the self-modification gate pauses Argus by design. This PR touches only CONTRIBUTING.md, CLAUDE.md and .changeset/, none of which are guarded, so Argus runs normally on it. The code_review check on this PR is the end-to-end proof that the fix works in CI.

Checks run locally: ./gradlew build :build-logic:test, node scripts/check-ai-review-workflow.cjs, npx changeset status, npx commitlint. No hooks skipped.

🤖 Generated with Claude Code


Open workspace in Conductor

Contributors get a review from `aao-ipr-bot[bot]` on every PR and nothing in
the repo explains it — not that the IPR App and the reviewer share one
identity, not that the reviewer is read-only, not that its approval does not
satisfy D21's code-owner requirement, and not that PRs touching the review
system are deliberately skipped.

Also corrects a stale count: `ROADMAP.md` has carried D1–D24 since D22–D24
landed, but `CONTRIBUTING.md` and `CLAUDE.md` still said 21. Both are the
first thing a new contributor or coding agent is told to read.

`.github/ai-review/expert-adcp-reviewer.md` carries the same stale D1–D21
reference in two places. It is a self-modification guarded path, so fixing it
here would pause Argus on this PR; it needs its own PR.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@bokelley
bokelley requested a review from MichielDean as a code owner October 16, 2026 16:24
aao-ipr-bot[bot]
aao-ipr-bot Bot previously approved these changes Oct 16, 2026

@aao-ipr-bot aao-ipr-bot Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Docs-and-changeset-only PR that fixes a stale decision count and documents the Argus review contract. The prose matches what .github/workflows/ai-review.yml actually does, which is what matters for a doc describing a gate.

No source, build or workflow files are touched, so no D-decision drift is possible here. CLAUDE.md is governance rather than docs-only, but the change is descriptive and doesn't alter any rule. code-reviewer is not required because no source code changed.

Things I checked

  • Decision count. ROADMAP.md L33–L36 carry D21–D24, so "24 confirmed decisions (D1–D24)" is correct in both CLAUDE.md:15 and CONTRIBUTING.md:25.
  • Guarded paths. The self-modification list in the docs matches the grep -E gate at ai-review.yml:96 for ai-review.yml, .github/ai-review/, CODEOWNERS, scripts/check-ai-review-workflow.cjs, .claude/agents/ and .agents/roles/. One path is missing; see nit 1.
  • Read-only claim. ai-review.yml:313-314 allows only Read,Glob,Grep,Task and disallows Bash,Edit,Write,NotebookEdit,MultiEdit,WebFetch,WebSearch. "No shell, no file writes, no network fetches" is accurate.
  • Bot identity. The review token is minted from secrets.IPR_APP_ID / IPR_APP_PRIVATE_KEY (ai-review.yml:327-328), so a review showing up as the IPR App is correctly explained.
  • Fail-closed claim. ai-review.yml:420-461 posts or updates a single failure notice. :491-507 exits 1 when review_posted != true. A missing review produces a red build, as the docs say. Fail-closed beats fail-open, and the docs now say so.
  • D21. It requires one code-owner approving review (ROADMAP.md:33). The statement that bot approval does not satisfy branch protection is consistent with that.
  • Validator wiring. validate-ai-review-config.yml is named Validate Argus review config and runs node scripts/check-ai-review-workflow.cjs at L33. That matches the CLAUDE.md text.
  • Changeset. It is empty, and that's appropriate: nothing affects published artifacts. D18 is satisfied.
  • Workspace artifact scan. It's clean: 3 files, no .wt-*, .context/ or local scratch.

Follow-ups (non-blocking — file as issues)

  • .github/ai-review/expert-adcp-reviewer.md still says D1–D21. The PR body already flags this as a guarded-path change that needs its own PR and a human reviewer. Agreed. That prompt is load-bearing for every review, so the count should be fixed soon.

Minor nits (non-blocking)

  1. Guarded list is missing one path. The gate at ai-review.yml:96 also covers .github/workflows/validate-ai-review-config.yml. Neither CLAUDE.md:37 nor CONTRIBUTING.md:71 lists it. Add it, or the next agent editing the validator workflow will be surprised.
  2. "Every non-draft PR" is slightly too broad. ai-review.yml:44 also skips PRs opened by dependabot[bot]. One clause in CONTRIBUTING.md:61 and CLAUDE.md:34 would fix it.
  3. CODEOWNERS path. The file lives at .github/CODEOWNERS. Writing the full path would match the other entries in the list.

It's notable that a PR documenting the reviewer is also the first proof that the reviewer runs.

LGTM. Follow-ups noted below.

Three accuracy fixes from Argus's review of this PR:

- the documented self-modification list omitted
  `.github/workflows/validate-ai-review-config.yml`, which the gate does
  cover — the next agent editing the validator workflow would have been
  surprised by the pause
- "every non-draft PR" skipped the dependabot exclusion
- `CODEOWNERS` is written as `.github/CODEOWNERS`, matching the gate and the
  other entries

The documented list is now identical to the seven paths in the `grep -E`
pattern at `ai-review.yml:96`.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
aao-ipr-bot[bot]
aao-ipr-bot Bot previously approved these changes Oct 16, 2026

@aao-ipr-bot aao-ipr-bot Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Documentation fix for an undocumented, load-bearing CI surface, plus a stale decision count corrected in the two files that tell contributors and agents what to read first. Every claim in the new sections checks out against ai-review.yml as it sits on main.

Things I checked

  • Decision count. ROADMAP.md has D22, D23 and D24 rows at L34–L36 directly after D21 (L33). "24 numbered decisions (D1–D24)" is accurate for the table. Both CLAUDE.md:15 and CONTRIBUTING.md:25 are updated.
  • Guarded-path list matches the gate exactly. The grep at .github/workflows/ai-review.yml:96 covers ai-review.yml, validate-ai-review-config.yml, .github/ai-review/, .github/CODEOWNERS, scripts/check-ai-review-workflow.cjs, .claude/agents/ and .agents/roles/. That is the same seven paths, in the same set, in both CLAUDE.md and CONTRIBUTING.md. None of the three files this PR touches is on the list, so this run is the end-to-end proof the PR description promises.
  • Skip conditions. ai-review.yml:44 gates on github.actor != 'dependabot[bot]' && draft == false. That matches "every non-draft, non-dependabot PR."
  • Same App identity. Both the paused-notice token (L110–L111) and the review-post token (L327–L328) are minted from IPR_APP_ID / IPR_APP_PRIVATE_KEY. The "IPR bot reviews your Java" explanation is correct.
  • Read-only claim. --disallowedTools "Bash,Edit,Write,NotebookEdit,MultiEdit,WebFetch,WebSearch" (L314) is backed by a permissions.deny mirror (L264–L282). "No shell, no file writes, no network fetches" is accurate.
  • Fail-closed claim. The prompt preflight exit 1s (L173, L244, L507) and the "Comment on PR if Argus review failed" step (L420) back the "red build plus a comment, never a silent pass" wording. The cancelled-run carve-out at L423 is consistent with "a crash, a timeout, output that fails validation."
  • Base-branch prompt read. The prompt is loaded via git show "${BASE_SHA}:.github/ai-review/expert-adcp-reviewer.md" (L175), which supports the self-modification rationale.
  • D21-aligned. The statement that a bot approval doesn't satisfy branch protection matches D21's "1 code-owner approving review" requirement. No required-check or branch-protection change, so there is no D21 drift.
  • D18 / changeset. An empty-frontmatter changeset is the right shape for a change that affects no published artifact. The title is a Conventional Commit with no agent prefix.
  • Workspace artifacts. None in the changed-file list.

No source, build or workflow files are touched, so no expert delegation was required.

Follow-ups (non-blocking — file as issues)

  • .github/ai-review/expert-adcp-reviewer.md still says D1–D21. The PR body calls this out as a guarded-path fix that needs its own human-reviewed PR. That is the right call. File it so it doesn't drift further.
  • ROADMAP.md:591 still reads "D1–D21 locked; D22 + D23 framing locked, D24 shipped." That is more nuanced than "24 confirmed decisions." It's worth aligning the two, either by tightening the status line or by saying in CONTRIBUTING.md that D22/D23 values are still being pinned in the roadmap body.

Minor nits (non-blocking)

  1. "ask a maintainer rather than re-running." CONTRIBUTING.md:70 advises against re-running a failed Argus job. That's sound for persistent failures like model access or schema validation. A transient API timeout is the case where a re-run is the cheapest fix. A half-sentence distinguishing the two would save a maintainer ping.

The one notable thing here: the PR's acceptance test is this review.

LGTM. Follow-ups noted below.

Two non-blocking points from Argus's review of 1b19434:

- "ask a maintainer rather than re-running" was too blunt. A transient API
  error is exactly the case where one re-run is the cheapest fix; the advice
  now distinguishes that from a failure that repeats.
- "24 confirmed decisions" is accurate for the table but flatter than
  `ROADMAP.md:591`, which has D1–D21 fully locked, D22/D23 framing locked
  with shape details in linked specs, and D24 shipped. Says so now.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
aao-ipr-bot[bot]
aao-ipr-bot Bot previously approved these changes Oct 16, 2026

@aao-ipr-bot aao-ipr-bot Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving. Two docs gaps closed: the decision count was wrong in the two files that tell a new contributor or coding agent what to read first, and the review bot had no documentation at all. The new text matches the workflow it describes.

Things I checked

  • Decision count. ROADMAP.md has rows D22, D23 and D24 (L34–L36). The added CONTRIBUTING sentence "D1–D21 are fully locked; D22 and D23 have their framing locked … and D24 has shipped" matches the status row at ROADMAP.md:591 almost word for word. The CLAUDE.md L15 change (21 → 24) is correct.
  • Guarded paths. The seven paths listed in both CLAUDE.md and CONTRIBUTING.md match the self-modification regex at .github/workflows/ai-review.yml:96 exactly. This PR touches none of them, so Argus runs here as the PR claims. This review is the end-to-end proof the PR asks for.
  • Skip conditions. "Non-draft, non-dependabot" matches the job guard at ai-review.yml:44.
  • Read-only claim. Bash, Edit, Write, WebFetch and WebSearch are denied both in permissions.deny (ai-review.yml:276-291) and in --disallowedTools (L314). The App token with write access is minted only after the model finishes (L322). "No shell, no file writes, no network fetches" is accurate for the reviewer.
  • D21 claim. .github/CODEOWNERS is * @bokelley @MichielDean. The App is not a code owner, so an Argus approval cannot satisfy the required code-owner review. Correctly documented.
  • Fail-closed claim. "Require that Argus posted a review" (ai-review.yml:491-507) exits 1 when no review was posted. This is the inverse of the #41 behaviour, as the PR says.
  • Validator workflow name. Validate Argus review config runs on pull_request with no paths filter (validate-ai-review-config.yml:1,17-18), so "on every PR" holds.
  • Changeset. It is empty (---\n---), which is correct for a docs-only change that ships no artifact. The title is Conventional (docs:). No workspace artifacts in the file list.
  • Experts. None consulted. No source, build or workflow files changed, and no ROADMAP decision row was edited.

Follow-ups (non-blocking — file as issues)

  • .github/ai-review/expert-adcp-reviewer.md still says D1–D21. The PR body flags this. It needs a separate PR with a human reviewer because that file is guarded. Until then, the reviewer prompt understates the decision set by three rows, and D22 (tenant-aware signing) and D24 (the 4 MiB response cap) are both load-bearing for MUST FIX calls.

Minor nits (non-blocking)

  1. "Posts a comment saying so" overclaims slightly. The failure comment only fires when steps.review-token.outcome == 'success' (ai-review.yml:425). If minting the App token fails, the build is still red but no comment is posted. Suggested wording: "the check fails, and in most cases it posts a comment saying so."
  2. Paused notices on unguarded PRs. The self-modification gate also fails closed when gh pr diff --name-only errors, for example on very large diffs (ai-review.yml:83-86). A contributor with a large PR that touches no guarded path can still get the "paused" notice. One sentence in CONTRIBUTING would head off that confusion.
  3. "It reads all of those from the base branch" is imprecise for the validator. Argus never reads scripts/check-ai-review-workflow.cjs. The validator runs from the PR's own copy on pull_request (validate-ai-review-config.yml:9-11). The reason for the gate still holds, because a PR that weakens the validator would otherwise go unreviewed by a human. The sentence should say "gates" rather than claim everything is read from base.

Notable: the only way to prove the review workflow works is a PR whose only job is to be reviewed by it. It works.

LGTM. Follow-ups noted below.

Final round of non-blocking precision fixes from Argus's review of e367806:

- "posts a comment saying so" overclaimed. The notice step requires
  `steps.review-token.outcome == 'success'`, so a token-minting failure
  leaves a red build with no comment. Says "in most cases" and names why.
- the paused notice can also appear on a PR touching no guarded path,
  because the gate fails closed when `gh pr diff` cannot list the changed
  files. Documented, so a large-PR author is not left guessing.
- "it reads all of those from the base branch" was wrong for the validator:
  Argus never reads `scripts/check-ai-review-workflow.cjs`, which runs from
  the PR's own copy on `pull_request`. The guard still holds — a PR that
  weakens the validator would otherwise go unreviewed — but the reason is
  "each of these gates the review", not "all are read from base".

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
aao-ipr-bot[bot]
aao-ipr-bot Bot previously approved these changes Oct 16, 2026

@aao-ipr-bot aao-ipr-bot Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Docs-only fix: it documents Argus for contributors and agents, and corrects the confirmed-decision count from 21 to 24. Every factual claim in the new prose matches ai-review.yml and ROADMAP.md at the base SHA. That accuracy is what matters, because these files tell new contributors what to read first.

Things I checked

  • The decision count is right. ROADMAP.md:34-36 contains D22 (multi-tenant signing keys), D23 (AdCP 3.1 beta parity target) and D24 (4 MiB response cap). "D1–D24" in CLAUDE.md and "24 confirmed post-RFC decisions" in CONTRIBUTING.md are both correct.
  • The guarded-path list matches the gate exactly. The regex at .github/workflows/ai-review.yml:96 covers ai-review.yml, validate-ai-review-config.yml, .github/ai-review/, CODEOWNERS, scripts/check-ai-review-workflow.cjs, .claude/agents/ and .agents/roles/. Both the CLAUDE.md and CONTRIBUTING.md lists name the same seven paths. None of the three files in this PR matches, so it is correct that Argus is running here.
  • The fail-closed claim is accurate. If gh pr diff --name-only fails, ai-review.yml:83-87 sets touches=true, so the "paused on a PR touching none of those paths" paragraph describes real behavior and is not speculation.
  • The "red build plus a comment" claim is accurate, including the caveat. "Require that Argus posted a review" at ai-review.yml:491-507 sets the job's conclusion. The failure notice at L420-461 depends on steps.review-token.outcome == 'success', so the parenthetical "a token-minting failure leaves a red build with no comment" is correct. Good to state that edge case instead of overclaiming.
  • The read-only claim is accurate. Bash, Edit, Write, WebFetch and WebSearch are denied in both permissions.deny (L276-290) and --disallowedTools (L314). The note on sandbox at L268-272 confirms the deny lists are the actual control, which fits the doc's wording.
  • The D21 claim is accurate. The branch-protection row at ROADMAP.md:33 requires a code-owner approval. .github/CODEOWNERS lists only @bokelley @MichielDean, so the App's approval cannot satisfy it.
  • The changeset is valid. An empty-frontmatter changeset is the right category for contributor docs and passes npx changeset status --since=origin/main in changeset-check.yml:39.
  • There is no D-decision drift. No source, build or workflow files are touched. D16 holds: no docs/adr/. D18 holds: the title is Conventional (docs:) with no agent prefix.
  • Expert delegation was skipped deliberately. The diff has no Java, build or workflow changes, only governance prose that I checked line by line against the files it describes. There was nothing for a code reviewer to run.
  • No stray workspace artifacts. The changed-file list is three files, none of them local or agent scratch files.

Follow-ups (non-blocking — file as issues)

  • .github/ai-review/expert-adcp-reviewer.md:9 and :138 still say "D1–D21". The PR body correctly says the file is guarded and needs its own human-reviewed PR. File the issue now so it doesn't drift further. Until then, this prompt tells the reviewer to audit 21 decisions when 24 exist.

Minor nits (non-blocking)

  1. D23 has no linked specs/ doc. CONTRIBUTING.md:25 says "D22 and D23 have their framing locked with shape details in linked specs/ docs." D22 links specs/signing-context.md, but the D23 row at ROADMAP.md:35 links no spec. Either drop D23 from that clause or describe it as "pinned to 3.1.0-beta.5, GA bump deferred to the codegen PR."
  2. Possible future drift in the guarded-path list. CLAUDE.md and CONTRIBUTING.md now each copy the list from ai-review.yml:96. Extending the regex will mean updating three places. A one-line pointer from the docs to the workflow line, or a check in check-ai-review-workflow.cjs, would prevent that drift.

The PR's own code_review check is its proof of concept. As validation strategies go, this one is hard to argue with.

LGTM. Follow-ups noted below.

I introduced this error last round: "D22 and D23 have their framing locked
with shape details in linked `specs/` docs" is wrong for D23, which links no
spec. D22 links `specs/signing-context.md`; D23 pins the parity target at
`3.1.0-beta.5` with the GA bump deferred to the codegen PR. Caught by Argus
on 501404b.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

@aao-ipr-bot aao-ipr-bot Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Documentation-only follow-up to #42. It documents the Argus review and fixes a decision count that had been wrong since D22–D24 landed. The prose matches the workflow line for line, and that matters most, because a contributor guide that misstates the review gates is worse than having no guide.

No source, build or workflow files change, so no experts were dispatched. It is governance prose (CLAUDE.md), so I checked every factual claim against the files it describes.

Things I checked

  • The decision count is correct. ROADMAP.md:34-36 has rows D22, D23 and D24. The new CONTRIBUTING.md:25 framing ("D1–D21 are fully locked … D23 pins … 3.1.0-beta.5 with the GA bump deferred to the codegen PR, and D24 has shipped") matches the status row at ROADMAP.md:591 and D23's own text. specs/signing-context.md exists, so the D22 link resolves.
  • The guarded-path list matches the gate exactly. All seven paths listed in CONTRIBUTING.md and CLAUDE.md match the regex at ai-review.yml:96: ai-review.yml, validate-ai-review-config.yml, .github/ai-review/, CODEOWNERS, check-ai-review-workflow.cjs, .claude/agents/, .agents/roles/. Nothing is missing and nothing is invented.
  • The trigger scope is correct. "Non-draft, dependabot skipped" matches the job if: at ai-review.yml:44.
  • The fail-closed claim is accurate, including its caveat. The "Require that Argus posted a review" step (ai-review.yml:491-507) fails the job whenever no review was verified. The failure comment (ai-review.yml:420-425) is gated on steps.review-token.outcome == 'success'. So the parenthetical "a token-minting failure leaves a red build with no comment" is exactly right. Good catch on documenting that edge instead of overclaiming.
  • The fail-closed pause for unreadable diffs is accurate. CONTRIBUTING.md:73 describes the gh pr diff failure branch at ai-review.yml:83-87, which sets touches=true.
  • The read-only claim is accurate. The deny list blocks Bash, Edit, Write, WebFetch and WebSearch in both permissions.deny and --disallowedTools (ai-review.yml:276-291, :314).
  • D21 is cited correctly. CODEOWNERS is * @bokelley @MichielDean, so the App is not a code owner and its approval cannot satisfy the code-owner requirement.
  • The changeset is fine. The empty changeset (---\n---) is the right form for a contributor-docs change. Nothing published changes.
  • No workspace artifacts. All three changed files are intended source.
  • The test plan is honest. It names the local commands. The one claim that can't be checked locally, that this PR proves #41 end to end, is shown by this review existing.

Follow-ups (non-blocking — file as issues)

  • A third stale count is still in the repo. .github/ISSUE_TEMPLATE/track-claim.yml:9 still says "Skim the [21 confirmed decisions]". It is not a guarded path, so it could have gone in this PR. It is the first text a track claimant reads.
  • The stale D1–D21 in .github/ai-review/expert-adcp-reviewer.md (L9, L138). Already noted in the PR body as needing its own human-reviewed PR. Agreed. Until it lands, the reviewer prompt tells Argus to walk D1–D21 only, which leaves D22 (tenant-aware signing) and D24 (response cap) out of the mandatory coherence audit. That gap is load-bearing for signing and transport PRs, so it should be next.

Minor nits (non-blocking)

  1. The wording for D1–D21 and D22 can be read two ways. CONTRIBUTING.md:25 says "D1–D21 are fully locked; D22's shape is detailed in …". It is accurate, but it can be read as saying D22 is not locked. ROADMAP.md:591 says "D22 + D23 framing locked". Borrowing that phrase would remove the ambiguity.
  2. "No network fetches" is slightly broad. CONTRIBUTING.md:64: the model still talks to the Anthropic API and holds a read-scoped GITHUB_TOKEN. "No web fetches" is the precise claim. It is a small point, but this is a security-posture sentence.

A PR whose acceptance test is the reviewer reviewing it is an interesting choice. It works.

LGTM. Follow-ups noted below.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant