Repository navigation
Conversation
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>
There was a problem hiding this comment.
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.mdL33–L36 carry D21–D24, so "24 confirmed decisions (D1–D24)" is correct in bothCLAUDE.md:15andCONTRIBUTING.md:25. - Guarded paths. The self-modification list in the docs matches the
grep -Egate atai-review.yml:96forai-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-314allows onlyRead,Glob,Grep,Taskand disallowsBash,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-461posts or updates a single failure notice.:491-507exits 1 whenreview_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.ymlis namedValidate Argus review configand runsnode scripts/check-ai-review-workflow.cjsat L33. That matches theCLAUDE.mdtext. - 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.mdstill 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)
- Guarded list is missing one path. The gate at
ai-review.yml:96also covers.github/workflows/validate-ai-review-config.yml. NeitherCLAUDE.md:37norCONTRIBUTING.md:71lists it. Add it, or the next agent editing the validator workflow will be surprised. - "Every non-draft PR" is slightly too broad.
ai-review.yml:44also skips PRs opened bydependabot[bot]. One clause inCONTRIBUTING.md:61andCLAUDE.md:34would fix it. CODEOWNERSpath. 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>
There was a problem hiding this comment.
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.mdhas D22, D23 and D24 rows at L34–L36 directly after D21 (L33). "24 numbered decisions (D1–D24)" is accurate for the table. BothCLAUDE.md:15andCONTRIBUTING.md:25are updated. - Guarded-path list matches the gate exactly. The grep at
.github/workflows/ai-review.yml:96coversai-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 bothCLAUDE.mdandCONTRIBUTING.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:44gates ongithub.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 apermissions.denymirror (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.mdstill saysD1–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:591still 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 inCONTRIBUTING.mdthat D22/D23 values are still being pinned in the roadmap body.
Minor nits (non-blocking)
- "ask a maintainer rather than re-running."
CONTRIBUTING.md:70advises 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>
There was a problem hiding this comment.
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.mdhas 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 atROADMAP.md:591almost word for word. The CLAUDE.md L15 change (21 → 24) is correct. - Guarded paths. The seven paths listed in both
CLAUDE.mdandCONTRIBUTING.mdmatch the self-modification regex at.github/workflows/ai-review.yml:96exactly. 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,WebFetchandWebSearchare denied both inpermissions.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/CODEOWNERSis* @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 configruns onpull_requestwith nopathsfilter (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.mdstill 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)
- "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." - Paused notices on unguarded PRs. The self-modification gate also fails closed when
gh pr diff --name-onlyerrors, 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. - "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 onpull_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>
There was a problem hiding this comment.
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-36contains D22 (multi-tenant signing keys), D23 (AdCP 3.1 beta parity target) and D24 (4 MiB response cap). "D1–D24" inCLAUDE.mdand "24 confirmed post-RFC decisions" inCONTRIBUTING.mdare both correct. - The guarded-path list matches the gate exactly. The regex at
.github/workflows/ai-review.yml:96coversai-review.yml,validate-ai-review-config.yml,.github/ai-review/,CODEOWNERS,scripts/check-ai-review-workflow.cjs,.claude/agents/and.agents/roles/. Both theCLAUDE.mdandCONTRIBUTING.mdlists 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-onlyfails,ai-review.yml:83-87setstouches=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-507sets the job's conclusion. The failure notice at L420-461 depends onsteps.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,WebFetchandWebSearchare denied in bothpermissions.deny(L276-290) and--disallowedTools(L314). The note onsandboxat 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:33requires a code-owner approval..github/CODEOWNERSlists 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/maininchangeset-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:9and:138still 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)
- D23 has no linked
specs/doc.CONTRIBUTING.md:25says "D22 and D23 have their framing locked with shape details in linkedspecs/docs." D22 linksspecs/signing-context.md, but the D23 row atROADMAP.md:35links no spec. Either drop D23 from that clause or describe it as "pinned to3.1.0-beta.5, GA bump deferred to the codegen PR." - Possible future drift in the guarded-path list.
CLAUDE.mdandCONTRIBUTING.mdnow each copy the list fromai-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 incheck-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>
There was a problem hiding this comment.
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-36has rows D22, D23 and D24. The newCONTRIBUTING.md:25framing ("D1–D21 are fully locked … D23 pins …3.1.0-beta.5with the GA bump deferred to the codegen PR, and D24 has shipped") matches the status row atROADMAP.md:591and D23's own text.specs/signing-context.mdexists, so the D22 link resolves. - The guarded-path list matches the gate exactly. All seven paths listed in
CONTRIBUTING.mdandCLAUDE.mdmatch the regex atai-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:atai-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 onsteps.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:73describes thegh pr difffailure branch atai-review.yml:83-87, which setstouches=true. - The read-only claim is accurate. The deny list blocks
Bash,Edit,Write,WebFetchandWebSearchin bothpermissions.denyand--disallowedTools(ai-review.yml:276-291,:314). - D21 is cited correctly.
CODEOWNERSis* @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:9still 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–D21in.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)
- The wording for D1–D21 and D22 can be read two ways.
CONTRIBUTING.md:25says "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:591says "D22 + D23 framing locked". Borrowing that phrase would remove the ambiguity. - "No network fetches" is slightly broad.
CONTRIBUTING.md:64: the model still talks to the Anthropic API and holds a read-scopedGITHUB_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.
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.mdgains a short section covering the parts that actually surprise people:The confirmed-decision count is stale.
ROADMAP.mdhas carried D1–D24 since D22–D24 landed, butCONTRIBUTING.mdandCLAUDE.mdboth still said 21 — in the exact lines that tell a new contributor or coding agent what to read first.CLAUDE.mdalso gains a short Argus section pointing atscripts/check-ai-review-workflow.cjsand 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.mdcarries the same staleD1–D21reference 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 onlyCONTRIBUTING.md,CLAUDE.mdand.changeset/, none of which are guarded, so Argus runs normally on it. Thecode_reviewcheck 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