Repository navigation
feat(core): add CORRECT006 — flag orphan $effect that throws effect_orphan at runtime - #233
Merged
Merged
Conversation
|
Warning Review limit reached
Next review available in: 15 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. 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 Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (10)
📝 WalkthroughWalkthroughAdds CORRECT006 orphan ChangesCORRECT006 orphan effect analysis
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Collector
participant parseComponentFacts
participant ComponentFacts
participant correct006OrphanEffect
Collector->>parseComponentFacts: parse .svelte, .svelte.ts, or .svelte.js source
parseComponentFacts->>ComponentFacts: populate orphanEffects and suppressions
Collector-->>correct006OrphanEffect: provide ComponentFacts
correct006OrphanEffect->>correct006OrphanEffect: emit critical findings for unsuppressed facts
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/core/test/correctness-rules.test.ts (1)
32-32: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the duplicated
comphelper to a shared test fixture.The
comp(orComponentFactsfactory) helper is duplicated across 6 different test files. As demonstrated in this PR, adding a single new fact type (orphanEffects) required updating 6 identical object literals.To reduce maintenance overhead and adhere to the DRY principle, consider extracting this helper into a shared test utility (e.g.,
packages/core/test/fixtures/component.ts) and importing it where needed.
packages/core/test/correctness-rules.test.ts#L32-L32: Extractcompto a shared utility and use it here.packages/core/test/architecture-rules.test.ts#L25-L25: Replace with the imported shared helper.packages/core/test/bundle-rules.test.ts#L28-L28: Replace with the imported shared helper.packages/core/test/component-rule.test.ts#L25-L25: Replace with the imported shared helper.packages/core/test/security-rules.test.ts#L25-L25: Replace with the imported shared helper.packages/cli/test/suppression-e2e.test.ts#L29-L29: Reuse the helper if feasible across package boundaries, or maintain a single CLI-specific copy.🤖 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 `@packages/core/test/correctness-rules.test.ts` at line 32, Extract the duplicated comp/ComponentFacts factory into a shared test fixture and replace the local helpers with imports: packages/core/test/correctness-rules.test.ts:32-32, packages/core/test/architecture-rules.test.ts:25-25, packages/core/test/bundle-rules.test.ts:28-28, packages/core/test/component-rule.test.ts:25-25, and packages/core/test/security-rules.test.ts:25-25. Reuse that fixture from packages/cli/test/suppression-e2e.test.ts:29-29 if cross-package imports are supported; otherwise retain one CLI-specific copy. Ensure the shared factory includes orphanEffects and all existing ComponentFacts fields.
🤖 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.
Nitpick comments:
In `@packages/core/test/correctness-rules.test.ts`:
- Line 32: Extract the duplicated comp/ComponentFacts factory into a shared test
fixture and replace the local helpers with imports:
packages/core/test/correctness-rules.test.ts:32-32,
packages/core/test/architecture-rules.test.ts:25-25,
packages/core/test/bundle-rules.test.ts:28-28,
packages/core/test/component-rule.test.ts:25-25, and
packages/core/test/security-rules.test.ts:25-25. Reuse that fixture from
packages/cli/test/suppression-e2e.test.ts:29-29 if cross-package imports are
supported; otherwise retain one CLI-specific copy. Ensure the shared factory
includes orphanEffects and all existing ComponentFacts fields.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e36ea575-3bd5-41d8-a027-4945a1689394
⛔ Files ignored due to path filters (1)
packages/action/dist/index.jsis excluded by!**/dist/**
📒 Files selected for processing (22)
.changeset/correct006-orphan-effect.mddocs/src/content/docs/guides/cli.mddocs/src/content/docs/ja/guides/cli.mddocs/src/content/docs/ja/rules/correct006.mddocs/src/content/docs/rules/correct006.mddocs/superpowers/plans/2026-07-15-correct006-orphan-effect.mddocs/superpowers/specs/2026-07-15-correct006-orphan-effect-design.mdpackages/cli/test/malformed-svelte.test.tspackages/cli/test/suppression-e2e.test.tspackages/core/src/component-collect.tspackages/core/src/component-parse.tspackages/core/src/component.tspackages/core/src/index.tspackages/core/src/rules/correctness/correct006-orphan-effect.tspackages/core/src/rules/index.tspackages/core/test/architecture-rules.test.tspackages/core/test/bundle-rules.test.tspackages/core/test/component-collect.test.tspackages/core/test/component-parse.test.tspackages/core/test/component-rule.test.tspackages/core/test/correctness-rules.test.tspackages/core/test/security-rules.test.ts
…ns and see through TS constructor overloads Pattern 2 (module-scope new of a same-file effectful class) walked the whole program looking for ClassDeclaration/ClassExpression nodes and NewExpression callees anywhere, so a block-scoped class shadowing an imported name of the same name, or a class expression's own (inner-only) name, could produce a false-positive critical finding. Restrict both halves to direct top-level statements (unwrapping export/export default only) — matches the design spec's own wording for pattern 2. Also: the constructor lookup picked the first MethodDefinition named "constructor", which for a TS-overloaded constructor is a bodiless signature — the class was never marked effectful. Require a body.
…opping analysis parseModuleFacts wraps a .svelte.ts/.svelte.js source in a <script lang="ts"> tag so the Svelte script parser can produce the ESTree program. A source containing the literal string "</script>" (e.g. in a string or comment) terminated that wrapper tag early and threw, silently losing both orphan- effect detection and inline suppressions for the whole file. Neutralise any literal `</script` occurrence with a same-length placeholder before wrapping — string contents don't affect fact extraction, and the same-length swap preserves every offset/line number. Suppressions keep being collected from the original (non-neutralised) source.
…RECT006 orphanEffects is typed as required on ComponentFacts, but a facts object built by an older/external constructor could omit it — applies() would throw and take the whole runRules Promise.all down with it. Default to an empty array in both applies and bad.
collectComponentFacts issued three separate rt.glob calls (.svelte,
.svelte.ts, .svelte.js) plus a Set-based dedupe of the concatenated
results. Both production Runtime.glob implementations (cli node runtime,
vite provider) delegate to tinyglobby, which supports picomatch-style
brace patterns — a single 'src/**/*.svelte{,.ts,.js}' call replaces all
three globs (one directory traversal) and the dedupe is no longer needed.
Also teaches the cli test helper's mock glob-to-regex converter to expand
brace groups (including the empty alternative), matching real
tinyglobby/picomatch behaviour, so collect-component-facts.test.ts keeps
passing against the new pattern.
type/start/end/loc/range was hand-listed as an ignored-key check in four places (walkEstree, walkScoped, bodyReadsReactive's IGNORED_KEYS, walkEvalScope). Extract one WALK_IGNORED_KEYS constant and reuse it in all four. Pure refactor, no behavior change.
…ent the conditional-effect caveat The +layout.svelte snippet imported '$lib/store.svelte.ts', which fails under a default SvelteKit tsconfig (TS5097) — use the Svelte-docs convention '$lib/store.svelte.js' instead. Also document that a conditionally-guarded constructor effect is still flagged, since the guard can't be evaluated statically (same policy as top-level if branches); suggest the inline suppression for an intentional guard. Updated in both en/ja.
Owner
Author
Post-review hardening (8 commits)A high-effort adversarial review (multi-agent, every finding independently verified with reproduction) surfaced 8 issues; all are addressed: Detection correctness
Robustness / perf / cleanup
Docs
New/updated tests: +8 (427 core tests total). Root 🤖 Generated with Claude Code |
This was referenced Jul 16, 2026
This was referenced Aug 2, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds CORRECT006, the first
criticalcorrectness rule: it flags$effect/$effect.precalls that are guaranteed to run outside component initialisation and therefore throw Svelte's runtimeeffect_orphanerror — a compile-clean failure that typically surfaces as a production 500. Verified against svelte 5.56.4: the compiler passes all detected patterns through to runtime, and eslint-plugin-svelte has no equivalent rule.Detected (conservative, no false positives by construction — the walk never crosses a function boundary):
$effectin a.svelte.ts/.svelte.jsrunes module or a.svelte<script module>blocknew X()of a same-file class whose constructor creates a bare$effect(not wrapped in$effect.root) — the shared-state-manager trapThis also introduces the first analysis of
.svelte.ts/.svelte.jsfiles: they are folded into the existingComponentFactspipeline via a<script lang="ts">wrap parse (zero new dependencies), so the CLI, vite plugin, and MCP all pick the rule up automatically. Module files populate onlyorphanEffects+suppressions(loc: 0), so ARCH001/PERF009/010 stay silent on them.Design doc:
docs/superpowers/specs/2026-07-15-correct006-orphan-effect-design.mdPlan:
docs/superpowers/plans/2026-07-15-correct006-orphan-effect.mdChanges
packages/core:OrphanEffectFact+ eval-scope walker (walkEvalScope/collectOrphanEffects) incomponent-parse.ts;.svelte.ts/.svelte.jsparse branch (parseModuleFacts); collector globs extended;correct006-orphan-effect.tsrule registered in all four sites@svelte-vitals/core,svelte-vitals,@svelte-vitals/vite,@svelte-vitals/mcpTest plan
</script>-literal fail-safe, rule mapping incl. suppression e2e)pnpm build/pnpm typecheck/pnpm test(1,279 tests) /pnpm lintall greennew,satisfies, getters) — no false positives found🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
$effectand$effect.precalls that can cause runtime failures..svelte.tsand.svelte.jsrunes modules, including relevant module-scope class instantiations.Documentation