Repository navigation
feat(core): tell "found nothing" from "never ran" in the JSON report - #358
Conversation
… json.rules
Three Minor findings from the feat/json-rule-evidence whole-branch review:
- reporters.md (en/ja): the exclusion list and "counts describe the
report" note now distinguish top-level `rules: { id: 'off' }` (removes
the rule from the map) from an `overrides`-disabled rule (still ran,
still appears as { findings: 0, passed: 0 }) — confirmed end to end
against the CLI's json reporter before writing the wording.
- app-shell.ts's formatHtmlReport and vite/ui/snapshot.ts's buildSnapshot
both build a JsonReport on buildJsonReport's 3-arg form, so their
`rules` map is results-only, unlike the json reporter's selection-based
map. Threading only the HTML report (a one-step change) would still
leave the field meaning two things across payloads, and the dashboard
side would cascade through plugin.ts/installUiMiddleware/buildSnapshot
into a live layer (hooks/handle.ts) that selects rules independently in
a separate process. Left both as-is; recorded a design-doc bullet and a
one-line comment at each call site.
- Added a CLI-level test pinning that a rule disabled via config `rules`
(not just --category) is absent from json.rules, alongside a rule that
stays on staying present — the design's config-off half of the
selectRules discrimination was previously only exercised by
json-report.test.ts, which never calls selectRules itself.
|
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 (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThe JSON reporter now includes a top-level ChangesJSON Rule Evidence
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant CLI as CLI analyzeProject
participant Vite as Vite analyze
participant Reporter as formatJsonReport
participant Builder as buildJsonReport
CLI->>Reporter: pass analysis.ruleIds
Vite->>Reporter: pass selected rule IDs
Reporter->>Builder: pass results, config, meta, ruleIds
Builder->>Builder: seed rules and count findings and passed results
Builder-->>Reporter: return JsonReport with rules
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
docs/superpowers/plans/2026-08-03-json-rule-evidence.md (1)
408-429: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRecord the shipped semver decision in the historical plan.
The plan specifies
patch, but.changeset/json-rule-evidence.mdshipsminorreleases for all three packages.Do not rewrite the historical steps. Add a dated supersession note that explains the decision and links to the shipped changeset.
Based on learnings, plans under
docs/superpowers/plans/must preserve historical wording and record shipped divergences with a dated, scoped supersession note.🤖 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 `@docs/superpowers/plans/2026-08-03-json-rule-evidence.md` around lines 408 - 429, Add a dated, scoped supersession note near the changeset step in the historical plan, preserving all existing wording. State that the planned patch releases were superseded by the shipped minor releases for all three packages, and link to `.changeset/json-rule-evidence.md` as the authoritative shipped decision.Source: Learnings
🤖 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/superpowers/plans/2026-08-03-json-rule-evidence.md`:
- Around line 229-233: Update Step 5 in the seeding verification plan to remove
the invalid expectation that git diff on json.ts is empty after restoring the
loop. Instead, instruct the agent to compare the restored file with a copy saved
before deletion, or verify that the for (const id of ruleIds ?? []) seed loop is
present, while preserving the expected test failures when the loop is removed.
In `@docs/superpowers/specs/2026-08-03-json-rule-evidence-design.md`:
- Around line 93-95: Update the `formatHtmlReport` source reference in the
documentation to `packages/core/src/reporter/html.ts`; keep `renderAppShell`
associated with `packages/core/src/reporter/app-shell.ts`.
In `@packages/core/src/reporter/json.ts`:
- Around line 22-23: Update the RuleEvidence documentation in
packages/core/src/reporter/json.ts (lines 22-23) to state that zero-count
entries prove selection only when ruleIds is provided; otherwise, compatibility
fallback entries are derived from results and missing keys do not prove
non-selection. Update
docs/superpowers/specs/2026-08-03-json-rule-evidence-design.md (lines 32-35) to
qualify the rules-presence table accordingly and explicitly state that
override-disabled rules remain represented.
---
Nitpick comments:
In `@docs/superpowers/plans/2026-08-03-json-rule-evidence.md`:
- Around line 408-429: Add a dated, scoped supersession note near the changeset
step in the historical plan, preserving all existing wording. State that the
planned patch releases were superseded by the shipped minor releases for all
three packages, and link to `.changeset/json-rule-evidence.md` as the
authoritative shipped decision.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 252d0358-c131-4fba-a095-460ef26b3382
📒 Files selected for processing (18)
.changeset/json-rule-evidence.mddocs/src/content/docs/guides/(reporting)/reporters.mddocs/src/content/docs/ja/guides/(reporting)/reporters.mddocs/superpowers/plans/2026-08-03-json-rule-evidence.mddocs/superpowers/specs/2026-08-03-json-rule-evidence-design.mdpackages/cli/src/index.tspackages/cli/test/run-suppressions.test.tspackages/cli/test/run.test.tspackages/core/src/index.tspackages/core/src/reporter/app-shell.tspackages/core/src/reporter/json.tspackages/core/test/html-report.test.tspackages/core/test/json-report.test.tspackages/vite/src/analyze.tspackages/vite/src/ui/snapshot.tspackages/vite/test/analyze.test.tspackages/vite/test/app-shell-static.test.tspackages/vite/test/ui-dashboard.test.ts
The type comment and the design table both stated presence proves selection unconditionally. That holds only when the caller supplies the rule ids; the compatibility fallback seeds from results, where absence means 'produced nothing'. A rule disabled through an overrides entry also stays present, since selectRules reads only top-level config.rules. Also drops an impossible verification step from the plan: it asked for an empty git diff after restoring a mutated line, while the task's own changes are still uncommitted at that point.
Why
--reporter jsoncould not answer "did this rule run?"issuesis filtered to penalized results, so a rule that found nothing contributes no entry.summarycounts severities project-wide, sopassedcannot be attributed to a rule. A rule reporting zero therefore had two indistinguishable meanings — every declaration matched and passed, or nothing matched at all — and zero is the output nobody thinks to question.Not hypothetical. A field test configured
architecture/unit-entry-file, saw nothing, and could not tell whether the tree conformed or the globs were dead. Proving the rule had executed required planting a deliberately non-conforming unit; the same probe was needed for two sibling rules.The console reporter never had this gap — it lists every passing result under
Passed (N). The gap was specific to the JSON channel, which is what the field test used and what CI uses.What
JsonReportgains a top-levelrulesmap:Presence is the answer; the counts are detail. An entry with
findings: 0ran and reported nothing; a rule missing from the map was not selected.That only works because the map is seeded from the ids of the rules that ran, passed as an optional fourth argument. Seeding from results alone would leave both cases empty — the original bug in a new place.
The distinction that made this harder than it looks
The list is not what
selectRulesreturns. In the CLI,--categorynarrows the set after selection:Passing
selectedwould report rules excluded by--categoryas having run — precisely the confusion this change exists to remove. The Vite plugin has no--categoryequivalent, so thereselectRules's output is what ran. The two channels are wired asymmetrically on purpose, andAnalyzeResultgainedruleIdsbecause the CLI computes the set inanalyzeProjectand formats the report inrun.Design decisions worth stating, so they are not re-litigated
summary. That type is shared with the console reporter, the markdown reporter, the CLI and the Vite plugin; a per-rule map would grow a type four consumers read and none of them want.findingsis deliberately redundant withroutes[].issues[]+siteIssues[]. Included so "did it run and find nothing" is a local question — forcing a full scan to answer the second half would defeat the change.idandseverity, so that grouping is derivable locally — unlikepassed.--difffiltering run before the report is built, so a rule whose findings were all suppressed showsfindings: 0and stays present. Pinned by a test, and documented, because it is the surprising half.overridesentry still appears, sinceselectRulesreads only top-levelconfig.rules. Verified by running the CLI both ways, and now stated in the guide — the earlier draft claimed presence always means the rule ran, which was wrong for that one path.Verification
core1156,cli805,vite206 tests pass; typecheck clean in all three after rebuilding core; lint and format clean.Both load-bearing mechanisms were confirmed by mutation, not assumed: deleting the seed loop fails 5 tests across the three packages, and swapping
rules.mapforselected.mapfails exactly the--categorytest. An end-to-end CLI run shows 45 default-on rules present with0/0, Σfindingsequals the report's issue count and Σpassedequalssummary.passedunder bothtreatDynamicAsmodes.Known limitation, recorded rather than fixed
The HTML report's embedded snapshot and the dev dashboard's
/data.jsoncallbuildJsonReporton the three-argument form, so theirrulesmap is results-only — presence there does not imply selection. Nothing renders it today. Threading the list would cascade through three signatures on the dashboard path and collide with a separately-computed rule selection in the live hook, and doing only the easy half achieves none of the payoff, so both call sites carry a comment and the design doc records the difference.Design:
docs/superpowers/specs/2026-08-03-json-rule-evidence-design.md. Plan:docs/superpowers/plans/2026-08-03-json-rule-evidence.md.🤖 Generated with Claude Code
Summary by CodeRabbit