diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index dfc4a4889..83218a639 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -277,7 +277,7 @@ jobs: - run: npm run verify:keyed-archive - run: npm run verify:asset-catalog - name: Native compiled NIB hierarchy regressions - run: npm run test:focused -- src/application/NibNativeHierarchy.test.ts + run: npm run test:focused -- tests/boundary/filesystem/nibNativeHierarchy.test.ts tests/boundary/filesystem/interfaceBuilderBundle.test.ts - name: Real binary metadata and Interface Builder integration run: npm run verify:apple-dispatch && npm run verify:interface-builder - name: Native plist CLI selection regressions diff --git a/docs/test-suite-audit.md b/docs/test-suite-audit.md deleted file mode 100644 index 2a439cb5d..000000000 --- a/docs/test-suite-audit.md +++ /dev/null @@ -1,169 +0,0 @@ -# Test suite audit - -## Scope and priority - -The expanded audit on 2026-10-04 reviewed every one of the 355 Vitest files -present at its start: 82 domain/contracts, 56 application/composition, 147 -adapter/configuration/process/filesystem/CLI files, and 70 MCP/acceptance/ -conformance/evaluation files. Three review passes checked the proposed removals -against existing stronger coverage and checked the final changes for lost -assertions. The remaining suite has 351 files. - -Prefer full real-provider E2E, then integration across schemas and transports, -then real-data goldens. Remove lower-level duplication once the replacement -asserts the same behavior. Keep distinct failure regressions that a successful -E2E cannot exercise. Do not lower coverage thresholds to accommodate pruning. - -## Changes made - -| Previous test or assertion | Decision | Replacement or retained claim | -| ----------------------------------------------------------------------------------------------- | ------------------------------------- | ------------------------------------------------------------------------------------------------------------------------------------ | -| Asset application tests using fabricated `.car` bytes and injected utility JSON | Remove two plumbing cases | `verify:asset-catalog`: real actool → assetutil → CLI and stdio MCP, including digest, full metadata, pagination and invalid input | -| Keyed archive positive binary/XML objects assembled with the same plist library as the reader | Remove two synthetic success cases | Captured Foundation XML plus reviewed complete object/reference graph golden; real Foundation binary/XML → CLI and stdio MCP | -| Generic contract schema/annotation/example loops and output-map inventory comparisons | Consolidate into one SDK wire test | Assert exported schemas, guidance, effect annotations and valid examples on the actual advertised MCP catalog | -| Ghidra function operation enum membership | Remove | Canonical catalog, provider handshake and actual operation roundtrips establish admission; malformed provider results remain covered | -| Asset page assertion pinning `10_001` | Remove | Invalid-page rejection through real MCP; malformed utility data still covered | -| Suggested prompt tool positions | Remove incidental ordering assertions | Keep authority, evidence, untrusted input and preparation constraints | -| Package “acceptance” assertions pinning CI matrix/shard values and fake launcher command arrays | Remove configuration-shaped cases | Actual package/install verifier remains; retain a compact release verification-before-publish and registry-canary static guard | -| Package failure classifier under acceptance | Move to package evaluation | Preserve all malformed/unrelated-failure rejection cases; do not call them E2E | -| Native value graph checks conditional on success | Strengthen | Require success before pagination/ambiguity assertions; unexpected errors can no longer pass | -| Native UI verifier treating permission denial as a positive pass | Separate outcomes | Default lane requires real capture/action success; opt-in permissions lane explicitly reports no positive E2E proof | - -Ten previous test cases were removed or consolidated; three replacement -Vitest cases cover the captured goldens and release guard. The two real artifact -E2E scripts add product-level evidence outside the deterministic Vitest count. - -## Exhaustive follow-up - -The follow-up removes or consolidates another 31 test declarations, including -three redundant test files. Parameterized declarations and executed cases have -different counts; the figures above count source declarations. It also repairs -assertions that could pass without exercising their stated behavior. - -| Area | Cleanup | Stronger retained evidence | -| ---------------------------- | ----------------------------------------------------------------------------------------------------------------------------- | ----------------------------------------------------------------------------------------------------------------- | -| Configuration | Merge duplicate target/loader tests and preserve custom launcher, explicit provider and actionable error diagnostics | Independent malformed inputs; each capability flag is rejected separately | -| Browser scenarios | Remove fake launch success and expression-selected artifact success | Real Chromium scenarios, actual artifacts, CLI capture and owned/external cleanup in `verify:browser` | -| CLI policy | Remove duplicate confirmation helper cases | Compiled CLI proves project files stay unchanged until `--yes`; distinct interactive choices remain | -| Provider capabilities | Remove numerical catalog freezes; consolidate Hopper byte-read discovery | Exact canonical capability identities, admission, mutation effects and detached descriptors | -| Evidence ledger | Merge reordered-key checks into serialized bundle import; remove one-record quota claims | Atomic import, conflicts, residual revisions, actual large collections and byte regressions | -| MCP batching and concurrency | Replace maximum concurrency and empty indistinguishable responses | Every distinct ordered batch result; concurrently gated requests complete in reverse order with matching payloads | -| MCP inventories and graphs | Remove traversal/inventory call order and redundant catalog/prose checks | Exact complete cyclic graph, full inline inventories, canonical SDK catalog and all-handler roundtrips | -| Authority and lifecycle | Attempt the refused Ghidra mutation; require successful setup, call and switch; close twice | Typed refusal with zero mutation dispatch, restored target calls and exact owned-resource cleanup | -| Process and provider output | Remove incidental environment read and command option order/defaults | PID/ownership validation, exact disabled-policy failure and complete decoded response above 10 MiB | -| Conformance and CI guards | Merge identical replay runs; remove duplicate missing-row fault, trivial hash/self equality and incidental worker/step counts | Distinct faulty evidence, digest order, actual test discovery, file existence and release/runner authority | -| Pure schemas | Remove fabricated comparison-dimension injection and impossible long boundary success | Real artifact comparison and SDK coverage evaluation with returned boundary identity | - -The removed long boundary selector test only parsed a 1,000-character request; -workspace boundary identities admit at most 200 characters, so that positive -could never represent a successful evaluation. The actual MCP evaluation stays -covered. One unused ledger serialization helper was also retired after removing -its only test consumer; no CLI or MCP contract changed. - -No further immediately safe pruning was identified in the final review. Retained -replacement candidates below are coverage gaps, rather than permission to delete -unique tests. Test-only domain scaffolding also stays pending a separate decision -about the corresponding source behavior; deleting those tests alone would leave -unverified code and fail unused-export checks. - -## Stronger replacements from the closing pass - -Three more synthetic success cases were removed. Native value composition now -runs through the production CLI and a separate stdio MCP subprocess with real -Ghidra, using a source-built native fixture. It checks argument/parameter/return -relationships, the fixture's global operand, authenticated Evidence, upstream -and workflow analysis profiles, identical CLI/MCP graphs, discovery availability -and successful MCP session closure. Ambiguity, cancellation, target changes, -missing flow and bounded-work regressions remain in the focused suite. - -That replacement found missing CLI workflow routing for `trace_native_values` -and missing workflow profile classification. The routing and discovery now use -the composed operation's actual provider prerequisites; workflow Evidence commits -to the upstream Ghidra profile. - -The lone hand-built client-document equality test is replaced by a real TOML -filesystem transaction: reversed environment keys leave the original bytes -unchanged and create no backup. A real ZIP entry with an application root longer -than 5,000 characters replaces injected schema-only positive data. Inventory -completeness tests partition actual scans into authenticated partial observations -and require exact assembled results and every source citation, including more -than 100 citations. These are consumer merge tests, not provider pagination -claims. Replay aggregation now covers all four statuses and retains actual runner -diagnostics; it does not claim to execute a real sandbox. Retained pixel-metric -fixtures now write correct PNG chunk checksums. - -The all-zero screenshot buffer is replaced by a real Chromium PNG of 9,467,940 -bytes, transported intact through CLI and stdio MCP and decoded by the production -pixel comparator. The verifier configures the SDK client's receive buffer because -its 10 MiB default is smaller than the inline JSON response. This does not claim -large comparison-request transport coverage. A real SDK initialize request, -captured during a successful production stdio handshake, replaces the JSON-RPC -decoder's field-count-only oracle with exact field/value expectations. - -The closing pass also removed assertions tied to internal call order, defaults, -private cache sizes, duplicated polling and fabricated schema extensions. Unique -malformed-input, security-capacity and cleanup regressions remain. - -## Real process regression found - -The full suite exposed a renderer ordering race. Publishing a terminal output -journal entry could synchronously trigger a reactive resize before the preceding -output was queued for rendering. Queue that output first. The existing real -source-owned interactive PTY test now also requires the first resized frame to -contain the output that triggered the resize, preserving the original event -order rather than sorting or changing timestamps to conceal it. - -## Coverage kept - -Malformed archives, broken and cyclic references, unsupported metadata pointers, -relative Objective-C method layouts, ambiguous call candidates, digest mismatch, -cancellation, host permission denial, bounded output and owned-resource cleanup -remain covered. These cases provide evidence beyond a successful real-tool run. -The hand-assembled relative method fixture stays until compiler-produced -relative-list fixtures establish equivalent assertions. - -## Remaining priorities - -1. Extend the real Ghidra CLI/stdio MCP journey from value tracing to dispatch - and UI handlers before pruning their hand-authored composition success cases. - Native value tracing now establishes full transport parity. -2. Real binary dispatch and nib → full product transports, with normalized - semantic goldens. The compiler-backed readers already run in the new Apple - CI lane; synthetic pointer/layout rejection tests remain useful. -3. An approved interactive macOS runner for native UI capture/actions, including - stdio MCP scenarios. Hosted runners without permissions cannot supply this - proof. No arbitrary application's windows should be opened for this lane. -4. Real macOS/Linux Ghidra CI runners with bring-your-own installations. This - change adds Apple artifact CI without installing Java or Ghidra. -5. Classify fake-runner replay/application acceptance accurately and replace - repeated happy paths only after real sandbox journeys assert the same claims. - The replay workflow's allow-unavailable mode must not imply executed replay. -6. Replace repeated-array schema ceiling tests with real producer/transport - regressions before deleting them. Existing integration tests do not yet - establish their large-input guarantees. - -Some static guards protect irreversible release authority rather than product -behavior. Keep these few guards explicit and separate from E2E evidence. - -## Verification after the expanded audit - -- `npm run check:pr`: 351 files passed; 1,794 tests passed and two skipped. - Typecheck, lint, formatting, unused-code and generated-document gates passed. -- `npm run check:fast`: passed; 70 lint warnings and zero errors. -- `npm run verify:browser`: actual Chromium observation and scenario CLI routes, - retained artifacts, owned/external lifecycle checks and large CLI/stdio MCP PNG - capture passed. -- `npm run verify:ghidra`: macOS ARM64 with Ghidra 12.1.4/JDK 21 passed, including - real native value tracing through CLI and stdio MCP: 171 nodes, 298 edges and - six decompilations. -- `npm run verify:asset-catalog`: real actool/assetutil → CLI and stdio MCP, - complete records, pagination and malformed input checks passed. -- `npm run verify:keyed-archive`: real Foundation binary/XML → CLI and stdio MCP, - full graph golden, shared/cyclic identity and malformed scope checks passed. - -- `npm run verify:fixtures`: seven source-built host fixtures, source/artifact - digests and exhaustive inventory checks passed. - -No Hopper windows were opened for this audit. The closing pass runs real Ghidra -on macOS ARM64; earlier Hopper evidence remains separate. No new hosted CI success -is claimed. diff --git a/docs/testing.md b/docs/testing.md index 206729722..35eb7743b 100644 --- a/docs/testing.md +++ b/docs/testing.md @@ -106,9 +106,6 @@ It uses an installed browser and accepts an optional installed REA entrypoint. The complete `verify:browser` lane also exercises passive script export through both public adapters. See [website script export](website-script-export.md). -The [test suite audit](test-suite-audit.md) records the pruning decisions, -replacement evidence and remaining priorities. - ## Real-toolchain verification lanes Each real-toolchain command must require only the host tools needed to prove diff --git a/src/domain/validationTaxonomy.test.ts b/src/domain/validationTaxonomy.test.ts index 4407f8097..97d273cc4 100644 --- a/src/domain/validationTaxonomy.test.ts +++ b/src/domain/validationTaxonomy.test.ts @@ -4,43 +4,8 @@ import { parseArtifactInventoryEvidence, tryAssembleInventorySet, } from "./artifactInventoryEvidence.js"; -import { EvidenceReferenceError } from "./evidenceErrors.js"; -import type { ReferenceSourceReaderError } from "../reference/ReferenceSourceReaderTypes.js"; - -/** - * The three validation taxonomies and their disjoint codes: - * - Filesystem reader: cancelled | invalid-root | io | unsupported - * (per-entry: cancelled | changed | io | symlink | unsupported). - * - Session evidence references: missing | wrong_operation | - * wrong_predicate | identity_mismatch. - * - Inventory assembly: TypeError strings, migrating to Result leaves - * starting with the empty-pages code below. - */ -const readerCodes: readonly ReferenceSourceReaderError["code"][] = [ - "cancelled", - "invalid-root", - "io", - "unsupported", -]; - -const referenceReasons = ( - [ - "missing", - "wrong_operation", - "wrong_predicate", - "identity_mismatch", - ] as const -).map( - (reason) => - new EvidenceReferenceError("ev_test", reason, "expected", null).reason, -); - -describe("validation taxonomy mapping", () => { - it("keeps session reference reasons disjoint from reader codes", () => { - for (const reason of referenceReasons) - expect(readerCodes).not.toContain(reason); - }); +describe("inventory validation failure channels", () => { it("returns the empty-pages leaf as Result without throwing", () => { const result = tryAssembleInventorySet([]); expect(result).toEqual({ diff --git a/tests/boundary/browser/browserProcessStartup.test.ts b/tests/boundary/browser/browserProcessStartup.test.ts index 093d02076..91c4f58e9 100644 --- a/tests/boundary/browser/browserProcessStartup.test.ts +++ b/tests/boundary/browser/browserProcessStartup.test.ts @@ -1,29 +1,23 @@ -import { spawn } from "node:child_process"; -import { rm } from "node:fs/promises"; import { join } from "node:path"; -import { afterEach, describe, expect, it } from "vitest"; +import { describe, expect } from "vitest"; import { createTestTempDirectory } from "../../fixtures/temporaryDirectory.js"; +import { + processTest as it, + waitForExit, +} from "../../support/process/processFixture.js"; import { BrowserStartupError, waitForBrowserDevtoolsPort, } from "../../../src/browser/BrowserProcessStartup.js"; -const roots: string[] = []; - -afterEach(async () => { - await Promise.all( - roots.splice(0).map((root) => rm(root, { recursive: true, force: true })), - ); -}); - describe("browser process startup", () => { - it("returns a delayed valid DevToolsActivePort", async () => { + it("returns a delayed valid DevToolsActivePort", async ({ processes }) => { const root = await temporaryRoot(); const portPath = join(root, "DevToolsActivePort"); - const child = spawn(process.execPath, [ + const child = processes.spawn(process.execPath, [ "-e", `setTimeout(() => require("node:fs").writeFileSync(${JSON.stringify(portPath)}, "43117\\n/browser"), 20); setTimeout(() => {}, 1000)`, ]); @@ -40,12 +34,15 @@ describe("browser process startup", () => { ).resolves.toBe(43_117); } finally { child.kill("SIGKILL"); + expect(await waitForExit(child, 5_000)).toBe(true); } }); - it("classifies signal termination instead of timing out", async () => { + it("classifies signal termination instead of timing out", async ({ + processes, + }) => { const root = await temporaryRoot(); - const child = spawn(process.execPath, [ + const child = processes.spawn(process.execPath, [ "-e", "process.kill(process.pid, 'SIGTERM')", ]); @@ -68,9 +65,12 @@ describe("browser process startup", () => { }); }); - it("reports bounded timeout diagnostics", async () => { + it("reports bounded timeout diagnostics", async ({ processes }) => { const root = await temporaryRoot(); - const child = spawn(process.execPath, ["-e", "setTimeout(() => {}, 1000)"]); + const child = processes.spawn(process.execPath, [ + "-e", + "setTimeout(() => {}, 1000)", + ]); try { const failure = await waitForBrowserDevtoolsPort({ child, @@ -90,12 +90,11 @@ describe("browser process startup", () => { expect(String(failure)).toContain("stderr="); } finally { child.kill("SIGKILL"); + expect(await waitForExit(child, 5_000)).toBe(true); } }); }); const temporaryRoot = async (): Promise => { - const root = await createTestTempDirectory("rea-browser-startup-"); - roots.push(root); - return root; + return createTestTempDirectory("rea-browser-startup-"); }; diff --git a/tests/boundary/browser/cdpBrowserProvider.lifecycle_failures.test.ts b/tests/boundary/browser/cdpBrowserProvider.lifecycle_failures.test.ts index a3bf58690..d559b9088 100644 --- a/tests/boundary/browser/cdpBrowserProvider.lifecycle_failures.test.ts +++ b/tests/boundary/browser/cdpBrowserProvider.lifecycle_failures.test.ts @@ -391,11 +391,12 @@ describeBrowser("CdpBrowserProvider: lifecycle failures 4", () => { ); if (!committed.ok) throw committed.error; expect(committed.value.target.origin).toBe(transitioning.allowedOrigin); - expect( - transitioning.commands.filter( - ({ method }) => method === "Page.getFrameTree", - ), - ).toHaveLength(5); + expect(committed.value.frames).toContainEqual( + expect.objectContaining({ + frame_id: "frame-main", + origin: transitioning.allowedOrigin, + }), + ); const navigated = await startFakeCdpBrowser({ attachedFrameUrl: "https://unapproved.example.test/after-attach", diff --git a/tests/boundary/browser/webSourceMapResponseCleanup.test.ts b/tests/boundary/browser/webSourceMapResponseCleanup.test.ts index a07e2c78f..88fdafbcb 100644 --- a/tests/boundary/browser/webSourceMapResponseCleanup.test.ts +++ b/tests/boundary/browser/webSourceMapResponseCleanup.test.ts @@ -1,5 +1,5 @@ import { createServer } from "node:http"; -import { describe, expect, it } from "vitest"; +import { describe, expect, it, onTestFinished } from "vitest"; import { fetchWebSourceMaps } from "../../../src/browser/WebSourceMapFetcher.js"; import { analyzeWebBundleInputSchema } from "../../../src/domain/webBundleAnalysis.js"; @@ -34,6 +34,12 @@ describe("source-map discarded HTTP response ownership", () => { notifyClosed?.(); }); }); + const closeServer = async (): Promise => { + server.closeAllConnections(); + if (server.listening) + await new Promise((resolve) => server.close(() => resolve())); + }; + onTestFinished(closeServer); try { await new Promise((resolve) => server.listen(0, "127.0.0.1", resolve), @@ -62,22 +68,10 @@ describe("source-map discarded HTTP response ownership", () => { expect(result.items[0]?.status).toBe( status === 302 ? "included" : "fetch_failed", ); - let timer: ReturnType | undefined; - try { - await Promise.race([ - closed, - new Promise((resolve) => { - timer = setTimeout(resolve, 500); - }), - ]); - expect(didClose).toBe(true); - } finally { - clearTimeout(timer); - } + await closed; + expect(didClose).toBe(true); } finally { - server.closeAllConnections(); - if (server.listening) - await new Promise((resolve) => server.close(() => resolve())); + await closeServer(); } }, ); diff --git a/src/application/InterfaceBuilderAnalysis.test.ts b/tests/boundary/filesystem/interfaceBuilderBundle.test.ts similarity index 86% rename from src/application/InterfaceBuilderAnalysis.test.ts rename to tests/boundary/filesystem/interfaceBuilderBundle.test.ts index eb02c1bf4..77c8ef7aa 100644 --- a/src/application/InterfaceBuilderAnalysis.test.ts +++ b/tests/boundary/filesystem/interfaceBuilderBundle.test.ts @@ -1,26 +1,20 @@ import { existsSync } from "node:fs"; -import { execFileSync } from "node:child_process"; -import { mkdtemp, mkdir, rm, writeFile } from "node:fs/promises"; -import { tmpdir } from "node:os"; +import { execFile } from "node:child_process"; +import { mkdir, writeFile } from "node:fs/promises"; import { join } from "node:path"; +import { promisify } from "node:util"; import { buildBinary } from "plist"; -import { afterEach, describe, expect, it } from "vitest"; +import { describe, expect, it } from "vitest"; -import { analyzeInterfaceBuilderBundle } from "./InterfaceBuilderAnalysis.js"; +import { analyzeInterfaceBuilderBundle } from "../../../src/application/InterfaceBuilderAnalysis.js"; +import { createTestTempDirectory } from "../../fixtures/temporaryDirectory.js"; -const roots: string[] = []; - -afterEach(async () => { - await Promise.all( - roots.splice(0).map((root) => rm(root, { recursive: true, force: true })), - ); -}); +const compile = promisify(execFile); describe("compiled Interface Builder bundle reader", () => { it("reads nib plist archives and reports provenance", async () => { - const root = await mkdtemp(join(tmpdir(), "rea-ib-test-")); - roots.push(root); + const root = await createTestTempDirectory("rea-ib-test-"); const bundle = join(root, "Example.app"); const nib = join( bundle, @@ -78,8 +72,7 @@ describe("compiled Interface Builder bundle reader", () => { }); it("honors cancellation during directory traversal", async () => { - const root = await mkdtemp(join(tmpdir(), "rea-ib-test-")); - roots.push(root); + const root = await createTestTempDirectory("rea-ib-test-"); const controller = new AbortController(); controller.abort(); await expect( @@ -94,8 +87,7 @@ describe("compiled Interface Builder bundle reader", () => { describe("bounded Interface Builder archive decoding", () => { it("counts malformed archives against the document limit", async () => { - const root = await mkdtemp(join(tmpdir(), "rea-ib-test-")); - roots.push(root); + const root = await createTestTempDirectory("rea-ib-test-"); const bundle = join(root, "Example.app"); const resources = join(bundle, "Contents", "Resources"); await mkdir(resources, { recursive: true }); @@ -122,8 +114,7 @@ describe("bounded Interface Builder archive decoding", () => { it.skipIf(process.platform !== "darwin" || !existsSync("/usr/bin/ibtool"))( "decodes an Xcode-compiled storyboard NIB and recovers its UI routes", async () => { - const root = await mkdtemp(join(tmpdir(), "rea-ib-compiled-test-")); - roots.push(root); + const root = await createTestTempDirectory("rea-ib-compiled-test-"); const bundle = join(root, "Example.app"); const resources = join(bundle, "Contents", "Resources"); const source = join( @@ -134,7 +125,7 @@ describe("bounded Interface Builder archive decoding", () => { "MacFixture.storyboard", ); await mkdir(resources, { recursive: true }); - execFileSync("/usr/bin/ibtool", [ + await compile("/usr/bin/ibtool", [ "--compile", join(resources, "MacFixture.storyboardc"), source, diff --git a/src/application/NibNativeHierarchy.test.ts b/tests/boundary/filesystem/nibNativeHierarchy.test.ts similarity index 93% rename from src/application/NibNativeHierarchy.test.ts rename to tests/boundary/filesystem/nibNativeHierarchy.test.ts index 3d8bc42ad..83b025b79 100644 --- a/src/application/NibNativeHierarchy.test.ts +++ b/tests/boundary/filesystem/nibNativeHierarchy.test.ts @@ -1,12 +1,15 @@ -import { execFileSync } from "node:child_process"; +import { execFile } from "node:child_process"; import { mkdir, readFile, writeFile } from "node:fs/promises"; import { join } from "node:path"; +import { promisify } from "node:util"; import { describe, expect, it } from "vitest"; -import { createTestTempDirectory } from "../../tests/fixtures/temporaryDirectory.js"; -import { decodeNibArchive } from "../artifacts/NibArchive.js"; -import { analyzeInterfaceBuilderBundle } from "./InterfaceBuilderAnalysis.js"; +import { createTestTempDirectory } from "../../fixtures/temporaryDirectory.js"; +import { decodeNibArchive } from "../../../src/artifacts/NibArchive.js"; +import { analyzeInterfaceBuilderBundle } from "../../../src/application/InterfaceBuilderAnalysis.js"; + +const compile = promisify(execFile); const compileNestedViews = async (depth: number, withWindow = false) => { const root = await createTestTempDirectory("rea-nib-hierarchy-"); @@ -34,7 +37,7 @@ const compileNestedViews = async (depth: number, withWindow = false) => { .replace("\n ", "\n "); await writeFile(xib, xibSource); const nib = join(resources, "Main.nib"); - execFileSync("/usr/bin/xcrun", [ + await compile("/usr/bin/xcrun", [ "ibtool", "--errors", "--warnings", diff --git a/tests/boundary/filesystem/setupConfig.test.ts b/tests/boundary/filesystem/setupConfig.test.ts index 4019e5cf2..2996f630c 100644 --- a/tests/boundary/filesystem/setupConfig.test.ts +++ b/tests/boundary/filesystem/setupConfig.test.ts @@ -11,24 +11,28 @@ import { PRODUCT_IDENTITY } from "../../../src/identity.js"; const pinnedPackage = PRODUCT_IDENTITY.registrationPackageSpecifier; describe("JSON client configuration transaction", () => { - it("rejects a symlink target not owned by the current user", async () => { - if (typeof process.getuid !== "function") return; - const currentUid = process.getuid(); - let statCalls = 0; - expect( - await resolveClientConfigTransactionPath("/config", { - lstat: () => { - statCalls += 1; - return Promise.resolve({ - uid: statCalls === 1 ? currentUid : currentUid + 1, - isFile: () => statCalls > 1, - isSymbolicLink: () => statCalls === 1, - }); - }, - realpath: () => Promise.resolve("/target"), - }), - ).toBeUndefined(); - }); + it.skipIf(typeof process.getuid !== "function")( + "rejects a symlink target not owned by the current user", + async () => { + const currentUid = process.getuid?.(); + if (currentUid === undefined) + throw new Error("UID support is required by this test"); + let statCalls = 0; + expect( + await resolveClientConfigTransactionPath("/config", { + lstat: () => { + statCalls += 1; + return Promise.resolve({ + uid: statCalls === 1 ? currentUid : currentUid + 1, + isFile: () => statCalls > 1, + isSymbolicLink: () => statCalls === 1, + }); + }, + realpath: () => Promise.resolve("/target"), + }), + ).toBeUndefined(); + }, + ); it("preserves existing keys, creates a backup, and reads back the MCP entry", async () => { const directory = await createTestTempDirectory("rea-setup-"); diff --git a/tests/boundary/mcp/mcpDoctor.test.ts b/tests/boundary/mcp/mcpDoctor.test.ts index 4d725c6ab..9858e4ff2 100644 --- a/tests/boundary/mcp/mcpDoctor.test.ts +++ b/tests/boundary/mcp/mcpDoctor.test.ts @@ -1,7 +1,7 @@ -import { readFile, rm } from "node:fs/promises"; +import { readFile } from "node:fs/promises"; import { join, resolve } from "node:path"; -import { afterEach, describe, expect, it } from "vitest"; +import { afterEach, describe, expect, it, vi } from "vitest"; import { createTestTempDirectory } from "../../fixtures/temporaryDirectory.js"; @@ -11,15 +11,7 @@ import { } from "../../../src/mcpDoctor.js"; import { CATALOG_IDENTITY } from "../../../src/catalogIdentity.js"; -const temporary: string[] = []; - -afterEach(async () => { - await Promise.all( - temporary - .splice(0) - .map((path) => rm(path, { recursive: true, force: true })), - ); -}); +afterEach(() => vi.useRealTimers()); describe("production MCP doctor", () => { it("connects to the production stdio child and verifies the canonical catalog", async () => { @@ -78,17 +70,30 @@ describe("production MCP doctor", () => { it("kills a child that misses the absolute startup deadline", async () => { const root = await createTestTempDirectory("rea-mcp-doctor-"); - temporary.push(root); const pidPath = join(root, "pid"); - const result = await runProductionMcpDoctor({ + // Keep filesystem/process I/O real. Advance only the deadline timers, + // after the fixture proves it started and published its PID. + vi.useFakeTimers({ toFake: ["setTimeout", "clearTimeout"] }); + const pending = runProductionMcpDoctor({ command: process.execPath, args: [resolve("tests/fixtures/mcpDoctorHang.mjs")], cwd: process.cwd(), environment: { ...process.env, REA_MCP_DOCTOR_PID_PATH: pidPath }, - deadlineMs: 250, + deadlineMs: 60_000, }); + const pid = await vi.waitFor( + async () => { + const value = Number.parseInt(await readFile(pidPath, "utf8"), 10); + expect(Number.isSafeInteger(value) && value > 0).toBe(true); + return value; + }, + { timeout: 10_000 }, + ); + await vi.advanceTimersByTimeAsync(60_000); + const result = await vi.waitFor(() => pending, { timeout: 10_000 }); + vi.useRealTimers(); expect(result.healthy).toBe(false); - const pid = Number.parseInt(await readFile(pidPath, "utf8"), 10); + expect(result.checks[0]?.detail).toMatch(/deadline|abort|timed/iu); await expect(waitForExit(pid)).resolves.toBeUndefined(); }, 10_000); }); @@ -98,11 +103,7 @@ const waitForExit = async (pid: number): Promise => { try { process.kill(pid, 0); } catch (cause: unknown) { - if ( - cause instanceof Error && - "code" in cause && - (cause as NodeJS.ErrnoException).code === "ESRCH" - ) + if (cause instanceof Error && "code" in cause && cause.code === "ESRCH") return; throw cause; } diff --git a/tests/boundary/providers/ghidra/ghidraClient.test.ts b/tests/boundary/providers/ghidra/ghidraClient.test.ts index a74529768..fa77d8c38 100644 --- a/tests/boundary/providers/ghidra/ghidraClient.test.ts +++ b/tests/boundary/providers/ghidra/ghidraClient.test.ts @@ -5,7 +5,7 @@ import { access, mkdir } from "node:fs/promises"; import { join } from "node:path"; import { fileURLToPath } from "node:url"; -import { afterEach, describe, expect, it } from "vitest"; +import { afterEach, describe, expect, it, vi } from "vitest"; import pino from "pino"; import type { Logger } from "../../../../src/logger.js"; @@ -20,6 +20,7 @@ import type { } from "../../../../src/ghidra/GhidraLauncher.js"; import type { GhidraTransportKind } from "../../../../src/ghidra/GhidraTransport.js"; import { GHIDRA_SESSION_CAPABILITIES } from "../../../../src/ghidra/GhidraSessionValues.js"; +import { waitForExit as waitForChildExit } from "../../../support/process/processFixture.js"; const fixturePath = fileURLToPath( new URL("../../../fixtures/fakeGhidra.mjs", import.meta.url), @@ -50,10 +51,15 @@ type FixtureMode = | "exit"; class FixtureLauncher implements GhidraLauncher { + #notifyStarted: ((child: ChildProcess) => void) | undefined; + readonly started = new Promise((resolve) => { + this.#notifyStarted = resolve; + }); readonly runtimeRoots: string[] = []; readonly endpointPaths: string[] = []; readonly tokens: string[] = []; readonly processes: ChildProcess[] = []; + readonly requests: string[] = []; constructor(readonly mode: FixtureMode = "success") {} @@ -64,7 +70,7 @@ class FixtureLauncher implements GhidraLauncher { const projectRoot = join(session.runtimeRoot, "project"); await mkdir(projectRoot, { recursive: true }); const process_ = spawn(process.execPath, [fixturePath], { - stdio: ["ignore", "pipe", "pipe"], + stdio: ["ignore", "pipe", "pipe", "ipc"], env: { ...process.env, REA_GHIDRA_FIXTURE_CONFIG: JSON.stringify({ @@ -79,7 +85,17 @@ class FixtureLauncher implements GhidraLauncher { }), }, }); + process_.on("message", (value: unknown) => { + if ( + typeof value === "object" && + value !== null && + "method" in value && + typeof value.method === "string" + ) + this.requests.push(value.method); + }); this.processes.push(process_); + process_.once("spawn", () => this.#notifyStarted?.(process_)); return ok({ process: process_, ownsProcessLifetime: true, @@ -120,6 +136,7 @@ const clientFor = ( }; afterEach(async () => { + vi.useRealTimers(); await Promise.all(clients.splice(0).map((client) => client.close())); }); @@ -246,10 +263,15 @@ describe("GhidraClient", () => { describe("GhidraClient startup lifecycle", () => { it("applies one startup deadline and removes the private runtime", async () => { + vi.useFakeTimers({ toFake: ["setTimeout", "clearTimeout", "Date"] }); const launcher = new FixtureLauncher("silent"); const client = clientFor(launcher, { startupTimeoutMs: 30 }); - const result = await client.start(); + const pending = client.start(); + const child = await launcher.started; + await vi.advanceTimersByTimeAsync(30); + const result = await vi.waitFor(() => pending, { timeout: 10_000 }); + vi.useRealTimers(); expect(result).toMatchObject({ ok: false, @@ -259,7 +281,7 @@ describe("GhidraClient startup lifecycle", () => { code: "ENOENT", }); await client.close(); - await expect(waitForExit(launcher.processes[0])).resolves.toBe(true); + await expect(waitForExit(child)).resolves.toBe(true); }); it("cancels startup promptly and leaves no project or process", async () => { @@ -267,7 +289,8 @@ describe("GhidraClient startup lifecycle", () => { const client = clientFor(launcher, { startupTimeoutMs: 10_000 }); const controller = new AbortController(); const pending = client.start(controller.signal); - setTimeout(() => controller.abort(), 10); + const process_ = await launcher.started; + controller.abort(); const result = await pending; @@ -276,9 +299,7 @@ describe("GhidraClient startup lifecycle", () => { code: "ENOENT", }); await client.close(); - const process_ = launcher.processes[0]; - if (process_ !== undefined) - await expect(waitForExit(process_)).resolves.toBe(true); + await expect(waitForExit(process_)).resolves.toBe(true); }); }); @@ -305,7 +326,9 @@ describe("GhidraClient established requests", () => { { document: null, procedure: "fixture_main" }, { signal: activeController.signal }, ); - await wait(5); + await vi.waitFor(() => + expect(launcher.requests).toContain("procedure_pseudo_code"), + ); const queuedController = new AbortController(); const queued = client.callTool( "procedure_info", @@ -322,7 +345,7 @@ describe("GhidraClient established requests", () => { document: null, procedure: "fixture_main", }); - await wait(5); + expect(launcher.requests).not.toContain("procedure_info"); const process_ = launcher.processes[0]; activeController.abort(); await expect(active).resolves.toMatchObject({ @@ -404,15 +427,9 @@ const exited = (process_: ChildProcess | undefined): boolean => const waitForExit = async ( process_: ChildProcess | undefined, ): Promise => { - for (let attempt = 0; attempt < 1_000 && !exited(process_); attempt += 1) { - await wait(10); - } - return exited(process_); + return process_ !== undefined && waitForChildExit(process_, 10_000); }; -const wait = (milliseconds: number): Promise => - new Promise((resolve) => setTimeout(resolve, milliseconds)); - describe("GhidraClient rejected requests", () => { it("redacts authentication after failed startup cleanup and retry", async () => { const fixture = new FixtureLauncher(); diff --git a/tests/boundary/providers/ghidra/ghidraLauncher.test.ts b/tests/boundary/providers/ghidra/ghidraLauncher.test.ts index 6a59ed0fa..6c44f4393 100644 --- a/tests/boundary/providers/ghidra/ghidraLauncher.test.ts +++ b/tests/boundary/providers/ghidra/ghidraLauncher.test.ts @@ -1,8 +1,9 @@ -import { access, chmod, readFile, rm, stat } from "node:fs/promises"; +import { access, chmod, readFile, stat } from "node:fs/promises"; import { join } from "node:path"; import { fileURLToPath } from "node:url"; import { afterEach, beforeAll, describe, expect, it, vi } from "vitest"; +import { z } from "zod"; import { createTestTempDirectory } from "../../../fixtures/temporaryDirectory.js"; import { createGhidraTestRuntime } from "../../../fixtures/ghidraRuntime.js"; @@ -22,9 +23,26 @@ const fixturePath = fileURLToPath( import.meta.url, ), ); -const roots: string[] = []; const runtimes: PrivateRuntimeRoot[] = []; +const launchCaptureSchema = z.object({ + arguments: z.array(z.string()), + environment: z.record(z.string(), z.string()), + descriptor_mode: z.number().int(), + descriptor_has_token: z.boolean(), +}); + +const expectOptions = ( + arguments_: readonly string[], + options: readonly (readonly [string, string])[], +): void => { + for (const [flag, value] of options) { + const index = arguments_.indexOf(flag); + expect(index, flag).toBeGreaterThanOrEqual(0); + expect(arguments_[index + 1], flag).toBe(value); + } +}; + const expectIsolatedEnvironment = ( environment: Record, runtimeRoot: string, @@ -68,9 +86,6 @@ beforeAll(async () => { afterEach(async () => { vi.unstubAllEnvs(); await Promise.all(runtimes.splice(0).map((runtime) => runtime.close())); - await Promise.all( - roots.splice(0).map((path) => rm(path, { recursive: true, force: true })), - ); }); describe("Ghidra COM loader", () => { @@ -84,15 +99,11 @@ describe("Ghidra COM loader", () => { scriptLogPath: "/tmp/script.log", dosCom: true, }); - expect(arguments_.slice(4, 12)).toEqual([ - "-loader", - "BinaryLoader", - "-loader-baseAddr", - "1000:0100", - "-processor", - "x86:LE:16:Real Mode", - "-cspec", - "default", + expectOptions(arguments_, [ + ["-loader", "BinaryLoader"], + ["-loader-baseAddr", "1000:0100"], + ["-processor", "x86:LE:16:Real Mode"], + ["-cspec", "default"], ]); const prepare = arguments_.indexOf("-preScript"); expect(arguments_[prepare + 1]).toBe( @@ -115,44 +126,36 @@ describe("Ghidra headless launcher", () => { scriptLogPath: "/tmp/script.log", dosMz: true, }); - expect(arguments_.slice(4, 10)).toEqual([ - "-loader", - "MzLoader", - "-processor", - "x86:LE:16:Real Mode", - "-cspec", - "default", + expectOptions(arguments_, [ + ["-loader", "MzLoader"], + ["-processor", "x86:LE:16:Real Mode"], + ["-cspec", "default"], ]); expect(arguments_).toContain("-readOnly"); expect(arguments_).toContain("-deleteProject"); }); - it("builds a read-only import in deterministic order", () => { - expect( - ghidraHeadlessArguments({ - projectRoot: "/tmp/project", - targetPath: "/tmp/target", - bridgeScriptPath: "/package/bridge/ReaGhidraBridge.java", - descriptorPath: "/tmp/session.json", - ghidraLogPath: "/tmp/ghidra.log", - scriptLogPath: "/tmp/script.log", - }), - ).toEqual([ - "/tmp/project", - "rea-project", - "-import", - "/tmp/target", - "-readOnly", - "-deleteProject", - "-log", - "/tmp/ghidra.log", - "-scriptlog", - "/tmp/script.log", - "-scriptPath", - "/package/bridge", - "-postScript", - "/package/bridge/ReaGhidraBridge.java", - "/tmp/session.json", + it("builds a read-only ephemeral import with its bridge descriptor", () => { + const arguments_ = ghidraHeadlessArguments({ + projectRoot: "/tmp/project", + targetPath: "/tmp/target", + bridgeScriptPath: "/package/bridge/ReaGhidraBridge.java", + descriptorPath: "/tmp/session.json", + ghidraLogPath: "/tmp/ghidra.log", + scriptLogPath: "/tmp/script.log", + }); + expect(arguments_.slice(0, 2)).toEqual(["/tmp/project", "rea-project"]); + expectOptions(arguments_, [ + ["-import", "/tmp/target"], + ["-log", "/tmp/ghidra.log"], + ["-scriptlog", "/tmp/script.log"], + ["-scriptPath", "/package/bridge"], + ["-postScript", "/package/bridge/ReaGhidraBridge.java"], ]); + expect(arguments_[arguments_.indexOf("-postScript") + 2]).toBe( + "/tmp/session.json", + ); + expect(arguments_).toContain("-readOnly"); + expect(arguments_).toContain("-deleteProject"); }); it("wraps the Windows batch launcher without enabling a Node shell", () => { @@ -203,7 +206,6 @@ describe("Ghidra headless launcher", () => { vi.stubEnv("JDK_JAVA_OPTIONS", "-XX:MaxRAMPercentage=99"); vi.stubEnv("_JAVA_OPTIONS", "-Xmx99G"); const parent = await createTestTempDirectory("rea-launcher-test-"); - roots.push(parent); const runtime = await createGhidraTestRuntime(parent); runtimes.push(runtime); const runtimeRoot = runtime.path; @@ -229,8 +231,10 @@ describe("Ghidra headless launcher", () => { expect(launched.ok).toBe(true); if (!launched.ok) return; const capturePath = join(runtimeRoot, "launch-capture.json"); - await waitFor(capturePath); - const capture = JSON.parse(await readFile(capturePath, "utf8")); + await vi.waitFor(() => access(`${capturePath}.ready`), { timeout: 10_000 }); + const capture = launchCaptureSchema.parse( + JSON.parse(await readFile(capturePath, "utf8")), + ); const encodedArguments = JSON.stringify(capture.arguments); const encodedEnvironment = JSON.stringify(capture.environment); expect(encodedArguments).not.toContain(token); @@ -255,15 +259,3 @@ describe("Ghidra headless launcher", () => { await expect(access(join(runtimeRoot, "project"))).resolves.toBeUndefined(); }); }); - -const waitFor = async (path: string): Promise => { - for (let attempt = 0; attempt < 100; attempt += 1) { - try { - await access(path); - return; - } catch { - await new Promise((resolve) => setTimeout(resolve, 10)); - } - } - throw new Error(`Timed out waiting for ${path}`); -}; diff --git a/tests/boundary/providers/hopper/hopperClient.cleanup.test.ts b/tests/boundary/providers/hopper/hopperClient.cleanup.test.ts index ae8f3cd99..eb9a10c4a 100644 --- a/tests/boundary/providers/hopper/hopperClient.cleanup.test.ts +++ b/tests/boundary/providers/hopper/hopperClient.cleanup.test.ts @@ -1,6 +1,5 @@ import { spawn, type ChildProcess } from "node:child_process"; import { access } from "node:fs/promises"; -import { fileURLToPath } from "node:url"; import { afterEach, describe, expect, it } from "vitest"; @@ -14,44 +13,11 @@ import { HopperClient } from "../../../../src/hopper/HopperClient.js"; import { cleanupOwnedProcessGroup } from "../../../../src/process/ProcessOwnership.js"; import { spawnOwnedProviderProcess } from "../../../../src/process/ProviderProcess.js"; -const fixturePath = fileURLToPath( - new URL("../../../fixtures/fakeHopper.mjs", import.meta.url), -); - -class FixtureLauncher implements BridgeLauncher { - socketPaths: string[] = []; - directories: string[] = []; - runIds: string[] = []; - processes: ChildProcess[] = []; - - constructor(readonly tokenOverride?: string) {} - - launch(session: BridgeSession) { - this.socketPaths.push(session.socketPath); - this.directories.push(session.directory); - this.runIds.push(session.runId); - const child = spawn( - process.execPath, - [ - fixturePath, - session.socketPath, - this.tokenOverride ?? session.token, - session.runId, - ], - { - stdio: ["ignore", "ignore", "pipe"], - }, - ); - this.processes.push(child); - return Promise.resolve( - ok({ - process: child, - ownsProcessLifetime: true, - shutdownMode: "bridge-request" as const, - }), - ); - } -} +import { + HopperFixtureLauncher as FixtureLauncher, + hopperFixturePath as fixturePath, + startHopperFixtureClient as startClient, +} from "./hopperClient.fixture.js"; class OwnedFixtureLauncher implements BridgeLauncher { ownership: @@ -131,18 +97,6 @@ class UnconfirmedOwnedFixtureLauncher implements BridgeLauncher { } const clients: HopperClient[] = []; -const startClient = async () => { - const client = new HopperClient({ - launcher: new FixtureLauncher(), - startupTimeoutMs: 1_000, - }); - clients.push(client); - await expect(client.start()).resolves.toEqual({ - ok: true, - value: { name: "REA Hopper bridge", version: "1.0.0" }, - }); - return client; -}; afterEach(async () => { await Promise.all(clients.splice(0).map((client) => client.close())); @@ -150,12 +104,15 @@ afterEach(async () => { describe("HopperClient cleanup", () => { it("waits for an operation reply without a fixed request deadline", async () => { - const client = await startClient(); - await expect( - client.callTool("echo", { value: "waited", delay: 150 }), - ).resolves.toEqual({ + const launcher = new FixtureLauncher(); + const client = await startClient(launcher); + const pending = client.callTool("echo", { value: "waited", gate: "reply" }); + const request = await launcher.waitForRequest("echo", "reply"); + expect(client.requestActivity()).toMatchObject({ operation: "echo" }); + await launcher.release(request); + await expect(pending).resolves.toEqual({ ok: true, - value: { value: "waited", delay: 150 }, + value: { value: "waited", gate: "reply" }, }); }); diff --git a/tests/boundary/providers/hopper/hopperClient.fixture.ts b/tests/boundary/providers/hopper/hopperClient.fixture.ts new file mode 100644 index 000000000..6a4e9f51d --- /dev/null +++ b/tests/boundary/providers/hopper/hopperClient.fixture.ts @@ -0,0 +1,112 @@ +import { spawn, type ChildProcess } from "node:child_process"; +import { fileURLToPath } from "node:url"; + +import { onTestFinished, vi } from "vitest"; +import { z } from "zod"; + +import { ok } from "../../../../src/domain/result.js"; +import type { + BridgeLauncher, + BridgeSession, +} from "../../../../src/hopper/BridgeLauncher.js"; +import { HopperClient } from "../../../../src/hopper/HopperClient.js"; + +export const hopperFixturePath = fileURLToPath( + new URL("../../../fixtures/fakeHopper.mjs", import.meta.url), +); + +const requestSchema = z.object({ + id: z.number().int(), + method: z.string(), + params: z.record(z.string(), z.unknown()).optional(), +}); +type FixtureRequest = z.infer; + +/** Real socket fixture with request acknowledgements and explicit response gates. */ +export class HopperFixtureLauncher implements BridgeLauncher { + readonly socketPaths: string[] = []; + readonly directories: string[] = []; + readonly runIds: string[] = []; + readonly processes: ChildProcess[] = []; + readonly requests: FixtureRequest[] = []; + + constructor(readonly tokenOverride?: string) {} + + launch(session: BridgeSession) { + this.socketPaths.push(session.socketPath); + this.directories.push(session.directory); + this.runIds.push(session.runId); + const child = spawn( + process.execPath, + [ + hopperFixturePath, + session.socketPath, + this.tokenOverride ?? session.token, + session.runId, + ], + { stdio: ["ignore", "ignore", "pipe", "ipc"] }, + ); + child.on("message", (value: unknown) => { + const request = requestSchema.safeParse(value); + if (request.success) this.requests.push(request.data); + }); + this.processes.push(child); + return Promise.resolve( + ok({ + process: child, + ownsProcessLifetime: true as const, + shutdownMode: "bridge-request" as const, + }), + ); + } + + /** Wait for acknowledgement that the fixture actually received a request. */ + waitForRequest(method: string, gate?: string): Promise { + return vi.waitFor( + () => { + const request = this.requests.find( + (value) => + value.method === method && + (gate === undefined || value.params?.gate === gate), + ); + if (request === undefined) + throw new Error(`Waiting for Hopper fixture request ${method}`); + return request; + }, + { timeout: 10_000 }, + ); + } + + /** Release a response held by the fixture's gate parameter. */ + release(request: FixtureRequest): Promise { + const child = this.processes.at(-1); + if (child === undefined) throw new Error("Hopper fixture was not launched"); + return new Promise((resolve, reject) => { + child.send({ release: request.id }, (error) => { + if (error !== null) reject(error); + else resolve(); + }); + }); + } +} + +/** Register owned client cleanup before any startup assertion can fail. */ +export const trackHopperClient = (client: HopperClient): HopperClient => { + onTestFinished(() => client.close()); + return client; +}; + +/** Start a real fixture session with test-scoped cleanup. */ +export const startHopperFixtureClient = async ( + launcher = new HopperFixtureLauncher(), +): Promise => { + const client = trackHopperClient( + new HopperClient({ + launcher, + startupTimeoutMs: 10_000, + }), + ); + const started = await client.start(); + if (!started.ok) throw started.error; + return client; +}; diff --git a/tests/boundary/providers/hopper/hopperClient.progress.test.ts b/tests/boundary/providers/hopper/hopperClient.progress.test.ts index 8089dca1e..b27d9f2ff 100644 --- a/tests/boundary/providers/hopper/hopperClient.progress.test.ts +++ b/tests/boundary/providers/hopper/hopperClient.progress.test.ts @@ -1,75 +1,14 @@ -import { spawn, type ChildProcess } from "node:child_process"; -import { fileURLToPath } from "node:url"; +import { describe, expect, it, vi } from "vitest"; -import { afterEach, describe, expect, it } from "vitest"; - -import { ok } from "../../../../src/domain/result.js"; -import type { - BridgeLauncher, - BridgeSession, -} from "../../../../src/hopper/BridgeLauncher.js"; -import { HopperClient } from "../../../../src/hopper/HopperClient.js"; - -const fixturePath = fileURLToPath( - new URL("../../../fixtures/fakeHopper.mjs", import.meta.url), -); - -class FixtureLauncher implements BridgeLauncher { - socketPaths: string[] = []; - directories: string[] = []; - runIds: string[] = []; - processes: ChildProcess[] = []; - - constructor(readonly tokenOverride?: string) {} - - launch(session: BridgeSession) { - this.socketPaths.push(session.socketPath); - this.directories.push(session.directory); - this.runIds.push(session.runId); - const child = spawn( - process.execPath, - [ - fixturePath, - session.socketPath, - this.tokenOverride ?? session.token, - session.runId, - ], - { - stdio: ["ignore", "ignore", "pipe"], - }, - ); - this.processes.push(child); - return Promise.resolve( - ok({ - process: child, - ownsProcessLifetime: true as const, - shutdownMode: "bridge-request" as const, - }), - ); - } -} - -const clients: HopperClient[] = []; -const startClient = async () => { - const client = new HopperClient({ - launcher: new FixtureLauncher(), - startupTimeoutMs: 1_000, - }); - clients.push(client); - await expect(client.start()).resolves.toEqual({ - ok: true, - value: { name: "REA Hopper bridge", version: "1.0.0" }, - }); - return client; -}; - -afterEach(async () => { - await Promise.all(clients.splice(0).map((client) => client.close())); -}); +import { + HopperFixtureLauncher, + startHopperFixtureClient as startClient, +} from "./hopperClient.fixture.js"; describe("HopperClient progress", () => { it("forwards correlated Python progress before the terminal response", async () => { - const client = await startClient(); + const launcher = new HopperFixtureLauncher(); + const client = await startClient(launcher); const updates: Array<{ readonly phase: string; readonly completed: number; @@ -77,7 +16,7 @@ describe("HopperClient progress", () => { }> = []; const pending = client.callTool( "echo", - { value: "progress" }, + { value: "progress", gate: "progress" }, { progress: { report: (update) => { @@ -87,9 +26,20 @@ describe("HopperClient progress", () => { }, }, ); + const request = await launcher.waitForRequest("echo", "progress"); + await vi.waitFor(() => + expect(updates).toContainEqual( + expect.objectContaining({ phase: "hopper_bridge", completed: 0 }), + ), + ); + expect(updates.some(({ terminal }) => terminal === true)).toBe(false); + await launcher.release(request); const result = await pending; - expect(result).toEqual({ ok: true, value: { value: "progress" } }); + expect(result).toEqual({ + ok: true, + value: { value: "progress", gate: "progress" }, + }); expect(updates).toEqual( expect.arrayContaining([ expect.objectContaining({ @@ -136,17 +86,22 @@ describe("HopperClient progress", () => { }); it("cancels and ignores late responses without corrupting the session", async () => { - const client = await startClient(); + const launcher = new HopperFixtureLauncher(); + const client = await startClient(launcher); const controller = new AbortController(); - const pending = client.callTool("hang", {}, { signal: controller.signal }); + const pending = client.callTool( + "echo", + { gate: "late-reply" }, + { signal: controller.signal }, + ); + const request = await launcher.waitForRequest("echo", "late-reply"); controller.abort(); const cancelled = await pending; expect(cancelled.ok).toBe(false); if (!cancelled.ok) expect(cancelled.error._tag).toBe("HopperCancelledError"); - const waited = await client.callTool("echo", { delay: 30 }); - expect(waited).toMatchObject({ ok: true }); + await launcher.release(request); await expect(client.callTool("echo", { value: "alive" })).resolves.toEqual({ ok: true, value: { value: "alive" }, @@ -154,12 +109,13 @@ describe("HopperClient progress", () => { }); it("retains cancelled bridge activity until the reply releases the queue", async () => { - const client = await startClient(); + const launcher = new HopperFixtureLauncher(); + const client = await startClient(launcher); const progress: string[] = []; const controller = new AbortController(); const pending = client.callTool( "echo", - { delay: 60 }, + { gate: "cancel-active" }, { signal: controller.signal, progress: { @@ -170,7 +126,7 @@ describe("HopperClient progress", () => { }, }, ); - await new Promise((resolve) => setTimeout(resolve, 5)); + const request = await launcher.waitForRequest("echo", "cancel-active"); controller.abort(); const result = await pending; @@ -187,8 +143,8 @@ describe("HopperClient progress", () => { expect.stringContaining("started on Hopper's serial bridge"), ]), ); - await new Promise((resolve) => setTimeout(resolve, 70)); - expect(client.requestActivity()).toBeNull(); + await launcher.release(request); + await vi.waitFor(() => expect(client.requestActivity()).toBeNull()); await expect(client.callTool("echo", { alive: true })).resolves.toEqual({ ok: true, value: { alive: true }, diff --git a/tests/boundary/providers/hopper/hopperClient.protocol.test.ts b/tests/boundary/providers/hopper/hopperClient.protocol.test.ts index a4e9ca87b..3c1da098a 100644 --- a/tests/boundary/providers/hopper/hopperClient.protocol.test.ts +++ b/tests/boundary/providers/hopper/hopperClient.protocol.test.ts @@ -1,74 +1,14 @@ -import { spawn, type ChildProcess } from "node:child_process"; -import { fileURLToPath } from "node:url"; - -import { afterEach, describe, expect, it } from "vitest"; +import { describe, expect, it, onTestFinished, vi } from "vitest"; import { HOPPER_PROVIDER_TOOL_CONTRACTS } from "../../../../src/hopper/HopperProvider.js"; -import { ok } from "../../../../src/domain/result.js"; -import type { - BridgeLauncher, - BridgeSession, -} from "../../../../src/hopper/BridgeLauncher.js"; import { HopperClient } from "../../../../src/hopper/HopperClient.js"; import type { HopperDiagnostic } from "../../../../src/hopper/HopperDiagnostics.js"; import { providerCleanupFailure } from "../../../../src/hopper/HopperDiagnostics.js"; -const fixturePath = fileURLToPath( - new URL("../../../fixtures/fakeHopper.mjs", import.meta.url), -); - -class FixtureLauncher implements BridgeLauncher { - socketPaths: string[] = []; - directories: string[] = []; - runIds: string[] = []; - processes: ChildProcess[] = []; - - constructor(readonly tokenOverride?: string) {} - - launch(session: BridgeSession) { - this.socketPaths.push(session.socketPath); - this.directories.push(session.directory); - this.runIds.push(session.runId); - const child = spawn( - process.execPath, - [ - fixturePath, - session.socketPath, - this.tokenOverride ?? session.token, - session.runId, - ], - { - stdio: ["ignore", "ignore", "pipe"], - }, - ); - this.processes.push(child); - return Promise.resolve( - ok({ - process: child, - ownsProcessLifetime: true as const, - shutdownMode: "bridge-request" as const, - }), - ); - } -} - -const clients: HopperClient[] = []; -const startClient = async () => { - const client = new HopperClient({ - launcher: new FixtureLauncher(), - startupTimeoutMs: 1_000, - }); - clients.push(client); - await expect(client.start()).resolves.toEqual({ - ok: true, - value: { name: "REA Hopper bridge", version: "1.0.0" }, - }); - return client; -}; - -afterEach(async () => { - await Promise.all(clients.splice(0).map((client) => client.close())); -}); +import { + HopperFixtureLauncher as FixtureLauncher, + startHopperFixtureClient as startClient, +} from "./hopperClient.fixture.js"; describe("HopperClient protocol", () => { it("sanitizes unexpected owned-cleanup exceptions", () => { @@ -89,7 +29,7 @@ describe("HopperClient protocol", () => { runId: "11111111-1111-4111-8111-111111111111", startupTimeoutMs: 1_000, }); - clients.push(client); + onTestFinished(() => client.close()); expect((await client.start()).ok).toBe(true); expect(launcher.runIds).toEqual(["11111111-1111-4111-8111-111111111111"]); @@ -98,25 +38,36 @@ describe("HopperClient protocol", () => { it("keeps the native socket path below macOS sockaddr_un limits", async () => { const launcher = new FixtureLauncher(); const client = new HopperClient({ launcher, startupTimeoutMs: 1_000 }); - clients.push(client); + onTestFinished(() => client.close()); const started = await client.start(); expect(started.ok).toBe(true); expect(Buffer.byteLength(launcher.socketPaths[0] ?? "")).toBeLessThan(104); }); it("serializes concurrent calls until the active Hopper reply arrives", async () => { - const client = await startClient(); - const slow = client.callTool("echo", { label: "slow", delay: 30 }); - const fast = client.callTool("echo", { label: "fast", delay: 1 }); + const launcher = new FixtureLauncher(); + const client = await startClient(launcher); + const slow = client.callTool("echo", { label: "slow", gate: "serial" }); + const request = await launcher.waitForRequest("echo", "serial"); + const fast = client.callTool("echo", { label: "fast" }); let fastSettled = false; void fast.then(() => { fastSettled = true; }); - await new Promise((resolve) => setTimeout(resolve, 10)); + await vi.waitFor(() => + expect(client.requestActivity()).toMatchObject({ + operation: "echo", + queuedRequests: 1, + }), + ); expect(fastSettled).toBe(false); + expect(launcher.requests.filter(({ method }) => method === "echo")).toEqual( + [request], + ); + await launcher.release(request); await expect(Promise.all([slow, fast])).resolves.toEqual([ - { ok: true, value: { label: "slow", delay: 30 } }, - { ok: true, value: { label: "fast", delay: 1 } }, + { ok: true, value: { label: "slow", gate: "serial" } }, + { ok: true, value: { label: "fast" } }, ]); }); @@ -136,7 +87,7 @@ describe("HopperClient protocol", () => { launcher: new FixtureLauncher("wrong-token"), startupTimeoutMs: 1_000, }); - clients.push(client); + onTestFinished(() => client.close()); const result = await client.start(); expect(result.ok).toBe(false); if (!result.ok) expect(result.error._tag).toBe("HopperRemoteError"); @@ -150,24 +101,29 @@ describe("HopperClient protocol", () => { ["remote_error", "HopperRemoteError"], ["exit", "HopperProcessError"], ])("projects %s as %s", async (method, expectedTag) => { - const client = await startClient(); + const launcher = new FixtureLauncher(); + const client = await startClient(launcher); const result = await client.callTool(method); expect(result.ok).toBe(false); if (!result.ok) { expect(result.error._tag).toBe(expectedTag); if (result.error._tag === "HopperRemoteError") - expect(result.error).toMatchObject({ operation: method, requestId: 2 }); + expect(result.error).toMatchObject({ + operation: method, + requestId: (await launcher.waitForRequest(method)).id, + }); } }); it("preserves a sanitized bridge exception diagnostic", async () => { + const launcher = new FixtureLauncher(); const diagnostics: HopperDiagnostic[] = []; const client = new HopperClient({ - launcher: new FixtureLauncher(), + launcher, startupTimeoutMs: 1_000, onDiagnostic: (event) => diagnostics.push(event), }); - clients.push(client); + onTestFinished(() => client.close()); await expect(client.start()).resolves.toMatchObject({ ok: true }); const result = await client.callTool("remote_error"); expect(result.ok).toBe(false); @@ -178,7 +134,7 @@ describe("HopperClient protocol", () => { }); expect(diagnostics).toContainEqual({ type: "bridge-diagnostic", - request_id: 2, + request_id: (await launcher.waitForRequest("remote_error")).id, code: -32001, category: "bridge_exception", message: "safe fake failure", diff --git a/tests/boundary/providers/hopper/hopperClient.restart.test.ts b/tests/boundary/providers/hopper/hopperClient.restart.test.ts index 0a1896908..3898956aa 100644 --- a/tests/boundary/providers/hopper/hopperClient.restart.test.ts +++ b/tests/boundary/providers/hopper/hopperClient.restart.test.ts @@ -1,71 +1,10 @@ -import { spawn, type ChildProcess } from "node:child_process"; -import { fileURLToPath } from "node:url"; - -import { afterEach, describe, expect, it } from "vitest"; - -import { ok } from "../../../../src/domain/result.js"; -import type { - BridgeLauncher, - BridgeSession, -} from "../../../../src/hopper/BridgeLauncher.js"; +import { describe, expect, it, onTestFinished } from "vitest"; import { HopperClient } from "../../../../src/hopper/HopperClient.js"; -const fixturePath = fileURLToPath( - new URL("../../../fixtures/fakeHopper.mjs", import.meta.url), -); - -class FixtureLauncher implements BridgeLauncher { - socketPaths: string[] = []; - directories: string[] = []; - runIds: string[] = []; - processes: ChildProcess[] = []; - - constructor(readonly tokenOverride?: string) {} - - launch(session: BridgeSession) { - this.socketPaths.push(session.socketPath); - this.directories.push(session.directory); - this.runIds.push(session.runId); - const child = spawn( - process.execPath, - [ - fixturePath, - session.socketPath, - this.tokenOverride ?? session.token, - session.runId, - ], - { - stdio: ["ignore", "ignore", "pipe"], - }, - ); - this.processes.push(child); - return Promise.resolve( - ok({ - process: child, - ownsProcessLifetime: true as const, - shutdownMode: "bridge-request" as const, - }), - ); - } -} - -const clients: HopperClient[] = []; -const startClient = async () => { - const client = new HopperClient({ - launcher: new FixtureLauncher(), - startupTimeoutMs: 1_000, - }); - clients.push(client); - await expect(client.start()).resolves.toEqual({ - ok: true, - value: { name: "REA Hopper bridge", version: "1.0.0" }, - }); - return client; -}; - -afterEach(async () => { - await Promise.all(clients.splice(0).map((client) => client.close())); -}); +import { + HopperFixtureLauncher as FixtureLauncher, + startHopperFixtureClient as startClient, +} from "./hopperClient.fixture.js"; describe("HopperClient restart", () => { it("does not finish startup after an immediate close", async () => { @@ -73,7 +12,7 @@ describe("HopperClient restart", () => { launcher: new FixtureLauncher(), startupTimeoutMs: 1_000, }); - clients.push(client); + onTestFinished(() => client.close()); const starting = client.start(); await client.close(); diff --git a/tests/boundary/providers/hopper/hopperClient.startupFailures.test.ts b/tests/boundary/providers/hopper/hopperClient.startupFailures.test.ts index 54c8cc61f..23c05d4f0 100644 --- a/tests/boundary/providers/hopper/hopperClient.startupFailures.test.ts +++ b/tests/boundary/providers/hopper/hopperClient.startupFailures.test.ts @@ -1,6 +1,5 @@ import { spawn, type ChildProcess } from "node:child_process"; import { access } from "node:fs/promises"; -import { fileURLToPath } from "node:url"; import { afterEach, describe, expect, it } from "vitest"; @@ -14,44 +13,7 @@ import type { import { HopperClient } from "../../../../src/hopper/HopperClient.js"; import { LINUX_PRIVATE_DISPLAY_DIAGNOSTIC_PREFIX } from "../../../../src/hopper/LinuxPrivateDisplayDiagnostic.js"; -const fixturePath = fileURLToPath( - new URL("../../../fixtures/fakeHopper.mjs", import.meta.url), -); - -class FixtureLauncher implements BridgeLauncher { - socketPaths: string[] = []; - directories: string[] = []; - runIds: string[] = []; - processes: ChildProcess[] = []; - - constructor(readonly tokenOverride?: string) {} - - launch(session: BridgeSession) { - this.socketPaths.push(session.socketPath); - this.directories.push(session.directory); - this.runIds.push(session.runId); - const child = spawn( - process.execPath, - [ - fixturePath, - session.socketPath, - this.tokenOverride ?? session.token, - session.runId, - ], - { - stdio: ["ignore", "ignore", "pipe"], - }, - ); - this.processes.push(child); - return Promise.resolve( - ok({ - process: child, - ownsProcessLifetime: true as const, - shutdownMode: "bridge-request" as const, - }), - ); - } -} +import { HopperFixtureLauncher as FixtureLauncher } from "./hopperClient.fixture.js"; class SilentLauncher implements BridgeLauncher { launch() { @@ -175,12 +137,10 @@ describe("HopperClient startup failures", () => { // Cancel after launch: a timer can fire during runtime-directory creation, // leaving the silent first launch to be consumed by the retry instead. const firstProcess = await launcher.firstLaunch; - const startedAt = Date.now(); controller.abort(); const result = await pending; expect(result.ok).toBe(false); if (!result.ok) expect(result.error._tag).toBe("HopperCancelledError"); - expect(Date.now() - startedAt).toBeLessThan(500); expect( firstProcess.exitCode !== null || firstProcess.signalCode !== null, ).toBe(true); diff --git a/tests/fixtures/captureGhidraLaunch.mjs b/tests/fixtures/captureGhidraLaunch.mjs index 1a8ca982d..29bb8d315 100755 --- a/tests/fixtures/captureGhidraLaunch.mjs +++ b/tests/fixtures/captureGhidraLaunch.mjs @@ -8,8 +8,9 @@ const descriptorPath = arguments_.at(-1); if (descriptorPath === undefined) process.exit(64); const descriptor = JSON.parse(await readFile(descriptorPath, "utf8")); const runtimeRoot = dirname(descriptorPath); +const capturePath = join(runtimeRoot, "launch-capture.json"); await writeFile( - join(runtimeRoot, "launch-capture.json"), + capturePath, `${JSON.stringify({ arguments: arguments_, environment: { @@ -38,4 +39,7 @@ await writeFile( })}\n`, { mode: 0o600 }, ); +// A Windows private runtime retains directory handles that restrict renames. +// Publish readiness only after writeFile has closed the complete JSON capture. +await writeFile(`${capturePath}.ready`, "", { flag: "wx", mode: 0o600 }); setInterval(() => undefined, 1_000); diff --git a/tests/fixtures/fakeGhidra.mjs b/tests/fixtures/fakeGhidra.mjs index d77b4864e..d0b9655f9 100644 --- a/tests/fixtures/fakeGhidra.mjs +++ b/tests/fixtures/fakeGhidra.mjs @@ -72,6 +72,7 @@ const onSocketData = (socket, server, chunk, state) => { state.buffer.value = state.buffer.value.slice(newline + 1); newline = state.buffer.value.indexOf("\n"); const request = JSON.parse(line); + process.send?.({ method: request.method }); if (request.token !== state.token) { socket.end( `${JSON.stringify({ diff --git a/tests/fixtures/fakeHopper.mjs b/tests/fixtures/fakeHopper.mjs index 79d695e21..10bdd4e71 100644 --- a/tests/fixtures/fakeHopper.mjs +++ b/tests/fixtures/fakeHopper.mjs @@ -2,6 +2,14 @@ import { createServer } from "node:net"; const [socketPath, token, runId, shutdownMode = "acknowledge"] = process.argv.slice(2); +const heldReplies = new Map(); +if (process.send !== undefined) + process.on("message", (message) => { + const reply = heldReplies.get(message?.release); + if (reply === undefined) return; + heldReplies.delete(message.release); + reply(); + }); const shutdownResult = () => ({ shutdown: true, analysis_stopped: true, @@ -115,7 +123,7 @@ const sendEchoWithProgress = (send, request) => { message: "Hopper bridge started request", }, }); - setTimeout(() => { + const reply = () => { send({ id: request.id, event: { @@ -128,7 +136,10 @@ const sendEchoWithProgress = (send, request) => { }, }); send({ id: request.id, result: request.params ?? {} }); - }, request.params?.delay ?? 0); + }; + if (typeof request.params?.gate === "string") + heldReplies.set(request.id, reply); + else setTimeout(reply, request.params?.delay ?? 0); }; const server = createServer((socket) => { @@ -147,6 +158,11 @@ const server = createServer((socket) => { let newline = buffer.indexOf("\n"); while (newline >= 0) { const request = JSON.parse(buffer.slice(0, newline)); + process.send?.({ + id: request.id, + method: request.method, + params: request.params, + }); const bridgeEventMessages = bridgeEventFixtureMessages(request); buffer = buffer.slice(newline + 1); if (request.token !== token) { @@ -169,7 +185,13 @@ const server = createServer((socket) => { ) { send({ id: request.id, result: shutdownResult() }); if (shutdownMode !== "cleanup-required") - setTimeout(() => server.close(), 2); + setTimeout( + () => + server.close(() => { + if (process.connected) process.disconnect(); + }), + 2, + ); } else if (request.method === "hang") { // Deliberately leave the request pending. } else if (request.method === "exit") {