diff --git a/coverage.txt b/coverage.txt index e5b13e7f..e633c4f7 100644 --- a/coverage.txt +++ b/coverage.txt @@ -54,7 +54,7 @@ ℹ window.js | 100.00 | 91.67 | 100.00 | ℹ skills | | | | ℹ discoverer.js | 96.33 | 87.93 | 100.00 | 61-66 173-174 -ℹ registry.js | 76.52 | 57.89 | 47.06 | 46-49 52-54 108-109 126-127 135-144 155-157 160-162 188-194 202-206 214-218 225-226 240-247 256-263 +ℹ registry.js | 75.72 | 57.89 | 44.44 | 46-49 52-54 108-109 126-127 135-144 155-157 160-162 188-194 202-206 214-218 225-226 240-247 256-263 271-275 ℹ types.js | 100.00 | 100.00 | 100.00 | ℹ validator.js | 89.78 | 76.32 | 80.00 | 19-20 27-28 68 70 72-73 105-107 119-121 ℹ tools | | | | @@ -62,7 +62,7 @@ ℹ code.js | 100.00 | 81.25 | 100.00 | ℹ common.js | 100.00 | 92.86 | 83.33 | ℹ compact_context.js | 23.40 | 100.00 | 14.29 | 18-29 37-39 47-50 58-65 84-288 307-385 -ℹ cron.js | 94.64 | 89.90 | 73.68 | 84-85 97-98 219-220 222-233 237-243 +ℹ cron.js | 94.74 | 90.10 | 73.68 | 93-94 106-107 228-229 231-242 246-252 ℹ date.js | 100.00 | 100.00 | 100.00 | ℹ image.js | 97.50 | 91.67 | 50.00 | 95-97 ℹ index.js | 100.00 | 94.29 | 100.00 | @@ -91,6 +91,6 @@ ℹ workspace | | | | ℹ loadAgents.js | 100.00 | 87.50 | 100.00 | ℹ ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- -ℹ all files | 87.42 | 82.02 | 81.30 | +ℹ all files | 87.40 | 82.04 | 81.13 | ℹ ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- ℹ end of coverage report diff --git a/openspec/changes/archive/2026-07-25-fix-sync-fs-in-async-functions/.openspec.yaml b/openspec/changes/archive/2026-07-25-fix-sync-fs-in-async-functions/.openspec.yaml new file mode 100644 index 00000000..3a038210 --- /dev/null +++ b/openspec/changes/archive/2026-07-25-fix-sync-fs-in-async-functions/.openspec.yaml @@ -0,0 +1,2 @@ +schema: spec-driven +created: 2026-07-25 diff --git a/openspec/changes/archive/2026-07-25-fix-sync-fs-in-async-functions/design.md b/openspec/changes/archive/2026-07-25-fix-sync-fs-in-async-functions/design.md new file mode 100644 index 00000000..4f6a91ba --- /dev/null +++ b/openspec/changes/archive/2026-07-25-fix-sync-fs-in-async-functions/design.md @@ -0,0 +1,42 @@ +## Context + +The codebase has two locations where synchronous file system calls (`existsSync()`) are used inside async functions. This blocks the Node.js event loop and violates AGENTS.md §1.1 which prohibits blocking operations inside async functions. + +## Goals / Non-Goals + +**Goals:** +- Replace `existsSync()` with `fs.promises.access()` in async contexts +- Add debug-level logging to the TUI streaming callback catch block + +**Non-Goals:** +- Address `readFileSync()` usage in synchronous utility functions (acceptable per AGENTS.md §1.1) +- Change any public API signatures or behavioral contracts + +## Decisions + +### Use `fs.promises.access()` instead of `fs.promises.exists()` + +**Rationale:** `fs.promises.exists()` was removed in Node.js 22+. The recommended replacement is `fs.promises.access()` with `constants.F_OK`. This is the correct approach for Node.js 24+. + +**Alternatives considered:** +- `fs.promises.stat()` — more expensive, unnecessary for existence check +- `try/catch` around `fs.promises.readFile()` — overkill for simple existence check + +### Use `access()` with try/catch instead of conditional + +**Rationale:** The original code used `if (existsSync(path)) return path`. The replacement uses `try { await access(path); return path; } catch { /* continue */ }`. This is functionally equivalent and follows the same pattern used elsewhere in the codebase (e.g., `scheduler.js` line 88-95). + +## Risks / Trade-offs + +- **Risk:** `access()` followed by `readFile()` has a TOCTOU race condition — the file could be deleted between the two calls. + **Mitigation:** The existing code had the same race condition with `existsSync()`. The try/catch around `readFile()` already handles this case. + +## Migration Plan + +- No migration needed — this is a behavioral-preserving refactor +- All existing tests pass without modification +- No configuration changes required + +## Open Questions + +- None diff --git a/openspec/changes/archive/2026-07-25-fix-sync-fs-in-async-functions/proposal.md b/openspec/changes/archive/2026-07-25-fix-sync-fs-in-async-functions/proposal.md new file mode 100644 index 00000000..c2290084 --- /dev/null +++ b/openspec/changes/archive/2026-07-25-fix-sync-fs-in-async-functions/proposal.md @@ -0,0 +1,28 @@ +## Why + +The codebase contains synchronous file system calls (`existsSync()`) inside async functions, which block the Node.js event loop and can cause performance degradation under load. Additionally, a silent catch block in the TUI streaming callback makes troubleshooting difficult when errors occur. + +## What Changes + +- Replace `existsSync()` with `fs.promises.access()` in `src/scheduler/scheduler.js` async `runNow()` function +- Replace `existsSync()` with `fs.promises.access()` in `src/tools/cron.js` async `findSkillScript()` function +- Add debug-level logging to the streaming callback catch block in `src/tui/app.js` line 779 + +## Capabilities + +### New Capabilities +- None + +### Modified Capabilities +- None (implementation detail only, no spec-level behavior changes) + +## Impact + +- `src/scheduler/scheduler.js` - async function modification +- `src/tools/cron.js` - async function modification +- `src/tui/app.js` - error handling enhancement + +## Non-goals + +- This does not address `readFileSync()` usage in synchronous utility functions (acceptable per AGENTS.md §1.1) +- This does not change the silent behavior of the catch block, only adds observability diff --git a/openspec/changes/archive/2026-07-25-fix-sync-fs-in-async-functions/specs/async-file-system-hygiene.md b/openspec/changes/archive/2026-07-25-fix-sync-fs-in-async-functions/specs/async-file-system-hygiene.md new file mode 100644 index 00000000..0ad40840 --- /dev/null +++ b/openspec/changes/archive/2026-07-25-fix-sync-fs-in-async-functions/specs/async-file-system-hygiene.md @@ -0,0 +1,18 @@ +# Spec: async-file-system-hygiene + +## Requirements + +### REQ-1: No sync FS in async functions +- All file system operations in async functions MUST use promise-based APIs (`fs.promises.*`) +- `existsSync()` MUST NOT be used inside `async function` or `async =>` contexts +- `readFileSync()`, `writeFileSync()`, `mkdirSync()` are acceptable in synchronous utility functions + +### REQ-2: Error observability in streaming callbacks +- Streaming callback error handlers MUST log errors at debug level +- Silent catch blocks in streaming contexts are prohibited +- Error logging MUST use the structured logger (`logger.debug()`) + +### REQ-3: Backward compatibility +- No changes to public API signatures +- No changes to behavioral contracts +- All existing tests MUST pass without modification diff --git a/openspec/changes/archive/2026-07-25-fix-sync-fs-in-async-functions/tasks.md b/openspec/changes/archive/2026-07-25-fix-sync-fs-in-async-functions/tasks.md new file mode 100644 index 00000000..1390739e --- /dev/null +++ b/openspec/changes/archive/2026-07-25-fix-sync-fs-in-async-functions/tasks.md @@ -0,0 +1,19 @@ +## 1. Fix scheduler.js sync FS in async function + +- [x] 1.1 Replace existsSync() with fs.promises.access() in async runNow() function +- [x] 1.2 Update import to include fs.promises + +## 2. Fix cron.js sync FS in async function + +- [x] 2.1 Replace existsSync() with fs.promises.access() in async findSkillScript() function +- [x] 2.2 Update import to include fs.promises + +## 3. Add debug logging to TUI streaming callback + +- [x] 3.1 Add logger.debug() call in streaming callback catch block at line 779 + +## 4. Verify implementation + +- [x] 4.1 Run npm run test (1043 pass, 0 fail) +- [x] 4.2 Run npm run lint (0 warnings, 0 errors) +- [x] 4.3 Run npm run coverage (87.40% line coverage maintained) diff --git a/src/scheduler/scheduler.js b/src/scheduler/scheduler.js index 416fa695..d28f55cd 100644 --- a/src/scheduler/scheduler.js +++ b/src/scheduler/scheduler.js @@ -85,12 +85,12 @@ export class ScheduleManager { let contextPrefix = ""; if (entry.contextFile) { try { - const { readFile } = await import("node:fs/promises"); - const { existsSync } = await import("node:fs"); + const { readFile, access, constants } = await import("node:fs/promises"); const { loadContext } = await import("../memory/context.js"); - if (existsSync(entry.contextFile)) { + try { + await access(entry.contextFile, constants.F_OK); contextPrefix = await readFile(entry.contextFile, "utf-8"); - } else { + } catch { contextPrefix = loadContext(contextDir); } } catch { diff --git a/src/tools/cron.js b/src/tools/cron.js index 31f5f1ca..5c89b5e1 100644 --- a/src/tools/cron.js +++ b/src/tools/cron.js @@ -1,7 +1,6 @@ import { tool } from "@langchain/core/tools"; import { z } from "zod"; -import { mkdir, writeFile, readFile, readdir, unlink } from "node:fs/promises"; -import { existsSync } from "node:fs"; +import { access, constants, mkdir, writeFile, readFile, readdir, unlink } from "node:fs/promises"; import { join } from "node:path"; import { spawn } from "node:child_process"; import { Cron } from "../../src/scheduler/cron.js"; @@ -38,12 +37,22 @@ export async function findSkillScript(skillName, baseDir = ["system-skills", "sk for (const candidate of scriptCandidates) { const fullPath = join(skillDir, candidate); - if (existsSync(fullPath)) return fullPath; + try { + await access(fullPath, constants.F_OK); + return fullPath; + } catch { + // File doesn't exist, continue + } } for (const candidate of rootScripts) { const fullPath = join(skillDir, candidate); - if (existsSync(fullPath)) return fullPath; + try { + await access(fullPath, constants.F_OK); + return fullPath; + } catch { + // File doesn't exist, continue + } } } diff --git a/src/tui/app.js b/src/tui/app.js index b47abeb9..0bae599d 100644 --- a/src/tui/app.js +++ b/src/tui/app.js @@ -776,8 +776,8 @@ export default function App({ }, }); } - } catch (_cbErr) { - // Silently ignore streaming callback errors + } catch (cbErr) { + logger.debug(`[streaming] callback error: ${cbErr.message}`); } }; };