From 5a0e0ba3b81ea0ec336addb07da98ec7bc0a35d3 Mon Sep 17 00:00:00 2001 From: morluto <76467478+morluto@users.noreply.github.com> Date: Wed, 7 Oct 2026 10:49:23 +0800 Subject: [PATCH] fix: address review nits from batch integration follow-ups (#818) - browser: reject empty Referrer-Policy tokens structurally while still skipping them per W3 fallback semantics. - browser: document that WebMCP keys cannot collapse since unknown frames are excluded before keying. - ghidra: split NativeAOT non-recovery diagnostics into reason-missing vs payload-present variants with exact assertions. - reference: lock secret-patterned exclusion before failure projection so failure messages cannot carry secret paths. - doctor: lock direct-launcher Codex timeout ordering both ways. - native: note plutil -n macOS 12+ requirement; add fat-output control guarding the thin pre-scan. - reconstruction: multi-unknown limitation coverage via shared helper. Tracks #817 for the remaining smol-toml advisory. --- src/application/BinaryTargetResolver.ts | 1 + ...ReconstructionObligationCandidates.test.ts | 157 +++++++++++------- src/browser/CdpSafeMetadata.ts | 3 +- src/browser/CdpWebMcpDiscovery.ts | 2 + .../extensions/GhidraExtensions.test.ts | 4 +- .../extensions/nativeaot/NativeAotReport.ts | 15 +- .../clientRegistrationStatus.test.ts | 19 +++ .../referenceSourceFailureDetails.test.ts | 25 +++ .../native/nativeTextRobustness.test.ts | 25 +++ 9 files changed, 180 insertions(+), 71 deletions(-) diff --git a/src/application/BinaryTargetResolver.ts b/src/application/BinaryTargetResolver.ts index 21a6a8ebfb..9d9b414b6b 100644 --- a/src/application/BinaryTargetResolver.ts +++ b/src/application/BinaryTargetResolver.ts @@ -275,6 +275,7 @@ const parseXmlPlistExecutable = (plist: string): string => { }; const readBinaryPlistExecutable = async (plistPath: string): Promise => + // `-n` strips only the newline plutil appends; it requires macOS 12+. ( await execFileAsync("/usr/bin/plutil", [ "-extract", diff --git a/src/application/ReconstructionObligationCandidates.test.ts b/src/application/ReconstructionObligationCandidates.test.ts index 38d308c3ca..ea96c3527d 100644 --- a/src/application/ReconstructionObligationCandidates.test.ts +++ b/src/application/ReconstructionObligationCandidates.test.ts @@ -16,28 +16,28 @@ import { jsonValueSchema } from "../domain/jsonValue.js"; import { MANAGED_WORKFLOW_PROVIDER } from "./InvestigationProviders.js"; import { deriveReconstructionObligationCandidates } from "./ReconstructionObligationCandidates.js"; -describe("reconstruction obligation candidates", () => { - it("preserves admitted unknown application nodes as limitations", () => { - const source = createEvidence( - undefined, - { id: "fixture-source", name: "Fixture source", version: "1" }, - { - operation: "inspect_fixture_source", - parameters: {}, - result: {}, - confidence: "observed", - authority: "shipped-artifact", - }, - ); - const base = buildSyntheticJavaScriptApplicationGraph(); - const unknownNode = createJavaScriptApplicationNode({ +const applicationWithUnknowns = (keys: readonly string[]) => { + const source = createEvidence( + undefined, + { id: "fixture-source", name: "Fixture source", version: "1" }, + { + operation: "inspect_fixture_source", + parameters: {}, + result: {}, + confidence: "observed", + authority: "shipped-artifact", + }, + ); + const base = buildSyntheticJavaScriptApplicationGraph(); + const unknownNodes = keys.map((key) => + createJavaScriptApplicationNode({ kind: "unknown", identity: { strategy: "artifact-local-key", stability: "artifact-version", artifact_sha256: APPLICATION_GRAPH_DIGESTS.asar, namespace: "fixture", - key: "unclassified-boundary", + key, }, observations: [ { @@ -49,52 +49,68 @@ describe("reconstruction obligation candidates", () => { ), }, ], - }); - const graph = createJavaScriptApplicationGraph({ - schema: "JavaScriptApplicationGraph", - root_node_ids: base.root_node_ids, - nodes: [...base.nodes, unknownNode], - edges: base.edges, - coverage: completeCoverage, - limitations: base.limitations, - }); - const application = createEvidence(undefined, MANAGED_WORKFLOW_PROVIDER, { - predicateType: "rea.managed-application-graph", - operation: "project_managed_application_graph", - parameters: {}, - result: jsonValueSchema.parse({ - projection_id: `magp_${"a".repeat(64)}`, - root_artifact_sha256: APPLICATION_GRAPH_DIGESTS.asar, - source_evidence: { - managed_artifact_evidence_id: source.evidence_id, - managed_members_evidence_id: null, - managed_native_boundaries_evidence_id: null, - }, - summary: { - graph_nodes: graph.nodes.length, - graph_edges: graph.edges.length, - assemblies: 0, - modules: 0, - types: 0, - methods: 0, - fields: 0, - pinvoke_imports: 0, - native_implementations: 0, - }, - graph, - coverage: { status: "complete-within-inputs" }, - evidence_links: [source.evidence_id], - limitations: [], - }), - confidence: "inferred", - authority: "analyst-inference", - evidenceLinks: [source.evidence_id], - }); - - const generated = deriveReconstructionObligationCandidates( + }), + ); + const graph = createJavaScriptApplicationGraph({ + schema: "JavaScriptApplicationGraph", + root_node_ids: base.root_node_ids, + nodes: [...base.nodes, ...unknownNodes], + edges: base.edges, + coverage: completeCoverage, + limitations: base.limitations, + }); + const application = createEvidence(undefined, MANAGED_WORKFLOW_PROVIDER, { + predicateType: "rea.managed-application-graph", + operation: "project_managed_application_graph", + parameters: {}, + result: jsonValueSchema.parse({ + projection_id: `magp_${"a".repeat(64)}`, + root_artifact_sha256: APPLICATION_GRAPH_DIGESTS.asar, + source_evidence: { + managed_artifact_evidence_id: source.evidence_id, + managed_members_evidence_id: null, + managed_native_boundaries_evidence_id: null, + }, + summary: { + graph_nodes: graph.nodes.length, + graph_edges: graph.edges.length, + assemblies: 0, + modules: 0, + types: 0, + methods: 0, + fields: 0, + pinvoke_imports: 0, + native_implementations: 0, + }, + graph, + coverage: { status: "complete-within-inputs" }, + evidence_links: [source.evidence_id], + limitations: [], + }), + confidence: "inferred", + authority: "analyst-inference", + evidenceLinks: [source.evidence_id], + }); + return { + graph, + unknownNodes, + generated: deriveReconstructionObligationCandidates( createEvidenceBundle([source, application]), [], - ); + ), + }; +}; + +const unknownLimitation = (graphId: string, nodeId: string): string => + `Application graph ${graphId} contains unresolved node ${nodeId}; reconstruction obligations remain unknown.`; + +describe("reconstruction obligation candidates", () => { + it("preserves admitted unknown application nodes as limitations", () => { + const { graph, unknownNodes, generated } = applicationWithUnknowns([ + "unclassified-boundary", + ]); + const unknownNode = unknownNodes[0]; + if (unknownNode === undefined) throw new Error("Expected one unknown node"); expect(generated.candidates.length).toBeGreaterThan(0); expect( @@ -103,7 +119,26 @@ describe("reconstruction obligation candidates", () => { ), ).toBe(false); expect(generated.limitations).toEqual([ - `Application graph ${graph.graph_id} contains unresolved node ${unknownNode.node_id}; reconstruction obligations remain unknown.`, + unknownLimitation(graph.graph_id, unknownNode.node_id), ]); }); + + it("emits one deterministic limitation per unknown node", () => { + const { graph, unknownNodes, generated } = applicationWithUnknowns([ + "unclassified-first", + "unclassified-second", + ]); + + expect([...generated.limitations].sort()).toEqual( + unknownNodes + .map((node) => unknownLimitation(graph.graph_id, node.node_id)) + .sort(), + ); + for (const node of unknownNodes) + expect( + generated.candidates.some( + ({ target }) => target.application_node_id === node.node_id, + ), + ).toBe(false); + }); }); diff --git a/src/browser/CdpSafeMetadata.ts b/src/browser/CdpSafeMetadata.ts index ab91a55930..8a2a2f54f3 100644 --- a/src/browser/CdpSafeMetadata.ts +++ b/src/browser/CdpSafeMetadata.ts @@ -331,7 +331,8 @@ const referrerPolicy = (value: string | undefined): string | null => { let policy: string | null = null; for (const raw of (value ?? "").split(",")) { const token = raw.trim().toLowerCase(); - if (!/^[a-z-]*$/u.test(token)) return null; + if (token === "") continue; + if (!/^[a-z-]+$/u.test(token)) return null; if (referrerPolicies.has(token)) policy = token; } return policy; diff --git a/src/browser/CdpWebMcpDiscovery.ts b/src/browser/CdpWebMcpDiscovery.ts index 1acacc8daa..9da2cdf514 100644 --- a/src/browser/CdpWebMcpDiscovery.ts +++ b/src/browser/CdpWebMcpDiscovery.ts @@ -498,6 +498,8 @@ const buildWebMcpResult = (options: WebMcpResultOptions): WebMcpDiscovery => { }; const toolKey = (frameUrl: string, frameId: string, name: string): string => + // Registrations without a known admitted frame are excluded above, so the + // frame ID here always distinguishes owners and keys cannot collapse. `webmcp_${createHash("sha256").update(`${frameUrl}\0${frameId}\0${name}`).digest("hex")}`; const booleanOrNull = (value: unknown): boolean | null => diff --git a/src/ghidra/extensions/GhidraExtensions.test.ts b/src/ghidra/extensions/GhidraExtensions.test.ts index 183ce74a71..b35e63f9cc 100644 --- a/src/ghidra/extensions/GhidraExtensions.test.ts +++ b/src/ghidra/extensions/GhidraExtensions.test.ts @@ -434,7 +434,7 @@ describe("Ghidra extension recovery coverage", () => { [extension], [{ ...report, result: { ...report.result, method_tables: 1 } }], ), - ).not.toBeNull(); + ).toBe("NativeAOT non-recovery status carries recovered metadata."); for (const reason of [null, "", " "] as const) expect( validateGhidraExtensionResults( @@ -447,7 +447,7 @@ describe("Ghidra extension recovery coverage", () => { }, ], ), - ).not.toBeNull(); + ).toBe("NativeAOT non-recovery status omits its required reason."); }, ); }); diff --git a/src/ghidra/extensions/nativeaot/NativeAotReport.ts b/src/ghidra/extensions/nativeaot/NativeAotReport.ts index 327263009f..5fff41e5ed 100644 --- a/src/ghidra/extensions/nativeaot/NativeAotReport.ts +++ b/src/ghidra/extensions/nativeaot/NativeAotReport.ts @@ -57,16 +57,17 @@ type NativeAotReport = z.infer; const validateNonRecoveryMetadata = ( value: NativeAotReport, recovered: boolean, -): string | null => - !recovered && - (value.reason === null || - value.reason.trim().length === 0 || - value.method_tables !== 0 || +): string | null => { + if (recovered) return null; + if (value.reason === null || value.reason.trim().length === 0) + return "NativeAOT non-recovery status omits its required reason."; + return value.method_tables !== 0 || value.types !== undefined || value.derived_memory !== undefined || - value.coverage !== undefined) - ? "NativeAOT non-recovery status contradicts its reason or recovered metadata." + value.coverage !== undefined + ? "NativeAOT non-recovery status carries recovered metadata." : null; +}; /** Validate the pinned producer representation before its result enters Evidence. */ export const validateNativeAotReport = ( diff --git a/tests/boundary/filesystem/clientRegistrationStatus.test.ts b/tests/boundary/filesystem/clientRegistrationStatus.test.ts index 49fc5c276f..ea9d046432 100644 --- a/tests/boundary/filesystem/clientRegistrationStatus.test.ts +++ b/tests/boundary/filesystem/clientRegistrationStatus.test.ts @@ -206,6 +206,25 @@ describe("Node-wrapped registration policy", () => { }, ); + it("checks a direct-launcher Codex startup timeout without Node wrapping", async () => { + const home = await createTestTempDirectory("rea-direct-registration-"); + await mkdir(join(home, ".codex")); + await writeFile( + join(home, ".codex/config.toml"), + '[mcp_servers.rea]\ncommand = "/current/rea"\nargs = ["mcp"]\nstartup_timeout_sec = 1\n', + ); + const statuses = await readClientRegistrationStatuses( + home, + "/current/rea", + { + environment: {}, + }, + ); + expect(statuses).toEqual([ + expect.objectContaining({ client: "codex", state: "stale" }), + ]); + }); + it.each([{ tools: ["binary_session"] }, { tools: ["*"] }])( "checks Copilot tool selection $tools independently of launcher", async ({ tools }) => { diff --git a/tests/boundary/filesystem/referenceSourceFailureDetails.test.ts b/tests/boundary/filesystem/referenceSourceFailureDetails.test.ts index 47898ad069..a4810e312d 100644 --- a/tests/boundary/filesystem/referenceSourceFailureDetails.test.ts +++ b/tests/boundary/filesystem/referenceSourceFailureDetails.test.ts @@ -69,6 +69,31 @@ describe("reference import native entry failure details", () => { }, ); + it.skipIf(process.platform === "win32")( + "excludes secret-patterned paths before failure projection", + async () => { + const root = await createTestTempDirectory("rea-import-native-secret-"); + await promisify(execFile)("mkfifo", [join(root, "secret-events.pipe")], { + timeout: 2_000, + }); + const imported = await importReferenceSource({ + root, + caller: "native-entry-secret-detail-test", + policy: { secretPatterns: ["*secret*"] }, + }); + if (!imported.ok) throw new Error(imported.error.message); + expect( + imported.value.entries.find( + (item) => item.path === "secret-events.pipe", + ), + ).toBeUndefined(); + expect(imported.value.exclusions).toContainEqual({ + path: "secret-events.pipe", + reason: "configured-secret", + }); + }, + ); + it("preserves an ordinary readable source-file control", async () => { const root = await createTestTempDirectory("rea-import-native-readable-"); await writeFile(join(root, "main.ts"), "export const value = 1;\n"); diff --git a/tests/boundary/providers/native/nativeTextRobustness.test.ts b/tests/boundary/providers/native/nativeTextRobustness.test.ts index 05ae9e0da6..7f17912547 100644 --- a/tests/boundary/providers/native/nativeTextRobustness.test.ts +++ b/tests/boundary/providers/native/nativeTextRobustness.test.ts @@ -19,6 +19,31 @@ it.each([ }, ); +it("does not mistake fat slice output for a thin Non-fat file record", () => { + const output = [ + "Fat header in: /owned/universal.dylib", + "fat_magic 0xcafebab2", + "nfat_arch 2", + "architecture arm64", + " cputype CPU_TYPE_ARM64", + " cpusubtype CPU_SUBTYPE_ARM64E", + " offset 16384", + " size 16777216", + " align 2^14 (16384)", + "architecture x86_64", + " cputype CPU_TYPE_X86_64", + " cpusubtype CPU_SUBTYPE_X86_64_ALL", + " offset 16793600", + " size 16777216", + " align 2^12 (4096)", + "", + ].join("\n"); + expect(parseLipoArchitectures(output)).toMatchObject([ + { name: "arm64", file_offset: 16384 }, + { name: "x86_64", file_offset: 16793600 }, + ]); +}); + it("keeps section blocks separate under CRLF captures", async () => { const lf = parseOtoolLoadCommands(await nativeFixture("otool-load.txt")); const crlf = parseOtoolLoadCommands(