From 52aeeee764c407a1888931ecbc9c46f6536650cc Mon Sep 17 00:00:00 2001 From: morluto <76467478+morluto@users.noreply.github.com> Date: Sat, 24 Oct 2026 05:44:56 +0800 Subject: [PATCH] fix(ida): preserve native workspace admission and startup outcomes Validate the selected workspace lookup through stat before canonicalization, preserving typed configuration causes and physical traversal semantics. Cover readiness without writes and tie startup cleanup regressions to explicit cancellation or timeout producer outcomes. Fixes #6828 --- docs/ida-provider.md | 7 ++ src/ida/IdaDoctor.test.ts | 32 +++++++++ src/ida/IdaStartupFailures.test.ts | 66 +++++++++++++++---- src/ida/IdaWorkspace.ts | 14 ++-- src/ida/IdaWorkspaceOwnership.test.ts | 40 ++++++++++- .../cli/idaDoctorRegistration.test.ts | 27 ++++++-- 6 files changed, 160 insertions(+), 26 deletions(-) diff --git a/docs/ida-provider.md b/docs/ida-provider.md index 25b0191606..04221abe8a 100644 --- a/docs/ida-provider.md +++ b/docs/ida-provider.md @@ -110,6 +110,13 @@ REA copies the admitted binary into a private temporary directory, checks its di One REA session owns one database. Stdio registrations default `IDA_MCP_MAX_WORKERS` to `1`; the explicit upstream `--max-workers 1` also bounds a supervisor. Database size and engine memory depend on the target and upstream; REA does not impose an engine memory limit. The original binary is never executed or overwritten. Optional `workspaceRoot` selects an existing absolute temporary-directory parent on the REA host. The selected parent is resolved by the filesystem before deriving its canonical identity, including POSIX symlink/parent components. Ensure it has space for the input copy and IDA working files. +Readiness and allocation check the selected spelling with native +[`stat`](https://github.com/nodejs/node/blob/v24.22.1/doc/api/fs.md#fspromisesstatpath-options) +before canonicalizing it. This retains missing and non-directory intermediate +lookup failures: [macOS `realpath`](https://github.com/apple-oss-distributions/Libc/blob/71bbe350ab79eef58113991d817ccc6165061a64/stdlib/FreeBSD/realpath.c) +alone can collapse `file/..`. Diagnostics retain the selected spelling; private +input paths use the canonical parent for upstream identity checks. + When a completed `idb_open` response reports success or is otherwise ambiguous, but no active owned private worker can be verified, REA retains the private workspace even if `idb_list` appears empty. A later close can retry verification if the original owned worker becomes discoverable. A completed explicit `idb_open` rejection retains the existing no-worker cleanup path. This is a local admission/ownership safeguard and **does not** provide a process-termination receipt or change the legacy reaped-worker protocol. Headless cleanup uses the upstream database lifecycle. Abruptly terminating REA, its proxy, or the supervisor does not establish worker release. Prefer `close_binary` for MCP sessions; after an interrupted process, inspect upstream session inventory and retained workspace diagnostics before removing working files. This adapter does not claim Windows Job Object containment for the upstream detached workers. diff --git a/src/ida/IdaDoctor.test.ts b/src/ida/IdaDoctor.test.ts index f925e79425..aaffe3bf2c 100644 --- a/src/ida/IdaDoctor.test.ts +++ b/src/ida/IdaDoctor.test.ts @@ -44,6 +44,38 @@ it.each([true, false])( }, ); +it.skipIf(process.platform === "win32")( + "rejects a non-directory intermediate component before canonicalizing readiness", + async () => { + const root = await mkdtemp(join(tmpdir(), "rea-ida-workspace-lookup-")); + roots.push(root); + const path = join(root, "registration.json"); + const workspace = join(root, "workspace"); + await mkdir(workspace); + await writeFile(join(root, "file"), "not a directory"); + const selected = `${root}/file/../workspace`; + await writeFile( + path, + JSON.stringify({ + command: "provider-must-not-launch", + mode: "headless", + workspaceRoot: selected, + }), + ); + const before = await readdir(root); + const inspection = await inspectIdaRegistration(path, resourceScope); + expect(inspection.available).toBe(false); + const check = inspection.checks.find( + ({ name }) => name === "private_workspace", + ); + expect(check).toMatchObject({ ok: false, code: "invalid_configuration" }); + expect(check?.detail).toContain(selected); + expect(check?.detail).toContain("ENOTDIR"); + expect(await readdir(root)).toEqual(before); + expect(await readdir(workspace)).toEqual([]); + }, +); + it("marks an unusable system temporary parent unavailable when workspaceRoot is omitted", async () => { if (process.platform === "win32") return; const root = await mkdtemp(join(tmpdir(), "rea-ida-default-parent-")); diff --git a/src/ida/IdaStartupFailures.test.ts b/src/ida/IdaStartupFailures.test.ts index a3fbcd01aa..f7be07f3f2 100644 --- a/src/ida/IdaStartupFailures.test.ts +++ b/src/ida/IdaStartupFailures.test.ts @@ -5,6 +5,7 @@ import { RecordingIdaMcp, } from "../../tests/fixtures/idaMcp.js"; import { + AnalysisCancelledError, AnalysisTimeoutError, AnalysisResourceConstraintError, } from "../domain/analysisErrorCore.js"; @@ -123,19 +124,35 @@ it.each(["timeout", "resource"] as const)( }, ); -it.each(["pending", "released"] as const)( - "keeps cancellation linked to the exact startup cleanup owner when %s", - async (state) => { +it.each([ + { kind: "cancellation", state: "pending" }, + { kind: "cancellation", state: "released" }, + { kind: "timeout", state: "pending" }, + { kind: "timeout", state: "released" }, +] as const)( + "keeps producer $kind linked to the exact startup cleanup owner when $state", + async ({ kind, state }) => { const { root, target } = await createIdaTarget(); const controller = new AbortController(); - const primary = new AnalysisTimeoutError("ida_mcp_connect", 123, { + const callerReason = { caller: "startup wait cancelled" }; + const sourceOptions = { + diagnostics: { stage_fact: "original" }, cleanup: { reason: "release denied", resources: ["owned transport"] }, capturedOutput: { - stdout: "", + stdout: "original output", stderr: "original warning", - truncated: false, + truncated: true, }, - }); + }; + // The producer authors the outcome; an aborted caller alone cannot turn + // an established startup timeout into cancellation. + const primary = + kind === "cancellation" + ? new AnalysisCancelledError("ida_mcp_connect", { + ...sourceOptions, + cause: callerReason, + }) + : new AnalysisTimeoutError("ida_mcp_connect", 123, sourceOptions); let permitRelease = state === "released"; const lease = new AnalysisCleanupLease(async () => { if (!permitRelease) throw new Error("release denied"); @@ -143,8 +160,9 @@ it.each(["pending", "released"] as const)( lease.bindCleanupError(primary); const producer = new RecordingIdaMcp(target, "headless"); producer.connect = async () => { + producer.connects++; if (state === "released") await lease.release(); - controller.abort("caller reason"); + controller.abort(callerReason); throw primary; }; const client = new IdaSessionClient( @@ -169,11 +187,22 @@ it.each(["pending", "released"] as const)( { procedure: "main" }, { signal: controller.signal }, ); - if (result.ok) throw new Error("Expected cancellation"); + if (result.ok) throw new Error("Expected startup failure"); + expect(producer.connects).toBe(1); + expect(producer.calls).toEqual([]); + expect(controller.signal.reason).toBe(callerReason); + expect(primary.operation).toBe("ida_mcp_connect"); + expect(result.error.cause).toBe( + kind === "cancellation" ? callerReason : primary, + ); if (state === "pending") { - expect(projectAnalysisError(result.error).code).toBe( - "cleanup_incomplete", - ); + expect(projectAnalysisError(result.error)).toMatchObject({ + code: "cleanup_incomplete", + details: { + resources: ["owned transport"], + captured_output: sourceOptions.capturedOutput, + }, + }); await expect(lease.release()).rejects.toThrow("release denied"); expect(projectAnalysisError(result.error).code).toBe( "cleanup_incomplete", @@ -182,12 +211,21 @@ it.each(["pending", "released"] as const)( await lease.release(); } expect(projectAnalysisError(result.error)).toMatchObject({ - code: "cancelled", + code: kind === "cancellation" ? "cancelled" : "provider_timeout", details: { operation: "procedure_pseudo_code", - captured_output: { stderr: "original warning" }, + captured_output: sourceOptions.capturedOutput, + diagnostics: sourceOptions.diagnostics, }, }); + if (kind === "timeout") + expect(projectAnalysisError(result.error)).toMatchObject({ + retryable: true, + details: { + timeout_ms: 123, + diagnostics: { startup_operation: "ida_mcp_connect" }, + }, + }); expect(projectAnalysisError(result.error).details).not.toHaveProperty( "resources", ); diff --git a/src/ida/IdaWorkspace.ts b/src/ida/IdaWorkspace.ts index 5251b3de35..9dd0106774 100644 --- a/src/ida/IdaWorkspace.ts +++ b/src/ida/IdaWorkspace.ts @@ -1,6 +1,6 @@ import { createHash, randomUUID } from "node:crypto"; import { createReadStream } from "node:fs"; -import { open, realpath } from "node:fs/promises"; +import { open, realpath, stat } from "node:fs/promises"; import { tmpdir } from "node:os"; import { basename, join, normalize } from "node:path"; import type { BinaryTarget } from "../domain/binaryTargetTypes.js"; @@ -22,9 +22,15 @@ export const sameIdaHostPath = (left: string, right: string): boolean => ? normalize(left).toLowerCase() === normalize(right).toLowerCase() : normalize(left) === normalize(right); -/** Resolve the exact existing parent used by both readiness and allocation. */ -export const resolveIdaWorkspaceParent = (root = tmpdir()): Promise => - realpath(root); +/** Admit the selected lookup before canonicalizing it for readiness and allocation. + * Native realpath can collapse file/.. without rejecting the non-directory. + */ +export const resolveIdaWorkspaceParent = async ( + root = tmpdir(), +): Promise => { + await stat(root); + return realpath(root); +}; /** Private input copy isolates upstream database and sidecar writes from the original. */ export class IdaWorkspace { diff --git a/src/ida/IdaWorkspaceOwnership.test.ts b/src/ida/IdaWorkspaceOwnership.test.ts index d0e014fa3e..7478abbfe3 100644 --- a/src/ida/IdaWorkspaceOwnership.test.ts +++ b/src/ida/IdaWorkspaceOwnership.test.ts @@ -1,5 +1,13 @@ import { requirePermissionDenial } from "../../tests/fixtures/permissionDenial.js"; -import { chmod, mkdir, readdir, realpath, rm, symlink } from "node:fs/promises"; +import { + chmod, + mkdir, + readdir, + realpath, + rm, + symlink, + writeFile, +} from "node:fs/promises"; import { dirname, join } from "node:path"; import { afterEach, describe, expect, it } from "vitest"; import { @@ -37,6 +45,36 @@ const clientFor = (root: string, target: BinaryTarget) => { }; describe("IDA workspace ownership before input preparation", () => { + it.skipIf(process.platform === "win32").each([ + ["missing", "ENOENT"], + ["file", "ENOTDIR"], + ] as const)( + "retains the native %s-component failure as the workspace configuration cause", + async (component, code) => { + const { root, target } = await createIdaTarget(); + roots.push(root); + const lexical = join(root, "workspace"); + await mkdir(lexical); + if (component === "file") + await writeFile(join(root, component), "not a directory"); + const selected = `${root}/${component}/../workspace`; + await expect( + IdaWorkspace.allocate(target, selected), + ).rejects.toMatchObject({ + _tag: "ConfigurationError", + cause: { code, path: selected }, + settings: [ + { + setting: "workspaceRoot", + constraint: expect.stringContaining(code), + }, + ], + diagnostics: { workspace_root: selected }, + }); + expect(await readdir(lexical)).toEqual([]); + }, + ); + it.skipIf(process.platform === "win32")( "allocates in the physical parent selected through a symlink and ..", async () => { diff --git a/tests/boundary/cli/idaDoctorRegistration.test.ts b/tests/boundary/cli/idaDoctorRegistration.test.ts index 8a9a5b870c..e7965c8663 100644 --- a/tests/boundary/cli/idaDoctorRegistration.test.ts +++ b/tests/boundary/cli/idaDoctorRegistration.test.ts @@ -1,5 +1,5 @@ import { execFileSync } from "node:child_process"; -import { mkdir, readdir, writeFile } from "node:fs/promises"; +import { mkdir, readFile, readdir, symlink, writeFile } from "node:fs/promises"; import { join } from "node:path"; import { expect } from "vitest"; @@ -37,13 +37,19 @@ cliTest.skipIf(process.platform === "win32")( ); cliTest.skipIf(process.platform !== "darwin").for([true, false])( - "doctor checks the IDA workspace parent without creating entries (usable=%s)", + "doctor checks the filesystem-selected IDA parent without creating entries (usable=%s)", async (usable, { cli }) => { const root = await createTestTempDirectory("rea-ida-doctor-parent-"); - const parent = join(root, "parent"); - await mkdir(parent); - // Keep the caller's spelling while every traversed component exists. - const workspaceRoot = usable ? `${parent}/./` : join(parent, "absent"); + const physical = join(root, "physical"); + const parent = join(physical, "workspace"); + const lexical = join(root, "workspace"); + await mkdir(join(physical, "inner"), { recursive: true }); + if (usable) { + await mkdir(parent); + await writeFile(lexical, "lexical parent is a file"); + } else await mkdir(lexical); + await symlink("physical/inner", join(root, "link")); + const workspaceRoot = `${root}/link/../workspace`; const registration = join(root, "registration.json"); await writeFile( registration, @@ -53,6 +59,8 @@ cliTest.skipIf(process.platform !== "darwin").for([true, false])( workspaceRoot, }), ); + const registrationBytes = await readFile(registration); + const before = await readdir(physical); const result = await cli.run({ arguments: ["doctor", "--provider", "ida", "--json"], environment: { HOME: root, REA_IDA_MCP_CONFIG: registration }, @@ -74,7 +82,12 @@ cliTest.skipIf(process.platform !== "darwin").for([true, false])( }), ]), }); - expect(await readdir(parent)).toEqual([]); + expect(await readFile(registration)).toEqual(registrationBytes); + expect(await readdir(physical)).toEqual(before); + if (usable) { + expect(await readdir(parent)).toEqual([]); + expect(await readFile(lexical, "utf8")).toBe("lexical parent is a file"); + } else expect(await readdir(lexical)).toEqual([]); }, );