morluto/rea

Fork 0

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

[PR #948] [MERGED] fix(contracts)!: require absolute local paths for session filesystem inputs #2525

New issue

Closed

opened 2026-10-17 21:07:10 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-17 21:07:10 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/948
Author: @DeryFerd
Created: 10/7/2026
Status: ✅ Merged
Merged: 10/7/2026
Merged by: @morluto

Base: main ← Head: fix/absolute-session-path-inputs

📝 Commits (2)

14daf86 fix(contracts): require absolute local paths for session filesystem inputs
fd81434 Merge remote-tracking branch 'origin/main' into fix/pr948-review

📊 Changes

9 files changed (+195 additions, -11 deletions)

View changed files

📝 docs/product-catalog.json (+2 -2)
📝 docs/verification/managed-conformance-manifest.json (+2 -2)
📝 skills/reverse-engineer-anything/SKILL.md (+1 -1)
📝 src/contracts/managed/managedToolContracts.ts (+8 -3)
📝 src/contracts/sessionLifecycleInputs.ts (+15 -2)
➕ src/contracts/sessionPathInputs.test.ts (+121 -0)
📝 src/contracts/sessionToolContracts.ts (+11 -1)
📝 src/domain/localPath.ts (+4 -0)
📝 tests/boundary/mcp/sessionMcp.test.ts (+31 -0)

📄 Description

Summary

The session lifecycle, evidence export, and managed target input contracts accepted any non-empty string for their filesystem path fields, so a relative path passed over MCP was resolved against the MCP server process working directory rather than a location the caller actually named. This PR requires absolute host paths on those fields, reports a clear validation error when a caller sends a relative one, and advertises the rule on the property descriptions so the tool catalog carries it.

There is no linked issue. I did not find an existing open or closed issue or pull request that tracks this behavior; the closest related work is #51 (bundle import/export tooling) and #109 (snapshot persistence), neither of which validates where the path points.

Problem and expected behavior

Smallest trigger: an MCP client calls close_binary with snapshot_path: "analysis.json". The old contract accepted it (z.string().min(1)), writeTextFile then ran resolve(path), and the snapshot landed under whatever directory the server happened to start in. A caller that names ./analysis.json means their own location; the server silently substituted its own, which the caller cannot see or control. The same pattern applied to open_binary's staged snapshot read, export_evidence_bundle's destination, and the managed inspection path.

Expected behavior: the contract states the requirement, and a relative path fails fast at input validation with an error that names the constraint and shows what a valid value looks like, instead of resolving against an invisible working directory.

The new validation issue for close_binary looks like this:

{
  "code": "custom",
  "path": ["snapshot_path"],
  "message": "snapshot_path must be an absolute local filesystem path (for example /tmp/rea/analysis.json or C:\\rea\\analysis.json)"
}

Over MCP the tool call returns isError: true with that message in the tool error text. Windows callers can pass both C:\rea\analysis.json and C:/rea/analysis.json; on a Linux host a drive-letter string is rejected, which matches how that host resolves paths.

Change and scope

I added an isAbsoluteLocalPath helper to src/domain/localPath.ts (host node:path semantics) and applied it as a Zod refinement on four fields: snapshot_path in openBinaryInputSchema and closeBinaryInputSchema, path in exportEvidenceBundleInputSchema, and path in managedTargetInputSchema. Optional fields keep their existing optional behavior; the refinement only runs when the field is present.

The check lives in the contract layer because that is where this codebase already validates caller-supplied paths of this kind: WebScriptExportService requires absolute capture_path/output_directory, and WebModuleTraceService and WebSourceLocationService do the same for their inputs. The MCP server registers the Zod schema itself, so runtime validation includes the refinement, and Zod 4 refinements keep the schema an object, which means registration, .shape projection, and the advertised draft-07 projection are unchanged. I also updated the property descriptions, since a custom refinement cannot be expressed in JSON Schema, and regenerated the derived artifacts (generatedMcpToolCatalog, docs/product-catalog.json, and the bundled skill's catalog_digest).

Intentionally not included: import_evidence_bundle reads a caller-named bundle file and can be tightened the same way in a follow-up if that is wanted. CLI options are untouched because CLI paths are relative to the operator's own shell working directory, which is the directory the operator is looking at. This change also does not introduce a containment root; where a caller may write remains the caller's choice, and only the working-directory ambiguity goes away.

Contract and boundary impact

Semantic owner and earliest changed stage: contracts — sessionLifecycleInputs, sessionToolContracts, and managed/managedToolContracts input schemas; enforcement happens during MCP input validation, before any handler runs.
CLI and MCP/tool-catalog contract: MCP input validation tightens for open_binary, close_binary, export_evidence_bundle, inspect_managed_artifact, inspect_managed_members, and inspect_managed_native_boundaries when the path fields are present; property descriptions now state the absolute-path requirement; the generated catalog and product catalog are regenerated. CLI behavior is unchanged.
Provider, bridge, target-format, or platform compatibility: no provider, bridge, or target-format impact. Platform: the check follows host semantics, and Windows drive-letter paths in both separator styles are covered by tests that run on win32.
Evidence, artifact, provenance, or reconstruction contract: none — evidence content, identity, and provenance handling are unchanged; only the destination argument is validated earlier.
Process execution, authorization, cleanup, or containment impact: none — no process spawning, authorization, or cleanup behavior changes, and no new containment root is introduced.
Generated metadata (docs/product-catalog.json), package, or installation impact: yes — src/generatedMcpToolCatalog.ts, docs/product-catalog.json, and skills/reverse-engineer-anything/SKILL.md are regenerated in this PR; package contents are otherwise unchanged.

Evidence and regression coverage

Tests added or updated: src/contracts/sessionPathInputs.test.ts adds 16 cases covering relative and parent-relative rejection on each schema, acceptance of absolute paths, the optional-omission paths staying valid, and win32-only acceptance of C:\ and C:/ forms. tests/boundary/mcp/sessionMcp.test.ts adds an end-to-end case that calls export_evidence_bundle and close_binary with relative paths through a live client/server pair and asserts the error text.
Base reproduction or other evidence: on the base revision the six relative-path schema tests fail (the schemas accept the strings), and a relative snapshot_path over MCP writes under the server working directory. With the refinement the six pass and the MCP call fails with the message above.
User-visible CLI/MCP output (if applicable): the new rejection message shown in the problem section; existing examples in toolContractExamples and managedToolContracts already use absolute paths and continue to parse.
Remaining proof gaps: the win32 path-form cases execute only on Windows hosts (describe.runIf), so Linux CI skips them; they ran green on this Windows machine. import_evidence_bundle still accepts relative paths by design in this PR.
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

npm run typecheck — passed.
npx vitest run src/contracts --coverage.enabled=false — 5 files, 34 tests passed, including the 16 new path-input cases.
npx vitest run tests/boundary/mcp/sessionMcp.test.ts --coverage.enabled=false — the new path-boundary case passes; the pre-existing target-free MCP workflow case exceeded its 10s timeout on this Windows host, and the same timeout reproduces on unmodified main (10.4s vs 10.3s), so it is machine speed rather than this change.
npx vitest run tests/boundary/mcp/toolSchemaValidity.test.ts --coverage.enabled=false — 5 passed; advertised input/output schemas stay valid draft-07 for every canonical tool.
npx vitest run tests/acceptance/contractPresentation.test.ts --coverage.enabled=false — 2 passed.
npx vitest run tests/evaluation/moduleBoundaries.test.ts --coverage.enabled=false — 75 passed.
npx oxlint <changed files> — 0 errors.
npx oxfmt --check <changed files> — all clean.
node scripts/generate-mcp-tool-catalog.mjs --check, node scripts/generate-product-catalog.mjs --check, node scripts/generate-skill-metadata.mjs --check — all pass after regeneration.
npm run jscpd — passed (424 clones, within threshold).
npm run check — reaches format:check, which fails across the whole checkout including untouched files because this Windows worktree has CRLF line endings; knip aborts with an array-buffer allocation error, reproduced identically on unmodified main.
npm run docs:check — the generators this PR touches pass their --check gates; docs:check:built stops at the completion-ledger file, a pre-existing line-ending digest mismatch on this Windows checkout that also reproduces on unmodified main.
Real-provider lanes: not run — no provider, bridge, or browser code changed.

Compatibility, safety, and release

Breaking changes or migration steps: callers that relied on relative paths being resolved against the server working directory now get a validation error and must send an absolute path. Docs and contract examples already use absolute paths, and the error message shows a valid example for each platform. Worth a maintainer decision on whether this warrants a ! in the release note.
Real Hopper/Ghidra, browser, or OS coverage: not applicable — nothing under the provider, bridge, or browser trees changed. Path-form behavior was verified on a Windows host.
Package or release metadata impact: regenerated tool catalog, product catalog, and skill catalog digest are included; no dependency, lockfile, or install-lifecycle changes.
Security, privacy, process, or containment review: removes silent resolution of caller-supplied paths against an unobservable server working directory for snapshot writes, snapshot reads, evidence export, and managed target selection. It adds no new authority: REA is a local-only tool and the destination remains the caller's choice. Capability-token, environment redaction, and process execution behavior are untouched.

Review checklist

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/948 **Author:** [@DeryFerd](https://github.com/DeryFerd) **Created:** 10/7/2026 **Status:** ✅ Merged **Merged:** 10/7/2026 **Merged by:** [@morluto](https://github.com/morluto) **Base:** `main` ← **Head:** `fix/absolute-session-path-inputs` --- ### 📝 Commits (2) - [`14daf86`](https://github.com/morluto/rea/commit/14daf86949a4b9603e3b098d4b8e32d9cbdad079) fix(contracts): require absolute local paths for session filesystem inputs - [`fd81434`](https://github.com/morluto/rea/commit/fd81434fd2c76f0e5760b058767fa88487668f24) Merge remote-tracking branch 'origin/main' into fix/pr948-review ### 📊 Changes **9 files changed** (+195 additions, -11 deletions) <details> <summary>View changed files</summary> 📝 `docs/product-catalog.json` (+2 -2) 📝 `docs/verification/managed-conformance-manifest.json` (+2 -2) 📝 `skills/reverse-engineer-anything/SKILL.md` (+1 -1) 📝 `src/contracts/managed/managedToolContracts.ts` (+8 -3) 📝 `src/contracts/sessionLifecycleInputs.ts` (+15 -2) ➕ `src/contracts/sessionPathInputs.test.ts` (+121 -0) 📝 `src/contracts/sessionToolContracts.ts` (+11 -1) 📝 `src/domain/localPath.ts` (+4 -0) 📝 `tests/boundary/mcp/sessionMcp.test.ts` (+31 -0) </details> ### 📄 Description ## Summary The session lifecycle, evidence export, and managed target input contracts accepted any non-empty string for their filesystem path fields, so a relative path passed over MCP was resolved against the MCP server process working directory rather than a location the caller actually named. This PR requires absolute host paths on those fields, reports a clear validation error when a caller sends a relative one, and advertises the rule on the property descriptions so the tool catalog carries it. There is no linked issue. I did not find an existing open or closed issue or pull request that tracks this behavior; the closest related work is #51 (bundle import/export tooling) and #109 (snapshot persistence), neither of which validates where the path points. ## Problem and expected behavior Smallest trigger: an MCP client calls `close_binary` with `snapshot_path: "analysis.json"`. The old contract accepted it (`z.string().min(1)`), `writeTextFile` then ran `resolve(path)`, and the snapshot landed under whatever directory the server happened to start in. A caller that names `./analysis.json` means their own location; the server silently substituted its own, which the caller cannot see or control. The same pattern applied to `open_binary`'s staged snapshot read, `export_evidence_bundle`'s destination, and the managed inspection `path`. Expected behavior: the contract states the requirement, and a relative path fails fast at input validation with an error that names the constraint and shows what a valid value looks like, instead of resolving against an invisible working directory. The new validation issue for `close_binary` looks like this: ```json { "code": "custom", "path": ["snapshot_path"], "message": "snapshot_path must be an absolute local filesystem path (for example /tmp/rea/analysis.json or C:\\rea\\analysis.json)" } ``` Over MCP the tool call returns `isError: true` with that message in the tool error text. Windows callers can pass both `C:\rea\analysis.json` and `C:/rea/analysis.json`; on a Linux host a drive-letter string is rejected, which matches how that host resolves paths. ## Change and scope I added an `isAbsoluteLocalPath` helper to `src/domain/localPath.ts` (host `node:path` semantics) and applied it as a Zod refinement on four fields: `snapshot_path` in `openBinaryInputSchema` and `closeBinaryInputSchema`, `path` in `exportEvidenceBundleInputSchema`, and `path` in `managedTargetInputSchema`. Optional fields keep their existing optional behavior; the refinement only runs when the field is present. The check lives in the contract layer because that is where this codebase already validates caller-supplied paths of this kind: `WebScriptExportService` requires absolute `capture_path`/`output_directory`, and `WebModuleTraceService` and `WebSourceLocationService` do the same for their inputs. The MCP server registers the Zod schema itself, so runtime validation includes the refinement, and Zod 4 refinements keep the schema an object, which means registration, `.shape` projection, and the advertised draft-07 projection are unchanged. I also updated the property descriptions, since a custom refinement cannot be expressed in JSON Schema, and regenerated the derived artifacts (`generatedMcpToolCatalog`, `docs/product-catalog.json`, and the bundled skill's `catalog_digest`). Intentionally not included: `import_evidence_bundle` reads a caller-named bundle file and can be tightened the same way in a follow-up if that is wanted. CLI options are untouched because CLI paths are relative to the operator's own shell working directory, which is the directory the operator is looking at. This change also does not introduce a containment root; where a caller may write remains the caller's choice, and only the working-directory ambiguity goes away. ## Contract and boundary impact - Semantic owner and earliest changed stage: contracts — `sessionLifecycleInputs`, `sessionToolContracts`, and `managed/managedToolContracts` input schemas; enforcement happens during MCP input validation, before any handler runs. - CLI and MCP/tool-catalog contract: MCP input validation tightens for `open_binary`, `close_binary`, `export_evidence_bundle`, `inspect_managed_artifact`, `inspect_managed_members`, and `inspect_managed_native_boundaries` when the path fields are present; property descriptions now state the absolute-path requirement; the generated catalog and product catalog are regenerated. CLI behavior is unchanged. - Provider, bridge, target-format, or platform compatibility: no provider, bridge, or target-format impact. Platform: the check follows host semantics, and Windows drive-letter paths in both separator styles are covered by tests that run on `win32`. - Evidence, artifact, provenance, or reconstruction contract: none — evidence content, identity, and provenance handling are unchanged; only the destination argument is validated earlier. - Process execution, authorization, cleanup, or containment impact: none — no process spawning, authorization, or cleanup behavior changes, and no new containment root is introduced. - Generated metadata (`docs/product-catalog.json`), package, or installation impact: yes — `src/generatedMcpToolCatalog.ts`, `docs/product-catalog.json`, and `skills/reverse-engineer-anything/SKILL.md` are regenerated in this PR; package contents are otherwise unchanged. ## Evidence and regression coverage - Tests added or updated: `src/contracts/sessionPathInputs.test.ts` adds 16 cases covering relative and parent-relative rejection on each schema, acceptance of absolute paths, the optional-omission paths staying valid, and `win32`-only acceptance of `C:\` and `C:/` forms. `tests/boundary/mcp/sessionMcp.test.ts` adds an end-to-end case that calls `export_evidence_bundle` and `close_binary` with relative paths through a live client/server pair and asserts the error text. - Base reproduction or other evidence: on the base revision the six relative-path schema tests fail (the schemas accept the strings), and a relative `snapshot_path` over MCP writes under the server working directory. With the refinement the six pass and the MCP call fails with the message above. - User-visible CLI/MCP output (if applicable): the new rejection message shown in the problem section; existing examples in `toolContractExamples` and `managedToolContracts` already use absolute paths and continue to parse. - Remaining proof gaps: the `win32` path-form cases execute only on Windows hosts (`describe.runIf`), so Linux CI skips them; they ran green on this Windows machine. `import_evidence_bundle` still accepts relative paths by design in this PR. - [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 - `npm run typecheck` — passed. - `npx vitest run src/contracts --coverage.enabled=false` — 5 files, 34 tests passed, including the 16 new path-input cases. - `npx vitest run tests/boundary/mcp/sessionMcp.test.ts --coverage.enabled=false` — the new path-boundary case passes; the pre-existing `target-free MCP workflow` case exceeded its 10s timeout on this Windows host, and the same timeout reproduces on unmodified `main` (10.4s vs 10.3s), so it is machine speed rather than this change. - `npx vitest run tests/boundary/mcp/toolSchemaValidity.test.ts --coverage.enabled=false` — 5 passed; advertised input/output schemas stay valid draft-07 for every canonical tool. - `npx vitest run tests/acceptance/contractPresentation.test.ts --coverage.enabled=false` — 2 passed. - `npx vitest run tests/evaluation/moduleBoundaries.test.ts --coverage.enabled=false` — 75 passed. - `npx oxlint <changed files>` — 0 errors. - `npx oxfmt --check <changed files>` — all clean. - `node scripts/generate-mcp-tool-catalog.mjs --check`, `node scripts/generate-product-catalog.mjs --check`, `node scripts/generate-skill-metadata.mjs --check` — all pass after regeneration. - `npm run jscpd` — passed (424 clones, within threshold). - `npm run check` — reaches `format:check`, which fails across the whole checkout including untouched files because this Windows worktree has CRLF line endings; `knip` aborts with an array-buffer allocation error, reproduced identically on unmodified `main`. - `npm run docs:check` — the generators this PR touches pass their `--check` gates; `docs:check:built` stops at the completion-ledger file, a pre-existing line-ending digest mismatch on this Windows checkout that also reproduces on unmodified `main`. - Real-provider lanes: not run — no provider, bridge, or browser code changed. ## Compatibility, safety, and release - Breaking changes or migration steps: callers that relied on relative paths being resolved against the server working directory now get a validation error and must send an absolute path. Docs and contract examples already use absolute paths, and the error message shows a valid example for each platform. Worth a maintainer decision on whether this warrants a `!` in the release note. - Real Hopper/Ghidra, browser, or OS coverage: not applicable — nothing under the provider, bridge, or browser trees changed. Path-form behavior was verified on a Windows host. - Package or release metadata impact: regenerated tool catalog, product catalog, and skill catalog digest are included; no dependency, lockfile, or install-lifecycle changes. - Security, privacy, process, or containment review: removes silent resolution of caller-supplied paths against an unobservable server working directory for snapshot writes, snapshot reads, evidence export, and managed target selection. It adds no new authority: REA is a local-only tool and the destination remains the caller's choice. Capability-token, environment redaction, and process execution behavior are untouched. ## Review checklist - [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. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-17 21:07:10 +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#2525

No description provided.

Rows
Columns