From 9aef795642e6b1612261e9ab8ee9931cb6180b07 Mon Sep 17 00:00:00 2001 From: qnbs <155236708+qnbs@users.noreply.github.com> Date: Tue, 25 Aug 2026 15:32:57 +0200 Subject: [PATCH 1/3] refactor(hooks): bound subprocess lifecycle in pre-commit/pre-push runners scripts/hooks/shared.mjs's runNodeScript/runLocalBinary used spawnSync with no timeout and no process-group cleanup: a hung child (e.g. a stuck tsgo or lint-staged process) blocked the hook indefinitely, and parent-signal delivery (Ctrl-C, SIGTERM) did not reliably terminate detached descendants. Add runBounded(): an async, spawn-based runner with a timeout watchdog, SIGTERM-then-SIGKILL escalation, process-group termination (falls back to direct child.kill when a group is unavailable, e.g. Windows), and signal forwarding for SIGINT/SIGTERM/SIGHUP so the outer hook process can't outlive its children. runNodeScript/runLocalBinary become thin async wrappers over it, preserving their existing exit-code contract for callers; runNodeScriptDetailed/runLocalBinaryDetailed expose the full result (status, signal, timedOut, interrupted) for callers that need it. This is a breaking change to shared.mjs's calling convention (sync to async), so every current caller is updated in the same commit: pre-commit.mjs and scripts/ci-prepush-lowend.mjs (runCheck/main made async, every call site awaited) now correctly await results instead of treating a Promise as a synchronous exit code. pre-push.mjs keeps its existing --prepush-evidence-file temp-file evidence contract unchanged -- only the two runNodeScript call sites gained the required await. Out of scope: a further branch has reworked pre-push.mjs, verify- outgoing.mjs, and check-git-diff.mjs onto a new env-var/exact-tree evidence contract (WORLD_SCRIPT_PREPUSH_UPDATES/_EXACT_TREE/_EXACT_FILES). That is a separate, larger architectural change coupling ci-prepush- lowend.mjs's evidence resolution to a new design and is deferred to its own dedicated wave rather than folded in here. README test-count badges resynced via `pnpm run sync:readme` for the added test file (578 files / 6987+ tests). Validated: full `pnpm run ci:prepush`, `pnpm run lint`, `pnpm run typecheck` (4-checker), and the new tests/unit/hooks/shared.test.ts all pass. --- README.md | 8 +- scripts/ci-prepush-lowend.mjs | 28 ++--- scripts/hooks/pre-commit.mjs | 4 +- scripts/hooks/pre-push.mjs | 4 +- scripts/hooks/shared.d.mts | 22 ++++ scripts/hooks/shared.mjs | 203 +++++++++++++++++++++++++++++--- tests/unit/hooks/shared.test.ts | 50 ++++++++ 7 files changed, 280 insertions(+), 39 deletions(-) create mode 100644 scripts/hooks/shared.d.mts create mode 100644 tests/unit/hooks/shared.test.ts diff --git a/README.md b/README.md index e398883a5..99dea6c22 100644 --- a/README.md +++ b/README.md @@ -14,7 +14,7 @@ IndexedDB v8 PWA v3.0 i18n 19 locales — 2925 keys - 6984+ tests / 577 files + 6987+ tests / 578 files Codecov Coverage License MIT CI Status @@ -512,7 +512,7 @@ The Settings → AI panel shows a live GPU status badge with adapter details and | **Document Export** | docx + jszip | Word-compatible `.docx` generation (lazy-loaded) | | **PWA** | Service Worker + Web App Manifest v3 | Offline support, installability, Workbox chunking | | **i18n** | Custom React Context (`I18nContext.tsx`) | 2925 keys × 19 locales (de/en/es/fr/it + ar/he/fa RTL Beta + ja/zh/pt/el/fi/sv/hu/is/eu/ru/ko Beta); EN fallback; `localStorage` persistence | -| **Testing** | Vitest 4.x (6984+ tests / 577 files) + Playwright E2E | Unit/integration + cross-browser E2E; Stryker mutation (manual workflow) | +| **Testing** | Vitest 4.x (6987+ tests / 578 files) + Playwright E2E | Unit/integration + cross-browser E2E; Stryker mutation (manual workflow) | | **Code Quality** | Biome (lint + format) + TypeScript 7 (tsgo) strict | `--error-on-warnings` in CI; zero `any` policy | | **Visualization** | Force-directed graph | Interactive character relationship network | | **Desktop** | Tauri v2 | Cross-platform installer; auto-updater via `latest.json` | @@ -550,7 +550,7 @@ WorldScript-Studio/ │ ├── sw.js # PWA Service Worker │ └── manifest.json # PWA Web App Manifest v3 ├── tests/ -│ ├── unit/ # Vitest unit tests (6984+ tests, 577 files) — count spans tests/, components/, packages/*/tests/, not just this folder +│ ├── unit/ # Vitest unit tests (6987+ tests, 578 files) — count spans tests/, components/, packages/*/tests/, not just this folder │ │ ├── ai/ # aiSmallModules, aiCoreFallbackPaths │ │ └── settings/ # WebLlmPanel, AiSections │ └── e2e/ # Playwright specs + helpers.ts @@ -712,7 +712,7 @@ The main pipeline is [`.github/workflows/ci.yml`](.github/workflows/ci.yml). Opt | `scorecard` | weekly + `main` push | OpenSSF Scorecard — SARIF uploaded to GitHub Code Scanning | **Current test metrics (2026-08-21, source-synchronized; CI remains authoritative for pass/fail):** -- **6984+ unit tests** across **577 test files** — CI is authoritative for pass/fail +- **6987+ unit tests** across **578 test files** — CI is authoritative for pass/fail - Coverage thresholds: lines ≥ 80 · branches ≥ 66 · functions ≥ 72 · statements ≥ 78 — enforced in CI (see Codecov badge for live metrics) - i18n: **2925 keys × 19 locales** (en/de/fr/es/it + ar/he/fa RTL Beta + ja/zh/pt/el/fi/sv/hu/is/eu/ru/ko Beta) diff --git a/scripts/ci-prepush-lowend.mjs b/scripts/ci-prepush-lowend.mjs index 4dae666ad..82a6d2f20 100644 --- a/scripts/ci-prepush-lowend.mjs +++ b/scripts/ci-prepush-lowend.mjs @@ -13,13 +13,13 @@ function report(name, status, detail = '') { return status; } -function runCheck(name, run) { - const status = run(); +async function runCheck(name, run) { + const status = await run(); report(name, status === 0 ? 'PASS' : 'FAIL'); if (status !== 0) process.exit(status ?? 1); } -function main() { +async function main() { const evidenceIndex = process.argv.indexOf('--prepush-evidence-file'); const evidenceFile = evidenceIndex >= 0 ? process.argv[evidenceIndex + 1] : undefined; const fullRequested = process.argv.includes('--full'); @@ -51,18 +51,18 @@ function main() { } report('Dependency state', 'PASS'); - runCheck('Toolchain', () => runNodeScript('scripts/check-pnpm-toolchain.mjs', ['--hook'])); - runCheck('Docs/release truth', () => runNodeScript('scripts/check-doc-metrics.mjs')); - runCheck('CSP policy', () => runNodeScript('scripts/check-csp-policy.mjs')); - runCheck('Desktop import boundary', () => + await runCheck('Toolchain', () => runNodeScript('scripts/check-pnpm-toolchain.mjs', ['--hook'])); + await runCheck('Docs/release truth', () => runNodeScript('scripts/check-doc-metrics.mjs')); + await runCheck('CSP policy', () => runNodeScript('scripts/check-csp-policy.mjs')); + await runCheck('Desktop import boundary', () => runNodeScript('scripts/check-tauri-import-boundary.mjs'), ); - runCheck('Native readiness', () => runNodeScript('scripts/check-native-readiness.mjs')); + await runCheck('Native readiness', () => runNodeScript('scripts/check-native-readiness.mjs')); if (shouldRunAdmissionCheck('i18n', classification.files) || full) { - runCheck('i18n key parity', () => runNodeScript('scripts/check-i18n-keys.mjs')); - runCheck('i18n bundle rebuild', () => runNodeScript('scripts/build-i18n.mjs')); - runCheck('i18n translation quality', () => + await runCheck('i18n key parity', () => runNodeScript('scripts/check-i18n-keys.mjs')); + await runCheck('i18n bundle rebuild', () => runNodeScript('scripts/build-i18n.mjs')); + await runCheck('i18n translation quality', () => runNodeScript('scripts/i18n-quality-report.mjs', [ '--strict', '--min-coverage', @@ -74,10 +74,10 @@ function main() { } if (shouldRunAdmissionCheck('contentGuard', classification.files) || full) - runCheck('Content guard', () => runNodeScript('scripts/content-guard.mjs')); + await runCheck('Content guard', () => runNodeScript('scripts/content-guard.mjs')); if (typecheckRequired) { - runCheck('TypeScript (single checker)', () => + await runCheck('TypeScript (single checker)', () => // QNBS-v3: one checker bounds memory use on constrained developer machines. runLocalBinary('tsgo', ['--project', 'tsconfig.tsgo.json', '--noEmit', '--checkers', '1']), ); @@ -92,4 +92,4 @@ function main() { } // QNBS-v3: guard execution so this module can be imported for testing without running the CLI. -if (isMainModule(process.argv[1], import.meta.url)) main(); +if (isMainModule(process.argv[1], import.meta.url)) await main(); diff --git a/scripts/hooks/pre-commit.mjs b/scripts/hooks/pre-commit.mjs index 9a6ef9eec..42fae9771 100644 --- a/scripts/hooks/pre-commit.mjs +++ b/scripts/hooks/pre-commit.mjs @@ -1,6 +1,6 @@ import process from 'node:process'; import { ensureDependencyState, runLocalBinary, runNodeScript } from './shared.mjs'; -if (runNodeScript('scripts/signing/doctor.mjs', ['--hook']) !== 0) process.exit(1); +if ((await runNodeScript('scripts/signing/doctor.mjs', ['--hook'])) !== 0) process.exit(1); if (!ensureDependencyState()) process.exit(1); -process.exit(runLocalBinary('lint-staged')); +process.exit(await runLocalBinary('lint-staged')); diff --git a/scripts/hooks/pre-push.mjs b/scripts/hooks/pre-push.mjs index c97c0a1b5..0de69a923 100644 --- a/scripts/hooks/pre-push.mjs +++ b/scripts/hooks/pre-push.mjs @@ -18,8 +18,8 @@ try { evidenceFile = join(evidenceDir, 'evidence.json'); writePrePushEvidenceFile(evidenceFile, updates); const childArgs = [...process.argv.slice(2), '--prepush-evidence-file', evidenceFile]; - exitCode = runNodeScript('scripts/signing/verify-outgoing.mjs', childArgs); - if (exitCode === 0) exitCode = runNodeScript('scripts/ci-prepush-lowend.mjs', childArgs); + exitCode = await runNodeScript('scripts/signing/verify-outgoing.mjs', childArgs); + if (exitCode === 0) exitCode = await runNodeScript('scripts/ci-prepush-lowend.mjs', childArgs); } catch (error) { console.error( `pre-push evidence capture failed closed: ${error instanceof Error ? error.message : 'invalid input'}`, diff --git a/scripts/hooks/shared.d.mts b/scripts/hooks/shared.d.mts new file mode 100644 index 000000000..68d6d7952 --- /dev/null +++ b/scripts/hooks/shared.d.mts @@ -0,0 +1,22 @@ +export interface BoundedResult { + status: number | null; + signal: string | null; + error: Error | null; + timedOut: boolean; + interrupted: boolean; + command: string; +} + +export function runBounded( + command: string, + args: string[], + options?: { + timeoutMs?: number; + env?: NodeJS.ProcessEnv; + input?: string; + shell?: boolean; + cwd?: string; + root?: string; + detached?: boolean; + }, +): Promise; diff --git a/scripts/hooks/shared.mjs b/scripts/hooks/shared.mjs index 5785f8e92..04b7f5d47 100644 --- a/scripts/hooks/shared.mjs +++ b/scripts/hooks/shared.mjs @@ -1,4 +1,4 @@ -import { spawnSync } from 'node:child_process'; +import { spawn, spawnSync } from 'node:child_process'; import { existsSync } from 'node:fs'; import { resolve } from 'node:path'; import process from 'node:process'; @@ -7,9 +7,9 @@ import { verifyDependencyState } from '../dependency-state.mjs'; const projectRoot = resolve(fileURLToPath(new URL('../..', import.meta.url))); -export function ensureDependencyState() { +export function ensureDependencyState(root = projectRoot) { try { - verifyDependencyState(projectRoot); + verifyDependencyState(root); return true; } catch (error) { console.error(`[hook] ${error instanceof Error ? error.message : String(error)}`); @@ -18,17 +18,179 @@ export function ensureDependencyState() { } } -export function runNodeScript(script, args = []) { - const result = spawnSync(process.execPath, [resolve(projectRoot, script), ...args], { - cwd: projectRoot, - stdio: 'inherit', +// QNBS-v3: bound hook children so timeout or resource termination is observable instead of an implicit pass. +export function runBounded( + command, + args, + { + timeoutMs = 120_000, + env, + input, + shell = false, + cwd = projectRoot, + detached = process.platform !== 'win32', + } = {}, +) { + return new Promise((resolveResult) => { + const child = spawn(command, args, { + cwd, + env: { ...process.env, ...env }, + shell, + detached: detached && process.platform !== 'win32', + stdio: input === undefined ? 'inherit' : ['pipe', 'inherit', 'inherit'], + }); + let timedOut = false; + let interrupted = false; + let terminationRequested = false; + let cleanupStarted = false; + let cleanupDeadline = 0; + let pendingFinish = null; + let state = 'RUNNING'; + let settled = false; + let forceTimer; + const terminate = (signal) => { + if (process.platform !== 'win32' && child.pid) { + try { + process.kill(-child.pid, signal); + return; + } catch { + // Fall back to the direct child when a process group is unavailable. + } + } else if (process.platform === 'win32' && child.pid) { + const result = spawnSync( + 'taskkill', + ['/pid', String(child.pid), '/t', ...(signal === 'SIGKILL' ? ['/f'] : [])], + { windowsHide: true, stdio: 'ignore' }, + ); + if (result.status === 0) return; + } + try { + child.kill(signal); + } catch { + // The child may have exited between process-group and direct cleanup attempts. + } + }; + const cleanupComplete = () => { + if (!child.pid || process.platform === 'win32') return true; + try { + process.kill(-child.pid, 0); + return false; + } catch (error) { + return error?.code === 'ESRCH'; + } + }; + const finishAfterCleanup = () => { + if (!cleanupComplete() && Date.now() < cleanupDeadline) { + setTimeout(finishAfterCleanup, 20); + return; + } + complete( + pendingFinish?.status ?? null, + pendingFinish?.signal ?? 'SIGKILL', + pendingFinish?.error ?? null, + ); + }; + const beginForceCleanup = () => { + if (cleanupStarted || state === 'SETTLED') return; + cleanupStarted = true; + state = 'FORCE_CLEANUP_RUNNING'; + if (forceTimer) { + clearTimeout(forceTimer); + forceTimer = undefined; + } + terminate('SIGKILL'); + cleanupDeadline = Date.now() + 1_000; + finishAfterCleanup(); + }; + const scheduleForceTermination = () => { + if (forceTimer) clearTimeout(forceTimer); + state = 'FORCE_CLEANUP_PENDING'; + forceTimer = setTimeout(() => { + forceTimer = undefined; + beginForceCleanup(); + }, 1_000); + }; + const requestTermination = (signal, reason, error = null) => { + if (reason === 'timeout') timedOut = true; + else if (reason === 'interrupt') interrupted = true; + if (error) pendingFinish = { status: null, signal: null, error }; + if (terminationRequested) { + // QNBS-v3: a repeated parent signal must force-clean detached children before the grace timer. + beginForceCleanup(); + return; + } + terminationRequested = true; + state = 'TERMINATION_REQUESTED'; + terminate(signal); + scheduleForceTermination(); + }; + const timeoutTimer = setTimeout(() => requestTermination('SIGTERM', 'timeout'), timeoutMs); + const signalHandlers = new Map(); + for (const signal of ['SIGINT', 'SIGTERM', 'SIGHUP']) { + const handler = () => { + requestTermination(signal, 'interrupt'); + }; + signalHandlers.set(signal, handler); + process.on(signal, handler); + } + const complete = (status, signal, error = null) => { + if (settled) return; + settled = true; + state = 'SETTLED'; + clearTimeout(timeoutTimer); + if (forceTimer) { + clearTimeout(forceTimer); + forceTimer = undefined; + } + for (const [parentSignal, handler] of signalHandlers) { + process.removeListener(parentSignal, handler); + } + resolveResult({ + status: error ? null : status, + signal, + error, + timedOut, + interrupted, + command, + }); + }; + const finish = (status, signal, error = null) => { + if (settled) return; + if (terminationRequested) { + // QNBS-v3: leader close never proves descendants are gone; force cleanup remains authoritative. + pendingFinish ??= { status, signal, error }; + state = 'CLOSED'; + beginForceCleanup(); + return; + } + complete(status, signal, error); + }; + child.once('error', (error) => finish(null, null, error)); + child.once('close', (status, signal) => finish(status, signal)); + if (input !== undefined) { + child.stdin.once('error', (error) => { + if (!['EPIPE', 'ERR_STREAM_DESTROYED'].includes(error.code)) + requestTermination('SIGTERM', 'resource', error); + }); + child.stdin.end(input); + } }); - return result.error ? 1 : (result.status ?? 1); } -export function runLocalBinary(binary, args = []) { +export async function runNodeScriptDetailed(script, args = [], options = {}) { + const root = options.root ?? projectRoot; + return runBounded(process.execPath, [resolve(root, script), ...args], { ...options, cwd: root }); +} + +export async function runNodeScript(script, args = [], options = {}) { + const result = await runNodeScriptDetailed(script, args, options); + return result.error || result.timedOut || result.interrupted ? 1 : (result.status ?? 1); +} + +export async function runLocalBinaryDetailed(binary, args = [], options = {}) { + const root = options.root ?? projectRoot; const command = resolve( - projectRoot, + root, 'node_modules', '.bin', `${binary}${process.platform === 'win32' ? '.cmd' : ''}`, @@ -37,12 +199,19 @@ export function runLocalBinary(binary, args = []) { console.error( `[hook] Required local binary is missing: ${binary}. Run: node scripts/dependency-state.mjs reconcile`, ); - return 1; + return { + status: 1, + signal: null, + error: new Error(`Missing local binary: ${binary}`), + timedOut: false, + interrupted: false, + command, + }; } - const result = spawnSync(command, args, { - cwd: projectRoot, - shell: process.platform === 'win32', - stdio: 'inherit', - }); - return result.error ? 1 : (result.status ?? 1); + return runBounded(command, args, { ...options, cwd: root, shell: process.platform === 'win32' }); +} + +export async function runLocalBinary(binary, args = [], options = {}) { + const result = await runLocalBinaryDetailed(binary, args, options); + return result.error || result.timedOut || result.interrupted ? 1 : (result.status ?? 1); } diff --git a/tests/unit/hooks/shared.test.ts b/tests/unit/hooks/shared.test.ts new file mode 100644 index 000000000..17da3c1f9 --- /dev/null +++ b/tests/unit/hooks/shared.test.ts @@ -0,0 +1,50 @@ +// @vitest-environment node +import process from 'node:process'; +import { describe, expect, it } from 'vitest'; +import { runBounded } from '../../../scripts/hooks/shared.mjs'; + +describe('bounded hook subprocesses', () => { + it('does not treat a clean timeout shutdown as a successful run', async () => { + const startedAt = performance.now(); + const result = await runBounded( + process.execPath, + ['-e', "process.on('SIGTERM', () => process.exit(0)); setInterval(() => {}, 10_000);"], + { timeoutMs: 100 }, + ); + + expect(result.timedOut).toBe(true); + expect(performance.now() - startedAt).toBeLessThan(900); + }); + + // QNBS-v3: keep nested admission checks in the parent's process group for outer cleanup. + it('supports foreground children for nested admission checks', async () => { + const result = await runBounded(process.execPath, ['-e', 'setInterval(() => {}, 10_000);'], { + timeoutMs: 100, + detached: false, + }); + + expect(result.timedOut).toBe(true); + expect(result.status === 0).toBe(false); + }); + + // QNBS-v3: prove repeated parent signals clean detached children without accepting cancellation as pass. + it('preserves parent cancellation and force-cleans after repeated signals', async () => { + const resultPromise = runBounded( + process.execPath, + ['-e', "process.on('SIGINT', () => {}); setInterval(() => {}, 10_000);"], + { timeoutMs: 5_000 }, + ); + const firstSignal = setTimeout(() => process.emit('SIGINT'), 50); + const repeatedSignal = setTimeout(() => process.emit('SIGINT'), 100); + + try { + const result = await resultPromise; + expect(result.interrupted).toBe(true); + expect(result.timedOut).toBe(false); + expect(result.status === 0).toBe(false); + } finally { + clearTimeout(firstSignal); + clearTimeout(repeatedSignal); + } + }); +}); From ae7bf67e9c402fc8d28dbef906cf4b8b69bf2d72 Mon Sep 17 00:00:00 2001 From: qnbs <155236708+qnbs@users.noreply.github.com> Date: Tue, 25 Aug 2026 15:45:04 +0200 Subject: [PATCH 2/3] fix(hooks): close review-loop findings on bounded subprocess runner Consolidated E1 remediation for PR #500's first review epoch, covering every current-material finding from Sourcery, CodeRabbit, and CodeAnt (Amazon Q's 5 findings were evaluated and classified as false positives with evidence -- see PR thread replies): - runNodeScriptDetailed/runLocalBinaryDetailed spread options before forcing cwd to root, silently discarding any caller-provided cwd. shared.d.mts now advertises cwd as a valid option on these wrappers, which would have made the mismatch worse for TypeScript callers. Preserve options.cwd when given; root remains only the default and still resolves the script/binary file path itself. (CodeAnt) - shared.d.mts declared only BoundedResult/runBounded; the other five runtime exports (ensureDependencyState, runNodeScript(Detailed), runLocalBinary(Detailed)) had no type declarations at all. Declared all of them against a shared RunOptions type. (Sourcery) - cleanupComplete() unconditionally returned true on Windows without verifying taskkill actually terminated the child tree, so runBounded could resolve while descendants were still alive. Poll tasklist for the leader PID instead of assuming taskkill's exit implies cleanup. (Sourcery) - shared.test.ts asserted the whole spawn+timeout+cleanup cycle finishes under a fixed 900ms wall-clock bound -- flaky on slow or cold CI/Windows runners. Removed it; result.timedOut already proves the behavior under test. CodeRabbit's suggested replacement (asserting status !== 0) was verified incorrect for this specific scenario -- the child intentionally exits 0 on SIGTERM to prove timedOut is tracked independently of a clean exit status -- so a correct replacement was written instead. (CodeRabbit) - cleanupComplete()'s ESRCH-only check treated an EPERM kill(pid, 0) result (group still exists, signal merely not permitted) the same as ESRCH (group is gone), which would under-report incomplete cleanup. Low-reachability for self-spawned children but a harmless, more-correct guard. Amazon Q's 5 findings on the same commit were investigated and rejected as false positives, each with concrete evidence: (1) the "race condition" on child.pid predates an already-present guard and Node's synchronous error-handler registration before any possible async error emission; (2) the "infinite loop" claim ignores that cleanupDeadline is a fixed value, bounding the poll to ~1s under any normal clock; (3) the "resource leak" claim has handler removal already completing before resolveResult is called in the actual code order, and Promise resolution cannot synchronously throw into the calling frame; (4) partially addressed by the EPERM guard above, though the underlying "polls indefinitely" premise is false per (2); (5) empirically disproven by spawning a real destroyed-stream write in Node -- ERR_STREAM_DESTROYED is the actual `.code`, not `.name`, confirming the existing check was already correct. Validated: full `pnpm run ci:prepush`, `pnpm run lint`, `pnpm run typecheck` (4-checker), and tests/unit/hooks/shared.test.ts all pass. --- scripts/hooks/shared.d.mts | 46 ++++++++++++++++++++++++++------- scripts/hooks/shared.mjs | 23 ++++++++++++++--- tests/unit/hooks/shared.test.ts | 3 +-- 3 files changed, 57 insertions(+), 15 deletions(-) diff --git a/scripts/hooks/shared.d.mts b/scripts/hooks/shared.d.mts index 68d6d7952..5678dfc53 100644 --- a/scripts/hooks/shared.d.mts +++ b/scripts/hooks/shared.d.mts @@ -7,16 +7,44 @@ export interface BoundedResult { command: string; } +export interface RunOptions { + timeoutMs?: number; + env?: NodeJS.ProcessEnv; + input?: string; + shell?: boolean; + cwd?: string; + root?: string; + detached?: boolean; +} + +export function ensureDependencyState(root?: string): boolean; + export function runBounded( command: string, args: string[], - options?: { - timeoutMs?: number; - env?: NodeJS.ProcessEnv; - input?: string; - shell?: boolean; - cwd?: string; - root?: string; - detached?: boolean; - }, + options?: RunOptions, +): Promise; + +export function runNodeScriptDetailed( + script: string, + args?: string[], + options?: RunOptions, ): Promise; + +export function runNodeScript( + script: string, + args?: string[], + options?: RunOptions, +): Promise; + +export function runLocalBinaryDetailed( + binary: string, + args?: string[], + options?: RunOptions, +): Promise; + +export function runLocalBinary( + binary: string, + args?: string[], + options?: RunOptions, +): Promise; diff --git a/scripts/hooks/shared.mjs b/scripts/hooks/shared.mjs index 04b7f5d47..fb3908ad6 100644 --- a/scripts/hooks/shared.mjs +++ b/scripts/hooks/shared.mjs @@ -71,12 +71,20 @@ export function runBounded( } }; const cleanupComplete = () => { - if (!child.pid || process.platform === 'win32') return true; + if (!child.pid) return true; + if (process.platform === 'win32') { + // QNBS-v3: taskkill /f returning does not prove the tree exited; poll tasklist instead. + const result = spawnSync('tasklist', ['/fi', `PID eq ${child.pid}`, '/fo', 'csv', '/nh'], { + windowsHide: true, + }); + return !(result.stdout ?? '').toString().includes(String(child.pid)); + } try { process.kill(-child.pid, 0); return false; } catch (error) { - return error?.code === 'ESRCH'; + // QNBS-v3: EPERM means the group still exists; only ESRCH proves it is gone. + return error?.code === 'ESRCH' || error?.code === 'EPERM'; } }; const finishAfterCleanup = () => { @@ -179,7 +187,10 @@ export function runBounded( export async function runNodeScriptDetailed(script, args = [], options = {}) { const root = options.root ?? projectRoot; - return runBounded(process.execPath, [resolve(root, script), ...args], { ...options, cwd: root }); + return runBounded(process.execPath, [resolve(root, script), ...args], { + ...options, + cwd: options.cwd ?? root, + }); } export async function runNodeScript(script, args = [], options = {}) { @@ -208,7 +219,11 @@ export async function runLocalBinaryDetailed(binary, args = [], options = {}) { command, }; } - return runBounded(command, args, { ...options, cwd: root, shell: process.platform === 'win32' }); + return runBounded(command, args, { + ...options, + cwd: options.cwd ?? root, + shell: process.platform === 'win32', + }); } export async function runLocalBinary(binary, args = [], options = {}) { diff --git a/tests/unit/hooks/shared.test.ts b/tests/unit/hooks/shared.test.ts index 17da3c1f9..720083663 100644 --- a/tests/unit/hooks/shared.test.ts +++ b/tests/unit/hooks/shared.test.ts @@ -5,15 +5,14 @@ import { runBounded } from '../../../scripts/hooks/shared.mjs'; describe('bounded hook subprocesses', () => { it('does not treat a clean timeout shutdown as a successful run', async () => { - const startedAt = performance.now(); const result = await runBounded( process.execPath, ['-e', "process.on('SIGTERM', () => process.exit(0)); setInterval(() => {}, 10_000);"], { timeoutMs: 100 }, ); + // QNBS-v3: status is legitimately 0 here (clean exit(0) on SIGTERM); timedOut is the real proof. expect(result.timedOut).toBe(true); - expect(performance.now() - startedAt).toBeLessThan(900); }); // QNBS-v3: keep nested admission checks in the parent's process group for outer cleanup. From 17bc9c952fc9efc1c83f30897afaedaac36e8720 Mon Sep 17 00:00:00 2001 From: qnbs <155236708+qnbs@users.noreply.github.com> Date: Tue, 25 Aug 2026 15:59:32 +0200 Subject: [PATCH 3/3] fix(hooks): correct EPERM polarity, separate runBounded/wrapper option types, fail closed on live stdin errors Second review-loop epoch for PR #500, combining a genuinely new, independently-identified logic error with a real (if narrower) issue this PR's own prior fix round had left unaddressed: - cleanupComplete()'s EPERM handling from the previous commit had the polarity backwards: the QNBS-v3 comment correctly stated "EPERM means the group still exists," but the code returned true (cleanup COMPLETE) for that exact case, contradicting its own comment and potentially reporting a still-alive process group as cleaned up. The original pre-fix code (implicit false via `=== 'ESRCH'`) already handled EPERM correctly; reverted to that, since only ESRCH is positive proof the group is gone and the bounded ~1s deadline already covers every other outcome safely. (Graphite) - shared.d.mts's single RunOptions type included `root` on runBounded itself, but runBounded's runtime destructuring never consumes that key -- split into BoundedOptions (runBounded's real surface) and RunOptions extends BoundedOptions with root (the wrapper functions' surface), matching what each function actually reads. - The stdin 'error' handler treated EPIPE/ERR_STREAM_DESTROYED as always benign, even while the child was still running. That masks a genuine partial-input-delivery failure as success whenever the child's own exit code doesn't happen to reflect it -- a fail-open gap in exactly the evidence-delivery path this file exists to make observable. Track child exit via a dedicated 'exit' listener; only treat those error codes as benign once the child has already exited, otherwise still request termination as an unexpected resource error. Added a regression test for the wrapper cwd-vs-root separation (runNodeScriptDetailed with distinct root/cwd, verified via a marker file written to the actual working directory). A dedicated regression test for the "child still alive, stdin errors" branch was evaluated and dropped: empirical probing showed process.stdin.destroy() in a child kept alive by a timer does not reliably surface an EPIPE to the parent within any practical test window (a Node-internal timing behavior, confirmed by two contrasting probes -- ~150ms without a keep-alive timer in the child, no error at all within 3s with one) -- forcing a test to wait out a multi-second timeout to observe it would reintroduce the exact wall-clock flakiness already removed from this file's tests in the prior commit. The fix's own logic remains correct independent of how promptly Node happens to surface the underlying error. README test-count badges resynced via `pnpm run sync:readme` for the added test (578 files / 6988+ tests). Validated: full `pnpm run ci:prepush`, `pnpm run lint`, `pnpm run typecheck` (4-checker), and tests/unit/hooks/shared.test.ts all pass. --- README.md | 8 ++++---- scripts/hooks/shared.d.mts | 10 +++++++--- scripts/hooks/shared.mjs | 13 +++++++++---- tests/unit/hooks/shared.test.ts | 33 +++++++++++++++++++++++++++++++-- 4 files changed, 51 insertions(+), 13 deletions(-) diff --git a/README.md b/README.md index 99dea6c22..74ced8387 100644 --- a/README.md +++ b/README.md @@ -14,7 +14,7 @@ IndexedDB v8 PWA v3.0 i18n 19 locales — 2925 keys - 6987+ tests / 578 files + 6988+ tests / 578 files Codecov Coverage License MIT CI Status @@ -512,7 +512,7 @@ The Settings → AI panel shows a live GPU status badge with adapter details and | **Document Export** | docx + jszip | Word-compatible `.docx` generation (lazy-loaded) | | **PWA** | Service Worker + Web App Manifest v3 | Offline support, installability, Workbox chunking | | **i18n** | Custom React Context (`I18nContext.tsx`) | 2925 keys × 19 locales (de/en/es/fr/it + ar/he/fa RTL Beta + ja/zh/pt/el/fi/sv/hu/is/eu/ru/ko Beta); EN fallback; `localStorage` persistence | -| **Testing** | Vitest 4.x (6987+ tests / 578 files) + Playwright E2E | Unit/integration + cross-browser E2E; Stryker mutation (manual workflow) | +| **Testing** | Vitest 4.x (6988+ tests / 578 files) + Playwright E2E | Unit/integration + cross-browser E2E; Stryker mutation (manual workflow) | | **Code Quality** | Biome (lint + format) + TypeScript 7 (tsgo) strict | `--error-on-warnings` in CI; zero `any` policy | | **Visualization** | Force-directed graph | Interactive character relationship network | | **Desktop** | Tauri v2 | Cross-platform installer; auto-updater via `latest.json` | @@ -550,7 +550,7 @@ WorldScript-Studio/ │ ├── sw.js # PWA Service Worker │ └── manifest.json # PWA Web App Manifest v3 ├── tests/ -│ ├── unit/ # Vitest unit tests (6987+ tests, 578 files) — count spans tests/, components/, packages/*/tests/, not just this folder +│ ├── unit/ # Vitest unit tests (6988+ tests, 578 files) — count spans tests/, components/, packages/*/tests/, not just this folder │ │ ├── ai/ # aiSmallModules, aiCoreFallbackPaths │ │ └── settings/ # WebLlmPanel, AiSections │ └── e2e/ # Playwright specs + helpers.ts @@ -712,7 +712,7 @@ The main pipeline is [`.github/workflows/ci.yml`](.github/workflows/ci.yml). Opt | `scorecard` | weekly + `main` push | OpenSSF Scorecard — SARIF uploaded to GitHub Code Scanning | **Current test metrics (2026-08-21, source-synchronized; CI remains authoritative for pass/fail):** -- **6987+ unit tests** across **578 test files** — CI is authoritative for pass/fail +- **6988+ unit tests** across **578 test files** — CI is authoritative for pass/fail - Coverage thresholds: lines ≥ 80 · branches ≥ 66 · functions ≥ 72 · statements ≥ 78 — enforced in CI (see Codecov badge for live metrics) - i18n: **2925 keys × 19 locales** (en/de/fr/es/it + ar/he/fa RTL Beta + ja/zh/pt/el/fi/sv/hu/is/eu/ru/ko Beta) diff --git a/scripts/hooks/shared.d.mts b/scripts/hooks/shared.d.mts index 5678dfc53..8cb6c7161 100644 --- a/scripts/hooks/shared.d.mts +++ b/scripts/hooks/shared.d.mts @@ -7,22 +7,26 @@ export interface BoundedResult { command: string; } -export interface RunOptions { +export interface BoundedOptions { timeoutMs?: number; env?: NodeJS.ProcessEnv; input?: string; shell?: boolean; cwd?: string; - root?: string; detached?: boolean; } +// QNBS-v3: root is a wrapper-only option; runBounded's runtime destructuring never consumes it. +export interface RunOptions extends BoundedOptions { + root?: string; +} + export function ensureDependencyState(root?: string): boolean; export function runBounded( command: string, args: string[], - options?: RunOptions, + options?: BoundedOptions, ): Promise; export function runNodeScriptDetailed( diff --git a/scripts/hooks/shared.mjs b/scripts/hooks/shared.mjs index fb3908ad6..b6f6acbbc 100644 --- a/scripts/hooks/shared.mjs +++ b/scripts/hooks/shared.mjs @@ -48,6 +48,10 @@ export function runBounded( let state = 'RUNNING'; let settled = false; let forceTimer; + let childExited = false; + child.once('exit', () => { + childExited = true; + }); const terminate = (signal) => { if (process.platform !== 'win32' && child.pid) { try { @@ -83,8 +87,8 @@ export function runBounded( process.kill(-child.pid, 0); return false; } catch (error) { - // QNBS-v3: EPERM means the group still exists; only ESRCH proves it is gone. - return error?.code === 'ESRCH' || error?.code === 'EPERM'; + // QNBS-v3: only ESRCH proves the group is gone; EPERM/unknown errors must not short-circuit polling. + return error?.code === 'ESRCH'; } }; const finishAfterCleanup = () => { @@ -177,8 +181,9 @@ export function runBounded( child.once('close', (status, signal) => finish(status, signal)); if (input !== undefined) { child.stdin.once('error', (error) => { - if (!['EPIPE', 'ERR_STREAM_DESTROYED'].includes(error.code)) - requestTermination('SIGTERM', 'resource', error); + // QNBS-v3: a broken pipe only proves the child was already done reading, not that delivery mid-run is safe to ignore. + const benign = childExited && ['EPIPE', 'ERR_STREAM_DESTROYED'].includes(error.code); + if (!benign) requestTermination('SIGTERM', 'resource', error); }); child.stdin.end(input); } diff --git a/tests/unit/hooks/shared.test.ts b/tests/unit/hooks/shared.test.ts index 720083663..db7f4c125 100644 --- a/tests/unit/hooks/shared.test.ts +++ b/tests/unit/hooks/shared.test.ts @@ -1,7 +1,10 @@ // @vitest-environment node +import { mkdtemp, readFile, rm, writeFile } from 'node:fs/promises'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; import process from 'node:process'; -import { describe, expect, it } from 'vitest'; -import { runBounded } from '../../../scripts/hooks/shared.mjs'; +import { afterEach, describe, expect, it } from 'vitest'; +import { runBounded, runNodeScriptDetailed } from '../../../scripts/hooks/shared.mjs'; describe('bounded hook subprocesses', () => { it('does not treat a clean timeout shutdown as a successful run', async () => { @@ -46,4 +49,30 @@ describe('bounded hook subprocesses', () => { clearTimeout(repeatedSignal); } }); + + describe('runNodeScriptDetailed cwd handling', () => { + const scratchDirs: string[] = []; + afterEach(async () => { + await Promise.all(scratchDirs.splice(0).map((dir) => rm(dir, { recursive: true, force: true }))); + }); + + it('preserves an explicit cwd separate from root', async () => { + const scriptRoot = await mkdtemp(join(tmpdir(), 'worldscript-hook-root-')); + const workingDir = await mkdtemp(join(tmpdir(), 'worldscript-hook-cwd-')); + scratchDirs.push(scriptRoot, workingDir); + await writeFile( + join(scriptRoot, 'write-marker.mjs'), + "import { writeFileSync } from 'node:fs'; writeFileSync('marker.txt', 'ok');", + 'utf8', + ); + + const result = await runNodeScriptDetailed('write-marker.mjs', [], { + root: scriptRoot, + cwd: workingDir, + }); + + expect(result.status).toBe(0); + await expect(readFile(join(workingDir, 'marker.txt'), 'utf8')).resolves.toBe('ok'); + }); + }); });