[PR #1904] [MERGED] fix(contracts): mark foreign execution and device mutation destructive #4897

New issue

Closed

opened 2026-10-31 00:56:33 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-31 00:56:33 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/1904
Author: @larinyo
Created: 10/11/2026
Status: ✅ Merged
Merged: 10/11/2026
Merged by: @morluto

Base: main ← Head: pr/honest-destructive-hints

📝 Commits (1)

caa5af5 fix(contracts): mark foreign execution and device mutation destructive

📊 Changes

6 files changed (+239 additions, -9 deletions)

View changed files

📝 docs/mcp-contracts.md (+7 -1)
➕ src/contracts/toolEffects.test.ts (+156 -0)
📝 src/contracts/toolEffects.ts (+57 -7)
📝 src/contracts/toolOutputSchemaPrimitives.ts (+2 -0)
📝 tests/boundary/mcp/toolResult.test.ts (+2 -0)
📝 tests/boundary/mcp/toolSchemaValidity.test.ts (+15 -1)

📄 Description

Summary

MCP destructiveHint was derived only from mayDiscardData. As a result, tools that run the analysis target or an executable the caller selected (capture_process_scenario, observe_native_calls) and tools that change an attached Android device (install_adb_package, uninstall_adb_package, push_adb_file, start_adb_app, start_adb_activity, stop_adb_app) advertised destructiveHint: false. Clients use that hint to decide whether to prompt, so these tools could be auto-approved as "safe".

Problem and expected behavior

Smallest trigger: tools/list reports capture_process_scenario with destructiveHint: false, even though it runs a caller-chosen program.
Violated invariant: annotations should describe effects honestly (docs/mcp-contracts.md, "Tool annotations describe effects").
New behavior: destructiveHint = mayDiscardData || executesForeignCode || mutatesDevice. readOnlyHint semantics are unchanged.

Change and scope

ToolEffects gains executesForeignCode and mutatesDevice, both documented in code. All 170 tool effect records were audited.
Tools now declaring executesForeignCode:
- capture_process_scenario, observe_native_calls;
- capture_browser_scenario, capture_electron_scenario, capture_native_ui_scenario. These three were already destructive through mayDiscardData.
Tools now declaring mutatesDevice: the six ADB tools listed above.
Separate, droppable change: set_address_name, set_addresses_names, set_bookmark, set_comment, set_inline_comment and annotate_native_function now declare mayDiscardData: true. They can replace or clear existing analyst names and comments in the Hopper/Ghidra session database, which is data the caller cannot recover from REA. This flips their destructiveHint to true and is a judgment call separate from the execution/device fix. If maintainers prefer to keep metadata writers non-destructive, these six hunks can be dropped without affecting the rest. Those tools are not in the drift test's pinned set, so dropping them needs only a catalog regeneration.
New src/contracts/toolEffects.test.ts:
- every public tool is audited exactly once;
- every tool whose description mentions running, launching, executing, starting, installing or uninstalling declares one of the new flags or appears in a reasoned exception list. Stale exceptions also fail, so wording and effects cannot drift apart silently;
- the set of foreign-execution and device-mutating tools is pinned;
- destructiveHint must follow from each flag on its own;
- advertised annotations must equal the derived ones.
The binary_session effects output schema carries the new flags, and docs/mcp-contracts.md documents them.
Not included: writesCallerPath, or any execution or approval policy.

Contract and boundary impact

Semantic owner and earliest changed stage: src/contracts/toolEffects.ts.
CLI and MCP/tool-catalog contract: annotations change from destructiveHint: false to true for 14 tools (8 through the new flags, 6 through mayDiscardData). The binary_session effects record gains two boolean fields.
Provider, bridge, target-format, or platform compatibility: none.
Evidence, artifact, provenance, or reconstruction contract: none.
Process execution, authorization, cleanup, or containment impact: these are hints only, not enforcement. Clients that honour destructiveHint will now prompt for these tools.
Generated metadata, package, or installation impact: the MCP tool catalog was regenerated (.cache/, untracked). The product catalog check passed.

Evidence and regression coverage

Tests added or updated: src/contracts/toolEffects.test.ts (new), tests/boundary/mcp/toolResult.test.ts, tests/boundary/mcp/toolSchemaValidity.test.ts.
Base reproduction or other evidence: on the base, the generated catalog shows destructiveHint: false for the tools listed above.

Validation performed

npx vitest run src/contracts tests/boundary/mcp (after npm run build and npm run mcp-catalog:generate): 103 files, 414 passed, 5 skipped.
npm run test:fast: 378 files, 4338 passed, 6 skipped.
npm run docs:check: passed.
npx tsc --noEmit: clean. npx oxlint: 0 errors. npm run verify:module-boundaries: passed. npm run format: no changes.

Compatibility, safety, and release

Breaking changes or migration steps: not marked breaking. Annotations are hints, and the effects record only gains fields. Clients that auto-approve on destructiveHint: false will now prompt for these 14 tools, which is the purpose of the change.
Real Hopper/Ghidra, browser, or OS coverage: not applicable (metadata only).
Package or release metadata impact: none.
Security, privacy, process, or containment review: this makes client approval prompts more accurate.

Limits / not verified

Not checked against a specific client's approval UI (Claude Code, Codex, and others).

_{🔄 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/1904 **Author:** [@larinyo](https://github.com/larinyo) **Created:** 10/11/2026 **Status:** ✅ Merged **Merged:** 10/11/2026 **Merged by:** [@morluto](https://github.com/morluto) **Base:** `main` ← **Head:** `pr/honest-destructive-hints` --- ### 📝 Commits (1) - [`caa5af5`](https://github.com/morluto/rea/commit/caa5af5fcf3a746069b89c85cd6f17e9421cbe5d) fix(contracts): mark foreign execution and device mutation destructive ### 📊 Changes **6 files changed** (+239 additions, -9 deletions) <details> <summary>View changed files</summary> 📝 `docs/mcp-contracts.md` (+7 -1) ➕ `src/contracts/toolEffects.test.ts` (+156 -0) 📝 `src/contracts/toolEffects.ts` (+57 -7) 📝 `src/contracts/toolOutputSchemaPrimitives.ts` (+2 -0) 📝 `tests/boundary/mcp/toolResult.test.ts` (+2 -0) 📝 `tests/boundary/mcp/toolSchemaValidity.test.ts` (+15 -1) </details> ### 📄 Description ## Summary MCP `destructiveHint` was derived only from `mayDiscardData`. As a result, tools that run the analysis target or an executable the caller selected (`capture_process_scenario`, `observe_native_calls`) and tools that change an attached Android device (`install_adb_package`, `uninstall_adb_package`, `push_adb_file`, `start_adb_app`, `start_adb_activity`, `stop_adb_app`) advertised `destructiveHint: false`. Clients use that hint to decide whether to prompt, so these tools could be auto-approved as "safe". ## Problem and expected behavior - Smallest trigger: `tools/list` reports `capture_process_scenario` with `destructiveHint: false`, even though it runs a caller-chosen program. - Violated invariant: annotations should describe effects honestly (docs/mcp-contracts.md, "Tool annotations describe effects"). - New behavior: `destructiveHint = mayDiscardData || executesForeignCode || mutatesDevice`. `readOnlyHint` semantics are unchanged. ## Change and scope - `ToolEffects` gains `executesForeignCode` and `mutatesDevice`, both documented in code. All 170 tool effect records were audited. - Tools now declaring `executesForeignCode`: - `capture_process_scenario`, `observe_native_calls`; - `capture_browser_scenario`, `capture_electron_scenario`, `capture_native_ui_scenario`. These three were already destructive through `mayDiscardData`. - Tools now declaring `mutatesDevice`: the six ADB tools listed above. - **Separate, droppable change:** `set_address_name`, `set_addresses_names`, `set_bookmark`, `set_comment`, `set_inline_comment` and `annotate_native_function` now declare `mayDiscardData: true`. They can replace or clear existing analyst names and comments in the Hopper/Ghidra session database, which is data the caller cannot recover from REA. This flips their `destructiveHint` to true and is a judgment call separate from the execution/device fix. If maintainers prefer to keep metadata writers non-destructive, these six hunks can be dropped without affecting the rest. Those tools are not in the drift test's pinned set, so dropping them needs only a catalog regeneration. - New `src/contracts/toolEffects.test.ts`: - every public tool is audited exactly once; - every tool whose description mentions running, launching, executing, starting, installing or uninstalling declares one of the new flags or appears in a reasoned exception list. Stale exceptions also fail, so wording and effects cannot drift apart silently; - the set of foreign-execution and device-mutating tools is pinned; - `destructiveHint` must follow from each flag on its own; - advertised annotations must equal the derived ones. - The `binary_session` effects output schema carries the new flags, and `docs/mcp-contracts.md` documents them. - Not included: `writesCallerPath`, or any execution or approval policy. ## Contract and boundary impact - Semantic owner and earliest changed stage: `src/contracts/toolEffects.ts`. - CLI and MCP/tool-catalog contract: annotations change from `destructiveHint: false` to `true` for 14 tools (8 through the new flags, 6 through `mayDiscardData`). The `binary_session` effects record gains two boolean fields. - Provider, bridge, target-format, or platform compatibility: none. - Evidence, artifact, provenance, or reconstruction contract: none. - Process execution, authorization, cleanup, or containment impact: these are hints only, not enforcement. Clients that honour `destructiveHint` will now prompt for these tools. - Generated metadata, package, or installation impact: the MCP tool catalog was regenerated (`.cache/`, untracked). The product catalog check passed. ## Evidence and regression coverage - Tests added or updated: `src/contracts/toolEffects.test.ts` (new), `tests/boundary/mcp/toolResult.test.ts`, `tests/boundary/mcp/toolSchemaValidity.test.ts`. - Base reproduction or other evidence: on the base, the generated catalog shows `destructiveHint: false` for the tools listed above. ## Validation performed - `npx vitest run src/contracts tests/boundary/mcp` (after `npm run build` and `npm run mcp-catalog:generate`): 103 files, 414 passed, 5 skipped. - `npm run test:fast`: 378 files, 4338 passed, 6 skipped. - `npm run docs:check`: passed. - `npx tsc --noEmit`: clean. `npx oxlint`: 0 errors. `npm run verify:module-boundaries`: passed. `npm run format`: no changes. ## Compatibility, safety, and release - Breaking changes or migration steps: not marked breaking. Annotations are hints, and the effects record only gains fields. Clients that auto-approve on `destructiveHint: false` will now prompt for these 14 tools, which is the purpose of the change. - Real Hopper/Ghidra, browser, or OS coverage: not applicable (metadata only). - Package or release metadata impact: none. - Security, privacy, process, or containment review: this makes client approval prompts more accurate. ## Limits / not verified - Not checked against a specific client's approval UI (Claude Code, Codex, and others). --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-31 00:56:33 +00:00