fix: prevent infinite rebuild loop in dev-watch mode#6361
Conversation
The dev-watch script was causing an infinite rebuild loop because: 1. generate-version.ts writes to src/version.ts on every build 2. version.ts contains BUILD_TIME which changes every build 3. The watcher sees this change in cli/src and triggers another rebuild This fix addresses the issue in two ways: 1. dev-watch.js: Added ignoredFiles Set to skip watching version.ts/version.js 2. generate-version.ts: Made smart - only rewrites if version/name/description changed Also removed unused variables (stdoutData, stderrData, buildOutput) from dev-watch.js. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Claude finished @0xbbjoker's task in 1m 20s —— View job PR Review: #6361 - Fix infinite rebuild loop in dev-watch mode
✅ ApprovedGood fix for a real problem. The approach is sound and well-documented. Summary
Changes Reviewed
💡 SUGGESTION: Test coverageThe existing test file Consider adding tests like: test('shouldIgnoreFile ignores version files', () => {
const devWatchContent = readFileSync(DEV_SCRIPT, 'utf-8');
expect(devWatchContent).toContain('ignoredFiles');
expect(devWatchContent).toContain("'version.ts'");
expect(devWatchContent).toContain("'version.js'");
expect(devWatchContent).toContain('shouldIgnoreFile');
});This is not blocking since:
Minor Observations (Non-blocking)
VerdictThe fix correctly addresses the infinite loop by:
|
| function shouldIgnoreFile(filename) { | ||
| if (!filename) return true; | ||
| return ignoredFiles.has(path.basename(filename)); | ||
| } |
There was a problem hiding this comment.
The shouldIgnoreFile function uses path.basename(filename) which will ignore any file named version.ts or version.js in any subdirectory. This is too broad and causes a critical bug:
The watcher monitors packages/server/src, and there's a legitimate file at packages/server/src/api/system/version.ts (the version API endpoint). When this file changes, the watcher callback receives filename = 'api/system/version.ts', and path.basename('api/system/version.ts') returns 'version.ts', which matches the ignored set. This means changes to the server's version endpoint will not trigger a rebuild.
Fix: Only ignore version.ts/version.js at the root level of watched directories:
| function shouldIgnoreFile(filename) { | |
| if (!filename) return true; | |
| return ignoredFiles.has(path.basename(filename)); | |
| } | |
| function shouldIgnoreFile(filename) { | |
| if (!filename) return true; | |
| const basename = path.basename(filename); | |
| // Only ignore version files at root level (not in subdirectories like api/system/version.ts) | |
| return ignoredFiles.has(basename) && !filename.includes(path.sep); | |
| } |
This ensures only version.ts (not api/system/version.ts) is ignored.
Prompt To Fix With AI
This is a comment left during a code review.
Path: scripts/dev-watch.js
Line: 490:493
Comment:
The `shouldIgnoreFile` function uses `path.basename(filename)` which will ignore **any file named `version.ts` or `version.js` in any subdirectory**. This is too broad and causes a critical bug:
The watcher monitors `packages/server/src`, and there's a legitimate file at `packages/server/src/api/system/version.ts` (the version API endpoint). When this file changes, the watcher callback receives `filename = 'api/system/version.ts'`, and `path.basename('api/system/version.ts')` returns `'version.ts'`, which matches the ignored set. This means **changes to the server's version endpoint will not trigger a rebuild**.
**Fix**: Only ignore `version.ts`/`version.js` at the root level of watched directories:
```suggestion
function shouldIgnoreFile(filename) {
if (!filename) return true;
const basename = path.basename(filename);
// Only ignore version files at root level (not in subdirectories like api/system/version.ts)
return ignoredFiles.has(basename) && !filename.includes(path.sep);
}
```
This ensures only `version.ts` (not `api/system/version.ts`) is ignored.
How can I resolve this? If you propose a fix, please make it concise.| export const CLI_VERSION = '${versionInfo.version}'; | ||
| export const CLI_NAME = '${versionInfo.name}'; | ||
| export const CLI_DESCRIPTION = '${versionInfo.description}'; |
There was a problem hiding this comment.
The generated content uses template string interpolation without escaping special characters. If package.json contains a single quote in the name or description fields (e.g., "eliza's CLI"), it will:
- Generate syntactically invalid TypeScript (unescaped quote breaks the string)
- Cause the regex in
extractExistingVersionInfoto fail, triggering unnecessary rewrites
While unlikely given the current package.json format, consider escaping single quotes for robustness:
| export const CLI_VERSION = '${versionInfo.version}'; | |
| export const CLI_NAME = '${versionInfo.name}'; | |
| export const CLI_DESCRIPTION = '${versionInfo.description}'; | |
| export const CLI_VERSION = '${versionInfo.version.replace(/'/g, "\\'")}'; | |
| export const CLI_NAME = '${versionInfo.name.replace(/'/g, "\\'")}'; | |
| export const CLI_DESCRIPTION = '${versionInfo.description.replace(/'/g, "\\'")}'; |
Alternatively, use double quotes in the generated string literals to avoid the issue entirely.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/cli/src/scripts/generate-version.ts
Line: 98:100
Comment:
The generated content uses template string interpolation without escaping special characters. If `package.json` contains a single quote in the `name` or `description` fields (e.g., `"eliza's CLI"`), it will:
1. Generate syntactically invalid TypeScript (unescaped quote breaks the string)
2. Cause the regex in `extractExistingVersionInfo` to fail, triggering unnecessary rewrites
While unlikely given the current package.json format, consider escaping single quotes for robustness:
```suggestion
export const CLI_VERSION = '${versionInfo.version.replace(/'/g, "\\'")}';
export const CLI_NAME = '${versionInfo.name.replace(/'/g, "\\'")}';
export const CLI_DESCRIPTION = '${versionInfo.description.replace(/'/g, "\\'")}';
```
Alternatively, use double quotes in the generated string literals to avoid the issue entirely.
How can I resolve this? If you propose a fix, please make it concise.|
Claude finished @0xbbjoker's task in 1m 30s —— View job PR Review: #6361 - Fix infinite rebuild loop in dev-watch mode
❌ Changes RequestedThe fix correctly addresses the root cause of the infinite loop, but the implementation has a critical bug that will break hot reload for legitimate files. Summary
❌ CRITICAL:
|
| function shouldIgnoreFile(filename) { | ||
| if (!filename) return true; | ||
| return ignoredFiles.has(path.basename(filename)); | ||
| } |
There was a problem hiding this comment.
❌ CRITICAL BUG: This function ignores any file named version.ts in any subdirectory, not just the auto-generated one at cli/src/version.ts.
Impact: packages/server/src/api/system/version.ts (the version API endpoint) will never trigger rebuilds when changed.
Repro:
shouldIgnoreFile('api/system/version.ts')returnstruebecausepath.basename()returns'version.ts'
| function shouldIgnoreFile(filename) { | |
| if (!filename) return true; | |
| return ignoredFiles.has(path.basename(filename)); | |
| } | |
| function shouldIgnoreFile(filename) { | |
| if (!filename) return true; | |
| // Only ignore version files at root level (no directory separators) | |
| // This ignores 'version.ts' but NOT 'api/system/version.ts' | |
| return ignoredFiles.has(filename); | |
| } |
This fix works because the watcher callback receives the relative path from the watched directory. For root-level files, filename is just 'version.ts'. For nested files, filename is 'api/system/version.ts'.
| export const CLI_VERSION = '${versionInfo.version}'; | ||
| export const CLI_NAME = '${versionInfo.name}'; | ||
| export const CLI_DESCRIPTION = '${versionInfo.description}'; |
There was a problem hiding this comment.
💡 SUGGESTION: If package.json ever contains a single quote in the description (e.g., "Eliza's CLI"), this would generate invalid TypeScript:
export const CLI_DESCRIPTION = 'Eliza's CLI'; // Syntax error!Low risk since current package.json doesn't have quotes, but for robustness consider:
| export const CLI_VERSION = '${versionInfo.version}'; | |
| export const CLI_NAME = '${versionInfo.name}'; | |
| export const CLI_DESCRIPTION = '${versionInfo.description}'; | |
| export const CLI_VERSION = ${JSON.stringify(versionInfo.version)}; | |
| export const CLI_NAME = ${JSON.stringify(versionInfo.name)}; | |
| export const CLI_DESCRIPTION = ${JSON.stringify(versionInfo.description)}; |
This produces the same output for simple strings and handles edge cases safely.
The shouldIgnoreFile function was using path.basename() which would ignore ANY file named version.ts in any subdirectory, including legitimate files like packages/server/src/api/system/version.ts. Fixed by checking the filename directly without basename extraction. The watcher callback provides the relative path from the watched directory root, so root-level 'version.ts' will match but nested 'api/system/version.ts' will not. Addresses review comment: #6361 (comment) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Changed template string interpolation to use JSON.stringify() for all string values. This safely handles special characters like quotes and backslashes that could break the generated TypeScript syntax. Also updated extractExistingVersionInfo regex to support both single and double quoted strings for backwards compatibility. Addresses review comment: #6361 (comment) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
Claude finished @0xbbjoker's task in 49s —— View job PR Review: #6361 - Fix infinite rebuild loop in dev-watch mode
✅ ApprovedBoth issues from the previous review have been properly addressed. Summary
Fixes Verified1. ✅ Previous issue: Used Fix: Now checks exact filename match: return ignoredFiles.has(filename);This works because the watcher callback receives relative paths:
2. ✅ Safe string escaping ( Previous issue: Single quotes without escaping could break if package.json contained quotes. Fix: Now uses export const CLI_VERSION = ${JSON.stringify(versionInfo.version)};
export const CLI_NAME = ${JSON.stringify(versionInfo.name)};
export const CLI_DESCRIPTION = ${JSON.stringify(versionInfo.description)};Implementation QualityThe fix correctly addresses the root cause through defense in depth:
Good documentation in comments explaining why the ignore logic exists. |
|
Claude finished @0xbbjoker's task in 1m 10s —— View job PR Review: #6361 - Fix infinite rebuild loop in dev-watch mode
✅ ApprovedBoth issues from the previous review have been properly addressed. Summary
Fixes Verified1. ✅ Previous issue: Used Fix applied in commit function shouldIgnoreFile(filename) {
if (!filename) return true;
// Only ignore version files at root level (no directory separators)
// This ignores 'version.ts' but NOT 'api/system/version.ts'
return ignoredFiles.has(filename);
}Verification: The watcher watches
2. ✅ Safe string escaping ( Previous issue: Single quotes without escaping could break if package.json contained quotes. Fix applied in commit export const CLI_VERSION = ${JSON.stringify(versionInfo.version)};
export const CLI_NAME = ${JSON.stringify(versionInfo.name)};
export const CLI_DESCRIPTION = ${JSON.stringify(versionInfo.description)};Implementation QualityThe fix correctly addresses the root cause through defense in depth:
Good documentation in comments explaining why the ignore logic exists. |
The previous regex [^"']+ would stop at apostrophes, causing incorrect extraction for values like "It's awesome" (would extract just "It"). Updated to use a proper JSON string pattern that handles: - Apostrophes (unescaped in JSON strings) - Escaped double quotes (\") - Other escaped characters (\\, \n, etc.) Addresses review comment: #6361 (comment) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
Claude finished @0xbbjoker's task in 1m 4s —— View job PR Review: #6361 - Fix infinite rebuild loop in dev-watch mode
✅ ApprovedAll previous issues have been properly addressed. The fix is ready to merge. Summary
Issues Fixed (All Verified ✅)1. ✅ Previous issue: Used Fix applied: function shouldIgnoreFile(filename) {
if (!filename) return true;
// Only ignore version files at root level (no directory separators)
return ignoredFiles.has(filename);
}Verification: The watcher watches directories with
2. ✅ Safe string escaping ( Previous issue: Single quotes without escaping could break if package.json contained quotes. Fix applied: export const CLI_VERSION = ${JSON.stringify(versionInfo.version)};
export const CLI_NAME = ${JSON.stringify(versionInfo.name)};
export const CLI_DESCRIPTION = ${JSON.stringify(versionInfo.description)};3. ✅ Regex handles JSON-stringified values ( Previous issue: Regex pattern Fix applied: const jsonStringPattern = (name: string) =>
new RegExp(`export const ${name} = "((?:[^"\\\\]|\\\\.)*)"`);This pattern correctly handles:
Implementation QualityThe fix uses defense in depth:
Good documentation in comments explaining why the ignore logic exists. |
| existingInfo.version === newInfo.version && | ||
| existingInfo.name === newInfo.name && | ||
| existingInfo.description === newInfo.description | ||
| ); |
There was a problem hiding this comment.
JSON escape mismatch causes failed version comparison
Medium Severity
The extractExistingVersionInfo function extracts values from a file written with JSON.stringify(), which escapes special characters (quotes, backslashes, etc.). However, versionInfoMatches compares these extracted JSON-escaped strings directly against raw values from package.json. For example, a description containing "quoted" would be stored as \"quoted\" in the file, but compared against the unescaped "quoted" from package.json. This mismatch causes the comparison to always fail when metadata contains escapable characters, defeating the optimization and potentially reintroducing the infinite rebuild loop.
Summary
bun run devcaused bygenerate-version.tswriting tosrc/version.tson every buildChanges
ignoredFilesSet to skip watchingversion.tsandversion.jsfilesstdoutData,stderrData,buildOutput) from dev-watch.jsRoot Cause
generate-version.tswritesBUILD_TIME = '${new Date().toISOString()}'which changes every buildcli/src/version.tsversion.tswith a new timestampTest plan
bun run devfrom monorepo rootpackages/core/src/runtime.ts)version.ts is up to datemessage appears on subsequent builds🤖 Generated with Claude Code
Note
Prevents watch-mode rebuild loops caused by auto-generated version files.
packages/cli/src/scripts/generate-version.ts: Adds idempotent generation (extracts existingCLI_VERSION/NAME/DESCRIPTION, compares, and skips write if unchanged), usesJSON.stringifyfor safe string literals, and introduces small FS helpers.scripts/dev-watch.js: Ignores auto-generatedversion.ts/version.jsviashouldIgnoreFile, forwards build stdout/stderr directly (removes unused buffers), and logs ignored files; preserves TypeScript-only watching with debounce.Written by Cursor Bugbot for commit 6d45870. This will update automatically on new commits. Configure here.
Greptile Overview
Greptile Summary
This PR fixes an infinite rebuild loop in dev-watch mode caused by the auto-generated
version.tsfile changing on every build. The fix involves two approaches:Smart version generation (
generate-version.ts): Only rewritesversion.tsif version/name/description actually changed, not on every build. This meansBUILD_TIMEbecomes stale in dev mode (intentional tradeoff).Watcher ignore list (
dev-watch.js): Added ignore logic to skip watchingversion.tsandversion.jsfiles.Critical Issue Found
The watcher ignore implementation is too broad and will incorrectly ignore legitimate files. It uses
path.basename(filename)to match filenames, which means ANY file namedversion.tsin ANY subdirectory will be ignored, including:packages/server/src/api/system/version.ts(the version API endpoint)This breaks hot reload for legitimate source files that happen to be named
version.ts.Root Cause Analysis
The original problem was that
generate-version.tswritesBUILD_TIME = '${new Date().toISOString()}'which changes every build, triggering the watcher. The fix addresses this but introduces a new bug in the ignore logic.Additional Changes
stdoutData,stderrData,buildOutput) - good cleanupConfidence Score: 2/5
Important Files Changed
File Analysis
Sequence Diagram
sequenceDiagram participant DevWatch as dev-watch.js participant Builder as Build Process participant GenVer as generate-version.ts participant FS as File System participant Watcher as File Watcher Note over DevWatch: Initial Startup DevWatch->>Builder: Start CLI build Builder->>GenVer: Execute generate-version.ts GenVer->>FS: Read package.json FS-->>GenVer: version, name, description GenVer->>FS: Check existing version.ts alt version.ts exists with same version info GenVer->>GenVer: Skip write (no change) Note over GenVer: Prevents rebuild loop else version.ts missing or different GenVer->>FS: Write new version.ts Note over GenVer: Includes BUILD_TIME (stale in dev) end Builder-->>DevWatch: Build complete DevWatch->>Watcher: Start watching source directories Note over Watcher: User modifies source file Watcher->>Watcher: Detect file change Watcher->>Watcher: Check shouldIgnoreFile(filename) alt filename basename is "version.ts" or "version.js" Watcher->>Watcher: Ignore (too broad - BUG!) Note over Watcher: Also ignores api/system/version.ts else Other TypeScript file Watcher->>DevWatch: Trigger rebuild DevWatch->>Builder: Rebuild CLI Builder->>GenVer: Execute generate-version.ts Note over GenVer: Smart comparison prevents<br/>unnecessary writes end