refactor: restructure agents directory and unify comparison pipeline#193
Merged
refactor: restructure agents directory and unify comparison pipeline#193
Conversation
…191) - Move design-tree files to dedicated src/core/design-tree/ directory - Move ablation experiments from src/agents/ablation/ to src/experiments/ablation/ - Unify compareScreenshots with sizeMismatch option ("pad" | "crop") and configurable threshold - Extract renderAndCompare to core/engine/visual-compare.ts for shared use - Add stripDeltaToDifficulty() for objective difficulty measurement foundation - Update all imports and CLAUDE.md documentation https://claude.ai/code/session_015dAVYmRnXTxnYLwQcZDqPf
- Move extractHtml, sanitizeHtml, injectLocalFont, processHtml to core/engine/html-utils.ts - Move getDesignTreeOptions, getFixtureScreenshotPath, copyFixtureImages to core/engine/fixture-helpers.ts - helpers.ts now re-exports from core (experiment scripts unchanged) - Only API calls and experiment config remain in experiments/ablation/helpers.ts https://claude.ai/code/session_015dAVYmRnXTxnYLwQcZDqPf
Contributor
📝 WalkthroughWalkthroughThis PR restructures design-tree, comparison, and fixture utilities into new core modules, moves ablation experiment code from Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 |
1 task
![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: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CLAUDE.md`:
- Around line 136-140: Add a blank line before the fenced code block that starts
with "```bash" under the heading "**`run-phase1.ts` — Strip experiments**" in
CLAUDE.md so the fenced block is surrounded by blank lines per MD031; locate the
block labeled with the bash fence and insert a single empty line immediately
above the "```bash" line.
In `@src/core/design-tree/delta.test.ts`:
- Around line 4-7: Add a unit test for negative delta values for
stripDeltaToDifficulty to document expected behavior when stripped similarity
exceeds baseline; specifically add an assertion like
expect(stripDeltaToDifficulty(-2)).toBe("easy") (or the appropriate difficulty
your implementation should return) alongside the existing cases so negatives are
covered and the intended mapping for values < 0 is explicit.
In `@src/core/engine/fixture-helpers.ts`:
- Around line 39-40: The loop over readdirSync in fixture-helpers.ts currently
calls copyFileSync for every entry unconditionally; update the loop that
iterates readdirSync(fixtureImagesDir) to skip entries that are not regular
files by using fs.lstatSync or fs.statSync on join(fixtureImagesDir, f) and
checking isFile() (or otherwise filtering out directories/symlinks) before
calling copyFileSync to join(runImagesDir, f); keep references to readdirSync,
copyFileSync, join, fixtureImagesDir and runImagesDir so it's easy to locate and
ensure only files are copied.
In `@src/core/engine/html-utils.ts`:
- Line 45: Replace the manual file:// string interpolation when building fontCss
with a proper file URL: import pathToFileURL from 'node:url' (or use require)
and call pathToFileURL(fontPath).href to produce the URL; update the
construction of fontCss (the constant named fontCss that currently uses
`file://${fontPath}`) to use that href so special characters and Windows paths
are encoded correctly.
- Around line 31-35: The current regex-based sanitization that mutates the
result string (the series of result.replace(...) calls) is incomplete and allows
unquoted on* handlers and dangerous protocols in src/xlink:href to survive;
replace this fragile approach by parsing the HTML into a DOM (or using a vetted
library like DOMPurify) and then remove any attributes whose name starts with
"on" (case-insensitive), strip style attributes, and reject or rewrite attribute
values for href/src/xlink:href that use dangerous schemes (javascript:,
vbscript:, data:) after decoding entities and normalizing case; ensure you
update the code where result is produced (the replace(...) sanitization block)
to perform DOM-based sanitization or call the library sanitizer so no executable
vectors remain.
In `@src/experiments/ablation/helpers.ts`:
- Around line 73-97: requireApiKey currently returns
process.env["ANTHROPIC_API_KEY"] as-is allowing whitespace-only or padded keys;
update requireApiKey to read process.env["ANTHROPIC_API_KEY"], apply .trim() to
the value, then validate that the trimmed string is non-empty (log the same
error and process.exit(1) if empty) and return the trimmed key; reference the
requireApiKey function and process.env["ANTHROPIC_API_KEY"] when making the
change.
- Around line 57-59: In the catch block that currently does const status = (err
as { status?: number }).status and then checks (status === 429 || status ===
529) && attempt < MAX_RETRIES, first ensure err is non-null and an object before
reading .status (e.g. check err !== null && typeof err === "object" or use 'in'
to test for "status"), then safely extract status into a local variable
(fallback to undefined if guard fails) and use that guarded status in the
existing retry condition; update the catch around the MAX_RETRIES/attempt logic
(the same scope where MAX_RETRIES and attempt are used) so a null/undefined err
cannot cause a TypeError.
🪄 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: 357a7bd1-11b7-40e5-9a2e-b206d8f27fb9
📒 Files selected for processing (21)
CLAUDE.mdsrc/agents/ablation/helpers.tssrc/cli/commands/design-tree.tssrc/cli/commands/implement.tssrc/core/design-tree/delta.test.tssrc/core/design-tree/delta.tssrc/core/design-tree/design-tree.test.tssrc/core/design-tree/design-tree.tssrc/core/design-tree/index.tssrc/core/design-tree/strip.test.tssrc/core/design-tree/strip.tssrc/core/engine/fixture-helpers.tssrc/core/engine/html-utils.tssrc/core/engine/visual-compare-helpers.tssrc/core/engine/visual-compare.tssrc/core/index.tssrc/experiments/ablation/helpers.tssrc/experiments/ablation/recompare.tssrc/experiments/ablation/run-condition.tssrc/experiments/ablation/run-phase1.tssrc/experiments/ablation/run-responsive.ts
💤 Files with no reviewable changes (1)
- src/agents/ablation/helpers.ts
src/core/engine/fixture-helpers.ts
Outdated
Comment on lines
+39
to
+40
| for (const f of readdirSync(fixtureImagesDir)) { | ||
| copyFileSync(join(fixtureImagesDir, f), join(runImagesDir, f)); |
Contributor
There was a problem hiding this comment.
Guard against directories before copying image entries.
Line 39 iterates all entries, and Line 40 blindly calls copyFileSync; this throws if any entry is a subdirectory/symlink, causing avoidable run failures.
Suggested fix
-import { existsSync, mkdirSync, copyFileSync, readdirSync } from "node:fs";
+import { existsSync, mkdirSync, copyFileSync, readdirSync } from "node:fs";
@@
- for (const f of readdirSync(fixtureImagesDir)) {
- copyFileSync(join(fixtureImagesDir, f), join(runImagesDir, f));
+ for (const entry of readdirSync(fixtureImagesDir, { withFileTypes: true })) {
+ if (!entry.isFile()) continue;
+ copyFileSync(join(fixtureImagesDir, entry.name), join(runImagesDir, entry.name));
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/core/engine/fixture-helpers.ts` around lines 39 - 40, The loop over
readdirSync in fixture-helpers.ts currently calls copyFileSync for every entry
unconditionally; update the loop that iterates readdirSync(fixtureImagesDir) to
skip entries that are not regular files by using fs.lstatSync or fs.statSync on
join(fixtureImagesDir, f) and checking isFile() (or otherwise filtering out
directories/symlinks) before calling copyFileSync to join(runImagesDir, f); keep
references to readdirSync, copyFileSync, join, fixtureImagesDir and runImagesDir
so it's easy to locate and ensure only files are copied.
src/core/engine/html-utils.ts
Outdated
Comment on lines
+31
to
+35
| result = result.replace(/<script[\s\S]*?<\/script>/gi, ""); | ||
| result = result.replace(/\s+on\w+\s*=\s*"[^"]*"/gi, ""); | ||
| result = result.replace(/\s+on\w+\s*=\s*'[^']*'/gi, ""); | ||
| result = result.replace(/href\s*=\s*"javascript:[^"]*"/gi, 'href="#"'); | ||
| result = result.replace(/href\s*=\s*'javascript:[^']*'/gi, "href='#'"); |
Contributor
There was a problem hiding this comment.
Sanitization is bypassable and leaves executable HTML vectors.
Lines 31-35 remove only a subset of dangerous patterns; unquoted on*= handlers and javascript: in src/xlink:href can survive and execute when rendered.
Suggested hardening
export function sanitizeHtml(html: string): string {
let result = html;
result = result.replace(/^\/\/\s*filename:.*\n/i, "");
result = result.replace(/<script[\s\S]*?<\/script>/gi, "");
- result = result.replace(/\s+on\w+\s*=\s*"[^"]*"/gi, "");
- result = result.replace(/\s+on\w+\s*=\s*'[^']*'/gi, "");
- result = result.replace(/href\s*=\s*"javascript:[^"]*"/gi, 'href="#"');
- result = result.replace(/href\s*=\s*'javascript:[^']*'/gi, "href='#'");
+ result = result.replace(/\s+on\w+\s*=\s*(?:"[^"]*"|'[^']*'|[^\s>]+)/gi, "");
+ result = result.replace(
+ /\s+(?:href|src|xlink:href)\s*=\s*(?:"\s*javascript:[^"]*"|'\s*javascript:[^']*'|javascript:[^\s>]+)/gi,
+ ' href="#"'
+ );
return result;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/core/engine/html-utils.ts` around lines 31 - 35, The current regex-based
sanitization that mutates the result string (the series of result.replace(...)
calls) is incomplete and allows unquoted on* handlers and dangerous protocols in
src/xlink:href to survive; replace this fragile approach by parsing the HTML
into a DOM (or using a vetted library like DOMPurify) and then remove any
attributes whose name starts with "on" (case-insensitive), strip style
attributes, and reject or rewrite attribute values for href/src/xlink:href that
use dangerous schemes (javascript:, vbscript:, data:) after decoding entities
and normalizing case; ensure you update the code where result is produced (the
replace(...) sanitization block) to perform DOM-based sanitization or call the
library sanitizer so no executable vectors remain.
src/core/engine/html-utils.ts
Outdated
| export function injectLocalFont(html: string): string { | ||
| const fontPath = resolve("assets/fonts/Inter.var.woff2"); | ||
| if (!existsSync(fontPath)) return html; | ||
| const fontCss = `@font-face { font-family: "Inter"; src: url("file://${fontPath}") format("woff2"); font-weight: 100 900; }`; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify current implementation uses manual file:// interpolation and not pathToFileURL
rg -n 'file://\$\{fontPath\}|pathToFileURL' src/core/engine/html-utils.ts -C2Repository: let-sunny/canicode
Length of output: 402
🏁 Script executed:
#!/bin/bash
# Check the full context of the function and existing imports
head -60 src/core/engine/html-utils.ts | cat -nRepository: let-sunny/canicode
Length of output: 3072
🏁 Script executed:
#!/bin/bash
# Check if pathToFileURL is already imported anywhere in the file
rg -n 'import.*pathToFileURL|from.*node:url' src/core/engine/html-utils.tsRepository: let-sunny/canicode
Length of output: 44
🏁 Script executed:
#!/bin/bash
# Verify pathToFileURL is the correct API from node:url
node -e "const { pathToFileURL } = require('url'); console.log(typeof pathToFileURL); const testPath = '/path/with spaces/file.txt'; console.log(pathToFileURL(testPath).href);"Repository: let-sunny/canicode
Length of output: 106
Use pathToFileURL instead of manual file:// concatenation.
Line 45 builds a file URL via string interpolation, which doesn't properly encode special characters (spaces, etc.) or handle Windows path separators. Use pathToFileURL(fontPath).href from the node:url module to generate correct file URLs.
Suggested fix
+import { pathToFileURL } from "node:url";
import { existsSync } from "node:fs";
import { resolve } from "node:path";
@@
- const fontCss = `@font-face { font-family: "Inter"; src: url("file://${fontPath}") format("woff2"); font-weight: 100 900; }`;
+ const fontUrl = pathToFileURL(fontPath).href;
+ const fontCss = `@font-face { font-family: "Inter"; src: url("${fontUrl}") format("woff2"); font-weight: 100 900; }`;📝 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
| const fontCss = `@font-face { font-family: "Inter"; src: url("file://${fontPath}") format("woff2"); font-weight: 100 900; }`; | |
| import { pathToFileURL } from "node:url"; | |
| import { existsSync } from "node:fs"; | |
| import { resolve } from "node:path"; | |
| const fontUrl = pathToFileURL(fontPath).href; | |
| const fontCss = `@font-face { font-family: "Inter"; src: url("${fontUrl}") format("woff2"); font-weight: 100 900; }`; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/core/engine/html-utils.ts` at line 45, Replace the manual file:// string
interpolation when building fontCss with a proper file URL: import pathToFileURL
from 'node:url' (or use require) and call pathToFileURL(fontPath).href to
produce the URL; update the construction of fontCss (the constant named fontCss
that currently uses `file://${fontPath}`) to use that href so special characters
and Windows paths are encoded correctly.
…191) - core/engine/ → analysis only (rule-engine, scoring, loader, config-store) - core/comparison/ → visual-compare, visual-compare-helpers, html-utils - core/utils/ → fixture-helpers - Update all imports across codebase and CLAUDE.md https://claude.ai/code/session_015dAVYmRnXTxnYLwQcZDqPf
- Add negative delta test case for stripDeltaToDifficulty - Guard against directories in copyFixtureImages (withFileTypes) - Use pathToFileURL for font URL construction - Harden sanitizeHtml: cover unquoted on* handlers and src/xlink:href - Guard err type before reading .status in callApi catch - Trim whitespace from API key in requireApiKey - Fix markdown blank line before fenced code block in CLAUDE.md https://claude.ai/code/session_015dAVYmRnXTxnYLwQcZDqPf
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/comparison/html-utils.ts`:
- Around line 34-37: The current replace call in html-utils.ts uses a regex that
matches attributes but always substitutes ' href="#"', which wrongly changes
other attributes like src to href; update the regex to capture the attribute
name and use a backreference in the replacement so the same attribute is
preserved (e.g., change the pattern to capture the attribute name like
/(\b(?:href|src|xlink:href))\s*=\s*(?:"\s*javascript:[^"]*"|'\s*javascript:[^']*'|javascript:[^\s>]+)/gi
and change the replacement to '$1="#"'), keeping the rest of the surrounding
code in result.replace unchanged.
🪄 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: 1f711d34-19ba-407a-9ea9-bb9d4a79d2aa
📒 Files selected for processing (15)
CLAUDE.mdsrc/agents/code-renderer.tssrc/cli/commands/visual-compare.tssrc/core/comparison/html-utils.tssrc/core/comparison/visual-compare-helpers.tssrc/core/comparison/visual-compare.test.tssrc/core/comparison/visual-compare.tssrc/core/design-tree/delta.test.tssrc/core/utils/fixture-helpers.tssrc/experiments/ablation/helpers.tssrc/experiments/ablation/recompare.tssrc/experiments/ablation/run-condition.tssrc/experiments/ablation/run-phase1.tssrc/experiments/ablation/run-responsive.tssrc/mcp/server.ts
Use capture group to keep original attribute (src, href, xlink:href) instead of always replacing with href="#". https://claude.ai/code/session_015dAVYmRnXTxnYLwQcZDqPf
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/comparison/html-utils.ts`:
- Around line 44-46: The path resolution in injectLocalFont() uses
resolve("assets/fonts/Inter.var.woff2") which is cwd-dependent; change it to
compute a module-relative path using import.meta.url (e.g., new
URL('../../assets/fonts/Inter.var.woff2', import.meta.url).pathname or similar)
so the file lookup is independent of the process CWD, then use existsSync on
that resolved path; update the fontPath assignment in injectLocalFont to use
this import.meta.url-based resolution so the function works when re-exported and
called from different contexts.
🪄 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: 90e4c578-fb91-40ca-9e5a-e34bdf61d571
📒 Files selected for processing (1)
src/core/comparison/html-utils.ts
let-sunny
pushed a commit
that referenced
this pull request
Mar 30, 2026
- Add Number.isFinite guard in stripDeltaToDifficulty to reject NaN/Infinity - Fix MD031 markdown lint (blank line after fenced code block in CLAUDE.md) - Note: html-utils.ts and fixture-helpers.ts fixes are on main (#193), not this branch https://claude.ai/code/session_01L3j7oeNw6hKdvX4HJnrGwE
This was referenced Mar 31, 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
2 participants
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
#191 의 1단계: 리팩토링 + 기반 작업. ablation을 calibration에 통합하기 전에, 공용 모듈을 추출하고 디렉토리 구조를 정리합니다.
2단계 작업은 #194 에서 진행 예정.
이 PR에서 한 것
1.
src/core/design-tree/디렉토리 신설engine/design-tree.ts→design-tree/design-tree.ts(design-tree 생성)engine/design-tree-strip.ts→design-tree/strip.ts(strip 함수)design-tree/delta.ts신규 —stripDeltaToDifficulty(): similarity delta → Difficulty 매핑 (feat: integrate ablation into calibration pipeline for objective difficulty measurement #191 Step 3 기반)design-tree/index.ts— 통합 re-export이유: design-tree 관련 파일이 engine/ 안에 rule-engine, scoring, visual-compare 등과 섞여 있었음. 순수 데이터 변환 모듈로 분리.
2.
src/experiments/ablation/으로 이동src/agents/ablation/→src/experiments/ablation/helpers.ts,run-phase1.ts,run-condition.ts,run-responsive.ts,recompare.ts이유: ablation 스크립트는
npx tsx로 직접 실행하는 독립 실험이지, calibration pipeline의 에이전트가 아님. agents/에 있으면 역할이 혼동됨.3. 비교 파이프라인 통합
compareScreenshots()에CompareOptions추가:sizeMismatch: "pad" | "crop"— 캘리브레이션은 pad(기존), 실험은 crop(기존)threshold: number— pixelmatch threshold (기존 0.1 하드코딩 → 옵션화)cropPng()헬퍼 추가 (기존padPng()과 대칭)renderAndCompare()—ablation/helpers.ts에서core/engine/visual-compare.ts로 이동. 캘리브레이션 + 실험 공용.이유: 같은 "렌더 → 비교" 작업인데 crop/pad 방식이 두 곳에 하드코딩. 옵션 기반으로 통합하면 어디서든 원하는 방식 선택 가능.
4. 공용 유틸 추출
core/engine/html-utils.ts신규 —extractHtml,sanitizeHtml,injectLocalFont,processHtmlcore/engine/fixture-helpers.ts신규 —getDesignTreeOptions,getFixtureScreenshotPath,copyFixtureImagesexperiments/ablation/helpers.ts— 위 공용 함수 제거, re-export만 유지. API 호출/실험 설정만 남음.이유: HTML 처리와 fixture 관리는 실험 전용이 아닌 공통 기능. 다음 PR에서 calibration이 strip 실행할 때 바로 사용 가능.
이 PR에서 하지 않은 것 (다음 PR → #194)
stripDeltas필드)MIN_RATIO_SAMPLES등)디렉토리 구조 (변경 후)
Test plan
pnpm build— 빌드 통과pnpm test:run— 38 files, 654 tests 통과pnpm lint— tsc --noEmit 통과https://claude.ai/code/session_015dAVYmRnXTxnYLwQcZDqPf
Summary by CodeRabbit
Documentation
New Features
Refactor