morluto/rea

Fork 0

mirror of https://github.com/morluto/rea.git synced 2026-11-05 06:45:22 +00:00

[PR #5288] [CLOSED] fix(server): refuse unretained layout and JavaScript summaries before analysis #6859

New issue

Closed

opened 2026-11-01 21:43:35 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-11-01 21:43:35 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/5288
Author: @Y0UW1SH
Created: 10/19/2026
Status: ❌ Closed

Base: main ← Head: fix/binary-electron-summary-retention-preflight

📝 Commits (8)

20cbc88 fix(server): refuse unretained layout and JavaScript summaries before analysis
c3ed010 fix(server): validate layout paths before the summary retention preflight
a296285 refactor(server): inspect validated layout input without reparsing it
f060f2d fix(server): keep cancellation ahead of raw layout input validation
d34c928 Merge remote-tracking branch 'upstream/main' into fix/binary-electron-summary-retention-preflight
c0a211c Merge remote-tracking branch 'upstream/main' into fix/binary-electron-summary-retention-preflight
1b2b8f9 Merge remote-tracking branch 'origin/main' into codex/blockers-5288
74aa4f8 Merge remote-tracking branch 'upstream/main' into fix/binary-electron-summary-retention-preflight

📊 Changes

3 files changed (+166 additions, -9 deletions)

View changed files

📝 docs/javascript-artifact-reconstruction.md (+2 -1)
📝 src/application/binaryDiagnostics/BinaryLayoutService.test.ts (+30 -0)
📝 tests/boundary/mcp/analysisSummaryDetailMcp.test.ts (+134 -8)

📄 Description

Summary

inspect_binary_layout and analyze_javascript_application with detail: "summary" ran the complete layout inspection or application analysis before checking whether the server can retain Evidence. On a fixed MCP server without session retention (the embedder/test seam createServer({ kind: "fixed", ... })), a successful analysis was followed by capability_unavailable, and a failing analysis replaced that diagnostic with its own error, so the caller never learned that selecting detail: "complete" is the fix. Both handlers now check retention before any analysis work, as the EVM, pcap, network-capture and (with #5274) WASM handlers do. Refs #5100.

No separate issue is linked: #5100 reports this ordering for inspect_wasm_artifact, and its fix #5274 deliberately left these two sibling handlers for a follow-up to keep one outcome per PR. This PR is that follow-up; the defect, trigger and expected contract are the ones described in #5100, applied to the two remaining detail-bearing handlers that call their analysis directly.

Problem and expected behavior

Trigger: a server created without a session (no recordEvidence/evidenceById), then inspect_binary_layout or analyze_javascript_application with detail: "summary".

Observed on main (1c2c2f1e7):

inspect_binary_layout: the layout provider was invoked once per summary request before the refusal (expect(inspect).not.toHaveBeenCalled() fails with one call carrying the requested path).
analyze_javascript_application: the application analysis started before the refusal. When the analysis itself failed (on the Windows host used here it fails without the packaged native controls artifact), the summary response was that analysis error (capability_unavailable, category unsupported_provider, "This analysis is unavailable for the current target…") instead of the retention diagnostic.

Expected: requireAnalysisViewRetention documents itself as "Fail before analysis when selected delivery cannot keep its complete source", docs/mcp-contracts.md describes one shared detail contract for all six tools, and registerEvmTools, registerPcapTools, registerWebNetworkCaptureTool and the shared retainedSummaryDelivery.preflight all refuse before analysis. Both handlers should refuse with the same capability_unavailable diagnostic and no analysis work, and complete delivery (omitted or explicit detail: "complete") should keep working on the same server.

Change and scope

src/server/registerBinaryDiagnosticsTools.ts: inside the logged operation, the handler first calls service.validate(request) so malformed paths keep their field-specific invalid_request; then a detail: "summary" request runs requireAnalysisViewRetention(contract.name, retention) and returns its error when the server retains no Evidence; only then does service.inspectValidated run with the already-validated input. The summary projection after a successful inspection reuses the same retention object.
src/application/binaryDiagnostics/BinaryLayoutService.ts: the schema parse and absolute-path check move into a public validate(); the raw entry points inspect() and inspectAndClose() validate once and delegate, and the new inspectValidated() runs the inspection for input that validate() already admitted, so the rules stay in one place and the handler never reparses; the raw entry points still check cancellation before validating, as the pre-change service did (follow-up commits c3ed010fb, a29628592 and f060f2d1f, matching the review outcome on #5274).
src/server/registerElectronTools.ts: the analyze_javascript_application callback runs the same preflight with the registration's recordEvidence/evidenceById before analyzeJavaScriptApplicationValidated. No validate step is needed there: the analysis resolves input_path rather than rejecting it, so the preflight pre-empts no field diagnostic.
docs/javascript-artifact-reconstruction.md: the "Read the analysis" paragraph now says the refusal happens before any analysis starts. docs/binary-diagnostics.md does not describe the refusal and is unchanged; the shared detail paragraph in docs/mcp-contracts.md is not touched here to avoid a conflicting hunk with #5274, which narrows its before-analysis claim to the tools that enforce it; once both land, a follow-up extends that sentence to inspect_binary_layout and analyze_javascript_application.
tests/boundary/mcp/analysisSummaryDetailMcp.test.ts: the fixed-server refusal test is replaced by two tests, one per tool (see below).

Intentionally not included: no shared helper for the four handlers that repeat this three-line preflight (registerEvmTools, registerWasmTools, and the two here); createEvidenceToolHandler and runElectronTool have different execution shapes, and folding the preflight into them would widen this fix beyond one outcome.

Maintainer scope decision: not applicable (ordinary fix within existing offline inspection and static analysis; no new provider, execution, injection or mutation).

Contract and boundary impact

Semantic owner and earliest changed stage: the two MCP handlers, after schema validation and inside the admitted operation, before the provider or analysis call.
Contribution scope fit and linked maintainer decision: existing offline layout inspection and static JavaScript analysis; none required.
CLI and MCP/tool-catalog contract: input and output schemas unchanged; the summary refusal keeps its existing code and text and now precedes analysis work. CLI is unaffected (CLI output is always complete).
Provider, bridge, target-format, or platform compatibility: none; the pwntools layout provider and the application analysis are reached only when the request can be fulfilled.
Evidence, artifact, provenance, or reconstruction contract: unchanged; no Evidence, partial observation or artifact scope is produced for a refused summary.
Target selection, authority, upstream behavior, and cleanup details: not applicable.
Generated metadata (docs/public/product-catalog.json), package, or installation impact: none (metadata:check passed inside npm run check).

Expected effects (check known effects; use documentation-only on its own).
Unchecked items do not establish absence; leave uncertainties visible.

Read existing files or captured data.
Launch or attach to a process.
Inject instrumentation or run scripts/code.
Change process memory, registers, files, or configuration.
Contact network services.
Recover protected code or work around protections.
Documentation or usability only; no target interaction.
Not sure yet; investigation or maintainer guidance needed.

Evidence and regression coverage

Tests added or updated: tests/boundary/mcp/analysisSummaryDetailMcp.test.ts, and src/application/binaryDiagnostics/BinaryLayoutService.test.ts gains "gives cancellation precedence over malformed raw input on every entry point" (aborted signal plus path: "relative.elf" on inspect() and inspectAndClose() returns AnalysisCancelledError with no provider call, while validate() alone returns AnalysisInputError; it failed on a29628592).
- "rejects malformed layout paths, refuses layout summary delivery before provider work when retention is unavailable, and keeps fixed-server complete delivery": against a fixed server with an injected vi.fn layout provider it asserts that summary returns capability_unavailable containing "does not retain Evidence" with the provider never called; a summary request with path: "relative.elf" returns invalid_request with the path issue, still with no provider call; omitted and explicit detail: "complete" both return the complete layout Evidence (two provider calls); after switching the provider to return an AnalysisInputError, a further summary request still returns capability_unavailable with "Select detail complete" and the call count stays at two.
- "refuses JavaScript summary delivery before analysis starts when retention is unavailable and keeps fixed-server complete delivery" (replaces "refuses summary detail when the server cannot retain the complete Evidence"): the application analysis has no injectable seam, and docs/testing.md keeps module mocks out of the shared-graph MCP boundary project, so the test observes the analysis through the request's MCP progress notifications: the first analysis phase reports progress before any artifact is read. It asserts that summary returns capability_unavailable containing "does not retain Evidence" with no progress notification; omitted and explicit detail: "complete" both return complete analyze_javascript_application Evidence for the caller-selected input_path, with the first progress message naming the analyze_javascript_application phase; a complete request for an ASAR file with an invalid header returns artifact_operation_failed (reason: "format", the same failure javascriptSourceSelection.test.ts already asserts); and a summary request for that same failing input still returns capability_unavailable with "Select detail complete" and adds no progress notification.
Base reproduction or other evidence: executed. On main (1c2c2f1e7) the file fails at the first provider assertion for the layout test:
```
AssertionError: expected "vi.fn()" to not be called at all, but actually been called 1 times
  1st vi.fn() call: [ { path: "/artifacts/view-source.elf" }, { signal: AbortSignal {…} } ]
```
and at the first assertion of the JavaScript test, where the summary response carried the analysis failure instead of the retention diagnostic:
```
- "message": StringContaining "does not retain Evidence",
+ "message": "This analysis is unavailable for the current target. Choose another analysis or target.",
```
With the fix, the layout test passes, and the JavaScript test passes its refusal assertions (typed diagnostic, zero progress notifications) and reaches the complete-delivery assertion; see the validation gap below. The malformed-path assertion added in c3ed010fb was run against 20cbc8878 and failed with the summary refusal (capability_unavailable) in place of the expected invalid_request; it passes on c3ed010fb.
User-visible CLI/MCP output: unchanged text. A fixed server now answers a summary request with the existing typed error before any analysis work: code: "capability_unavailable", message "This server does not retain Evidence. Select detail complete to receive the complete analysis inline." Complete delivery on the same server returns the complete Evidence as before.
Remaining proof gaps: the complete-delivery, invalid-ASAR and final-refusal assertions of the JavaScript test could not be executed on the Windows host used here, because the source checkout has no native/windows/build/ artifact and analyze_javascript_application therefore fails before analysis for every input (the pre-existing test "returns the summary of complete Evidence retained for later views" fails identically on unmodified main on this host). Those assertions rest on JavaScriptApplicationEvidence.parameters, the first progress.report in analyzeJavaScriptApplicationValidated, and the existing invalid-ASAR assertion in javascriptSourceSelection.test.ts; CI on Linux/macOS, and on Windows with the built artifact, executes them. Mocked layout provider only; no real pwntools run (the change does not touch provider code).

For evidence-bearing changes:

Observed, derived, and inferred claims remain distinguishable.
Artifact identity, source provenance, and failed attempts remain preserved.
Unsupported, incomplete, unavailable, or uncertain outcomes remain visible.

Validation performed

Windows 11 x64, Node 24, locked dependencies (npm ci), base main at 1c2c2f1e7, source checkout without native/windows/build/.

node node_modules/vitest/vitest.mjs run --project mcp-boundary tests/boundary/mcp/analysisSummaryDetailMcp.test.ts before the fix — 3 failed (the two new tests at the assertions quoted above, plus the pre-existing complete-delivery test, which fails on this host for the native-controls reason), 1 passed.
Same command after the fix — 2 passed (both layout tests), 2 failed (both JavaScript tests, each at the first complete-delivery call, with the native-controls capability_unavailable error, not at any retention assertion).
Follow-up (c3ed010fb): the same file — 2 passed (both layout tests), 2 failed (the JavaScript tests, same native-controls reason); the new malformed-path assertion against 20cbc8878 — failed as described above; node node_modules/vitest/vitest.mjs run src/application/binaryDiagnostics/BinaryLayoutService.test.ts — 10 passed; node node_modules/vitest/vitest.mjs run --project mcp-boundary tests/boundary/mcp/binaryDiagnosticsMcp.test.ts tests/boundary/mcp/analysisViewMcp.test.ts — 21 passed; npm run check:fast — passed; npx oxfmt --check on the three changed files — formatted.
Follow-up (a29628592, no reparse): the same three MCP files — 23 passed, 2 failed (the JavaScript tests, same native-controls reason); BinaryLayoutService.test.ts — 10 passed; npm run check:fast — passed; npx oxfmt — formatted.
Follow-up (f060f2d1f, cancellation precedence): BinaryLayoutService.test.ts — 11 passed (the new test failed against the a29628592 service with AnalysisInputError); npm run check:fast — passed; npx oxfmt — formatted.
npm run check (typecheck, lint:built, format:check, knip, metadata:check, architecture guards) — passed.
npx oxfmt --check on the four changed files — formatted.
Not run: Linux/macOS, Windows with the native controls artifact, real pwntools provider (REA_PWNTOOLS_PYTHON), node scripts/check-doc-facts.mjs (needs a build), npm run check:pr.

Compatibility, safety, and release

Breaking changes or migration steps: none. Callers that already selected detail: "complete" on a fixed server are unaffected; callers that selected summary there receive the same error, earlier and regardless of analysis outcome.
Real Hopper/Ghidra, browser, or OS coverage: not applicable; no external provider is involved in the changed path.
Package or release metadata impact: none.
Security, privacy, process, or containment review: the pwntools subprocess and the JavaScript analysis (artifact reads, semantic workers) are no longer started for a request that cannot be delivered; no new file, process or network access.

Author readiness

Human owner: @Y0UW1SH

I reviewed the final diff and relevant existing code, understand the change and its edge cases, and take responsibility for the complete PR, including AI-assisted work.
I checked the evidence above, confirmed it supports the current change, and recorded meaningful validation gaps.
I declare this PR ready for initial maintainer review.

Before merge

This PR addresses a concrete problem or an agreed enhancement.
Material scope and authority questions are resolved, and any required maintainer scope decision covers the final diff.
The PR has one focused outcome and the title follows type(scope): outcome.
Related issue is linked, or the reason for not linking one is stated above.
Tests cover changed observable behavior and meaningful failure paths.
Owning docs, contracts, and generated metadata are updated where needed.
User-visible CLI/MCP changes include representative output.
I checked the final diff for secrets, unrelated cleanup, and unsupported claims.

🤖 Generated with Claude Code

Maintainer blocker verification

Merged main 3dbe314ada9f6dcc994ca4655dc0faf082503847 without conflicts. On the resulting head, Node 24.18.0/npm 11.16.0, the focused BinaryLayoutService, analysisSummaryDetailMcp, binaryDiagnosticsMcp and analysisViewMcp suites passed 38/38; check:fast and docs:check passed. Cases cover cancellation and malformed-path precedence, zero analysis work after retention refusal, and complete delivery. Hosted new-head checks were still queued/running after push.

_{🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.}

## 📋 Pull Request Information **Original PR:** https://github.com/morluto/rea/pull/5288 **Author:** [@Y0UW1SH](https://github.com/Y0UW1SH) **Created:** 10/19/2026 **Status:** ❌ Closed **Base:** `main` ← **Head:** `fix/binary-electron-summary-retention-preflight` --- ### 📝 Commits (8) - [`20cbc88`](https://github.com/morluto/rea/commit/20cbc88783ac904225f05e9eda3fe3fced888472) fix(server): refuse unretained layout and JavaScript summaries before analysis - [`c3ed010`](https://github.com/morluto/rea/commit/c3ed010fb33c982fbffd28699f9476b715387877) fix(server): validate layout paths before the summary retention preflight - [`a296285`](https://github.com/morluto/rea/commit/a2962859248a43b7b8292f77c56d17fc011117e4) refactor(server): inspect validated layout input without reparsing it - [`f060f2d`](https://github.com/morluto/rea/commit/f060f2d1fd179951b6c79abe1643b5d9ed712dfc) fix(server): keep cancellation ahead of raw layout input validation - [`d34c928`](https://github.com/morluto/rea/commit/d34c92867ddd198fc0d3a7524f1f030cec330dd4) Merge remote-tracking branch 'upstream/main' into fix/binary-electron-summary-retention-preflight - [`c0a211c`](https://github.com/morluto/rea/commit/c0a211cc1f27642685cb4d2d7264934dea0e0aca) Merge remote-tracking branch 'upstream/main' into fix/binary-electron-summary-retention-preflight - [`1b2b8f9`](https://github.com/morluto/rea/commit/1b2b8f9c5a31a41e6c85c6e13f09ea61249110a1) Merge remote-tracking branch 'origin/main' into codex/blockers-5288 - [`74aa4f8`](https://github.com/morluto/rea/commit/74aa4f8fe18f50af3b6091b4f086b38abc83f9c4) Merge remote-tracking branch 'upstream/main' into fix/binary-electron-summary-retention-preflight ### 📊 Changes **3 files changed** (+166 additions, -9 deletions) <details> <summary>View changed files</summary> 📝 `docs/javascript-artifact-reconstruction.md` (+2 -1) 📝 `src/application/binaryDiagnostics/BinaryLayoutService.test.ts` (+30 -0) 📝 `tests/boundary/mcp/analysisSummaryDetailMcp.test.ts` (+134 -8) </details> ### 📄 Description ## Summary `inspect_binary_layout` and `analyze_javascript_application` with `detail: "summary"` ran the complete layout inspection or application analysis before checking whether the server can retain Evidence. On a fixed MCP server without session retention (the embedder/test seam `createServer({ kind: "fixed", ... })`), a successful analysis was followed by `capability_unavailable`, and a failing analysis replaced that diagnostic with its own error, so the caller never learned that selecting `detail: "complete"` is the fix. Both handlers now check retention before any analysis work, as the EVM, pcap, network-capture and (with #5274) WASM handlers do. Refs #5100. No separate issue is linked: #5100 reports this ordering for `inspect_wasm_artifact`, and its fix #5274 deliberately left these two sibling handlers for a follow-up to keep one outcome per PR. This PR is that follow-up; the defect, trigger and expected contract are the ones described in #5100, applied to the two remaining `detail`-bearing handlers that call their analysis directly. ## Problem and expected behavior Trigger: a server created without a session (no `recordEvidence`/`evidenceById`), then `inspect_binary_layout` or `analyze_javascript_application` with `detail: "summary"`. Observed on `main` (`1c2c2f1e7`): - `inspect_binary_layout`: the layout provider was invoked once per summary request before the refusal (`expect(inspect).not.toHaveBeenCalled()` fails with one call carrying the requested path). - `analyze_javascript_application`: the application analysis started before the refusal. When the analysis itself failed (on the Windows host used here it fails without the packaged native controls artifact), the summary response was that analysis error (`capability_unavailable`, category `unsupported_provider`, "This analysis is unavailable for the current target…") instead of the retention diagnostic. Expected: `requireAnalysisViewRetention` documents itself as "Fail before analysis when selected delivery cannot keep its complete source", `docs/mcp-contracts.md` describes one shared `detail` contract for all six tools, and `registerEvmTools`, `registerPcapTools`, `registerWebNetworkCaptureTool` and the shared `retainedSummaryDelivery.preflight` all refuse before analysis. Both handlers should refuse with the same `capability_unavailable` diagnostic and no analysis work, and complete delivery (omitted or explicit `detail: "complete"`) should keep working on the same server. ## Change and scope - `src/server/registerBinaryDiagnosticsTools.ts`: inside the logged operation, the handler first calls `service.validate(request)` so malformed paths keep their field-specific `invalid_request`; then a `detail: "summary"` request runs `requireAnalysisViewRetention(contract.name, retention)` and returns its error when the server retains no Evidence; only then does `service.inspectValidated` run with the already-validated input. The summary projection after a successful inspection reuses the same `retention` object. - `src/application/binaryDiagnostics/BinaryLayoutService.ts`: the schema parse and absolute-path check move into a public `validate()`; the raw entry points `inspect()` and `inspectAndClose()` validate once and delegate, and the new `inspectValidated()` runs the inspection for input that `validate()` already admitted, so the rules stay in one place and the handler never reparses; the raw entry points still check cancellation before validating, as the pre-change service did (follow-up commits `c3ed010fb`, `a29628592` and `f060f2d1f`, matching the review outcome on #5274). - `src/server/registerElectronTools.ts`: the `analyze_javascript_application` callback runs the same preflight with the registration's `recordEvidence`/`evidenceById` before `analyzeJavaScriptApplicationValidated`. No validate step is needed there: the analysis resolves `input_path` rather than rejecting it, so the preflight pre-empts no field diagnostic. - `docs/javascript-artifact-reconstruction.md`: the "Read the analysis" paragraph now says the refusal happens before any analysis starts. `docs/binary-diagnostics.md` does not describe the refusal and is unchanged; the shared `detail` paragraph in `docs/mcp-contracts.md` is not touched here to avoid a conflicting hunk with #5274, which narrows its before-analysis claim to the tools that enforce it; once both land, a follow-up extends that sentence to `inspect_binary_layout` and `analyze_javascript_application`. - `tests/boundary/mcp/analysisSummaryDetailMcp.test.ts`: the fixed-server refusal test is replaced by two tests, one per tool (see below). Intentionally not included: no shared helper for the four handlers that repeat this three-line preflight (`registerEvmTools`, `registerWasmTools`, and the two here); `createEvidenceToolHandler` and `runElectronTool` have different execution shapes, and folding the preflight into them would widen this fix beyond one outcome. Maintainer scope decision: not applicable (ordinary fix within existing offline inspection and static analysis; no new provider, execution, injection or mutation). ## Contract and boundary impact - Semantic owner and earliest changed stage: the two MCP handlers, after schema validation and inside the admitted operation, before the provider or analysis call. - Contribution scope fit and linked maintainer decision: existing offline layout inspection and static JavaScript analysis; none required. - CLI and MCP/tool-catalog contract: input and output schemas unchanged; the summary refusal keeps its existing code and text and now precedes analysis work. CLI is unaffected (CLI output is always complete). - Provider, bridge, target-format, or platform compatibility: none; the pwntools layout provider and the application analysis are reached only when the request can be fulfilled. - Evidence, artifact, provenance, or reconstruction contract: unchanged; no Evidence, partial observation or artifact scope is produced for a refused summary. - Target selection, authority, upstream behavior, and cleanup details: not applicable. - Generated metadata (`docs/public/product-catalog.json`), package, or installation impact: none (`metadata:check` passed inside `npm run check`). Expected effects (check known effects; use documentation-only on its own). Unchecked items do not establish absence; leave uncertainties visible. - [x] Read existing files or captured data. - [ ] Launch or attach to a process. - [ ] Inject instrumentation or run scripts/code. - [ ] Change process memory, registers, files, or configuration. - [ ] Contact network services. - [ ] Recover protected code or work around protections. - [ ] Documentation or usability only; no target interaction. - [ ] Not sure yet; investigation or maintainer guidance needed. ## Evidence and regression coverage - Tests added or updated: `tests/boundary/mcp/analysisSummaryDetailMcp.test.ts`, and `src/application/binaryDiagnostics/BinaryLayoutService.test.ts` gains "gives cancellation precedence over malformed raw input on every entry point" (aborted signal plus `path: "relative.elf"` on `inspect()` and `inspectAndClose()` returns `AnalysisCancelledError` with no provider call, while `validate()` alone returns `AnalysisInputError`; it failed on `a29628592`). - "rejects malformed layout paths, refuses layout summary delivery before provider work when retention is unavailable, and keeps fixed-server complete delivery": against a fixed server with an injected `vi.fn` layout provider it asserts that summary returns `capability_unavailable` containing "does not retain Evidence" with the provider never called; a summary request with `path: "relative.elf"` returns `invalid_request` with the `path` issue, still with no provider call; omitted and explicit `detail: "complete"` both return the complete layout Evidence (two provider calls); after switching the provider to return an `AnalysisInputError`, a further summary request still returns `capability_unavailable` with "Select detail complete" and the call count stays at two. - "refuses JavaScript summary delivery before analysis starts when retention is unavailable and keeps fixed-server complete delivery" (replaces "refuses summary detail when the server cannot retain the complete Evidence"): the application analysis has no injectable seam, and `docs/testing.md` keeps module mocks out of the shared-graph MCP boundary project, so the test observes the analysis through the request's MCP progress notifications: the first analysis phase reports progress before any artifact is read. It asserts that summary returns `capability_unavailable` containing "does not retain Evidence" with no progress notification; omitted and explicit `detail: "complete"` both return complete `analyze_javascript_application` Evidence for the caller-selected `input_path`, with the first progress message naming the `analyze_javascript_application` phase; a complete request for an ASAR file with an invalid header returns `artifact_operation_failed` (`reason: "format"`, the same failure `javascriptSourceSelection.test.ts` already asserts); and a summary request for that same failing input still returns `capability_unavailable` with "Select detail complete" and adds no progress notification. - Base reproduction or other evidence: executed. On `main` (`1c2c2f1e7`) the file fails at the first provider assertion for the layout test: ```text AssertionError: expected "vi.fn()" to not be called at all, but actually been called 1 times 1st vi.fn() call: [ { path: "/artifacts/view-source.elf" }, { signal: AbortSignal {…} } ] ``` and at the first assertion of the JavaScript test, where the summary response carried the analysis failure instead of the retention diagnostic: ```text - "message": StringContaining "does not retain Evidence", + "message": "This analysis is unavailable for the current target. Choose another analysis or target.", ``` With the fix, the layout test passes, and the JavaScript test passes its refusal assertions (typed diagnostic, zero progress notifications) and reaches the complete-delivery assertion; see the validation gap below. The malformed-path assertion added in `c3ed010fb` was run against `20cbc8878` and failed with the summary refusal (`capability_unavailable`) in place of the expected `invalid_request`; it passes on `c3ed010fb`. - User-visible CLI/MCP output: unchanged text. A fixed server now answers a summary request with the existing typed error before any analysis work: `code: "capability_unavailable"`, message "This server does not retain Evidence. Select detail complete to receive the complete analysis inline." Complete delivery on the same server returns the complete Evidence as before. - Remaining proof gaps: the complete-delivery, invalid-ASAR and final-refusal assertions of the JavaScript test could not be executed on the Windows host used here, because the source checkout has no `native/windows/build/` artifact and `analyze_javascript_application` therefore fails before analysis for every input (the pre-existing test "returns the summary of complete Evidence retained for later views" fails identically on unmodified `main` on this host). Those assertions rest on `JavaScriptApplicationEvidence.parameters`, the first `progress.report` in `analyzeJavaScriptApplicationValidated`, and the existing invalid-ASAR assertion in `javascriptSourceSelection.test.ts`; CI on Linux/macOS, and on Windows with the built artifact, executes them. Mocked layout provider only; no real pwntools run (the change does not touch provider code). For evidence-bearing changes: - [x] Observed, derived, and inferred claims remain distinguishable. - [x] Artifact identity, source provenance, and failed attempts remain preserved. - [x] Unsupported, incomplete, unavailable, or uncertain outcomes remain visible. ## Validation performed Windows 11 x64, Node 24, locked dependencies (`npm ci`), base `main` at `1c2c2f1e7`, source checkout without `native/windows/build/`. - `node node_modules/vitest/vitest.mjs run --project mcp-boundary tests/boundary/mcp/analysisSummaryDetailMcp.test.ts` before the fix — 3 failed (the two new tests at the assertions quoted above, plus the pre-existing complete-delivery test, which fails on this host for the native-controls reason), 1 passed. - Same command after the fix — 2 passed (both layout tests), 2 failed (both JavaScript tests, each at the first complete-delivery call, with the native-controls `capability_unavailable` error, not at any retention assertion). - Follow-up (`c3ed010fb`): the same file — 2 passed (both layout tests), 2 failed (the JavaScript tests, same native-controls reason); the new malformed-path assertion against `20cbc8878` — failed as described above; `node node_modules/vitest/vitest.mjs run src/application/binaryDiagnostics/BinaryLayoutService.test.ts` — 10 passed; `node node_modules/vitest/vitest.mjs run --project mcp-boundary tests/boundary/mcp/binaryDiagnosticsMcp.test.ts tests/boundary/mcp/analysisViewMcp.test.ts` — 21 passed; `npm run check:fast` — passed; `npx oxfmt --check` on the three changed files — formatted. - Follow-up (`a29628592`, no reparse): the same three MCP files — 23 passed, 2 failed (the JavaScript tests, same native-controls reason); `BinaryLayoutService.test.ts` — 10 passed; `npm run check:fast` — passed; `npx oxfmt` — formatted. - Follow-up (`f060f2d1f`, cancellation precedence): `BinaryLayoutService.test.ts` — 11 passed (the new test failed against the `a29628592` service with `AnalysisInputError`); `npm run check:fast` — passed; `npx oxfmt` — formatted. - `npm run check` (typecheck, lint:built, format:check, knip, metadata:check, architecture guards) — passed. - `npx oxfmt --check` on the four changed files — formatted. - Not run: Linux/macOS, Windows with the native controls artifact, real pwntools provider (`REA_PWNTOOLS_PYTHON`), `node scripts/check-doc-facts.mjs` (needs a build), `npm run check:pr`. ## Compatibility, safety, and release - Breaking changes or migration steps: none. Callers that already selected `detail: "complete"` on a fixed server are unaffected; callers that selected `summary` there receive the same error, earlier and regardless of analysis outcome. - Real Hopper/Ghidra, browser, or OS coverage: not applicable; no external provider is involved in the changed path. - Package or release metadata impact: none. - Security, privacy, process, or containment review: the pwntools subprocess and the JavaScript analysis (artifact reads, semantic workers) are no longer started for a request that cannot be delivered; no new file, process or network access. ## Author readiness Human owner: @Y0UW1SH - [x] I reviewed the final diff and relevant existing code, understand the change and its edge cases, and take responsibility for the complete PR, including AI-assisted work. - [x] I checked the evidence above, confirmed it supports the current change, and recorded meaningful validation gaps. - [x] I declare this PR ready for initial maintainer review. ## Before merge - [x] This PR addresses a concrete problem or an agreed enhancement. - [x] Material scope and authority questions are resolved, and any required maintainer scope decision covers the final diff. - [x] The PR has one focused outcome and the title follows `type(scope): outcome`. - [x] Related issue is linked, or the reason for not linking one is stated above. - [x] Tests cover changed observable behavior and meaningful failure paths. - [x] Owning docs, contracts, and generated metadata are updated where needed. - [x] User-visible CLI/MCP changes include representative output. - [x] I checked the final diff for secrets, unrelated cleanup, and unsupported claims. 🤖 Generated with [Claude Code](https://claude.com/claude-code)  ### Maintainer blocker verification Merged main `3dbe314ada9f6dcc994ca4655dc0faf082503847` without conflicts. On the resulting head, Node 24.18.0/npm 11.16.0, the focused BinaryLayoutService, analysisSummaryDetailMcp, binaryDiagnosticsMcp and analysisViewMcp suites passed 38/38; check:fast and docs:check passed. Cases cover cancellation and malformed-path precedence, zero analysis work after retention refusal, and complete delivery. Hosted new-head checks were still queued/running after push. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-11-01 21:43:35 +00:00

closed this issue
added the
pull-request
label

No milestone

No project

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

morluto/rea#6859

No description provided.

Rows
Columns