fix: replace sync fs calls in async contexts and silent catch blocks

openspec/changes/archive/2026-08-03-replace-sync-fs-calls-and-silent-catches/.openspec.yaml

@@ -0,0 +1,2 @@
+schema: spec-driven
+created: 2026-08-02

openspec/changes/archive/2026-08-03-replace-sync-fs-calls-and-silent-catches/design.md

@@ -0,0 +1,97 @@
+## Context
+
+The src/ audit (issue #683) identified two categories of violations against AGENTS.md §1.1:
+1. Synchronous fs operations in async contexts that block the Node.js event loop
+2. Bare silent catch blocks that swallow errors without logging
+
+These violations exist across 25+ files in the codebase. The codebase uses Node.js 24+ with ESM, and the existing pattern for async fs operations is well-established in many files (e.g., `src/tools/session_search.js`, `src/tools/sampling.js`, `src/scheduler/cron.js`).
+
+## Goals / Non-Goals
+
+**Goals:**
+- Convert all sync fs calls in async contexts to async equivalents from `node:fs/promises`
+- Replace all bare `catch {}` blocks with proper error logging via the existing `logger` singleton
+- Preserve existing function signatures, return types, and error handling semantics
+- Maintain backward compatibility — no breaking changes to public APIs
+
+**Non-Goals:**
+- Converting module-level sync fs calls in `config/loader.js` and `logger.js` (synchronous initialization)
+- Adding new error handling libraries or frameworks
+- Changing the behavior of error recovery — only adding logging
+- Converting sync fs calls in files that are definitively only called from sync contexts
+
+## Decisions
+
+### Decision 1: Use `node:fs/promises` instead of `util.promisify`
+
+**Choice:** Import `readFile`, `writeFile`, `readdir`, `stat`, `access`, `mkdir`, `unlink` from `node:fs/promises` directly.
+
+**Rationale:** This is the existing pattern throughout the codebase (e.g., `src/tools/session_search.js`, `src/tools/sampling.js`, `src/scheduler/cron.js`). It's cleaner than `util.promisify` and more readable.
+
+**Alternatives considered:**
+- `util.promisify(fs.readFileSync)` — adds indirection, not used elsewhere in the codebase
+- Third-party promise libraries — unnecessary dependency
+
+### Decision 2: Error logging severity levels
+
+**Choice:** Use `logger.debug()` for expected/non-critical failures (file not found, directory doesn't exist, YAML parse errors during discovery) and `logger.error()` for unexpected/critical failures (schedule execution failures, context load failures).
+
+**Rationale:** The existing `logger` singleton from `src/logger.js` provides structured JSON logging. Using `debug` for expected failures avoids log noise while still providing visibility. Using `error` for unexpected failures ensures they surface in monitoring.
+
+**Alternatives considered:**
+- Always use `logger.error()` — creates log noise for expected failures
+- Always use `logger.warn()` — doesn't distinguish between expected and unexpected failures
+- Re-throw all errors — would break existing error handling patterns in callers
+
+### Decision 3: Preserve sync fs in module-level initialization
+
+**Choice:** Leave `readFileSync` in `config/loader.js` and `logger.js` as-is.
+
+**Rationale:** These files are imported at module level and called synchronously during application startup. Converting them would require restructuring the entire initialization flow, which is out of scope for this fix.
+
+**Alternatives considered:**
+- Convert to async and make initialization async — would require changes to `index.js` and all subsystems
+- Leave as-is with a comment explaining why — acceptable for now, could be addressed in a future refactor
+
+### Decision 4: Handle `existsSync` → `access`
+
+**Choice:** Replace `existsSync(path)` with `access(path, constants.F_OK)` from `node:fs/promises`, wrapped in try/catch.
+
+**Rationale:** `access` is the async equivalent of `existsSync`. It throws if the file doesn't exist, so callers need to handle the error. This is consistent with existing patterns in the codebase (e.g., `src/tools/session_search.js:17`).
+
+**Alternatives considered:**
+- `stat` — more information than needed, slightly slower
+- `readFile` — overkill for existence check
+
+## Risks / Trade-offs
+
+### Risk 1: Performance regression in hot paths
+
+**Impact:** Async fs operations have slightly higher overhead than sync operations due to the event loop scheduling.
+
+**Mitigation:** The affected functions are not in hot paths — they are called during session loading, skill discovery, and prompt loading, which are infrequent operations. The performance difference is negligible.
+
+### Risk 2: Test failures due to async changes
+
+**Impact:** Tests that mock sync fs calls may need to be updated to mock async fs calls.
+
+**Mitigation:** Review test files for affected functions and update mocks to return promises. The existing test patterns in the codebase already use async mocks (e.g., `src/tools/session_search.test.js`).
+
+### Risk 3: Silent catch replacement changes behavior
+
+**Impact:** Replacing `catch {}` with `catch (err) { logger.debug(...) }` will add log output where there was none before.
+
+**Mitigation:** This is the intended behavior — silent catches are a bug. The debug-level logging ensures visibility without noise. Tests that verify no log output may need adjustment.
+
+## Migration Plan
+
+1. Convert sync fs calls to async in each file (10 files)
+2. Replace bare catch blocks with proper error handling (25+ files)
+3. Run `npm run test` to verify all tests pass
+4. Run `npm run lint` to verify linting passes
+5. Run `npm run coverage` to verify coverage is maintained
+6. Commit and push to the feature branch
+
+## Open Questions
+
+- None — all affected files have been identified and the approach is clear.

openspec/changes/archive/2026-08-03-replace-sync-fs-calls-and-silent-catches/proposal.md

@@ -0,0 +1,31 @@
+## Why
+
+The src/ audit (issue #683) identified two categories of violations against AGENTS.md §1.1: synchronous fs operations in async contexts that block the event loop, and bare silent catch blocks that swallow errors without logging. Both patterns degrade reliability and make debugging difficult.
+
+## What Changes
+
+- Convert synchronous fs calls (`readFileSync`, `writeFileSync`, `readdirSync`, `statSync`, `existsSync`, `mkdirSync`, `unlinkSync`) to async counterparts (`readFile`, `writeFile`, `readdir`, `stat`, `access`, `mkdir`, `unlink`) from `node:fs/promises` in all files called from async contexts
+- Replace bare `catch {}` blocks with proper error handling using the existing `logger` singleton — `logger.debug()` for expected/non-critical failures, `logger.error()` for unexpected/critical failures
+- Preserve module-level sync fs calls in `config/loader.js` and `logger.js` where they are used during synchronous initialization
+
+## Capabilities
+
+### New Capabilities
+- `async-fs`: Requirement that all fs operations in async contexts use `node:fs/promises` instead of blocking `node:fs`
+- `error-logging`: Requirement that all catch blocks log errors via the structured logger or re-throw — no silent catches
+
+### Modified Capabilities
+- None — no existing spec-level behavior changes, only implementation improvements
+
+## Impact
+
+- **Affected files**: `src/memory/context.js`, `src/memory/prompts.js`, `src/skills/registry.js`, `src/memory/reader.js`, `src/memory/writer.js`, `src/session/loader.js`, `src/memory/profile.js`, `src/sandbox/runner.js`, `src/skills/discoverer.js`, `src/memory/retention.js`, `src/memory/expireEphemeral.js`, `src/scheduler/scheduler.js`, `src/scheduler/cron.js`, `src/agent/agents/coding.js`, `src/agent/agents/debug.js`, `src/agent/agents/documentation.js`, `src/agent/agents/code-review.js`, `src/agent/agents/search.js`, `src/agent/agents/security-audit.js`, `src/agent/agents/performance.js`, `src/agent/agents/testing.js`, `src/agent/agents/research.js`, `src/tui/contextTokens.js`, `src/tui/statusBar.js`, `src/workspace/loadAgents.js`
+- **APIs**: No breaking changes — function signatures and return types remain identical
+- **Dependencies**: No new dependencies — uses existing `node:fs/promises` and `src/logger.js`
+- **Tests**: Existing tests must continue to pass; some tests may need adjustment if they mock sync fs calls
+
+## Non-goals
+
+- Converting module-level sync fs calls in `config/loader.js` and `logger.js` (these are synchronous initialization)
+- Adding new error handling frameworks or libraries
+- Changing the behavior of error recovery — only adding logging, not changing what gets caught

openspec/changes/archive/2026-08-03-replace-sync-fs-calls-and-silent-catches/specs/async-fs/spec.md

@@ -0,0 +1,35 @@
+## ADDED Requirements
+
+### Requirement: Async fs operations in async contexts
+The system SHALL use `node:fs/promises` for all fs operations (`readFile`, `writeFile`, `readdir`, `stat`, `access`, `mkdir`, `unlink`) in functions that are async or called from async contexts. Synchronous fs operations (`readFileSync`, `writeFileSync`, `readdirSync`, `statSync`, `existsSync`, `mkdirSync`, `unlinkSync`) are prohibited in async contexts per AGENTS.md §1.1.
+
+#### Scenario: loadContext uses async fs
+- **WHEN** `loadContext()` in `src/memory/context.js` reads context files
+- **THEN** it uses `readFile` and `readdir` from `node:fs/promises` instead of `readFileSync` and `readdirSync`
+
+#### Scenario: loadSystemPrompt uses async fs
+- **WHEN** `loadSystemPrompt()` in `src/memory/prompts.js` reads the system prompt file
+- **THEN** it uses `readFile` from `node:fs/promises` instead of `readFileSync`
+
+#### Scenario: getSkillBody uses async fs
+- **WHEN** `getSkillBody()` in `src/skills/registry.js` reads a skill's SKILL.md body
+- **THEN** it uses `readFile` from `node:fs/promises` instead of `readFileSync`
+
+#### Scenario: Module-level sync fs preserved
+- **WHEN** `config/loader.js` or `logger.js` perform module-level initialization
+- **THEN** synchronous fs operations are preserved (they are not called from async contexts during initialization)
+
+### Requirement: No blocking fs in async call chains
+The system SHALL ensure that no file in the async call chain uses blocking fs operations. If a function is called from an async context, all fs operations within it and its transitive callees must be async.
+
+#### Scenario: detectShebang uses async fs
+- **WHEN** `detectShebang()` in `src/sandbox/runner.js` reads a script's first line
+- **THEN** it uses `readFile` and `access` from `node:fs/promises` instead of `readFileSync` and `existsSync`
+
+#### Scenario: discoverSkills uses async fs
+- **WHEN** `discoverSkills()` in `src/skills/discoverer.js` scans skill directories
+- **THEN** it uses `readdir`, `stat`, `readFile`, and `access` from `node:fs/promises` instead of sync equivalents
+
+#### Scenario: loadSession uses async fs
+- **WHEN** `loadSession()` in `src/session/loader.js` reads session files
+- **THEN** it uses `readFile`, `readdir`, and `stat` from `node:fs/promises` instead of sync equivalents

openspec/changes/archive/2026-08-03-replace-sync-fs-calls-and-silent-catches/specs/error-logging/spec.md

@@ -0,0 +1,27 @@
+## ADDED Requirements
+
+### Requirement: All catch blocks log errors
+The system SHALL log all caught errors using the structured `logger` singleton from `src/logger.js`. No bare `catch {}` blocks are permitted per AGENTS.md §1.1.
+
+#### Scenario: Expected failures use debug level
+- **WHEN** a file not found error occurs during skill discovery
+- **THEN** the catch block logs via `logger.debug()` with the error message
+
+#### Scenario: Unexpected failures use error level
+- **WHEN** a schedule execution fails unexpectedly
+- **THEN** the catch block logs via `logger.error()` with the error message
+
+#### Scenario: Silent catches are eliminated
+- **WHEN** any file in the codebase has a `catch {}` block
+- **THEN** it has been replaced with `catch (err) { logger.debug(...) }` or `catch (err) { logger.error(...) }`
+
+### Requirement: Error logging preserves existing semantics
+The system SHALL preserve existing error handling semantics — errors that were previously swallowed should still be handled gracefully, but now with logging. Errors that were previously re-thrown should continue to be re-thrown.
+
+#### Scenario: Graceful degradation with logging
+- **WHEN** `loadSystemPrompt()` fails to find the system prompt file
+- **THEN** it logs the error and returns an empty string (same behavior as before, but now logged)
+
+#### Scenario: Retention cleanup with logging
+- **WHEN** `cleanRetainedMemory()` encounters a directory it cannot read
+- **THEN** it logs the error and returns 0 (same behavior as before, but now logged)

openspec/changes/archive/2026-08-03-replace-sync-fs-calls-and-silent-catches/tasks.md

@@ -0,0 +1,86 @@
+## 1. Convert sync fs in memory module
+
+- [ ] 1.1 Convert `src/memory/context.js` — replace `readdirSync`/`readFileSync` with `readdir`/`readFile` from `node:fs/promises`, make `loadContext()` async
+- [ ] 1.2 Convert `src/memory/reader.js` — replace `readFileSync`/`existsSync` with `readFile`/`access` from `node:fs/promises`, make `readMemoryFile()` async
+- [ ] 1.3 Convert `src/memory/writer.js` — replace `mkdirSync`/`writeFileSync` with `mkdir`/`writeFile` from `node:fs/promises`, make `writeMemoryFile()` async
+- [ ] 1.4 Convert `src/memory/profile.js` — replace `readFileSync`/`writeFileSync`/`mkdirSync`/`existsSync`/`renameSync` with async equivalents, make `loadProfile()` and `saveProfile()` async
+- [ ] 1.5 Convert `src/memory/prompts.js` — replace `readFileSync` with `readFile` from `node:fs/promises`, make `loadSystemPrompt()` async
+- [ ] 1.6 Convert `src/memory/retention.js` — replace `readdirSync`/`statSync`/`unlinkSync` with async equivalents, make `cleanRetainedMemory()` and `enforceMaxEntries()` async
+
+## 2. Convert sync fs in skills module
+
+- [ ] 2.1 Convert `src/skills/discoverer.js` — replace `readdirSync`/`statSync`/`readFileSync`/`existsSync` with async equivalents, make `findSkillFiles()` and `discoverSkills()` async
+- [ ] 2.2 Convert `src/skills/registry.js` — replace `readFileSync` with `readFile` from `node:fs/promises`, make `getSkillBody()` async
+
+## 3. Convert sync fs in session module
+
+- [ ] 3.1 Convert `src/session/loader.js` — replace `readdirSync`/`readFileSync`/`statSync` with async equivalents, make `loadSession()` and `loadFile()` async
+
+## 4. Convert sync fs in sandbox module
+
+- [ ] 4.1 Convert `src/sandbox/runner.js` — replace `existsSync`/`readFileSync` with `access`/`readFile` from `node:fs/promises`, make `detectShebang()` async
+
+## 5. Convert sync fs in agent module
+
+- [ ] 5.1 Convert `src/agent/agents/coding.js` — replace `readFileSync` with `readFile` from `node:fs/promises`, make `loadCodingPrompt()` async
+- [ ] 5.2 Convert `src/agent/agents/debug.js` — replace `readFileSync` with `readFile` from `node:fs/promises`, make `loadDebugPrompt()` async
+- [ ] 5.3 Convert `src/agent/agents/documentation.js` — replace `readFileSync` with `readFile` from `node:fs/promises`, make `loadDocumentationPrompt()` async
+- [ ] 5.4 Convert `src/agent/agents/code-review.js` — replace `readFileSync` with `readFile` from `node:fs/promises`, make `loadCodeReviewPrompt()` async
+- [ ] 5.5 Convert `src/agent/agents/search.js` — replace `readFileSync` with `readFile` from `node:fs/promises`, make `loadSearchPrompt()` async
+- [ ] 5.6 Convert `src/agent/agents/security-audit.js` — replace `readFileSync` with `readFile` from `node:fs/promises`, make `loadSecurityAuditPrompt()` async
+- [ ] 5.7 Convert `src/agent/agents/performance.js` — replace `readFileSync` with `readFile` from `node:fs/promises`, make `loadPerformancePrompt()` async
+- [ ] 5.8 Convert `src/agent/agents/testing.js` — replace `readFileSync` with `readFile` from `node:fs/promises`, make `loadTestingPrompt()` async
+- [ ] 5.9 Convert `src/agent/agents/research.js` — replace `readFileSync` with `readFile` from `node:fs/promises`, make `loadResearchPrompt()` async
+
+## 6. Replace silent catch blocks
+
+- [ ] 6.1 Replace bare `catch {}` in `src/memory/retention.js` (lines 29, 63) with `catch (err) { logger.debug(...) }`
+- [ ] 6.2 Replace bare `catch {}` in `src/memory/expireEphemeral.js` (lines 23, 52, 64) with `catch (err) { logger.debug(...) }`
+- [ ] 6.3 Replace bare `catch {}` in `src/scheduler/scheduler.js` (lines 65, 69, 197, 200) with `catch (err) { logger.error(...) }`
+- [ ] 6.4 Replace bare `catch {}` in `src/scheduler/cron.js` (lines 429, 432, 439, 475, 479) with `catch (err) { logger.debug(...) }`
+- [ ] 6.5 Replace bare `catch {}` in `src/skills/discoverer.js` (lines 42, 57, 81, 94, 172) with `catch (err) { logger.debug(...) }`
+- [ ] 6.6 Replace bare `catch {}` in `src/agent/agents/coding.js` (line 17) with `catch (err) { logger.debug(...) }`
+- [ ] 6.7 Replace bare `catch {}` in `src/agent/agents/debug.js` (line 17) with `catch (err) { logger.debug(...) }`
+- [ ] 6.8 Replace bare `catch {}` in `src/agent/agents/documentation.js` (line 17) with `catch (err) { logger.debug(...) }`
+- [ ] 6.9 Replace bare `catch {}` in `src/agent/agents/code-review.js` (line 17) with `catch (err) { logger.debug(...) }`
+- [ ] 6.10 Replace bare `catch {}` in `src/agent/agents/search.js` (line 17) with `catch (err) { logger.debug(...) }`
+- [ ] 6.11 Replace bare `catch {}` in `src/agent/agents/security-audit.js` (line 17) with `catch (err) { logger.debug(...) }`
+- [ ] 6.12 Replace bare `catch {}` in `src/agent/agents/performance.js` (line 17) with `catch (err) { logger.debug(...) }`
+- [ ] 6.13 Replace bare `catch {}` in `src/agent/agents/testing.js` (line 17) with `catch (err) { logger.debug(...) }`
+- [ ] 6.14 Replace bare `catch {}` in `src/agent/agents/research.js` (line 17) with `catch (err) { logger.debug(...) }`
+- [ ] 6.15 Replace bare `catch {}` in `src/tui/contextTokens.js` (lines 21, 40) with `catch (err) { logger.debug(...) }`
+- [ ] 6.16 Replace bare `catch {}` in `src/tui/statusBar.js` (line 35) with `catch (err) { logger.debug(...) }`
+- [ ] 6.17 Replace bare `catch {}` in `src/workspace/loadAgents.js` (line 16) with `catch (err) { logger.debug(...) }`
+
+## 7. Update callers of converted functions
+
+- [ ] 7.1 Update `src/agent/deepAgents.js` — await `loadSystemPrompt()` and `skillRegistry.discover()`
+- [ ] 7.2 Update `src/scheduler/scheduler.js` — await `loadContext()`
+- [ ] 7.3 Update `src/tui/app.js` — await `loadSystemPrompt()` calls
+- [ ] 7.4 Update `src/tools/skills.js` — await `skillRegistry.getSkillBody()` and `skillRegistry.discover()`
+- [ ] 7.5 Update `src/tools/session_search.js` — await `parseFrontmatter` calls (if needed)
+- [ ] 7.6 Update `src/session/factory.js` — await `loadSession()`
+- [ ] 7.7 Update `src/session/onboarding.js` — await `saveProfile()`
+- [ ] 7.8 Update `src/skills/registry.js` — await `discoverSkills()` in `discover()` method
+
+## 8. Update tests
+
+- [ ] 8.1 Update `tests/unit/memory/context.test.js` — mock async fs calls
+- [ ] 8.2 Update `tests/unit/memory/reader.test.js` — mock async fs calls
+- [ ] 8.3 Update `tests/unit/memory/writer.test.js` — mock async fs calls
+- [ ] 8.4 Update `tests/unit/memory/profile.test.js` — mock async fs calls
+- [ ] 8.5 Update `tests/unit/memory/prompts.test.js` — mock async fs calls
+- [ ] 8.6 Update `tests/unit/memory/retention.test.js` — mock async fs calls
+- [ ] 8.7 Update `tests/unit/skills/discoverer.test.js` — mock async fs calls
+- [ ] 8.8 Update `tests/unit/skills/registry.test.js` — mock async fs calls
+- [ ] 8.9 Update `tests/unit/session/loader.test.js` — mock async fs calls
+- [ ] 8.10 Update `tests/unit/sandbox/runner.test.js` — mock async fs calls
+- [ ] 8.11 Update `tests/unit/agent/agents/*.test.js` — mock async fs calls
+
+## 9. Verification
+
+- [ ] 9.1 Run `npm run test` — all tests pass
+- [ ] 9.2 Run `npm run lint` — no lint errors
+- [ ] 9.3 Run `npm run coverage` — coverage maintained
+- [ ] 9.4 Verify no sync fs calls remain in async contexts: `grep -rn 'readFileSync\|writeFileSync\|readdirSync\|statSync\|existsSync\|mkdirSync\|unlinkSync' src/ --include='*.js' | grep -v 'config/loader.js' | grep -v 'logger.js'`
+- [ ] 9.5 Verify no bare catch blocks remain: `grep -rn 'catch {' src/ --include='*.js' | grep -v '.test.'`