diff --git a/src/application/reverse/ReverseEngineeringService.ts b/src/application/reverse/ReverseEngineeringService.ts index 022586879a..74dada85d3 100644 --- a/src/application/reverse/ReverseEngineeringService.ts +++ b/src/application/reverse/ReverseEngineeringService.ts @@ -23,6 +23,7 @@ import { import { AnalysisOutputError } from "../../domain/analysisErrorCore.js"; import { ProviderAdapterError } from "../../domain/providerAdapterError.js"; import { createEvidence, type Evidence } from "../../domain/evidence.js"; +import { analysisInputErrorFromIssues } from "../../domain/inputIssueProjection.js"; import type { JsonValue } from "../../domain/jsonValue.js"; import { err, ok, type Result } from "../../domain/result.js"; import { @@ -30,6 +31,7 @@ import { MAX_REVERSE_COMMAND_TIMEOUT_MS, objdumpInputSchema, rizinInputSchema, + type RizinInput, } from "../../domain/reverseEngineering.js"; import type { ReverseEngineeringCommandPort, @@ -83,6 +85,23 @@ export type ReverseEngineeringServiceRuntimeOptions = Omit< "commands" | "environment" >; +/** Resolve one command for shared execution and adapter admission decisions. */ +export const resolveRizinCommandRequest = ( + input: unknown, +): Result => { + const parsed = rizinInputSchema.safeParse(input); + return parsed.success + ? ok(parsed.data) + : err( + analysisInputErrorFromIssues( + "execute_rizin_command", + parsed.error.issues, + input, + { cause: parsed.error }, + ), + ); +}; + /** Shared application workflow for caller-selected local analysis commands. */ export class ReverseEngineeringService { readonly #environment: Readonly; @@ -234,18 +253,24 @@ export class ReverseEngineeringService { rawInput: unknown, options: { readonly signal?: AbortSignal } = {}, ): Promise> { - const parsed = rizinInputSchema.safeParse(rawInput); - if (!parsed.success) - return Promise.resolve( - err(new AnalysisInputError("execute_rizin_command")), - ); - const { command_timeout_ms: commandTimeoutMs, ...input } = parsed.data; + const parsed = resolveRizinCommandRequest(rawInput); + return parsed.ok + ? this.executeRizinCommandValidated(parsed.value, options) + : Promise.resolve(parsed); + } + + /** Execute owner-validated input after the adapter has admitted its target. */ + executeRizinCommandValidated( + request: RizinInput, + options: { readonly signal?: AbortSignal } = {}, + ): Promise> { + const { command_timeout_ms: commandTimeoutMs, ...input } = request; const command = this.#commands.rizin(input); return this.#observe( "execute_rizin_command", command, input.path, - commandTimeoutMs === undefined ? input : parsed.data, + commandTimeoutMs === undefined ? input : request, options.signal, commandTimeoutMs ?? DEFAULT_REVERSE_COMMAND_TIMEOUT_MS, ); diff --git a/src/domain/reverseEngineering.ts b/src/domain/reverseEngineering.ts index 90b435db0c..0e87c9bfbf 100644 --- a/src/domain/reverseEngineering.ts +++ b/src/domain/reverseEngineering.ts @@ -90,4 +90,4 @@ export type ObjdumpSelection = | z.input | z.input | z.input; -export type RizinInput = z.input; +export type RizinInput = z.output; diff --git a/src/server/registerReverseEngineeringTools.ts b/src/server/registerReverseEngineeringTools.ts index 8e686dcf11..f2b0540244 100644 --- a/src/server/registerReverseEngineeringTools.ts +++ b/src/server/registerReverseEngineeringTools.ts @@ -6,7 +6,10 @@ import type { } from "@modelcontextprotocol/server"; import type { Logger } from "pino"; -import type { ReverseEngineeringService } from "../application/reverse/ReverseEngineeringService.js"; +import { + resolveRizinCommandRequest, + type ReverseEngineeringService, +} from "../application/reverse/ReverseEngineeringService.js"; import type { EvidenceLookup } from "../application/EvidenceInputResolver.js"; import type { EvidenceWriter } from "../application/investigation/InvestigationRecordPort.js"; import type { Evidence } from "../domain/evidence.js"; @@ -16,7 +19,6 @@ import { logToolExecution } from "./toolLogging.js"; import { toolRegistrationOptions } from "./toolRegistrationOptions.js"; import type { ToolResultDelivery } from "./toolResult.js"; import type { WithAdmittedAnalysis } from "./analysisAdmission.js"; -import { rizinInputSchema } from "../domain/reverseEngineering.js"; import { AnalysisOutputError } from "../domain/analysisErrorCore.js"; import type { AnalysisError } from "../domain/analysisErrorBase.js"; import type { Result } from "../domain/result.js"; @@ -81,18 +83,18 @@ export const registerReverseEngineeringTools = ( rizin.name, toolRegistrationOptions(rizin), async (input, context: ServerContext) => { - const parsed = rizinInputSchema.safeParse(input); - if (parsed.success && withAdmittedExternalMutation !== undefined) { + const parsed = resolveRizinCommandRequest(input); + if (parsed.ok && withAdmittedExternalMutation !== undefined) { const cleanup = await service.retryCleanup(rizin.name); if (!cleanup.ok) return deliverFailure(cleanup.error, rizin, delivery, recordEvidence); const admitted = await withAdmittedExternalMutation( - parsed.data.path, + parsed.value.path, context.mcpReq.signal, async () => { invalidateSnapshotForExternalMutation?.(); return logToolExecution(logger, rizin.name, () => - service.executeRizinCommand(parsed.data, { + service.executeRizinCommandValidated(parsed.value, { signal: context.mcpReq.signal, }), ); @@ -143,15 +145,14 @@ export const registerReverseEngineeringTools = ( withAdmittedAnalysis, delivery, async () => { - if ( - invalidateSnapshotForExternalMutation !== undefined && - rizinInputSchema.safeParse(input).success - ) + if (invalidateSnapshotForExternalMutation !== undefined && parsed.ok) invalidateSnapshotForExternalMutation(); const result = await logToolExecution(logger, rizin.name, () => - service.executeRizinCommand(input, { - signal: context.mcpReq.signal, - }), + parsed.ok + ? service.executeRizinCommandValidated(parsed.value, { + signal: context.mcpReq.signal, + }) + : Promise.resolve(parsed), ); if (!result.ok) return deliverFailure( diff --git a/tests/boundary/cli/reverseEngineeringTargetInput.test.ts b/tests/boundary/cli/reverseEngineeringTargetInput.test.ts index f7ca9169a6..5a65b2247a 100644 --- a/tests/boundary/cli/reverseEngineeringTargetInput.test.ts +++ b/tests/boundary/cli/reverseEngineeringTargetInput.test.ts @@ -50,3 +50,34 @@ cliTest( } }, ); + +cliTest( + "reports the rejected Rizin command before inspecting the target", + async ({ cli }) => { + const root = await createTestTempDirectory("rea-rizin-invalid-command-"); + const result = await cli.run({ + arguments: [ + "execute-rizin-command", + join(root, "unopened.bin"), + "iI\nq", + "--format", + "json", + ], + cwd: root, + }); + expect(result.exitCode).toBe(1); + expect(analysisErrorProjectionSchema.parse(result.json)).toMatchObject({ + code: "invalid_request", + details: { + operation: "execute_rizin_command", + issues: [ + { + path: ["command"], + reason: "invalid_value", + message: "A single command line is required", + }, + ], + }, + }); + }, +); diff --git a/tests/boundary/mcp/reverseEngineeringPartialMcp.test.ts b/tests/boundary/mcp/reverseEngineeringPartialMcp.test.ts index cf9eebeda3..28ffe79402 100644 --- a/tests/boundary/mcp/reverseEngineeringPartialMcp.test.ts +++ b/tests/boundary/mcp/reverseEngineeringPartialMcp.test.ts @@ -1,4 +1,6 @@ import pino from "pino"; +import type { CallToolResult } from "@modelcontextprotocol/server"; +import { parseMcpToolError } from "../../fixtures/mcpToolError.js"; import { mkdtemp, rm, symlink, writeFile } from "node:fs/promises"; import { tmpdir } from "node:os"; import { dirname, join } from "node:path"; @@ -131,11 +133,9 @@ it("invalidates the snapshot in the legacy Rizin admission path", async () => { it("skips snapshot invalidation for malformed Rizin input", async () => { const { server, handler } = createReverseEngineeringMcpHarness(); const invalidateSnapshot = vi.fn(); - const executeRizinCommand = vi.fn(async () => - err(new AnalysisInputError("execute_rizin_command")), - ); + const executeRizinCommandValidated = vi.fn(); const service = { - executeRizinCommand, + executeRizinCommandValidated, } as unknown as ReverseEngineeringService; registerReverseEngineeringTools( server, @@ -150,10 +150,17 @@ it("skips snapshot invalidation for malformed Rizin input", async () => { const response = (await invokeWithLiveSignal(invoke, { path: "package.json", - })) as { readonly isError?: boolean }; + })) as CallToolResult; - expect(response.isError).toBe(true); - expect(executeRizinCommand).toHaveBeenCalledOnce(); + expect(parseMcpToolError(response).error).toMatchObject({ + code: "invalid_request", + details: { + issues: [ + { path: ["command"], reason: "missing_argument", expected: "string" }, + ], + }, + }); + expect(executeRizinCommandValidated).not.toHaveBeenCalled(); expect(invalidateSnapshot).not.toHaveBeenCalled(); }); @@ -193,7 +200,7 @@ it("preserves the Rizin operation error when analysis retirement also fails", as const retirement = new AnalysisInputError("rebind"); const service = { retryCleanup: async () => ok(null), - executeRizinCommand: async () => err(primary), + executeRizinCommandValidated: async () => err(primary), } as unknown as ReverseEngineeringService; const observedOperationResult = vi.fn(); registerReverseEngineeringTools( @@ -421,7 +428,7 @@ it("does not treat a malformed partial observation as retained Evidence", async ); const service = { inspectWithObjdump: async () => err(invalidError), - executeRizinCommand: async () => err(invalidError), + executeRizinCommandValidated: async () => err(invalidError), } as unknown as ReverseEngineeringService; const recordEvidence = vi.fn(() => ok("added" as const)); registerReverseEngineeringTools( @@ -489,7 +496,7 @@ it("returns admission failures without invoking a reverse-engineering provider", const inspectWithObjdump = vi.fn(); const service = { inspectWithObjdump, - executeRizinCommand: vi.fn(), + executeRizinCommandValidated: vi.fn(), } as unknown as ReverseEngineeringService; const admission: WithAdmittedAnalysis = async () => err(new AnalysisInputError("inspect_with_objdump")); diff --git a/tests/composition/reverseEngineeringEdgeCases.test.ts b/tests/composition/reverseEngineeringEdgeCases.test.ts index e29fbad97c..5dfc282187 100644 --- a/tests/composition/reverseEngineeringEdgeCases.test.ts +++ b/tests/composition/reverseEngineeringEdgeCases.test.ts @@ -307,7 +307,13 @@ it("rejects malformed Rizin requests before touching the provider", async () => expect(result).toMatchObject({ ok: false, - error: { _tag: "AnalysisInputError", operation: "execute_rizin_command" }, + error: { + _tag: "AnalysisInputError", + operation: "execute_rizin_command", + issues: [ + { path: ["command"], reason: "missing_argument", expected: "string" }, + ], + }, }); expect(run).not.toHaveBeenCalled(); });