From 320d357b51663b7fdc2cb90034789c8be7e2b9a0 Mon Sep 17 00:00:00 2001 From: mohammed naji Date: Fri, 24 Jul 2026 12:33:14 +0400 Subject: [PATCH 1/3] test: stabilize packed CLI fixture lifecycle --- tests/package-contract.test.ts | 365 ++++++++++++++++++++++++++++----- 1 file changed, 316 insertions(+), 49 deletions(-) diff --git a/tests/package-contract.test.ts b/tests/package-contract.test.ts index 9bd9c7f8..a200532a 100644 --- a/tests/package-contract.test.ts +++ b/tests/package-contract.test.ts @@ -7,6 +7,7 @@ import { fileURLToPath } from "node:url"; import { tmpdir } from "node:os"; import { basename, dirname, join } from "node:path"; import { beforeAll, describe, expect, it } from "vitest"; +import { startFakeRemoteUpstream } from "./helpers/fake-remote-upstream.js"; interface PackageManifest { name?: string; @@ -306,6 +307,10 @@ class TermIgnoringNpmProcess extends EventEmitter implements NpmProcess { readonly stderr = new PassThrough(); readonly signals: NodeJS.Signals[] = []; + constructor(readonly pid?: number) { + super(); + } + kill(signal?: NodeJS.Signals): boolean { if (signal !== undefined) this.signals.push(signal); return true; @@ -374,6 +379,192 @@ function runInstalledBinary(binary: string, args: readonly string[], cwd: string ); } +interface InstalledBinaryResult { + readonly status: number | null; + readonly stdout: string; + readonly stderr: string; +} + +interface InstalledBinaryProcess { + readonly stdout: Readable; + readonly stderr: Readable; + readonly pid?: number; + kill(signal?: NodeJS.Signals): boolean; + once(event: "error", listener: (error: Error) => void): unknown; + once(event: "close", listener: (status: number | null, signal: NodeJS.Signals | null) => void): unknown; +} + +interface InstalledBinarySpawnOptions { + readonly cwd: string; + readonly shell: false; + readonly windowsHide: true; + readonly windowsVerbatimArguments?: true; + readonly stdio: ["ignore", "pipe", "pipe"]; +} + +type InstalledBinarySpawner = ( + command: string, + args: readonly string[], + options: InstalledBinarySpawnOptions +) => InstalledBinaryProcess; + +interface InstalledBinaryRuntime { + readonly platform: NodeJS.Platform; + terminateWindowsProcessTree(pid: number): Promise; +} + +function installedBinaryInvocation(binary: string, args: readonly string[]): { command: string; args: readonly string[] } { + if (process.platform !== "win32") return { command: binary, args }; + + return { + command: process.env.ComSpec ?? "cmd.exe", + args: ["/d", "/s", "/c", buildWindowsCommand(binary, args)] + }; +} + +const spawnInstalledBinary: InstalledBinarySpawner = (command, args, options) => { + const child = spawn(command, args, options); + if (child.stdout === null || child.stderr === null) { + throw new Error("Installed binary must be spawned with piped stdout and stderr."); + } + return child; +}; + +function windowsTaskkillPath(): string { + const systemRoot = process.env.SystemRoot ?? process.env.windir; + if (!systemRoot) throw new Error("SystemRoot is required to terminate a Windows process tree safely."); + return join(systemRoot, "System32", "taskkill.exe"); +} + +async function terminateWindowsProcessTree(pid: number): Promise { + return new Promise((resolve, reject) => { + const taskkill = spawn(windowsTaskkillPath(), ["/pid", String(pid), "/t", "/f"], { + shell: false, + windowsHide: true, + stdio: "ignore" + }); + taskkill.once("error", (error) => reject(error)); + taskkill.once("close", (status, signal) => { + if (status === 0 || status === 128) { + resolve(); + return; + } + reject( + new Error( + `taskkill could not terminate process tree ${pid}: ${ + status === null ? `terminated by ${signal ?? "an unknown signal"}` : `exited with status ${status}` + }` + ) + ); + }); + }); +} + +const defaultInstalledBinaryRuntime: InstalledBinaryRuntime = { + platform: process.platform, + terminateWindowsProcessTree +}; + +async function terminateInstalledBinary( + child: InstalledBinaryProcess, + runtime: InstalledBinaryRuntime, + force: boolean +): Promise { + if (runtime.platform === "win32") { + if (child.pid === undefined) { + throw new Error("Windows installed binary process did not expose a PID for process-tree cleanup."); + } + await runtime.terminateWindowsProcessTree(child.pid); + return; + } + child.kill(force ? "SIGKILL" : "SIGTERM"); +} + +async function runInstalledBinaryAsync( + binary: string, + args: readonly string[], + cwd: string, + timeoutMs = npmCommandTimeoutMs, + spawnProcess: InstalledBinarySpawner = spawnInstalledBinary, + runtime: InstalledBinaryRuntime = defaultInstalledBinaryRuntime +): Promise { + const invocation = installedBinaryInvocation(binary, args); + return new Promise((resolve, reject) => { + const child = spawnProcess(invocation.command, invocation.args, { + cwd, + shell: false, + windowsHide: true, + ...(process.platform === "win32" ? { windowsVerbatimArguments: true } : {}), + stdio: ["ignore", "pipe", "pipe"] + }); + let stdout = ""; + let stderr = ""; + let settled = false; + let timedOut = false; + let forceKill: ReturnType | undefined; + const timeoutError = () => + new Error(`Installed binary ${args.join(" ")} timed out after ${timeoutMs}ms.${npmDiagnostics(stdout, stderr)}`); + const settle = (outcome: () => void): void => { + if (settled) return; + settled = true; + clearTimeout(timeout); + if (forceKill !== undefined) clearTimeout(forceKill); + outcome(); + }; + const cleanupFailure = (error: unknown): void => { + child.kill("SIGKILL"); + settle(() => { + const message = error instanceof Error ? error.message : String(error); + reject(new Error(`Installed binary cleanup failed: ${message}.${npmDiagnostics(stdout, stderr)}`)); + }); + }; + const terminate = (force: boolean): void => { + void terminateInstalledBinary(child, runtime, force).catch(cleanupFailure); + }; + const timeout = setTimeout(() => { + timedOut = true; + terminate(false); + forceKill = setTimeout(() => { + if (settled) return; + if (runtime.platform !== "win32") { + child.kill("SIGKILL"); + settle(() => reject(timeoutError())); + return; + } + void terminateInstalledBinary(child, runtime, true).then( + () => settle(() => reject(timeoutError())), + cleanupFailure + ); + }, npmTerminationGraceMs); + }, timeoutMs); + + child.stdout.setEncoding("utf8"); + child.stdout.on("data", (chunk: string) => { + stdout += chunk; + }); + child.stderr.setEncoding("utf8"); + child.stderr.on("data", (chunk: string) => { + stderr += chunk; + }); + child.once("error", (error) => { + settle(() => reject(new Error(`Installed binary ${args.join(" ")} could not start: ${error.message}.${npmDiagnostics(stdout, stderr)}`))); + }); + child.once("close", (status, signal) => { + settle(() => { + if (timedOut) { + reject(timeoutError()); + return; + } + if (status === null) { + reject(new Error(`Installed binary ${args.join(" ")} terminated by ${signal ?? "an unknown signal"}.${npmDiagnostics(stdout, stderr)}`)); + return; + } + resolve({ status, stdout, stderr }); + }); + }); + }); +} + function quoteForPosixShell(value: string): string { return `'${value.replace(/'/gu, "'\"'\"'")}'`; } @@ -598,6 +789,53 @@ describe("packed artifact contract", () => { await expect(running).resolves.toMatchObject({ status: 0 }); }); + it("keeps the test worker responsive while a remote-backed installed binary is pending", async () => { + let completed = false; + const child = new DelayedNpmProcess(100); + const running = runInstalledBinaryAsync( + "miftah", + ["doctor", "--config", "remote.json"], + repositoryRoot, + 1_000, + () => child + ).finally(() => { + completed = true; + }); + + await new Promise((resolve) => setTimeout(resolve, 20)); + expect(completed).toBe(false); + await expect(running).resolves.toMatchObject({ status: 0 }); + }); + + it("kills the whole Windows command tree when an async installed binary times out", async () => { + const child = new TermIgnoringNpmProcess(42_424); + const killedTreePids: number[] = []; + const result = await Promise.race([ + runInstalledBinaryAsync( + "miftah.cmd", + ["doctor", "--config", "remote.json"], + repositoryRoot, + 5, + () => child, + { + platform: "win32", + terminateWindowsProcessTree: async (pid) => { + killedTreePids.push(pid); + child.emit("close", null, "SIGKILL"); + } + } + ).then( + () => "resolved", + (error: unknown) => error + ), + new Promise<"pending">((resolve) => setTimeout(() => resolve("pending"), 500)) + ]); + + expect(result).toBeInstanceOf(Error); + expect(killedTreePids).toEqual([42_424]); + expect(child.signals).toEqual([]); + }); + it("keeps an active npm command alive when output proves forward progress", async () => { const child = new ProgressingNpmProcess(15, 25); const spawnProgressingChild: NpmSpawner = () => child; @@ -934,7 +1172,7 @@ describe("packed artifact contract", () => { TEST_INITIALIZED_PATH: healthyInitializedPath }) ); - const healthyDoctor = runInstalledBinary(binary, ["doctor", "--config", healthyConfigPath], directory); + const healthyDoctor = await runInstalledBinaryAsync(binary, ["doctor", "--config", healthyConfigPath], directory); expect( healthyDoctor.status, [healthyDoctor.stderr || healthyDoctor.stdout, fixtureLifecycleDiagnostic(healthyStartedPath, healthyInitializedPath)] @@ -951,7 +1189,7 @@ describe("packed artifact contract", () => { "doctor-degraded.json", doctorConfig("packed-doctor-degraded", { TEST_FAIL_LIST_RESOURCES: "true" }) ); - const degradedDoctor = runInstalledBinary( + const degradedDoctor = await runInstalledBinaryAsync( binary, ["doctor", "--json", "--config", degradedConfigPath], directory @@ -1027,6 +1265,20 @@ describe("packed artifact contract", () => { process: { startupTimeoutMs: 1_000, shutdownTimeoutMs: 1_000 }, ...extras }); + const remoteCliConfig = ( + name: string, + profiles: Record, + url: string, + extras: Record = {} + ) => ({ + version: "1", + name, + defaultProfile: "work", + upstream: { transport: "streamable-http", url }, + profiles, + process: { startupTimeoutMs: 1_000, shutdownTimeoutMs: 1_000 }, + ...extras + }); const httpServeConfigPath = await writeCliConfig( "http serve config.json", @@ -1198,52 +1450,67 @@ describe("packed artifact contract", () => { } }); - const automationConfigPath = await writeCliConfig( - "automation config with spaces.json", - cliConfig("packed-cli-automation", { - work: { env: { TEST_ACCOUNT_NAME: "automation-account" } } - }) - ); - const schemaAutomation = runInstalledBinary(binary, ["schema"], cliContractDirectory); - expect(schemaAutomation.status, schemaAutomation.stderr || schemaAutomation.stdout).toBe(0); - expect(schemaAutomation.stderr).toBe(""); - expect(JSON.parse(schemaAutomation.stdout)).toMatchObject({ - $schema: "https://json-schema.org/draft/2019-09/schema#" - }); - const validateAutomation = runInstalledBinary( - binary, - ["validate", "--config", automationConfigPath], - cliContractDirectory - ); - expect(validateAutomation.status, validateAutomation.stderr || validateAutomation.stdout).toBe(0); - expect(validateAutomation.stderr).toBe(""); - expect(JSON.parse(validateAutomation.stdout)).toMatchObject({ ok: true, name: "packed-cli-automation" }); - const doctorAutomation = runInstalledBinary( - binary, - ["doctor", "--json", "--config", automationConfigPath], - cliContractDirectory - ); - expect(doctorAutomation.status, doctorAutomation.stderr || doctorAutomation.stdout).toBe(0); - expect(doctorAutomation.stderr).toBe(""); - expect(JSON.parse(doctorAutomation.stdout)).toMatchObject({ ok: true, overallStatus: "healthy" }); - const listedTools = runInstalledBinary( - binary, - ["list-tools", "--config", automationConfigPath, "--profile", "work"], - cliContractDirectory - ); - expect(listedTools.status, listedTools.stderr || listedTools.stdout).toBe(0); - expect(listedTools.stderr).toBe(""); - expect(JSON.parse(listedTools.stdout)).toEqual( - expect.arrayContaining([expect.objectContaining({ name: "whoami" })]) - ); - const testedProfile = runInstalledBinary( - binary, - ["test-profile", "--config", automationConfigPath, "--profile", "work"], - cliContractDirectory - ); - expect(testedProfile.status, testedProfile.stderr || testedProfile.stdout).toBe(0); - expect(testedProfile.stderr).toBe(""); - expect(JSON.parse(testedProfile.stdout)).toEqual({ ok: true, profile: "work" }); + const automationUpstream = await startFakeRemoteUpstream(); + try { + const automationConfigPath = await writeCliConfig( + "automation config with spaces.json", + remoteCliConfig( + "packed-cli-automation", + { work: { headers: { "X-Profile": "automation-account" } } }, + automationUpstream.streamableHttpUrl + ) + ); + const schemaAutomation = runInstalledBinary(binary, ["schema"], cliContractDirectory); + expect(schemaAutomation.status, schemaAutomation.stderr || schemaAutomation.stdout).toBe(0); + expect(schemaAutomation.stderr).toBe(""); + expect(JSON.parse(schemaAutomation.stdout)).toMatchObject({ + $schema: "https://json-schema.org/draft/2019-09/schema#" + }); + const validateAutomation = runInstalledBinary( + binary, + ["validate", "--config", automationConfigPath], + cliContractDirectory + ); + expect(validateAutomation.status, validateAutomation.stderr || validateAutomation.stdout).toBe(0); + expect(validateAutomation.stderr).toBe(""); + expect(JSON.parse(validateAutomation.stdout)).toMatchObject({ ok: true, name: "packed-cli-automation" }); + const doctorAutomation = await runInstalledBinaryAsync( + binary, + ["doctor", "--json", "--config", automationConfigPath], + cliContractDirectory + ); + expect(doctorAutomation.status, doctorAutomation.stderr || doctorAutomation.stdout).toBe(0); + expect(doctorAutomation.stderr).toBe(""); + expect(JSON.parse(doctorAutomation.stdout)).toMatchObject({ ok: true, overallStatus: "healthy" }); + const listedTools = await runInstalledBinaryAsync( + binary, + ["list-tools", "--config", automationConfigPath, "--profile", "work"], + cliContractDirectory + ); + expect(listedTools.status, listedTools.stderr || listedTools.stdout).toBe(0); + expect(listedTools.stderr).toBe(""); + expect(JSON.parse(listedTools.stdout)).toEqual( + expect.arrayContaining([expect.objectContaining({ name: "whoami" })]) + ); + const testedProfile = await runInstalledBinaryAsync( + binary, + ["test-profile", "--config", automationConfigPath, "--profile", "work"], + cliContractDirectory + ); + expect(testedProfile.status, testedProfile.stderr || testedProfile.stdout).toBe(0); + expect(testedProfile.stderr).toBe(""); + expect(JSON.parse(testedProfile.stdout)).toEqual({ ok: true, profile: "work" }); + expect(automationUpstream.requests()).toEqual( + expect.arrayContaining([ + expect.objectContaining({ + path: "/mcp", + headers: expect.objectContaining({ "x-profile": "automation-account" }) + }) + ]) + ); + } finally { + await automationUpstream.close(); + } const noRuntimeStartPath = join(cliContractDirectory, "runtime must not start"); const unavailableSecretName = "MIFTAH_PACKED_CONTRACT_MISSING_SECRET"; @@ -1320,7 +1587,7 @@ describe("packed artifact contract", () => { { secrets: { allowPlaintextSecrets: true } } ) ); - const failedInit = runInstalledBinary( + const failedInit = await runInstalledBinaryAsync( binary, ["test-profile", "--config", failedInitConfigPath], cliContractDirectory From be228f039fd1b6c23d502a422d2de27c77051493 Mon Sep 17 00:00:00 2001 From: mohammed naji Date: Fri, 24 Jul 2026 13:10:15 +0400 Subject: [PATCH 2/3] fix(audit): tolerate queued local lock probe results --- src/audit/audit-journal.ts | 9 +++++- tests/audit-integrity.test.ts | 60 +++++++++++++++++++++++++++++++++++ 2 files changed, 68 insertions(+), 1 deletion(-) diff --git a/src/audit/audit-journal.ts b/src/audit/audit-journal.ts index fca26ed5..2652fe2d 100644 --- a/src/audit/audit-journal.ts +++ b/src/audit/audit-journal.ts @@ -683,10 +683,12 @@ async function inspectLocalLockPort(port: number, key: string): Promise | undefined; const settle = (state: LocalLockPortState): void => { if (settled) return; settled = true; clearTimeout(timeout); + if (timeoutImmediate !== undefined) clearImmediate(timeoutImmediate); socket.destroy(); resolve(state); }; @@ -694,7 +696,12 @@ async function inspectLocalLockPort(port: number, key: string): Promise settle("unknown"), localLockProbeMilliseconds); + const timeout = setTimeout(() => { + // Under host scheduling pressure, a local connection result can already + // be queued when the timer phase runs. Give that result one check phase + // to settle before treating the holder as incomplete and failing closed. + timeoutImmediate = setImmediate(() => settle("unknown")); + }, localLockProbeMilliseconds); socket.setEncoding("utf8"); socket.on("data", (chunk: string) => { response += chunk; diff --git a/tests/audit-integrity.test.ts b/tests/audit-integrity.test.ts index ba8fd279..47c3fcab 100644 --- a/tests/audit-integrity.test.ts +++ b/tests/audit-integrity.test.ts @@ -1,4 +1,5 @@ import { createHash } from "node:crypto"; +import { EventEmitter } from "node:events"; import { copyFile, link, mkdtemp, open, readFile, readdir, rename, symlink, unlink, writeFile } from "node:fs/promises"; import type { FileHandle } from "node:fs/promises"; import { createServer } from "node:net"; @@ -157,6 +158,65 @@ describe("audit journal integrity", () => { } }); + it("does not mistake a queued local refusal for an incomplete lock holder", async () => { + const directory = await mkdtemp(join(tmpdir(), "miftah-audit-integrity-delayed-lock-probe-")); + const path = join(directory, "audit.jsonl"); + class ProbeSocket extends EventEmitter { + setEncoding(): this { + return this; + } + + destroy(): this { + return this; + } + } + + const socket = new ProbeSocket(); + const originalSetTimeout = global.setTimeout; + let probeTimedOut = false; + let probeTimerIntercepted = false; + vi.resetModules(); + vi.doMock("node:net", async () => { + const actual = await vi.importActual("node:net"); + return { ...actual, connect: () => socket }; + }); + const { AuditLogger: IsolatedAuditLogger } = await import("../src/audit/audit-logger.js"); + const nowSpy = vi.spyOn(Date, "now").mockImplementation(() => (probeTimedOut ? 5_000 : 0)); + const setTimeoutSpy = vi.spyOn(global, "setTimeout").mockImplementation( + ((callback: (...args: never[]) => void, delay?: number, ...args: never[]) => { + if (!probeTimerIntercepted && delay === 100) { + probeTimerIntercepted = true; + const timer = originalSetTimeout(() => undefined, 1_000); + queueMicrotask(() => { + probeTimedOut = true; + callback(...args); + socket.emit("error", Object.assign(new Error("connection refused"), { code: "ECONNREFUSED" })); + }); + return timer; + } + return originalSetTimeout(callback, delay, ...args); + }) as unknown as typeof setTimeout + ); + + try { + const logger = new IsolatedAuditLogger(path, { integrity: { algorithm: "sha256-chain" } }); + await expect(logger.log({ + wrapper: "github", + profile: "work", + operation: "tools/call", + name: "writes-after-delayed-local-lock-probe", + status: "success", + durationMs: 1 + })).resolves.toBeUndefined(); + expect(probeTimerIntercepted).toBe(true); + } finally { + setTimeoutSpy.mockRestore(); + nowSpy.mockRestore(); + vi.doUnmock("node:net"); + vi.resetModules(); + } + }); + it("identifies the first tampered chained record without returning record content", async () => { const directory = await mkdtemp(join(tmpdir(), "miftah-audit-integrity-")); const path = join(directory, "audit.jsonl"); From 30e0310e114a34938adba22bf4ce9ae21c973b51 Mon Sep 17 00:00:00 2001 From: mohammed naji Date: Fri, 24 Jul 2026 13:12:32 +0400 Subject: [PATCH 3/3] test: share installed binary invocation contract --- tests/package-contract.test.ts | 24 +++++++----------------- 1 file changed, 7 insertions(+), 17 deletions(-) diff --git a/tests/package-contract.test.ts b/tests/package-contract.test.ts index a200532a..4dcc8858 100644 --- a/tests/package-contract.test.ts +++ b/tests/package-contract.test.ts @@ -360,23 +360,13 @@ function buildWindowsCommand(binary: string, args: readonly string[]): string { } function runInstalledBinary(binary: string, args: readonly string[], cwd: string) { - if (process.platform !== "win32") { - return spawnSync(binary, args, { - cwd, - encoding: "utf8", - timeout: npmCommandTimeoutMs - }); - } - return spawnSync( - process.env.ComSpec ?? "cmd.exe", - ["/d", "/s", "/c", buildWindowsCommand(binary, args)], - { - cwd, - encoding: "utf8", - timeout: npmCommandTimeoutMs, - windowsVerbatimArguments: true - } - ); + const invocation = installedBinaryInvocation(binary, args); + return spawnSync(invocation.command, invocation.args, { + cwd, + encoding: "utf8", + timeout: npmCommandTimeoutMs, + ...(process.platform === "win32" ? { windowsVerbatimArguments: true } : {}) + }); } interface InstalledBinaryResult {