Skip to content

Add AI code review workflow with Dependabot skip - #186

Merged
circlecube merged 2 commits into
mainfrom
fix/ai-code-review-skip-dependabot
Aug 3, 2026
Merged

Add AI code review workflow with Dependabot skip#186
circlecube merged 2 commits into
mainfrom
fix/ai-code-review-skip-dependabot

Conversation

@circlecube

Copy link
Copy Markdown
Member

Summary

  • Add the shared AI code review workflow
  • Skip the workflow on Dependabot PRs, since GitHub withholds org secrets from Dependabot-context runs

Test plan

  • Verify workflow YAML is valid
  • Confirm AI code review runs on normal PRs
  • Confirm Dependabot PRs skip the review job (job shows as skipped, not failed)

@github-actions

github-actions Bot commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

AI code review

✅ Strengths

  • Centralized, reusable workflow adoption with clear inputs and secrets mapping; keeps logic DRY across repos.
  • Sensible guards: skips drafts and fork PRs to avoid secret exposure; concurrency configured to prevent duplicate runs.
  • Scoped token permissions at the job level (contents: read, pull-requests: write, checks: write) instead of broad defaults — good least-privilege posture.
  • tests/wpunit updates set an editor user context for hooks registration, reducing flakiness and improving test determinism.

⚠️ Suggestions (non-blocking)

  • Pin the reusable workflow to a commit SHA rather than @main to reduce supply-chain risk; set prompts_ref to the same SHA for consistency.
  • Consider adding issues: write to permissions if the reusable workflow posts regular PR conversation comments (issue comments). Some implementations require it; otherwise comment updates can 403.
  • In the tests, consider wrapping wp_set_current_user changes in a try/finally to guarantee cleanup even if assertions fail.

❌ Issues (blocking / critical)

  • Dependabot skip guard uses github.actor. On Dependabot-authored PRs, if a maintainer pushes or retriggers, actor may not be dependabot[bot], but secrets are still withheld for the PR. This can cause hard failures instead of the intended skip. Use github.event.pull_request.user.login == 'dependabot[bot]' (or an equivalent condition) to reliably skip based on PR author, not actor.

Follow-up vs. prior feedback

  • ⚠️ The prior approval calls out a Dependabot skip guard. While present, basing the skip on actor can miss some Dependabot PR event paths. Using the PR author login is more robust and aligns with the stated goal of avoiding secret-withholding failures.

Verdict

❌ Verdict: Request changes — please switch the Dependabot skip to check the PR author (not the actor), and consider pinning the reusable workflow to a SHA. After that, this looks good to merge.

@arunshenoy99 arunshenoy99 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM — adds the shared AI code review workflow with a Dependabot skip guard to avoid secret-withholding failures.

`admin_enqueue_scripts` and `block_editor_settings_all` hooks are only
registered inside `if ( Permissions::is_editor() )`. Without a logged-in
editor the constructor skips them, so `has_action`/`has_filter` returned
false in the test environment. Create an editor user and call
`wp_set_current_user()` before instantiating ChatEditor so the guard
passes and the hooks are registered as expected.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@github-actions

Copy link
Copy Markdown
Contributor

Coverage: Base 44.74% → PR 47.85% (must not decrease).
✅ No decrease.

Code Coverage

project coverage report 47.85% @ 79a5ab6

@circlecube
circlecube requested review from a team and arunshenoy99 July 31, 2026 22:29
@circlecube

Copy link
Copy Markdown
Member Author

Fixed the failing wpunit test so we can merge

@circlecube
circlecube merged commit c7f25c9 into main Aug 3, 2026
15 of 16 checks passed
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.

3 participants