fix: config reload — watch nested deps, fix Ctrl+C, propagate to runner

[buger]

Summary

Fixes three bugs in the dynamic config reloading feature (#358):

• Nested dependency watching: ConfigWatcher only watched the main config file. Changes to files referenced via extends/include/imports (workflows, skills, parent configs) were silently ignored. Now recursively collects all local dependency paths by parsing YAML and watches every resolved file. After each successful reload the watch list is refreshed to track newly added/removed dependencies.

• Ctrl+C doesn't exit: The SIGINT/SIGTERM handler ran cleanup but never re-raised the signal, so the process kept running (printing [ConfigWatcher] Stopped on every Ctrl+C). Now removes itself and re-raises so the default handler terminates the process.

• Config not propagated to runner: 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.

Test plan

• 21 unit tests for ConfigWatcher + collectLocalConfigDeps (extends, include, imports, transitive, circular, workflow config refs)
• 7 integration tests with real ConfigManager, real YAML parsing, real fs.watch:
  • Full pipeline: file edit → watcher → reloader → onSwap
  • Nested dependency (extends parent change)
  • Imported skill change
  • Invalid YAML → reload fails gracefully
  • Dynamically added dependency watched after reload
  • Config propagation to runner via updateConfig()
  • Transitive chain (grandparent → parent → config)
• Manual test: visor --slack --config .visor.yaml --watch + edit skill file → verify reload
• Manual test: Ctrl+C exits cleanly

🤖 Generated with Claude Code

[probelabs]

PR Overview: Config Reload Bug Fixes

Summary

This PR fixes three critical bugs in the dynamic config reloading feature introduced in #358:

1. Nested dependency watching: ConfigWatcher only monitored the main config file, ignoring changes to files referenced via extends, include, imports, and workflow config: fields. Now recursively collects and watches all local dependency paths.

2. Ctrl+C doesn't exit: The SIGINT/SIGTERM handler performed cleanup but never re-raised the signal, causing the process to continue running. Now properly removes listeners and re-raises the signal for clean termination.

3. Config not propagated to runner: The onSwap callback only updated a local variable in cli-main.ts, but SlackSocketRunner maintained its own private config copy. Added runner.updateConfig() method to propagate changes.

Files Changed

• src/cli-main.ts (+9/-4): Fixed signal handling to re-raise SIGINT/SIGTERM after cleanup; added runner.updateConfig() call in onSwap callback
• src/config/config-watcher.ts (+159/-22): Complete rewrite of watching logic; added collectLocalConfigDeps() function for recursive dependency discovery; changed from single watcher to multi-file watcher map; added refreshWatches() to update watch list after successful reloads
• src/slack/socket-runner.ts (+5/-0): Added updateConfig() method to hot-swap config for future requests
• tests/integration/config-reload-integration.test.ts (+450): New integration tests covering full pipeline, nested dependencies, imported skills, invalid YAML, dynamic dependencies, runner propagation, and transitive chains
• tests/unit/config/config-watcher.test.ts (+212/-1): Added 21 unit tests for collectLocalConfigDeps() covering extends, include, imports, transitive deps, circular refs, remote URLs, and workflow config refs

Architecture & Impact Assessment

What This PR Accomplishes

The PR transforms ConfigWatcher from a single-file watcher into a sophisticated multi-file dependency tracking system:

Before:

Config.yaml → [Watcher] → Reload → onSwap → local variable

After:

Config.yaml → [Dependency Collector] → [Multi-file Watchers]
     ↓              ↓                         ↓
  parent.yaml   skill.yaml              workflow.yaml
     ↓              ↓                         ↓
nested deps   imported skills          referenced configs
     ↓              ↓                         ↓
[All Changes] → [Debounced Reload] → [Refresh Watches] → onSwap → runner.updateConfig()

Key Technical Changes

1. Dependency Collection (collectLocalConfigDeps):

   • Parses YAML files using js-yaml
   • Recursively follows extends, include, imports chains
   • Detects workflow config: field references
   • Filters out remote URLs and "default" source
   • Handles circular references via visited set
   • Returns absolute paths for all local dependencies

2. Multi-File Watching:

   • Replaced single watcher with watchers: Map<string, fs.FSWatcher>
   • Each dependency gets its own watcher with persistent: false
   • Individual error handling per file
   • Dynamic watch list refresh after successful reloads

3. Signal Handling Fix:

   • Changed from cleanup() to onSignal(sig) that captures signal type
   • Removes listeners before re-raising to prevent loops
   • Uses process.kill(process.pid, sig) to re-raise

4. Config Propagation:

   • Added SlackSocketRunner.updateConfig(cfg) method
   • Called from onSwap callback in cli-main.ts
   • Ensures future requests use new config (in-flight requests unaffected)

Affected System Components

• ConfigWatcher: Core watching logic completely rewritten
• ConfigReloader: Now returns success boolean for watch refresh logic
• SlackSocketRunner: New public method for config hot-swap
• CLI Main: Signal handling and onSwap callback updated
• Test Suite: 28 new tests (21 unit + 7 integration)

Component Relationships

graph TD
    A[ConfigWatcher.start] --> B[collectLocalConfigDeps]
    B --> C[Parse YAML]
    C --> D[Find extends/include/imports]
    D --> E[Resolve Paths]
    E --> F[Recursive Collection]
    F --> G[watchFile for each dep]
    G --> H[fs.watch with persistent:false]
    H --> I[File Change Detected]
    I --> J[debouncedReload]
    J --> K[ConfigReloader.reload]
    K --> L{Success?}
    L -->|Yes| M[refreshWatches]
    L -->|No| N[Log Error]
    M --> O[Re-collect dependencies]
    O --> P[Add new watchers]
    P --> Q[Remove stale watchers]
    K --> R[onSwap callback]
    R --> S[runner.updateConfig]
    
    T[SIGINT/SIGTERM] --> U[onSignal handler]
    U --> V[watcher.stop]
    V --> W[store.shutdown]
    W --> X[Remove listeners]
    X --> Y[process.kill pid, sig]

Loading

Scope Discovery & Context Expansion

Direct Impact

• Slack Socket Mode: Primary beneficiary of config reload feature
• Watch Mode: All users running visor --slack --watch
• Config Development: Developers editing parent configs, skills, or workflows

Related Modules (Inferred)

Based on the changes, these related areas should be considered:

1. ConfigManager (src/config/config-manager.ts): Loads and validates configs; should be tested with nested dependencies
2. ConfigLoader (src/config/config-loader.ts): Handles extends/include resolution; ensure consistency with collectLocalConfigDeps
3. YAML Parsing: Dependency on js-yaml; verify error handling for malformed YAML
4. File System Operations: Watch behavior on different OSes (Windows vs Unix)
5. Snapshot Store (src/config/config-snapshot-store.ts): Used in reload pipeline; integration tested

Potential Edge Cases

• Symbolic links: Dependency resolution may follow or not follow symlinks
• Network drives: fs.watch behavior may differ on mounted filesystems
• Concurrent edits: Multiple files changing simultaneously
• Large dependency trees: Performance with deeply nested extends chains
• File deletion: Watch behavior when dependency files are deleted

Testing Coverage

The PR includes comprehensive testing:

Unit Tests (21):

• Simple config (no deps)
• Non-existent file handling
• extends/include dependencies
• imports dependencies
• Nested/transitive dependencies
• Remote URL filtering
• "default" source handling
• Circular reference detection
• Workflow config: references
• Imports from within workflow files

Integration Tests (7):

• Full pipeline (file edit → watcher → reloader → onSwap)
• Nested dependency (parent file change)
• Imported skill change
• Invalid YAML graceful failure
• Dynamically added dependency watching
• Config propagation to runner
• Transitive chain (grandparent → parent → config)

Review Recommendations

1. Focus Areas:

   • collectLocalConfigDeps() recursion logic and circular reference handling
   • Signal handling re-raise mechanism
   • Watch refresh logic for race conditions

2. Testing:

   • Manual testing with visor --slack --config .visor.yaml --watch
   • Verify Ctrl+C exits cleanly
   • Test editing skill files and parent configs

3. Performance:

   • Consider impact of watching many files in large projects
   • Debounce timing (500ms default) may need tuning

4. Documentation:

   • Update docs/config-reload.md if it exists
   • Document watch behavior in README or CLI help

Labels

• Type: bug (fixes three bugs in existing feature)
• Review Effort: 3 (moderate - complex logic but well-tested)
• Components: config, slack, watcher
• Testing: comprehensive (28 new tests)

The PR is well-structured with extensive test coverage and clear separation of concerns. The dependency collection logic is robust with proper handling of edge cases like circular references and remote URLs.

Metadata

• Review Effort: 3 / 5
• Primary Label: bug

---

Powered by Visor from Probelabs

Last updated: 2026-02-15T19:25:05.934Z | Triggered by: pr_opened | Commit: 95592a0

💡 TIP: You can chat with Visor using /visor ask <your question>

[probelabs]

Security Issues (1)

Severity	Location	Issue
🟠 Error	contract:0	Output schema validation failed: must have required property 'issues'

Architecture Issues (1)

Severity	Location	Issue
🟠 Error	contract:0	Output schema validation failed: must have required property 'issues'

Performance Issues (1)

Severity	Location	Issue
🟠 Error	contract:0	Output schema validation failed: must have required property 'issues'

Quality Issues (15)

Severity	Location	Issue
🟢 Info	src/config/config-watcher.ts:59	The isLocal check logic is duplicated inline. This validation pattern appears multiple times and should be extracted to a reusable function. 💡 Suggestion Extract isLocal to a named function at module level for better testability and reusability.
🟢 Info	src/config/config-watcher.ts:168	refreshWatches is called from safeReload after successful reload, but there's no mechanism to handle the case where refreshWatches itself fails (e.g., if new dependencies can't be watched). 💡 Suggestion Consider wrapping refreshWatches in try-catch and logging errors. The watcher should continue watching old files even if adding new watches fails.
🟢 Info	tests/unit/config/config-watcher.test.ts:177	Test directly checks mockReloader.reload call count. This tests implementation rather than behavior - it verifies reload was called but not that the config was actually reloaded correctly. 💡 Suggestion Consider testing the outcome (e.g., onSwap callback was called with new config) rather than the implementation detail (reload was called).
🟢 Info	src/config/config-watcher.ts:59	collectLocalConfigDeps has deeply nested logic for handling different dependency types (extends, imports, workflow configs). The function is becoming complex and hard to maintain. 💡 Suggestion Consider extracting dependency collection for each type (extends, imports, workflow refs) into separate helper functions to improve readability and testability.
🟡 Warning	src/config/config-watcher.ts:28	collectLocalConfigDeps silently returns empty array on file read errors, making debugging difficult. Errors are caught but not logged, so users won't know why dependencies aren't being watched. 💡 Suggestion Log a warning when file read fails so users can diagnose missing dependencies. Consider differentiating between 'file not found' (expected for optional deps) and 'permission denied' (actual error).
🟡 Warning	src/config/config-watcher.ts:28	YAML parse errors are silently swallowed. Invalid YAML in dependency files will cause those dependencies to not be watched without any indication to the user. 💡 Suggestion Log a warning when YAML parsing fails, including the file path and parse error. This helps users identify malformed config files.
🟡 Warning	src/config/config-watcher.ts:28	Using 'any' type for parsed YAML loses type safety and makes the code harder to maintain. The function should use proper typing or at least 'unknown'. 💡 Suggestion Use 'unknown' instead of 'any' and add type guards or validation. Consider defining a minimal interface for config objects with extends/include/imports properties.
🟡 Warning	src/config/config-watcher.ts:195	watchFile catches errors but only logs warnings. If watching fails for a critical dependency (like the main config), the watcher will appear to work but won't detect changes. 💡 Suggestion Consider tracking watch failures and exposing a method to check if all critical files are being watched successfully. Log at higher severity for main config watch failures.
🟡 Warning	src/cli-main.ts:1341	Signal handler removes listeners synchronously but then calls process.kill asynchronously. There's a small window where another signal could arrive after removal but before kill, potentially causing duplicate cleanup. 💡 Suggestion Add a flag to track if cleanup is already in progress, and ignore subsequent signals while cleanup is running.
🟡 Warning	tests/unit/config/config-watcher.test.ts:121	Test uses magic number 200ms for debounce timing. This value is arbitrary and may cause flaky tests on slower systems. The test doesn't verify this is the correct debounce duration. 💡 Suggestion Extract debounce timing to a named constant (e.g., TEST_DEBOUNCE_MS) that's clearly related to the watcher's debounce parameter (50ms). Consider using a larger multiplier to ensure reliability.
🟡 Warning	tests/integration/config-reload-integration.test.ts:68	Test uses magic number 500ms for waiting on async operations. This arbitrary timeout may cause flaky tests on slow CI systems or fail to catch race conditions on fast systems. 💡 Suggestion Use a polling approach with a timeout and explicit assertion, or extract to a named constant with a comment explaining why this duration is sufficient.
🟡 Warning	tests/integration/config-reload-integration.test.ts:68	Integration tests rely on fixed timeouts (500ms) instead of waiting for actual completion. This can cause flaky tests - too short fails on slow systems, too long wastes time. 💡 Suggestion Use promises or callbacks to wait for the actual onSwap event, or implement a retry mechanism with explicit timeout.
🟡 Warning	tests/unit/config/config-watcher.test.ts:121	Unit test uses setTimeout(200) to wait for debounce. The watcher is configured with 50ms debounce, but 200ms may not be enough on heavily loaded systems. No assertion verifies the debounce completed. 💡 Suggestion Wait for the mockReloader.reload promise to resolve instead of using arbitrary timeout, or use a much larger multiplier (10x) with a comment explaining the safety margin.
🟡 Warning	src/config/config-watcher.ts:28	No tests verify error handling in collectLocalConfigDeps. The function silently returns empty arrays on errors, but there are no tests confirming this behavior or that errors are properly handled. 💡 Suggestion Add tests for: permission denied errors, malformed YAML in dependencies, circular references with errors, and verify that error paths don't crash the watcher.
🟡 Warning	tests/integration/config-reload-integration.test.ts:283	Test for invalid YAML only checks that onSwap isn't called and onError is called. It doesn't verify the watcher continues working after the error, or that subsequent valid changes are detected. 💡 Suggestion Add a test step that writes valid YAML after the invalid YAML and verifies the watcher recovers and detects the change.

---

Powered by Visor from Probelabs

Last updated: 2026-02-15T19:25:10.465Z | Triggered by: pr_opened | Commit: 95592a0

💡 TIP: You can chat with Visor using /visor ask <your question>