fix(ci): re-narrow the security lane's skip-actor exception to two actors - #1845
fix(ci): re-narrow the security lane's skip-actor exception to two actors#1845kyle-sexton wants to merge 5 commits into
Conversation
…o ratified actors The v0.9.1 lane re-pin deleted this caller's explicit `skip-actors` list, so it inherited the reusable workflow's default (`dependabot[bot],claude[bot],melodic-ai[bot],melodic-standards-sync[bot]`) and silently widened ADR 0002's operator-ratified two-actor exception to four — `claude[bot]` and `melodic-ai[bot]` PRs would satisfy the REQUIRED `security-review / security-review` check with no review run, on security-sensitive paths. Re-deliberated per the ADR's own revisit trigger (#1767) and rejected. The caller restores `skip-actors: "dependabot[bot],melodic-standards-sync[bot]"`. The two ratified actors each carry a compensating control; the two new ones carry none, and the general review lane skips them too, so an AI-app-pushed commit would get zero review of any kind. A stale explicit list fails closed; a stale inherited default fails open — which is exactly how this widening happened. The general lane (`claude-review.yml`) is deliberately untouched: advisory with no required check, so its skip list is runner-minute economy, not evidence. ADR 0002 gains an addendum recording the ruling, its rationale, the rejected alternatives, the cross-vendor advisory, and the composition with github-iac #228/#248. The final revisit trigger is rewritten to cover both widening channels — a caller-list edit AND inherited-default drift at a re-pin, the channel that actually fired — and the caller carries a matching comment, since runner-policy rejects unexpected inputs but cannot require one. Fixes #1767 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FVoZoMYXqf8ZVbQYixPVPW
|
Claude finished @kyle-sexton's task in 2m 29s —— View job Review: PR #1845
Per FindingsNo correctness, design, or maintainability issues found. This is a well-scoped, well-documented two-file change. Independently verified the PR's factual claims rather than taking them on faith:
Documentation quality: the new ADR addendum ( Workflow comment ( I wasn't able to execute No |
![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: ac1ad67cd3
ℹ️ 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".
| # inherited default fails open. runner-policy rejects inputs outside the | ||
| # reviewed contract but cannot require one, so dropping this line | ||
| # re-widens the exception silently and CI stays green. | ||
| skip-actors: "dependabot[bot],melodic-standards-sync[bot]" |
There was a problem hiding this comment.
At this revision, .github/standards/runner-policy/policy.json allows only runner and paths-file for the pinned security-review workflow, while the Runner policy CI job rejects every input absent from that contract. Consequently, this line makes every CI run fail with runner-target-contract: ... inputs absent from its reviewed contract: skip-actors, so the upstream contract change and its standards sync must land before this input is added rather than leaving the commit permanently unmergeable or patching the managed copy locally.
AGENTS.md reference: AGENTS.md:L11-L16
Useful? React with 👍 / 👎.
|
Claude has reviewed this PR 5 times. The lane skips further automatic reviews after 5; deleting this comment resets the count. |
The addendum claimed the ci-workflows split-the-default deliberation was "recorded there" and left the standards `requiredInputs` gap unfiled, so both follow-ups could drop silently. File them and cite the numbers: ci-workflows#330 (narrow the security reusable's own skip-actors default) and standards#308 (a `requiredInputs` contract field, so deleting the caller's input cannot pass green). Also grounds the latency-is-not-absence strand: the `@claude` dispatcher whose arrival would activate the bypass is not hypothetical — ci-workflows#255 proposes exactly that lane org-wide. Refs #1767 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FVoZoMYXqf8ZVbQYixPVPW
|
Claude finished @kyle-sexton's task in 3m 24s —— View job Review: PR #1845 (update — new commit
|
The latency-is-not-absence strand said adopting the proposed `@claude` responder lane would activate the review bypass. Its V1 is deliberately read-only — no Edit/Write, no push — so V1 would not; only its V2 (tag-mode fix-and-push, behind that issue's own approval gate) opens the channel. Overstating the imminence weakens the argument it was meant to support, and the accurate version is the stronger one: the activating step is gated, which is precisely when pre-authorizing a bypass is cheapest to avoid. Refs #1767 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FVoZoMYXqf8ZVbQYixPVPW
|
Claude finished @kyle-sexton's task in 58s —— View job Review: PR #1845 (update — new commit
|
`#248` and `#228` were written bare inside a claude-code-plugins document, so GitHub autolinks them to this repository's own items — the wrong targets. The repo's ADR convention requires a repository-qualified URL for a cross-repository reference. Qualify both, and name the four app-pinned contexts explicitly (`pr-title / pr-title`, `do-not-merge / do-not-merge`, `ci-status`, `security-review / security-review`) rather than leaving "all four" for a future reader to reconstruct; verified against the live rulesets, all pinned to integration_id 15368. Refs #1767 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FVoZoMYXqf8ZVbQYixPVPW
|
Claude finished @kyle-sexton's task in 1m 4s —— View job Review: PR #1845 (update — new commit
|
The parenthetical explained why the reference is repository-qualified. The ADR convention already owns that rule; restating it beside one use is duplication that says nothing about the decision. Refs #1767 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FVoZoMYXqf8ZVbQYixPVPW
|
Claude finished @kyle-sexton's task in 48s —— View job Review: PR #1845 (update — new commit
|
…contract (#307) ## Summary Adds `skip-actors` to the `allowedInputs` of the runner-policy contract entry for `melodic-software/ci-workflows/.github/workflows/claude-security-review.yml@c136b27` — the pin both security-lane consumers run today. Why now: `claude-code-plugins` re-deliberated ADR 0002's skip-actor exception (melodic-software/claude-code-plugins#1767) and **rejected** the reusable workflow's inherited default (`dependabot[bot],claude[bot],melodic-ai[bot],melodic-standards-sync[bot]`) for the *security* lane. `claude[bot]` and `melodic-ai[bot]` carry none of the compensating controls that justified the two operator-ratified actors — a Dependabot pin bump is forced through this very runner-policy contract, and a standards-sync PR materializes byte-exact upstream-reviewed content — so an AI-app-pushed commit would satisfy that repo's **required** security check with no review run of any kind. Its caller must therefore pass an explicit narrow list (`dependabot[bot],melodic-standards-sync[bot]`), and the caller can only pass an input the contract admits. This **restores parity rather than widening new ground**: the previous approved security SHA's contract (`66073e58`) already admitted `skip-actors`; the `c136b27` entry dropped it, which is why the consumer's caller could not keep its explicit list through the re-pin. The `claude-review.yml` contracts at `df54d0e` and `4dbb0df` admit it too. A second commit reconciles `components/runner-policy/README.md`, whose `c136b27` review note still recorded the security caller as able to pass only `runner` and `paths-file` — a reviewer consulting it would have read `skip-actors` as denied. The `(of 8)` denominator is unchanged: `skip-actors` is one of the reusable's eight declared inputs at that SHA (`runner`, `paths`, `paths-file`, `prompt`, `claude-args`, `exclude-comments-by-actor`, `skip-actors`, `retry-delay-seconds`), so the admitted set stays narrower than the declared surface. **Blast radius — permissive widening only.** `runner-policy` is a managed component across the fleet, so this materializes everywhere, but nothing starts passing the input: the validator rejects inputs *outside* `allowedInputs` and never requires one. The only behavior change is that a caller may now pass `skip-actors` to this pin without the policy check failing. The entry carries no `autoApproved` block, so it is a human-reviewed contract edit by design. Known residual gap (follow-up, not this PR): the validator rejects unexpected inputs but cannot *require* one, so a future re-pin that drops a caller's `skip-actors` line passes CI and silently re-widens the exception. Guarding that needs a `requiredInputs`-style contract field — filed as #308, which also records the matching hazard in the parked `claude-security-review-caller` component (its bytes pass no `skip-actors`, so an adopter inherits the widened default when it unparks). ## Test plan - `node -e "JSON.parse(...)"` on `components/runner-policy/policy.json` — parses. - `biome check components/runner-policy/policy.json` — clean (also clean via the `pre-commit` lefthook run: typos, gitleaks, editorconfig, biome all pass). - Diff is one line: `"allowedInputs": ["runner", "paths-file"]` → `["runner", "paths-file", "skip-actors"]`, matching the file's one-line-array style and the schema's `uniqueStringArray`. - Input verified against the reusable at the pinned SHA: `skip-actors` is a declared `workflow_call` input on `claude-security-review.yml@c136b27`, defaulting to the four-actor list and gating the `security-review` **job** (not the workflow), so a skipped actor still reports a name-stable passing check. - Downstream verification: with this change applied locally to the consumer's materialized `policy.json`, `runner-policy.mjs` reports `Runner policy passed.` against the narrowed caller; without it, exactly one finding (`inputs absent from its reviewed contract: skip-actors`). ## Related No linked issue in this repository — deliberately. The deliberation and its record live in the consuming repo (melodic-software/claude-code-plugins#1767), which is closed by its own caller PR; a cross-repo closing keyword here would close that issue on the wrong merge. - Decision: melodic-software/claude-code-plugins#1767 (ADR 0002 re-deliberation) - Sequencing: this PR → standards→claude-code-plugins sync PR → the caller + ADR PR in claude-code-plugins - Consuming PR (blocked on this one): melodic-software/claude-code-plugins#1845 — its `runner-policy` check stays red until this merges and the sync materializes - Follow-ups filed, not implemented here: #308 (`requiredInputs` contract field) and melodic-software/ci-workflows#330 (narrow the security reusable's own `skip-actors` default) - Contract precedent: #211 (security-review contract), #229 (`paths-file` input), #161 (`claude-review` runner-input contract) 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01FVoZoMYXqf8ZVbQYixPVPW --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
|
Cross-lane finding: this PR cannot pass as written, and the fix belongs upstream rather than in this caller.
The substantive argument here is correct and I am not disputing it: inheriting the reusable's four-actor default lets But the concern is fleet-wide, not repo-local — every consumer that re-pins to v0.9.1 inherits the same widening, and a per-caller override is exactly the drift the centralization was meant to end (this repo's callers previously lost the ban list that way). So the durable fix is to narrow the security reusable's own default in ci-workflows, leaving the review lane's wider default alone since that lane is advisory and gates nothing. I am opening that upstream change now and will link it here. Suggest holding this PR until it lands, then closing it as superseded — or keeping only its ADR 0002 documentation half, which stands on its own merits. |
|
Full analysis and disposition recorded on #1767 (issuecomment-5147088300). Summary for this PR specifically: the workflow half cannot be salvaged as written — passing The real fix lives upstream in ci-workflows (both the |
|
Stopping here rather than resolving this one — the conflict is a decision, not a mechanic, and the decision is yours. What happenedI rebased this branch onto
and states four is the ratified baseline that future additions are measured against. Why I did not pickThat is an explicit operator decision dated four days after this branch's argument, on a security-gate exception. Merging this PR reverses it; closing it discards the reasoning that produced it. Neither is a call I should make silently inside a conflict resolution — and the ADR itself demands the opposite, since its revisit trigger requires the widening be re-deliberated and recorded, not settled by whichever branch merged last. Worth noting: the 2026-08-04 addendum reads as having considered this branch's position — its Branch A / Branch B framing weighs the two-actor case explicitly and rules against it on the The branch is untouched; The three ways forward
Tell me which and I will finish it. |
|
Closing as superseded, with the operator's confirmation. What supersedes itThis branch's Addendum (2026-07-31) rules the skip-actor widening REJECTED and re-narrows the exception to two actors.
and it names four as the baseline future additions are measured against. That addendum weighs this branch's position on its merits — its Branch A / Branch B framing puts the two-actor case directly against the Merging would have reversed that decision inside a conflict resolution, which is not where a security-gate exception should be re-decided. What was salvagedThe branch's Ported in #2069, with the actor list left at the ratified four. What is NOT closed by this#1767 stays open. The residual gap it tracks is structural, not editorial: The branch stays
|
No linked issue — this closes nothing. #1767 stays open deliberately: its residual gap is structural (`runner-policy` cannot *require* an input), and a comment guards against a careful reviewer, not an inattentive one. Closing it on a documentation change would record the gap as fixed when it is not. ## What Seventeen lines of comment above `skip-actors` in `.github/workflows/claude-security-review.yml`. No value changes; the actor list stays at the ratified four. ## Why The existing comment says the list is stated explicitly rather than inherited, and records how #1766 widened it. It does not say why deleting the line is unsafe **when its value matches the upstream default of the moment** — which is the only state in which a reviewer is tempted to delete it, and precisely the state #1766 was in. Two things the re-pin reviewer needs and the comment did not carry: - **Deleting is not a no-op at parity.** The four actors are ADR 0002's 2026-08-04 ratified baseline. Inheriting re-delegates that ratification to whatever `ci-workflows` ships next, and every actor in an inherited default satisfies this repo's REQUIRED `security-review / security-review` check with no review run, on security-sensitive paths. - **The failure directions are asymmetric**, which is what makes explicit correct here without contradicting the fleet's inheritance-over-explicit-list default elsewhere. A stale explicit list fails CLOSED and LOUD — for an actor the reusable's `allowed_bots` does not permit, the action throws and the fail-closed mapping turns that into a red required check no push can fix. A stale inherited default fails OPEN and SILENT — an actor this repo never deliberated skips review with CI green. Loud-and-blocking is the direction to fail in; silent-and-permissive is not. The first draft of this comment described the closed direction as "an actor gets a review it may not have needed — visible, cheap." That was wrong, and review caught it: ADR 0002's `skip-actors`-vs-`allowed_bots` section records that removing an actor from `skip-actors` alone does not restore review of its PRs — it blocks the merge. Corrected in the second commit, which also notes that un-skipping an actor for real requires widening `allowed_bots` upstream, so removing a name here is never a one-line change. The comment also names standards#308 at the point of use: `runner-policy` rejects inputs outside the reviewed contract but cannot *require* one, so dropping this line re-widens the exception with CI green and nothing said. ## Provenance Salvaged from PR #1845, which I closed as superseded. That branch re-narrowed the exception to two actors on 2026-07-31; the 2026-08-04 addendum records an explicit operator decision ratifying four and names four as the baseline future additions are measured against, having weighed the two-actor case on its merits. The narrowing is therefore correctly dead — but the branch's comment block was the better explanation of an invariant that survives the ruling either way, so it is ported here with the actor list left alone. ## Verification - `actionlint` clean on the changed file. - The `skip-actors` value is byte-identical before and after — confirmed by reading the parsed line back, not by eye. - Comment-only diff: 17 insertions, 0 deletions. ## Related - #1767 — the re-deliberation issue this comment serves; left open, since the residual `requiredInputs` gap is not closed by a comment. - #1845 — the superseded branch this is salvaged from. - #1766 — the re-pin that dropped the line and silently widened the exception. - `docs/adr/0002-default-on-ai-review-advisory-with-earned-promotion.md` — the 2026-08-04 addendum that ratified four actors. - melodic-software/standards#308 — the `requiredInputs`-style contract field that would make this structurally enforced rather than comment-enforced. --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 participant
Summary
Implements the delegated-authority decision recorded on #1767: the skip-actor widening
introduced by the v0.9.1 lane re-pin is REJECTED for the security lane.
.github/workflows/claude-security-review.ymlrestores an explicitskip-actors: "dependabot[bot],melodic-standards-sync[bot]", re-narrowing the exception toexactly the two actors ADR 0002's step-3 addendum ratified. Inheriting the reusable workflow's
default (
dependabot[bot],claude[bot],melodic-ai[bot],melodic-standards-sync[bot]) letclaude[bot]andmelodic-ai[bot]satisfy the requiredsecurity-review / security-reviewcheck with no review run, on security-sensitive paths, with no compensating control — and the
general review lane skips them too, so an AI-app-pushed commit would get zero review of any kind.
.github/workflows/claude-review.ymlis deliberately untouched — advisory with no requiredcheck, so its skip list is runner-minute economy, not evidence. It keeps inheriting.
2026-07-31addendum with the ruling, the three rationale strands(compensating controls, latency-is-not-absence, staleness asymmetry), the three rejected
alternatives, the cross-vendor advisory and its accepted counterargument, the composition with
github-iac feat(claude-ops): add plugins skill for plugin-fleet sync/audit/converge #248/feat(disk-hygiene): post-#160 follow-ups — leak-signature hints, standing policy discovery, OS auto-clean awareness #228, and the sequencing.
edit (Channel A) and inherited-default drift at a re-pin (Channel B — the one that actually
fired here and which the old trigger did not name). The re-pin trigger above it now cross-links.
The restored value is byte-identical to the list this caller carried before the re-pin deleted it
(
9ed2956a, #1766) — verified against that commit's diff, which dropped the same line from bothcallers.
runner-policycheck is EXPECTED RED on this PR — do not "fix" it by dropping the inputpolicy.jsonis a managed materialization of the standardsrunner-policycomponent, and thecontract for
claude-security-review.yml@c136b27currently admits onlyrunnerandpaths-file.Restoring
skip-actorsis an input-surface change that must land upstream first. This isintentional fail-closed ordering, not a defect:
skip-actorsto that contract entry'sallowedInputs(open, this is the blocker). It restores parity: the previous approved SHA's contract
(
66073e58) already admitted the input; thec136b27entry dropped it.policy.json.Verified locally: with the local
policy.jsontemporarily patched to the post-sync shape, thevalidator reports
Runner policy passed.; reverted before commit, so this PR touches no managedfile.
The reason a green-check reflex is dangerous here is the residual gap the ADR records: runner-policy
rejects unexpected inputs but cannot require one, so deleting the
skip-actorsline turns thecheck green and silently re-widens a required-check bypass. The caller carries an inline comment
saying exactly that, and the gap is filed upstream as melodic-software/standards#308.
Test plan
actionlint .github/workflows/claude-security-review.yml— clean.zizmor .github/workflows/claude-security-review.yml— "No findings to report."markdownlint-cli2on the ADR — 0 issues.node .github/standards/runner-policy/runner-policy.mjs— fails with exactly one finding,runner-target-contract: the reusable workflow call has inputs absent from its reviewed contract: skip-actors, and nothing else; passes cleanly (Runner policy passed.) once the contract entryis patched to the post-sync shape (simulated locally, then reverted).
(
ci-workflows@c136b27f404dd32ce3873f39a6f3443891d1c16e):skip-actorsis a declaredworkflow_callinput whose default is the four-actor list, and it gates thesecurity-reviewjob, not the workflow — so the check stays name-stable and a skipped actor still reports a
passing context. No wedge risk for the two retained actors.
git grepover.github/workflows/onmainfinds noissue_commenttrigger and no
@claudedispatcher, so neither app can push in this repo today. The change istherefore dormant-by-construction on merge. The activating step is charted but gated:
feat(claude-assistant): org @claude mention-responder lane (reusable workflow, V1 answer/re-review only) ci-workflows#255's V1 is read-only (no Edit/Write, no push) and would not open
the channel; only its V2 (tag-mode fix-and-push, behind that issue's own approval gate) would.
Landing now keeps that gate from silently carrying a security-review-bypass decision it was never
scoped to make.
security-review / security-reviewitself.Related
allowedInputs), then its sync PRmelodic-software/github-iac#228 (open; live forgery test — this PR deliberately does not touch
ADR 0002's "cannot forge it" sentence, which stays feat(disk-hygiene): post-#160 follow-ups — leak-signature hints, standing policy discovery, OS auto-clean awareness #228's to update)
skip-actorsdefault,so the next adopter does not inherit the widened exception
requiredInputs-style contract field, closing the residualgap where deleting a caller's input passes green (also records the parked
claude-security-review-callercomponent's matching hazard when it unparks)🤖 Generated with Claude Code
https://claude.ai/code/session_01FVoZoMYXqf8ZVbQYixPVPW