Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
73 changes: 46 additions & 27 deletions REVIEW.md
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,17 @@ 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.

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
Comment thread
kyle-sexton marked this conversation as resolved.
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

This organization's criteria (`conventions/review/` in `standards`) resolve
Expand Down Expand Up @@ -48,7 +59,41 @@ 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. 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.
Comment on lines +67 to +70

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Gate security suppression on a lane that can run

When a repository first adds .github/workflows/claude-security-review.yml, the file exists in the PR checkout and therefore satisfies this test, but a pull_request workflow newly introduced by that PR cannot run until the workflow exists on the default branch. The code-review lane will consequently omit every security finding from the rollout PR even though no security lane reviews it, so any security-sensitive workflow change bundled into that PR has no owning reviewer. Gate suppression on the security lane being active for the current PR (or on the caller existing in the base branch), rather than mere head-tree file existence.

AGENTS.md reference: AGENTS.md:L29-L32

Useful? React with 👍 / 👎.

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:

- 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
Expand All @@ -62,29 +107,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.
2 changes: 1 addition & 1 deletion conventions/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.

Expand Down
4 changes: 2 additions & 2 deletions conventions/review/ai-review-bot-composition.md
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
64 changes: 48 additions & 16 deletions distribution/REVIEW-CREDENTIAL.md
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -84,18 +86,46 @@ as an open item, not attempted here.
> 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
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.
Comment thread
kyle-sexton marked this conversation as resolved.
- **Provisioning stays deferred behind `github-iac` governance**, and under
the current state there is nothing to provision. The classification below
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
documented, credential-dependent path (the one genuinely
credential-dependent open question below).

## Classification

- **Type:** a dedicated **GitHub App installation token**, not a personal
Expand Down Expand Up @@ -188,11 +218,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.
Expand Down