mirror of
https://github.com/morluto/rea
synced 2026-10-25 22:08:11 -04:00
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
This commit is contained in:
@@ -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.
|
||||
|
||||
@@ -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-"));
|
||||
|
||||
@@ -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",
|
||||
);
|
||||
|
||||
+10
-4
@@ -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<string> =>
|
||||
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<string> => {
|
||||
await stat(root);
|
||||
return realpath(root);
|
||||
};
|
||||
|
||||
/** Private input copy isolates upstream database and sidecar writes from the original. */
|
||||
export class IdaWorkspace {
|
||||
|
||||
@@ -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 () => {
|
||||
|
||||
@@ -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([]);
|
||||
},
|
||||
);
|
||||
|
||||
|
||||
Reference in New Issue
Block a user