From 7ccf31a59b5227d66a95b2d52932d68b59e2b702 Mon Sep 17 00:00:00 2001 From: pku-xht Date: Thu, 9 Jul 2026 10:20:42 +0800 Subject: [PATCH] fix review finding: root-portable rm-failure injection in the config-dir test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The best-effort-remove test provoked a real EACCES via a chmod-000 subtree, which only fails for unprivileged users — under root, recursive rm ignores permission bits, deleting the subtree: the existsSync assertion goes red and the swallow branch loses coverage, failing the per-file gate. The rejection is now injected deterministically at the node:fs/promises boundary (rm wrapped with a real-passthrough vi.fn; one test queues a single rejection), the fs-failure boundary being exactly the non-deterministic seam the testing policy sanctions mocking. Everything else in the suite stays on the real filesystem, and the swallow contract stays error-kind agnostic. --- packages/subagent/subagent-process/README.md | 2 +- .../tests/subagent-process.spec.ts | 39 +++++++++++-------- 2 files changed, 24 insertions(+), 17 deletions(-) diff --git a/packages/subagent/subagent-process/README.md b/packages/subagent/subagent-process/README.md index 597470f52a..e42bb4b611 100644 --- a/packages/subagent/subagent-process/README.md +++ b/packages/subagent/subagent-process/README.md @@ -37,4 +37,4 @@ A per-run isolated config directory for an external CLI child (the target of `CL ## Testing -`tests/subagent-process.spec.ts`: the env scrub and config-dir helpers run against the real process env and real filesystem (including an rm-failure path proving `remove()` never rejects); the exit waits and the dispose ladder run against a scriptable fake child, driving each escalation tier deterministically. The [ACP backend suite](../subagent-acp/README.md) exercises the same ladder against real subprocesses (EOF-cooperative, EOF-ignoring, and SIGTERM-trapping children) end to end. +`tests/subagent-process.spec.ts`: the env scrub and config-dir helpers run against the real process env and real filesystem (the rm-failure path injects its rejection at the fs boundary — a real recursive-rm failure is not portably provokable, and root ignores permission bits); the exit waits and the dispose ladder run against a scriptable fake child, driving each escalation tier deterministically. The [ACP backend suite](../subagent-acp/README.md) exercises the same ladder against real subprocesses (EOF-cooperative, EOF-ignoring, and SIGTERM-trapping children) end to end. diff --git a/packages/subagent/subagent-process/tests/subagent-process.spec.ts b/packages/subagent/subagent-process/tests/subagent-process.spec.ts index c4828d8498..24f2a3a97d 100644 --- a/packages/subagent/subagent-process/tests/subagent-process.spec.ts +++ b/packages/subagent/subagent-process/tests/subagent-process.spec.ts @@ -1,7 +1,7 @@ -import { describe, expect, it } from 'vitest' +import { describe, expect, it, vi } from 'vitest' import { EventEmitter } from 'node:events' import { existsSync } from 'node:fs' -import { chmod, mkdir, mkdtemp, rm, stat, writeFile } from 'node:fs/promises' +import { mkdtemp, rm, stat, writeFile } from 'node:fs/promises' import { tmpdir } from 'node:os' import { join } from 'node:path' import type { ChildProcess } from 'node:child_process' @@ -15,13 +15,24 @@ import { waitForExit, } from '../src/index.ts' +// `rm` is wrapped (real-passthrough by default) so ONE test can inject a +// rejection deterministically. A real recursive-rm failure is not portably +// provokable — permission tricks (a chmod-000 subtree) fail only for +// unprivileged users and are ignored by root — so this is the fs boundary +// the testing policy sanctions mocking; everything else stays the real fs. +vi.mock('node:fs/promises', async (importOriginal) => { + const actual = await importOriginal() + return { ...actual, rm: vi.fn(actual.rm) } +}) + /** * Unit tests for the shared out-of-process machinery. The env scrub and the * isolated-config-dir helpers run against the REAL process env and REAL - * filesystem; the exit waits and the dispose ladder run against a scriptable - * fake child so each escalation tier's timing is driven deterministically - * (the ACP backend's suite exercises the same ladder against real - * subprocesses end to end). + * filesystem (one exception: the rm-failure path injects its rejection at the + * mocked fs boundary, see above); the exit waits and the dispose ladder run + * against a scriptable fake child so each escalation tier's timing is driven + * deterministically (the ACP backend's suite exercises the same ladder + * against real subprocesses end to end). */ /** What fells a scripted {@link FakeChild}. */ @@ -287,20 +298,16 @@ describe('createIsolatedConfigDir', () => { expect(existsSync(missing)).toBe(false) }) - it('remove() is best-effort: an rm failure resolves instead of rejecting', async () => { + it('remove() is best-effort: an rm rejection resolves instead of rejecting', async () => { const dir = await createIsolatedConfigDir('dsh-subagent-process-locked-') - const locked = join(dir.path, 'locked') - await mkdir(locked) - await writeFile(join(locked, 'entry'), 'x') - // An unreadable, unwritable non-empty subdir makes recursive rm fail - // (EACCES on readdir/unlink) for a non-root user. - await chmod(locked, 0o000) try { + // The swallow contract is error-kind agnostic; EACCES stands in for the + // family (EBUSY, a vanished mount, …) that best-effort must absorb. + vi.mocked(rm).mockRejectedValueOnce(Object.assign(new Error('EACCES: permission denied'), { code: 'EACCES' })) await expect(dir.remove()).resolves.toBeUndefined() - // rm really did fail — the locked subtree is still there. - expect(existsSync(locked)).toBe(true) + // The injected rejection consumed the only rm call — nothing was deleted. + expect(existsSync(dir.path)).toBe(true) } finally { - await chmod(locked, 0o700) await rm(dir.path, { recursive: true, force: true }) } })