ci: enforce full pipeline as required checks for PR merge#134
Conversation
- Add Typecheck job to CI (pnpm typecheck: crypto + web) - Restructure CI dependency chain: lint + typecheck -> test -> build - Remove continue-on-error from E2E workflow (was a leftover from Phase 12.3.1/12.4 auth rework) - Update branch protection to require all 7 checks: Lint, Typecheck, Verify API Spec & Client, Test, Build, E2E Tests, lint-pr-title - Previously only "Build" was required, allowing PRs to merge with failing tests or E2E Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 WalkthroughAdds a typecheck job to CI that runs workspace TypeScript checks and wires it into test/build job dependencies; adjusts CI job dependency graph. Removes Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 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 |
tsc --noEmit validates but produces no output, so web's tsc -b couldn't resolve @cipherbox/crypto types. Build crypto first to emit dist/index.d.ts. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.github/workflows/ci.yml (1)
186-188: Nit:lintandtypecheckinneedsare redundant sincetestalready depends on them.This is purely cosmetic — GitHub Actions resolves transitive dependencies — but removing them would reduce maintenance surface if the upstream graph changes. Totally fine to keep for explicitness.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml around lines 186 - 188, The workflow's build job lists redundant dependencies—remove "lint" and "typecheck" from the build job's needs array (the job named build with needs: [...]) so it only depends on the upstream jobs that are not already transitively required (e.g., keep "api-spec" and "test"); update the needs field in the build job to exclude "lint" and "typecheck".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 186-188: The workflow's build job lists redundant
dependencies—remove "lint" and "typecheck" from the build job's needs array (the
job named build with needs: [...]) so it only depends on the upstream jobs that
are not already transitively required (e.g., keep "api-spec" and "test"); update
the needs field in the build job to exclude "lint" and "typecheck".
Summary
pnpm typecheck— runstsc --noEmiton crypto + web packages)continue-on-error: truefrom E2E workflow — was a leftover from Phase 12.3.1/12.4 auth rework, no longer neededstrict: trueso PRs must be up-to-date with main before mergingBefore
Only "Build" was required. PRs could merge with failing tests, failing E2E, and no type checking.
After
Every check must pass before the merge button is enabled.
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
Chores
Tests