[kyle-sexton]

Summary

Adds a job-level defence-in-depth guard (#443) on both Claude review lanes: when github.actor ends with [bot], the job runs only if the actor is dependabot[bot] or the PR author's author_association is OWNER, MEMBER, or COLLABORATOR. Human actors are unchanged.

Fix

• Extend the existing review / security-review job if: clauses in claude-review.yml and claude-security-review.yml.
• Exempt dependabot[bot] so org Dependabot PRs (which may carry CONTRIBUTOR or NONE) are not regressed.
• Scope the association check to pull_request events (like the draft/fork gates) because dispatch payloads carry no author_association.
• Retry paths inherit the same job-level gate automatically.
• Update SECURITY MODEL / allowed-bots commentary and add contract tests.

Verification

• node --test .github/scripts/claude-lane-bot-association.test.cjs .github/scripts/claude-security-review-fail-closed.test.cjs — pass (25 tests)
• actionlint — not available in this environment

Related

Closes #443

[cursor]

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.

[claude]

Claude finished @kyle-sexton's task in 4m 1s —— View job

---

Code Review Complete

• Gather context (PR diff, CLAUDE.md, changed files)
• Review claude-review.yml bot-association guard
• Review claude-security-review.yml bot-association guard
• Review new/updated contract tests
• Post findings (inline + summary)

No blocking findings. This is a well-scoped, correctly-implemented defence-in-depth guard.

What I checked

• Boolean-logic correctness. The new job if: clause on both lanes (claude-review.yml#L352-L355, claude-security-review.yml#L865-L868) is:
  !endsWith(actor,'[bot]') || actor=='dependabot[bot]' || (event_name!='pull_request' || association-ok).
  I initially suspected the nested (event_name != 'pull_request' || …) group was a no-op that let any bot bypass the gate on non-pull_request events (e.g. workflow_dispatch). Working it through De Morgan's law against the stated intent ("block only when: bot AND not-dependabot AND pull_request event AND association not in {OWNER,MEMBER,COLLABORATOR}") shows the coded expression is exactly the correct negation — not a bug. GH Actions || is flat/left-associative, so the extra parens don't change semantics; they're just for readability, matching the existing draft-gate (claude-review.yml) and fork-gate (claude-security-review.yml) style.
  • I checked why the check is intentionally inert outside pull_request: both files' if: blocks must still let pull_request_target/workflow_run events reach the job so the Reject privileged triggers step (claude-review.yml#L389-L395) can hard-fail them — an unscoped check would let those privileged-trigger misconfigurations skip silently instead. This is the same reasoning already documented for the fork gate, and the new clause correctly reuses it.
  • workflow_dispatch itself already requires the invoking actor to have write access at the platform level, so leaving the association check inert there isn't a new exposure — consistent with the PR's stated rationale.
• Dependabot exemption uses exact equality (==), not substring/contains, so it can't be spoofed by another bot name containing dependabot[bot] as a substring.
• Retry-path coverage: both lanes are single jobs with in-job retry (no separate retry job), confirmed via job listing — the claim that "retry paths inherit the same job-level gate automatically" holds.
• Tests: the new claude-lane-bot-association.test.cjs and the additions to claude-security-review-fail-closed.test.cjs follow the exact same pattern already used throughout that file (regex-match fragments against the extracted job-condition string) — consistent with established convention, not a new gap.
• Docs: SECURITY MODEL commentary in both workflow headers and the inline job comments were updated consistently and accurately describe the new behavior.

Not run

node --test and actionlint could not be executed in this sandbox (Bash commands required interactive approval that wasn't available), so I verified the logic by hand instead of re-running the PR's own stated verification. Manual trace of the boolean expression and cross-reference against the job structure didn't surface any discrepancy with the PR description.

[claude]

Claude finished @kyle-sexton's task in 3m 20s —— View job

---

Security review — claude-lanes bot-actor gating (#443)

• Gather context (PR diff, changed files, CLAUDE.md)
• Review workflow gating logic in claude-review.yml / claude-security-review.yml
• Review new/updated test files
• Check for trust-boundary or bypass issues
• Post findings

Note: the /review:security-review skill invocation failed to load in this environment (tool error with no diagnostic detail), so I performed the equivalent review manually against the diff (git diff origin/main...HEAD) and the two workflow files.

Summary

No security issues found. This is a well-scoped defence-in-depth addition and it correctly follows the two precedents already established in this file (the fork gate and the draft gate).

What I verified

Logic correctness — the new clause added to both job if: conditions:

(!endsWith(github.actor, '[bot]')
    || github.actor == 'dependabot[bot]'
    || (github.event_name != 'pull_request'
        || contains(format(',{0},', 'OWNER,MEMBER,COLLABORATOR'), format(',{0},', github.event.pull_request.author_association))))

correctly implements "allow if not a bot, or is dependabot, or (not a pull_request event, or the PR author's association is trusted)". Parenthesization is correct given && binds tighter than || in GitHub Actions expressions, so this evaluates as a single AND-ed term alongside the existing skip-actors / draft / fork clauses — .github/workflows/claude-review.yml#L342-L349, .github/workflows/claude-security-review.yml#L865-L871.

Why scoping to pull_request is deliberately safe, not a gap — my first read flagged the github.event_name != 'pull_request' escape hatch as a possible bypass for non-PR events (e.g. workflow_dispatch, merge_group). The existing test at claude-security-review-fail-closed.test.cjs#L349-L354 explains why this is intentional for the sibling fork-gate, and the same reasoning applies here: if the bot-actor check were not scoped to pull_request, a bot actor arriving via a privileged event (pull_request_target/workflow_run) would cause the job condition to evaluate false — skipping the job entirely and skipping past the Reject privileged triggers tripwire step, turning a dangerous consumer misconfiguration into a silent pass. Scoping to pull_request means the job still runs on those events and reaches the tripwire, which hard-fails it. Good catch on staying consistent with that pattern.

dependabot[bot] exemption — safe because GitHub enforces a read-only default GITHUB_TOKEN for Dependabot-triggered pull_request runs at the platform level (independent of this workflow's own permissions: block), and pull_request_target is already hard-blocked by the tripwire, so the exemption can't be combined with an elevated token.

Actor/association identity mismatch (design note, not a defect) — for claude-security-review.yml, which also triggers on synchronize, github.actor on that event is whoever pushed the commit, while github.event.pull_request.author_association is the original PR author's association — these can differ (e.g. a bot pushing a follow-up commit to someone else's PR). The SECURITY MODEL comment already documents this as intentional (cursor[bot] pushing to an operator's own branch), and it's a defence-in-depth layer on top of the Claude Code Action's own actor write-access gate, not a standalone control — so this is consistent with the documented threat model rather than a new gap.

No injection risk — github.actor and author_association are used only inside contains()/format() expression functions (pure string comparison evaluated by the runner), not interpolated into shell/run: steps, so there's no command-injection surface from a crafted actor name.

Tests — the new claude-lane-bot-association.test.cjs and the additions to claude-security-review-fail-closed.test.cjs correctly pin the three sub-clauses (bot guard, dependabot exemption, author-association allow-list) against both workflow files, and are picked up automatically by the existing node --test .github/scripts/*.test.cjs step in ci.yml — no CI wiring changes needed. I wasn't able to execute node --test directly in this sandboxed environment (Bash execution requires interactive approval here), so I verified the regex/content match by inspection against the diff rather than by running the suite.

Nothing else in the diff (comment-only SECURITY MODEL updates) changes behavior.
· Branch

[github-actions]

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