ci(actions): add dedicated pull request workflow#66
Conversation
📝 WalkthroughWalkthroughpr validation was moved into a dedicated workflow ( Changes
Sequence Diagram(s)sequenceDiagram
participant PR as pull_request
participant GH as github_actions
participant Runner as runner(ubuntu-latest)
participant Node as node/npm
participant Tests as test-suite
PR->>GH: open/update PR to main
GH->>Runner: schedule validate job (concurrency: pr number)
Runner->>Node: setup node 20.x + npm cache
Runner->>Node: npm ci
Runner->>Node: npm audit / lockfile check
Runner->>Tests: run hygiene → typecheck → lint → unit tests → build
Tests-->>Runner: result (pass/fail)
Runner-->>GH: report status
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes notes and risks
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/pr-ci.yml:
- Around line 29-33: Add the missing npm audit CI step by inserting a new
workflow step after the "Install dependencies" step to run "npm run audit:ci"
(name it something like "Run audit:ci" or "Security audit") so PRs run the same
vulnerability check as the main CI; ensure the step executes the npm script (npm
run audit:ci) and fails the job on any high-severity findings, colocating it
before the "Repository hygiene check" step.
- Around line 23-27: PR CI currently pins actions/setup-node to node-version:
20.x so PR validation only runs on 20.x while ci.yml tests both 20.x and 22.x;
update the PR workflow step that uses actions/setup-node@v4 to run a
node-version matrix (include 20.x and 22.x) or, if intentionally limited, add a
clear comment/README note explaining the gap; locate the setup step referencing
actions/setup-node@v4 and change it to use a matrix variable (node-version) or
add documentation in the workflow or repo README to justify and record this
decision.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 694b790e-135b-4f39-bf55-59d947ea70a6
📒 Files selected for processing (2)
.github/workflows/ci.yml.github/workflows/pr-ci.yml
📜 Review details
🔇 Additional comments (5)
.github/workflows/ci.yml (2)
24-24: cache syntax standardization looks good.the unquoted
npmis valid yaml and consistent across all three jobs now. no functional change.
63-63: lint and codex-compat jobs follow the same pattern.cache config is now consistent across test, lint, and codex-compat jobs.
Also applies to: 83-83
.github/workflows/pr-ci.yml (3)
1-12: solid workflow structure with good security defaults.concurrency group prevents redundant runs on rapid pushes, and
permissions: contents: readfollows least-privilege principle. the fallback togithub.refin the concurrency group handles edge cases wherepull_request.numbermight be unavailable.
41-42: usingnpm testinstead ofnpm run coverageis fine for pr validation.coverage generation adds overhead; running tests without coverage for PRs keeps the feedback loop fast while the full coverage run happens on merge to main.
44-45: build step at the end ensures the pr produces valid artifacts.good practice to catch build failures before merge.
| - name: Setup Node.js | ||
| uses: actions/setup-node@v4 | ||
| with: | ||
| node-version: 20.x | ||
| cache: npm |
There was a problem hiding this comment.
pr validation only runs on node 20.x, but ci.yml tests both 20.x and 22.x.
this means a regression specific to node 22.x could slip through PR validation and only fail after merge to main. consider adding a matrix here too, or at minimum document this gap is intentional.
suggested matrix addition
jobs:
validate:
name: PR Validation
runs-on: ubuntu-latest
+
+ strategy:
+ matrix:
+ node-version: [20.x, 22.x]
steps:
- name: Checkout code
uses: actions/checkout@v4
- name: Setup Node.js
uses: actions/setup-node@v4
with:
- node-version: 20.x
+ node-version: ${{ matrix.node-version }}
cache: npm🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/pr-ci.yml around lines 23 - 27, PR CI currently pins
actions/setup-node to node-version: 20.x so PR validation only runs on 20.x
while ci.yml tests both 20.x and 22.x; update the PR workflow step that uses
actions/setup-node@v4 to run a node-version matrix (include 20.x and 22.x) or,
if intentionally limited, add a clear comment/README note explaining the gap;
locate the setup step referencing actions/setup-node@v4 and change it to use a
matrix variable (node-version) or add documentation in the workflow or repo
README to justify and record this decision.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/pr-ci.yml:
- Around line 35-36: The workflow currently runs
test/lockfile-version-floor.test.ts twice: once in the explicit "Lockfile floor
guard" step and again in the general npm test run; either remove the dedicated
"Lockfile floor guard" step so the guard only runs as part of the full "npm
test" step, or keep the dedicated step and change the general "npm test"
invocation to exclude test/lockfile-version-floor.test.ts (use your test
runner's exclude/ignore flag) so the guard only executes once.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: e0734087-fc11-4c18-abe2-84bab7ef1b13
📒 Files selected for processing (1)
.github/workflows/pr-ci.yml
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Greptile Review
🔇 Additional comments (5)
.github/workflows/pr-ci.yml (5)
1-12: workflow metadata and concurrency setup look solid.concurrency group keyed by pr number with
cancel-in-progress: trueprevents stacking runs on force-pushes. read-only permissions follow least-privilege. good baseline.
23-27: pr validation only runs on node 20.x while ci.yml tests both 20.x and 22.x.this was flagged previously and remains unaddressed. a node 22.x-specific regression could pass pr validation and fail only after merge to main. consider adding the matrix or document this gap as intentional.
32-33: audit:ci step now present - addresses prior review feedback.
.github/workflows/pr-ci.yml:32-33now runs the same security audit as ci.yml, ensuring vulnerable dependencies are caught before merge. good fix.
41-45: typecheck and lint steps are correctly ordered.static analysis runs before tests which is the expected flow. scripts match
package.jsondefinitions.
47-51: test and build steps complete the validation pipeline.running
npm test(vitest) followed by build ensures the pr is both functionally correct and compiles cleanly. no issues here.
| - name: Lockfile floor guard | ||
| run: npm run test -- test/lockfile-version-floor.test.ts |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
lockfile floor guard test runs twice in this workflow.
test/lockfile-version-floor.test.ts runs explicitly here (line 36) and again as part of npm test (line 48). this is a minor inefficiency. if fail-fast is the goal, consider excluding it from the full suite run:
- name: Run tests
- run: npm test
+ run: npm run test -- --exclude test/lockfile-version-floor.test.tsalternatively, accept the duplication since it's fast and keeps the explicit guard visible.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/pr-ci.yml around lines 35 - 36, The workflow currently
runs test/lockfile-version-floor.test.ts twice: once in the explicit "Lockfile
floor guard" step and again in the general npm test run; either remove the
dedicated "Lockfile floor guard" step so the guard only runs as part of the full
"npm test" step, or keep the dedicated step and change the general "npm test"
invocation to exclude test/lockfile-version-floor.test.ts (use your test
runner's exclude/ignore flag) so the guard only executes once.
|
Blocking review comments are addressed; the only remaining bot note is the optional Node 22 parity suggestion. Please merge PR #67 first because it resolves the Hono audit finding that audit:ci now needs. |
1 similar comment
|
Blocking review comments are addressed; the only remaining bot note is the optional Node 22 parity suggestion. Please merge PR #67 first because it resolves the Hono audit finding that audit:ci now needs. |
ci(actions): add dedicated pull request workflow
1 participant
Introduce a lightweight CI run for pull requests while keeping the existing heavier workflow on push to main.
Summary by cubic
Adds a dedicated PR CI workflow for faster feedback while keeping the full CI only on pushes to
main. Adds security parity on PRs and standardizes caching.pr-ci.ymlfor PRs tomainwith concurrency cancel and read-only permissions.audit:ci), lockfile floor guard, hygiene, typecheck, lint, tests, and build.ci.ymlto trigger only on push tomainand normalizedcache: npm.Written for commit 0aae5a0. Summary will update on new commits.
note: greptile review for oc-chatgpt-multi-auth. cite files like
lib/foo.ts:123. confirm regression tests + windows concurrency/token redaction coverage.Greptile Summary
this pr successfully separates pr ci from the heavier push-to-main ci, maintaining clear triggers and caching strategy. the pr workflow correctly includes security audit (
audit:ci), lockfile floor guard, hygiene check, type check, lint, tests, and build on node 20.x — all the gates needed for fast feedback on pull requests.key changes:
ci.ymlnow triggers only on push tomain, eliminating duplicate runspr-ci.ymlintroduces a focusedvalidatejob with essential checksthe lightweight design is intentional: some checks like full dependency audit (
audit:all, non-blocking) and coverage reporting are deferred to the heavier main ci, keeping pr ci fast. this aligns with the stated goal of "lightweight CI run for pull requests."Confidence Score: 5/5
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[git event] --> B{trigger type?} B -->|push to main| C[ci.yml] B -->|pull_request to main| D[pr-ci.yml] C --> C1[test job\nNode 20.x + 22.x matrix] C --> C2[lint job] C --> C3[codex-compat job] C1 --> C1a[clean:repo:check] C1a --> C1b[audit:ci blocking] C1b --> C1c[lockfile-floor guard] C1c --> C1d[audit:all non-blocking] C1d --> C1e[typecheck] C1e --> C1f[npm run coverage] C1f --> C1g[build] C3 --> C3a[codex.test.ts\nhost-codex-prompt.test.ts\nrequest-transformer.test.ts\nfetch-helpers.test.ts] D --> D1[validate job\nNode 20.x only\nconcurrency cancel-in-progress\ncontents: read] D1 --> D1a[audit:ci blocking] D1a --> D1b[lockfile-floor guard] D1b --> D1c[clean:repo:check] D1c --> D1d[typecheck] D1d --> D1e[lint] D1e --> D1f[npm test] D1f --> D1g[build]Last reviewed commit: 0aae5a0