Repository navigation
refactor(rules): give the rule layer one entry point (#84) - #194
Merged
Merged
Conversation
Re-checked #84 against main at 044719c. Still reproducing, and the seam it reports widened from two rule-evaluation call sites to four since it was filed (#168, #183, #187, #191). Its own prediction — that another rule type would have to guess which side of the seam it belonged on — has played out three times. Rewrote the issue body: the fix it originally proposed (a synchronous evaluateRules taking the inventory) no longer fits the async registry and plugin phases, and its "nobody reads the half-built report yet" caveat has expired — src/plugins/runner.ts:59 does, through public plugin API. Plan 039 splits AnalysisFacts from AggregatedReport, gives the rule layer a single runRules() entry point, feeds plugins their inputs instead of a partially-assembled report, and constructs AggregatedReport exactly once. Internal only; acceptance test is a zero-diff `pnpm run test:output`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Rule evaluation happened in four places — package rules inside aggregateReports, six families in evaluateRules, registry rules and plugins in runPipeline — and the results were stitched onto an already-constructed AggregatedReport by reassigning a field three times. - Split AnalysisFacts (what the run observed) from AggregatedReport (facts plus verdicts). aggregateReports returns facts only and no longer evaluates rules. - Add runRules() in src/rules/run.ts: the one place that knows the rule families exist, running package -> evaluator -> registry in one explicitly ordered list. Registry progress goes out through callbacks so the rule layer does not import ora. - runPlugins takes facts and violations as separate arguments. buildInventoryView no longer reaches into a report that is still being assembled — the promise that PluginInventoryView.violations is complete is now kept by the signature rather than by call ordering. - runPipeline constructs AggregatedReport exactly once, at the end. No field is reassigned anywhere. printScanResults typed its parameter as ReturnType<typeof aggregateReports>, which silently retargeted a renderer at the narrowed type; it now names AggregatedReport directly, which is what it always meant. No behaviour change: all 30 output-review cases match main byte for byte. Emission order is preserved exactly — it is now chosen in one commented array rather than inherited from whichever call site ran first. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
|
Contributor
Output Review0 of 30 case(s) changed · 0 invariant breach(es) · all 30 cases Reference: Output is unchanged. Nothing to review. |
Contributor
Coverage Report
File Coverage
|
|||||||||||||||||||||||||||||||||||||||||||||
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.
Closes #84.
What
Rule evaluation happened in four places, and the results were stitched onto an already-constructed
AggregatedReportby reassigning a field three times:aggregateReportsno-packages,require-packagesevaluateRulesmax-file-size, script, package-field, engine-version, codeownersevaluateRegistryRulesno-outdated-packages,no-deprecated-packagesrunPluginsThis gives the rule layer one entry point and builds the report once.
AnalysisFactssplit fromAggregatedReport.aggregateReportsreturns what the run observed;AggregatedReport extends AnalysisFactsadds the verdicts. Package rules move out of aggregation.runRules()(src/rules/run.ts) — the only place that knows the rule families exist. Package → evaluator → registry, in one explicitly commented array. Registry progress leaves through callbacks, so the rule layer doesn't importora.runPluginstakesfactsandviolationsas separate arguments.buildInventoryViewno longer reaches into a report that is still being assembled.runPipelineconstructsAggregatedReportexactly once, at the end. Nothing reassigns a field on it.Why now
#84 predicted this would bite "before another rule type gets added and has to guess which side of the seam it belongs on." Three have been added since it was filed (#168, #183, #187), each making that call unaided — the seam widened from two sites to four rather than closing.
The issue's original "nobody reads the half-built report yet" caveat had also expired.
src/plugins/runner.tsdid:That was correct only because the plugin phase happens to run last — and
PluginInventoryView.violationsis public plugin API, so the completeness promise was made to third-party authors and kept by one unenforced line. It's now kept by the signature.Verification
pnpm run test:output: all 30 cases matchmain— byte-identical CLI output. This is the acceptance test; emission order is preserved exactly, now chosen in one array rather than inherited from call-site sequencing.aggregateReportsandAggregatedReportare not exported fromsrc/index.ts,HermexScanResultis untouched, andPluginInventoryView's shape is unchanged — only how it's fed.One thing worth a reviewer's eye
printScanResults(src/commands/scan.ts) typed its parameter asReturnType<typeof aggregateReports>, so narrowing that return type silently retargeted a renderer at the facts-only type. It now namesAggregatedReportdirectly, which is what the annotation always meant. There may be otherReturnType<typeof …>annotations standing in for a named type — they defeat exactly the kind of compiler check this refactor leaned on. Not swept here; noted inplans/README.md.Contents
4a35226— plan 039 + README entry (the repo'splans/handoff format)0352d86— the refactor🤖 Generated with Claude Code