feat: hot reload caplets serve config#18
Open
ian-pascoe wants to merge 8 commits into
Open
Conversation
|
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 hot-reload support for ChangesHot Reload Feature
Sequence DiagramssequenceDiagram
participant FileSystem
participant CapletsRuntime
participant DownstreamManager
participant OpenApiManager
participant GraphQLManager
participant MCP_Server
FileSystem->>CapletsRuntime: file change (config or caplet)
CapletsRuntime->>CapletsRuntime: scheduleReload() (debounced)
CapletsRuntime->>CapletsRuntime: reload() -> build new registry
CapletsRuntime->>DownstreamManager: updateRegistry(newRegistry)
CapletsRuntime->>OpenApiManager: updateRegistry(newRegistry)
CapletsRuntime->>GraphQLManager: updateRegistry(newRegistry)
CapletsRuntime->>DownstreamManager: closeServer(serverId) for changed MCP backends
CapletsRuntime->>OpenApiManager: invalidate(serverId) for changed OpenAPI endpoints
CapletsRuntime->>GraphQLManager: invalidate(serverId) for changed GraphQL endpoints
CapletsRuntime->>CapletsRuntime: reconcileTools() add/update/remove MCP tools
CapletsRuntime->>MCP_Server: registerTool / update / remove calls
CapletsRuntime->>CapletsRuntime: resetWatchers() recreate fs watchers
🎯 4 (Complex) | ⏱️ ~45 minutes
🚥 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)
Comment |
ian-pascoe
force-pushed
the
codex/hot-reload-caplets
branch
from
04ecb9a to
b75bbbf
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.changeset/quiet-rockets-serve.md:
- Line 5: Add a top-level Markdown heading to the changeset so it satisfies
markdownlint MD041: open the changeset for "quiet-rockets-serve" and insert a
leading H1 (e.g., "# quiet-rockets-serve" or a meaningful title) above the
existing body line "Add default hot reload for `caplets serve`, including live
config and Caplet file reconciliation without restarting the MCP process." to
ensure the file begins with a heading.
In `@docs/plans/2026-05-12-hot-reload.md`:
- Around line 13-69: Update the Markdown task headings so they use level-2
headings (##) instead of level-3 (###) to avoid MD001 warnings: change the
headings for "Task 1: Runtime Controller", "Task 2: Reload Reconciliation",
"Task 3: Filesystem Watching", "Task 4: Documentation", and "Task 5:
Verification" from ### to ## and ensure any related sublists remain correctly
nested under those updated headings.
In `@src/runtime.ts`:
- Around line 250-252: The loop currently skips setting up watches when
existsSync(entry.path) is false (entry.path), which causes missed reloads if the
directory appears later; instead, when entry.path is missing, register a watcher
on its parent directory (e.g., path.dirname(entry.path)) using fs.watch or your
chokidar instance (with ignoreInitial: true) to detect creation/rename events
and then invoke the same watch-setup routine you use for existing paths (so
reuse the code that runs when existsSync is true); ensure you remove the parent
watcher once the target entry.path is created and the real watch is established.
- Around line 234-243: The code only invalidates the cache for the new backend
when a caplet with the same serverId changes backend, leaving the previous
backend's cache stale; update the logic to explicitly invalidate caches for both
the previous and the new backend when a caplet changes (use before?.backend and
after?.backend), and still handle content changes by comparing
serializeCaplet(before) !== serializeCaplet(after); specifically, call
this.openapi.invalidate(serverId) and/or this.graphql.invalidate(serverId) for
whichever backend values are present on before and on after (and keep the
existing branch that invalidates both when !after).
- Around line 255-257: The current fs.watch call (watch(entry.path, {
persistent: true }, () => { this.scheduleReload(); })) does not recurse into
nested directories; replace it with a recursive watcher (preferably using
chokidar) that watches entry.path recursively and invokes this.scheduleReload()
on change events; add the chokidar import, create the watcher with options like
{ persistent: true, ignoreInitial: true } and subscribe to change/add/unlink (or
the 'all') events to call this.scheduleReload(), and ensure you close the
watcher when appropriate to avoid leaks.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 2487883c-925d-44be-aea3-0c06f4b0c198
📒 Files selected for processing (12)
.changeset/quiet-rockets-serve.mdREADME.mddocs/plans/2026-05-12-hot-reload.mdsrc/downstream.tssrc/graphql.tssrc/index.tssrc/openapi.tssrc/runtime.tstest/downstream.test.tstest/graphql.test.tstest/openapi.test.tstest/runtime.test.ts
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/downstream.ts (1)
224-229:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winGuard
transport.onerrorduring intentional shutdown.Line 224 can still mark a server
unavailableduringclose()/closeServer(), even though shutdown is intentional. Add the sameclosingshort-circuit used inonclose.Suggested patch
transport.onerror = (error: Error) => { + if (connection.closing) { + return; + } this.registry.setStatus( server.server, "unavailable", toSafeError(error, "SERVER_UNAVAILABLE"), ); };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/downstream.ts` around lines 224 - 229, The transport.onerror handler currently marks servers unavailable even during an intentional shutdown; modify transport.onerror (the same place that uses the onclose closing short-circuit) to return early when the instance is in the shutdown state (e.g., check this.closing or the same flag used by onclose/close()/closeServer()) before calling this.registry.setStatus(server.server, "unavailable", ...); ensure the guard mirrors the one in onclose so intentional shutdowns don't flip status.
♻️ Duplicate comments (4)
src/runtime.ts (3)
255-257:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse recursive watching (or chokidar) so nested Caplet edits trigger reloads.
fs.watchwithout recursion can miss deep file changes under Caplet subdirectories.In Node.js v22, does fs.watch(path, { recursive: false/default }) emit change events for files in nested subdirectories, or only direct children?🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/runtime.ts` around lines 255 - 257, The current fs.watch call that invokes this.scheduleReload() can miss nested Caplet file edits; replace the non-recursive watcher with a recursive watcher or a robust file-watcher library: either set fs.watch(entry.path, { persistent: true, recursive: true }) where supported, or switch to using chokidar.watch(entry.path, { persistent: true, ignoreInitial: true }) and listen for add/change/unlink events to call this.scheduleReload(); update the code path around the existing watch(...) invocation and ensure scheduleReload() is called on those events instead of relying on the default non-recursive fs.watch behavior.
224-243:⚠️ Potential issue | 🟠 Major | ⚡ Quick winInvalidate both previous and next backend caches when backend type changes on the same
serverId.Current logic invalidates OpenAPI/GraphQL mostly from
after?.backend, which can leave stale cache when a Caplet switches backend type (for example, OpenAPI → GraphQL).Suggested patch
for (const serverId of changedIds) { const before = previousCaplets.get(serverId); const after = nextCaplets.get(serverId); + const changed = serializeCaplet(before) !== serializeCaplet(after); + if (!changed) { + continue; + } - if ( - before && - before.backend === "mcp" && - serializeCaplet(before) !== serializeCaplet(after) - ) { + if (before?.backend === "mcp") { await this.downstream.closeServer(serverId); } - if (after?.backend === "openapi" && serializeCaplet(before) !== serializeCaplet(after)) { + if (before?.backend === "openapi" || after?.backend === "openapi" || !after) { this.openapi.invalidate(serverId); } - if (after?.backend === "graphql" && serializeCaplet(before) !== serializeCaplet(after)) { + if (before?.backend === "graphql" || after?.backend === "graphql" || !after) { this.graphql.invalidate(serverId); } - if (!after) { - this.openapi.invalidate(serverId); - this.graphql.invalidate(serverId); - } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/runtime.ts` around lines 224 - 243, The loop over changedIds currently only invalidates caches based on after?.backend which can leave stale caches when a caplet's backend changes; update the logic in the for (const serverId of changedIds) block (using previousCaplets, nextCaplets, serializeCaplet) to detect backend transitions by comparing before?.backend and after?.backend and call the appropriate invalidations for both sides: if before?.backend === "openapi" call this.openapi.invalidate(serverId), if before?.backend === "graphql" call this.graphql.invalidate(serverId), and likewise invalidate the backend indicated by after?.backend; retain the existing this.downstream.closeServer(serverId) behavior for MCP and still handle deletion (!after) by invalidating both openapi and graphql as now.
247-252:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon’t skip watch setup entirely when the target path is missing at startup.
If a watched directory is created later, hot reload will not attach until process restart.
Suggested patch
private resetWatchers(): void { this.closeWatchers(); + const attached = new Set<string>(); for (const entry of watchedPaths(this.paths)) { - if (!existsSync(entry.path)) { + const watchPath = existsSync(entry.path) ? entry.path : dirname(entry.path); + if (!existsSync(watchPath) || attached.has(watchPath)) { continue; } try { + attached.add(watchPath); this.watchers.push( - watch(entry.path, { persistent: true }, () => { + watch(watchPath, { persistent: true }, () => { this.scheduleReload(); }), );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/runtime.ts` around lines 247 - 252, The current resetWatchers method skips any watchedPaths entries when the target path doesn't exist, so new directories created later never get watched; update resetWatchers (and related closeWatchers/watchedPaths usage) to still register a placeholder listener for missing entries: watch the nearest existing parent (or use a filesystem-watch-for-creation helper) and on creation attach the real watcher for entry.path, ensuring you track and clean up these placeholder listeners in closeWatchers and reuse the same attach logic you use for existing paths so hot-reload will pick up directories created after startup.docs/plans/2026-05-12-hot-reload.md (1)
13-69:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix heading level increments to satisfy markdownlint MD001.
Task headers should be
##(not###) under the top-level title.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/plans/2026-05-12-hot-reload.md` around lines 13 - 69, Update the task section headings so they use level-2 markdown headers (##) instead of level-3 (###); specifically change "### Task 1: Runtime Controller", "### Task 2: Reload Reconciliation", "### Task 3: Filesystem Watching", "### Task 4: Documentation", and "### Task 5: Verification" to "## Task 1: Runtime Controller", "## Task 2: Reload Reconciliation", "## Task 3: Filesystem Watching", "## Task 4: Documentation", and "## Task 5: Verification" respectively so the heading hierarchy satisfies markdownlint MD001 while leaving the top-level title unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/runtime.ts`:
- Around line 112-121: close() can race with reloadOnce(): if close() runs while
reloadOnce() is executing, reloadOnce() may recreate watchers after shutdown;
fix by introducing a short lifecycle guard: set this.closed = true immediately
in close(), add a reload-in-progress flag (e.g., this.reloading) or Promise so
close() can await any in-flight reloadOnce() before calling
closeWatchers()/server/downstream.close(), and update reloadOnce() to check
this.closed (and/or this.reloading) at key points before reattaching watchers or
scheduling timers (using reloadTimer) and abort early if closed; reference the
methods/fields reloadOnce(), close(), closeWatchers(), reloadTimer, this.closed,
and server/downstream to locate where to add the guard and awaiting logic.
---
Outside diff comments:
In `@src/downstream.ts`:
- Around line 224-229: The transport.onerror handler currently marks servers
unavailable even during an intentional shutdown; modify transport.onerror (the
same place that uses the onclose closing short-circuit) to return early when the
instance is in the shutdown state (e.g., check this.closing or the same flag
used by onclose/close()/closeServer()) before calling
this.registry.setStatus(server.server, "unavailable", ...); ensure the guard
mirrors the one in onclose so intentional shutdowns don't flip status.
---
Duplicate comments:
In `@docs/plans/2026-05-12-hot-reload.md`:
- Around line 13-69: Update the task section headings so they use level-2
markdown headers (##) instead of level-3 (###); specifically change "### Task 1:
Runtime Controller", "### Task 2: Reload Reconciliation", "### Task 3:
Filesystem Watching", "### Task 4: Documentation", and "### Task 5:
Verification" to "## Task 1: Runtime Controller", "## Task 2: Reload
Reconciliation", "## Task 3: Filesystem Watching", "## Task 4: Documentation",
and "## Task 5: Verification" respectively so the heading hierarchy satisfies
markdownlint MD001 while leaving the top-level title unchanged.
In `@src/runtime.ts`:
- Around line 255-257: The current fs.watch call that invokes
this.scheduleReload() can miss nested Caplet file edits; replace the
non-recursive watcher with a recursive watcher or a robust file-watcher library:
either set fs.watch(entry.path, { persistent: true, recursive: true }) where
supported, or switch to using chokidar.watch(entry.path, { persistent: true,
ignoreInitial: true }) and listen for add/change/unlink events to call
this.scheduleReload(); update the code path around the existing watch(...)
invocation and ensure scheduleReload() is called on those events instead of
relying on the default non-recursive fs.watch behavior.
- Around line 224-243: The loop over changedIds currently only invalidates
caches based on after?.backend which can leave stale caches when a caplet's
backend changes; update the logic in the for (const serverId of changedIds)
block (using previousCaplets, nextCaplets, serializeCaplet) to detect backend
transitions by comparing before?.backend and after?.backend and call the
appropriate invalidations for both sides: if before?.backend === "openapi" call
this.openapi.invalidate(serverId), if before?.backend === "graphql" call
this.graphql.invalidate(serverId), and likewise invalidate the backend indicated
by after?.backend; retain the existing this.downstream.closeServer(serverId)
behavior for MCP and still handle deletion (!after) by invalidating both openapi
and graphql as now.
- Around line 247-252: The current resetWatchers method skips any watchedPaths
entries when the target path doesn't exist, so new directories created later
never get watched; update resetWatchers (and related closeWatchers/watchedPaths
usage) to still register a placeholder listener for missing entries: watch the
nearest existing parent (or use a filesystem-watch-for-creation helper) and on
creation attach the real watcher for entry.path, ensuring you track and clean up
these placeholder listeners in closeWatchers and reuse the same attach logic you
use for existing paths so hot-reload will pick up directories created after
startup.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: f45ef35d-efa3-42a5-8366-4156a7e346bf
📒 Files selected for processing (12)
.changeset/quiet-rockets-serve.mdREADME.mddocs/plans/2026-05-12-hot-reload.mdsrc/downstream.tssrc/graphql.tssrc/index.tssrc/openapi.tssrc/runtime.tstest/downstream.test.tstest/graphql.test.tstest/openapi.test.tstest/runtime.test.ts
|
| Filename | Overview |
|---|---|
| src/runtime.ts | New CapletsRuntime class: reconciles MCP tools, manages watcher lifecycle, and debounces reloads. Contains a deduplication bug in watchedPaths that silently drops the caplets watch reason for the user and project caplets directory, preventing recursive directory watching and watcher-refresh on rename events. |
| src/downstream.ts | Moves connection object creation before client.connect() so onclose/onerror handlers can reference it; adds closing flag to suppress restart logic on intentional close; adds closeServer() and updateRegistry() for hot-reload support. |
| src/graphql.ts | Adds updateRegistry() and invalidate() methods for hot-reload support; mutable registry field change is safe and well-encapsulated. |
| src/openapi.ts | Adds updateRegistry() and invalidate() methods for hot-reload support; mirrors the GraphQL changes consistently. |
| src/index.ts | Simplified entrypoint delegates server lifecycle to CapletsRuntime; clean refactor with no logic changes. |
| test/runtime.test.ts | New tests cover initial registration, add/update/remove reconciliation, last-known-good fallback, and mid-flight pending reload behavior; mock server correctly tracks registered tools. |
Comments Outside Diff (1)
-
src/runtime.ts, line 713-724 (link)User caplets directory always deduped to "config" reason
resolveCapletsRoot(configPath)is defined asdirname(configPath)(seesrc/config/paths.tsline 18), so the thirdwatchedPathsentry always resolves to the same path as the first entry.uniqueWatchedPathskeeps the first occurrence (reason: "config") and silently drops the{ ..., reason: "caplets" }entry.Concretely: with the default config at
~/.caplets/config.json,resolveCapletsRootreturns~/.caplets/, which is identical todirname(configPath). The "caplets" entry is deduped away, sowatchEntrynever reaches thereason === "caplets"branch,watchDirectoryTreeis never invoked for the user caplets directory, and rename events in~/.caplets/never triggerscheduleWatcherRefresh. Any caplet files stored in subdirectories under~/.caplets/will not be watched. The same collision occurs for the project caplets directory whenTRUST_PROJECT_CAPLETS_ENVis enabled.A straightforward fix is to prefer "caplets" over "config" when the same path appears multiple times — either by inserting the caplets entries before their config counterparts so that
uniqueWatchedPathskeeps the higher-priority reason, or by makinguniqueWatchedPathsmerge reasons (keeping "caplets" when duplicated).
Reviews (5): Last reviewed commit: "fix: tighten hot reload review edge case..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/downstream.ts (1)
209-227:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard stale transport callbacks from mutating newer connections
At Line 210,
onclosealways deletesthis.connectionsfor the server before checking whether this callback belongs to the active connection. If a reconnect happens quickly, an old transport callback can remove or mark unavailable the new connection.Suggested fix
transport.onclose = () => { - this.connections.delete(server.server); + const current = this.connections.get(server.server); + if (current === connection) { + this.connections.delete(server.server); + } if (connection.closing) { return; } + if (current !== connection) { + return; + } this.restartState.set(server.server, { restartUsed: true, backoffUntil: Date.now() + 1_000, @@ transport.onerror = (error: Error) => { if (connection.closing) { return; } + if (this.connections.get(server.server) !== connection) { + return; + } this.registry.setStatus( server.server, "unavailable",🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/downstream.ts` around lines 209 - 227, The transport.onclose handler mutates shared state (this.connections, restartState, registry) even if a newer connection has replaced the one captured as `connection`; guard the callback by verifying the active connection matches the captured one before making changes (e.g., check that this.connections.get(server.server) === connection), and only then perform deletion, restartState.set, and registry.setStatus; apply the same guard pattern for transport.onerror to prevent stale callbacks from mutating newer connections.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/runtime.ts`:
- Around line 107-111: reloadOnce() can reject and make this.reloading an
rejecting promise, causing unhandled rejections from void this.reload() and
breaking shutdown/cleanup; change assignments so this.reloading is a
non-rejecting promise by piping a .catch(err => { /* log/record err */; return
<failure-sentinel>; }) before .finally, e.g. set this.reloading =
this.reloadOnce().catch(err => { processLogger.error("reloadOnce failed", err);
return false; }).finally(() => { this.reloading = undefined; }); and update all
places that await this.reloading (and the call site reload()) to expect the
sentinel/boolean instead of throwing, ensuring cleanup/shutdown paths always
run.
- Around line 301-313: watchDirectoryTree can leak watch handles if a call to
watch(directory, ...) throws; wrap the per-directory watch creation in a
try/catch so that on any exception you iterate existing watchers (the array
returned by watchDirectoryTree) and call their close() method to release
resources before rethrowing the error; keep the existing behavior of invoking
scheduleReload() and scheduleWatcherRefresh() in the watcher callback and ensure
the function still returns the created FSWatcher[] on success.
---
Outside diff comments:
In `@src/downstream.ts`:
- Around line 209-227: The transport.onclose handler mutates shared state
(this.connections, restartState, registry) even if a newer connection has
replaced the one captured as `connection`; guard the callback by verifying the
active connection matches the captured one before making changes (e.g., check
that this.connections.get(server.server) === connection), and only then perform
deletion, restartState.set, and registry.setStatus; apply the same guard pattern
for transport.onerror to prevent stale callbacks from mutating newer
connections.
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 9f434f4d-a2bf-404c-9291-03b6bd253ab7
📒 Files selected for processing (4)
.changeset/quiet-rockets-serve.mddocs/plans/2026-05-12-hot-reload.mdsrc/downstream.tssrc/runtime.ts
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 1 file(s) based on 2 unresolved review comments. Files modified:
Commit: The changes have been pushed to the Time taken: |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/runtime.ts (1)
1-429:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRun the formatter on this file before merge.
CI is currently failing
oxfmt --check .onsrc/runtime.ts, so this change set won’t verify cleanly until the file is reformatted.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/runtime.ts` around lines 1 - 429, The file fails the repository formatter check; run the project formatter (oxfmt) on the changed file containing the CapletsRuntime class and its helper functions (e.g., watchedPaths, resetWatchers, discoverDirectories) and commit the results so the formatting of src/runtime.ts matches CI expectations; specifically run the same command used in CI (oxfmt --check . locally to see issues, then oxfmt . or your project's format script) and re-run tests to ensure oxfmt passes before pushing.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/runtime.ts`:
- Around line 296-300: Currently the file watcher calls scheduleWatcherRefresh()
for every event inside the watch callback, causing redundant full watcher
rebuilds on ordinary change events; update the watch callbacks (the watch(...)
handlers that call scheduleReload() and scheduleWatcherRefresh()) to inspect the
event type (e.g., "change" vs "rename"/file-creation) and only call
scheduleWatcherRefresh() when topology changes occur (rename or target
creation), leaving scheduleReload() for all events; note that reloadOnce()
already calls resetWatchers(), so avoid invoking scheduleWatcherRefresh() on
plain "change" events to prevent duplicate full teardown/rebuild cycles (apply
the same change to the other similar watch callback around
scheduleReload()/scheduleWatcherRefresh()).
- Around line 103-105: The current branch that does "if (this.reloading) { await
this.reloading; return !this.closed; }" drops any edits that arrive while a
reload is in-flight; change this so events received during an active reload set
a pending flag (e.g. this.pendingReload = true) instead of being consumed, then
after awaiting this.reloading check that flag and trigger an additional reload
(call reloadOnce() or loop to drain pendingReload) before returning; update the
code paths that set/clear this.reloading and this.pendingReload so a single
extra change causes one more reload and the flag is cleared when handled.
---
Outside diff comments:
In `@src/runtime.ts`:
- Around line 1-429: The file fails the repository formatter check; run the
project formatter (oxfmt) on the changed file containing the CapletsRuntime
class and its helper functions (e.g., watchedPaths, resetWatchers,
discoverDirectories) and commit the results so the formatting of src/runtime.ts
matches CI expectations; specifically run the same command used in CI (oxfmt
--check . locally to see issues, then oxfmt . or your project's format script)
and re-run tests to ensure oxfmt passes before pushing.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 94f4e50b-01a8-4154-a876-6d50abe3cfec
📒 Files selected for processing (1)
src/runtime.ts
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/runtime.ts`:
- Around line 267-277: The resetWatchers loop currently calls watchPathTree for
every watchedPaths entry which causes full recursive watching of parent
directories and nearest-existing ancestors; change it so only Caplet root
entries use recursive watching via this.watchPathTree(watchPath, entry.path) and
all other entries (config parent directories and fallback
nearest-existing-parent sentinels) register non-recursive watchers instead
(e.g., a single-path watcher method or this.watchPathTree with a recursive=false
flag). Update the same pattern where watchPathTree is used around the other
spots noted (the blocks around lines 292-295 and 349-356) so recursive traversal
is limited to actual Caplet roots and parents/missing-targets get non-recursive
watches. Ensure you reference watchedPaths(...) entries to decide which path is
a Caplet root vs a parent/sentinel and preserve the existing
this.watchers.push(...) logic for both cases.
- Around line 99-117: The reload() method currently discards the boolean result
from the this.reloading promise and returns !this.closed or true
unconditionally; change it to propagate the actual outcome from reloadOnce by
awaiting and returning the boolean resolved by this.reloading. Specifically:
ensure the branch that waits for an in-flight reload (when this.reloading is
truthy) does "const result = await this.reloading; return result &&
!this.closed" (or simply return the awaited boolean if reloadOnce already
accounts for closed), and in the initial-start path await this.reloading and
return its resolved boolean instead of returning true; keep the existing catch
that resolves false so failures propagate as false. Ensure references to
reload(), reloadOnce(), and this.reloading are updated accordingly.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: c2e6a423-8aa1-472d-9885-81f732a76743
📒 Files selected for processing (1)
src/runtime.ts
Fixed 1 file(s) based on 2 unresolved review comments. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
ian-pascoe
force-pushed
the
codex/hot-reload-caplets
branch
from
e53e262 to
319e038
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
CapletsRuntimeforcaplets servethat hot reloads config and Caplet files by defaultRegisteredToolhandles while preserving last known-good config on invalid editsVerification
pnpm verifySummary by CodeRabbit
New Features
caplets serveenables default hot-reload: live config validation, auto-apply of valid edits, and Caplet file add/update/remove without restarting.Bug Fixes / Reliability
Documentation
Tests