Skip to content

feat(runner-policy): admit skip-actors on the claude-security-review contract - #307

Merged
kyle-sexton merged 3 commits into
mainfrom
fix/security-review-caller-skip-actors-contract
Jul 31, 2026
Merged

feat(runner-policy): admit skip-actors on the claude-security-review contract#307
kyle-sexton merged 3 commits into
mainfrom
fix/security-review-caller-skip-actors-contract

Conversation

@kyle-sexton

@kyle-sexton kyle-sexton commented Jul 31, 2026

Copy link
Copy Markdown
Contributor

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.

🤖 Generated with Claude Code

https://claude.ai/code/session_01FVoZoMYXqf8ZVbQYixPVPW

…contract

ADR 0002 (claude-code-plugins) re-deliberated the skip-actor exception and
rejected the reusable workflow's inherited default for the security lane: it
covers `claude[bot]` and `melodic-ai[bot]`, which carry none of the
compensating controls that justified the two ratified actors, so an
AI-app-pushed commit would satisfy the required security check with no review
run of any kind. The caller must therefore pass an explicit narrow list.

That divergence is only expressible if the contract admits the input. Add
`skip-actors` to the `allowedInputs` of the
`claude-security-review.yml@c136b27` entry, matching the `claude-review.yml`
contracts that already admit it.

Permissive widening only: no consumer starts passing the input, and the
validator continues to reject any input outside this list.

Refs: melodic-software/claude-code-plugins#1767

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FVoZoMYXqf8ZVbQYixPVPW
@claude

claude Bot commented Jul 31, 2026

Copy link
Copy Markdown

Claude finished @kyle-sexton's task in 2m 17s —— View job


Code review

  • Gather context (diff, CLAUDE.md/REVIEW.md, policy.json schema, tests)
  • Understand the request (code review, not implementation)
  • Review the change for correctness and maintainability
  • Post findings

This PR is a one-line data change to components/runner-policy/policy.json: adding skip-actors to allowedInputs for the claude-security-review.yml@c136b27 contract entry. Per REVIEW.md, this repo runs a security lane, so I've scoped this review to correctness, test coverage, and maintainability and left the actor-list/security judgment itself to that lane.

Correctness of the mechanism

  • allowedInputs is coerced to a Set at load time (runner-policy.mjs:243) and sorted before being used in reviewed-contract comparisons (runner-policy.mjs:1543), so array order is not semantically significant — confirmed against the schema too (policy.schema.json's uniqueStringArray has no order constraint). The change itself is a correct, minimal widening of the allowlist with no other contract fields (allowedSecrets, allowedCallerPermissions) touched.

🟡 Nit — test coverage gap for the new admission
No test in runner-policy.test.mjs exercises a caller actually passing skip-actors to this specific claude-security-review.yml@c136b27 contract entry. The only existing reference to this SHA in the test file is PATHS_FILE_LANE_SHA (runner-policy.test.mjs:31), used solely in the selector-allowlist tests (:2938, :2962) — unrelated to this contract's allowedInputs. Compare this to the analogous case for claude-review.yml, which has a dedicated positive test, "production Claude review contract accepts its supported skip-actors input" (runner-policy.test.mjs:6307), that constructs a caller workflow with skip-actors: and asserts a clean audit.

Since BASE_POLICY is loaded directly from the real policy.json (:74), the new entry is exercised passively by generic/negative tests (e.g., "rejects every undeclared input"), but nothing positively confirms the exact behavior this PR exists to enable: a caller pinned to claude-security-review.yml@c136b27 passing skip-actors and getting a clean audit. Given the downstream consumer (claude-code-plugins#1767) depends on exactly this working, a mirrored positive test here would close the loop before the sync PR lands. Not blocking — the diff matches its stated test plan and the fail-closed sequencing described in the PR body already catches a real breakage downstream — but worth adding for the same reason claude-review.yml's admission got one.

🟡 Nit — array ordering inconsistent with sibling entries
Every other allowedInputs entry that admits skip-actors places it immediately after runner (e.g. ["runner", "skip-actors", "paths"] at policy.json:410, :491, :504). This entry appends it last: ["runner", "paths-file", "skip-actors"] (policy.json:530). Functionally inert (order doesn't affect validation, per above), but this file is explicitly human-reviewed per-entry, and matching the established convention would make future diffs against sibling pins easier to eyeball.

Everything else checks out: the diff is scoped exactly as described, doesn't touch allowedSecrets/allowedCallerPermissions, has no autoApproved block (consistent with "human-reviewed by design"), and the residual requiredInputs gap is already called out by the author as a follow-up rather than silently left.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: d55d12bd2c

ℹ️ 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".

Comment thread components/runner-policy/policy.json
@github-actions

Copy link
Copy Markdown

Claude has reviewed this PR 1 time. The lane skips further automatic reviews after 5; deleting this comment resets the count.

kyle-sexton and others added 2 commits July 31, 2026 00:08
… list

The `c136b27f` review note still recorded the security caller as able to pass
only `runner` and `paths-file`, so the contract record contradicted the
`allowedInputs` widening in the same component and a reviewer consulting it
would have read `skip-actors` as denied.

`skip-actors` is one of the reusable's 8 declared inputs at that SHA
(`runner`, `paths`, `paths-file`, `prompt`, `claude-args`,
`exclude-comments-by-actor`, `skip-actors`, `retry-delay-seconds`), so the
"(of 8)" denominator is unchanged and the set stays narrower than the
declared surface.

Refs: melodic-software/claude-code-plugins#1767

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013dsTvnGqMJeLTZe4DpGGVQ
The README's contract-surface paragraph for the v0.9.1 lane revision still said
the security caller may pass only `runner` and `paths-file`, which the previous
commit made false. Note the third admitted input, and why: it restores parity
with the `66073e58` contract, which already admitted it, and a consumer whose
ruleset makes the security check required needs it to narrow the reusable's
four-actor default.

Refs: melodic-software/claude-code-plugins#1767

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FVoZoMYXqf8ZVbQYixPVPW
@kyle-sexton
kyle-sexton merged commit 1588d0e into main Jul 31, 2026
44 checks passed
@kyle-sexton
kyle-sexton deleted the fix/security-review-caller-skip-actors-contract branch July 31, 2026 13:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant