morluto/rea

Fork 0

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

[PR #3572] [CLOSED] fix(evidence): share the exported bundle result projection #5833

New issue

Closed

opened 2026-10-31 01:09:46 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-31 01:09:46 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/3572
Author: @XiaoBinGan
Created: 10/14/2026
Status: ❌ Closed

Base: main ← Head: fix/evidence-export-contract-shape

📝 Commits (4)

450c46c fix(evidence): share the exported bundle result projection
35b7b26 Merge remote-tracking branch 'origin/main' into fix/evidence-export-contract-shape
2e05efb Merge remote-tracking branch 'upstream/main' into fix/evidence-export-contract-shape
1033d28 Merge remote-tracking branch 'upstream/main' into fix/evidence-export-contract-shape

📊 Changes

3 files changed (+183 additions, -14 deletions)

View changed files

📝 src/application/EvidenceBundleCommands.ts (+10 -3)
📝 src/server/registerSessionRecordTools.ts (+5 -11)
➕ tests/boundary/cli/evidenceExportOutputContract.test.ts (+168 -0)

📄 Description

Summary

rea evidence-export --json emitted a result that does not satisfy the schema REA advertises for export_evidence_bundle: the CLI path omitted unknowns, which the contract marks required, and unknowns is the only count of residual-unknown revisions in the export. The MCP path produced the same shape independently, so the two adapters were two copies of one fact and had already drifted.

No issue is linked: this was found by auditing the producing boundary for the "one fact, two owners" class, not from a user report. The contract change needed to expose the drift already exists in the repo (src/contracts/toolOutputSchemaGroups.ts), which is why the CLI output could be checked against a single owner instead of a hand-written expectation.

Base revision: 569ef4159.

Problem and expected behavior

Smallest trigger: export any canonical Evidence bundle through the CLI and inspect the JSON.

$ node scripts/rea.mjs evidence-export bundle.json out.json --json     # before
{"path":"…","bytes":2615,"records":2}

The contract for this operation is export_evidence_bundle: lifecycleResultOf(z.object({ path, bytes, records, unknowns })) — all four fields required. Two separate projections existed:

CLI: src/application/EvidenceBundleCommands.ts had a private projectWrite() returning {path, bytes, records}.
MCP: src/server/registerSessionRecordTools.ts inlined its own object literal {path, bytes, records, unknowns: bundle.unknowns.length}.

So the CLI silently violated the advertised contract, and a CLI consumer could not see the residual-unknown count at all.

Expected: both adapters produce the contract-complete shape from one owner.

$ node scripts/rea.mjs evidence-export bundle.json out.json --json     # after
{"path":"…","bytes":2615,"records":2,"unknowns":2}

Change and scope

src/application/EvidenceBundleCommands.ts: the private projectWrite becomes the exported projectExportedEvidenceBundle(bundle, written), the single owner of the exported-bundle result shape. It returns the contract-complete {path, bytes, records, unknowns}.
src/server/registerSessionRecordTools.ts: the MCP export handler consumes that owner instead of its inline literal; the literal is deleted.
No contract change and no new field. unknowns already existed in the advertised schema; the CLI path simply never produced it.
The application layer was chosen because it already owns exportEvidenceBundleCommand, the workflow that produces the fact, and both adapters already import this module. Dependency flow stays inward (application → consumed by CLI/MCP); verify:module-boundaries passes.

Intentionally not included: the two adapters still interpret one outcome differently, and this PR does not touch it. When writeEvidenceBundle fails after committing a write (EvidenceFileError.published set) and cleanup succeeds, the CLI promotes the result to success and exits 0 with the cleanup failure invisible, while the MCP handler returns the write error. Unifying that changes caller-visible semantics for one adapter and needs a maintainer decision, so the error policy of each adapter is preserved and the shared projection returns err(written.error) unchanged. Details and exact lines are in "Compatibility" below. bundleForSerialization(session) also stays MCP-specific; only the advertised-contract projection was unified.

Contract and boundary impact

Semantic owner and earliest changed stage: projectExportedEvidenceBundle() in src/application/EvidenceBundleCommands.ts; earliest changed stage is the application workflow that projects the written bundle.
Contribution scope fit and linked maintainer decision: an ordinary fix inside existing boundaries; no scope decision required.
CLI and MCP/tool-catalog contract: the CLI output now satisfies export_evidence_bundle's existing contract. No schema, field name, catalog entry, or ordering change; the MCP output is byte-equivalent to before.
Provider, bridge, target-format, or platform compatibility: none.
Evidence, artifact, provenance, or reconstruction contract: none. path, bytes and records are unchanged; unknowns is added to the CLI projection only.
Target selection, authority, upstream behavior, and cleanup details: not applicable — no target interaction. The retained-cleanup outcome question is described above and left unchanged.
Generated metadata / package / installation impact: none. No file under src/contracts/ was touched, so docs:check was not required.

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

Original failure and the fix, on the real CLI with a fresh npm run build:cached of each revision (the pre-fix run used a build of the stashed original source, so dist/ could not mask it):

BEFORE  {"path":"/tmp/…/prefix-out.json","bytes":2615,"records":2}
AFTER   {"path":"/tmp/…/nonzero-final.json","bytes":2615,"records":2,"unknowns":2}
AFTER   {"path":"/tmp/…/zero-final.json","bytes":671,"records":1,"unknowns":0}

Regression coverage added in tests/boundary/cli/evidenceExportOutputContract.test.ts (new file, so it does not overlap the in-flight edit to evidenceBundleCommands.test.ts in #3561):

real CLI evidence-export output parsed through toolContract("export_evidence_bundle").outputSchema — the test validates against the contract itself rather than restating the shape, so it fails for any future drift, not just this field;
a bundle with zero residual unknowns, pinning unknowns: 0 rather than only the populated case;
a CLI/MCP parity case driving the real server registration in-process (createServer + InMemoryTransport) and asserting both adapters report the same records/unknowns.

The test fails on the unmodified revision — with the source change stashed and rebuilt:

Test Files  1 failed (1)
     Tests  3 failed (3)
ZodError … "path": ["result","unknowns"], "message": "Invalid input: expected number, received undefined"

Checks actually run:

npm run test:focused -- tests/boundary/cli/evidenceExportOutputContract.test.ts tests/boundary/cli/evidenceBundleCommands.test.ts tests/boundary/mcp/evidenceBundleInline.test.ts tests/boundary/mcp/evidenceExportCancellation.test.ts tests/boundary/filesystem/evidenceBundleFiles.test.ts → 5 passed, 30 tests.
npm run test:focused -- tests/boundary/cli/evidenceExportOutputContract.test.ts tests/boundary/cli/evidenceBundleCommands.test.ts → 2 passed, 6 tests, re-run independently by the reviewer of this diff.
npm run check:fast → Tasks: 2 successful, 2 total; npm run verify:module-boundaries → 1885 source files checked.
npm run format:check → All matched files use the correct format. (3517 files).
npm run verify:test-discovery → Discovered all 1286 test files exactly once.

Unverified gaps: the exported input bundles are canonical bundles built with the project's own production domain constructors, not bundles captured from a real provider-backed analysis session — the CLI/MCP code paths, contract validation and server registration are real, but real-provider bundle content is not covered here. The parity case uses the real registration and contract over an in-process MCP transport, not stdio; stdio MCP coverage exists elsewhere in the suite but was not exercised for this change.

Residual related cleanup deliberately left: the revisionBundle fixture is duplicated with tests/boundary/cli/evidenceBundleCommands.test.ts rather than extracted, because that file has an unrelated in-flight edit (#3561); a shared tests/fixtures/ module would be the follow-up if wanted. The published outcome asymmetry described above remains.

Compatibility, safety, and release

Breaking changes or migration steps: none. No ! / BREAKING CHANGE: marker. The CLI result gains a field the contract already required; path, bytes and records are unchanged.
Real Hopper/Ghidra, browser, or OS coverage: none required; no provider or target interaction.
Package or release metadata impact: none.
Security, privacy, process, or containment review: none applicable. The CLI still preserves the caller's requested destination path and overwrite behavior; only the result projection changed.

Author readiness

Human owner: @XiaoBinGan

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/3572 **Author:** [@XiaoBinGan](https://github.com/XiaoBinGan) **Created:** 10/14/2026 **Status:** ❌ Closed **Base:** `main` ← **Head:** `fix/evidence-export-contract-shape` --- ### 📝 Commits (4) - [`450c46c`](https://github.com/morluto/rea/commit/450c46c3d31840278f1c8a4624b75aa164ebb245) fix(evidence): share the exported bundle result projection - [`35b7b26`](https://github.com/morluto/rea/commit/35b7b26d150182544c1e10c178c1a7d8336eaafb) Merge remote-tracking branch 'origin/main' into fix/evidence-export-contract-shape - [`2e05efb`](https://github.com/morluto/rea/commit/2e05efbe0a2b00b9e8523eedd86f20996e3d92d5) Merge remote-tracking branch 'upstream/main' into fix/evidence-export-contract-shape - [`1033d28`](https://github.com/morluto/rea/commit/1033d2886bec353690e8a4f48790f539d56a40e7) Merge remote-tracking branch 'upstream/main' into fix/evidence-export-contract-shape ### 📊 Changes **3 files changed** (+183 additions, -14 deletions) <details> <summary>View changed files</summary> 📝 `src/application/EvidenceBundleCommands.ts` (+10 -3) 📝 `src/server/registerSessionRecordTools.ts` (+5 -11) ➕ `tests/boundary/cli/evidenceExportOutputContract.test.ts` (+168 -0) </details> ### 📄 Description ## Summary `rea evidence-export --json` emitted a result that does not satisfy the schema REA advertises for `export_evidence_bundle`: the CLI path omitted `unknowns`, which the contract marks required, and `unknowns` is the only count of residual-unknown revisions in the export. The MCP path produced the same shape independently, so the two adapters were two copies of one fact and had already drifted. No issue is linked: this was found by auditing the producing boundary for the "one fact, two owners" class, not from a user report. The contract change needed to expose the drift already exists in the repo (`src/contracts/toolOutputSchemaGroups.ts`), which is why the CLI output could be checked against a single owner instead of a hand-written expectation. Base revision: `569ef4159`. ## Problem and expected behavior Smallest trigger: export any canonical Evidence bundle through the CLI and inspect the JSON. ``` $ node scripts/rea.mjs evidence-export bundle.json out.json --json # before {"path":"…","bytes":2615,"records":2} ``` The contract for this operation is `export_evidence_bundle: lifecycleResultOf(z.object({ path, bytes, records, unknowns }))` — all four fields required. Two separate projections existed: - CLI: `src/application/EvidenceBundleCommands.ts` had a private `projectWrite()` returning `{path, bytes, records}`. - MCP: `src/server/registerSessionRecordTools.ts` inlined its own object literal `{path, bytes, records, unknowns: bundle.unknowns.length}`. So the CLI silently violated the advertised contract, and a CLI consumer could not see the residual-unknown count at all. Expected: both adapters produce the contract-complete shape from one owner. ``` $ node scripts/rea.mjs evidence-export bundle.json out.json --json # after {"path":"…","bytes":2615,"records":2,"unknowns":2} ``` ## Change and scope - `src/application/EvidenceBundleCommands.ts`: the private `projectWrite` becomes the exported `projectExportedEvidenceBundle(bundle, written)`, the **single owner** of the exported-bundle result shape. It returns the contract-complete `{path, bytes, records, unknowns}`. - `src/server/registerSessionRecordTools.ts`: the MCP export handler consumes that owner instead of its inline literal; the literal is deleted. - No contract change and no new field. `unknowns` already existed in the advertised schema; the CLI path simply never produced it. - The application layer was chosen because it already owns `exportEvidenceBundleCommand`, the workflow that produces the fact, and both adapters already import this module. Dependency flow stays inward (application → consumed by CLI/MCP); `verify:module-boundaries` passes. **Intentionally not included:** the two adapters still interpret one *outcome* differently, and this PR does not touch it. When `writeEvidenceBundle` fails after committing a write (`EvidenceFileError.published` set) and cleanup succeeds, the CLI promotes the result to success and exits 0 with the cleanup failure invisible, while the MCP handler returns the write error. Unifying that changes caller-visible semantics for one adapter and needs a maintainer decision, so the error policy of each adapter is preserved and the shared projection returns `err(written.error)` unchanged. Details and exact lines are in "Compatibility" below. `bundleForSerialization(session)` also stays MCP-specific; only the advertised-contract projection was unified. ## Contract and boundary impact - Semantic owner and earliest changed stage: `projectExportedEvidenceBundle()` in `src/application/EvidenceBundleCommands.ts`; earliest changed stage is the application workflow that projects the written bundle. - Contribution scope fit and linked maintainer decision: an ordinary fix inside existing boundaries; no scope decision required. - CLI and MCP/tool-catalog contract: the CLI output now satisfies `export_evidence_bundle`'s existing contract. No schema, field name, catalog entry, or ordering change; the MCP output is byte-equivalent to before. - Provider, bridge, target-format, or platform compatibility: none. - Evidence, artifact, provenance, or reconstruction contract: none. `path`, `bytes` and `records` are unchanged; `unknowns` is added to the CLI projection only. - Target selection, authority, upstream behavior, and cleanup details: not applicable — no target interaction. The retained-cleanup outcome question is described above and left unchanged. - Generated metadata / package / installation impact: none. No file under `src/contracts/` was touched, so `docs:check` was not required. 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. - [x] Documentation or usability only; no target interaction. - [ ] Not sure yet; investigation or maintainer guidance needed. ## Evidence and regression coverage Original failure and the fix, on the real CLI with a fresh `npm run build:cached` of each revision (the pre-fix run used a build of the stashed original source, so `dist/` could not mask it): ``` BEFORE {"path":"/tmp/…/prefix-out.json","bytes":2615,"records":2} AFTER {"path":"/tmp/…/nonzero-final.json","bytes":2615,"records":2,"unknowns":2} AFTER {"path":"/tmp/…/zero-final.json","bytes":671,"records":1,"unknowns":0} ``` Regression coverage added in `tests/boundary/cli/evidenceExportOutputContract.test.ts` (new file, so it does not overlap the in-flight edit to `evidenceBundleCommands.test.ts` in #3561): - real CLI `evidence-export` output parsed through `toolContract("export_evidence_bundle").outputSchema` — the test validates against the contract itself rather than restating the shape, so it fails for any future drift, not just this field; - a bundle with zero residual unknowns, pinning `unknowns: 0` rather than only the populated case; - a CLI/MCP parity case driving the real server registration in-process (`createServer` + `InMemoryTransport`) and asserting both adapters report the same `records`/`unknowns`. The test fails on the unmodified revision — with the source change stashed and rebuilt: ``` Test Files 1 failed (1) Tests 3 failed (3) ZodError … "path": ["result","unknowns"], "message": "Invalid input: expected number, received undefined" ``` Checks actually run: - `npm run test:focused -- tests/boundary/cli/evidenceExportOutputContract.test.ts tests/boundary/cli/evidenceBundleCommands.test.ts tests/boundary/mcp/evidenceBundleInline.test.ts tests/boundary/mcp/evidenceExportCancellation.test.ts tests/boundary/filesystem/evidenceBundleFiles.test.ts` → **5 passed, 30 tests**. - `npm run test:focused -- tests/boundary/cli/evidenceExportOutputContract.test.ts tests/boundary/cli/evidenceBundleCommands.test.ts` → **2 passed, 6 tests**, re-run independently by the reviewer of this diff. - `npm run check:fast` → `Tasks: 2 successful, 2 total`; `npm run verify:module-boundaries` → 1885 source files checked. - `npm run format:check` → `All matched files use the correct format.` (3517 files). - `npm run verify:test-discovery` → `Discovered all 1286 test files exactly once.` Unverified gaps: the exported input bundles are canonical bundles built with the project's own production domain constructors, not bundles captured from a real provider-backed analysis session — the CLI/MCP code paths, contract validation and server registration are real, but real-provider bundle content is not covered here. The parity case uses the real registration and contract over an in-process MCP transport, not stdio; stdio MCP coverage exists elsewhere in the suite but was not exercised for this change. Residual related cleanup deliberately left: the `revisionBundle` fixture is duplicated with `tests/boundary/cli/evidenceBundleCommands.test.ts` rather than extracted, because that file has an unrelated in-flight edit (#3561); a shared `tests/fixtures/` module would be the follow-up if wanted. The `published` outcome asymmetry described above remains. ## Compatibility, safety, and release - Breaking changes or migration steps: none. No `!` / `BREAKING CHANGE:` marker. The CLI result gains a field the contract already required; `path`, `bytes` and `records` are unchanged. - Real Hopper/Ghidra, browser, or OS coverage: none required; no provider or target interaction. - Package or release metadata impact: none. - Security, privacy, process, or containment review: none applicable. The CLI still preserves the caller's requested destination path and overwrite behavior; only the result projection changed. ## Author readiness  Human owner: @XiaoBinGan - [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. - [ ] 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. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-31 01:09:46 +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#5833

No description provided.

Rows
Columns