docs(ci): say why the skip-actors line must survive a re-pin - #2069
Conversation
The comment explained where the exception lives and how #1766 widened it, but not why deleting the line is unsafe even when its value matches the upstream default of the moment. That is the property a re-pin reviewer needs: the four actors are ADR 0002's 2026-08-04 ratified baseline, and inheriting re-delegates that decision to whatever ci-workflows ships next. Records the asymmetry the ADR's own rationale turns on — a stale explicit list fails closed, a stale inherited default fails open — and names standards#308 as the residual gap, since runner-policy can reject an unexpected input but cannot require an expected one. Comment-only. The actor list is unchanged and stays at the ratified four. Salvaged from PR #1845, closed as superseded by the 2026-08-04 operator decision; this was the part of that branch worth keeping. Refs #1767 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 73b7b4b709
ℹ️ 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".
|
Claude finished @kyle-sexton's task in 1m 56s —— View job Review complete ✅
SummaryThis PR adds 17 comment lines above
No correctness, design, or maintainability issues found. The new comment is accurate, non-duplicative of the existing header comment (lines 38–43 explain why explicit; the new lines explain why deletion-at-parity is unsafe and the asymmetric failure-direction rationale — genuinely new information), and consistent with the file's dense ADR-referencing comment style already established at lines 3–16. I have no inline findings to raise — the diff is comment-only and every claim within it is traceable and correct. |
Review caught a factual error in the previous commit. It described a stale
explicit list as costing "an actor gets a review it may not have needed —
visible, cheap." That is not what happens.
For an actor the reusable's `allowed_bots` does not permit, the action throws
and this lane's fail-closed mapping turns it into a RED required check no push
can fix — a merge block, not an extra review. ADR 0002 records this directly
("skip-actors and the action's allowed_bots are different levers with different
outcomes"), and it is why removing a name from this list is never a one-line
change: un-skipping an actor for real means widening `allowed_bots` upstream.
The asymmetry the comment exists to convey survives and is sharper stated
correctly — loud-and-blocking versus silent-and-permissive, rather than a cheap
extra review versus a silent skip.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Claude has reviewed this PR 2 times. The lane skips further automatic reviews after 5; deleting this comment resets the count. |
|
Claude finished @kyle-sexton's task in 1m 58s —— View job Code Review
Scope note: this PR is a pure comment addition to a GitHub Actions workflow file — no Codex's finding (P2, "Correct the stale-list failure mode") is already resolved. It was raised against commit Independent verification performed:
No new findings. The added comment is accurate against the ADR, consistent with the actual commit history (#1766, #1845, #1766's re-pin), and stays within a comment-only diff with no behavior change. One very minor, sub-nit-level observation not worth a blocking comment: |
No linked issue — this closes nothing. #1767 stays open deliberately: its residual gap is
structural (
runner-policycannot require an input), and a comment guards against a carefulreviewer, 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-actorsin.github/workflows/claude-security-review.yml. No value changes; the actor list stays at theratified 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-workflowsships next, andevery actor in an inherited default satisfies this repo's REQUIRED
security-review / security-reviewcheck 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_botsdoes not permit, the actionthrows 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_botssection records that removing an actor fromskip-actorsalonedoes 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_botsupstream, soremoving a name here is never a one-line change.
The comment also names standards#308 at the point of use:
runner-policyrejects inputs outside thereviewed 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
actionlintclean on the changed file.skip-actorsvalue is byte-identical before and after — confirmed by reading the parsed lineback, not by eye.
Related
requiredInputsgap is not closed by a comment.docs/adr/0002-default-on-ai-review-advisory-with-earned-promotion.md— the 2026-08-04 addendumthat ratified four actors.
requiredInputs-style contract field that would make thisstructurally enforced rather than comment-enforced.