Hot reload config and trust rules on file change#121
Conversation
Watch ~/.vellum/config.json and ~/.vellum/trust.json for changes while the daemon is running. When either file is modified: - config.json: invalidate the config cache, re-initialize providers, and evict/mark-stale all sessions so they pick up the new settings - trust.json: clear the trust rule cache so permission checks use the latest rules immediately Uses fs.watch() with 200ms debounce (editors often write in multiple steps). Watchers are started on daemon start and cleaned up on stop. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| const watchFile = (filePath: string, label: string, onChange: () => void) => { | ||
| if (!existsSync(filePath)) return; |
There was a problem hiding this comment.
🔴 File watcher silently skipped when config/trust file doesn't exist at daemon startup
The watchFile helper at line 90 returns early with if (!existsSync(filePath)) return;, meaning no watcher is set up if the file doesn't exist when the daemon starts. If a user starts the daemon before creating ~/.vellum/config.json or ~/.vellum/trust.json, and later creates or modifies those files, the changes will never be detected.
Root Cause and Impact
The config.json file is only created when the user explicitly runs a config-set command (via saveRawConfig at assistant/src/config/loader.ts:131-136), and trust.json is only created when a trust rule is first added (assistant/src/permissions/trust-store.ts:43-53). Both files may legitimately not exist at daemon startup.
Since fs.watch() requires the file to exist, the fix should either watch the parent directory for file creation events, or use fs.watchFile() (polling-based) which works with non-existent paths.
Impact: The hot-reload feature — the entire purpose of this PR — silently does nothing for users who haven't previously created these config files, which is likely the common case for new users.
Prompt for agents
In assistant/src/daemon/server.ts, the watchFile helper on line 89-105 skips setting up a watcher when the file doesn't exist. Instead of watching individual files with fs.watch(), consider watching the parent directory (dataDir) for changes. When a 'rename' event fires for the target filename, trigger the onChange callback. This handles both file creation and editors that use atomic rename-based saves. Alternatively, use fs.watchFile() (polling-based) which works even when the file doesn't exist yet, though it uses more resources. The directory-watching approach would look something like: watch(dataDir, (eventType, filename) => { if (filename === basename(filePath)) { ... debounce and call onChange ... } }).
Was this helpful? React with 👍 or 👎 to provide feedback.
|
|
||
| private stopFileWatchers(): void { | ||
| for (const watcher of this.watchers) { | ||
| watcher.close(); | ||
| } | ||
| this.watchers = []; |
There was a problem hiding this comment.
🟡 Pending debounce timer fires after server stop, causing use-after-teardown
When stopFileWatchers() is called during stop(), it closes the FSWatcher instances but does not cancel any pending setTimeout debounce callbacks. If a file change event fired shortly before stop(), the 200ms debounced callback will still execute, accessing this.sessions and calling invalidateConfigCache() / initializeProviders() after the server has been torn down.
Root Cause and Impact
At assistant/src/daemon/server.ts:92-99, the debounceTimer is a local variable captured in the watch callback closure. The stopFileWatchers method at lines 132-136 only calls watcher.close() and clears the this.watchers array — it has no reference to the debounce timers to clear them.
When the debounced callback fires after stop:
invalidateConfigCache()clears the config cache unnecessarilygetConfig()andinitializeProviders()re-initialize providers on a stopped server- The
for (const [id, session] of this.sessions)loop mutates session state after teardown
Impact: Could cause unexpected errors or log noise during shutdown. In the worst case, it could interfere with a subsequent server restart if the process is reused.
Prompt for agents
In assistant/src/daemon/server.ts, the stopFileWatchers method at lines 131-136 needs to also clear any pending debounce timers. One approach: store the debounce timer IDs alongside the watchers (e.g., change this.watchers to store objects like { watcher: FSWatcher, clearDebounce: () => void }). In the watchFile helper, expose a way to clear the debounceTimer. In stopFileWatchers, call clearDebounce() for each entry before closing the watcher. Alternatively, add a boolean flag like this.stopped = true and check it at the start of each onChange callback.
Was this helpful? React with 👍 or 👎 to provide feedback.
|
|
||
| watchFile(configPath, 'config.json', () => { | ||
| invalidateConfigCache(); | ||
| try { | ||
| const config = getConfig(); | ||
| initializeProviders(config); | ||
| } catch (err) { | ||
| log.error({ err }, 'Failed to reload config'); | ||
| return; | ||
| } | ||
| // Evict idle sessions; mark busy ones as stale | ||
| for (const [id, session] of this.sessions) { | ||
| if (!session.isProcessing()) { | ||
| this.sessions.delete(id); | ||
| } else { | ||
| session.markStale(); | ||
| } | ||
| } |
There was a problem hiding this comment.
🟡 Self-triggered config reload: handleModelSet writes config.json which triggers the file watcher
When handleModelSet at line 354 calls saveRawConfig(raw), it writes to config.json. This triggers the file watcher's onChange callback (after 200ms debounce), which redundantly invalidates the config cache, re-initializes providers, and evicts/marks sessions stale a second time.
Root Cause and Impact
The file watcher at lines 108-124 fires for any change to config.json, including changes made by the daemon itself via handleModelSet (line 349-378). The handleModelSet method already performs the full reload sequence (invalidate cache → getConfig → initializeProviders → evict sessions). The watcher then does it all again 200ms later.
The second eviction pass is particularly problematic: any new sessions created in the 200ms window between handleModelSet completing and the debounced watcher firing will be incorrectly evicted or marked stale, even though they were already created with the latest config.
Impact: Sessions created immediately after a /model command may be unnecessarily evicted or marked stale by the delayed watcher callback, forcing an extra reload on the next user message.
Prompt for agents
In assistant/src/daemon/server.ts, add a mechanism to suppress the file watcher callback when the daemon itself writes to config.json. One approach: add a private field like `private suppressConfigReload = false;` to DaemonServer. Set it to true before calling saveRawConfig() in handleModelSet, and reset it after. In the config watcher onChange callback (lines 108-124), check this flag and skip the reload if it's set. Alternatively, use a timestamp-based approach: record when the daemon last wrote the file, and in the watcher callback, skip if the change happened within a small window of the last self-write.
Was this helpful? React with 👍 or 👎 to provide feedback.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cad49fcb7f
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const trustPath = join(dataDir, 'trust.json'); | ||
|
|
||
| const watchFile = (filePath: string, label: string, onChange: () => void) => { | ||
| if (!existsSync(filePath)) return; |
There was a problem hiding this comment.
The new watchFile helper exits immediately when the target file is missing (if (!existsSync(filePath)) return;), which means fresh daemon runs never subscribe to config.json changes because loadConfig() can run entirely from defaults without creating that file. In that common case, a later vellum config set ... (or manual file creation) will not trigger cache invalidation/provider reinit, so the daemon keeps stale settings until restart; the same blind spot applies to first-time trust.json creation after cache warmup.
Useful? React with 👍 / 👎.
| if (!existsSync(filePath)) return; | ||
| try { | ||
| let debounceTimer: ReturnType<typeof setTimeout> | null = null; | ||
| const watcher = watch(filePath, () => { |
There was a problem hiding this comment.
The watcher is bound directly to the file path with watch(filePath, ...) but does not handle rename by recreating the watcher. trust-store persists updates via renameSync(tmpPath, path), so after the first atomic replace, many platforms stop reporting future changes for that watch handle; subsequent trust edits then no longer clear the in-memory cache and permission decisions can remain stale until process restart.
Useful? React with 👍 / 👎.
…ppression Addresses all review feedback on #121: 1. Watch the data directory instead of individual files, so watchers work even when config.json/trust.json don't exist at daemon startup (P1). Also fixes atomic rename writes (trust-store uses renameSync) that would break per-file watchers on many platforms. 2. Cancel pending debounce timers in stopFileWatchers() to prevent use-after-teardown callbacks firing after server shutdown. 3. Suppress redundant config reload when handleModelSet writes config.json — the method already does the full reload sequence, and a delayed watcher callback would incorrectly evict sessions created in the 200ms debounce window. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ppression (#124) Addresses all review feedback on #121: 1. Watch the data directory instead of individual files, so watchers work even when config.json/trust.json don't exist at daemon startup (P1). Also fixes atomic rename writes (trust-store uses renameSync) that would break per-file watchers on many platforms. 2. Cancel pending debounce timers in stopFileWatchers() to prevent use-after-teardown callbacks firing after server shutdown. 3. Suppress redundant config reload when handleModelSet writes config.json — the method already does the full reload sequence, and a delayed watcher callback would incorrectly evict sessions created in the 200ms debounce window. Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
1 participant
Summary
~/.vellum/config.jsonand~/.vellum/trust.jsonfor changes while the daemon is running/modelcommand)fs.watch()with 200ms debounce to handle editors that write files in multiple stepsserver.start()and cleaned up onserver.stop()invalidateConfigCache()export toconfig/loader.tsTest plan
bun tsc --noEmitpassesbun test)🤖 Generated with Claude Code