From 3e1dca379c5b7e6338739a5db713d5e20db58b8b Mon Sep 17 00:00:00 2001 From: Jeff Repanich Date: Sat, 10 Oct 2026 00:51:27 -0400 Subject: [PATCH 1/2] fix: classify lock reads and dispose rejected OpenAPI bodies --- CHANGELOG.md | 7 ++ docs/0.5.0-api.md | 2 + docs/0.5.0-hardening.md | 21 ++++ src/filesystem-lock.ts | 46 +++---- src/generate/generator.ts | 20 +-- tests/filesystem-locks.test.ts | 39 ++++++ tests/fixtures/create-process-worker.ts | 3 + tests/generate-remote.test.ts | 112 ++++++++++++++++- tests/generation-publication.test.ts | 156 ++++++++++++++++++++++++ tests/update-cli.test.ts | 59 +++++++++ 10 files changed, 428 insertions(+), 37 deletions(-) create mode 100644 docs/0.5.0-hardening.md create mode 100644 tests/fixtures/create-process-worker.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index 307a537..3d4805e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,6 +16,13 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- Dispose rejected or redirected OpenAPI response bodies, including header + validation and redirect-limit failures. Stop streaming unused bodies after + reporting an error. Keep normal complete responses and load limits unchanged. +- Keep lock preflight errors separate from rename contention so a denied + observation cannot silently admit a transaction. Preserve the existing + bounded retry for Windows lock observations and propagate other failures. + - Retry a denied lock-directory scan during Windows publication handoff within the existing wait bound. Also retry denied owner-record reads and lock observations while a prior owner removes them. Retain the native error when diff --git a/docs/0.5.0-api.md b/docs/0.5.0-api.md index febc6e1..1fe3b20 100644 --- a/docs/0.5.0-api.md +++ b/docs/0.5.0-api.md @@ -56,6 +56,8 @@ The executed matrix covers two simultaneous stale-owner observations for file an On Windows, inspecting a lock directory or opening its owner record while its previous owner removes it can return `EPERM`. The acquisition loop retries read-only lock observations (`lstat`, `scandir`, `open`, `read`) within its existing ten-second bound and retains the native cause if denial persists; write/deletion failures and other filesystem errors still propagate. A repeated four-writer generator probe reproduced native scan and owner-open failures on separate source heads. Deterministic file/directory cases verify transient recovery, permanent-denial timeout, exact bytes and owner preservation; separate deletion-denial cases verify those errors propagate without entering a transaction. +Lock preflight and rename now have separate error classification. A one-shot `EACCES` during preflight propagates without entering a transaction, preserving its exact owner record and file bytes. OpenAPI loading destroys unused response bodies before rejecting headers/status or following a redirect. The executed matrix and tested process boundaries are recorded in [0.5.0 hardening](0.5.0-hardening.md). + ## Release boundaries The normal packed consumer checks both retained declaration names, nine rejected old imports and private paths under strict TypeScript 6 and 7, then runs the installed CLI help/version commands. No production dependency, package version or peer range changes in this cut. Full candidate graph, generated website API and maintainer review still gate release. CLI hardening issue #159 stays open until its remaining qualification is complete. diff --git a/docs/0.5.0-hardening.md b/docs/0.5.0-hardening.md new file mode 100644 index 0000000..681d243 --- /dev/null +++ b/docs/0.5.0-hardening.md @@ -0,0 +1,21 @@ +# 0.5.0 CLI hardening + +Issue [#159](https://github.com/askrjs/askr-cli/issues/159) covers CLI failure boundaries. These are executed assertions and reproduced failures. Package versions remain unchanged during preparation; the final candidate graph and maintainer review are separate release gates. + +| Invariant | Executed evidence | Outcome and boundary | +| ------------------------------------------------------------------------- | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| Failed file updates preserve original data and report recoverable copies. | [File transaction failures](../tests/file-transaction-failures.test.ts), [file changes](../tests/file-changes.test.ts), [update CLI](../tests/update-cli.test.ts); 17 added cases in [#176](https://github.com/askrjs/askr-cli/pull/176), 13 failing pre-fix assertions. | Partial writes and failed replacement/rollback/cleanup are covered. POSIX file modes are preserved; five mode assertions are skipped on Windows. Actual process termination retains original copies for manual restoration. Automatic file restoration after abrupt termination is not claimed. | +| Directory publication recovers before the next operation. | [Directory swaps](../tests/directory-swap.test.ts) and [termination worker](../tests/fixtures/directory-publication-crash-worker.ts); 24 added cases in [#177](https://github.com/askrjs/askr-cli/pull/177), six failing pre-fix assertions. | Actual termination at prepared, backed-up and published checkpoints; failed rollback and partial cleanup; corrupt records, conflicting trees, missing trees and junctions. Recovery preserves originals or complete new output. Ambiguous state fails with retained recovery paths. | +| Generation and create preserve destination ownership. | [Generation publication](../tests/generation-publication.test.ts) and its [fault worker](../tests/fixtures/generation-publication-worker.ts); 17 added cases in [#178](https://github.com/askrjs/askr-cli/pull/178), twelve failing pre-fix assertions. | Generator shares directory recovery. Partial stages are cleaned, a cleanup failure names its retained stage, and partial backup deletion does not roll back complete output. Concurrent creates have one winner and preserve its unrelated files. Success follows publication. | +| Contending commands do not delete another lock owner. | [Lock ownership](../tests/filesystem-locks.test.ts), [lock termination worker](../tests/fixtures/filesystem-lock-crash-worker.ts), repeated [generation](../tests/generate.test.ts); [#179](https://github.com/askrjs/askr-cli/pull/179), [#180](https://github.com/askrjs/askr-cli/pull/180), [#181](https://github.com/askrjs/askr-cli/pull/181). | Unique tokens, delayed reapers, release handoff, stale writes, legacy recovery, ambiguous contents, junctions, preparation collision and cleanup failures are covered. Two processes are actually killed before lock publication. A repeated four-writer case checks 200 publications and complete final output. Earlier Windows scan/open failures remain in CI history; corrected PR and merge checks passed on all three platforms. | +| Read failures are classified at the operation that failed. | [Lock preflight and observation assertions](../tests/filesystem-locks.test.ts). | Native `EPERM` observations retry within the existing ten-second bound, preserving the cause if permanent. A one-shot preflight `EACCES` propagates without entering either a file or directory transaction. Rename contention and deletion denials retain their separate handling. | +| Rejected remote downloads stop consuming their bodies. | Extended [remote transport tests](../tests/generate-remote.test.ts); separate real local HTTPS probe under Node 24. | Encoding, declared size, redirect limit, missing Location, HTTP error and cross-origin redirect failures destroy unused bodies. A successful redirect preserves its complete final response. Chunk limits, body deadline and abortion also destroy the response. Before the fix, five real HTTPS cases retained one or two sockets and continued streaming after rejection; after the fix, all five closed with no subsequent body writes. This real HTTPS probe ran locally; hosted assertions use controlled transport. | +| Malformed input fails before mutation or registry access. | Nineteen parameterized [update CLI cases](../tests/update-cli.test.ts) and separate real command probes. | Truncated/null/array/scalar manifests; malformed workspaces and update policy; invalid pnpm YAML and package declarations. Every file name and byte remains unchanged, and registry calls stay at zero. Real malformed SSG module and missing-registry probes preserve existing published output. | +| Actual installer process outcomes preserve publication boundaries. | Five cases in [generation publication](../tests/generation-publication.test.ts), using the [create worker](../tests/fixtures/create-process-worker.ts) and local installer shim. | Missing command, nonzero exit, externally terminated installer, successful installer and abruptly terminated CLI are exercised through real `spawnSync`. Failure exits preserve unrelated data, report no success, publish no destination and remove their private project stage. Success publishes the complete prepared project. Killing the CLI retains an unpublished private stage; the test explicitly stops its owned installer and removes its own fixture. | +| Path pivots and repeated commands preserve unrelated files. | [Generate](../tests/generate.test.ts), [remote transport](../tests/generate-remote.test.ts), [discovery](../tests/update-discovery.test.ts), [CLI smoke](../tests/cli-smoke.test.ts) and the failure suites above. | Unsafe project names, file/junction destinations, output containing input, local reference traversal, remote-to-file pivots, allowed origins, source limits, workspace boundaries, dry-run immutability, stale plans and concurrent skill copies are exercised. Existing assertions remain in the full suite. | + +The final read-failure pass adds eleven cases and strengthens an existing remote assertion. Ten assertions fail against immutable pre-fix source: two preflight denials and eight response-disposal cases. Nineteen malformed-input cases and five actual installer cases extend the existing suites without adding command features. + +Validation uses Node LTS: `npm run check`, `npm run bench`, `npm run test:templates`, `npm run test:peer-floor`, `npm run fmt -- --check` and production dependency audit. The normal packed consumer checks two retained declaration names, nine removed imports and fifteen private paths under actual TypeScript 6.0.3 and 7.0.2, then invokes installed help/version. Packed template integration performs normal installs for all five starters. The unchanged performance suite has 22 gates. + +Parent-process coverage does not measure the lines executed by termination workers. The fault assertions and actual process checkpoints provide separate evidence. Tests of current cooperating lock owners do not qualify concurrent use of older recursive-lock protocols. Process termination does not establish power-loss durability. The installer has no automatic timeout contract: externally stopping a real installer and injecting an `ETIMEDOUT` result are covered, but an automatic installer timeout is not claimed. diff --git a/src/filesystem-lock.ts b/src/filesystem-lock.ts index 1083e4e..4f84197 100644 --- a/src/filesystem-lock.ts +++ b/src/filesystem-lock.ts @@ -118,32 +118,32 @@ export async function acquireFilesystemLock( if (lockStat && (!lockStat.isDirectory() || lockStat.isSymbolicLink())) { throw ambiguousLock(lock); } - await fs.rename(stage, lock); - return async () => { - await fs.unlink(path.join(lock, ownerName)); - // Another contender can already have replaced the empty old lock with - // its nonempty lock. Never recursively remove a successor's contents. - await removeEmptyLock(lock); - }; - } catch (error) { - const code = error instanceof Error && "code" in error ? String(error.code) : ""; - if (!RENAME_CONTENTION_CODES.has(code)) throw error; - lastError = error; try { - if (await removeOrphanedLock(lock)) continue; - } catch (inspectionError) { - // Windows can deny reads while another owner removes its lock record. - // Retry within the same deadline; a permanent denial retains its cause. - if ( - !isNodeError(inspectionError, "EPERM") || - !LOCK_READ_SYSCALLS.has(inspectionError.syscall ?? "") - ) { - throw inspectionError; - } - lastError = inspectionError; + await fs.rename(stage, lock); + return async () => { + await fs.unlink(path.join(lock, ownerName)); + // Another contender can already have replaced the empty old lock with + // its nonempty lock. Never recursively remove a successor's contents. + await removeEmptyLock(lock); + }; + } catch (error) { + const code = error instanceof Error && "code" in error ? String(error.code) : ""; + if (!RENAME_CONTENTION_CODES.has(code)) throw error; + lastError = error; } - await new Promise((resolve) => setTimeout(resolve, LOCK_RETRY_MS)); + if (await removeOrphanedLock(lock)) continue; + } catch (inspectionError) { + // Windows can deny reads while another owner removes its lock record. + // Keep read errors distinct from rename contention and deletion failures. + if ( + !isNodeError(inspectionError, "EPERM") || + !LOCK_READ_SYSCALLS.has(inspectionError.syscall ?? "") + ) { + throw inspectionError; + } + lastError = inspectionError; } + await new Promise((resolve) => setTimeout(resolve, LOCK_RETRY_MS)); } } catch (error) { try { diff --git a/src/generate/generator.ts b/src/generate/generator.ts index 5968c53..753dd91 100644 --- a/src/generate/generator.ts +++ b/src/generate/generator.ts @@ -357,17 +357,17 @@ async function responseText( maxBytes: number, deadline: number, ): Promise { - const declared = Number(response.headers["content-length"]); - if (Number.isFinite(declared) && declared > maxBytes) - throw new GenerationError(`OpenAPI reference exceeds ${maxBytes} bytes: ${uri}`); - const encoding = response.headers["content-encoding"]; - if (encoding && encoding !== "identity") - throw new GenerationError( - `OpenAPI reference returned unsupported content encoding: ${encoding}`, - ); const chunks: Uint8Array[] = []; let size = 0; try { + const declared = Number(response.headers["content-length"]); + if (Number.isFinite(declared) && declared > maxBytes) + throw new GenerationError(`OpenAPI reference exceeds ${maxBytes} bytes: ${uri}`); + const encoding = response.headers["content-encoding"]; + if (encoding && encoding !== "identity") + throw new GenerationError( + `OpenAPI reference returned unsupported content encoding: ${encoding}`, + ); await withDeadline( new Promise((resolve, reject) => { response.message.on("data", (value: Buffer) => { @@ -416,9 +416,9 @@ async function fetchSource(uri: string, options: ResolvedLoadOptions): Promise= 300 && response.status < 400) { + response.message.destroy(); if (redirects >= options.maxRedirects) throw new GenerationError(`Too many redirects while fetching OpenAPI reference ${uri}`); - response.message.resume(); const location = response.headers.location; if (!location) throw new GenerationError(`OpenAPI redirect is missing Location: ${current.href}`); @@ -426,7 +426,7 @@ async function fetchSource(uri: string, options: ResolvedLoadOptions): Promise= 300) { - response.message.resume(); + response.message.destroy(); throw new GenerationError( `Unable to fetch OpenAPI reference ${current.href}: ${response.status} ${response.statusText}`, ); diff --git a/tests/filesystem-locks.test.ts b/tests/filesystem-locks.test.ts index df37444..537650d 100644 --- a/tests/filesystem-locks.test.ts +++ b/tests/filesystem-locks.test.ts @@ -76,6 +76,45 @@ function denyLockObservation( } describe("filesystem lock ownership", () => { + it.each(["directory", "file"])( + "propagates a one-shot %s lock preflight EACCES without misclassifying it as a rename collision", + async (kind) => { + const { root, target, lock } = await fixture(kind); + await fs.mkdir(lock); + const owner = path.join(lock, "owner.json"); + await fs.writeFile(owner, '{"pid":2147483647}'); + const lstat = fs.lstat.bind(fs); + const denied = Object.assign(new Error("injected preflight read denial"), { + code: "EACCES", + syscall: "lstat", + path: lock, + }); + let injected = false; + vi.spyOn(fs, "lstat").mockImplementation((async (...args: Parameters) => { + if (String(args[0]) === lock && !injected) { + injected = true; + throw denied; + } + return lstat(...args); + }) as typeof fs.lstat); + let entered = false; + await expect( + operate(kind, target, async () => { + entered = true; + }), + ).rejects.toBe(denied); + expect(injected).toBe(true); + expect(entered).toBe(false); + expect(await fs.readFile(owner, "utf8")).toBe('{"pid":2147483647}'); + if (kind === "file") expect(await fs.readFile(target, "utf8")).toBe("old"); + expect((await fs.readdir(root)).sort()).toEqual( + kind === "directory" + ? [path.basename(lock)] + : [path.basename(lock), "manifest.json"].sort(), + ); + }, + ); + it.each(observationCases)( "retries a transient Windows-style $syscall denial while the $kind lock is handed off", async ({ kind, syscall }) => { diff --git a/tests/fixtures/create-process-worker.ts b/tests/fixtures/create-process-worker.ts new file mode 100644 index 0000000..c07d60c --- /dev/null +++ b/tests/fixtures/create-process-worker.ts @@ -0,0 +1,3 @@ +import { runCreateCli } from "../../src/bin/create"; + +process.exit(await runCreateCli(["spa", "test-app", "--dir", process.argv[2], "--no-skills"])); diff --git a/tests/generate-remote.test.ts b/tests/generate-remote.test.ts index 89597a6..026b22d 100644 --- a/tests/generate-remote.test.ts +++ b/tests/generate-remote.test.ts @@ -1,13 +1,16 @@ import { EventEmitter } from "node:events"; -import { beforeEach, describe, expect, it, vi } from "vitest"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; const transport = vi.hoisted(() => ({ responses: [] as Array<{ status?: number; headers?: Record; chunks?: string[]; + neverEnd?: boolean; + abort?: boolean; }>, requests: [] as Array<{ url: URL; options: Record }>, + observations: [] as Array<{ destroyed: boolean; resumed: boolean; bytes: number }>, })); vi.mock("node:dns/promises", () => ({ @@ -38,14 +41,24 @@ vi.mock("node:https", async () => { message.statusCode = response.status ?? 200; message.statusMessage = "OK"; message.headers = response.headers ?? {}; - message.resume = () => {}; - message.destroy = () => {}; + const state = { destroyed: false, resumed: false, bytes: 0 }; + transport.observations.push(state); + message.resume = () => { + state.resumed = true; + }; + message.destroy = () => { + state.destroyed = true; + }; callback(message); setTimeout(() => { for (const chunk of response.chunks ?? []) { + if (state.destroyed) return; + state.bytes += Buffer.byteLength(chunk); message.emit("data", Buffer.from(chunk)); } - message.emit("end"); + if (state.destroyed) return; + if (response.abort) message.emit("aborted"); + else if (!response.neverEnd) message.emit("end"); }, 0); }; return request; @@ -61,6 +74,10 @@ describe("remote OpenAPI transport", () => { beforeEach(() => { transport.responses.length = 0; transport.requests.length = 0; + transport.observations.length = 0; + }); + afterEach(() => { + vi.useRealTimers(); }); it("should pin vetted addresses while preserving the TLS hostname", async () => { @@ -77,6 +94,7 @@ describe("remote OpenAPI transport", () => { const callback = vi.fn(); lookup("spec.example", {}, callback); expect(callback).toHaveBeenCalledWith(null, "93.184.216.34", 4); + expect(transport.observations[0]!.destroyed).toBe(false); }); it("should revalidate redirects and reject encoded bodies", async () => { @@ -91,6 +109,7 @@ describe("remote OpenAPI transport", () => { "/openapi.yaml", "/next.yaml", ]); + expect(transport.observations.map(({ destroyed }) => destroyed)).toEqual([true, true]); }); it("should reject chunked bodies above the byte ceiling", async () => { @@ -98,5 +117,90 @@ describe("remote OpenAPI transport", () => { await expect(loadOpenApi("https://spec.example/openapi.yaml", { maxBytes: 8 })).rejects.toThrow( "exceeds 8 bytes", ); + expect(transport.observations[0]!.destroyed).toBe(true); + }); + + it.each([ + [ + "encoding", + [{ headers: { "content-encoding": "gzip" }, neverEnd: true }], + {}, + /unsupported content encoding/, + ], + [ + "declared byte limit", + [{ headers: { "content-length": "100" }, neverEnd: true }], + { maxBytes: 8 }, + /exceeds 8 bytes/, + ], + [ + "redirect limit", + [ + { status: 302, headers: { location: "/again" }, neverEnd: true }, + { status: 302, headers: { location: "/again" }, neverEnd: true }, + ], + { maxRedirects: 1 }, + /Too many redirects/, + ], + ["missing redirect location", [{ status: 302, neverEnd: true }], {}, /missing Location/], + ["HTTP error", [{ status: 503, neverEnd: true }], {}, /Unable to fetch/], + ] as const)( + "destroys rejected %s response bodies instead of leaving them streaming", + async (_name, responses, options, message) => { + transport.responses.push(...responses); + await expect(loadOpenApi("https://spec.example/openapi.yaml", options)).rejects.toThrow( + message, + ); + expect(transport.observations).toHaveLength(responses.length); + expect(transport.observations.every(({ destroyed }) => destroyed)).toBe(true); + expect(transport.observations.every(({ resumed }) => !resumed)).toBe(true); + }, + ); + + it("disposes a redirected body before following a permitted redirect and retains the complete final response", async () => { + transport.responses.push( + { status: 302, headers: { location: "/next.yaml" }, neverEnd: true }, + { chunks: [document] }, + ); + await expect(loadOpenApi("https://spec.example/openapi.yaml")).resolves.toMatchObject({ + openapi: "3.1.0", + }); + expect(transport.observations.map(({ destroyed }) => destroyed)).toEqual([true, false]); + expect(transport.requests.map(({ url }) => url.pathname)).toEqual([ + "/openapi.yaml", + "/next.yaml", + ]); + }); + + it("disposes a redirect body before rejecting its cross-origin destination", async () => { + transport.responses.push({ + status: 302, + headers: { location: "https://foreign.example/spec" }, + neverEnd: true, + }); + await expect(loadOpenApi("https://spec.example/openapi.yaml")).rejects.toThrow( + "Cross-origin OpenAPI reference is not allowed", + ); + expect(transport.requests).toHaveLength(1); + expect(transport.observations[0]!.destroyed).toBe(true); + }); + + it("destroys a response that stalls past the load deadline", async () => { + vi.useFakeTimers(); + transport.responses.push({ chunks: ["partial"], neverEnd: true }); + const result = expect( + loadOpenApi("https://spec.example/openapi.yaml", { timeoutMs: 20 }), + ).rejects.toThrow(/Timed out/); + await vi.runAllTimersAsync(); + await result; + expect(transport.observations[0]!.destroyed).toBe(true); + }); + + it("destroys an aborted response without parsing its incomplete body", async () => { + transport.responses.push({ chunks: ["partial"], abort: true }); + await expect(loadOpenApi("https://spec.example/openapi.yaml")).rejects.toThrow( + "OpenAPI response body was aborted", + ); + expect(transport.observations[0]!.destroyed).toBe(true); }); }); diff --git a/tests/generation-publication.test.ts b/tests/generation-publication.test.ts index a4482f3..ff5123f 100644 --- a/tests/generation-publication.test.ts +++ b/tests/generation-publication.test.ts @@ -1,4 +1,5 @@ import fs from "node:fs/promises"; +import { watch } from "node:fs"; import path from "node:path"; import os from "node:os"; import { fork } from "node:child_process"; @@ -64,6 +65,161 @@ afterEach(async () => { await Promise.all(roots.splice(0).map((root) => fs.rm(root, { recursive: true, force: true }))); }); +describe("actual installer process outcomes", () => { + it.each(["failure", "missing", "terminated-installer", "terminated-cli", "success"])( + "preserves project ownership after %s with a real local installer process", + async (behavior) => { + const root = await fs.mkdtemp(path.join(os.tmpdir(), "askr-real-installer-")); + roots.push(root); + const target = path.join(root, "project"); + const bin = path.join(root, "bin"); + await fs.mkdir(bin); + await fs.writeFile(path.join(root, "unrelated.txt"), "unrelated\r\n"); + const markerPath = path.join(root, "started.json"); + const shim = path.join(bin, "installer.cjs"); + const source = `const fs = require("node:fs"); +fs.writeFileSync("package-lock.json", "partial owned install"); +fs.writeFileSync(${JSON.stringify(`${markerPath}.tmp`)}, JSON.stringify({ pid: process.pid, cwd: process.cwd() })); +fs.renameSync(${JSON.stringify(`${markerPath}.tmp`)}, ${JSON.stringify(markerPath)}); +${behavior === "success" ? "process.exit(0);" : behavior === "failure" ? "process.exit(23);" : "Atomics.wait(new Int32Array(new SharedArrayBuffer(4)), 0, 0);"} +`; + if (behavior !== "missing") { + await fs.writeFile(shim, source); + if (process.platform === "win32") { + await fs.writeFile( + path.join(bin, "npm.cmd"), + `@echo off\r\n"${process.execPath}" "${shim}"\r\n`, + ); + } else { + const quote = (value: string) => `'${value.replaceAll("'", "'\\''")}'`; + await fs.writeFile( + path.join(bin, "npm"), + `#!/bin/sh\nexec ${quote(process.execPath)} ${quote(shim)}\n`, + ); + await fs.chmod(path.join(bin, "npm"), 0o755); + } + } + type Installer = { pid: number; cwd: string }; + let ready!: (installer: Installer) => void; + const started = new Promise((resolve) => { + ready = resolve; + }); + const watcher = watch(root, async (_event, filename) => { + if (filename === null || String(filename) === "started.json") { + try { + ready(JSON.parse(await fs.readFile(markerPath, "utf8")) as Installer); + } catch {} + } + }); + const child = fork( + fileURLToPath(new URL("./fixtures/create-process-worker.ts", import.meta.url)), + [target], + { + silent: true, + execArgv: ["--import", "tsx"], + env: { + ...process.env, + PATH: behavior === "missing" ? bin : `${bin}${path.delimiter}${process.env.PATH}`, + npm_config_user_agent: "npm/12.0.2", + }, + }, + ); + const exited = once(child, "exit"); + let stdout = "", + stderr = ""; + child.stdout?.on("data", (chunk: Buffer) => { + stdout += chunk.toString(); + }); + child.stderr?.on("data", (chunk: Buffer) => { + stderr += chunk.toString(); + }); + let installer: Installer | undefined; + let installerWasTerminated = false; + let startTimer: ReturnType | undefined; + try { + if (behavior !== "missing") { + installer = await Promise.race([ + started, + exited.then(async () => { + try { + return JSON.parse(await fs.readFile(markerPath, "utf8")) as Installer; + } catch { + throw new Error(`CLI exited before the installer marker: ${stdout}\n${stderr}`); + } + }), + new Promise((_resolve, reject) => { + startTimer = setTimeout( + () => reject(new Error(`Installer did not start: ${stderr}`)), + 10_000, + ); + startTimer.unref(); + }), + ]); + expect(await fs.realpath(path.dirname(installer.cwd))).toBe(await fs.realpath(root)); + if (behavior === "terminated-installer") { + process.kill(installer.pid, "SIGKILL"); + installerWasTerminated = true; + } + if (behavior === "terminated-cli") expect(child.kill("SIGKILL")).toBe(true); + } + const [code, signal] = await exited; + expect(await fs.readFile(path.join(root, "unrelated.txt"), "utf8")).toBe("unrelated\r\n"); + if (behavior === "success") { + expect(code).toBe(0); + expect(signal).toBeNull(); + expect(stdout).toContain("Success! Created test-app"); + expect( + JSON.parse(await fs.readFile(path.join(target, "package.json"), "utf8")).name, + ).toBe("test-app"); + expect(await fs.readFile(path.join(target, "package-lock.json"), "utf8")).toBe( + "partial owned install", + ); + await expect(fs.access(installer!.cwd)).rejects.toMatchObject({ code: "ENOENT" }); + } else { + expect(stdout).not.toContain("Success! Created"); + await expect(fs.access(target)).rejects.toMatchObject({ code: "ENOENT" }); + if (behavior === "terminated-cli") { + expect([code, signal]).not.toEqual([0, null]); + expect(await fs.readFile(path.join(installer!.cwd, "package-lock.json"), "utf8")).toBe( + "partial owned install", + ); + } else { + expect(code).toBe(1); + expect(stderr).toContain("no project files were published"); + } + } + if (behavior !== "terminated-cli") { + expect( + (await fs.readdir(root)).some((entry) => entry.startsWith(".project.askr-create-")), + ).toBe(false); + } + } finally { + watcher.close(); + if (startTimer) clearTimeout(startTimer); + if (child.exitCode === null && child.signalCode === null) child.kill("SIGKILL"); + await exited; + if (!installer && (behavior === "terminated-cli" || behavior === "terminated-installer")) { + try { + installer = JSON.parse(await fs.readFile(markerPath, "utf8")) as Installer; + } catch {} + } + if ( + installer && + !installerWasTerminated && + (behavior === "terminated-cli" || behavior === "terminated-installer") + ) { + try { + process.kill(installer.pid, "SIGKILL"); + } catch (error) { + if ((error as NodeJS.ErrnoException).code !== "ESRCH") throw error; + } + } + } + }, + 20_000, + ); +}); + describe("generated directory ownership and recovery", () => { it.each([ "create-publish-failure", diff --git a/tests/update-cli.test.ts b/tests/update-cli.test.ts index d9760fa..debff6c 100644 --- a/tests/update-cli.test.ts +++ b/tests/update-cli.test.ts @@ -49,6 +49,65 @@ afterEach(async () => { }); describe("update CLI", () => { + test.each([ + ["truncated JSON", "{", null, /Malformed package manifest/], + ["null JSON", "null", null, /Malformed package manifest/], + ["array JSON", "[]", null, /Malformed package manifest/], + ["scalar JSON", "42", null, /Malformed package manifest/], + ["null workspaces", { workspaces: null }, null, /Invalid package.json workspaces/], + ["mixed workspaces", { workspaces: [42] }, null, /Invalid package.json workspaces/], + [ + "invalid packages", + { workspaces: { packages: false } }, + null, + /Invalid package.json workspaces/, + ], + ["null askr config", { askr: null }, null, /Invalid askr update configuration/], + ["array askr config", { askr: [] }, null, /Invalid askr update configuration/], + [ + "boolean update config", + { askr: { update: true } }, + null, + /Invalid askr.update configuration/, + ], + ["string ignore", { askr: { update: { ignore: "foo" } } }, null, /Invalid askr.update.ignore/], + ["mixed ignore", { askr: { update: { ignore: [1] } } }, null, /Invalid askr.update.ignore/], + ["array tags", { askr: { update: { tags: [] } } }, null, /Invalid askr.update.tags/], + ["numeric tag", { askr: { update: { tags: { foo: 4 } } } }, null, /Invalid askr.update.tags/], + ["empty tag", { askr: { update: { tags: { foo: "" } } } }, null, /Invalid askr.update.tags/], + ["malformed pnpm YAML", {}, "packages: [", /Malformed pnpm workspace declaration/], + ["scalar pnpm YAML", {}, "true", /Invalid pnpm workspace declaration/], + ["missing pnpm packages", {}, "other: []", /Invalid pnpm workspace declaration/], + ["mixed pnpm packages", {}, "packages: [42]", /Invalid pnpm workspace declaration/], + ] as const)( + "rejects %s before registry access and preserves every input byte", + async (_name, manifest, extra, diagnostic) => { + const root = await tempRoot( + typeof manifest === "string" ? manifest : JSON.stringify(manifest), + ); + await fs.writeFile(path.join(root, "unrelated.txt"), "unrelated\r\nexact bytes\0\n"); + if (extra !== null) await fs.writeFile(path.join(root, "pnpm-workspace.yaml"), extra); + const names = (await fs.readdir(root)).sort(); + const before = await Promise.all(names.map((name) => fs.readFile(path.join(root, name)))); + let registryCalls = 0; + const capture = ioCapture(); + expect( + await runUpdateCli(["--cwd", root, "--json"], capture.io, { + registry: async () => { + registryCalls += 1; + return { packuments: new Map(), failures: new Map() }; + }, + }), + ).toBe(1); + expect(registryCalls).toBe(0); + expect([...capture.errors, ...capture.logs].join("\n")).toMatch(diagnostic); + expect((await fs.readdir(root)).sort()).toEqual(names); + expect(await Promise.all(names.map((name) => fs.readFile(path.join(root, name))))).toEqual( + before, + ); + }, + ); + test("should leave a manifest byte-for-byte unchanged given a dry run when an update exists", async () => { const source = '{\r\n\t"name": "fixture",\r\n\t"dependencies": { "foo": "~1.0.0" }\r\n}'; const root = await tempRoot(source); From a1c4c6b8397a6276c1eb81a449f2859fb58ee97e Mon Sep 17 00:00:00 2001 From: Jeff Repanich Date: Sat, 10 Oct 2026 00:54:57 -0400 Subject: [PATCH 2/2] test: wait for installer handles before fixture cleanup --- tests/generation-publication.test.ts | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/tests/generation-publication.test.ts b/tests/generation-publication.test.ts index ff5123f..b539c99 100644 --- a/tests/generation-publication.test.ts +++ b/tests/generation-publication.test.ts @@ -125,6 +125,7 @@ ${behavior === "success" ? "process.exit(0);" : behavior === "failure" ? "proces }, ); const exited = once(child, "exit"); + const closed = once(child, "close"); let stdout = "", stderr = ""; child.stdout?.on("data", (chunk: Buffer) => { @@ -214,6 +215,9 @@ ${behavior === "success" ? "process.exit(0);" : behavior === "failure" ? "proces if ((error as NodeJS.ErrnoException).code !== "ESRCH") throw error; } } + // The installer (and Windows command shell) inherit these pipes. Wait + // for their handles to close before removing the retained fixture cwd. + await closed; } }, 20_000,