morluto/rea

Fork 0

mirror of https://github.com/morluto/rea.git synced 2026-11-05 07:47:29 +00:00

[PR #5477] [CLOSED] fix(mcp): reject relative compare_bundles paths #6935

New issue

Closed

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

bsakaguchi commented

2026-11-01 21:44:11 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/5477
Author: @Yashasm18
Created: 10/20/2026
Status: ❌ Closed

Base: main ← Head: fix/5450-compare-bundles-absolute-paths

📝 Commits (1)

2803756 fix(mcp): reject relative compare_bundles paths

📊 Changes

4 files changed (+84 additions, -2 deletions)

View changed files

📝 src/domain/bundleComparison.test.ts (+22 -0)
📝 src/domain/bundleComparison.ts (+12 -2)
📝 tests/boundary/cli/bundleComparisonCli.test.ts (+20 -0)
📝 tests/boundary/mcp/bundleComparisonMcp.test.ts (+30 -0)

📄 Description

Summary

Fixes #5450

Over MCP, compare_bundles was happy to take relative paths for left_bundle_path and right_bundle_path. It then resolved them against whatever directory the server was started from. That goes against the 6.0.0 rule that MCP filesystem inputs have to be absolute. import_evidence_bundle and export_evidence_bundle already enforce that rule, so this PR brings compare_bundles in line with them. The CLI still accepts relative paths, as before.

Problem and expected behavior

If you call compare_bundles with left_bundle_path: "evidence.json" on current main, the call succeeds. The server reads evidence.json from its own working directory. If no such file exists, you get a "file does not exist" error that shows the path resolved against the server's directory. That error never tells the agent the actual problem: the path needs to be absolute.

After this change, a relative value in either field fails validation before anything is read from disk. The error names the field and says an absolute path is required. Absolute comparisons work as before, and rea compare a.json b.json from a shell still works with relative paths.

Change and scope

The change is in src/domain/bundleComparison.ts. Both path fields now go through localPathStringSchema.refine(isAbsolute, ...). The message follows the same style as the import and export contracts, and I included the field name so it's clear which side is wrong. I also added a schema description saying relative paths are rejected, so clients can see the rule before they make a call.

The CLI isn't affected. bundleComparisonInputSchema is only used by the MCP compare_bundles contract, and the CLI compare command calls compareEvidenceBundlesCommand directly without going through this schema. I added a CLI test anyway, so a future refactor can't quietly change that.

I kept this PR to this one issue. It's a regular bug fix inside existing boundaries, so no scope decision is needed.

Contract and boundary impact

Semantic owner and earliest changed stage: bundleComparisonInputSchema, at MCP request validation.
Contribution scope fit and linked maintainer decision: bug fix within existing boundaries, not applicable.
CLI and MCP/tool-catalog contract: MCP compare_bundles now rejects relative paths, matching the documented 6.0.0 behavior. Both path fields get a schema description. The CLI is unchanged.
Provider, bridge, target-format, or platform compatibility: none. isAbsolute follows the host platform, the same as the import and export contracts.
Evidence, artifact, provenance, or reconstruction contract: none. Successful comparisons produce the same Evidence as before.
Target selection, authority, upstream behavior, and cleanup details: not applicable.
Generated metadata, package, or installation impact: the generated catalog picks up the new field descriptions. No generated files are committed.

Expected effects:

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

I added three tests:

In src/domain/bundleComparison.test.ts, I check that each field rejects a relative value on its own, and that the error points at that field.
In tests/boundary/mcp/bundleComparisonMcp.test.ts, I make real MCP calls with a relative name that doesn't exist on disk. The response has to be the absolute-path validation error, not a missing-file error. This shows validation happens before any read.
In tests/boundary/cli/bundleComparisonCli.test.ts, I run rea compare evidence.json evidence.json --json from the bundle's directory and check that it still returns unchanged.

I wrote the tests first and ran them against main. The two schema tests failed with expected true to be false. The MCP test failed with this response, which shows the relative path being resolved into the server's directory:

{"error":{"code":"invalid_request","category":"invalid_input","message":"Evidence file does not exist at the selected path. ...","details":{"operation":"read","reason":"missing","path":"<server cwd>/absent-bundle.json"}}}

With the fix in place, I ran the reproduction script from the issue, unchanged, against the built CLI:

relative-both    {"isError":true,"mentionsAbsolute":true}
relative-left    {"isError":true,"mentionsAbsolute":true}
relative-right   {"isError":true,"mentionsAbsolute":true}
absolute-control {"isError":false,"status":"unchanged","mentionsAbsolute":false}
relative-import  {"isError":true,"mentionsAbsolute":true}
absolute-import  {"isError":false,"mentionsAbsolute":false}

Running rea compare evidence.json evidence.json --json from the fixture directory still returned status: "unchanged".

The main gap is that I only ran this on macOS. I didn't test Windows locally. The Windows path handling comes from node:path, the same as the import and export contracts, and CI covers Windows.

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

All on macOS arm64 with Node v22.23.1:

npm run test:focused on the bundle comparison unit, MCP and cancellation suites: 16 passed. Before the fix, the 3 new tests failed as shown above.
npm run test:focused -- tests/boundary/cli/bundleComparisonCli.test.ts: 2 passed.
npm run test:focused -- tests/acceptance/tools/localToolContracts.test.ts, together with both bundle comparison MCP suites: 12 passed.
npm run check:changed: 1339 tests passed, 3 skipped.
npm run check: passed, including metadata, module boundary and architecture guard checks.
npm run docs:check: passed.

Compatibility, safety, and release

Breaking changes or migration steps: none intended. Relying on relative compare_bundles paths over MCP was never documented, and it contradicted the 6.0.0 contract. If anyone did rely on it, they just need to pass absolute paths, which import_evidence_bundle already requires.
Real Hopper/Ghidra, browser, or OS coverage: no provider is involved. I only tested on macOS.
Package or release metadata impact: only the generated catalog descriptions.
Security, privacy, process, or containment review: this only narrows what the tool accepts. A relative name can no longer pick up a same-named file from wherever the server was launched.

Author readiness

Human owner: @Yashasm18

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.

_{🔄 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/5477 **Author:** [@Yashasm18](https://github.com/Yashasm18) **Created:** 10/20/2026 **Status:** ❌ Closed **Base:** `main` ← **Head:** `fix/5450-compare-bundles-absolute-paths` --- ### 📝 Commits (1) - [`2803756`](https://github.com/morluto/rea/commit/2803756697ef2c583eecc142ccfa87598159c61d) fix(mcp): reject relative compare_bundles paths ### 📊 Changes **4 files changed** (+84 additions, -2 deletions) <details> <summary>View changed files</summary> 📝 `src/domain/bundleComparison.test.ts` (+22 -0) 📝 `src/domain/bundleComparison.ts` (+12 -2) 📝 `tests/boundary/cli/bundleComparisonCli.test.ts` (+20 -0) 📝 `tests/boundary/mcp/bundleComparisonMcp.test.ts` (+30 -0) </details> ### 📄 Description ## Summary Fixes #5450 Over MCP, `compare_bundles` was happy to take relative paths for `left_bundle_path` and `right_bundle_path`. It then resolved them against whatever directory the server was started from. That goes against the 6.0.0 rule that MCP filesystem inputs have to be absolute. `import_evidence_bundle` and `export_evidence_bundle` already enforce that rule, so this PR brings `compare_bundles` in line with them. The CLI still accepts relative paths, as before. ## Problem and expected behavior If you call `compare_bundles` with `left_bundle_path: "evidence.json"` on current main, the call succeeds. The server reads `evidence.json` from its own working directory. If no such file exists, you get a "file does not exist" error that shows the path resolved against the server's directory. That error never tells the agent the actual problem: the path needs to be absolute. After this change, a relative value in either field fails validation before anything is read from disk. The error names the field and says an absolute path is required. Absolute comparisons work as before, and `rea compare a.json b.json` from a shell still works with relative paths. ## Change and scope The change is in `src/domain/bundleComparison.ts`. Both path fields now go through `localPathStringSchema.refine(isAbsolute, ...)`. The message follows the same style as the import and export contracts, and I included the field name so it's clear which side is wrong. I also added a schema description saying relative paths are rejected, so clients can see the rule before they make a call. The CLI isn't affected. `bundleComparisonInputSchema` is only used by the MCP `compare_bundles` contract, and the CLI `compare` command calls `compareEvidenceBundlesCommand` directly without going through this schema. I added a CLI test anyway, so a future refactor can't quietly change that. I kept this PR to this one issue. It's a regular bug fix inside existing boundaries, so no scope decision is needed. ## Contract and boundary impact - Semantic owner and earliest changed stage: `bundleComparisonInputSchema`, at MCP request validation. - Contribution scope fit and linked maintainer decision: bug fix within existing boundaries, not applicable. - CLI and MCP/tool-catalog contract: MCP `compare_bundles` now rejects relative paths, matching the documented 6.0.0 behavior. Both path fields get a schema description. The CLI is unchanged. - Provider, bridge, target-format, or platform compatibility: none. `isAbsolute` follows the host platform, the same as the import and export contracts. - Evidence, artifact, provenance, or reconstruction contract: none. Successful comparisons produce the same Evidence as before. - Target selection, authority, upstream behavior, and cleanup details: not applicable. - Generated metadata, package, or installation impact: the generated catalog picks up the new field descriptions. No generated files are committed. Expected effects: - [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 I added three tests: - In `src/domain/bundleComparison.test.ts`, I check that each field rejects a relative value on its own, and that the error points at that field. - In `tests/boundary/mcp/bundleComparisonMcp.test.ts`, I make real MCP calls with a relative name that doesn't exist on disk. The response has to be the absolute-path validation error, not a missing-file error. This shows validation happens before any read. - In `tests/boundary/cli/bundleComparisonCli.test.ts`, I run `rea compare evidence.json evidence.json --json` from the bundle's directory and check that it still returns `unchanged`. I wrote the tests first and ran them against main. The two schema tests failed with `expected true to be false`. The MCP test failed with this response, which shows the relative path being resolved into the server's directory: ``` {"error":{"code":"invalid_request","category":"invalid_input","message":"Evidence file does not exist at the selected path. ...","details":{"operation":"read","reason":"missing","path":"<server cwd>/absent-bundle.json"}}} ``` With the fix in place, I ran the reproduction script from the issue, unchanged, against the built CLI: ``` relative-both {"isError":true,"mentionsAbsolute":true} relative-left {"isError":true,"mentionsAbsolute":true} relative-right {"isError":true,"mentionsAbsolute":true} absolute-control {"isError":false,"status":"unchanged","mentionsAbsolute":false} relative-import {"isError":true,"mentionsAbsolute":true} absolute-import {"isError":false,"mentionsAbsolute":false} ``` Running `rea compare evidence.json evidence.json --json` from the fixture directory still returned `status: "unchanged"`. The main gap is that I only ran this on macOS. I didn't test Windows locally. The Windows path handling comes from `node:path`, the same as the import and export contracts, and CI covers Windows. 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 All on macOS arm64 with Node v22.23.1: - `npm run test:focused` on the bundle comparison unit, MCP and cancellation suites: 16 passed. Before the fix, the 3 new tests failed as shown above. - `npm run test:focused -- tests/boundary/cli/bundleComparisonCli.test.ts`: 2 passed. - `npm run test:focused -- tests/acceptance/tools/localToolContracts.test.ts`, together with both bundle comparison MCP suites: 12 passed. - `npm run check:changed`: 1339 tests passed, 3 skipped. - `npm run check`: passed, including metadata, module boundary and architecture guard checks. - `npm run docs:check`: passed. ## Compatibility, safety, and release - Breaking changes or migration steps: none intended. Relying on relative `compare_bundles` paths over MCP was never documented, and it contradicted the 6.0.0 contract. If anyone did rely on it, they just need to pass absolute paths, which `import_evidence_bundle` already requires. - Real Hopper/Ghidra, browser, or OS coverage: no provider is involved. I only tested on macOS. - Package or release metadata impact: only the generated catalog descriptions. - Security, privacy, process, or containment review: this only narrows what the tool accepts. A relative name can no longer pick up a same-named file from wherever the server was launched. ## Author readiness Human owner: @Yashasm18 - [ ] 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 - [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. - [ ] 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-11-01 21:44:11 +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#6935

No description provided.

Rows
Columns