refactor(ci): give the review skip-actors list one owner - #3466
Merged
Conversation
The literal skip-actors list was restated at five sites across both review workflows and both evidence guards, and the restatement drifted: 2b4d8ab added cursor[bot] to both workflow lines and left verify-security-review-evidence.sh's default behind, so the guard judged cursor[bot] PRs in scope while the lane skipped them. .github/claude-skip-actors now carries the one statement of ADR 0002's skip-actor exception (same repo-owned-data shape as .github/claude-skip-actors' sibling .github/claude-security-paths), and scripts/read-skip-actors.sh owns the one parse, fail-closed on a missing file, an empty active set, or a malformed entry. The claude-security-review caller reads the PR's BASE copy in a prep job (mirroring the reusable's paths-file discipline) and hands it to the lane's skip-actors input across needs, with the evidence job failing closed when that read did not succeed; the claude-review caller reads it in a step; both evidence guards default from it when SKIP_ACTORS is not in the environment. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QkMx972GYEQ3vCb98pLw8m
Two adversarial-review findings on the skip-actors single-sourcing: - claude-review.yml read the list inside echo's argument, so under bash -e a parser failure returned echo's status, emitted `list=`, and left the step green — red arrived only because the guard's own default re-read the file. The read is now a plain assignment first, matching the security caller's fail-closed form. - claude-security-review.yml's prep job swallowed every git error behind one 2>/dev/null, so a failed base fetch silently fell back to the head copy. Each branch is now explicit: an unresolvable base ref hard-fails the step; only the list file absent at a resolvable base (the adopting PR's pre-adoption base) falls back to the head copy, with a ::notice. The job comment now states the real property — base-reading protects the list's CONTENT, while the workflow, parser, and composite remain head-controlled on pull_request, bounded by the reusable's allowed_bots — instead of implying full head-tamper protection. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QkMx972GYEQ3vCb98pLw8m
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No linked issue
Summary
The review skip-actors bot list was restated at five sites and had already drifted. Route-lane finding from the
/coupling:reducepass merged in #3457, split out of #3464 for the reason above.scripts/verify-security-review-evidence.sh's default was missingcursor[bot], which2b4d8abfadded to both workflow lines only — so the two guards silently disagreed about that actor on any invocation that does not set the env.Fix
One owned data file,
.github/claude-skip-actors, and one fail-closed parser,scripts/read-skip-actors.sh, following the pattern.github/claude-security-pathsalready establishes. Both workflows and bothverify-*.shdefaults now resolve through it.The reusable's
with: skip-actorscannot read a file, so a prep job reads the list and passes it acrossneeds, mirroring the reusable's ownpaths-filediscipline.Verification
Parser behavior, every shape — it can never succeed with a partial or empty list:
Suite 8/8. Mutation-tested: removing the empty-set check → 1 failure; removing the malformed-entry check → 2 failures.
Fail direction traced at every consumer, which is what matters here. An independent adversarial verifier confirmed that a malformed or empty list can never cause a security review to be skipped: the security lane's plain assignment aborts the prep job under
bash -e;security-reviewthen reportsskipped; andsecurity-review-evidencestill runs underalways()and exits 1 on its first step (needs.skip-actors.result != 'success') — correctly ordered before the guard, which would otherwise exit 0 onLANE_RESULT=skipped. Even if an empty list did reach a guard,",,"cannot match*",actor,"*, so the review runs rather than being skipped.Behavior fix shown end to end: a
cursor[bot]actor under the old drifted default → guard exit 1; under the file-sourced default → skip-listed, exit 0.Two verifier findings were fixed before this PR was opened:
claude-review.yml's read step usedecho "list=$(...)", which returnsecho's status and so did not fail the step on a parser error. It is now a plain assignment, matching the security caller's fail-closed form.git show ... 2>/dev/nullswallowed every error, silently falling back to the head copy on a failed base fetch. An unresolvable base ref is now a hard error; an absent file at a resolvable base is an explicit notice. The job comment now states the real property rather than a stronger one: reading the base copy protects the list's content, but the workflow, parser, andcheckout-with-basecomposite are all head-controlled onpull_request— not a regression (on main the literal lived in the head-controlled workflow file), and bounded by the reusable'sallowed_bots.Gates:
actionlinton both workflows,shellcheck,check-shell-portability.sh,zizmor(0 findings),typos,editorconfig-checker, runner-policy,affected-tests.sh --run("All 3 selected suites passed") — all pass.Related
Split from #3464. Follow-up to #3457.
Generated by Claude Code