ci: migrate to reusable node-ci in .github repo#27
Conversation
Replace inline build/test/lint jobs with a thin caller of `resq-software/.github/.github/workflows/node-ci.yml` pinned to the PR#12 tip SHA (5a72372). Adds a top-level `required` aggregator job to emit the status-check context consumed by the org ruleset `default-branch-baseline` (currently in evaluate mode). Behavior parity: - lint: bun run lint (biome) - typecheck: bun run tsc --noEmit - build: bun run build - test: bun test --coverage Security scanning remains in security.yml (independent cadence). When PR#12 lands on resq-software/.github, update the @sha ref to pin against the merge commit or a semver tag.
|
Note Gemini is unable to generate a review for this pull request due to the file types involved not being currently supported. |
📝 WalkthroughWalkthroughCI workflow refactored to delegate build, test, lint, and typecheck jobs to a reusable workflow from resq-software/.github. Concurrency groups added to cancel in-progress runs. New aggregation job interprets reusable workflow results. License header updated with SPDX identifier and security scanning note. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 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 |
Updates @sha from the feat-branch tip to the merge commit of resq-software/.github#12 (f4b51a620aa1bf89c0bce4f434b36f92ff7d517d). Functionally equivalent — same content — but pins to a ref that now exists on main rather than a closed PR branch.
Previous lockfile drifted from package.json, causing 'bun install --frozen-lockfile' in CI to fail with: error: lockfile had changes, but lockfile is frozen Regenerated locally; no dependency version changes required beyond what package.json already specified.
![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/ci.yml:
- Around line 42-50: The case in the Aggregate step currently treats "skipped"
as an OK result which can let the required-status gate pass without CI running;
update the case that inspects NODE_RESULT (the case "$NODE_RESULT" in block) to
accept only success (and optionally the empty string "") and treat any other
value (including skipped) as an error so the run exits non-zero; adjust the
error message to clearly state the unexpected NODE_RESULT value when failing.
- Around line 22-24: The current concurrency block uses cancel-in-progress: true
which can cancel CI runs on the default branch and prevent subsequent deploy
triggers; update the concurrency configuration so cancel-in-progress is only
enabled for non-default branch runs (e.g., PRs/feature branches) and disabled
for the default branch and tag pushes. Locate the concurrency block (symbols:
concurrency, group, cancel-in-progress) and change the logic to set
cancel-in-progress to false when github.ref is the default branch (e.g.,
refs/heads/main) or a tag, and true otherwise so PRs still cancel but main/tag
pushes do not get cancelled and can trigger workflow_run deploys.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8a895c93-4b8f-4ca5-b214-6c338e2c0d99
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (1)
.github/workflows/ci.yml
| concurrency: | ||
| group: ${{ github.workflow }}-${{ github.ref }} | ||
| cancel-in-progress: true |
There was a problem hiding this comment.
cancel-in-progress: true on main can swallow deploys.
deploy.yml triggers off workflow_run and only runs when conclusion == 'success'. When two pushes land on main in quick succession, this concurrency config cancels the first CI run (conclusion: cancelled, not success), so the first push will not trigger a deploy. Cancellation is desirable on PRs but usually not on the default branch or tags.
🛠️ Suggested guard
concurrency:
group: ${{ github.workflow }}-${{ github.ref }}
- cancel-in-progress: true
+ # Don't cancel in-flight runs on protected refs; deploy.yml listens on workflow_run.
+ cancel-in-progress: ${{ github.ref != 'refs/heads/main' }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/ci.yml around lines 22 - 24, The current concurrency block
uses cancel-in-progress: true which can cancel CI runs on the default branch and
prevent subsequent deploy triggers; update the concurrency configuration so
cancel-in-progress is only enabled for non-default branch runs (e.g.,
PRs/feature branches) and disabled for the default branch and tag pushes. Locate
the concurrency block (symbols: concurrency, group, cancel-in-progress) and
change the logic to set cancel-in-progress to false when github.ref is the
default branch (e.g., refs/heads/main) or a tag, and true otherwise so PRs still
cancel but main/tag pushes do not get cancelled and can trigger workflow_run
deploys.
| - name: Aggregate | ||
| env: | ||
| NODE_RESULT: ${{ needs.node.result }} | ||
| run: | | ||
| if find src -name "*.test.ts" -o -name "*.test.tsx" -o -name "*.spec.ts" -o -name "*.spec.tsx" | grep -q .; then | ||
| bun test --coverage | ||
| else | ||
| echo "No test files found — skipping." | ||
| fi | ||
|
|
||
| lint: | ||
| runs-on: ubuntu-latest | ||
| timeout-minutes: 10 | ||
| steps: | ||
| - uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 | ||
|
|
||
| - uses: oven-sh/setup-bun@0c5077e51419868618aeaa5fe8019c62421857d6 # v2.2.0 | ||
|
|
||
| - run: bun install --frozen-lockfile | ||
|
|
||
| - run: bun run lint | ||
| set -eu | ||
| case "$NODE_RESULT" in | ||
| success|skipped|"") echo "ok: node=$NODE_RESULT" ;; | ||
| *) echo "::error::node reusable returned: $NODE_RESULT"; exit 1 ;; | ||
| esac |
There was a problem hiding this comment.
Don't treat skipped as success for a required-status gate.
required is the aggregator consumed by the default-branch-baseline ruleset, so its success is the branch-protection gate. Today node has no if: and can't be skipped, but accepting skipped as ok is a silent footgun: a future path filter, matrix guard, or upstream change that skips node would make required green without CI actually running. Recommend narrowing to success only (and optionally keep "" defensively).
🛡️ Suggested tightening
case "$NODE_RESULT" in
- success|skipped|"") echo "ok: node=$NODE_RESULT" ;;
- *) echo "::error::node reusable returned: $NODE_RESULT"; exit 1 ;;
+ success) echo "ok: node=$NODE_RESULT" ;;
+ *) echo "::error::node reusable returned: '$NODE_RESULT'"; exit 1 ;;
esac📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - name: Aggregate | |
| env: | |
| NODE_RESULT: ${{ needs.node.result }} | |
| run: | | |
| if find src -name "*.test.ts" -o -name "*.test.tsx" -o -name "*.spec.ts" -o -name "*.spec.tsx" | grep -q .; then | |
| bun test --coverage | |
| else | |
| echo "No test files found — skipping." | |
| fi | |
| lint: | |
| runs-on: ubuntu-latest | |
| timeout-minutes: 10 | |
| steps: | |
| - uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 | |
| - uses: oven-sh/setup-bun@0c5077e51419868618aeaa5fe8019c62421857d6 # v2.2.0 | |
| - run: bun install --frozen-lockfile | |
| - run: bun run lint | |
| set -eu | |
| case "$NODE_RESULT" in | |
| success|skipped|"") echo "ok: node=$NODE_RESULT" ;; | |
| *) echo "::error::node reusable returned: $NODE_RESULT"; exit 1 ;; | |
| esac | |
| - name: Aggregate | |
| env: | |
| NODE_RESULT: ${{ needs.node.result }} | |
| run: | | |
| set -eu | |
| case "$NODE_RESULT" in | |
| success) echo "ok: node=$NODE_RESULT" ;; | |
| *) echo "::error::node reusable returned: '$NODE_RESULT'"; exit 1 ;; | |
| esac |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/ci.yml around lines 42 - 50, The case in the Aggregate
step currently treats "skipped" as an OK result which can let the
required-status gate pass without CI running; update the case that inspects
NODE_RESULT (the case "$NODE_RESULT" in block) to accept only success (and
optionally the empty string "") and treat any other value (including skipped) as
an error so the run exits non-zero; adjust the error message to clearly state
the unexpected NODE_RESULT value when failing.
1 participant
Summary
Replaces inline build/test/lint jobs in
.github/workflows/ci.ymlwith a thin caller of the org-wide reusable workflowresq-software/.github/.github/workflows/node-ci.yml.Adds a top-level
requiredaggregator job that emits therequiredstatus-check context consumed by the org rulesetdefault-branch-baseline(id 15191038, currently in evaluate mode — does not block merges yet).Blocked on: resq-software/.github#12 (defines
node-ci.yml). This PR pins to the feat branch tip SHA5a72372. After #12 lands, update the ref to the merge commit or a semver tag.Behavior parity
bun run lintin its own joblint-cmdinput tonode-ci.ymlbun run tsc --noEmitinbuildjobtypecheck-cmdinputbun run buildbuild-cmdinputbun test --coveragewith conditional file checktest-cmd: bun test --coverage(bun test exits 0 on no matches)Not changed
security.ymlkeeps its independent schedule; it still callssecurity-scan.yml@maindirectly.deploy.yml,labeler.yml, etc.) untouched.Test plan
node / Node CI+requiredstatus contexts green.requiredstatus check appears in branch protection selector on settings → rulesets.Summary by CodeRabbit