From 99d2aedc9dc45edb323807fad7f0a6173c05e7f7 Mon Sep 17 00:00:00 2001 From: Brian O'Kelley Date: Fri, 16 Oct 2026 16:24:29 +0000 Subject: [PATCH 1/5] docs: document the automated PR review and fix the decision count MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- .changeset/document-argus-review.md | 8 ++++++++ CLAUDE.md | 9 ++++++++- CONTRIBUTING.md | 16 +++++++++++++++- 3 files changed, 31 insertions(+), 2 deletions(-) create mode 100644 .changeset/document-argus-review.md diff --git a/.changeset/document-argus-review.md b/.changeset/document-argus-review.md new file mode 100644 index 0000000..28304dd --- /dev/null +++ b/.changeset/document-argus-review.md @@ -0,0 +1,8 @@ +--- +--- + +docs: document the automated PR review in `CONTRIBUTING.md` and correct the +confirmed-decision count (21 → 24) in `CONTRIBUTING.md` and `CLAUDE.md`. + +Empty changeset because this only affects contributor documentation, not the +published Java SDK artifacts. diff --git a/CLAUDE.md b/CLAUDE.md index 12494c4..0ec6616 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -12,7 +12,7 @@ Status: harness phase. Eight Gradle modules wired and green; most surfaces are p In order of precedence: -1. **[`ROADMAP.md`](ROADMAP.md) §Confirmed decisions** — 21 numbered decisions (D1–D21) that lock in architecture, build, conventions. **These override the RFC where they differ.** +1. **[`ROADMAP.md`](ROADMAP.md) §Confirmed decisions** — 24 numbered decisions (D1–D24) that lock in architecture, build, conventions. **These override the RFC where they differ.** 2. **[`docs/rfc/java-sdk-rfc.md`](docs/rfc/java-sdk-rfc.md)** — the merged RFC. 3. **[`ROADMAP.md`](ROADMAP.md) §Tracks** — what each module is supposed to contain, in track-claim form. 4. **[`ROADMAP.md`](ROADMAP.md) §7.x deltas** — what changed in the TS SDK between the RFC's 6.x baseline and the current 7.2.0. @@ -75,6 +75,13 @@ The SDK generates Java types from JSON Schemas at `https://adcontextprotocol.org JUnit 5. Conventional test layout (`src/test/java/...`). Tests run with `./gradlew test`. Storyboard CI lives in `adcp-testing` and hits the `@adcp/sdk/mock-server` sidecar in CI (D8). +## AI PR review (Argus) + +`.github/workflows/ai-review.yml` runs an LLM reviewer on every non-draft PR. Two things to know before you touch it: + +- Its invariants are enforced by `scripts/check-ai-review-workflow.cjs`, run by the `Validate Argus review config` workflow on every PR. Run it locally with `node scripts/check-ai-review-workflow.cjs`. If it fails, read the message — each check exists because that failure mode already shipped once. +- The workflow, its prompt under `.github/ai-review/`, the validator, `.claude/agents/`, `.agents/roles/` and `CODEOWNERS` are **self-modification guarded**: a PR touching any of them pauses Argus and requires a human reviewer. Expect no bot review on those PRs. + ## Picking up work The contributor onboarding path is in [`CONTRIBUTING.md`](CONTRIBUTING.md): claim a track via issue, work on a feature branch, PR to `main`. The 14 tracks live in [`ROADMAP.md`](ROADMAP.md#tracks). diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 9821dae..73719a1 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -22,7 +22,7 @@ Read [`ROADMAP.md`](ROADMAP.md) first. It divides v0.1 → v1.0 into 14 claimabl A single contributor can claim multiple non-conflicting tracks. High-coupling tracks (`async-l3` + `lifecycle`) benefit from a single owner pair. -Before designing anything new, read the [21 confirmed post-RFC decisions](ROADMAP.md#confirmed-decisions). They override the RFC where they differ. +Before designing anything new, read the [24 confirmed post-RFC decisions](ROADMAP.md#confirmed-decisions). They override the RFC where they differ. ## Dev setup @@ -60,6 +60,20 @@ rm settings-gradle.lockfile && ./gradlew help --write-locks CI verifies lockfiles are up to date on every PR — the `Verify lockfiles are up to date` step will fail if you forget. Commit all updated lockfiles alongside your dependency change. +## Automated PR review + +Every non-draft pull request gets an automated review from **Argus**, an LLM reviewer that reads your diff and posts an approve / comment / request-changes review. Two things usually surprise first-time contributors: + +- **The review arrives from `aao-ipr-bot[bot]`** — the same GitHub App that handles the IPR signature check. One App covers both, which is why the "IPR bot" appears to be reviewing your Java. +- **Argus cannot change your branch.** It runs read-only: no shell, no file writes, no network fetches. It reads the diff and repository files and returns a review. + +What its verdict does and does not mean: + +- An Argus approval **does not** satisfy branch protection. `main` still requires a review from a human code owner (D21), and the bot is not one. +- If Argus cannot complete — a crash, a timeout, output that fails validation — the check **fails** and it posts a comment saying so. A missing review is a red build, never a silent pass. If you see that comment, ask a maintainer to review rather than re-running the job. + +Argus deliberately skips PRs that change the review system itself (`.github/workflows/ai-review.yml`, `.github/ai-review/`, `scripts/check-ai-review-workflow.cjs`, `.claude/agents/`, `.agents/roles/`, `CODEOWNERS`). It reads all of those from the base branch, so it would be judging a change to its own gates using the version that change replaces. Those PRs get a "paused" notice and need a human reviewer. + ## Code conventions - **Java 21 baseline** (D2). Use records, sealed types, pattern matching, virtual threads. No `Optional` returns — use `@Nullable T` (JSpecify) instead. From 1b194344fcd65091702b947dc2599c915076100a Mon Sep 17 00:00:00 2001 From: Brian O'Kelley Date: Fri, 16 Oct 2026 16:27:10 +0000 Subject: [PATCH 2/5] docs: correct the guarded-path list and the review trigger MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- CLAUDE.md | 4 ++-- CONTRIBUTING.md | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 0ec6616..d29446f 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -77,10 +77,10 @@ JUnit 5. Conventional test layout (`src/test/java/...`). Tests run with `./gradl ## AI PR review (Argus) -`.github/workflows/ai-review.yml` runs an LLM reviewer on every non-draft PR. Two things to know before you touch it: +`.github/workflows/ai-review.yml` runs an LLM reviewer on every non-draft, non-dependabot PR. Two things to know before you touch it: - Its invariants are enforced by `scripts/check-ai-review-workflow.cjs`, run by the `Validate Argus review config` workflow on every PR. Run it locally with `node scripts/check-ai-review-workflow.cjs`. If it fails, read the message — each check exists because that failure mode already shipped once. -- The workflow, its prompt under `.github/ai-review/`, the validator, `.claude/agents/`, `.agents/roles/` and `CODEOWNERS` are **self-modification guarded**: a PR touching any of them pauses Argus and requires a human reviewer. Expect no bot review on those PRs. +- The workflow, `.github/workflows/validate-ai-review-config.yml`, the prompt under `.github/ai-review/`, the validator, `.claude/agents/`, `.agents/roles/` and `.github/CODEOWNERS` are **self-modification guarded**: a PR touching any of them pauses Argus and requires a human reviewer. Expect no bot review on those PRs. ## Picking up work diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 73719a1..fbd5404 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -62,7 +62,7 @@ CI verifies lockfiles are up to date on every PR — the `Verify lockfiles are u ## Automated PR review -Every non-draft pull request gets an automated review from **Argus**, an LLM reviewer that reads your diff and posts an approve / comment / request-changes review. Two things usually surprise first-time contributors: +Every non-draft pull request gets an automated review from **Argus**, an LLM reviewer that reads your diff and posts an approve / comment / request-changes review. (Dependabot PRs are skipped.) Two things usually surprise first-time contributors: - **The review arrives from `aao-ipr-bot[bot]`** — the same GitHub App that handles the IPR signature check. One App covers both, which is why the "IPR bot" appears to be reviewing your Java. - **Argus cannot change your branch.** It runs read-only: no shell, no file writes, no network fetches. It reads the diff and repository files and returns a review. @@ -72,7 +72,7 @@ What its verdict does and does not mean: - An Argus approval **does not** satisfy branch protection. `main` still requires a review from a human code owner (D21), and the bot is not one. - If Argus cannot complete — a crash, a timeout, output that fails validation — the check **fails** and it posts a comment saying so. A missing review is a red build, never a silent pass. If you see that comment, ask a maintainer to review rather than re-running the job. -Argus deliberately skips PRs that change the review system itself (`.github/workflows/ai-review.yml`, `.github/ai-review/`, `scripts/check-ai-review-workflow.cjs`, `.claude/agents/`, `.agents/roles/`, `CODEOWNERS`). It reads all of those from the base branch, so it would be judging a change to its own gates using the version that change replaces. Those PRs get a "paused" notice and need a human reviewer. +Argus deliberately skips PRs that change the review system itself (`.github/workflows/ai-review.yml`, `.github/workflows/validate-ai-review-config.yml`, `.github/ai-review/`, `scripts/check-ai-review-workflow.cjs`, `.claude/agents/`, `.agents/roles/`, `.github/CODEOWNERS`). It reads all of those from the base branch, so it would be judging a change to its own gates using the version that change replaces. Those PRs get a "paused" notice and need a human reviewer. ## Code conventions From e3678068f329b38368fd4c536d5da0180ef34f98 Mon Sep 17 00:00:00 2001 From: Brian O'Kelley Date: Fri, 16 Oct 2026 16:29:16 +0000 Subject: [PATCH 3/5] docs: soften the re-run advice and note the D22/D23 nuance MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- CONTRIBUTING.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index fbd5404..c3cbe39 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -22,7 +22,7 @@ Read [`ROADMAP.md`](ROADMAP.md) first. It divides v0.1 → v1.0 into 14 claimabl A single contributor can claim multiple non-conflicting tracks. High-coupling tracks (`async-l3` + `lifecycle`) benefit from a single owner pair. -Before designing anything new, read the [24 confirmed post-RFC decisions](ROADMAP.md#confirmed-decisions). They override the RFC where they differ. +Before designing anything new, read the [24 confirmed post-RFC decisions](ROADMAP.md#confirmed-decisions). They override the RFC where they differ. D1–D21 are fully locked; D22 and D23 have their framing locked with shape details in linked `specs/` docs, and D24 has shipped. ## Dev setup @@ -70,7 +70,7 @@ Every non-draft pull request gets an automated review from **Argus**, an LLM rev What its verdict does and does not mean: - An Argus approval **does not** satisfy branch protection. `main` still requires a review from a human code owner (D21), and the bot is not one. -- If Argus cannot complete — a crash, a timeout, output that fails validation — the check **fails** and it posts a comment saying so. A missing review is a red build, never a silent pass. If you see that comment, ask a maintainer to review rather than re-running the job. +- If Argus cannot complete — a crash, a timeout, output that fails validation — the check **fails** and it posts a comment saying so. A missing review is a red build, never a silent pass. Re-running the job is worth one try if the log shows a transient API error; if it fails the same way twice, ask a maintainer to review instead. Argus deliberately skips PRs that change the review system itself (`.github/workflows/ai-review.yml`, `.github/workflows/validate-ai-review-config.yml`, `.github/ai-review/`, `scripts/check-ai-review-workflow.cjs`, `.claude/agents/`, `.agents/roles/`, `.github/CODEOWNERS`). It reads all of those from the base branch, so it would be judging a change to its own gates using the version that change replaces. Those PRs get a "paused" notice and need a human reviewer. From 501404bf6571edd01ad30addbfe65b3dbef7147c Mon Sep 17 00:00:00 2001 From: Brian O'Kelley Date: Fri, 16 Oct 2026 16:32:02 +0000 Subject: [PATCH 4/5] docs: tighten three overclaims in the review section MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- CONTRIBUTING.md | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index c3cbe39..1c17325 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -70,9 +70,11 @@ Every non-draft pull request gets an automated review from **Argus**, an LLM rev What its verdict does and does not mean: - An Argus approval **does not** satisfy branch protection. `main` still requires a review from a human code owner (D21), and the bot is not one. -- If Argus cannot complete — a crash, a timeout, output that fails validation — the check **fails** and it posts a comment saying so. A missing review is a red build, never a silent pass. Re-running the job is worth one try if the log shows a transient API error; if it fails the same way twice, ask a maintainer to review instead. +- If Argus cannot complete — a crash, a timeout, output that fails validation — the check **fails**, and in most cases it posts a comment saying so (the comment needs an App token, so a token-minting failure leaves a red build with no comment). A missing review is a red build, never a silent pass. Re-running the job is worth one try if the log shows a transient API error; if it fails the same way twice, ask a maintainer to review instead. -Argus deliberately skips PRs that change the review system itself (`.github/workflows/ai-review.yml`, `.github/workflows/validate-ai-review-config.yml`, `.github/ai-review/`, `scripts/check-ai-review-workflow.cjs`, `.claude/agents/`, `.agents/roles/`, `.github/CODEOWNERS`). It reads all of those from the base branch, so it would be judging a change to its own gates using the version that change replaces. Those PRs get a "paused" notice and need a human reviewer. +Argus deliberately skips PRs that change the review system itself (`.github/workflows/ai-review.yml`, `.github/workflows/validate-ai-review-config.yml`, `.github/ai-review/`, `scripts/check-ai-review-workflow.cjs`, `.claude/agents/`, `.agents/roles/`, `.github/CODEOWNERS`). Each of those gates the review in some way — the workflow, prompt and agent definitions are the ones Argus itself loads from the base branch, so it would be judging a change to its own gates using the version that change replaces. Those PRs get a "paused" notice and need a human reviewer. + +You can also get that notice on a PR touching none of those paths: the gate fails closed, so if the changed-file list can't be read at all — `gh pr diff` rejects very large diffs — it pauses rather than guess. ## Code conventions From 0acd676142eeabf956ce3a753e2650bd755aa5fc Mon Sep 17 00:00:00 2001 From: Brian O'Kelley Date: Fri, 16 Oct 2026 16:34:33 +0000 Subject: [PATCH 5/5] docs: correct the D23 description 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) --- CONTRIBUTING.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 1c17325..3378960 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -22,7 +22,7 @@ Read [`ROADMAP.md`](ROADMAP.md) first. It divides v0.1 → v1.0 into 14 claimabl A single contributor can claim multiple non-conflicting tracks. High-coupling tracks (`async-l3` + `lifecycle`) benefit from a single owner pair. -Before designing anything new, read the [24 confirmed post-RFC decisions](ROADMAP.md#confirmed-decisions). They override the RFC where they differ. D1–D21 are fully locked; D22 and D23 have their framing locked with shape details in linked `specs/` docs, and D24 has shipped. +Before designing anything new, read the [24 confirmed post-RFC decisions](ROADMAP.md#confirmed-decisions). They override the RFC where they differ. D1–D21 are fully locked; D22's shape is detailed in [`specs/signing-context.md`](specs/signing-context.md), D23 pins the parity target at `3.1.0-beta.5` with the GA bump deferred to the codegen PR, and D24 has shipped. ## Dev setup