refactor: reorganize rule categories and scores based on ablation data#154
Merged
refactor: reorganize rule categories and scores based on ablation data#154
Conversation
…eriment data Categories changed from intuition-based (structure/token/component/naming/behavior) to experiment-based (pixel-critical/responsive-critical/code-quality/token-management/minor). Removed 10 low-impact rules, merged 6 rules into 2 (raw-value, irregular-spacing). Scores recalibrated using Phase 1+2 ablation results (PR #149, #150). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughReplaces the five-category taxonomy with a new five-category set; consolidates multiple token/spacing rules into Changes
Sequence Diagram(s)(Skipped — changes are taxonomy and rule consolidation/refactors, not new multi-component sequential flows.) Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/agents/orchestrator.test.ts (1)
86-136:⚠️ Potential issue | 🟡 MinorMissing
responsive-criticalcategory in test fixture.The
byCategoryobject is missing the"responsive-critical"category. TheScoreReport.byCategorytype isRecord<Category, CategoryScoreResult>, which requires entries for all 5 categories defined inCategorySchema. The production code insrc/core/engine/scoring.ts(lines 252-256) iterates over allCATEGORIESand accessescategoryScores[category], which would cause issues if a category is missing.🔧 Proposed fix: Add the missing category
minor: { category: "minor" as const, score: 100, maxScore: 100, percentage: 100, issueCount: 0, uniqueRuleCount: 0, weightedIssueCount: 0, densityScore: 100, diversityScore: 100, bySeverity: { blocking: 0, risk: 0, "missing-info": 0, suggestion: 0 }, }, + "responsive-critical": { + category: "responsive-critical" as const, + score: 100, + maxScore: 100, + percentage: 100, + issueCount: 0, + uniqueRuleCount: 0, + weightedIssueCount: 0, + densityScore: 100, + diversityScore: 100, + bySeverity: { blocking: 0, risk: 0, "missing-info": 0, suggestion: 0 }, + }, },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agents/orchestrator.test.ts` around lines 86 - 136, The test fixture for ScoreReport is missing the "responsive-critical" entry in the byCategory object, which violates the ScoreReport.byCategory (Record<Category, CategoryScoreResult>) expectation and will break code that iterates CATEGORIES; update the test data used in orchestrator.test.ts to add a "responsive-critical" key with a valid CategoryScoreResult shape (same fields as the other category entries) so that byCategory includes all categories from CategorySchema/CATEGORIES referenced in src/core/engine/scoring.ts.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@examples/config.json`:
- Around line 10-12: The example config's "raw-value" object uses outdated
override values; update the "raw-value" entry (the keys "score" and "severity")
in examples/config.json to match the calibrated baseline defaults (use severity
"missing-info" and score -3) so the sample aligns with current experiment
defaults and won't mislead users.
In `@src/agents/report-generator.test.ts`:
- Around line 11-14: ALL_CATEGORIES in src/agents/report-generator.test.ts omits
the "responsive-critical" category, leaving the test fixture Record<Category,
CategoryScoreResult> incomplete; update the ALL_CATEGORIES array/constant to
include "responsive-critical" alongside "pixel-critical", "token-management",
"code-quality", and "minor" so the test generates a full mapping for all
Category keys used by the report formatting logic (look for the ALL_CATEGORIES
variable in this test file and add the missing category string).
In `@src/core/engine/scoring.ts`:
- Around line 359-368: The getCategoryLabel function is duplicating the label
map; replace the local labels Record<Category,string> with the shared
CATEGORY_LABELS constant: remove the inline labels object in getCategoryLabel
and return CATEGORY_LABELS[category] instead, and add an import for
CATEGORY_LABELS (and Category if needed) from the contracts module so
getCategoryLabel reuses the existing mapping.
In `@src/core/rules/rule-config.ts`:
- Around line 85-94: The rule "missing-component" in rule-config.ts has been
lowered to score -5 which violates the guideline to keep component-related rule
scores at or above the code-quality floor until full fixture rerun; update the
"missing-component" entry (key: "missing-component") to restore the approved
floor score (do not lower it based on small-fixture calibration), leaving
enabled: true and the options (minRepetitions, structureMinRepetitions,
maxFingerprintDepth) unchanged.
In `@src/core/rules/token/index.ts`:
- Around line 55-73: The current TEXT-node branch only flags raw fonts when both
fontFamily and fontSize are unbound; change the condition in the TEXT branch
(the hasStyleReference(node, "text") check and the hasBoundVariable calls) so
that any unbound font axis counts as raw-value debt — e.g., replace the &&
between hasBoundVariable(node,"fontFamily") and
hasBoundVariable(node,"fontSize") with || (so nodes with either fontFamily or
fontSize unbound are reported). Keep the rest of the logic that builds fontParts
and returns rawValueDef.id/nodeId/nodePath/message unchanged.
---
Outside diff comments:
In `@src/agents/orchestrator.test.ts`:
- Around line 86-136: The test fixture for ScoreReport is missing the
"responsive-critical" entry in the byCategory object, which violates the
ScoreReport.byCategory (Record<Category, CategoryScoreResult>) expectation and
will break code that iterates CATEGORIES; update the test data used in
orchestrator.test.ts to add a "responsive-critical" key with a valid
CategoryScoreResult shape (same fields as the other category entries) so that
byCategory includes all categories from CategorySchema/CATEGORIES referenced in
src/core/engine/scoring.ts.
🪄 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: 2af08228-e801-49a5-bdbe-68ea90d11559
📒 Files selected for processing (50)
.claude/agents/rule-discovery/designer.md.github/ISSUE_TEMPLATE/bug.ymldocs/REFERENCE.mdexamples/config.jsonsrc/agents/analysis-agent.test.tssrc/agents/evidence-collector.test.tssrc/agents/gap-rule-report.test.tssrc/agents/orchestrator.test.tssrc/agents/report-generator.test.tssrc/agents/tuning-agent.test.tssrc/cli/docs.tssrc/core/contracts/category.tssrc/core/contracts/rule.tssrc/core/engine/integration.test.tssrc/core/engine/rule-engine.test.tssrc/core/engine/scoring.test.tssrc/core/engine/scoring.tssrc/core/rules/behavior/index.tssrc/core/rules/behavior/overflow-behavior-unknown.test.tssrc/core/rules/behavior/prototype-link-in-design.test.tssrc/core/rules/behavior/text-truncation-unhandled.test.tssrc/core/rules/behavior/wrap-behavior-unknown.test.tssrc/core/rules/component/detached-instance.test.tssrc/core/rules/component/index.test.tssrc/core/rules/component/index.tssrc/core/rules/component/missing-component.test.tssrc/core/rules/component/variant-structure-mismatch.test.tssrc/core/rules/config-loader.test.tssrc/core/rules/config-loader.tssrc/core/rules/index.tssrc/core/rules/naming/default-name.test.tssrc/core/rules/naming/inconsistent-naming-convention.test.tssrc/core/rules/naming/index.tssrc/core/rules/naming/non-semantic-name.test.tssrc/core/rules/naming/numeric-suffix-name.test.tssrc/core/rules/naming/too-long-name.test.tssrc/core/rules/rule-config.tssrc/core/rules/structure/index.tssrc/core/rules/structure/no-auto-layout.test.tssrc/core/rules/structure/unnecessary-node.test.tssrc/core/rules/structure/z-index-dependent-layout.test.tssrc/core/rules/token/inconsistent-spacing.test.tssrc/core/rules/token/index.tssrc/core/rules/token/magic-number-spacing.test.tssrc/core/rules/token/multiple-fill-colors.test.tssrc/core/rules/token/raw-color.test.tssrc/core/rules/token/raw-font.test.tssrc/core/rules/token/raw-opacity.test.tssrc/core/rules/token/raw-shadow.test.tssrc/core/ui-constants.ts
💤 Files with no reviewable changes (19)
- src/core/rules/index.ts
- src/core/rules/behavior/text-truncation-unhandled.test.ts
- src/core/rules/token/magic-number-spacing.test.ts
- src/core/rules/structure/z-index-dependent-layout.test.ts
- src/core/rules/behavior/overflow-behavior-unknown.test.ts
- src/core/rules/naming/too-long-name.test.ts
- src/core/rules/naming/numeric-suffix-name.test.ts
- src/core/rules/behavior/wrap-behavior-unknown.test.ts
- src/core/rules/token/raw-opacity.test.ts
- src/core/rules/behavior/prototype-link-in-design.test.ts
- src/core/rules/token/multiple-fill-colors.test.ts
- src/core/rules/token/raw-shadow.test.ts
- src/core/rules/token/raw-font.test.ts
- src/core/rules/behavior/index.ts
- src/core/rules/component/index.test.ts
- src/core/rules/token/raw-color.test.ts
- src/core/rules/structure/unnecessary-node.test.ts
- src/core/rules/token/inconsistent-spacing.test.ts
- src/core/rules/config-loader.ts
Comment on lines
+85
to
94
| "missing-component": { | ||
| severity: "risk", | ||
| score: -5, | ||
| depthWeight: 1.2, | ||
| enabled: true, | ||
| }, | ||
| "deep-nesting": { | ||
| severity: "risk", | ||
| score: -4, | ||
| enabled: true, | ||
| options: { | ||
| maxDepth: 5, | ||
| minRepetitions: 3, | ||
| structureMinRepetitions: 2, | ||
| maxFingerprintDepth: 3, | ||
| }, | ||
| }, |
Contributor
There was a problem hiding this comment.
Keep missing-component above the code-quality floor until the full fixture rerun finishes.
The PR notes that real-fixture re-analysis is still pending, so leaving this component rule at -5 weakens a signal the repo explicitly says not to lower on small-fixture calibration.
Suggested adjustment
"missing-component": {
severity: "risk",
- score: -5,
+ score: -7,
enabled: true,
options: {
minRepetitions: 3,As per coding guidelines, "Component-related rule scores (missing-component, etc.) should NOT be lowered based on small fixture calibration".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/core/rules/rule-config.ts` around lines 85 - 94, The rule
"missing-component" in rule-config.ts has been lowered to score -5 which
violates the guideline to keep component-related rule scores at or above the
code-quality floor until full fixture rerun; update the "missing-component"
entry (key: "missing-component") to restore the approved floor score (do not
lower it based on small-fixture calibration), leaving enabled: true and the
options (minRepetitions, structureMinRepetitions, maxFingerprintDepth)
unchanged.
- Add missing responsive-critical to orchestrator + report-generator tests - Restore missing-component score to -7 (CLAUDE.md guideline) - Fix raw-value font check: flag partial tokenization (fontFamily OR fontSize) - Reuse CATEGORY_LABELS in getCategoryLabel (remove duplication) - Align example config with baseline defaults Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Add optional subType field to RuleViolation for programmatic grouping (e.g., raw-value has color/font/shadow/opacity/spacing sub-types) - Create rule-messages.ts with all message template functions - Replace inline message strings in all 15 rules with centralized constants - Include subType in JSON output (buildResultJson) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…), add subType to default-name and irregular-spacing - Remove missing-responsive-behavior rule (15→14 rules) — duplicates no-auto-layout - Add subType to default-name: frame/group/vector/shape/text/image/component/instance - Add subType to irregular-spacing: padding/gap - Separate vector from shape in default-name subType for granular control Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ction - Rename rule: group-usage → non-layout-container (broader scope) - Add Section detection: Sections used as layout containers are flagged - SubTypes: group (blocking, -8) and section (same score, flagged only with children) - Remove missing-responsive-behavior (duplicate of no-auto-layout) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…p/grid) Messages pre-defined for all 4 sub-types. Currently only max-width is detected — min-width, wrap, and grid conditions to be implemented in #152. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This was referenced Mar 28, 2026
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/core/engine/scoring.test.ts (1)
420-440: 🧹 Nitpick | 🔵 TrivialAdd a regression assertion for
subTypein the JSON output.
buildResultJson()now emitsviolation.subTypewhen present, but these assertions still only coverruleId,severity,nodeId,nodePath, andmessage. A single case withissue.violation.subTypeset would keep this new JSON contract from regressing unnoticed.🧪 Possible test extension
it("includes detailed issues list with severity and node info", () => { + const tokenIssue = makeIssue({ + ruleId: "raw-value", + category: "token-management", + severity: "missing-info", + }); + tokenIssue.violation.subType = "color"; + const result = makeResult([ makeIssue({ ruleId: "no-auto-layout", category: "pixel-critical", severity: "blocking" }), - makeIssue({ ruleId: "raw-value", category: "token-management", severity: "missing-info" }), + tokenIssue, ]); const scores = calculateScores(result); const json = buildResultJson("TestFile", result, scores); const issues = json.issues as Array<{ ruleId: string; severity: string; nodeId: string; nodePath: string; message: string }>; expect(issues).toHaveLength(2); expect(issues[0]).toMatchObject({ ruleId: "no-auto-layout", severity: "blocking", nodeId: expect.any(String), nodePath: expect.any(String), message: expect.any(String), }); expect(issues[1]).toMatchObject({ ruleId: "raw-value", + subType: "color", severity: "missing-info", }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/engine/scoring.test.ts` around lines 420 - 440, Add a regression assertion to the test that verifies violation.subType is emitted by buildResultJson: update the "includes detailed issues list..." test to create at least one issue with issue.violation.subType set (via makeIssue), call makeResult/calculateScores/buildResultJson as already done, and assert that the corresponding entry in json.issues includes subType (e.g., expect(issues[0].subType).toEqual(<the subType string>) or use toMatchObject including subType). This ensures buildResultJson continues to include violation.subType when present.
♻️ Duplicate comments (1)
src/agents/report-generator.test.ts (1)
10-15: 🧹 Nitpick | 🔵 TrivialReuse the canonical category list here.
ALL_CATEGORIESalready caused one taxonomy drift bug, and the local order is still different fromsrc/core/contracts/category.ts(token-managementcomes beforecode-quality). ImportingCATEGORIESfor this fixture keeps the test exhaustive and ordered like production.♻️ Possible refactor
-import type { Category } from "../core/contracts/category.js"; +import { CATEGORIES, type Category } from "../core/contracts/category.js"; -const ALL_CATEGORIES: Category[] = [ - "pixel-critical", - "responsive-critical", - "token-management", - "code-quality", - "minor", -]; +const ALL_CATEGORIES: Category[] = [...CATEGORIES];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agents/report-generator.test.ts` around lines 10 - 15, Replace the locally defined ALL_CATEGORIES test fixture with the canonical CATEGORIES export to avoid taxonomy drift: remove the ALL_CATEGORIES constant and import the CATEGORIES symbol (the exported canonical category array) from the module that defines it, then use CATEGORIES in the test (ensuring its type aligns with Category[] if needed); this will preserve the production order (e.g., token-management before code-quality) and keep the test exhaustive and consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@examples/config.json`:
- Around line 10-12: The config loader currently uses z.record(z.string(),
RuleOverrideSchema) in src/core/rules/config-loader.ts which allows
legacy/removed rule keys; change the schema to validate keys against the
canonical rule-id set (e.g., replace z.string() with z.enum(...) or validate
keys with .refine using the exported supported rule IDs constant such as
SUPPORTED_RULE_IDS or supportedRuleIds) so any unknown keys (including removed
names like raw-color, missing-responsive-behavior or top-level colorTolerance)
are rejected during parsing; update imports to pull the authoritative rule ID
list and ensure RuleOverrideSchema remains the value schema.
In `@src/core/rules/rule-config.ts`:
- Around line 13-33: The change to RULE_ID_CATEGORY removed/renamed several rule
IDs which will make older user configs silently ignored; update
src/core/rules/config-loader.ts to detect unknown or retired rule IDs and either
map them to new IDs via an alias/deprecation table or emit a clear
warning/error; implement a small ALIAS_MAP (deprecatedId -> currentId) lookup
used when loading configs so config-loader validates each incoming ruleId
against RULE_ID_CATEGORY and substitutes or logs a deprecation notice, and
ensure unknown ruleIds that have no alias cause a visible validation error (or
documented fallback) so users are not left with rules that silently fail to
apply.
In `@src/core/rules/structure/index.ts`:
- Around line 296-304: The SECTION branch in the non-layout-container check is
too broad; stop flagging SECTION nodes as blocking by removing or gating that
branch—specifically, in the block that checks node.type === "SECTION" (which
currently returns the object using nonLayoutContainerDef.id and
nonLayoutContainerMsg.section), change the logic to skip SECTION nodes (early
return null) until a stronger layout-container heuristic is implemented, or
replace the children.length check with a more specific predicate before emitting
the rule; update any related tests/assertions that expect SECTION to be
reported.
In `@src/core/rules/token/index.ts`:
- Around line 36-135: The rawValueCheck function currently returns on the first
matched token and therefore only emits a single violation per node; modify
rawValueCheck so it collects all matching subType findings (use an array of
findings within rawValueCheck) instead of returning immediately in each check
(replace each early return with pushing an object holding subType, message,
nodeId/nodePath, ruleId); after all checks, if findings.length > 0 either (A)
return the array of findings (if the engine/check contract supports multiple
results per rule) or (B) fold them into a single aggregated result (e.g.,
ruleId: rawValueDef.id, subTypes: findings.map(f=>f.subType), messages:
findings.map(f=>f.message) or a combined message) and update callers
accordingly; reference rawValueCheck, rawValueDef, rawValueMsg,
hasStyleReference, hasBoundVariable when making these changes.
---
Outside diff comments:
In `@src/core/engine/scoring.test.ts`:
- Around line 420-440: Add a regression assertion to the test that verifies
violation.subType is emitted by buildResultJson: update the "includes detailed
issues list..." test to create at least one issue with issue.violation.subType
set (via makeIssue), call makeResult/calculateScores/buildResultJson as already
done, and assert that the corresponding entry in json.issues includes subType
(e.g., expect(issues[0].subType).toEqual(<the subType string>) or use
toMatchObject including subType). This ensures buildResultJson continues to
include violation.subType when present.
---
Duplicate comments:
In `@src/agents/report-generator.test.ts`:
- Around line 10-15: Replace the locally defined ALL_CATEGORIES test fixture
with the canonical CATEGORIES export to avoid taxonomy drift: remove the
ALL_CATEGORIES constant and import the CATEGORIES symbol (the exported canonical
category array) from the module that defines it, then use CATEGORIES in the test
(ensuring its type aligns with Category[] if needed); this will preserve the
production order (e.g., token-management before code-quality) and keep the test
exhaustive and consistent.
🪄 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: 0f9bd16b-8c5e-49e0-be7c-f3c2fa842a13
📒 Files selected for processing (14)
docs/REFERENCE.mdexamples/config.jsonsrc/agents/orchestrator.test.tssrc/agents/report-generator.test.tssrc/core/contracts/rule.tssrc/core/engine/rule-engine.test.tssrc/core/engine/scoring.test.tssrc/core/engine/scoring.tssrc/core/rules/component/index.tssrc/core/rules/naming/index.tssrc/core/rules/rule-config.tssrc/core/rules/rule-messages.tssrc/core/rules/structure/index.tssrc/core/rules/token/index.ts
Comment on lines
+296
to
+304
| if (node.type === "SECTION") { | ||
| // Only flag Sections that have children (used as layout container) | ||
| if (!node.children || node.children.length === 0) return null; | ||
| return { | ||
| ruleId: missingResponsiveBehaviorDef.id, | ||
| ruleId: nonLayoutContainerDef.id, | ||
| subType: "section" as const, | ||
| nodeId: node.id, | ||
| nodePath: context.path.join(" > "), | ||
| message: `"${node.name}" has no responsive behavior configured — apply auto-layout or set constraints`, | ||
| message: nonLayoutContainerMsg.section(node.name), |
Contributor
There was a problem hiding this comment.
The SECTION branch is too broad for a blocking rule.
children.length > 0 does not prove a section is being used as a layout container; it only proves the section is non-empty. With the current predicate, any ordinary section wrapper in an analyzed subtree will raise a blocking non-layout-container issue. Please gate this on a stronger signal or skip SECTION until that heuristic exists.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/core/rules/structure/index.ts` around lines 296 - 304, The SECTION branch
in the non-layout-container check is too broad; stop flagging SECTION nodes as
blocking by removing or gating that branch—specifically, in the block that
checks node.type === "SECTION" (which currently returns the object using
nonLayoutContainerDef.id and nonLayoutContainerMsg.section), change the logic to
skip SECTION nodes (early return null) until a stronger layout-container
heuristic is implemented, or replace the children.length check with a more
specific predicate before emitting the rule; update any related tests/assertions
that expect SECTION to be reported.
Owner
Author
There was a problem hiding this comment.
Keeping as-is. Section is not a layout container by design — using it as one is a semantic misuse, which is exactly what this rule flags. children.length > 0 is sufficient because a non-empty Section in a UI layout tree means someone is using it structurally.
Contributor
There was a problem hiding this comment.
@let-sunny — Got it, I'm here and responsive! 👋 Let me know if you have any questions or feedback on the review comment about the SECTION branch in non-layout-container.
🧠 Learnings used
Learnt from: CR
Repo: let-sunny/canicode PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-28T00:25:18.075Z
Learning: Applies to src/**/rules/rule-config.ts : `no-auto-layout` is the single highest-impact rule with score -10 — empirically validated via ablation experiments
Learnt from: CR
Repo: let-sunny/canicode PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-28T00:25:18.075Z
Learning: Applies to src/cli/**/*.ts : Full-file analysis is discouraged — use section or page level analysis with `node-id` required in URL
Learnt from: let-sunny
Repo: let-sunny/canicode PR: 93
File: src/core/rules/rule-config.ts:76-80
Timestamp: 2026-03-26T01:28:57.785Z
Learning: In the let-sunny/canicode repo, `src/core/rules/rule-config.ts` is automatically adjusted by a nightly calibration pipeline. Do NOT suggest adding inline comments to this file for calibration rationale — the change evidence is tracked in PR descriptions, commit messages, and `data/calibration-evidence.json` instead. Inline comments would create clutter as the file is frequently auto-modified.
Learnt from: CR
Repo: let-sunny/canicode PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-28T00:25:18.075Z
Learning: Applies to src/**/rules/**/*.ts : All rule scores, severity, and thresholds are managed in `rules/rule-config.ts` — rule logic and score config must be intentionally separated
Learnt from: CR
Repo: let-sunny/canicode PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-28T00:25:18.075Z
Learning: Applies to src/**/rules/rule-config.ts : Component-related rule scores (missing-component, etc.) should NOT be lowered based on small fixture calibration
Contributor
There was a problem hiding this comment.
@let-sunny, understood — that's a clear and well-reasoned distinction. Since Figma's SECTION type is explicitly not a layout container, any non-empty SECTION in a layout tree represents a semantic misuse, making children.length > 0 the correct and sufficient gate. I'll defer to your judgment here.
✏️ Learnings added
Learnt from: let-sunny
Repo: let-sunny/canicode PR: 154
File: src/core/rules/structure/index.ts:296-304
Timestamp: 2026-03-29T00:24:13.455Z
Learning: In let-sunny/canicode, the `non-layout-container` rule in `src/core/rules/structure/index.ts` intentionally flags non-empty `SECTION` nodes (`children.length > 0`) because Figma `SECTION` is not a layout container by design — using it structurally with children is treated as semantic misuse. This predicate is considered sufficient and intentional; do not flag it as too broad.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: CR
Repo: let-sunny/canicode PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-28T00:25:18.075Z
Learning: Applies to src/**/rules/rule-config.ts : `no-auto-layout` is the single highest-impact rule with score -10 — empirically validated via ablation experiments
Learnt from: CR
Repo: let-sunny/canicode PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-28T00:25:18.075Z
Learning: Applies to src/**/rules/rule-config.ts : Component-related rule scores (missing-component, etc.) should NOT be lowered based on small fixture calibration
Learnt from: let-sunny
Repo: let-sunny/canicode PR: 93
File: src/core/rules/rule-config.ts:76-80
Timestamp: 2026-03-26T01:28:57.785Z
Learning: In the let-sunny/canicode repo, `src/core/rules/rule-config.ts` is automatically adjusted by a nightly calibration pipeline. Do NOT suggest adding inline comments to this file for calibration rationale — the change evidence is tracked in PR descriptions, commit messages, and `data/calibration-evidence.json` instead. Inline comments would create clutter as the file is frequently auto-modified.
Learnt from: CR
Repo: let-sunny/canicode PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-28T00:25:18.075Z
Learning: Applies to src/**/rules/**/*.ts : All rule scores, severity, and thresholds are managed in `rules/rule-config.ts` — rule logic and score config must be intentionally separated
- Add subType regression test in buildResultJson - Use CATEGORIES import in report-generator.test.ts (prevent taxonomy drift) - Legacy config deprecation → #156 (separate issue) - raw-value single-violation-per-node → by design (RuleCheckFn contract) - SECTION detection condition kept as-is (semantic misuse = valid flag) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/core/engine/scoring.test.ts (1)
420-446: 🧹 Nitpick | 🔵 TrivialGood coverage of subType conditional spreading.
The test correctly verifies both cases:
- Issues without
subTypehave it omitted from JSON output- Issues with
subType="color"have it includedConsider adding an edge case test for empty string
subType.Per the relevant code snippet at
scoring.ts:392-399, the guardissue.violation.subType && { subType: ... }treats falsy values (including empty string"") as absent. This is subtle behavior worth documenting with a test.💡 Suggested edge case test
it("omits subType when it is an empty string", () => { const emptySubTypeIssue = makeIssue({ ruleId: "raw-value", category: "token-management", severity: "missing-info" }); emptySubTypeIssue.violation.subType = ""; const result = makeResult([emptySubTypeIssue]); const scores = calculateScores(result); const json = buildResultJson("TestFile", result, scores); const issues = json.issues as Array<{ ruleId: string; subType?: string }>; expect(issues[0]!["subType"]).toBeUndefined(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/engine/scoring.test.ts` around lines 420 - 446, Add a focused edge-case test to scoring.test.ts that verifies an empty-string subType is treated as absent: create an issue via makeIssue, set issue.violation.subType = "" (empty string), run makeResult and calculateScores, call buildResultJson and assert that the resulting json.issues[0]["subType"] is undefined; this mirrors the guard in scoring.ts (the issue.violation.subType && { subType: ... } logic) and ensures empty string is omitted from output.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/core/engine/scoring.test.ts`:
- Around line 420-446: Add a focused edge-case test to scoring.test.ts that
verifies an empty-string subType is treated as absent: create an issue via
makeIssue, set issue.violation.subType = "" (empty string), run makeResult and
calculateScores, call buildResultJson and assert that the resulting
json.issues[0]["subType"] is undefined; this mirrors the guard in scoring.ts
(the issue.violation.subType && { subType: ... } logic) and ensures empty string
is omitted from output.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7d2ecb8d-297d-4ec0-a473-86b0cc6a6eea
📒 Files selected for processing (2)
src/agents/report-generator.test.tssrc/core/engine/scoring.test.ts
Verifies that falsy subType (empty string) is omitted from JSON output, matching the conditional spread guard in buildResultJson. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/core/engine/scoring.test.ts`:
- Around line 452-458: The test reads issues[0] without guaranteeing the array
has elements; update the test that uses makeResult, calculateScores and
buildResultJson to assert the issues array length before indexed access (e.g.,
expect(issues.length).toBeGreaterThan(0) or toEqual(1)) so the subsequent
expect(issues[0]!["subType"]).toBeUndefined() is guarded and satisfies
noUncheckedIndexedAccess.
🪄 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: 3f44a2e9-8349-4119-8f7a-904eb73dad6e
📒 Files selected for processing (1)
src/core/engine/scoring.test.ts
Config files with invalid rule IDs now throw with a clear error message listing the unknown IDs and all valid options. No legacy alias mapping needed — no existing users to support. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This was referenced Mar 29, 2026
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/core/rules/config-loader.test.ts`:
- Around line 88-98: The test for "throws for unknown rule ID" only asserts
/Unknown rule ID/; update it to assert the full error contract by checking that
the thrown error contains both the unknown ID mention and the list/phrase about
valid IDs. Modify the assertion around loadConfigFile(filePath) (in
src/core/rules/config-loader.test.ts, test "throws for unknown rule ID") to
either use a regex that matches both "Unknown rule ID" and "valid IDs" (e.g.
/Unknown rule ID[\s\S]*valid IDs/) or explicitly inspect error.message to assert
it includes both substrings, ensuring the error from loadConfigFile contains
both pieces.
🪄 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: 082a6e19-5756-44f0-9d21-07759e477846
📒 Files selected for processing (2)
src/core/rules/config-loader.test.tssrc/core/rules/config-loader.ts
- Guard indexed array access with length assertion (noUncheckedIndexedAccess) - Strengthen unknown rule ID test to assert both error and valid IDs list Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This was referenced Mar 29, 2026
5 tasks
10 tasks
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
Ablation 실험 데이터(#149, #150)를 기반으로 카테고리/severity/score 체계를 직관 기반 → 실험 기반으로 전면 재구성.
Category 변경
Rule 변경
제거 (10개)
unnecessary-nodemissing-component-descriptionprototype-link-in-designnumeric-suffix-nametoo-long-namez-index-dependent-layoutmultiple-fill-colorstext-truncation-unhandledoverflow-behavior-unknownwrap-behavior-unknown합침 (6 → 2)
raw-color,raw-font,raw-shadow,raw-opacityraw-valueinconsistent-spacing,magic-number-spacingirregular-spacingScore 재조정
group-usage-5 → -8 (blocking 승격)fixed-size-in-auto-layout-3 → -6missing-component-7 → -5raw-value-3 (신규)default-name-2 → -1Related Issues
Test plan
pnpm lint— cleanpnpm test:run— 521/521 passcanicode analyze돌려보기)🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Changes