From edfdf10f5bd2cef1311961609ac7aa0e6bb52802 Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Sun, 26 Jul 2026 18:34:16 -0400 Subject: [PATCH 1/3] feat(review): scope REVIEW.md per lane and re-derive the review credential Restructure REVIEW.md into mutually exclusive code-review-lane and security-lane scope sections, dropping the report-stage filtering sections ("Do not report", "Cap the nits") per Anthropic's Sonnet-5 harness-effect guidance. Re-derive REVIEW-CREDENTIAL.md per #264: with standards public no consumer needs the credential; the --add-dir subagent-visibility open question is resolved empirically; provisioning stays deferred behind github-iac governance. Closes #264 Co-Authored-By: Claude Fable 5 --- REVIEW.md | 64 ++++++++++++++++++------------- distribution/REVIEW-CREDENTIAL.md | 45 +++++++++++++++++----- 2 files changed, 73 insertions(+), 36 deletions(-) diff --git a/REVIEW.md b/REVIEW.md index 2a820cf5..c5fe9114 100644 --- a/REVIEW.md +++ b/REVIEW.md @@ -7,6 +7,12 @@ below (see "Depth"). A self-hosted or local review running the `review` plugin reads this file too, and additionally follows its citations into the `standards` repository for the full reasoned criterion behind each line. +Two review lanes consume this file: the **code-review lane** +(`claude-review`) and the **security lane** (`claude-security-review`). +Each lane applies only its own scope section below; the two scopes are +mutually exclusive, so a finding belongs to exactly one lane and is never +reported twice. + ## Severity This organization's criteria (`conventions/review/` in `standards`) resolve @@ -48,7 +54,37 @@ rather than left solely behind the cite. Every line still cites its SSOT criterion; a citation here never substitutes prose that isn't needed, per `conventions/engineering/reference-dont-duplicate.md`. -## Always check +## Code-review lane scope + +This lane owns every review dimension except security: correctness, +design, conventions, error handling, observability, tests, and +documentation. It does **not** report security findings — vulnerabilities, +authorization or tenancy gaps, credential exposure, injection — those +belong exclusively to the security lane and are omitted here even when a +hunk plainly contains one. + +Always check: + +- A high-risk security action — authentication, an authorization failure, a + privilege change, sensitive-data access — has a corresponding audit-log + entry (`conventions/review/observability.md#logging`). This is an + observability completeness check on the logging seam; whether the action + itself is safe is the security lane's question. +- A change that writes two or more related records, files, or state + locations carries an atomicity mechanism — a transaction, an atomic + rename, a constraint, or a compensation step — spanning them; an + interruption between steps must not leave state no code path expects + (`conventions/review/error-handling.md#atomicity`). + +## Security lane scope + +This lane reports **only** security findings: vulnerabilities, missing +authorization or tenant scoping, exposed secrets or credentials, injection, +and their direct enablers. Everything else — style, design, correctness +with no security impact, test coverage — is out of scope here and is owned +by the code-review lane; omit it. + +Always check: - A handler or endpoint that receives an object id (a path segment, body field, or query parameter) checks the caller is authorized for that @@ -62,29 +98,3 @@ criterion; a citation here never substitutes prose that isn't needed, per - A query or process invocation built from external input is parameterized, never string-concatenated — `blocking` (`conventions/review/security.md#trust-boundaries-and-injection`). -- A high-risk security action — authentication, an authorization failure, a - privilege change, sensitive-data access — has a corresponding audit-log - entry (`conventions/review/observability.md#logging`). -- A change that writes two or more related records, files, or state - locations carries an atomicity mechanism — a transaction, an atomic - rename, a constraint, or a compensation step — spanning them; an - interruption between steps must not leave state no code path expects - (`conventions/review/error-handling.md#atomicity`). - -## Do not report - -- Anything CI already enforces and that is not also tagged `blocking` - above: lint, formatting, type errors, and whatever a stack overlay under - `conventions/review/overlays/` names as its component's mechanical - backstop. A CI backstop (for example a `gitleaks` lane) catches an - instance mechanically; it does not make the `blocking` check itself - skippable here. -- Generated files and lockfiles, except a `blocking` finding — a leaked - secret in a lockfile or a supply-chain regression visible only there is - still reportable; this rule only waives low-signal mechanical noise - (formatting, regenerated diffs) in those files. - -## Cap the nits - -Report at most five Nits per review; note any beyond that as a count in the -summary instead of posting them inline. diff --git a/distribution/REVIEW-CREDENTIAL.md b/distribution/REVIEW-CREDENTIAL.md index 19bcc7b7..f9eaf897 100644 --- a/distribution/REVIEW-CREDENTIAL.md +++ b/distribution/REVIEW-CREDENTIAL.md @@ -12,10 +12,12 @@ authentication and belongs instead to the reusable workflow in engine: it authenticates a *review* job's checkout, never a materialization apply. -**Status: classified, not provisioned.** Nothing below has been created. -Provisioning — registering the GitHub App, generating its key, and wiring the -org-secret visibility list — is `github-iac` governance work and is recorded -as an open item, not attempted here. +**Status: classified, not provisioned — re-derived 2026-07-26 as not +currently needed (see "Re-derivation" below).** Nothing below has been +created. Provisioning — registering the GitHub App, generating its key, and +wiring the org-secret visibility list — is `github-iac` governance work and +stays deferred behind it; under the current visibility state there is +nothing to provision. ## Why a dedicated credential @@ -96,6 +98,29 @@ That reasoning stands on its own terms; what changed is that `standards` is no longer a private source, so it no longer has a boundary of this kind to protect. +## Re-derivation (2026-07-26) + +The re-derivation [standards#264](https://github.com/melodic-software/standards/issues/264) +asked for, performed against the corrected visibility table above: + +- **No consumer needs this credential today.** With `standards` public, a + native-reference mount is an anonymous checkout — every calling repo, + public or private, can reach the same tree without authenticating. The + eligible-consumer question under the public-caller prohibition is moot: + that prohibition exists to keep *private* mounted content out of + published review output, and there is currently no private content to + protect. `claude-code-plugins` being public is therefore not a per-repo + ineligibility; the credential is simply unnecessary for the mount. +- **Provisioning stays deferred behind `github-iac` governance**, and under + the current state there is nothing to provision. The classification below + is retained as the record of what would be provisioned if the need + returns. +- **The need returns in exactly two cases**, both already covered by the + review triggers: `standards` (or another mounted source) becoming + private again, or private-marketplace authentication becoming a + documented, credential-dependent path (the one genuinely + credential-dependent open question below). + ## Classification - **Type:** a dedicated **GitHub App installation token**, not a personal @@ -188,11 +213,13 @@ credential is needed at all rather than assuming one is. `claude-code-action` setup guide do not currently document private- marketplace authentication. Treat as unsupported until confirmed empirically. -- **`--add-dir` visibility to a plugin subagent's `Read` tool.** Whether a - mounted path is visible to `code-reviewer`/`security-reviewer`/ - `architecture-guardian` the same way it is to the main session (the B4 - plugin change assumes so; see `claude-code-plugins`' review-plugin PR's - own "Explicitly unverified" note). +- **`--add-dir` visibility to a plugin subagent's `Read` tool.** + **Resolved 2026-07-26** — empirically verified + ([standards#264](https://github.com/melodic-software/standards/issues/264)): + in a live session, a subagent performed Read, Glob, and Grep against an + additional working directory outside the primary working directory — the + same mechanism `--add-dir` uses. Mounted paths are visible to plugin + subagents the same way they are to the main session. - **Self-hosted Agent-SDK `.claude/rules/` auto-load** in a non-interactive CI invocation — not confirmed to behave like an interactive session's auto-load. From 50b49697d4a797dd23f321dc86a3efafe12d4da0 Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Sun, 26 Jul 2026 19:15:42 -0400 Subject: [PATCH 2/3] fix(review): state the lane split as intent, gate exclusion on lane presence MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Independent verification caught two defects in the restructure: the intro asserted the security lane consumes this file (it runs from its own security-only prompt and never reads it), and the unconditional security-findings exclusion would suppress the four blocking checks on repositories whose only reviewer is the code-review lane until the fleet security-lane rollout lands. The intro now records scope ownership instead of wiring, and the exclusion applies only where a claude-security-review workflow exists — self-resolving as the rollout proceeds, with no coverage window. Co-Authored-By: Claude Fable 5 --- REVIEW.md | 27 ++++++++++++++++++--------- 1 file changed, 18 insertions(+), 9 deletions(-) diff --git a/REVIEW.md b/REVIEW.md index c5fe9114..e8977388 100644 --- a/REVIEW.md +++ b/REVIEW.md @@ -7,11 +7,16 @@ below (see "Depth"). A self-hosted or local review running the `review` plugin reads this file too, and additionally follows its citations into the `standards` repository for the full reasoned criterion behind each line. -Two review lanes consume this file: the **code-review lane** -(`claude-review`) and the **security lane** (`claude-security-review`). -Each lane applies only its own scope section below; the two scopes are -mutually exclusive, so a finding belongs to exactly one lane and is never -reported twice. +Review in this organization is split into two mutually exclusive scopes, +so a finding belongs to exactly one scope and is never reported twice. +The **code-review lane** (`claude-review`, and Managed Code Review) +reads this file and applies the code-review scope below. The **security +lane** (`claude-security-review`) runs from its own security-only prompt +rather than reading this file; its scope section below records the same +split for every surface that does read it — so the code-review lane +knows what to leave to the security lane, and so a self-hosted or local +review (for example a `review`-plugin security agent) applies the right +section. ## Severity @@ -58,10 +63,14 @@ criterion; a citation here never substitutes prose that isn't needed, per This lane owns every review dimension except security: correctness, design, conventions, error handling, observability, tests, and -documentation. It does **not** report security findings — vulnerabilities, -authorization or tenancy gaps, credential exposure, injection — those -belong exclusively to the security lane and are omitted here even when a -hunk plainly contains one. +documentation. On a repository whose CI runs the security lane (a +`.github/workflows/claude-security-review.yml` workflow exists), it does +**not** report security findings — vulnerabilities, authorization or +tenancy gaps, credential exposure, injection — those belong exclusively +to that lane and are omitted here even when a hunk plainly contains one. +On a repository without that workflow no security lane exists yet, and +suppressed findings would have no other reader: report security findings +under this lane too, applying the security-scope checks below. Always check: From 18bda17a57ee75f658e85f628ed84b9619ae506d Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Sun, 26 Jul 2026 20:00:28 -0400 Subject: [PATCH 3/3] docs(review): reconcile public-standards state across credential and composition docs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses the three Codex review findings on PR #278: - REVIEW-CREDENTIAL.md: the native-reference prohibition now conditions on mounting private content instead of unconditionally forbidding public callers, matching the re-derivation's conclusion that a public `standards` mount is an anonymous checkout open to any caller; the retained classification is now explicitly historical — its scope and storage lists must not be provisioned as written, and a returning need re-derives from the then-current visibility table. - conventions/README.md: the native-reference bullet no longer scopes the mount to private consumers; the boundary rule (never mount private content into a more widely readable review output) is stated and linked to REVIEW-CREDENTIAL.md. - ai-review-bot-composition.md: the governed class baseline is now the two scope-split lanes (code review + security), with the split recorded in the root REVIEW.md; the high-traffic exception is clarified as governing a second same-scope reviewer, which the security lane is not. Co-Authored-By: Claude Fable 5 --- conventions/README.md | 2 +- .../review/ai-review-bot-composition.md | 4 ++-- distribution/REVIEW-CREDENTIAL.md | 23 +++++++++++-------- 3 files changed, 17 insertions(+), 12 deletions(-) diff --git a/conventions/README.md b/conventions/README.md index ece061c6..82f6f759 100644 --- a/conventions/README.md +++ b/conventions/README.md @@ -13,7 +13,7 @@ Three concerns live here: Prose is read, not executed. A reviewer or an authoring agent reads these files; no tool walks the directory tree to find them. That makes the distribution model deliberately simple and decoupled: - **Adopt by copy or by pointer.** `engineering/` and `process/` are copy-or-pointer only: a consuming repo either copies the files it adopts into its own tree, or points its contributor and review guides at this directory, and either way there is **no runtime coupling** to this repo — nothing there is fetched or imported at build or review time. A copy carries the back-link and drift-check requirement in [`../distribution/governance-process.md`](../distribution/governance-process.md). -- **`review/` additionally supports native reference.** A private consuming repo's `REVIEW.md` can cite a heading in this tree, and a self-hosted or local review running the `review` plugin follows that citation into a pinned `standards` checkout mounted via `--add-dir` — depth that genuinely is fetched at review time. `REVIEW.md` itself (`../REVIEW.md`) states the mechanism and its severity crosswalk in full; this is a pointer, not a restatement of it. +- **`review/` additionally supports native reference.** A consuming repo's `REVIEW.md` can cite a heading in this tree, and a self-hosted or local review running the `review` plugin follows that citation into a pinned `standards` checkout mounted via `--add-dir` — depth that genuinely is fetched at review time. While `standards` is public that mount is an anonymous checkout open to any consumer, public or private; what stays forbidden is mounting *private* content into a repo whose review output is more widely readable than that content ([`../distribution/REVIEW-CREDENTIAL.md`](../distribution/REVIEW-CREDENTIAL.md)). `REVIEW.md` itself (`../REVIEW.md`) states the mechanism and its severity crosswalk in full; this is a pointer, not a restatement of it. - **Curate on adoption.** These are agent-agnostic standards. A consumer wires them into whatever harness it uses (a contributor guide, a review checklist, an agent's instructions) without this catalog dictating that harness. Adopt the files that fit; the standards do not assume a particular toolchain, repository layout, or agent. - **Single owner, no double-statement.** Where a component config or hook lane already enforces a convention, prose references that enforcement rather than restating the rule, keeping executable policy the one source of truth. diff --git a/conventions/review/ai-review-bot-composition.md b/conventions/review/ai-review-bot-composition.md index cdd26ea2..a69e5538 100644 --- a/conventions/review/ai-review-bot-composition.md +++ b/conventions/review/ai-review-bot-composition.md @@ -6,12 +6,12 @@ Which automated AI reviewers a repository runs follows its governance class in ` - **Archived (`Archived: true`)** — a repository frozen read-only (e.g. a superseded snapshot). No PR-time reviewer: a finding against closed history has no one positioned to act on it. - **No CI surface (`RequiresCi: false`, not archived)** — a content, artifact, or single-purpose test repository with no `ci-status`-emitting workflow (for example: an artifact store, or a private acceptance-only canary target). No PR-time reviewer today, because there is no PR-time workflow surface for one to hang off. This is a **deferred**, not a permanent, absence: if such a repo grows a workflow surface, it moves into the governed class below and should adopt that class's default composition. Trigger: the first `.github/workflows/*.yml` lands in the repo. -- **Governed (`RequiresCi: true`, not archived)** — the default and largest class, spanning both public and private repos, and both org-originated and personal-account-transferred repos alike. One PR-time AI reviewer: the org's `claude-review.yml` reusable-workflow caller, reviewing every non-fork PR, with two intentional exclusions: fork PRs (no secrets and no automated review, by design — see the workflow's own header comment) and PRs whose triggering actor is `melodic-standards-sync[bot]` (excluded via the caller's `skip-actors` input: sync PRs distribute content already reviewed at its source in `standards`, so a second AI pass adds token cost without marginal catch, while the deterministic gates still run on them). +- **Governed (`RequiresCi: true`, not archived)** — the default and largest class, spanning both public and private repos, and both org-originated and personal-account-transferred repos alike. Two PR-time AI lanes, split by scope rather than doubled up. The **code-review lane**: the org's `claude-review.yml` reusable-workflow caller, reviewing every non-fork PR, with two intentional exclusions: fork PRs (no secrets and no automated review, by design — see the workflow's own header comment) and PRs whose triggering actor is `melodic-standards-sync[bot]` (excluded via the caller's `skip-actors` input: sync PRs distribute content already reviewed at its source in `standards`, so a second AI pass adds token cost without marginal catch, while the deterministic gates still run on them). The **security lane**: the org's `claude-security-review.yml` caller, running per PR head only when security-relevant paths change and skipping name-stably otherwise. The two lanes' scopes are mutually exclusive — the root `REVIEW.md` records the split — so each finding has exactly one owning reviewer; the security lane is scope partitioning, not the same-scope second reviewer whose evidence-gated exception defines the high-traffic class below. - **High-traffic primary development repo** — a repository whose PR volume and change surface justify a second PR-time reviewer, gated alongside the first via a dedicated matrix workflow. A repo in this class may additionally run author-side review agents that inform the author but never post to the PR — that wiring is repo-specific and drifts independently of this class definition, so verify it against the repo's own `.claude/` and `.github/workflows/` rather than assuming it here. Entering this class is an evidence-gated exception, not a default: it follows from a documented per-repo investigation weighing a second bot's marginal catch rate against its added comment volume and PR-cycle-time cost, including whether the two reviewers' findings are deduplicated (a coordinator is a further, separately evidence-gated step, not implied by running two reviewers). A repo does not enter this class by assumption — it earns it by running (and keeping current) the same kind of investigation. ## Why the class, not visibility or origin -`GovernedRepositorySpec`'s visibility and origin flags (public/private, org-created/personal-transferred) drive cost posture — which billable security SKUs apply, which repos are eligible for self-hosted CI — but they are not what predicts bot composition. A private, personal-origin repo and a public, org-originated repo in the governed class both run the same single-reviewer default; what moves a repo between classes is whether it has a PR-time workflow surface at all (`RequiresCi`), whether it is still live (`Archived`), and — for the two-bot exception — demonstrated PR throughput, not who owns the repo or who can see it. +`GovernedRepositorySpec`'s visibility and origin flags (public/private, org-created/personal-transferred) drive cost posture — which billable security SKUs apply, which repos are eligible for self-hosted CI — but they are not what predicts bot composition. A private, personal-origin repo and a public, org-originated repo in the governed class both run the same default lane composition; what moves a repo between classes is whether it has a PR-time workflow surface at all (`RequiresCi`), whether it is still live (`Archived`), and — for the two-bot exception — demonstrated PR throughput, not who owns the repo or who can see it. ## Freshness diff --git a/distribution/REVIEW-CREDENTIAL.md b/distribution/REVIEW-CREDENTIAL.md index f9eaf897..d6be49fc 100644 --- a/distribution/REVIEW-CREDENTIAL.md +++ b/distribution/REVIEW-CREDENTIAL.md @@ -86,13 +86,15 @@ nothing to provision. > unedited so the re-derivation can see exactly what was proposed; they are a > record of a superseded plan, not instructions. -The native-reference mount is **forbidden on any public calling repo**. -Mounting private content into a review agent whose output is published -wherever the calling repo's visibility allows moves the confidentiality -boundary from "who can read the mounted repo" to "who can read the review -output." A credential scoped to exactly the private targets, and never -resolvable in a public workflow run, is what keeps that boundary from moving -silently. +A native-reference mount of **private** content is forbidden on any public +calling repo. Mounting private content into a review agent whose output is +published wherever the calling repo's visibility allows moves the +confidentiality boundary from "who can read the mounted repo" to "who can +read the review output." A credential scoped to exactly the private targets, +and never resolvable in a public workflow run, is what keeps that boundary +from moving silently. Mounting **public** content raises no such boundary: +while `standards` is public, the anonymous mount is allowed from any calling +repo, public or private. That reasoning stands on its own terms; what changed is that `standards` is no longer a private source, so it no longer has a boundary of this kind to @@ -113,8 +115,11 @@ asked for, performed against the corrected visibility table above: ineligibility; the credential is simply unnecessary for the mount. - **Provisioning stays deferred behind `github-iac` governance**, and under the current state there is nothing to provision. The classification below - is retained as the record of what would be provisioned if the need - returns. + is retained strictly as the historical record of the superseded proposal — + per the caution above, its scope and storage lists must not be provisioned + as written. If the need returns, re-derive the classification from the + then-current visibility table first; the marketplace-authentication case + may require a different credential shape altogether. - **The need returns in exactly two cases**, both already covered by the review triggers: `standards` (or another mounted source) becoming private again, or private-marketplace authentication becoming a