From 46217c69bc4d42a3ea2f72152d582eae84b8a936 Mon Sep 17 00:00:00 2001 From: morluto <76467478+morluto@users.noreply.github.com> Date: Fri, 23 Oct 2026 04:29:35 +0800 Subject: [PATCH] refactor(rizin): share validated command requests across admission and execution Resolve raw input at the application boundary and reuse its facts in both MCP admission paths. Keep cleanup, mutation retirement and execution ordering while retaining structured correction details for rejected commands. Validation: 186 focused tests, source checks, docs conformance and duplication gate passed. The new compiled CLI regression fails on the main baseline. Provider process controls use deterministic fixtures; no real Rizin verification is claimed. --- .../reverse/ReverseEngineeringService.ts | 39 +++++++++++++++---- src/domain/reverseEngineering.ts | 2 +- src/server/registerReverseEngineeringTools.ts | 27 ++++++------- .../cli/reverseEngineeringTargetInput.test.ts | 31 +++++++++++++++ .../mcp/reverseEngineeringPartialMcp.test.ts | 27 ++++++++----- .../reverseEngineeringEdgeCases.test.ts | 8 +++- 6 files changed, 102 insertions(+), 32 deletions(-) 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(); });