fix: sync hardcoded rule values and add auto-sync tooling#112
Merged
fix: sync hardcoded rule values and add auto-sync tooling#112
Conversation
Contributor
📝 WalkthroughWalkthroughThis pull request introduces automated tooling and testing infrastructure to keep rule configuration defaults synchronized between Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
let-sunny
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@package.json`:
- Line 27: Add tsx to devDependencies in package.json and update the "sync-docs"
npm script to invoke the locally installed binary instead of using npx (change
the script key "sync-docs" that currently runs "npx tsx
scripts/sync-rule-docs.ts" to call the local tsx directly via the script). This
ensures CI and local runs use the pinned devDependency, avoiding network lookups
and improving reproducibility; update package.json's devDependencies to include
the appropriate tsx version and remove the npx prefix from the "sync-docs"
script.
In `@scripts/sync-rule-docs.ts`:
- Around line 27-32: The mapping of ruleRegistry.getByCategory(...) to build
rules can yield undefined configs when RULE_CONFIGS lacks an entry for a rule;
update the mapping over ruleRegistry.getByCategory/category to check
RULE_CONFIGS[r.definition.id] for each rule (or filter out missing entries) and
fail fast with a clear error reference to r.definition.id (or skip with a logged
warning) so the constructed rules array never contains undefined config values;
modify the block around the rules variable and use ruleRegistry, getByCategory,
RULE_CONFIGS and r.definition.id to implement this guard.
In `@src/core/rules/rule-config.test.ts`:
- Around line 20-22: The Map construction uses non-null assertions on regex
capture groups (in the tableRows.map callback creating docRules) which violates
noUncheckedIndexedAccess; instead, defensively filter or validate tableRows
entries before mapping: ensure each row array has defined elements at indices
1–3 (or provide safe defaults), e.g., filter tableRows for rows where m[1],
m[2], and m[3] are !== undefined and then map to [m[1], { score: Number(m[2]),
severity: m[3] }]; update the code paths around docRules and the tableRows.map
callback to remove the `!` assertions and use the validated values so TypeScript
no longer complains.
- Around line 66-87: The current test in rule-config.test.ts reads indexContent
and tries to verify numeric counts using regexes but those comments no longer
exist so the assertions are never executed; update the test (the describe block
using CATEGORIES, indexContent, and ruleRegistry.getByCategory) to assert that
no stale count comments are present by searching indexContent for a pattern like
"// .*rules.*\\(\\d+\\)" (or category-specific patterns) and expecting no
matches (e.g., expect(match).toBeNull()), rather than trying to compare
extracted counts to rules.length; keep the loop over CATEGORIES and use
ruleRegistry.getByCategory only if needed for a category-specific negative
check.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: dc1c2985-9b81-4fb4-8b9e-2269db4ac763
📒 Files selected for processing (8)
docs/REFERENCE.mdpackage.jsonscripts/sync-rule-docs.tssrc/core/contracts/rule.tssrc/core/engine/scoring.test.tssrc/core/rules/index.tssrc/core/rules/rule-config.test.tssrc/core/rules/rule-config.ts
let-sunny
force-pushed
the
fix/sync-hardcoded-values
branch
from
a318723 to
a08f3a8
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/sync-rule-docs.ts`:
- Line 19: The code uses import.meta.dirname in the REFERENCE_PATH declaration
which requires Node 20.11+; replace that usage with a portable pattern: import
file URL handling (fileURLToPath) and path.dirname to compute the current module
directory, then call resolve(...) to build REFERENCE_PATH. Update the
module-level logic that defines REFERENCE_PATH (replace
resolve(import.meta.dirname, "../docs/REFERENCE.md")) to derive the directory
from fileURLToPath(import.meta.url) and path.dirname so the script runs on Node
18+.
In `@src/core/engine/scoring.ts`:
- Around line 229-235: The diversity calculation can divide by zero when
weightedTotal is 0; in the block using catScore, ruleScorePerCategory,
weightedTriggered and weightedTotal (and computing diversityRatio and
diversityScore), guard the denominator by checking if weightedTotal === 0 and in
that case set diversityRatio to 0 (or an appropriate default) before computing
diversityScore with clamp/Math.round; update the logic around
diversityRatio/weightedTotal to avoid NaN propagation.
In `@src/core/rules/rule-config.test.ts`:
- Around line 16-24: The test currently scans the entire REFERENCE.md for tables
which can pick up unrelated tables; update the logic that builds
tableRows/docRules to first locate RULE_TABLE_START and RULE_TABLE_END in the
file content (verify both markers exist and throw/assert if missing), slice
content between those markers, then run the existing matchAll regex on that
sliced section to produce tableRows and docRules (keep the existing Map
construction using tableRows and Number(m[2]) for score and m[3] for severity).
- Around line 54-66: Add a test that verifies RULE_ID_CATEGORY keys and values
align with the registry/config: for each id in RULE_ID_CATEGORY ensure
ruleRegistry.has(id as RuleId) is true and also assert that RULE_ID_CATEGORY[id]
equals the category declared in RULE_CONFIGS[id] (or at minimum that
RULE_CONFIGS[id].category is defined and matches), so any missing/incorrect
category mapping will fail the suite; reference RULE_ID_CATEGORY, RULE_CONFIGS,
ruleRegistry, and calculateScores in the new assertions.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 991a0aa0-bb11-4b50-83d1-0ce8612acd08
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml,!pnpm-lock.yaml
📒 Files selected for processing (11)
docs/REFERENCE.mdpackage.jsonscripts/sync-rule-docs.tssrc/cli/commands/analyze.tssrc/core/contracts/rule.tssrc/core/engine/scoring.test.tssrc/core/engine/scoring.tssrc/core/rules/index.tssrc/core/rules/rule-config.test.tssrc/core/rules/rule-config.tssrc/mcp/server.ts
Comment on lines
229
to
235
| let diversityScore = 100; | ||
| if (catScore.issueCount > 0) { | ||
| const diversityRatio = uniqueRules.size / totalRules; | ||
| const ruleScores = ruleScorePerCategory.get(category)!; | ||
| const weightedTriggered = Array.from(ruleScores.values()).reduce((sum, s) => sum + s, 0); | ||
| const weightedTotal = totalScorePerCategory[category]; | ||
| const diversityRatio = weightedTriggered / weightedTotal; | ||
| diversityScore = clamp(Math.round((1 - diversityRatio) * 100), 0, 100); |
Contributor
There was a problem hiding this comment.
Guard the diversity denominator when a category totals 0.
A valid override can set every enabled rule in a category to score: 0. In that case weightedTotal becomes 0, weightedTriggered / weightedTotal evaluates to NaN, and the category/overall percentages follow it.
Suggested fix
if (catScore.issueCount > 0) {
const ruleScores = ruleScorePerCategory.get(category)!;
const weightedTriggered = Array.from(ruleScores.values()).reduce((sum, s) => sum + s, 0);
const weightedTotal = totalScorePerCategory[category];
- const diversityRatio = weightedTriggered / weightedTotal;
- diversityScore = clamp(Math.round((1 - diversityRatio) * 100), 0, 100);
+ if (weightedTotal > 0) {
+ const diversityRatio = weightedTriggered / weightedTotal;
+ diversityScore = clamp(Math.round((1 - diversityRatio) * 100), 0, 100);
+ }
}As per coding guidelines, src/core/engine/scoring.ts: Verify density/diversity weighting logic is mathematically sound.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/core/engine/scoring.ts` around lines 229 - 235, The diversity calculation
can divide by zero when weightedTotal is 0; in the block using catScore,
ruleScorePerCategory, weightedTriggered and weightedTotal (and computing
diversityRatio and diversityScore), guard the denominator by checking if
weightedTotal === 0 and in that case set diversityRatio to 0 (or an appropriate
default) before computing diversityScore with clamp/Math.round; update the logic
around diversityRatio/weightedTotal to avoid NaN propagation.
Comment on lines
+54
to
+66
| describe("rule registry covers all rule-config.ts entries", () => { | ||
| it("every RULE_CONFIGS entry has a registered rule", () => { | ||
| for (const id of Object.keys(RULE_CONFIGS)) { | ||
| expect(ruleRegistry.has(id as RuleId)).toBe(true); | ||
| } | ||
| }); | ||
|
|
||
| it("every registered rule has a RULE_CONFIGS entry", () => { | ||
| for (const rule of ruleRegistry.getAll()) { | ||
| expect(RULE_CONFIGS[rule.definition.id as RuleId]).toBeDefined(); | ||
| } | ||
| }); | ||
| }); |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Also validate RULE_ID_CATEGORY against the registry.
calculateScores() now uses RULE_ID_CATEGORY for denominator bucketing, but this suite only checks that rule IDs exist in both places. A wrong category mapping would still pass here and silently skew the per-category scores.
Suggested fix
-import { RULE_CONFIGS } from "./rule-config.js";
+import { RULE_CONFIGS, RULE_ID_CATEGORY } from "./rule-config.js";
@@
describe("rule registry covers all rule-config.ts entries", () => {
@@
it("every registered rule has a RULE_CONFIGS entry", () => {
for (const rule of ruleRegistry.getAll()) {
expect(RULE_CONFIGS[rule.definition.id as RuleId]).toBeDefined();
}
});
+
+ it("RULE_ID_CATEGORY matches each registered rule definition", () => {
+ for (const rule of ruleRegistry.getAll()) {
+ expect(RULE_ID_CATEGORY[rule.definition.id as RuleId]).toBe(
+ rule.definition.category
+ );
+ }
+ });
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| describe("rule registry covers all rule-config.ts entries", () => { | |
| it("every RULE_CONFIGS entry has a registered rule", () => { | |
| for (const id of Object.keys(RULE_CONFIGS)) { | |
| expect(ruleRegistry.has(id as RuleId)).toBe(true); | |
| } | |
| }); | |
| it("every registered rule has a RULE_CONFIGS entry", () => { | |
| for (const rule of ruleRegistry.getAll()) { | |
| expect(RULE_CONFIGS[rule.definition.id as RuleId]).toBeDefined(); | |
| } | |
| }); | |
| }); | |
| import { ruleRegistry } from "./index.js"; | |
| import { RULE_CONFIGS, RULE_ID_CATEGORY } from "./rule-config.js"; | |
| import { describe, it, expect } from "vitest"; | |
| import type { RuleId } from "../contracts/index.js"; | |
| describe("rule registry covers all rule-config.ts entries", () => { | |
| it("every RULE_CONFIGS entry has a registered rule", () => { | |
| for (const id of Object.keys(RULE_CONFIGS)) { | |
| expect(ruleRegistry.has(id as RuleId)).toBe(true); | |
| } | |
| }); | |
| it("every registered rule has a RULE_CONFIGS entry", () => { | |
| for (const rule of ruleRegistry.getAll()) { | |
| expect(RULE_CONFIGS[rule.definition.id as RuleId]).toBeDefined(); | |
| } | |
| }); | |
| it("RULE_ID_CATEGORY matches each registered rule definition", () => { | |
| for (const rule of ruleRegistry.getAll()) { | |
| expect(RULE_ID_CATEGORY[rule.definition.id as RuleId]).toBe( | |
| rule.definition.category | |
| ); | |
| } | |
| }); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/core/rules/rule-config.test.ts` around lines 54 - 66, Add a test that
verifies RULE_ID_CATEGORY keys and values align with the registry/config: for
each id in RULE_ID_CATEGORY ensure ruleRegistry.has(id as RuleId) is true and
also assert that RULE_ID_CATEGORY[id] equals the category declared in
RULE_CONFIGS[id] (or at minimum that RULE_CONFIGS[id].category is defined and
matches), so any missing/incorrect category mapping will fail the suite;
reference RULE_ID_CATEGORY, RULE_CONFIGS, ruleRegistry, and calculateScores in
the new assertions.
- REFERENCE.md: fix 10 score/severity mismatches (e.g. no-auto-layout -7→-10, raw-font blocking→risk, numeric-suffix-name missing-info→suggestion) - scoring.test.ts: add missing raw-opacity, multiple-fill-colors to token list - rule-config.ts: simplify section comments to avoid stale rule counts Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Add scripts/sync-rule-docs.ts to auto-generate REFERENCE.md rule tables from rule-config.ts + rule registry (replaces manual maintenance) - Add RULE_TABLE markers in REFERENCE.md for script to target - Add rule-config.test.ts: validates REFERENCE.md values match rule-config.ts, registry covers all config entries (CI will catch drift) - Add pnpm sync-docs script - Remove hardcoded rule count comments from rule.ts, rules/index.ts, rule-config.ts Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Add tsx to devDependencies for reproducible script execution - Guard against missing RULE_CONFIGS entries in sync script - Remove non-null assertions in test regex parsing - Replace no-op count validation with stale comment detection test Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Use fileURLToPath for Node 18 compatibility (import.meta.dirname requires 20.11+) - Parse only between RULE_TABLE markers in test to avoid false matches Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
let-sunny
force-pushed
the
fix/sync-hardcoded-values
branch
from
a08f3a8 to
2ab2e40
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/sync-rule-docs.ts`:
- Around line 63-66: The guard that checks for RULE_TABLE markers only tests for
startIdx === -1 or endIdx === -1 but misses the case where endIdx <= startIdx;
update the check in the sync-rule-docs.ts logic (where startIdx and endIdx are
computed) to also treat endIdx <= startIdx as an error, logging a clear message
like "RULE_TABLE markers misordered" (or similar) and exiting (process.exit(1))
to fail fast and avoid corrupting the rewritten section.
In `@src/core/rules/rule-config.test.ts`:
- Around line 77-79: The test uses import.meta.dirname when resolving the index
file; replace import.meta.dirname with the already-derived __dirname (defined
near the top of rule-config.test.ts) so resolve(__dirname, "./index.ts") is used
instead to maintain Node 18 compatibility.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 79e6d93c-793f-4d58-908b-5e0535cd8484
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml,!pnpm-lock.yaml
📒 Files selected for processing (8)
docs/REFERENCE.mdpackage.jsonscripts/sync-rule-docs.tssrc/core/contracts/rule.tssrc/core/engine/scoring.test.tssrc/core/rules/index.tssrc/core/rules/rule-config.test.tssrc/core/rules/rule-config.ts
Comment on lines
+63
to
+66
| if (startIdx === -1 || endIdx === -1) { | ||
| console.error("RULE_TABLE markers not found in REFERENCE.md"); | ||
| process.exit(1); | ||
| } |
Contributor
There was a problem hiding this comment.
Fail fast when markers are misordered.
The current guard misses the endIdx <= startIdx case, which can corrupt the rewritten section.
Suggested fix
-if (startIdx === -1 || endIdx === -1) {
+if (startIdx === -1 || endIdx === -1 || endIdx <= startIdx) {
console.error("RULE_TABLE markers not found in REFERENCE.md");
process.exit(1);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (startIdx === -1 || endIdx === -1) { | |
| console.error("RULE_TABLE markers not found in REFERENCE.md"); | |
| process.exit(1); | |
| } | |
| if (startIdx === -1 || endIdx === -1 || endIdx <= startIdx) { | |
| console.error("RULE_TABLE markers not found in REFERENCE.md"); | |
| process.exit(1); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/sync-rule-docs.ts` around lines 63 - 66, The guard that checks for
RULE_TABLE markers only tests for startIdx === -1 or endIdx === -1 but misses
the case where endIdx <= startIdx; update the check in the sync-rule-docs.ts
logic (where startIdx and endIdx are computed) to also treat endIdx <= startIdx
as an error, logging a clear message like "RULE_TABLE markers misordered" (or
similar) and exiting (process.exit(1)) to fail fast and avoid corrupting the
rewritten section.
Comment on lines
+77
to
+79
| resolve(import.meta.dirname, "./index.ts"), | ||
| "utf-8" | ||
| ); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify declared Node engine and usage sites of import.meta.dirname
jq -r '.engines.node' package.json
rg -n 'import\.meta\.dirname' src/core/rules/rule-config.test.ts scripts/sync-rule-docs.tsRepository: let-sunny/canicode
Length of output: 154
🏁 Script executed:
cat -n src/core/rules/rule-config.test.ts | head -100Repository: let-sunny/canicode
Length of output: 3707
🏁 Script executed:
rg -n 'import\.meta\.dirname' --type ts --type tsxRepository: let-sunny/canicode
Length of output: 89
🏁 Script executed:
rg -n 'import\.meta\.dirname'Repository: let-sunny/canicode
Length of output: 288
Replace import.meta.dirname with the already-derived __dirname for Node >=18 compatibility.
import.meta.dirname is only available in Node.js 20.11.0 and 18.20.0+, which breaks compatibility with Node 18.0–18.19.x. The file already derives __dirname correctly at line 11; use it instead.
Suggested fix
const indexContent = readFileSync(
- resolve(import.meta.dirname, "./index.ts"),
+ resolve(__dirname, "./index.ts"),
"utf-8"
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| resolve(import.meta.dirname, "./index.ts"), | |
| "utf-8" | |
| ); | |
| resolve(__dirname, "./index.ts"), | |
| "utf-8" | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/core/rules/rule-config.test.ts` around lines 77 - 79, The test uses
import.meta.dirname when resolving the index file; replace import.meta.dirname
with the already-derived __dirname (defined near the top of rule-config.test.ts)
so resolve(__dirname, "./index.ts") is used instead to maintain Node 18
compatibility.
This was referenced Mar 27, 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
docs/REFERENCE.mdthat drifted after calibrationscripts/sync-rule-docs.tsto auto-generate REFERENCE.md rule tables from rule-config.tsrule-config.test.tsvalidation tests — CI fails if REFERENCE.md drifts from rule-config.tsscoring.test.tsWhat was wrong
Calibration loop updated
rule-config.tsscores/severity, butREFERENCE.mdwas never synced:no-auto-layoutabsolute-position-in-auto-layoutmissing-responsive-behaviorfixed-size-in-auto-layoutmissing-size-constraintraw-fontmagic-number-spacingraw-shadowraw-opacitynumeric-suffix-namePrevention
Two layers to prevent future drift:
pnpm sync-docs): auto-generates REFERENCE.md rule tables from rule-config.ts + rule registryrule-config.test.ts): validates every RULE_CONFIGS entry has matching score/severity in REFERENCE.md — CI fails on mismatchTest plan
pnpm sync-docsis idempotent (running twice = no changes)🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Tests