feat(core): report how many places each declaration examined - #380
Merged
Merged
Conversation
runRules now returns { results, examined } instead of a bare array: the
engine hands each rule a recordExamined sink via RuleContext and keys
whatever a rule writes to it by rule id, so a caller can never silently
drop the counts by forgetting to build one itself. buildJsonReport gains
a trailing examined argument and a top-level `examined` field, included
only when non-empty so existing reports stay byte-identical.
The CLI and the Vite build-mode analyzer thread the counts through to
their JSON report; the dev-server hooks build no JSON report and drop
them, noted inline. Updated the one existing direct runRules() caller in
core's own test suite to the new return shape.
architecture/reserved-name-placement now calls ctx.recordExamined with a per-declaration count of directories judged (permitted or rejected), keyed by the same label the aggregated diagnostic already uses. Empty-value declarations and override-only declarations stay uncounted, matching what the diagnostic classifies. Also documents recordExamined's silent last-write-wins contract, and adds a CLI-level fixture pinning that --diff scoping narrows results but not the examined count.
…e-placement Only the name-in-no-map and empty-value exits actually key on the directory's name; inert, root-level and excluded key on the directory itself. The prior comment claimed all five did.
…or examined counts Two of the five early exits that gate the reserved-name-placement examined count (excluded, root-level) had no test that would fail if the increment were moved above them, and the isMentionedAnywhere guard had no test distinguishing it from the already-pinned sourceFiles===undefined guard. Verified each new test against a hoisted-increment/hoisted-call mutation before restoring the correct code. Also drops a redundant assertion in the CLI --diff test that re-checked the same examined value applyScope never receives, and corrects the comment that oversold what it was proving.
The increment was keyed on the globally resolved labels while every early exit and the placement check used the per-directory resolved maps, so an overrides layer that replaces a name's value credited the global declaration with directories its own glob never reached — one report saying a declaration judged a place and matched no directory at once. A value repeating a glob multiplied the count for the same reason: the label array was pushed once per glob and splitNames does not dedupe.
Both trailing parameters of buildJsonReport are optional, so deleting the field from the report or the argument from either report-producing caller compiled and left every suite green. Verified by mutation: each deletion now fails a test.
A rule that counts while declaring nothing reports an empty entry, which five places said could not happen. The changeset named three changed exported shapes where AnalyzeResult makes four. The design's zero table listed an exit that cannot produce a zero, and the plan carried two instructions execution proved wrong.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe change adds per-rule, per-declaration examined counts to rule execution and JSON reports. It updates the reserved-name-placement rule, CLI and Vite pipelines, public contracts, tests, reporter documentation, and release metadata. ChangesExamined counts reporting
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant analyzeProject
participant runRules
participant reserved-name-placement
participant buildJsonReport
analyzeProject->>runRules: execute rules
runRules->>reserved-name-placement: provide recordExamined callback
reserved-name-placement->>runRules: return declaration counts
runRules->>analyzeProject: return results and examined
analyzeProject->>buildJsonReport: pass results and examined
buildJsonReport->>analyzeProject: serialize JSON report
Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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.
Actionable comments posted: 1
🤖 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 `@docs/src/content/docs/guides/`(reporting)/reporters.md:
- Around line 139-149: Update the English reporter guide’s description of the
top-level examined field to state that it is omitted when no rule reports
counts, while preserving the existing distinction between absent rule entries
and empty entries. Apply the equivalent Japanese statement in
docs/src/content/docs/ja/guides/(reporting)/reporters.md at lines 111-111 to
keep both reporter guides synchronized; both sites require direct changes.
🪄 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: 509b2d14-2927-4fa9-8bb2-db4077103fbc
📒 Files selected for processing (27)
.changeset/examined-counts.mddocs/src/content/docs/guides/(reporting)/reporters.mddocs/src/content/docs/ja/guides/(reporting)/reporters.mddocs/superpowers/plans/2026-08-07-examined-counts.mddocs/superpowers/specs/2026-08-07-examined-counts-design.mdpackages/cli/src/index.tspackages/cli/test/analyze-project.test.tspackages/cli/test/fixtures/reserved-name-placement-project/package.jsonpackages/cli/test/fixtures/reserved-name-placement-project/src/app.htmlpackages/cli/test/fixtures/reserved-name-placement-project/src/lib/Card/Card.sveltepackages/cli/test/fixtures/reserved-name-placement-project/src/lib/Card/parts/a.sveltepackages/cli/test/fixtures/reserved-name-placement-project/src/lib/legacy/parts/c.sveltepackages/cli/test/fixtures/reserved-name-placement-project/src/lib/other/parts/b.sveltepackages/cli/test/fixtures/reserved-name-placement-project/src/routes/+page.sveltepackages/cli/test/fixtures/reserved-name-placement-project/svelte-vitals.config.mjspackages/cli/test/run.test.tspackages/core/src/engine.tspackages/core/src/reporter/json.tspackages/core/src/rule.tspackages/core/src/rules/architecture/reserved-name-placement.tspackages/core/test/engine.test.tspackages/core/test/json-report.test.tspackages/core/test/reserved-name-placement.test.tspackages/core/test/seo001.test.tspackages/vite/src/analyze.tspackages/vite/src/hooks/handle.tspackages/vite/test/analyze.test.ts
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.
Why
Verifying a real project meant planting a deliberate violation to see whether anything fired.
architecture/reserved-name-placementis glob-configured and emits no pass results, so a compliant project produces zero routes, zero findings and zero diagnostics. Confirming that0meant "the tree complies" rather than "nothing was checked" required adding a violating directory, observing the finding, and deleting it again. Nothing in the output could answer the question.The charter's inverse-precision gate names this exactly —
— and records the fix as its own unbuilt spec. This is that spec, one level finer: per declaration, not per rule. One configuration of this rule carries eight declarations, and "the rule examined 137 places" does not answer "did
partssee 28 or 0?".The gap got wider in 0.42.0, not narrower. Before it, one of the rule's diagnostics fired on a correct declaration — and that false note was the only evidence the rule had run. Fixing it left a compliant project with a completely empty result. The observability was accidental, and correcting the diagnostic removed it.
More diagnostics cannot answer this. Reporting a declaration that examined nothing is precisely the case 0.42.0 deliberately made silent: a convention document declaring every permitted position, including ones nothing occupies yet, is correct, and telling its author to "remove the declaration" is advice to delete a check they will want. What a reader needs is information, not a verdict.
What
A top-level
examinedmap in the JSON report — rule id, then declaration label, then how many places that declaration judged.The engine owns the sink.
runRulesbuilds it and returns{ results, examined }, rather than each of the three call sites threading one. A caller that forgot would drop the counts silently — the exact failure this feature removes, and a shape this repository hit twice recently in option forwarding.Not on
rules[id], and the reporters guide already said why: "The counts describe the report, not the tree."findings/passeddescribe what survived reporting; this describes what the analysis examined and is deliberately unfiltered by--diff,--baselineand suppressions. Two scopes behind sibling keys with nothing marking the difference is one field carrying two meanings — the shape behind two defects fixed earlier this week. A top-level map makes the difference structural, next toinventories, which is there for the same reason. The guide's sentence is now scoped torules.The labels are the diagnostic's own strings, verbatim. The count is what makes a silent declaration legible beside the diagnostic that describes a broken one; different names for the same declaration would defeat that.
Three states, deliberately: no entry (the rule doesn't count, or returned early), an empty entry (it counts and nothing is declared), and an entry containing
0(a declaration judged nothing).Zero means the declaration judged nothing, and nothing more
An earlier draft of the design claimed zero meant "live, reachable and currently unoccupied". Review falsified that by execution with two supported configurations in which zero appears while occupying directories exist: a sibling map's empty value ungoverns the name everywhere, and an
overrides-suppliedexcludeskips the directories while the diagnostic classifier consults only the globalexclude— producing a zero with no diagnostic at all.The rule reaches its judging phase past five early exits, each a separate reason a count can be zero. The design enumerates them; the report states a number and stops. The useful direction is sound and is the one documented: a non-zero count is what distinguishes "complies" from "nothing was checked".
Verification
core1295,cli838,vite207 pass;tsc --noEmitclean in all three; lint and format clean across 969 files; docs gates 27;floor-smoke8/8.The whole-branch review ran 13 mutations, seven behavioural probes and an end-to-end run of the built CLI; the fix-wave re-review added four more mutations proving the new tests load-bearing, and re-ran all five zero-causes and four counting scenarios.
The interesting part: the feature's only visible surface was pinned by nothing
The final review deleted the single line that puts
examinedinto the report and ran everything: 2333 tests passed with the field gone from every report. Deleting the argument at either report-producing caller left those suites green too. Both trailing parameters ofbuildJsonReportare optional, so the type system did not catch it either — "the engine owns it, so omission is a type error" held at therunRulesboundary but not at the reporter boundary, which is the one a user's report actually crosses.The design's testing list required exactly this ("the two report-producing callers carry the counts, asserted by enumerating the call sites, not by sampling"). The plan's own self-review downgraded it to a grep — a one-off manual command, not an assertion. A feature built to stop a silent absence shipped its own silent absence, one review away from merging.
Two counting defects were found the same way. A duplicated glob inside one value multiplied the count —
parts: 'src/lib/** | src/routes/** | src/lib/**'would report 56 where 28 were judged, with no diagnostic, because the duplicate deduped into the alternatives map and counted as used. And anoverrideslayer that replaces a global declaration's value inflated that global declaration's count, so a single report claimed the same label had both judged a place and matched no directory — breaking the label join the design calls its whole justification. The increment now fires only where the resolved value judges.Earlier in execution, two of the five early exits turned out to be pinned by nothing: the increment could move above the exclusion check or the root check with the suite green — and the exclusion exit is the one the design singles out in Deliberately not solved as a deliberate, documented contract.
The recurring shape is worth naming. Four of the plan's own instructions were wrong and each was caught by executing them: a caller grep scoped to
src/that missed a test file (in a plan whose point is enumerating callers), a fixture asserting0for a directory the spec says is judged, a mutation row its own named test could not catch, and the self-review above. Every one surfaced because the process runs each new test against the unchanged code first and works a mutation table where each row names the single test that must fail.Design:
docs/superpowers/specs/2026-08-07-examined-counts-design.md. Plan:docs/superpowers/plans/2026-08-07-examined-counts.md, corrected in place each time execution proved a step wrong.🤖 Generated with Claude Code
Summary by CodeRabbit