From 95592a016a2647af2325b2763d11ff0e27cc194e Mon Sep 17 00:00:00 2001 From: Leonid Bugaev Date: Sun, 15 Feb 2026 19:15:25 +0000 Subject: [PATCH] =?UTF-8?q?fix:=20config=20reload=20=E2=80=94=20watch=20ne?= =?UTF-8?q?sted=20deps,=20fix=20Ctrl+C,=20propagate=20to=20runner?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three bugs in the dynamic config reloading feature: 1. ConfigWatcher only watched the main config file. Changes to files referenced via extends/include/imports (workflows, skills, parent configs) were silently ignored. Now collects all local dependency paths by recursively parsing YAML and watches every resolved file. After each successful reload the watch list is refreshed to pick up newly added or removed dependencies. 2. The SIGINT/SIGTERM handler ran cleanup but never re-raised the signal, preventing process exit (Ctrl+C printed "[ConfigWatcher] Stopped" but the process kept running). Now removes itself and re-raises so the default handler terminates the process. 3. The onSwap callback only updated a local variable in cli-main.ts but SlackSocketRunner kept its own private config copy. Future requests after a reload still used the old config. Added runner.updateConfig() method and call it from onSwap. Co-Authored-By: Claude Opus 4.6 --- src/cli-main.ts | 13 +- src/config/config-watcher.ts | 181 ++++++- src/slack/socket-runner.ts | 5 + .../config-reload-integration.test.ts | 450 ++++++++++++++++++ tests/unit/config/config-watcher.test.ts | 213 ++++++++- 5 files changed, 835 insertions(+), 27 deletions(-) create mode 100644 tests/integration/config-reload-integration.test.ts diff --git a/src/cli-main.ts b/src/cli-main.ts index 569968ad8..cdf1859b9 100644 --- a/src/cli-main.ts +++ b/src/cli-main.ts @@ -1332,6 +1332,7 @@ export async function main(): Promise { snapshotStore: watchStore, onSwap: newConfig => { config = newConfig; + runner.updateConfig(newConfig); logger.info('[Watch] Config updated'); }, }); @@ -1339,13 +1340,17 @@ export async function main(): Promise { watcher.start(); logger.info('Config watching enabled'); - // Clean up watcher resources on process shutdown - const cleanup = () => { + // Clean up watcher resources on process shutdown, then re-raise + // the signal so the default handler (or other listeners) can terminate. + const onSignal = (sig: NodeJS.Signals) => { watcher.stop(); watchStore.shutdown().catch(() => {}); + process.removeListener('SIGINT', onSignal); + process.removeListener('SIGTERM', onSignal); + process.kill(process.pid, sig); }; - process.on('SIGINT', cleanup); - process.on('SIGTERM', cleanup); + process.on('SIGINT', onSignal); + process.on('SIGTERM', onSignal); } catch (watchErr: unknown) { logger.warn(`Config watch setup failed (Slack mode continues without it): ${watchErr}`); } diff --git a/src/config/config-watcher.ts b/src/config/config-watcher.ts index f60f74d20..2fc69447f 100644 --- a/src/config/config-watcher.ts +++ b/src/config/config-watcher.ts @@ -1,24 +1,108 @@ /** * Config file watcher — watches for file changes and SIGUSR2 signals. * + * Watches the main config file AND all local dependencies (extends, include, + * imports) so that changes to nested workflows, skills, or parent configs + * trigger a reload. + * * Uses fs.watch (no external deps) with debouncing to handle editor quirks * (multiple write events per save). SIGUSR2 is guarded for non-Windows only. - * The watcher uses persistent: false so it doesn't keep the process alive. + * All watchers use persistent: false so they don't keep the process alive. * - * All reload calls are wrapped in error handling so that failures never - * propagate as unhandled promise rejections or crash the process. + * After each successful reload the watch list is refreshed so newly added + * or removed dependencies are tracked automatically. */ import fs from 'fs'; +import path from 'path'; import { logger } from '../logger'; import { ConfigReloader } from './config-reloader'; const DEFAULT_DEBOUNCE_MS = 500; +/** + * Collect all local file paths that a config depends on by parsing YAML + * and following extends/include/imports chains recursively. + * Remote URLs and the special "default" source are skipped. + */ +export function collectLocalConfigDeps(configPath: string, visited?: Set): string[] { + visited = visited || new Set(); + const absPath = path.resolve(configPath); + if (visited.has(absPath)) return []; + visited.add(absPath); + + let content: string; + try { + content = fs.readFileSync(absPath, 'utf8'); + } catch { + return []; + } + + // Use a lightweight YAML parse — js-yaml is already a bundled dependency + let parsed: any; + try { + const yaml = require('js-yaml') as typeof import('js-yaml'); + parsed = yaml.load(content); + } catch { + return []; + } + + if (!parsed || typeof parsed !== 'object') return []; + + const deps: string[] = []; + const baseDir = path.dirname(absPath); + + const isLocal = (src: string): boolean => + typeof src === 'string' && + src !== 'default' && + !src.startsWith('http://') && + !src.startsWith('https://'); + + // extends / include + const extendsVal = parsed.extends || parsed.include; + if (extendsVal) { + const sources: string[] = Array.isArray(extendsVal) ? extendsVal : [extendsVal]; + for (const src of sources) { + if (!isLocal(src)) continue; + const resolved = path.resolve(baseDir, src); + deps.push(resolved); + deps.push(...collectLocalConfigDeps(resolved, visited)); + } + } + + // imports (workflow / skill files) + if (Array.isArray(parsed.imports)) { + for (const src of parsed.imports) { + if (!isLocal(src)) continue; + const resolved = path.resolve(baseDir, src); + deps.push(resolved); + deps.push(...collectLocalConfigDeps(resolved, visited)); + } + } + + // checks/steps that reference external workflow configs via `config:` field + const checks = parsed.checks || parsed.steps; + if (checks && typeof checks === 'object') { + for (const check of Object.values(checks) as any[]) { + if ( + check?.type === 'workflow' && + typeof check?.config === 'string' && + isLocal(check.config) + ) { + const resolved = path.resolve(baseDir, check.config); + deps.push(resolved); + deps.push(...collectLocalConfigDeps(resolved, visited)); + } + } + } + + return deps; +} + export class ConfigWatcher { private configPath: string; private reloader: ConfigReloader; private debounceMs: number; - private watcher: fs.FSWatcher | null = null; + private watchers: Map = new Map(); private debounceTimer: ReturnType | null = null; private signalHandler: (() => void) | null = null; @@ -32,18 +116,11 @@ export class ConfigWatcher { // Remove any previous listeners in case start() is called after stop() this.stop(); - // Watch the config file - try { - this.watcher = fs.watch(this.configPath, { persistent: false }, _eventType => { - this.debouncedReload(); - }); + // Collect all files to watch: main config + dependencies + const filesToWatch = this.collectWatchTargets(); - this.watcher.on('error', err => { - logger.warn(`[ConfigWatcher] File watch error: ${err.message}`); - }); - } catch (err: unknown) { - const msg = err instanceof Error ? err.message : String(err); - logger.warn(`[ConfigWatcher] Could not watch file: ${msg}`); + for (const filePath of filesToWatch) { + this.watchFile(filePath); } // Listen for SIGUSR2 (non-Windows) @@ -55,14 +132,16 @@ export class ConfigWatcher { process.on('SIGUSR2', this.signalHandler); } - logger.info(`[ConfigWatcher] Watching ${this.configPath} for changes`); + const depCount = filesToWatch.length - 1; + const depMsg = depCount > 0 ? ` (+ ${depCount} dependencies)` : ''; + logger.info(`[ConfigWatcher] Watching ${this.configPath}${depMsg} for changes`); } stop(): void { - if (this.watcher) { - this.watcher.close(); - this.watcher = null; + for (const [, watcher] of this.watchers) { + watcher.close(); } + this.watchers.clear(); if (this.debounceTimer) { clearTimeout(this.debounceTimer); @@ -79,6 +158,56 @@ export class ConfigWatcher { logger.debug('[ConfigWatcher] Stopped'); } + /** + * Refresh the set of watched files after a successful reload. + * New dependencies are watched; removed ones are unwatched. + */ + private refreshWatches(): void { + const newTargets = new Set(this.collectWatchTargets()); + const current = new Set(this.watchers.keys()); + + // Stop watching removed files + for (const filePath of current) { + if (!newTargets.has(filePath)) { + this.watchers.get(filePath)?.close(); + this.watchers.delete(filePath); + logger.debug(`[ConfigWatcher] Unwatched removed dep: ${filePath}`); + } + } + + // Start watching new files + for (const filePath of newTargets) { + if (!current.has(filePath)) { + this.watchFile(filePath); + logger.debug(`[ConfigWatcher] Watching new dep: ${filePath}`); + } + } + } + + private collectWatchTargets(): string[] { + const mainPath = path.resolve(this.configPath); + const deps = collectLocalConfigDeps(this.configPath); + // Deduplicate + return [...new Set([mainPath, ...deps])]; + } + + private watchFile(filePath: string): void { + try { + const watcher = fs.watch(filePath, { persistent: false }, _eventType => { + this.debouncedReload(); + }); + + watcher.on('error', err => { + logger.warn(`[ConfigWatcher] Watch error on ${filePath}: ${err.message}`); + }); + + this.watchers.set(filePath, watcher); + } catch (err: unknown) { + const msg = err instanceof Error ? err.message : String(err); + logger.warn(`[ConfigWatcher] Could not watch ${filePath}: ${msg}`); + } + } + private debouncedReload(): void { if (this.debounceTimer) { clearTimeout(this.debounceTimer); @@ -92,11 +221,19 @@ export class ConfigWatcher { /** * Fire-and-forget reload with full error handling. + * On success, refreshes the watch list to pick up new/removed dependencies. * Ensures unhandled promise rejections never escape. */ private safeReload(): void { - this.reloader.reload().catch(err => { - logger.error(`[ConfigWatcher] Unhandled reload error: ${err}`); - }); + this.reloader + .reload() + .then(success => { + if (success) { + this.refreshWatches(); + } + }) + .catch(err => { + logger.error(`[ConfigWatcher] Unhandled reload error: ${err}`); + }); } } diff --git a/src/slack/socket-runner.ts b/src/slack/socket-runner.ts index 553bb619c..4a54507bc 100644 --- a/src/slack/socket-runner.ts +++ b/src/slack/socket-runner.ts @@ -71,6 +71,11 @@ export class SlackSocketRunner { this.cfg = cfg; } + /** Hot-swap the config used for future requests (does not affect in-flight ones). */ + updateConfig(cfg: VisorConfig): void { + this.cfg = cfg; + } + /** * Lazily initialize the SlackClient if not already set. * Called by both start() and handleMessage() to ensure the client is available. diff --git a/tests/integration/config-reload-integration.test.ts b/tests/integration/config-reload-integration.test.ts new file mode 100644 index 000000000..014b2e488 --- /dev/null +++ b/tests/integration/config-reload-integration.test.ts @@ -0,0 +1,450 @@ +/** + * Integration test for the full config reload pipeline: + * file change → watcher detects → reloader loads (real ConfigManager) → onSwap fires + * + * Uses real filesystem, real YAML parsing, real config validation. + * Only the snapshot store is stubbed (avoids SQLite native dep in CI). + */ +import fs from 'fs'; +import path from 'path'; +import os from 'os'; +import { ConfigWatcher } from '../../src/config/config-watcher'; +import { ConfigReloader } from '../../src/config/config-reloader'; +import { ConfigManager } from '../../src/config'; +import type { VisorConfig } from '../../src/types/config'; + +jest.mock('../../src/logger', () => ({ + logger: { + info: jest.fn(), + warn: jest.fn(), + debug: jest.fn(), + error: jest.fn(), + }, +})); + +// Use the real execSync for git init (global setup mocks spawn but not execSync) +const realExecSync = (jest.requireActual('child_process') as typeof import('child_process')) + .execSync; + +describe('Config Reload Integration', () => { + let tmpDir: string; + let configManager: ConfigManager; + let mockStore: any; + let originalCwd: string; + + beforeEach(() => { + tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'visor-reload-integ-')); + + // Init a git repo in tmpDir so ConfigLoader's path traversal check + // uses tmpDir as the project root (it calls `git rev-parse --show-toplevel`). + realExecSync('git init', { cwd: tmpDir, stdio: 'ignore' }); + originalCwd = process.cwd(); + process.chdir(tmpDir); + + configManager = new ConfigManager(); + + // Stub the snapshot store (avoids needing better-sqlite3 native addon) + mockStore = { + save: jest.fn().mockResolvedValue({ id: 1 }), + initialize: jest.fn().mockResolvedValue(undefined), + shutdown: jest.fn().mockResolvedValue(undefined), + }; + }); + + afterEach(() => { + process.chdir(originalCwd); + fs.rmSync(tmpDir, { recursive: true, force: true }); + }); + + test('full pipeline: file edit → watcher → reloader → onSwap with new config', async () => { + const configPath = path.join(tmpDir, '.visor.yaml'); + fs.writeFileSync( + configPath, + [ + 'version: "1.0"', + 'checks:', + ' hello:', + ' type: log', + ' message: "original"', + '', + ].join('\n'), + 'utf8' + ); + + let swappedConfig: VisorConfig | null = null; + + const reloader = new ConfigReloader({ + configPath, + configManager, + snapshotStore: mockStore, + onSwap: cfg => { + swappedConfig = cfg; + }, + }); + + const watcher = new ConfigWatcher(configPath, reloader, 50); + watcher.start(); + + try { + // Edit the config file + fs.writeFileSync( + configPath, + [ + 'version: "1.0"', + 'checks:', + ' hello:', + ' type: log', + ' message: "updated"', + ' new-check:', + ' type: log', + ' message: "added"', + '', + ].join('\n'), + 'utf8' + ); + + // Wait for debounce + async reload + await new Promise(resolve => setTimeout(resolve, 500)); + + expect(swappedConfig).not.toBeNull(); + // The new check should be present + expect(swappedConfig!.checks).toHaveProperty('new-check'); + expect((swappedConfig!.checks as any)['hello'].message).toBe('updated'); + } finally { + watcher.stop(); + } + }); + + test('nested dependency change: editing an extended file triggers reload', async () => { + // Create parent config + const parentPath = path.join(tmpDir, 'base.yaml'); + fs.writeFileSync( + parentPath, + [ + 'version: "1.0"', + 'checks:', + ' base-check:', + ' type: log', + ' message: "from base"', + '', + ].join('\n'), + 'utf8' + ); + + // Main config extends the parent + const configPath = path.join(tmpDir, '.visor.yaml'); + fs.writeFileSync( + configPath, + [ + 'version: "1.0"', + 'extends: ./base.yaml', + 'checks:', + ' own-check:', + ' type: log', + '', + ].join('\n'), + 'utf8' + ); + + let swappedConfig: VisorConfig | null = null; + + const reloader = new ConfigReloader({ + configPath, + configManager, + snapshotStore: mockStore, + onSwap: cfg => { + swappedConfig = cfg; + }, + }); + + const watcher = new ConfigWatcher(configPath, reloader, 50); + watcher.start(); + + try { + // Modify the PARENT — not the main config + fs.writeFileSync( + parentPath, + [ + 'version: "1.0"', + 'checks:', + ' base-check:', + ' type: log', + ' message: "updated base"', + ' extra-base:', + ' type: log', + '', + ].join('\n'), + 'utf8' + ); + + await new Promise(resolve => setTimeout(resolve, 500)); + + expect(swappedConfig).not.toBeNull(); + // Should have both the base checks and the child check + expect(swappedConfig!.checks).toHaveProperty('base-check'); + expect(swappedConfig!.checks).toHaveProperty('extra-base'); + expect(swappedConfig!.checks).toHaveProperty('own-check'); + } finally { + watcher.stop(); + } + }); + + test('imported workflow/skill change triggers reload', async () => { + // Create a skill file + const skillPath = path.join(tmpDir, 'my-skill.yaml'); + fs.writeFileSync( + skillPath, + [ + 'id: my-skill', + 'name: My Skill', + 'steps:', + ' greet:', + ' type: log', + ' message: "hello from skill"', + '', + ].join('\n'), + 'utf8' + ); + + // Main config imports the skill + const configPath = path.join(tmpDir, '.visor.yaml'); + fs.writeFileSync( + configPath, + [ + 'version: "1.0"', + 'imports:', + ' - ./my-skill.yaml', + 'checks:', + ' main-check:', + ' type: log', + '', + ].join('\n'), + 'utf8' + ); + + let swapCount = 0; + + const reloader = new ConfigReloader({ + configPath, + configManager, + snapshotStore: mockStore, + onSwap: () => { + swapCount++; + }, + }); + + const watcher = new ConfigWatcher(configPath, reloader, 50); + watcher.start(); + + try { + // Modify the SKILL file — not the main config + fs.writeFileSync( + skillPath, + [ + 'id: my-skill', + 'name: My Skill', + 'steps:', + ' greet:', + ' type: log', + ' message: "updated skill"', + ' farewell:', + ' type: log', + ' message: "bye"', + '', + ].join('\n'), + 'utf8' + ); + + await new Promise(resolve => setTimeout(resolve, 500)); + + expect(swapCount).toBe(1); + } finally { + watcher.stop(); + } + }); + + test('invalid config change: reload fails, onSwap not called', async () => { + const configPath = path.join(tmpDir, '.visor.yaml'); + fs.writeFileSync(configPath, 'version: "1.0"\nchecks:\n ok:\n type: log\n', 'utf8'); + + let swapCalled = false; + let errorCalled = false; + + const reloader = new ConfigReloader({ + configPath, + configManager, + snapshotStore: mockStore, + onSwap: () => { + swapCalled = true; + }, + onError: () => { + errorCalled = true; + }, + }); + + const watcher = new ConfigWatcher(configPath, reloader, 50); + watcher.start(); + + try { + // Write invalid YAML + fs.writeFileSync(configPath, '{{{{invalid yaml!!!!', 'utf8'); + + await new Promise(resolve => setTimeout(resolve, 500)); + + expect(swapCalled).toBe(false); + expect(errorCalled).toBe(true); + } finally { + watcher.stop(); + } + }); + + test('dynamically added dependency is watched after reload', async () => { + const configPath = path.join(tmpDir, '.visor.yaml'); + // Start with no dependencies + fs.writeFileSync(configPath, 'version: "1.0"\nchecks:\n a:\n type: log\n', 'utf8'); + + let swapCount = 0; + let latestConfig: VisorConfig | null = null; + + const reloader = new ConfigReloader({ + configPath, + configManager, + snapshotStore: mockStore, + onSwap: cfg => { + swapCount++; + latestConfig = cfg; + }, + }); + + const watcher = new ConfigWatcher(configPath, reloader, 50); + watcher.start(); + + try { + // Phase 1: Add a parent config file and update main to extend it + const parentPath = path.join(tmpDir, 'parent.yaml'); + fs.writeFileSync( + parentPath, + 'version: "1.0"\nchecks:\n from-parent:\n type: log\n', + 'utf8' + ); + fs.writeFileSync( + configPath, + 'version: "1.0"\nextends: ./parent.yaml\nchecks:\n a:\n type: log\n', + 'utf8' + ); + + // Wait for first reload to complete (this triggers refreshWatches) + await new Promise(resolve => setTimeout(resolve, 500)); + expect(swapCount).toBe(1); + expect(latestConfig!.checks).toHaveProperty('from-parent'); + + // Phase 2: Now modify the parent (which was NOT watched before Phase 1) + fs.writeFileSync( + parentPath, + 'version: "1.0"\nchecks:\n from-parent:\n type: log\n bonus:\n type: log\n', + 'utf8' + ); + + await new Promise(resolve => setTimeout(resolve, 500)); + expect(swapCount).toBe(2); + expect(latestConfig!.checks).toHaveProperty('bonus'); + } finally { + watcher.stop(); + } + }); + + test('runner.updateConfig propagates new config to future requests', async () => { + // This tests that the onSwap callback pattern correctly updates the runner. + // We simulate the pattern from cli-main.ts without starting a real WebSocket. + const configPath = path.join(tmpDir, '.visor.yaml'); + fs.writeFileSync(configPath, 'version: "1.0"\nchecks:\n original:\n type: log\n', 'utf8'); + + // Simulate the runner object (lightweight stand-in — no real WS needed) + let runnerCfg: VisorConfig = { + version: '1.0', + checks: { original: { type: 'log' } }, + } as VisorConfig; + + const fakeRunner = { + updateConfig(cfg: VisorConfig) { + runnerCfg = cfg; + }, + }; + + const reloader = new ConfigReloader({ + configPath, + configManager, + snapshotStore: mockStore, + onSwap: newConfig => { + fakeRunner.updateConfig(newConfig); + }, + }); + + const watcher = new ConfigWatcher(configPath, reloader, 50); + watcher.start(); + + try { + // Edit config + fs.writeFileSync(configPath, 'version: "1.0"\nchecks:\n replaced:\n type: log\n', 'utf8'); + + await new Promise(resolve => setTimeout(resolve, 500)); + + // The "runner" should now have the new config + expect(runnerCfg.checks).toHaveProperty('replaced'); + expect(runnerCfg.checks).not.toHaveProperty('original'); + } finally { + watcher.stop(); + } + }); + + test('transitive dependency chain: grandparent change triggers reload', async () => { + // grandparent.yaml ← parent.yaml ← config.yaml + const grandparentPath = path.join(tmpDir, 'grandparent.yaml'); + fs.writeFileSync( + grandparentPath, + 'version: "1.0"\nchecks:\n gp-check:\n type: log\n', + 'utf8' + ); + + const parentPath = path.join(tmpDir, 'parent.yaml'); + fs.writeFileSync(parentPath, 'extends: ./grandparent.yaml\n', 'utf8'); + + const configPath = path.join(tmpDir, '.visor.yaml'); + fs.writeFileSync( + configPath, + 'version: "1.0"\nextends: ./parent.yaml\nchecks:\n leaf:\n type: log\n', + 'utf8' + ); + + let swappedConfig: VisorConfig | null = null; + + const reloader = new ConfigReloader({ + configPath, + configManager, + snapshotStore: mockStore, + onSwap: cfg => { + swappedConfig = cfg; + }, + }); + + const watcher = new ConfigWatcher(configPath, reloader, 50); + watcher.start(); + + try { + // Modify the GRANDPARENT — two levels up + fs.writeFileSync( + grandparentPath, + 'version: "1.0"\nchecks:\n gp-check:\n type: log\n gp-new:\n type: log\n', + 'utf8' + ); + + await new Promise(resolve => setTimeout(resolve, 500)); + + expect(swappedConfig).not.toBeNull(); + expect(swappedConfig!.checks).toHaveProperty('gp-new'); + expect(swappedConfig!.checks).toHaveProperty('leaf'); + } finally { + watcher.stop(); + } + }); +}); diff --git a/tests/unit/config/config-watcher.test.ts b/tests/unit/config/config-watcher.test.ts index ff2e142d9..ee1eca8a4 100644 --- a/tests/unit/config/config-watcher.test.ts +++ b/tests/unit/config/config-watcher.test.ts @@ -1,7 +1,7 @@ import fs from 'fs'; import path from 'path'; import os from 'os'; -import { ConfigWatcher } from '../../../src/config/config-watcher'; +import { ConfigWatcher, collectLocalConfigDeps } from '../../../src/config/config-watcher'; import { ConfigReloader } from '../../../src/config/config-reloader'; jest.mock('../../../src/logger', () => ({ @@ -106,4 +106,215 @@ describe('ConfigWatcher', () => { watcher.start(); // Should stop previous watcher first, then start fresh watcher.stop(); }); + + test('watches dependency files (extends/include)', async () => { + // Create a parent config that the main config extends + const parentPath = path.join(tmpDir, 'parent.yaml'); + fs.writeFileSync(parentPath, 'version: "1.0"\nsteps:\n base-check:\n type: log\n', 'utf8'); + + // Main config extends the parent + fs.writeFileSync(configPath, `version: "1.0"\nextends: ./parent.yaml\n`, 'utf8'); + + const watcher = new ConfigWatcher(configPath, mockReloader, 50); + watcher.start(); + + // Modify the PARENT file (not the main config) + fs.writeFileSync(parentPath, 'version: "2.0"\nsteps:\n base-check:\n type: log\n', 'utf8'); + + // Wait for debounce + await new Promise(resolve => setTimeout(resolve, 200)); + + expect(mockReloader.reload).toHaveBeenCalledTimes(1); + watcher.stop(); + }); + + test('watches imported workflow/skill files', async () => { + // Create a workflow (skill) file + const skillPath = path.join(tmpDir, 'my-skill.yaml'); + fs.writeFileSync(skillPath, 'id: my-skill\nsteps:\n run:\n type: log\n', 'utf8'); + + // Main config imports the skill + fs.writeFileSync(configPath, `version: "1.0"\nimports:\n - ./my-skill.yaml\n`, 'utf8'); + + const watcher = new ConfigWatcher(configPath, mockReloader, 50); + watcher.start(); + + // Modify the SKILL file + fs.writeFileSync( + skillPath, + 'id: my-skill\nsteps:\n run:\n type: log\n message: updated\n', + 'utf8' + ); + + // Wait for debounce + await new Promise(resolve => setTimeout(resolve, 200)); + + expect(mockReloader.reload).toHaveBeenCalledTimes(1); + watcher.stop(); + }); + + test('refreshes watches after successful reload', async () => { + // Start with no dependencies + fs.writeFileSync(configPath, 'version: "1.0"\n', 'utf8'); + + const watcher = new ConfigWatcher(configPath, mockReloader, 50); + watcher.start(); + + // Now update config to add a dependency + const depPath = path.join(tmpDir, 'dep.yaml'); + fs.writeFileSync(depPath, 'version: "1.0"\n', 'utf8'); + fs.writeFileSync(configPath, `version: "1.0"\nimports:\n - ./dep.yaml\n`, 'utf8'); + + // Wait for debounce + reload + refresh + await new Promise(resolve => setTimeout(resolve, 200)); + expect(mockReloader.reload).toHaveBeenCalledTimes(1); + + // Reset call count + mockReloader.reload.mockClear(); + + // Now modify the NEW dependency — should trigger reload + fs.writeFileSync(depPath, 'version: "2.0"\n', 'utf8'); + await new Promise(resolve => setTimeout(resolve, 200)); + + expect(mockReloader.reload).toHaveBeenCalledTimes(1); + watcher.stop(); + }); +}); + +describe('collectLocalConfigDeps', () => { + let tmpDir: string; + + beforeEach(() => { + tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'visor-deps-test-')); + }); + + afterEach(() => { + fs.rmSync(tmpDir, { recursive: true, force: true }); + }); + + test('returns empty for simple config with no deps', () => { + const configPath = path.join(tmpDir, 'simple.yaml'); + fs.writeFileSync(configPath, 'version: "1.0"\n', 'utf8'); + + const deps = collectLocalConfigDeps(configPath); + expect(deps).toEqual([]); + }); + + test('returns empty for non-existent file', () => { + const deps = collectLocalConfigDeps('/nonexistent/config.yaml'); + expect(deps).toEqual([]); + }); + + test('collects extends dependencies', () => { + const parentPath = path.join(tmpDir, 'parent.yaml'); + fs.writeFileSync(parentPath, 'version: "1.0"\n', 'utf8'); + + const configPath = path.join(tmpDir, 'config.yaml'); + fs.writeFileSync(configPath, `extends: ./parent.yaml\n`, 'utf8'); + + const deps = collectLocalConfigDeps(configPath); + expect(deps).toContain(parentPath); + }); + + test('collects include dependencies (alias for extends)', () => { + const parentPath = path.join(tmpDir, 'parent.yaml'); + fs.writeFileSync(parentPath, 'version: "1.0"\n', 'utf8'); + + const configPath = path.join(tmpDir, 'config.yaml'); + fs.writeFileSync(configPath, `include: ./parent.yaml\n`, 'utf8'); + + const deps = collectLocalConfigDeps(configPath); + expect(deps).toContain(parentPath); + }); + + test('collects imports dependencies', () => { + const skillPath = path.join(tmpDir, 'skill.yaml'); + fs.writeFileSync(skillPath, 'id: my-skill\n', 'utf8'); + + const configPath = path.join(tmpDir, 'config.yaml'); + fs.writeFileSync(configPath, `imports:\n - ./skill.yaml\n`, 'utf8'); + + const deps = collectLocalConfigDeps(configPath); + expect(deps).toContain(skillPath); + }); + + test('collects nested/transitive dependencies', () => { + const grandparent = path.join(tmpDir, 'grandparent.yaml'); + fs.writeFileSync(grandparent, 'version: "1.0"\n', 'utf8'); + + const parent = path.join(tmpDir, 'parent.yaml'); + fs.writeFileSync(parent, `extends: ./grandparent.yaml\n`, 'utf8'); + + const configPath = path.join(tmpDir, 'config.yaml'); + fs.writeFileSync(configPath, `extends: ./parent.yaml\n`, 'utf8'); + + const deps = collectLocalConfigDeps(configPath); + expect(deps).toContain(parent); + expect(deps).toContain(grandparent); + }); + + test('skips remote URLs', () => { + const configPath = path.join(tmpDir, 'config.yaml'); + fs.writeFileSync( + configPath, + `extends:\n - https://example.com/base.yaml\n - ./local.yaml\n`, + 'utf8' + ); + + const localPath = path.join(tmpDir, 'local.yaml'); + fs.writeFileSync(localPath, 'version: "1.0"\n', 'utf8'); + + const deps = collectLocalConfigDeps(configPath); + expect(deps).toContain(localPath); + expect(deps).not.toContainEqual(expect.stringContaining('https://')); + }); + + test('skips "default" extends source', () => { + const configPath = path.join(tmpDir, 'config.yaml'); + fs.writeFileSync(configPath, `extends: default\n`, 'utf8'); + + const deps = collectLocalConfigDeps(configPath); + expect(deps).toEqual([]); + }); + + test('handles circular references without infinite loop', () => { + const a = path.join(tmpDir, 'a.yaml'); + const b = path.join(tmpDir, 'b.yaml'); + fs.writeFileSync(a, `imports:\n - ./b.yaml\n`, 'utf8'); + fs.writeFileSync(b, `imports:\n - ./a.yaml\n`, 'utf8'); + + // Should not hang or throw + const deps = collectLocalConfigDeps(a); + expect(deps).toContain(b); + }); + + test('collects workflow config: references from checks', () => { + const workflowPath = path.join(tmpDir, 'workflow.yaml'); + fs.writeFileSync(workflowPath, 'id: my-workflow\nsteps:\n run:\n type: log\n', 'utf8'); + + const configPath = path.join(tmpDir, 'config.yaml'); + fs.writeFileSync( + configPath, + `checks:\n my-check:\n type: workflow\n config: ./workflow.yaml\n`, + 'utf8' + ); + + const deps = collectLocalConfigDeps(configPath); + expect(deps).toContain(workflowPath); + }); + + test('collects imports from within workflow files', () => { + const depSkill = path.join(tmpDir, 'dep-skill.yaml'); + fs.writeFileSync(depSkill, 'id: dep-skill\n', 'utf8'); + + const skill = path.join(tmpDir, 'skill.yaml'); + fs.writeFileSync(skill, `id: main-skill\nimports:\n - ./dep-skill.yaml\n`, 'utf8'); + + const configPath = path.join(tmpDir, 'config.yaml'); + fs.writeFileSync(configPath, `imports:\n - ./skill.yaml\n`, 'utf8'); + + const deps = collectLocalConfigDeps(configPath); + expect(deps).toContain(skill); + expect(deps).toContain(depSkill); + }); });