feat(ci): auto-approve low-risk fix and refactor PRs - #3544
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (8)
🚧 Files skipped from review as they are similar to previous changes (6)
📝 WalkthroughWalkthroughThe PR adds centralized auto-approval rules, CODEOWNERS matching, manual workflow dispatch, draft detection, stale approval dismissal, sticky comments, and Node.js test execution. ChangesPR auto-approval
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested reviewers: ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (1)
.github/scripts/pr-auto-approval/rules.js (1)
43-45: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winThe bot check matches human logins that contain "bot".
/bot/imatches logins such asAbbottorbotanist. Those authors never receive auto-approval. Anchor the check to the GitHub bot suffix, and keep a small allowlist of known bot accounts if needed.♻️ Proposed refinement
- if (/bot/i.test(authorLogin)) { + if (/\[bot\]$/i.test(authorLogin)) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/scripts/pr-auto-approval/rules.js around lines 43 - 45, Update the bot detection condition in the author-validation rule to match GitHub bot logins by their bot suffix rather than any occurrence of “bot”; preserve an explicit allowlist for known bot accounts if the surrounding logic requires it, while allowing human logins such as “Abbott” and “botanist” to proceed.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/CODEOWNERS:
- Around line 16-19: Update .github/CODEOWNERS to add ownership patterns
covering application-root and repository-root .env* files, alongside the
existing environment-directory rule, so these files require the designated human
owner. Add policy tests verifying both application-root files such as
.env.production and repository-root files such as .env.example are matched.
- Around line 8-14: Update the CODEOWNERS rule ordering so test-file exemption
patterns appear after all protected-path ownership rules, ensuring billing test
files remain owner-less under last-match semantics. Preserve the existing
protected-path owners and align the ordering with the expectation in
codeowners.test.js.
In @.github/scripts/pr-auto-approval/rules.js:
- Around line 5-6: Update TEST_FILE_PATTERN to recognize JavaScript test files
alongside TypeScript files, and match repository-relative test directories
whether they are at the root or nested. Ensure paths such as test/foo.spec.ts
and .github/scripts/pr-auto-approval/rules.test.js are classified as test files
without requiring a leading slash.
In @.github/workflows/github-actions-lint.yml:
- Around line 18-19: Update the workflow before the “Run workflow script tests”
step to add the pinned actions/setup-node action configured with
node-version-file: package.json, ensuring the tests run with the
repository-declared Node.js version.
In @.github/workflows/pr-auto-approval.yml:
- Around line 17-27: Add a GitHub Actions concurrency group for the
check-whether-to-approve job keyed by the associated pull request, and disable
cancellation of in-progress runs so concurrent evaluations for the same PR are
serialized and the latest evaluation completes.
- Around line 29-33: Update the sparse-checkout pattern under the checkout
action to use .github instead of ./.github, matching the repository-root
cone-mode pattern used by the other workflows.
- Around line 151-153: Update the review_id assignment to use gh api with
--paginate --slurp, then pipe the combined page results to jq so filtering for
the last approved github-actions[bot] review occurs across one collected array
and produces a single ID value.
---
Nitpick comments:
In @.github/scripts/pr-auto-approval/rules.js:
- Around line 43-45: Update the bot detection condition in the author-validation
rule to match GitHub bot logins by their bot suffix rather than any occurrence
of “bot”; preserve an explicit allowlist for known bot accounts if the
surrounding logic requires it, while allowing human logins such as “Abbott” and
“botanist” to proceed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9c29558f-54fb-45c3-aeac-9ac1101df2f6
📒 Files selected for processing (11)
.github/CODEOWNERS.github/actions/associated-pr/action.yml.github/scripts/pr-auto-approval/codeowners.js.github/scripts/pr-auto-approval/codeowners.test.js.github/scripts/pr-auto-approval/rules.js.github/scripts/pr-auto-approval/rules.test.js.github/scripts/pr-auto-approval/sticky-comment.js.github/scripts/pr-auto-approval/sticky-comment.test.js.github/workflows/github-actions-lint.yml.github/workflows/pr-auto-approval.ymlknip.json
There was a problem hiding this comment.
Beyond the inline finding, this run also examined three other candidate issues and ruled them out: the CODEOWNERS test/tests exclusion carving a hole in critical-path ownership (checked — critical paths like billing/auth/tx-signer don't have test-dir carve-outs that matter in practice), workflow_dispatch bypassing the CI-success gate for real approve/dismiss actions (checked — dispatch is a manual, permissioned replay harness, not attacker-reachable), and provider-proxy's src being left unowned by the new CODEOWNERS (checked — consistent with the PR's stated scope, not a regression it introduces).
Extended reasoning...
This PR rewrites the CI auto-approval bot itself (CODEOWNERS ownership map, decision engine, and review-dismissal/sticky-comment logic) — a security/process-sensitive system that grants merge approval without human review. It is XL-sized and, per the author's own description, requires console-team review regardless of bugs found. Given the inline nit already found this run, I'm only adding a short note on other candidates that were checked and ruled out, not re-litigating the verdict.
- escape filenames in the sticky comment so a crafted PR path cannot break out of the markdown code span and inject a link - clear the stale sticky comment when a blocked PR later becomes skipped/ineligible without a dismissal - read review pages with --paginate --slurp so the dismissal lookup works when a PR has more than one page of reviews - serialize auto-approval runs per PR via a concurrency group - run the workflow script tests on .github/scripts/** changes and pin the repo Node version for them - own .env* files wherever they live, not only under env/ - count JavaScript test files and repo-root test dirs as test evidence - normalize the sparse-checkout pattern to .github
There was a problem hiding this comment.
Beyond the two inline nits from this run, I also checked whether the CODEOWNERS test-file exemption (*.spec.ts, **/test/, etc.) could let a file with a test-like name but real production logic bypass both code-owner review and the rules.js code-owned-paths blocker (since both consume the same CODEOWNERS parse). That exemption predates this PR — it already excluded spec files from ownership before this change — and exploiting it still requires an already-trusted experienced-contributor author, so I did not flag it as a new issue introduced by this diff.
Extended reasoning...
This is the narrow ruled-out-note exception: no new review body issues beyond the inline nits already posted. I independently re-derived the CODEOWNERS test-exemption bypass concern flagged by finder agents (a file like apps/api/src/billing/x.spec.ts clears ownership via the pre-existing '*.spec.ts' unowned pattern, and the same CODEOWNERS parse is reused by rules.js's code-owned-paths blocker) and confirmed it is not new to this PR: the spec-file exclusion from CODEOWNERS existed prior to this change, and reaching it still requires the same trusted-author/size/type gates as every other finding on this PR. Recording this here so it is not re-explored from scratch in a later pass.
- concurrency group now keys on workflow_run.pull_requests[0].number (falling back to the dispatch input) so all pushes to a PR serialize, not just re-runs of one commit - SOURCE_CODE_PATTERN matches .mjs/.cjs for parity with the widened TEST_FILE_PATTERN, so a chore PR editing eslint configs is skipped rather than auto-approved
There was a problem hiding this comment.
No new bugs found in this pass — the issues I previously flagged (stale-comment leak, sparse-checkout, pagination truncation, concurrency grouping, .mjs/.cjs chore-scope gap) are all fixed in 73945a1 and cf3a1a2. This PR is still worth a human look before merge: it rewrites the auto-approval gating logic itself (CODEOWNERS risk-map + evaluate()/rules engine), so a subtle bug here directly weakens repo-wide review coverage, and per the PR's own notes .github/** is code-owned and requires console-team review anyway.
Beyond the inline findings from earlier rounds, this pass also checked and ruled out: the workflow_dispatch PR-resolution path omitting a merged_by fallback (not applicable — dispatch always resolves a live, unmerged-context PR via pulls.get, unlike the commit-association lookup that needs the fallback), the concurrency group collapsing for fork-originated workflow_run events (moot — fork PRs are rejected by findIneligibilityReason before any mutating step runs, so there's nothing to race), and the fork check being bypassed when the source fork is deleted (the fork/same-repo detection happens off webhook payload metadata at dispatch time, not by querying the fork itself, so deletion doesn't change the outcome).
Extended reasoning...
Overview
PR overhauls the CI auto-approval workflow (pr-auto-approval.yml) and its supporting engine (.github/scripts/pr-auto-approval/{rules,codeowners,sticky-comment}.js), and rewrites .github/CODEOWNERS to serve as the risk map that the engine consults. It also adds a workflow_dispatch dry-run replay path and wires script tests into github-actions-lint.yml.
Security risks
This code is a review-bypass control: a logic bug here can let an unreviewed PR merge into main. The engine reads all PR-controlled values (title, labels, filenames) through env: rather than interpolating into script bodies, avoiding the classic github-script injection pattern. Earlier rounds (mine and CodeRabbit's) found and got fixed real issues in this area — a markdown/link-injection vector in the sticky-comment filenames, a concurrency-group key that didn't actually serialize per-PR, and pagination truncation on the reviews lookup — all resolved in 73945a1 and cf3a1a2 with regression tests.
Level of scrutiny
High — this is exactly the kind of change the guidelines call out for deferral: it's XL-sized, touches CI gating/security-sensitive logic, and the PR description itself acknowledges .github/** is code-owned and needs console-team sign-off. Automated review clearing zero new bugs doesn't lower the bar here given the blast radius of a mistake (silently widening what auto-merges).
Other factors
53 unit tests across the three new modules, actionlint/zizmor clean, and dry-run replays against six real historical PRs matching expected decisions per the PR description. Two full rounds of review (CodeRabbit + this system) already surfaced and got fixed several concrete bugs, which is a good signal the harness is working, but also underscores this is intricate enough to warrant a human's final pass rather than resting on a clean automated run.
There was a problem hiding this comment.
thought: it's still too broad. My idea was to identify critical flows - they will always require human, even if it's a fix or refactor. refactor of stripe related logic IMO must have human review.
There was a problem hiding this comment.
suggestion: would be nice to delegate this completely to github and enforce it by github, not by our script
There was a problem hiding this comment.
what is the purpose of this script?
| core.setOutput("head_repo", pr.head.repo?.full_name || ""); | ||
| core.setOutput("labels", JSON.stringify((pr.labels || []).map(l => l.name))); | ||
| core.setOutput("closed", pr.closed_at ? "true" : "false"); | ||
| core.setOutput("draft", pr.draft ? "true" : "false"); |
There was a problem hiding this comment.
question: could you please explain why associated-pr cannot provide this outputs?
|
CI related things cannot be feat -> it's |
|
My idea was to categorize changes by tiers and process them in this way:
instead of code rabbit or additionally we could use claude because coderabbit can be rate limited |
| /packages/*/src/ @akash-network/console | ||
| /config @akash-network/console | ||
|
|
||
| # Critical business flows |
There was a problem hiding this comment.
suggestion: managed wallet API should also be a part of business critical flow
3 participants
Why
The auto-approval workflow only ever approved
chore/test/docsPRs, and even those approvals were mostly decorative: theprotected3ruleset requires a code-owner review andCODEOWNERSblanket-owned/apps/*/src/and/packages/*/src/, so any code-touching PR needed a human regardless. This implements the mechanically-checkable subset of the team's Slack rules for AI-driven tasks (size cap, task categorization, risk map, migrations/public API need humans) as a two-layer system:fix(with regression-test evidence) andrefactorPRs up tosize: Mfromexperienced-contributorauthors auto-approve.What
Behavior changes
Loosening
fixPRs (must add/update at least one test file) andrefactorPRs ≤size: Mnow auto-approve and become mergeable without a human on un-owned paths.feat/XL/untrusted PRs there now need any-collaborator approval instead of specifically a code owner (accepted trade-off; in practice reviewers are the same team).test:scope check additionally accepts.test./.e2e.suffixes andplaywright.config.*.Intentional tightening
apps/*/drizzle/,apps/*/env/, and allDockerfile*become code-owned. Today they are un-owned and chore-approvable, i.e. zero-human mergeable — this closes that gap.dismiss_stale_reviews_on_push: false, so the action cleans up after itself), with a sticky comment explaining why.Mechanics
.github/scripts/pr-auto-approval/(CommonJS, zero-dependency, tested withnode:test): a gitignore-style CODEOWNERS parser (the action parses.github/CODEOWNERSas the single source of truth for its owned-path blocker — no duplicated path list), a pureevaluate()decision engine with 4 outcomes (ineligible/blocked/skipped/approved), and a sticky-comment upserter.workflow_dispatchwithpr-number+dry-run(default true) as a replay harness; the decision JSON is always logged.associated-praction gains adraftoutput.github-actions-lint.ymlruns the script tests;knip.jsonregisters the scripts as entries.env:, never${{ }}in script bodies.Deviations & notes for reviewers
*.test.jsnaming deviates from the repo's*.spec.tsconvention — it's thenode --testdiscovery default and these files are outside vitest/tsc scope (.github/**is also outside the codecov patch gate).f.includes("/apps/")) never matched becauselistFilespaths have no leading slash; the port fixes this latent bug (/^(apps|packages)\//), so chores touching code are now correctly skipped.codeowners.test.jspins the real file's policy semantics (billing owned, components un-owned, spec files un-owned, drizzle owned).workflow_runexecutes the main-branch engine, so a PR can never approve itself with modified rules; this PR touches.github/**(owned) and requires console-team review.Verification
node --test ".github/scripts/**/*.test.js"),actionlintclean,zizmorfindings identical to the pre-change baseline,npx knip --workspace .unchanged vs main.workflow_dispatchdry-runs replayed real historical PRs; every decision matched the file evidence:approved: bug fix within approved scope that includes test changesapproved: all changes are related to documentationapps/api/src/billing/+apps/tx-signer/blocked: code-owned-paths.helm/filesblocked: code-owned-pathsineligible: missing required size label (XS, S, or M)approved: refactor within approved scopePost-merge follow-up: run a scratch
fix:PR without a test to empirically confirm comment/dismissal permissions (pull-requests: writeshould suffice; fallback is addingissues: write), then monitor the first organic approvals for a week and tune CODEOWNERS as needed.Summary by CodeRabbit
New Features
Tests
Chores