Repository navigation
docs(distribution): correct the review-credential visibility facts - #276
Conversation
REVIEW-CREDENTIAL.md classified `standards` and `claude-code-plugins` as private, "confirmed firsthand". Both are public — re-verified against the live GitHub API on 2026-07-26. `dotfiles`, `provisioning`, `github-iac`, and `medley` are private as stated; `ci-workflows` is public as stated. Two consequences, in severity order. The document's premise is void. Its rationale is that mounting private `standards` content into a review agent moves the confidentiality boundary from "who can read standards" to "who can read the review output". With `standards` public there is no such boundary, so the App this document classifies may be unnecessary rather than merely scoped too widely. Its scope and storage lists would leak secrets into public repositories. They name `standards` and `claude-code-plugins` as private targets, so wiring the org-secret visibility list as written would make both STANDARDS_REVIEW_APP_* secrets resolvable in a public repo's workflow runs — violating this document's own rule that they are never resolvable in a public-repo workflow run. Nothing is provisioned, so nothing is exposed today; this closes the gap before anything is created. The superseded lists are left unedited under an explicit caution so a re-derivation can see what was proposed rather than inheriting a silently patched plan. The republication limits are retained: the prompt-injection control is independent of repository visibility and keeps its full force. Found while reviewing the runner-policy contract approval for ci-workflows@e295107 (#275), which gates these same two secrets out of every governed caller. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Claude finished @kyle-sexton's task in 1m 44s —— View job Review
The change is technically sound and well-scoped. The editorial approach — adding a prominent CAUTION block, leaving the superseded lists intact for re-derivation, and disentangling the prompt-injection control from the now-void confidentiality rationale — is the right call. No blocking issues. One nit in the retained text: The republication limits preamble (lines 106–110) correctly explains that the bullets are kept for the prompt-injection control, not confidentiality. But the second bullet was left fully verbatim, and it still says:
Line 122–124 — Observations (no action required):
The CAUTION block's language is clear and its two-consequence ordering (void premise first, secret-leak risk second) matches the actual severity. No findings on correctness or security; the nit above is the only real edit worth making before merge. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 41e5d8eb9b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…serted Review found the correction had been applied to the site in front of it rather than to every site the fact governs — the same failure the fact itself documents. Two other places still described the runner-policy enrollment set as private while it contains the public `claude-code-plugins`: distribution/README.md's consumer handoff, and the sync-manifest test's assertion label. The enrolled set is correct; only the adjective was wrong, so the wording now says what the list actually is — an adoption list, not a visibility class. Adds repository visibility to the review triggers. Visibility is mutable, and a stale firsthand claim is precisely what invalidated this classification once already, so the claim now carries a verification date and a copy-paste recheck command instead of resting on its own authority (conventions/engineering/documentation-and-citations.md: time-bound external claims need a recheck trigger). Rewrites the retained republication limits around output integrity rather than access boundaries. With `standards` public no review output can reach an audience that could not already read the cited criteria, so framing them as confidentiality controls preserved the premise the change had just retired. The prompt-injection-via-diff control is the part that survives, and it survives on its own terms. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Claude finished @kyle-sexton's task in 1m 35s —— View job Review
The second commit ( One nit carried from the prior review: The prompt-injection bullet at
Observations (no action required):
The overall approach remains sound. The one remaining edit is the phrase above; everything else is in order. |
|
All three P2s addressed in 1. Reconcile the public classification elsewhere — fixed, and the finding was right about more than the two sites it named. This was the exact failure the change itself documents: the correction was applied to the file in front of me instead of to every site the fact governs. I grepped the tracked tree rather than patching the two cited lines:
Other hits for the private-repo vocabulary ( 2. Visibility added to the review triggers — fixed. You are right that this is the one trigger whose absence caused the failure. Visibility is now an explicit recheck trigger, the claim carries its verification date (2026-07-26), and there is a copy-paste 3. Remaining private-boundary rationale — fixed. You are correct that the sentence preserved the premise the change had just retired. Rewritten around output integrity rather than access boundaries: with Lint clean: markdownlint 0 errors, typos clean, shellcheck clean, gitleaks clean, editorconfig clean. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6f81c9b961
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…imary source Review noted the table turned repository visibility into a durable security-bearing claim while resting on a single generating source (authenticated `gh api`). source-authority-tiers.md is explicit that such a claim needs more primary-rung sources, not more corroborators. Adds a second, genuinely independent primary: anonymous git transport (`git -c credential.helper= ls-remote`, GIT_TERMINAL_PROMPT=0), which reaches a public repository and fails on a private one. Different plane from the REST API and a different authorization decision, so it is not the same source counted twice. Both agree on all seven rows. The four private rows are the control — the probe discriminates rather than succeeding for everything, so the two public rows are a finding, not an artifact of the method. A third plane agrees for claude-code-plugins: Actions supplies CI_REPOSITORY_VISIBILITY to its own runner-policy job, which is the value the gate reads. The recheck command now runs both planes and states what to do when they disagree — record the claim as open rather than resolve it by preferring one. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Claude finished @kyle-sexton's task in 1m 46s —— View job Review
The third commit ( Corroboration assessment — Codex P2 resolved: The CAUTION block now cites two independent primary-rung sources:
These are genuinely independent: different protocols, different authorization decisions (authenticated identity vs. unauthenticated reachability). The document correctly notes they are "not the same source twice." A third plane is cited for The control group (four private repos that return The recheck script is well-constructed: both planes in a single loop, side-by-side output, and the surrounding prose explicitly states "if they diverge, the claim is uncorroborated at any count." The The citation "requires more primary sources for those, not more corroborators" is a precise reading of One nit — third notice, still unaddressed:
This was flagged in both prior reviews. The preamble added in commit Observations (no action required):
The overall three-commit arc is sound. The one remaining edit is the phrase above. |
|
Fourth finding addressed in Corroborate the durable visibility classification — you were right, and the convention you cited says something stronger than "add a corroborator".
Second primary added — anonymous git transport: with Both agree on all seven rows:
The four private rows are the control. Without them the probe proves nothing — a method that returns "public" for everything would produce the same two public rows. They discriminate, so the finding is a finding and not an artifact. A third plane agrees for The recheck command in the review-triggers section now runs both planes, and states what to do when they diverge — record the claim as open rather than resolve it by preferring one, per the same convention's "a claim where the retrieved sources disagree is not corroborated at any count". |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d2ca0fe46b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…ming it Review is right that the three observation planes are one source, not three: authenticated REST, anonymous git transport, and the Actions-supplied visibility value all read GitHub's repository state, and source-authority-tiers.md counts sources sharing an upstream pool once. Conceding rather than patching. Repository visibility is a fact GitHub owns; no independent pool exists to corroborate it from, so the floor of one primary plus two independent corroborators is unmeetable for this class of claim, not merely unmet here. Labelling three planes as independent sources was the overclaim. The text now says what the evidence is worth: three planes buy protection against observation error — stale credential context, cached value, mistyped repository — which is the realistic failure mode, and buy nothing against GitHub being wrong about its own repositories, which is not a failure mode because that state is the fact. Also ungates the open questions. They were gated on an App the caution block says not to provision, which deadlocks the re-derivation that block asks for. With `standards` public an anonymous checkout mounts the same tree, so --add-dir visibility, SDK rule loading, default tool grants, and OAuth quota are all pilotable now; only private-marketplace authentication remains genuinely credential-dependent. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Claude finished @kyle-sexton's task in 1m 39s —— View job Review
The fourth commit ( Codex P2 — corroboration overclaiming — resolved: The CAUTION block now says outright: "These are one source, not three, and the claim does not meet this repository's corroboration floor." It then explains why the floor is unmeetable for this class of claim — repository visibility is a fact GitHub owns; no independent pool exists outside GitHub to corroborate it from. The commit doesn't try to manufacture additional sources; it correctly characterizes the limit and explains what multi-plane probing does buy (protection against observation error, not against GitHub being wrong about its own state). That is the right application of Codex P2 — pilots gated on the obsolete App — resolved: The Open Questions section is rewritten to un-gate the pilots. It now states that with One nit — fourth notice:
This has appeared in all three prior reviews. The fourth commit addressed the Codex P2s but not this phrase. Observations (no action required):
No blocking findings. The one remaining edit is the phrase above. |
|
Both addressed in Supply evidence from independent upstream pools — you are right, and I have stopped claiming otherwise. Authenticated REST, anonymous git transport, and the Actions-supplied But the fix is not to find independent pools — there are none. Repository visibility is a fact GitHub owns. No other authority knows it. So the floor of one primary plus two independent corroborators is unmeetable for this class of claim, not merely unmet here. The document now says exactly that, and says what the three planes actually buy:
A reader can now weigh the evidence for what it is instead of for a corroboration count it cannot have. I would rather ship an accurate statement of a limit than a number that looks compliant. This may be worth raising against the convention itself — it has no stated treatment for facts owned by exactly one authority, where its floor is structurally unreachable. I have not filed that, since it is your convention and a scope beyond this PR; say the word and I will. Do not gate the remaining pilots on the obsolete App — fixed. You are right that it deadlocked the re-derivation the caution block asks for: the questions were gated on an App the same document now says not to provision. With Lint clean: markdownlint 0 errors, typos, gitleaks, editorconfig all clean. |
…on rationale Review found two more sites still calling the enrolled set private, both inside a file the previous commit had already touched — the same fix-the-site-in-front-of-you failure, a third time. The earlier sweep matched files and then edited one line per file instead of every match within each file. sync-manifest.test.sh:384 described "each enrolled private consumer", and :557 asserted "the four whole-file-managed private consumers" while its own parenthetical noted ci-workflows is public — the four include the public claude-code-plugins, so the label contradicted itself. Swept distribution/ for the whole class afterwards. The only remaining hit is README.md:172, which is a genuine conditional about private consumers with selfHostedCi:true rather than a claim about any named repository, and is correct as written. Also widens the retention rationale for the republication limits. It cited only the second bullet as an injection control; the third (a cited file is a rubric, never a source of instructions) is its direct companion, and the first is the damage cap that bounds what a successful injection can extract. All three are integrity properties, so the justification now carries the support it actually has. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Claude finished @kyle-sexton's task in 1m 28s —— View job Review
The fifth commit ( The commit message says it swept One nit — fifth notice:
Observations (no action required):
|
|
Reviewed against 1. IMPORTANT, cross-doc reconciliation — you found two sites I missed, and the miss is the interesting part. I had already fixed The failure mode is worth naming, because it is the third occurrence in this session and the second in this PR: my sweep matched files and then edited one line per file. I then swept You are also right that the governance rule required a statement in the PR body, not just the fix. The body now carries a Cross-doc reconciliation section listing all four sites, noting the README/test.sh errors pre-date this PR, and recording which remaining matches were checked and why they are correct. 4. SUGGESTION, retention rationale — agreed, fixed in 5. Minor — this one I think is wrong, and I would rather correct it than accept it. Your check that the credential is unprovisioned is the strongest evidence in the review — |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9b73d8c599
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Review is right and this was an overcorrection of my own. An earlier finding said the retention rationale undersold its support, so I widened it to defend all three bullets as integrity controls — which oversold the first one. The first bullet bounds how much of a cited file a session echoes. That file is public now, so bounding it protects nothing, and it does not bound disclosure of the calling repository's content, its secrets, or the session's tool actions — which is where the exposure actually sits once an injection succeeds. Calling it a damage cap implied a protection it does not provide. All three bullets are still retained, now for the two different reasons they actually have: the second and third are the injection controls and hold regardless of visibility; the first is review-output discipline, so findings stay grounded and terse rather than quoting rubric at the reader. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Fixed in Your earlier suggestion said the retention rationale undersold its support, so I widened it to defend all three bullets as integrity controls. That oversold the first one, and you caught it in the same PR. You are right about what it actually bounds: the first bullet limits how much of a cited file a session echoes. That file is public now, so bounding it protects nothing — and it does not bound disclosure of the calling repository's content, its secrets, or the session's tool actions, which is where the exposure sits once an injection succeeds. "Damage cap" implied a protection it does not provide. All three bullets are still retained, now for the two different reasons they actually have:
That is three rounds where the correct move was to weaken a claim rather than defend it: the corroboration count, the pilot gating, and now this. Worth noting since the pattern is mine, not the document's. |
|
Claude finished @kyle-sexton's task in 1m 31s —— View job Review
The sixth commit ( One nit — sixth notice, still unaddressed:
This has appeared in every prior review round (commits Observations (no action required):
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e088407a34
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| > **These are one source, not three, and the claim does not meet this | ||
| > repository's corroboration floor.** Under | ||
| > [`source-authority-tiers.md`](../conventions/engineering/source-authority-tiers.md) | ||
| > sources that "share an upstream pool" count once, and all three read GitHub's | ||
| > repository state. Repository visibility is a fact GitHub *owns*; no |
There was a problem hiding this comment.
The fresh evidence in this revision is the explicit concession that all three observations count as one source and therefore do not meet the repository's corroboration floor, yet the following table and downstream edits still treat the visibility classification as settled durable security guidance. Either keep the classification and resulting credential decision open until the stated floor is met, or amend the source-authority convention to define and justify an exception for upstream-owned state; declaring the rule unmeetable does not itself waive it.
AGENTS.md reference: AGENTS.md:L25-L28
Useful? React with 👍 / 👎.
| > 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**. |
There was a problem hiding this comment.
When a public caller such as claude-code-plugins uses the distributed review instructions, this absolute prohibition still forbids its native-reference mount even though this change establishes that the mounted standards source is public and needs no credential. The following rationale only applies when the mounted content is private, so qualify the prohibition accordingly; otherwise operators can disable a valid public-source review path based on a confidentiality boundary that lines 95–97 say no longer exists.
Useful? React with 👍 / 👎.
| > boundary that no longer exists. A native-reference mount of a public repo | ||
| > needs no credential at all, and the App this document classifies may be | ||
| > unnecessary rather than merely scoped too widely. |
There was a problem hiding this comment.
This new conclusion that mounting public standards needs no credential leaves distribution/README.md:22-24 contradicting it: the distribution entry point still says a private caller uses a separate read-only credential for exactly this mount. Readers starting from the README can therefore continue designing or provisioning the obsolete App without reaching the caution here; update that overview to describe the classification as superseded or unresolved.
AGENTS.md reference: AGENTS.md:L29-L32
Useful? React with 👍 / 👎.
…ntial (#278) ## What - **REVIEW.md**: restructured into mutually exclusive lane-scope sections — `## Code-review lane scope` (audit-logging + atomicity always-checks) and `## Security lane scope` (the four `blocking` always-checks). Deleted the report-stage filtering sections (`## Do not report`, `## Cap the nits`) per Anthropic's Sonnet-5 harness-effect guidance. `## Severity`, `## Depth`, and `blocking` tag semantics unchanged. - **distribution/REVIEW-CREDENTIAL.md**: re-derivation #264 asked for, on top of #276's corrected visibility facts — with `standards` public no consumer needs the credential (anonymous checkout reaches the same tree); the `--add-dir` subagent-visibility open question marked empirically resolved; provisioning stays deferred behind github-iac governance. Closes #264 ## Related - #276 — corrected the visibility facts this re-derivation builds on (merged 2026-07-26). - melodic-software/ci-workflows `docs/topics/claude-review-lanes/PLAN.md` — the approved implementation plan; this PR is its Phase 1. Phase 2's model-switch PR is gated on the five downstream `review-instructions` sync PRs this merge triggers. - melodic-software/claude-code-plugins#289 — carries the verification annex #264 drew from. ## Notes - **Independent verification (fresh-context agent, 7/7 criteria PASS + 1 blocking flag, fixed in 50b4969):** the first cut asserted the security lane consumes this file (it runs from its own security-only prompt and never reads it) and made the security-findings exclusion unconditional — which would have suppressed the four `blocking` checks on the five repos whose only reviewer is the code-review lane until the Phase 3 fleet rollout. The intro now records scope ownership rather than wiring, and the exclusion is gated on a `claude-security-review` workflow existing in the repo — self-resolving as the rollout proceeds, no coverage window. - Deviation from the plan's original premise: the plan predates #276 and framed the re-derivation as "claude-code-plugins is public → ineligible". #276 established `standards` itself is public, which supersedes per-repo eligibility — the re-derivation records that no credential is needed at all under current state. Direction (defer provisioning) unchanged. - Deviation: the plan said "update `## Review triggers`" in REVIEW-CREDENTIAL.md; left untouched — verified the existing triggers already cover both credential-return cases (visibility change; open-question resolution). - Known loss: the deleted "Do not report" section was the only REVIEW.md mention of `conventions/review/overlays/`; overlays still reach plugin surfaces via `conventions/review/README.md`. - Follow-up (ci-workflows Phase 2): `claude-review.yml`'s default prompt says "review for … security"; with this split that wording should name the lane and defer security scope per this file. - `review-instructions` is a managed component in 5 repos and locally-owned in medley; medley content-equivalence is a Phase 5 follow-up in the plan above. - Cross-doc reconciliation swept: no other doc cites the deleted sections; `conventions/review/README.md`'s `blocking` crosswalk statements still hold against the restructured file. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
1 participant
Summary
distribution/REVIEW-CREDENTIAL.mdclassifiesstandardsandclaude-code-pluginsas private, "confirmed firsthand". Both are public. Re-verified against the live GitHub API on 2026-07-26:standardsclaude-code-pluginsci-workflowsdotfiles,provisioning,github-iac,medleyTwo consequences, in severity order.
1. The document's premise is void. Its entire rationale is that mounting private
standardscontent into a review agent moves the confidentiality boundary from "who can readstandards" to "who can read the review output". Withstandardspublic there is no such boundary. A native-reference mount of a public repo needs no credential at all — so the App this document classifies may be unnecessary, not merely scoped too widely.2. Its scope and storage lists would leak secrets into public repositories. They name
standardsandclaude-code-pluginsas private targets. Wiring the org-secret visibility list as written would make bothSTANDARDS_REVIEW_APP_*secrets resolvable in a public repository's workflow runs — violating this document's own rule, two paragraphs later, that they are "never resolvable in a public-repo workflow run".Nothing is provisioned (
Status: classified, not provisioned), so nothing is exposed today. This closes the gap before anything is created.What this change does and does not do
CAUTIONblock with the verified table and both consequences, stating plainly that the section must not be provisioned from and that the requirement should be re-derived from scratch — possibly to "no credential needed".This does not decide whether the credential should exist. That is a design question for whoever re-derives it, and the document now says so instead of implying the answer.
Cross-doc reconciliation
Per
distribution/governance-process.md's "Cross-doc reconciliation before a normative-doc change merges", stated explicitly rather than implied:Searched the tracked tree for every site asserting the same concept —
git grep -nE "private (target|consumer|caller|repo)s?|enrolled private|whole-file-managed private". Four sites carried the stale claim and all four are reconciled in this PR:distribution/REVIEW-CREDENTIAL.mddistribution/README.md:141distribution/sync-manifest.test.sh:384distribution/sync-manifest.test.sh:557The README.md and test.sh errors pre-date this PR —
claude-code-pluginswas already public before this document was touched — but the duty to reconcile attaches to the change correcting the underlying fact, so they are fixed here rather than deferred.Nothing else in the repository now contradicts the corrected fact. The remaining matches were checked individually and are correct as written:
components/runner-policy/README.mdanddistribution/README.md:172describe private repositories generically (conditional behaviour forselfHostedCi: trueconsumers) without asserting any named repository's visibility;lychee.tomland.github/dependabot.ymlconfigure tooling against private repos as a class.Test plan
typos— clean.markdownlint-cli2— 0 errors.gitleaks,editorconfig— clean (pre-commit hook run).gh api repos/melodic-software/<repo> --jq .visibilityrather than carried over from the document.Related
No linked issue.
Found while reviewing the runner-policy contract approval for
ci-workflows@e295107(#275). That approval gates these same two secrets out of every governed caller viaallowedSecrets, which is what prompted checking whether the credential's own classification still held. It did not.