Skip to content

fix: rule condition review and updates (#152) #163

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
let-sunny merged 4 commits into from
Mar 29, 2026
Merged

fix: rule condition review and updates (#152) #163

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5cc84c7
fix: rule condition updates (#152)
let-sunny Mar 29, 2026
ffa2d69
fix: wrap/grid subType — only flag FILL children, not FIXED
let-sunny Mar 29, 2026
f286c9e
fix: address CodeRabbit review — duplicate ID, exact subType tests, c…
let-sunny Mar 29, 2026
98991f9
fix: tighten overlap test assertion to exact subType "frame"
let-sunny Mar 29, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions docs/REFERENCE.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,12 +129,11 @@ Override score, severity, or enable/disable individual rules:
| `missing-interaction-state` | -3 | missing-info |
| `missing-prototype` | -3 | missing-info |

**Minor (4 rules)** — naming issues with negligible impact (ΔV < 2%)
**Minor (3 rules)** — naming issues with negligible impact (ΔV < 2%)

| Rule ID | Default Score | Default Severity |
|---------|--------------|-----------------|
| `non-standard-naming` | -1 | suggestion |
| `default-name` | -1 | suggestion |
| `non-semantic-name` | -1 | suggestion |
| `inconsistent-naming-convention` | -1 | suggestion |
<!-- RULE_TABLE_END -->
Expand Down
2 changes: 1 addition & 1 deletion src/cli/docs.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,7 @@ EXAMPLE
"rules": {
"no-auto-layout": { "score": -15, "severity": "blocking" },
"raw-value": { "score": -5 },
"default-name": { "enabled": false }
"non-semantic-name": { "enabled": false }
}
}

Expand Down
1 change: 0 additions & 1 deletion src/core/contracts/rule.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,6 @@ export type RuleId =
| "missing-prototype"
// Minor — naming issues with negligible impact (ΔV < 2%)
| "non-standard-naming"
| "default-name"
| "non-semantic-name"
| "inconsistent-naming-convention";

Expand Down
14 changes: 7 additions & 7 deletions src/core/engine/rule-engine.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -206,14 +206,14 @@ describe("RuleEngine.analyze — rule filtering", () => {
const file = makeFile({ document: doc });

// Enable only default-name rule
const result = analyzeFile(file, { enabledRules: ["default-name"] });
const result = analyzeFile(file, { enabledRules: ["non-semantic-name"] });

// Must have at least one issue to avoid vacuous pass
expect(result.issues.length).toBeGreaterThan(0);

// All issues should be from default-name only
for (const issue of result.issues) {
expect(issue.violation.ruleId).toBe("default-name");
expect(issue.violation.ruleId).toBe("non-semantic-name");
}
});

Expand All @@ -229,13 +229,13 @@ describe("RuleEngine.analyze — rule filtering", () => {
const file = makeFile({ document: doc });

const resultAll = analyzeFile(file);
const resultDisabled = analyzeFile(file, { disabledRules: ["default-name"] });
const resultDisabled = analyzeFile(file, { disabledRules: ["non-semantic-name"] });

const defaultNameAll = resultAll.issues.filter(
(i) => i.violation.ruleId === "default-name"
(i) => i.violation.ruleId === "non-semantic-name"
);
const defaultNameDisabled = resultDisabled.issues.filter(
(i) => i.violation.ruleId === "default-name"
(i) => i.violation.ruleId === "non-semantic-name"
);

// Baseline must have default-name issues to validate the filter
Expand Down Expand Up @@ -392,7 +392,7 @@ describe("RuleEngine.analyze — error resilience", () => {
const file = makeFile({ document: doc });

// Make an existing rule's check throw to verify error resilience
const defaultNameRule = ruleRegistry.get("default-name");
const defaultNameRule = ruleRegistry.get("non-semantic-name");
expect(defaultNameRule).toBeDefined();

const originalCheck = defaultNameRule!.check;
Expand All @@ -407,7 +407,7 @@ describe("RuleEngine.analyze — error resilience", () => {
// failedRules should contain the failure details
expect(result.failedRules.length).toBeGreaterThan(0);

const failure = result.failedRules.find((f) => f.ruleId === "default-name");
const failure = result.failedRules.find((f) => f.ruleId === "non-semantic-name");
expect(failure).toBeDefined();
expect(failure!.error).toBe("boom");
expect(failure!.nodeName).toBeDefined();
Expand Down
10 changes: 5 additions & 5 deletions src/core/engine/scoring.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ describe("calculateScores", () => {
makeIssue({ ruleId: "no-auto-layout", category: "pixel-critical", severity: "blocking" }),
makeIssue({ ruleId: "non-layout-container", category: "pixel-critical", severity: "risk" }),
makeIssue({ ruleId: "raw-value", category: "token-management", severity: "missing-info" }),
makeIssue({ ruleId: "default-name", category: "minor", severity: "suggestion" }),
makeIssue({ ruleId: "non-semantic-name", category: "minor", severity: "suggestion" }),
];
const scores = calculateScores(makeResult(issues));

Expand All @@ -90,7 +90,7 @@ describe("calculateScores", () => {
const heavyIssue = makeIssue({ ruleId: "no-auto-layout", category: "pixel-critical", severity: "blocking", score: -10 });
heavyIssue.calculatedScore = -15; // Simulate depthWeight effect

const lightIssue = makeIssue({ ruleId: "default-name", category: "minor", severity: "suggestion", score: -1 });
const lightIssue = makeIssue({ ruleId: "non-semantic-name", category: "minor", severity: "suggestion", score: -1 });
lightIssue.calculatedScore = -1;

const heavy = calculateScores(makeResult([heavyIssue], 100));
Expand Down Expand Up @@ -159,7 +159,7 @@ describe("calculateScores", () => {
], 100));

const lightRule = calculateScores(makeResult([
makeIssue({ ruleId: "default-name", category: "minor", severity: "suggestion", score: -1 }),
makeIssue({ ruleId: "non-semantic-name", category: "minor", severity: "suggestion", score: -1 }),
], 100));

expect(heavyRule.byCategory["pixel-critical"].diversityScore).toBeLessThan(
Expand All @@ -169,7 +169,7 @@ describe("calculateScores", () => {

it("low-severity rules have minimal diversity impact (intentional)", () => {
const lowSeverity = calculateScores(makeResult([
makeIssue({ ruleId: "default-name", category: "minor", severity: "suggestion", score: -1 }),
makeIssue({ ruleId: "non-semantic-name", category: "minor", severity: "suggestion", score: -1 }),
], 100));

const highSeverity = calculateScores(makeResult([
Expand Down Expand Up @@ -304,7 +304,7 @@ describe("calculateGrade (via calculateScores)", () => {
"code-quality": ["missing-component", "detached-instance", "variant-structure-mismatch", "deep-nesting"],
"token-management": ["raw-value", "irregular-spacing"],
"interaction": ["missing-interaction-state", "missing-prototype"],
"minor": ["non-standard-naming", "default-name", "non-semantic-name", "inconsistent-naming-convention"],
"minor": ["non-standard-naming", "non-semantic-name", "inconsistent-naming-convention"],
};

for (const cat of categories) {
Expand Down
2 changes: 1 addition & 1 deletion src/core/engine/scoring.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ export type Grade = "S" | "A+" | "A" | "B+" | "B" | "C+" | "C" | "D" | "F";
* the per-rule scores in rule-config.ts effectively unused.
*
* Now: `no-auto-layout` (score: -10, depthWeight: 1.5) at root contributes 15
* to density, while `default-name` (score: -1, no depthWeight) contributes 1.
* to density, while `non-semantic-name` (score: -1, no depthWeight) contributes 1.
* This makes calibration loop score adjustments flow through to user-facing scores.
*/

Expand Down
20 changes: 20 additions & 0 deletions src/core/rules/component/detached-instance.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,26 @@ describe("detached-instance", () => {
expect(detachedInstance.check(node, makeContext({ file }))).toBeNull();
});

it("case-sensitive: 'button' does not match component 'Button'", () => {
const file = makeFile({
components: {
"comp-1": { key: "k1", name: "Button", description: "" },
},
});
const node = makeNode({ type: "FRAME", name: "button" });
expect(detachedInstance.check(node, makeContext({ file }))).toBeNull();
});

it("word boundary: 'Discard' does not match component 'Card'", () => {
const file = makeFile({
components: {
"comp-1": { key: "k1", name: "Card", description: "" },
},
});
const node = makeNode({ type: "FRAME", name: "Discard" });
expect(detachedInstance.check(node, makeContext({ file }))).toBeNull();
});

it("returns null when file has no components", () => {
const node = makeNode({ type: "FRAME", name: "SomeFrame" });
expect(detachedInstance.check(node, makeContext())).toBeNull();
Expand Down
6 changes: 3 additions & 3 deletions src/core/rules/component/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -295,12 +295,12 @@ const detachedInstanceCheck: RuleCheckFn = (node, context) => {
// Heuristic: Frame with a name that looks like it came from a component
if (node.type !== "FRAME") return null;

// Check if there's a component in the file with a similar name
// Check if there's a component in the file with a matching name (word boundary)
const components = context.file.components;
const nodeName = node.name.toLowerCase();

for (const [, component] of Object.entries(components)) {
if (nodeName.includes(component.name.toLowerCase())) {
const pattern = new RegExp(`\\b${component.name.replace(/[.*+?^${}()|[\]\\]/g, "\\$&")}\\b`);
if (pattern.test(node.name)) {
Comment on lines +298 to +303
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot Mar 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Description: Search for test cases or comments documenting case-sensitivity behavior for detached-instance

# Check test file for case-sensitivity tests
rg -n -i 'case|lowercase|uppercase|insensitive' src/core/rules/component/detached-instance.test.ts

# Check for any documentation about the case-sensitivity change
rg -n -C2 'exact case|case.?sensitiv' --type=md

Repository: let-sunny/canicode

Length of output: 44

🏁 Script executed:

# First, check if the test file exists and find related files
fd -t f 'detached-instance' --type f
# Also check the structure of the component rules directory
fd -t f . src/core/rules/component/ --type f

Repository: let-sunny/canicode

Length of output: 308

🏁 Script executed:

# Read the actual rule implementation to understand the case-sensitivity logic
cat -n src/core/rules/component/index.ts | head -320 | tail -30

Repository: let-sunny/canicode

Length of output: 1308

🏁 Script executed:

# Search for any PR context or documentation about case-sensitivity
rg -n 'exact case|case.?sensitiv|toLowerCase' src/core/rules/component/
# Also check rule-config.ts for any configuration
rg -n 'detached' src/core/rules/rule-config.ts -A3 -B3

Repository: let-sunny/canicode

Length of output: 755

🏁 Script executed:

# Read the detached-instance test file to check test coverage
cat -n src/core/rules/component/detached-instance.test.ts

Repository: let-sunny/canicode

Length of output: 2398

🏁 Script executed:

# Check for any documentation or PR comments about case-sensitivity
rg -n 'detached|instance' --type=md -A 2 -B 2

Repository: let-sunny/canicode

Length of output: 2015

🏁 Script executed:

# Compare the two rule implementations side by side for context
cat -n src/core/rules/component/index.ts | sed -n '120,145p'

Repository: let-sunny/canicode

Length of output: 1281

Clarify case-sensitivity approach and add test coverage for this rule.

The implementation uses case-sensitive matching, but this choice is neither documented in tests nor documented in comments. More importantly, the missing-component rule in the same file uses case-insensitive matching (lines 125, 139-141), creating an inconsistency.

The test suite has no coverage for case-sensitivity variations (e.g., frame "button" vs component "Button"). Add a test case demonstrating the case-sensitive behavior to make the design choice explicit, or align both rules on a consistent approach.

🧰 Tools
🪛 ast-grep (0.41.1)

[warning] 301-301: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(\\b${component.name.replace(/[.*+?^${}()|[\]\\]/g, "\\$&")}\\b)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html

(regexp-from-variable)

🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@src/core/rules/component/index.ts` around lines 298 - 303, The component name
matching currently builds a case-sensitive RegExp in the loop (pattern variable)
while the missing-component rule uses case-insensitive matching; make the
behavior consistent and add tests: either change the RegExp construction in the
loop that uses component.name to include the 'i' flag (or use
RegExp.prototype.test with case-normalized strings) so it matches
missing-component’s case-insensitive approach, or update the missing-component
logic to be case-sensitive instead; then add unit tests exercising both "Button"
vs "button" cases to the rule test suite to document the chosen behavior and
update a brief comment near the pattern creation and the missing-component rule
to state the intended case-sensitivity.

Comment on lines 301 to +303
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot Mar 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial

Regex escaping is correct; static analysis ReDoS warning is a false positive here.

The escape pattern correctly covers all standard regex metacharacters (. * + ? ^ $ { } ( ) | [ ] \), and the resulting pattern is purely literal characters within \b...\b — no quantifiers or alternations that could cause catastrophic backtracking. The fixtures with names like "Menu/16" and "Platform=Desktop" will be handled correctly.

One edge case worth noting: \b in JavaScript only recognizes ASCII word characters [a-zA-Z0-9_]. If component names contain non-ASCII characters (e.g., CJK, Cyrillic), word boundary matching may behave unexpectedly. This is likely acceptable given the design tool context, but worth documenting if internationalized component naming is expected.

📝 Optional: Add brief comment explaining the escaping rationale 
  for (const [, component] of Object.entries(components)) {
+   // Escape regex metacharacters; resulting pattern is pure literals within word boundaries (no ReDoS risk)
    const pattern = new RegExp(`\\b${component.name.replace(/[.*+?^${}()|[\]\\]/g, "\\$&")}\\b`);
    if (pattern.test(node.name)) {
📝 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
for (const [, component] of Object.entries(components)) {
if (nodeName.includes(component.name.toLowerCase())) {
const pattern = new RegExp(`\\b${component.name.replace(/[.*+?^${}()|[\]\\]/g, "\\$&")}\\b`);
if (pattern.test(node.name)) {
for (const [, component] of Object.entries(components)) {
// Escape regex metacharacters; resulting pattern is pure literals within word boundaries (no ReDoS risk)
const pattern = new RegExp(`\\b${component.name.replace(/[.*+?^${}()|[\]\\]/g, "\\$&")}\\b`);
if (pattern.test(node.name)) {
🧰 Tools
🪛 ast-grep (0.41.1)

[warning] 301-301: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(\\b${component.name.replace(/[.*+?^${}()|[\]\\]/g, "\\$&")}\\b)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html

(regexp-from-variable)

🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@src/core/rules/component/index.ts` around lines 301 - 303, Static analysis
flagged a ReDoS risk but the escape used when building pattern from
component.name is correct and safe; update the code around the loop that
iterates over components (the for (const [, component] of
Object.entries(components)) block) to add a brief inline comment explaining that
component.name is escaped with component.name.replace(/[.*+?^${}()|[\]\\]/g,
"\\$&") to force a literal match, that the RegExp is wrapped in \b...\b so no
quantifiers/alternations exist (hence no catastrophic backtracking), and note
the edge-case that JavaScript \b only recognizes ASCII word characters so
non‑ASCII component names (CJK, Cyrillic) may not behave as expected.

// This frame might be a detached instance of this component
return {
ruleId: detachedInstanceDef.id,
Expand Down
25 changes: 13 additions & 12 deletions src/core/rules/component/missing-component.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ describe("missing-component — Stage 1: Component exists but not used", () => {
id: "0:1",
name: "Document",
type: "DOCUMENT",
children: [frame, makeNode({ id: "f:2", name: "Card" })],
children: [frame],
});

const ctx = makeContext({
Expand Down Expand Up @@ -118,11 +118,12 @@ describe("missing-component — Stage 1: Component exists but not used", () => {
it("only flags first occurrence (dedup)", () => {
const frameA = makeNode({ id: "f:1", name: "Button" });
const frameB = makeNode({ id: "f:2", name: "Button" });
const frameC = makeNode({ id: "f:3", name: "Button" });
const doc = makeNode({
id: "0:1",
name: "Document",
type: "DOCUMENT",
children: [frameA, frameB],
children: [frameA, frameB, frameC],
});

const ctx = makeContext({
Expand All @@ -134,12 +135,13 @@ describe("missing-component — Stage 1: Component exists but not used", () => {
}),
});

// First call flags
// First call on frameA flags (stage 1: unused-component, frameA is first frame)
const result1 = missingComponent.check(frameA, ctx);
expect(result1).not.toBeNull();
expect(result1!.subType).toBe("unused-component");

// Second call with same name is deduped
const result2 = missingComponent.check(frameA, ctx);
// frameB is not first frame → stage 1 deduped, stage 2 skips (not first frame)
const result2 = missingComponent.check(frameB, ctx);
expect(result2).toBeNull();
});

Expand Down Expand Up @@ -179,19 +181,18 @@ describe("missing-component — Stage 2: Name-based repetition", () => {

it("returns null below minRepetitions threshold", () => {
const frameA = makeNode({ id: "f:1", name: "Card" });
const frameB = makeNode({ id: "f:2", name: "Card" });
const doc = makeNode({
id: "0:1",
name: "Document",
type: "DOCUMENT",
children: [frameA, frameB],
children: [frameA],
});

const ctx = makeContext({
file: makeFile({ document: doc }),
});

// Default minRepetitions is 3, only 2 frames
// Default minRepetitions is 2, only 1 frame
expect(missingComponent.check(frameA, ctx)).toBeNull();
});

Expand Down Expand Up @@ -724,11 +725,12 @@ describe("missing-component — General", () => {
it("fresh analysisState clears dedup state", () => {
const frameA = makeNode({ id: "f:1", name: "Button" });
const frameB = makeNode({ id: "f:2", name: "Button" });
const frameC = makeNode({ id: "f:3", name: "Button" });
const doc = makeNode({
id: "0:1",
name: "Document",
type: "DOCUMENT",
children: [frameA, frameB],
children: [frameA, frameB, frameC],
});

const file = makeFile({
Expand All @@ -742,9 +744,8 @@ describe("missing-component — General", () => {

// First call flags (Stage 1)
expect(missingComponent.check(frameA, ctx)).not.toBeNull();

// Deduped
expect(missingComponent.check(frameA, ctx)).toBeNull();
// frameB: not first frame → deduped
expect(missingComponent.check(frameB, ctx)).toBeNull();

// Fresh analysisState simulates a new analysis run — should flag again
analysisState = new Map();
Expand Down
41 changes: 0 additions & 41 deletions src/core/rules/naming/default-name.test.ts
View file
Open in desktop

This file was deleted.

Loading
Loading