feat(cli): Add --watch flag for automatic re-packing on file changes#1466
feat(cli): Add --watch flag for automatic re-packing on file changes#1466PAMulligan wants to merge 14 commits intoyamadashy:mainfrom
Conversation
Ensures --watch + --stdout conflict error message is visible to the user instead of being suppressed by stdout's SILENT log level.
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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:
📝 WalkthroughWalkthroughAdds a CLI watch mode that runs an initial pack, starts a chokidar watcher over resolved directories, triggers debounced (300ms) serialized rebuilds on file add/change/unlink, logs rebuild timestamps, enforces mutual-exclusion rules, supports graceful shutdown via AbortSignal or SIGINT/SIGTERM, and includes tests and README docs. Changes
Sequence DiagramsequenceDiagram
participant User as User/CLI
participant CliRun as cliRun
participant WatchAction as runWatchAction
participant Chokidar as Chokidar
participant Packager as Packager
participant Reporter as Reporter
User->>CliRun: run(['path'], cwd, {watch: true})
CliRun->>CliRun: Validate --watch compatibility
CliRun->>WatchAction: runWatchAction(directories, cwd, options)
WatchAction->>WatchAction: Load config & migrations
WatchAction->>Packager: pack() initial run
Packager->>Reporter: Report initial results
WatchAction->>Chokidar: Create watcher (ignoreInitial, awaitWriteFinish, ignored)
Chokidar->>WatchAction: File add/change/unlink events
WatchAction->>WatchAction: Schedule debounced rebuild (300ms)
WatchAction->>Packager: pack() after debounce
Packager->>Reporter: Report rebuild results
Reporter->>User: Log "Rebuilt at ..."
User->>WatchAction: Signal abort (Ctrl+C / AbortSignal)
WatchAction->>Chokidar: Close watcher
WatchAction->>User: Exit gracefully
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new watch mode to Repomix, enabling automatic re-packing upon file changes using the chokidar library. It includes the implementation of a watch action with debouncing, CLI integration with conflict validation, and comprehensive tests. Review feedback identifies a critical bug where file resolution fails when multiple directories are watched and notes that the current implementation fails to detect newly created files. Additionally, concerns were raised regarding the use of global process event listeners, which could cause issues when the tool is used as a library.
| // Resolve safeFilePaths against the first target path for watching | ||
| const rootDir = targetPaths[0]; | ||
| const absoluteWatchPaths = packResult.safeFilePaths.map((filePath) => path.resolve(rootDir, filePath)); | ||
|
|
||
| // Set up chokidar watcher | ||
| const watcher = resolvedDeps.watch(absoluteWatchPaths, { | ||
| ignoreInitial: true, | ||
| awaitWriteFinish: { stabilityThreshold: 100 }, | ||
| }); |
There was a problem hiding this comment.
The current implementation of the file watcher has two significant issues:
- Multiple Directories Bug: It resolves all file paths against
targetPaths[0]. If multiple directories are provided as positional arguments (e.g.,repomix dir1 dir2), files belonging todir2will be incorrectly resolved relative todir1, causing the watcher to fail for those files. - New File Detection: By watching a fixed list of files (
safeFilePaths) discovered during the initial pack, the watcher cannot detect newly created files. This contradicts the PR description which states that it handles created files.
Watching the targetPaths (directories) directly solves both issues. It ensures all roots are covered and allows Chokidar to detect new files within those directories. Note that we should ignore the output file to prevent an infinite rebuild loop.
// Set up chokidar watcher on target directories to catch new files and handle multiple roots
const watcher = resolvedDeps.watch(targetPaths, {
ignoreInitial: true,
awaitWriteFinish: { stabilityThreshold: 100 },
ignored: config.output.filePath ? path.resolve(cwd, config.output.filePath) : undefined,
});| } else { | ||
| process.on('SIGINT', async () => { | ||
| await cleanup(); | ||
| process.exit(0); | ||
| }); | ||
| process.on('SIGTERM', async () => { | ||
| await cleanup(); | ||
| process.exit(0); | ||
| }); | ||
| } |
There was a problem hiding this comment.
When runWatchAction is used as a library (which is a supported use case for Repomix), attaching global listeners to process.on('SIGINT') and process.on('SIGTERM') that call process.exit(0) is problematic. It will terminate the entire host process when the user intended to only stop the watch action.
Additionally, if this function is called multiple times in a long-running process (e.g., during tests or in a wrapper), it will leak event listeners on the process object. Consider only attaching these listeners if no AbortSignal is provided and ensure they are cleaned up properly.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/cli/actions/watchAction.ts`:
- Around line 79-87: The watcher is using the static packResult.safeFilePaths
snapshot and resolving each entry against targetPaths[0], which prevents
observing newly created files and mis-roots when multiple input directories are
provided; change the watch input to use the selected watch roots/patterns (e.g.
the original roots/selection used to compute safeFilePaths or targetPaths array
itself) instead of packResult.safeFilePaths, ensure you pass all targetPaths
(not just targetPaths[0]) or their corresponding glob roots into
resolvedDeps.watch, and add the generated output path to the watch ignore list
so rebuilds don’t loop (keep existing options like ignoreInitial and
awaitWriteFinish).
In `@src/cli/cliRun.ts`:
- Around line 242-253: The watch-mode validation misses positional remote URLs,
so extend the existing check in runCli (the block that tests options.watch and
options.remote/stdout/stdin) to also scan parsed positional arguments (e.g.,
options._ or the parsed positional array) for remote URLs and reject them;
implement a small predicate (isRemoteUrl) that matches common remote forms
(https?:// or git@) and if any positional matches, throw the same
RepomixError('--watch cannot be used with --remote. Watch mode only works with
local directories.').
🪄 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: CHILL
Plan: Pro
Run ID: 550a0ece-5160-4c8b-8e30-c7fc04aa1228
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (6)
README.mdpackage.jsonsrc/cli/actions/watchAction.tssrc/cli/cliRun.tssrc/cli/types.tstests/cli/actions/watchAction.test.ts
| // Validate --watch conflicts early, before log level changes can suppress error messages | ||
| if (options.watch) { | ||
| if (options.remote) { | ||
| throw new RepomixError('--watch cannot be used with --remote. Watch mode only works with local directories.'); | ||
| } | ||
| if (options.stdout) { | ||
| throw new RepomixError('--watch cannot be used with --stdout. Watch mode writes to a file.'); | ||
| } | ||
| if (options.stdin) { | ||
| throw new RepomixError('--watch cannot be used with --stdin. Watch mode discovers files automatically.'); | ||
| } | ||
| } |
There was a problem hiding this comment.
Reject --watch with positional remote URLs too.
This only guards options.remote. repomix -w https://github.com/... still falls through to the explicit-URL auto-detection later in runCli, so watch mode is silently ignored instead of producing the documented conflict error.
Suggested fix
// Validate --watch conflicts early, before log level changes can suppress error messages
if (options.watch) {
- if (options.remote) {
+ const positionalRemote = directories.length === 1 && isExplicitRemoteUrl(directories[0]);
+ if (options.remote || positionalRemote) {
throw new RepomixError('--watch cannot be used with --remote. Watch mode only works with local directories.');
}
if (options.stdout) {
throw new RepomixError('--watch cannot be used with --stdout. Watch mode writes to a file.');
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/cli/cliRun.ts` around lines 242 - 253, The watch-mode validation misses
positional remote URLs, so extend the existing check in runCli (the block that
tests options.watch and options.remote/stdout/stdin) to also scan parsed
positional arguments (e.g., options._ or the parsed positional array) for remote
URLs and reject them; implement a small predicate (isRemoteUrl) that matches
common remote forms (https?:// or git@) and if any positional matches, throw the
same RepomixError('--watch cannot be used with --remote. Watch mode only works
with local directories.').
|
Thanks for the thorough reviews! All valid points. I'm working on a follow-up commit to address these:
Will push the fixes shortly. |
- Watch directories instead of individual files to detect new files - Ignore output file path to prevent infinite rebuild loops - Add rebuild guard to prevent concurrent packs with pending queue - Use named signal handlers with cleanup to avoid listener leaks - Remove process.exit() calls for library compatibility
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| if (options.watch) { | ||
| if (options.remote) { | ||
| throw new RepomixError('--watch cannot be used with --remote. Watch mode only works with local directories.'); | ||
| } | ||
| if (options.stdout) { | ||
| throw new RepomixError('--watch cannot be used with --stdout. Watch mode writes to a file.'); | ||
| } | ||
| if (options.stdin) { | ||
| throw new RepomixError('--watch cannot be used with --stdin. Watch mode discovers files automatically.'); | ||
| } | ||
| if (options.splitOutput) { | ||
| throw new RepomixError( | ||
| '--watch cannot be used with --split-output. Watch mode does not yet support split output files.', | ||
| ); | ||
| } | ||
| if (directories.length === 1 && isExplicitRemoteUrl(directories[0])) { | ||
| throw new RepomixError('--watch cannot be used with remote URLs. Watch mode only works with local directories.'); | ||
| } | ||
| } |
There was a problem hiding this comment.
🟡 Watch mode silently ignores --skill-generate instead of rejecting the incompatible combination
The --watch conflict validation in src/cli/cliRun.ts:243-261 checks for --remote, --stdout, --stdin, and --split-output, but does not check for --skill-generate. If a user runs repomix --watch --skill-generate myskill, the config's skillGenerate is set via mergeConfigs, but watchAction.ts:106 calls pack(targetPaths, config, handleProgress) without passing PackOptions.skillDir. The pack function at src/core/packager.ts:171 checks config.skillGenerate !== undefined && options.skillDir — since options.skillDir is undefined, it silently falls through to normal output generation. The user gets regular packed output instead of skill output with no error or warning.
| if (options.watch) { | |
| if (options.remote) { | |
| throw new RepomixError('--watch cannot be used with --remote. Watch mode only works with local directories.'); | |
| } | |
| if (options.stdout) { | |
| throw new RepomixError('--watch cannot be used with --stdout. Watch mode writes to a file.'); | |
| } | |
| if (options.stdin) { | |
| throw new RepomixError('--watch cannot be used with --stdin. Watch mode discovers files automatically.'); | |
| } | |
| if (options.splitOutput) { | |
| throw new RepomixError( | |
| '--watch cannot be used with --split-output. Watch mode does not yet support split output files.', | |
| ); | |
| } | |
| if (directories.length === 1 && isExplicitRemoteUrl(directories[0])) { | |
| throw new RepomixError('--watch cannot be used with remote URLs. Watch mode only works with local directories.'); | |
| } | |
| } | |
| // Validate --watch conflicts early, before log level changes can suppress error messages | |
| if (options.watch) { | |
| if (options.remote) { | |
| throw new RepomixError('--watch cannot be used with --remote. Watch mode only works with local directories.'); | |
| } | |
| if (options.stdout) { | |
| throw new RepomixError('--watch cannot be used with --stdout. Watch mode writes to a file.'); | |
| } | |
| if (options.stdin) { | |
| throw new RepomixError('--watch cannot be used with --stdin. Watch mode discovers files automatically.'); | |
| } | |
| if (options.splitOutput) { | |
| throw new RepomixError( | |
| '--watch cannot be used with --split-output. Watch mode does not yet support split output files.', | |
| ); | |
| } | |
| if (options.skillGenerate !== undefined) { | |
| throw new RepomixError('--watch cannot be used with --skill-generate. Watch mode does not support skill generation.'); | |
| } | |
| if (directories.length === 1 && isExplicitRemoteUrl(directories[0])) { | |
| throw new RepomixError('--watch cannot be used with remote URLs. Watch mode only works with local directories.'); | |
| } | |
| } |
Was this helpful? React with 👍 or 👎 to provide feedback.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
- Add early-return guard before any await to prevent race conditions - Skip chokidar import when mock watch function is provided - Extend ignored patterns to include defaultIgnoreList and custom patterns - Add shuttingDown flag to prevent pendingRebuild during shutdown - Reorder cleanup to close watcher before resolving keepAlive - Update test assertions for array-based ignore patterns Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
| if (resolvedDeps.signal) { | ||
| resolvedDeps.signal.addEventListener('abort', () => { | ||
| cleanup(); | ||
| }); | ||
| } else { | ||
| process.on('SIGINT', onSigint); | ||
| process.on('SIGTERM', onSigterm); | ||
| } | ||
|
|
||
| // Keep alive — wait until signal is aborted (in tests) or cleanup resolves the promise | ||
| if (resolvedDeps.signal) { | ||
| await new Promise<void>((resolve) => { | ||
| if (resolvedDeps.signal?.aborted) { | ||
| resolve(); | ||
| return; | ||
| } | ||
| resolvedDeps.signal?.addEventListener('abort', () => resolve()); | ||
| }); |
There was a problem hiding this comment.
🟡 Signal-path cleanup races with keep-alive resolve, allowing runWatchAction to return before watcher is closed
When using an AbortSignal (the signal path), two separate abort event listeners are registered: one at line 195 that calls cleanup() without awaiting it, and another at line 210 that immediately calls resolve(). When abort fires, both listeners execute: cleanup() starts its async work (including await watcher.close() at line 183), but the second listener resolves the keep-alive promise immediately. This means runWatchAction() returns before watcher.close() finishes, violating the stated invariant in the comment at lines 181-182: "Close watcher before resolving keepAlive so callers don't see runWatchAction() resolve until shutdown is truly finished." In tests, the mock close resolves synchronously so this is masked, but the semantic guarantee is broken and in real usage with a real AbortSignal, resources may not be fully cleaned up when the function returns.
How the two listeners interact on abort
Listener 1 (line 195–197): cleanup() is called but not awaited — starts async teardown.
Listener 2 (line 210): resolve() is called synchronously — resolves the keep-alive promise.
The keep-alive promise at line 205 resolves before cleanup() finishes because watcher.close() is still pending. Meanwhile, keepAliveResolve is null in the signal path (it's only set at line 214 in the non-signal branch), so cleanup()'s keepAliveResolve?.() at line 184 is a no-op.
| if (resolvedDeps.signal) { | |
| resolvedDeps.signal.addEventListener('abort', () => { | |
| cleanup(); | |
| }); | |
| } else { | |
| process.on('SIGINT', onSigint); | |
| process.on('SIGTERM', onSigterm); | |
| } | |
| // Keep alive — wait until signal is aborted (in tests) or cleanup resolves the promise | |
| if (resolvedDeps.signal) { | |
| await new Promise<void>((resolve) => { | |
| if (resolvedDeps.signal?.aborted) { | |
| resolve(); | |
| return; | |
| } | |
| resolvedDeps.signal?.addEventListener('abort', () => resolve()); | |
| }); | |
| // Keep alive — wait until signal is aborted (in tests) or cleanup resolves the promise | |
| if (resolvedDeps.signal) { | |
| await new Promise<void>((resolve) => { | |
| if (resolvedDeps.signal?.aborted) { | |
| cleanup().then(resolve); | |
| return; | |
| } | |
| resolvedDeps.signal?.addEventListener('abort', () => { | |
| cleanup().then(resolve); | |
| }); | |
| }); | |
| } else { | |
| process.on('SIGINT', onSigint); | |
| process.on('SIGTERM', onSigterm); | |
| await new Promise<void>((resolve) => { | |
| keepAliveResolve = resolve; | |
| }); | |
| } |
Was this helpful? React with 👍 or 👎 to provide feedback.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
♻️ Duplicate comments (2)
src/cli/actions/watchAction.ts (2)
132-165:⚠️ Potential issue | 🟠 Major
scheduleRebuild()can still start work after shutdown begins.Line 175 flips
shuttingDown, but Line 132 never checks it before arming a fresh timer, and Line 136 never re-checks it beforerunPack(). A late chokidar event whilewatcher.close()is draining can still kick off one more rebuild after Ctrl+C.Suggested guard
const scheduleRebuild = () => { + if (shuttingDown) { + return; + } if (debounceTimer !== null) { clearTimeout(debounceTimer); } debounceTimer = setTimeout(async () => { debounceTimer = null; + if (shuttingDown) { + return; + } if (isRebuilding) { pendingRebuild = true; return; } @@ const cleanup = async () => { shuttingDown = true; if (debounceTimer !== null) { clearTimeout(debounceTimer); + debounceTimer = null; }Also applies to: 174-178
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli/actions/watchAction.ts` around lines 132 - 165, scheduleRebuild() can still kick off a rebuild after shutdown; add guards to avoid arming or executing work when shuttingDown is true: before creating/setting debounceTimer in scheduleRebuild() return early if shuttingDown, and inside the setTimeout callback re-check shuttingDown (and return without setting isRebuilding or calling runPack) to prevent starting work while watcher.close() is draining; ensure pendingRebuild is cleared when shuttingDown and that existing timers are cleared during shutdown so no late timers fire and call runPack().
174-185:⚠️ Potential issue | 🟠 MajorAbort-driven shutdown can still leak the watcher and return early.
If the signal flips after startup begins but before Line 195 registers the listener, that late handler never fires and the Line 206 fast-path returns without calling
cleanup(), leaving the watcher open. Even in the non-racy path, this branch waits for the abort event rather than the async cleanup, sorunWatchAction()can resolve before Line 183 finisheswatcher.close(). Please funnel both branches through one named{ once: true }abort handler / cleanup promise and await that before returning.Run this to confirm the late-listener behavior and inspect the current shutdown branch. Expect
lateListenerFiredto befalse.#!/bin/bash set -euo pipefail echo "Current shutdown logic:" sed -n '174,211p' src/cli/actions/watchAction.ts echo echo "AbortSignal late-listener check:" node <<'NODE' const controller = new AbortController(); controller.abort(); let lateListenerFired = false; controller.signal.addEventListener('abort', () => { lateListenerFired = true; }); setImmediate(() => { console.log(JSON.stringify({ aborted: controller.signal.aborted, lateListenerFired }, null, 2)); }); NODEAlso applies to: 194-211
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli/actions/watchAction.ts` around lines 174 - 185, The shutdown logic can miss racey aborts and resolve before watcher.close() completes; refactor runWatchAction to funnel all shutdown paths through a single named abort handler that runs cleanup(), register the AbortSignal listener with { once: true } (so late handlers fire deterministically), remove the duplicated fast-path that resolves without awaiting cleanup, and ensure the function awaits the cleanup promise (which should call watcher.close(), removeListener for onSigint/onSigterm, set shuttingDown and invoke keepAliveResolve) before returning; use the existing cleanup function name and call sites (cleanup, watcher.close, onSigint, onSigterm, keepAliveResolve, runWatchAction) to locate and replace the early-return branch so all exits await the same cleanup promise.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/cli/actions/watchAction.ts`:
- Around line 132-165: scheduleRebuild() can still kick off a rebuild after
shutdown; add guards to avoid arming or executing work when shuttingDown is
true: before creating/setting debounceTimer in scheduleRebuild() return early if
shuttingDown, and inside the setTimeout callback re-check shuttingDown (and
return without setting isRebuilding or calling runPack) to prevent starting work
while watcher.close() is draining; ensure pendingRebuild is cleared when
shuttingDown and that existing timers are cleared during shutdown so no late
timers fire and call runPack().
- Around line 174-185: The shutdown logic can miss racey aborts and resolve
before watcher.close() completes; refactor runWatchAction to funnel all shutdown
paths through a single named abort handler that runs cleanup(), register the
AbortSignal listener with { once: true } (so late handlers fire
deterministically), remove the duplicated fast-path that resolves without
awaiting cleanup, and ensure the function awaits the cleanup promise (which
should call watcher.close(), removeListener for onSigint/onSigterm, set
shuttingDown and invoke keepAliveResolve) before returning; use the existing
cleanup function name and call sites (cleanup, watcher.close, onSigint,
onSigterm, keepAliveResolve, runWatchAction) to locate and replace the
early-return branch so all exits await the same cleanup promise.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7f6117b2-6442-4f1a-9d6f-9096e3729592
📒 Files selected for processing (2)
src/cli/actions/watchAction.tstests/cli/actions/watchAction.test.ts
- Add shuttingDown guard at top of scheduleRebuild() to prevent new timers
- Re-check shuttingDown inside timer callback before calling runPack()
- Add cleanupStarted flag to prevent multiple cleanup() invocations
- Add cleanupDone flag to handle race where cleanup finishes before promise
- Register abort listener with { once: true } to avoid duplicate calls
- Check signal.aborted after registering listener for late-abort race
- Both signal and SIGINT/SIGTERM paths now await full cleanup completion
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
1 similar comment
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/cli/actions/watchAction.ts`:
- Around line 141-174: The shutdown logic must wait for any in-flight rebuild
started in the debounce timer (where runPack, reportResults and logger calls
occur) and always resolve the keep-alive via cleanupResolve; to fix it, create a
promise (e.g., activeRebuildPromise / activeRebuildResolve) that is assigned
when the debounce timer sets isRebuilding and cleared in the finally block after
runPack completes, update cleanup() to await watcher.close() and then await
activeRebuildPromise (if present) before resolving, and wrap the watcher.close()
+ activeRebuild await in a try-finally so cleanupResolve?.() is always called
even if watcher.close() rejects; ensure you still flip
pendingRebuild/isRebuilding and call scheduleRebuild() as before inside the
debounce finally block.
- Around line 120-124: The FSWatcher created by resolvedDeps.watch (assigned to
watcher) lacks an 'error' event handler; add watcher.on('error', handler) that
catches watcher errors (e.g., EMFILE, EACCES) and routes them into your existing
graceful shutdown logic (call the same shutdown/cleanup function you use for
SIGINT/SIGTERM or invoke the shutdown sequence used elsewhere) so the process
doesn't crash on unhandled 'error' events; ensure the handler logs the error via
the same logger used nearby and performs the same cleanup/exit path as other
shutdown triggers.
🪄 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: CHILL
Plan: Pro
Run ID: c3826b4e-55c8-4e97-8e1a-753639580e2c
📒 Files selected for processing (1)
src/cli/actions/watchAction.ts
| const watcher = resolvedDeps.watch(targetPaths, { | ||
| ignoreInitial: true, | ||
| awaitWriteFinish: { stabilityThreshold: 100 }, | ||
| ignored: watchIgnorePatterns, | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/cli/actions/watchAction.ts | head -200Repository: yamadashy/repomix
Length of output: 8259
🏁 Script executed:
cat -n src/cli/actions/watchAction.ts | sed -n '200,250p'Repository: yamadashy/repomix
Length of output: 1283
🏁 Script executed:
# Check if there are any existing error handlers on the watcher elsewhere
grep -n "watcher\.on" src/cli/actions/watchAction.tsRepository: yamadashy/repomix
Length of output: 192
🌐 Web query:
chokidar FSWatcher error event handling EventEmitter
💡 Result:
In chokidar, the FSWatcher class extends Node.js EventEmitter and emits an 'error' event when significant filesystem errors occur during watching. This includes errors like EMFILE (too many open files), UNKNOWN, EPERM, EACCES (unless ignorePermissionErrors is true), and others excluding common ignorable ones like ENOENT and ENOTDIR. To handle these errors and prevent unhandled error crashes, always attach an 'error' event listener: const watcher = chokidar.watch('path/to/watch', { ignorePermissionErrors: true // Optional: suppress permission errors }); watcher.on('error', (error) => { console.error('FS Watcher error:', error); // Handle gracefully, e.g., log, retry, or close watcher // watcher.close; }); The internal _handleError method decides whether to emit the error: - Ignores ENOENT, ENOTDIR - Ignores EPERM/EACCES if ignorePermissionErrors: true - Emits 'error' event for other significant errors - Common in large directories (e.g., node_modules) causing EMFILE Without a listener, errors can crash the process with "Emitted 'error' event on FSWatcher instance". Best practice: always listen for 'error' events, especially in production or watch mode. Example from official docs: watcher.on('error', (error) => log(Watcher error: ${error}));
Citations:
- 1: https://github.com/paulmillr/chokidar/blob/master/README.md
- 2: https://www.npmjs.com/package/chokidar
- 3: https://github.com/paulmillr/chokidar
- 4: https://github.com/paulmillr/chokidar/blob/main/src/index.ts
- 5: https://github.com/paulmillr/chokidar/blob/3.5.3/types/index.d.ts
Add error event handler to watcher.
FSWatcher extends Node.js EventEmitter and emits error events when filesystem monitoring fails (e.g., EMFILE, EACCES, EPERM). Without a listener, these errors crash the process with "Emitted 'error' event on FSWatcher instance". Hook error events into graceful shutdown to prevent watch mode from crashing on filesystem errors.
Suggested fix
watcher.on('change', scheduleRebuild);
watcher.on('add', scheduleRebuild);
watcher.on('unlink', scheduleRebuild);
+ watcher.on('error', (error) => {
+ logger.error('Watcher failed:', error);
+ void cleanup();
+ });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/cli/actions/watchAction.ts` around lines 120 - 124, The FSWatcher created
by resolvedDeps.watch (assigned to watcher) lacks an 'error' event handler; add
watcher.on('error', handler) that catches watcher errors (e.g., EMFILE, EACCES)
and routes them into your existing graceful shutdown logic (call the same
shutdown/cleanup function you use for SIGINT/SIGTERM or invoke the shutdown
sequence used elsewhere) so the process doesn't crash on unhandled 'error'
events; ensure the handler logs the error via the same logger used nearby and
performs the same cleanup/exit path as other shutdown triggers.
…utdown - Add error event handler to chokidar watcher to prevent uncaught exceptions when filesystem monitoring fails (EMFILE, EACCES, EPERM) - Track active rebuild promise and await it in cleanup() so shutdown doesn't complete while a pack operation is still running - Wrap cleanup body in try-finally to always settle the keep-alive promise, even if watcher.close() or the rebuild throws Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
| if (options.watch) { | ||
| if (options.remote) { | ||
| throw new RepomixError('--watch cannot be used with --remote. Watch mode only works with local directories.'); | ||
| } | ||
| if (options.stdout) { | ||
| throw new RepomixError('--watch cannot be used with --stdout. Watch mode writes to a file.'); | ||
| } | ||
| if (options.stdin) { | ||
| throw new RepomixError('--watch cannot be used with --stdin. Watch mode discovers files automatically.'); | ||
| } | ||
| if (options.splitOutput) { | ||
| throw new RepomixError( | ||
| '--watch cannot be used with --split-output. Watch mode does not yet support split output files.', | ||
| ); | ||
| } | ||
| if (directories.length === 1 && isExplicitRemoteUrl(directories[0])) { | ||
| throw new RepomixError('--watch cannot be used with remote URLs. Watch mode only works with local directories.'); | ||
| } | ||
| } |
There was a problem hiding this comment.
🔴 Missing conflict check: --watch --skill-generate silently ignores skill generation
When a user passes --watch --skill-generate, the watch action runs but --skill-generate is silently ignored. The buildCliConfig at src/cli/actions/defaultAction.ts:341 correctly adds skillGenerate to the config, and mergeConfigs includes it in the merged config. However, runPack in watchAction.ts:41 calls pack(targetPaths, config, handleProgress) without passing any PackOptions (specifically skillDir). In pack() at src/core/packager.ts:171, the skill generation path requires config.skillGenerate !== undefined && options.skillDir, which is false because skillDir is never set. The result is regular output (repomix-output.xml) instead of skill directory output, with no error or warning to the user. The validation block at src/cli/cliRun.ts:243-261 checks for --remote, --stdout, --stdin, and --split-output conflicts but misses --skill-generate.
| if (options.watch) { | |
| if (options.remote) { | |
| throw new RepomixError('--watch cannot be used with --remote. Watch mode only works with local directories.'); | |
| } | |
| if (options.stdout) { | |
| throw new RepomixError('--watch cannot be used with --stdout. Watch mode writes to a file.'); | |
| } | |
| if (options.stdin) { | |
| throw new RepomixError('--watch cannot be used with --stdin. Watch mode discovers files automatically.'); | |
| } | |
| if (options.splitOutput) { | |
| throw new RepomixError( | |
| '--watch cannot be used with --split-output. Watch mode does not yet support split output files.', | |
| ); | |
| } | |
| if (directories.length === 1 && isExplicitRemoteUrl(directories[0])) { | |
| throw new RepomixError('--watch cannot be used with remote URLs. Watch mode only works with local directories.'); | |
| } | |
| } | |
| if (options.watch) { | |
| if (options.remote) { | |
| throw new RepomixError('--watch cannot be used with --remote. Watch mode only works with local directories.'); | |
| } | |
| if (options.stdout) { | |
| throw new RepomixError('--watch cannot be used with --stdout. Watch mode writes to a file.'); | |
| } | |
| if (options.stdin) { | |
| throw new RepomixError('--watch cannot be used with --stdin. Watch mode discovers files automatically.'); | |
| } | |
| if (options.splitOutput) { | |
| throw new RepomixError( | |
| '--watch cannot be used with --split-output. Watch mode does not yet support split output files.', | |
| ); | |
| } | |
| if (options.skillGenerate !== undefined) { | |
| throw new RepomixError( | |
| '--watch cannot be used with --skill-generate. Watch mode does not support skill generation.', | |
| ); | |
| } | |
| if (directories.length === 1 && isExplicitRemoteUrl(directories[0])) { | |
| throw new RepomixError('--watch cannot be used with remote URLs. Watch mode only works with local directories.'); | |
| } | |
| } |
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/cli/actions/watchAction.ts (1)
212-219:⚠️ Potential issue | 🟠 MajorStill drain the active rebuild if
watcher.close()fails.If
watcher.close()rejects on Line 215, control jumps straight tofinallyand skips Lines 217-219. That letsrunWatchAction()resolve before the in-flight rebuild finishes, and the rejectedcleanup()promise is then fire-and-forget from the signal/abort handlers.Suggested fix
try { - // Close watcher before resolving so callers don't see - // runWatchAction() resolve until shutdown is truly finished - await watcher.close(); - // Wait for any in-flight rebuild to complete before resolving + try { + // Close watcher before resolving so callers don't see + // runWatchAction() resolve until shutdown is truly finished + await watcher.close(); + } catch (error) { + logger.error('Failed to close file watcher:', error); + } + // Wait for any in-flight rebuild to complete even if close() failed if (activeRebuildPromise) { await activeRebuildPromise; } } finally {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli/actions/watchAction.ts` around lines 212 - 219, If watcher.close() rejects, we must still await any in-flight activeRebuildPromise before letting runWatchAction() propagate the error; wrap the await watcher.close() call in a try/catch (or use Promise.allSettled semantics) so that on error you still await activeRebuildPromise (if set) and then rethrow the original error so cleanup() and signal/abort handlers don't leave the cleanup promise fire-and-forget; update the block around watcher.close(), activeRebuildPromise, runWatchAction(), and cleanup() to ensure activeRebuildPromise is always drained even when watcher.close() fails.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/cli/actions/watchAction.ts`:
- Around line 212-219: If watcher.close() rejects, we must still await any
in-flight activeRebuildPromise before letting runWatchAction() propagate the
error; wrap the await watcher.close() call in a try/catch (or use
Promise.allSettled semantics) so that on error you still await
activeRebuildPromise (if set) and then rethrow the original error so cleanup()
and signal/abort handlers don't leave the cleanup promise fire-and-forget;
update the block around watcher.close(), activeRebuildPromise, runWatchAction(),
and cleanup() to ensure activeRebuildPromise is always drained even when
watcher.close() fails.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d970fec0-7764-49e8-b591-06773ca468fa
📒 Files selected for processing (1)
src/cli/actions/watchAction.ts
Use separate try/catch blocks so that activeRebuildPromise is always awaited before resolving, regardless of whether watcher.close() throws. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/cli/actions/watchAction.ts (2)
233-238: Prefixcleanup()calls withvoidto silence floating-promise lint rules.The signal handlers call
cleanup()without awaiting the returned Promise. While functionally correct (the keep-alive promise waits forcleanupResolve), this may trigger@typescript-eslint/no-floating-promisesor similar lint rules.♻️ Suggested fix
const onSigint = () => { - cleanup(); + void cleanup(); }; const onSigterm = () => { - cleanup(); + void cleanup(); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli/actions/watchAction.ts` around lines 233 - 238, The signal handlers onSigint and onSigterm call cleanup() but leave its returned Promise unhandled; prefix those calls with void (e.g., void cleanup()) to explicitly ignore the Promise and satisfy `@typescript-eslint/no-floating-promises` linting while keeping the existing behavior of cleanup and the keep-alive promise resolution.
127-129: Consider triggering cleanup for critical watcher errors.The error handler prevents uncaught exceptions, which is good. However, for critical errors like
EMFILE(too many open files) orEACCES, the watcher may be in a broken state and continuing watch mode is not meaningful. Consider calling cleanup on error so the user knows watch mode has stopped.♻️ Optional enhancement
// Handle watcher errors (EMFILE, EACCES, EPERM, etc.) to prevent uncaught exceptions watcher.on('error', (error) => { logger.error('File watcher error:', error); + void cleanup(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli/actions/watchAction.ts` around lines 127 - 129, The file watcher error handler (watcher.on('error', (error) => { logger.error(...) })) should detect critical OS errors (e.g., error.code === 'EMFILE' or 'EACCES') and trigger the existing cleanup routine so watch mode stops and the user is informed; update the watcher.on('error') handler to log the error as before, then if the error.code matches critical list call cleanup() (or the cleanup function used in this module) and surface a clear message/exit flow so the process doesn't keep running in a broken watch state.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/cli/actions/watchAction.ts`:
- Around line 233-238: The signal handlers onSigint and onSigterm call cleanup()
but leave its returned Promise unhandled; prefix those calls with void (e.g.,
void cleanup()) to explicitly ignore the Promise and satisfy
`@typescript-eslint/no-floating-promises` linting while keeping the existing
behavior of cleanup and the keep-alive promise resolution.
- Around line 127-129: The file watcher error handler (watcher.on('error',
(error) => { logger.error(...) })) should detect critical OS errors (e.g.,
error.code === 'EMFILE' or 'EACCES') and trigger the existing cleanup routine so
watch mode stops and the user is informed; update the watcher.on('error')
handler to log the error as before, then if the error.code matches critical list
call cleanup() (or the cleanup function used in this module) and surface a clear
message/exit flow so the process doesn't keep running in a broken watch state.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9e6fb424-81b5-474a-8c51-adac849a0b8d
📒 Files selected for processing (1)
src/cli/actions/watchAction.ts
Summary of changes across all review roundsHi @yamadashy — this PR adds a Core feature (commits 1–6)
Review round 1 — Gemini + CodeRabbit + Devin
Review round 2 — CodeRabbit
Review round 3 — CodeRabbit
Review round 4 — CodeRabbit
Review round 5 — CodeRabbit
All tests pass ( Ready for your review when you get a chance! |
1 participant
Closes #1429
Adds a
--watch/-wflag that watches input files for changes and automatically re-packs when files are modified, created, or deleted.How it works
safeFilePaths)change,add,unlink), debounces for 300ms then re-runs the packCtrl+C(SIGINT/SIGTERM)Usage
Conflict validation
--watchcannot be combined with:--remote(watch mode only works with local directories)--stdout(watch mode writes to a file)--stdin(watch mode discovers files automatically)Clear error messages are shown when conflicting flags are used.
Implementation details
awaitWriteFinishfor stabilityremoteAction,defaultAction) with dependency injection viadepsparameter for testabilityimport()so the dependency is only loaded when--watchis usedAbortSignalfor deterministic testingChecklist
npm run test(1116 passed, 0 failures)npm run lint(0 errors, only pre-existing warnings)