morluto/rea

Fork 0

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

[PR #2398] [MERGED] fix(server): mark retained partial-observation errors as target content #5199

New issue

Closed

opened 2026-10-31 01:04:45 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-31 01:04:45 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/2398
Author: @jbernous
Created: 10/12/2026
Status: ✅ Merged
Merged: 10/13/2026
Merged by: @morluto

Base: main ← Head: fix/contract-tool-handler-target-content

📝 Commits (1)

7d88cdb fix(server): mark retained partial-observation errors as target content

📊 Changes

2 files changed (+87 additions, -0 deletions)

View changed files

📝 src/server/contractToolHandler.ts (+1 -0)
➕ tests/boundary/mcp/contractToolHandlerTargetContent.test.ts (+86 -0)

📄 Description

Summary

Restore the targetContent argument at the shared Evidence handler's recorded partial-observation call site (src/server/contractToolHandler.ts), matching the Electron call site.
Add a regression test that fails on the base revision and passes with the fix.

No linked issue: this restores a broken main gate (one omitted argument at one call site) rather than requesting an enhancement. CONTRIBUTING.md scopes routine fixes within existing boundaries without a prior maintainer decision — happy to open and link an issue instead if you prefer that route.

Problem and expected behavior

Smallest trigger: any tool registered through createEvidenceToolHandler (inspect_packet_capture, inspect_signal_recording, EVM/Go/analysis-view tools) that fails while retaining a partial observation.

The handler records the partial Evidence, then calls toRecordedPartialErrorToolResult(error, evidenceId, recorded).
That signature requires a fourth targetContent: boolean (src/server/toolResult.ts:109), introduced by the untrusted target-content framing.

Violated invariant: a failure can carry target-derived text (message, captured output, or a partial observation), so it must be marked and budgeted exactly like that tool's successful results. With the argument omitted, the delivery neither adds the provenance marker nor counts the marker's bytes in its wire budget.

The omitted marker is masked on the wire today, because EvidenceMcpServer marks errors that bypass ToolResultDelivery; that fallback does not restore the budget accounting, so the delivered error can exceed the configured budget by the marker's bytes.

The same omission broke the type gate on main:

src/server/contractToolHandler.ts(69,38): error TS2554: Expected 4 arguments, but got 3.

This fails CI Static checks (npm run check:ci) and Build package (npm run test:prepare). Because boundary lanes build conservatively, it also blocks every compiled boundary test lane.

Change and scope

Pass contract.returnsTargetContent at the existing call site. ToolContract already declares readonly returnsTargetContent: boolean, and the same handler already passes contract to toEvidenceToolResult one call site below, so no new contract, plumbing, or abstraction is introduced.

Intentionally not included: a byte-boundary test for the undercounted budget (see remaining proof gaps).

Scope decision: not applicable — ordinary fix within existing boundaries; no new provider, target execution, injection, mutation, or protection recovery.

Contract and boundary impact

Semantic owner and earliest changed stage: ToolResultDelivery owns the marker and the wire budget; the change is at its call site inside the shared Evidence handler.
Contribution scope fit and linked maintainer decision: not applicable (ordinary fix).
CLI and MCP/tool-catalog contract: none; no schema, tool, or catalog change.
Provider, bridge, target-format, or platform compatibility: none; no provider behavior changes.
Evidence, artifact, provenance, or reconstruction contract: restores untrusted target-content framing for failures that retain partial Evidence.
Target selection, authority, upstream behavior, and cleanup details: none.
Generated metadata, package, or installation impact: none.

Expected effects: no new effect. The change supplies an existing argument to an existing delivery call, and the added test uses in-memory fixtures with no target interaction. Unchecked items below do not establish absence.

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/contractToolHandlerTargetContent.test.ts delivers a retained partial-observation failure through createEvidenceToolHandler and asserts the delivered result is marked as untrusted target content.
Base reproduction: on the base revision that test fails on the marker assertion with Received: undefined (the partial-observation assertion passes), and npm run typecheck fails with the TS2554 above. Upstream CI reproduces the same error on main in run 38308946902 (Static checks and Build package, Node 24.18.0).
User-visible CLI/MCP output: not applicable; the wire marker was already restored by the server fallback, so the observable change is the corrected budget accounting.
Remaining proof gaps: the regression test asserts the invariant at the producer (delivery) because the server fallback masks the marker at the transport. The undercounted TARGET_CONTENT_META_BYTES window is not pinned by a byte-boundary test.

For evidence-bearing changes: not applicable — this change adds no evidence claims. Observed, derived, and inferred facts remain distinguishable; the passed-through partial observation, its evidence_id, and the failed attempt are unchanged.

Validation performed

npm run check:fast — passes (fails on the base revision)
npm run typecheck — 0 errors (1 on the base revision)
npx vitest run tests/boundary/mcp/contractToolHandlerTargetContent.test.ts — 1 passed (fails on the base revision)
npm run lint — 0 errors (pre-existing warnings unchanged)
npm run format:check — all matched files use the correct format
npm run verify:test-discovery — all 996 test files discovered exactly once

Environment note: the local host runs Node 26.5.0 while .nvmrc pins 24.18.0 and CI uses 24.18.0. npm ci succeeds and the base failure reproduces identically on both, so the toolchain deviation is not the cause. No real Hopper/Ghidra, browser, or OS provider lane applies to this change.

Compatibility, safety, and release

Breaking changes or migration steps: none.
Real Hopper/Ghidra, browser, or OS coverage: not applicable; no provider behavior changed.
Package or release metadata impact: none.
Security, privacy, process, or containment review: the fix restores untrusted target-content framing and its budget accounting for failures that retain partial Evidence; it does not weaken containment.

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. (None needed: the documented framing contract is unchanged.)
User-visible CLI/MCP changes include representative output. (No new user-visible change: the marker was already restored on the wire by the server fallback.)
I checked the final diff for secrets, unrelated cleanup, and unsupported claims.

_{🔄 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/2398 **Author:** [@jbernous](https://github.com/jbernous) **Created:** 10/12/2026 **Status:** ✅ Merged **Merged:** 10/13/2026 **Merged by:** [@morluto](https://github.com/morluto) **Base:** `main` ← **Head:** `fix/contract-tool-handler-target-content` --- ### 📝 Commits (1) - [`7d88cdb`](https://github.com/morluto/rea/commit/7d88cdb2d9cf8d983a49124aa732b3e305c4f0a0) fix(server): mark retained partial-observation errors as target content ### 📊 Changes **2 files changed** (+87 additions, -0 deletions) <details> <summary>View changed files</summary> 📝 `src/server/contractToolHandler.ts` (+1 -0) ➕ `tests/boundary/mcp/contractToolHandlerTargetContent.test.ts` (+86 -0) </details> ### 📄 Description ## Summary - Restore the `targetContent` argument at the shared Evidence handler's recorded partial-observation call site (`src/server/contractToolHandler.ts`), matching the Electron call site. - Add a regression test that fails on the base revision and passes with the fix. No linked issue: this restores a broken `main` gate (one omitted argument at one call site) rather than requesting an enhancement. `CONTRIBUTING.md` scopes routine fixes within existing boundaries without a prior maintainer decision — happy to open and link an issue instead if you prefer that route. ## Problem and expected behavior Smallest trigger: any tool registered through `createEvidenceToolHandler` (`inspect_packet_capture`, `inspect_signal_recording`, EVM/Go/analysis-view tools) that fails while retaining a partial observation. 1. The handler records the partial Evidence, then calls `toRecordedPartialErrorToolResult(error, evidenceId, recorded)`. 2. That signature requires a fourth `targetContent: boolean` (`src/server/toolResult.ts:109`), introduced by the untrusted target-content framing. Violated invariant: a failure can carry target-derived text (message, captured output, or a partial observation), so it must be marked and budgeted exactly like that tool's successful results. With the argument omitted, the delivery neither adds the provenance marker nor counts the marker's bytes in its wire budget. The omitted marker is masked on the wire today, because `EvidenceMcpServer` marks errors that bypass `ToolResultDelivery`; that fallback does not restore the budget accounting, so the delivered error can exceed the configured budget by the marker's bytes. The same omission broke the type gate on `main`: ``` src/server/contractToolHandler.ts(69,38): error TS2554: Expected 4 arguments, but got 3. ``` This fails CI `Static checks` (`npm run check:ci`) and `Build package` (`npm run test:prepare`). Because boundary lanes build conservatively, it also blocks every compiled boundary test lane. ## Change and scope Pass `contract.returnsTargetContent` at the existing call site. `ToolContract` already declares `readonly returnsTargetContent: boolean`, and the same handler already passes `contract` to `toEvidenceToolResult` one call site below, so no new contract, plumbing, or abstraction is introduced. Intentionally not included: a byte-boundary test for the undercounted budget (see remaining proof gaps). Scope decision: not applicable — ordinary fix within existing boundaries; no new provider, target execution, injection, mutation, or protection recovery. ## Contract and boundary impact - Semantic owner and earliest changed stage: `ToolResultDelivery` owns the marker and the wire budget; the change is at its call site inside the shared Evidence handler. - Contribution scope fit and linked maintainer decision: not applicable (ordinary fix). - CLI and MCP/tool-catalog contract: none; no schema, tool, or catalog change. - Provider, bridge, target-format, or platform compatibility: none; no provider behavior changes. - Evidence, artifact, provenance, or reconstruction contract: restores untrusted target-content framing for failures that retain partial Evidence. - Target selection, authority, upstream behavior, and cleanup details: none. - Generated metadata, package, or installation impact: none. Expected effects: no new effect. The change supplies an existing argument to an existing delivery call, and the added test uses in-memory fixtures with no target interaction. Unchecked items below do not establish absence. - [ ] 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/contractToolHandlerTargetContent.test.ts` delivers a retained partial-observation failure through `createEvidenceToolHandler` and asserts the delivered result is marked as untrusted target content. - Base reproduction: on the base revision that test fails on the marker assertion with `Received: undefined` (the partial-observation assertion passes), and `npm run typecheck` fails with the `TS2554` above. Upstream CI reproduces the same error on `main` in run 38308946902 (`Static checks` and `Build package`, Node 24.18.0). - User-visible CLI/MCP output: not applicable; the wire marker was already restored by the server fallback, so the observable change is the corrected budget accounting. - Remaining proof gaps: the regression test asserts the invariant at the producer (delivery) because the server fallback masks the marker at the transport. The undercounted `TARGET_CONTENT_META_BYTES` window is not pinned by a byte-boundary test. For evidence-bearing changes: not applicable — this change adds no evidence claims. Observed, derived, and inferred facts remain distinguishable; the passed-through partial observation, its `evidence_id`, and the failed attempt are unchanged. ## Validation performed - `npm run check:fast` — passes (fails on the base revision) - `npm run typecheck` — 0 errors (1 on the base revision) - `npx vitest run tests/boundary/mcp/contractToolHandlerTargetContent.test.ts` — 1 passed (fails on the base revision) - `npm run lint` — 0 errors (pre-existing warnings unchanged) - `npm run format:check` — all matched files use the correct format - `npm run verify:test-discovery` — all 996 test files discovered exactly once Environment note: the local host runs Node 26.5.0 while `.nvmrc` pins 24.18.0 and CI uses 24.18.0. `npm ci` succeeds and the base failure reproduces identically on both, so the toolchain deviation is not the cause. No real Hopper/Ghidra, browser, or OS provider lane applies to this change. ## Compatibility, safety, and release - Breaking changes or migration steps: none. - Real Hopper/Ghidra, browser, or OS coverage: not applicable; no provider behavior changed. - Package or release metadata impact: none. - Security, privacy, process, or containment review: the fix restores untrusted target-content framing and its budget accounting for failures that retain partial Evidence; it does not weaken containment. ## 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. (None needed: the documented framing contract is unchanged.) - [x] User-visible CLI/MCP changes include representative output. (No new user-visible change: the marker was already restored on the wire by the server fallback.) - [x] I checked the final diff for secrets, unrelated cleanup, and unsupported claims. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-31 01:04:45 +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#5199

No description provided.

Rows
Columns