fix(core): isolate rule failures so one crashing rule no longer kills the whole run - #464
Conversation
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 51 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughRule execution now isolates synchronous and asynchronous failures. Failed rules produce warnings, remain outside successful results, and can be disabled for Health-score weighting. The CLI and Vite integrations consume the new failure data, with core, scoring, and end-to-end tests covering the behavior. ChangesRule Failure Isolation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant analyzeProject
participant runRules
participant withFailedRulesOff
participant HealthScore
analyzeProject->>runRules: execute configured rules
runRules-->>analyzeProject: return results and failedRules
analyzeProject->>withFailedRulesOff: disable failed rule IDs
withFailedRulesOff-->>HealthScore: provide scoring configuration
HealthScore-->>analyzeProject: calculate Health score
analyzeProject-->>analyzeProject: append failed-rule warnings
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
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 |
|
@coderabbitai review |
|
|
@coderabbitai review |
|
|
@coderabbitai review |
|
|
@coderabbitai review |
|
|
@coderabbitai review |
|
|
@coderabbitai review |
|
|
@coderabbitai review |
|
|
@coderabbitai review |
|
|
@coderabbitai review |
|
|
@coderabbitai review |
|
|
@coderabbitai review |
|
|
@coderabbitai review |
|
|
@coderabbitai review |
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@packages/core/test/engine.test.ts`:
- Around line 153-154: Remove the audit-history comment immediately preceding
the relevant test in engine.test.ts, including the 2608-CORE-06 reference and
CORE-07 note; leave the test title and implementation unchanged.
In `@packages/vite/src/analyze.ts`:
- Line 122: Update the documentation for AnalyzeResult.warnings to describe both
config-file issues and non-fatal rule-analysis failures added by the failedRules
loop. Remove or revise the claim that warnings is empty when no config file
exists, since rule failures can still populate it.
- Around line 118-122: Update the failedRules handling in the analyze flow to
derive a non-mutating scoreConfig withFailedRulesOff(config, failedRules.map((f)
=> f.id)) when failures exist. Use scoreConfig instead of config for
computeScore, formatConsoleReport, and formatJsonReport, while preserving config
for the remaining analysis behavior.
🪄 Autofix
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 2e6c3733-fe09-4e8d-b390-2ef589656c67
📒 Files selected for processing (12)
.changeset/rule-failure-isolation.mdpackages/cli/src/index.tspackages/cli/test/rule-failure-isolation.test.tspackages/core/src/config-apply.tspackages/core/src/engine.tspackages/core/src/index.tspackages/core/src/rules/correctness/orphan-effect.tspackages/core/test/config-apply.test.tspackages/core/test/engine.test.tspackages/core/test/score.test.tspackages/vite/src/analyze.tspackages/vite/src/hooks/handle.ts
1 participant
Summary
Previously, a single rule throwing during
check()crashed the entire analysis (exit 2, no results). Now each rule runs inside its own try/catch:runRulesreturnsfailedRules: { id, message }[]alongsideresults/examined.'off'drop mechanism (withFailedRulesOffinconfig-apply.ts), so it leaves the Health denominator instead of silently counting as a pass — measured cell: a crashing rule shifts Health 83 → 80 in the fixture project because its inventory weight is dropped, not zero-filled.svelte-vitals: rule <id> failed and was skipped: <first line>), using the same channel as skipped-file warnings. The vite plugin surfaces failures through its existing warnings array.Declared behavior change: a run containing a crashing rule previously died with exit 2 and no output; it now completes with real results for every other rule, plus a stderr warning naming the failed rule.
Verification
packages/core/test/engine.test.ts(crash → other rules' results intact, failedRules populated, Health denominator excludes the crashed rule).pnpm -r test(core 1423 / cli 1151 / vite 214),pnpm lint,pnpm -r typecheckall pass.Changeset:
@svelte-vitals/core+svelte-vitalspatch.🤖 Generated with Claude Code
Summary by CodeRabbit