[PR #2798] [MERGED] fix(reconstruction): reject contradictory function comparison status #5401

Closed
opened 2026-10-31 01:06:22 +00:00 by bsakaguchi · 0 comments
Owner

📋 Pull Request Information

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

Base: main ← Head: fix/function-comparison-status


📝 Commits (2)

  • 967d1c1 fix(reconstruction): reject contradictory function comparison status
  • 7d242ab test(reconstruction): cover imported function match precedence

📊 Changes

3 files changed (+212 additions, -15 deletions)

View changed files

📝 src/domain/functionComparison.test.ts (+48 -0)
📝 src/domain/functionComparisonSchemas.ts (+28 -14)
📝 tests/boundary/mcp/functionComparisonMcp.test.ts (+136 -1)

📄 Description

Summary

Fixes #2623. Reconstruction verification now rejects an imported function comparison whose overall status disagrees with its dimensions or function match, rather than allowing a false overall pass.

Problem and expected behavior

A content-authenticated comparison can be imported with changed or unknown dimensions and a stored status: "unchanged". Previously verify_reconstruction trusted that aggregate. Authentication establishes record integrity, but does not establish that the producer's semantic invariant holds.

Change and scope

Validate the aggregate in functionComparisonResultSchema using the producer's existing overallStatus rule. Reconstruction, changed-behavior, and static/runtime correlation consumers all parse that schema, so the check has one owner and no downstream repairs. Contradictory records remain retained as source Evidence; they cannot support successful verification.

This is an ordinary validation fix within the existing contribution scope. No advance maintainer decision applies.

Contract and boundary impact

  • Semantic owner and earliest changed stage: canonical function comparison result parsing.
  • Contribution scope fit and linked maintainer decision: not applicable; no new capability.
  • CLI and MCP/tool-catalog contract: cross-field runtime validation; JSON property schemas and successful producer results are unchanged.
  • Provider, bridge, target-format, or platform compatibility: no provider changes.
  • Evidence, artifact, provenance, or reconstruction contract: malformed aggregate status is rejected with a typed input error when consumed; imported Evidence identity and content remain preserved.
  • Target selection, authority, upstream behavior, and cleanup details: unchanged; the regression launches no analysis provider.
  • Generated metadata, package, or installation impact: no tracked generated changes.

Expected effects in the tested import/verification workflow:

  • Read existing files or captured data.

No target execution, instrumentation, mutation, network acquisition, or protection recovery is added.

Evidence and regression coverage

  • Added real MCP file-bundle import and reconstruction tests for inconsistent changed/unknown aggregates. Both failed on base 43738bec5 because reconstruction returned a successful response instead of rejecting the contradiction; six other tests passed, including the unchanged control.

  • Consistent changed, unknown and unchanged comparisons still yield fail, unknown and pass, respectively. The tests check retained contradictory Evidence and session health after rejection.

  • Corrected MCP behavior: invalid_request, with invalid_value guidance stating Function comparison status contradicts its dimensions or function match.

  • Focused schema regressions accept correct aggregates and reject every contradictory status for mismatched/ambiguous matches, including ambiguous matches with changed dimensions. Existing producer tests also cover these match branches.

  • Remaining proof gaps: no real native analysis engine or Windows/Linux execution. This fixes aggregate status consistency, not every possible inconsistency in counts, change lists, or dimension membership. No performance benchmark was run; the added check traverses the existing eight dimensions.

  • 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

macOS arm64, Node 24.18.0, npm 11.16.0, locked dependencies:

  • npm run test:focused -- src/domain/functionComparison.test.ts src/domain/reconstructionVerification.test.ts src/domain/reconstructionVerification.functionSources.test.ts src/domain/changedBehavior.test.ts src/domain/staticRuntimeCorrelation.test.ts tests/boundary/mcp/functionComparisonMcp.test.ts tests/boundary/mcp/functionComparisonCoverageMcp.test.ts tests/boundary/mcp/comparisonUnknownsMcp.test.ts tests/boundary/mcp/reconstructionCoverageMcp.test.ts tests/boundary/mcp/toolSchemaValidity.test.ts — 157 tests passed across ten files; runtime/skill/catalog build passed.
  • npm run check — blocked by a pre-existing lint error in unchanged tests/boundary/providers/ghidra/ghidraLauncher.test.ts:431: function length 176 exceeds 150. Independently reproduced with npm run lint on a clean 43738bec5 worktree; both report 1 error and 1052 warnings. Type/format/metadata tasks that completed passed.
  • Normal pre-push npm run check:fast — stopped on the same unchanged Ghidra lint error. After reproducing it on clean main and running the unaffected checks separately, the fork pushes used command-scoped HUSKY=0; no global hook configuration was changed.
  • npm run typecheck, npm run knip, npm run verify:module-boundaries, npm run verify:architecture-guards — passed separately after the aggregate check stopped.
  • npm run verify:test-discovery — all 1061 test files discovered exactly once.
  • npm run docs:check — passed generated checks and both README consistency tests.
  • git diff --check — passed.
  • Independent static review — no findings; reviewer did not independently run tests.

Review follow-up on the final head

The two Copilot suggestions were submitted against 967d1c1e, and were checked against current 7d242abbe71fdb197cb9455973a60751e13bde79:

  • Teardown binding: tests/fixtures/applicationMcpHarness.ts:46-48 defines close as an async arrow closure over client, server, and session, with no this dependency. The installed Vitest 5.0.3 runner's callTestHooks awaits the callback; its timeout wrapper follows the returned thenable. Passing this callback directly preserves its promise and cleanup. Adding a wrapper would not repair an existing failure.
  • Test grouping: this file has no suite-level hooks or configuration on which the new cases depend. tests/boundary/mcp/functionComparisonCoverageMcp.test.ts also uses a top-level test, and repository guidance does not mandate a describe wrapper. Focused verification selects exact files; the cases have explicit test names. No functional filtering or reporting defect was established, so no cosmetic-only regrouping was added.
  • npm run test:focused -- tests/boundary/mcp/functionComparisonMcp.test.ts was rerun on the final head: all eight tests passed. No source change or new commit was needed for these two suggestions.

Greptile passed on the final head and GitHub Codex review completed. Copilot produced a Lite review, but its full agentic review did not run because a runner was unavailable before timeout; this is unavailable coverage, not a completed full review. CI remains pending and is reported separately.

Compatibility, safety, and release

  • Breaking changes or migration steps: normal comparison records remain valid. Regenerate contradictory comparisons from retained source Evidence before using them for derived claims.
  • Real Hopper/Ghidra, browser, or OS coverage: no engine/browser changes; macOS MCP harness only.
  • Package or release metadata impact: none.
  • Security, privacy, process, or containment review: no additional authority, target effects, or loss of imported data. The unrelated baseline lint failure remains visible above.

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/2798 **Author:** [@maxazure](https://github.com/maxazure) **Created:** 10/13/2026 **Status:** ✅ Merged **Merged:** 10/13/2026 **Merged by:** [@morluto](https://github.com/morluto) **Base:** `main` ← **Head:** `fix/function-comparison-status` --- ### 📝 Commits (2) - [`967d1c1`](https://github.com/morluto/rea/commit/967d1c1e8e28d0087d93cbb7aa05672ab8319240) fix(reconstruction): reject contradictory function comparison status - [`7d242ab`](https://github.com/morluto/rea/commit/7d242abbe71fdb197cb9455973a60751e13bde79) test(reconstruction): cover imported function match precedence ### 📊 Changes **3 files changed** (+212 additions, -15 deletions) <details> <summary>View changed files</summary> 📝 `src/domain/functionComparison.test.ts` (+48 -0) 📝 `src/domain/functionComparisonSchemas.ts` (+28 -14) 📝 `tests/boundary/mcp/functionComparisonMcp.test.ts` (+136 -1) </details> ### 📄 Description ## Summary Fixes #2623. Reconstruction verification now rejects an imported function comparison whose overall status disagrees with its dimensions or function match, rather than allowing a false overall pass. ## Problem and expected behavior A content-authenticated comparison can be imported with changed or unknown dimensions and a stored `status: "unchanged"`. Previously `verify_reconstruction` trusted that aggregate. Authentication establishes record integrity, but does not establish that the producer's semantic invariant holds. ## Change and scope Validate the aggregate in `functionComparisonResultSchema` using the producer's existing `overallStatus` rule. Reconstruction, changed-behavior, and static/runtime correlation consumers all parse that schema, so the check has one owner and no downstream repairs. Contradictory records remain retained as source Evidence; they cannot support successful verification. This is an ordinary validation fix within the existing contribution scope. No advance maintainer decision applies. ## Contract and boundary impact - Semantic owner and earliest changed stage: canonical function comparison result parsing. - Contribution scope fit and linked maintainer decision: not applicable; no new capability. - CLI and MCP/tool-catalog contract: cross-field runtime validation; JSON property schemas and successful producer results are unchanged. - Provider, bridge, target-format, or platform compatibility: no provider changes. - Evidence, artifact, provenance, or reconstruction contract: malformed aggregate status is rejected with a typed input error when consumed; imported Evidence identity and content remain preserved. - Target selection, authority, upstream behavior, and cleanup details: unchanged; the regression launches no analysis provider. - Generated metadata, package, or installation impact: no tracked generated changes. Expected effects in the tested import/verification workflow: - [x] Read existing files or captured data. No target execution, instrumentation, mutation, network acquisition, or protection recovery is added. ## Evidence and regression coverage - Added real MCP file-bundle import and reconstruction tests for inconsistent changed/unknown aggregates. Both failed on base `43738bec5` because reconstruction returned a successful response instead of rejecting the contradiction; six other tests passed, including the unchanged control. - Consistent changed, unknown and unchanged comparisons still yield fail, unknown and pass, respectively. The tests check retained contradictory Evidence and session health after rejection. - Corrected MCP behavior: `invalid_request`, with `invalid_value` guidance stating `Function comparison status contradicts its dimensions or function match.` - Focused schema regressions accept correct aggregates and reject every contradictory status for mismatched/ambiguous matches, including ambiguous matches with changed dimensions. Existing producer tests also cover these match branches. - Remaining proof gaps: no real native analysis engine or Windows/Linux execution. This fixes aggregate status consistency, not every possible inconsistency in counts, change lists, or dimension membership. No performance benchmark was run; the added check traverses the existing eight dimensions. - [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 macOS arm64, Node 24.18.0, npm 11.16.0, locked dependencies: - `npm run test:focused -- src/domain/functionComparison.test.ts src/domain/reconstructionVerification.test.ts src/domain/reconstructionVerification.functionSources.test.ts src/domain/changedBehavior.test.ts src/domain/staticRuntimeCorrelation.test.ts tests/boundary/mcp/functionComparisonMcp.test.ts tests/boundary/mcp/functionComparisonCoverageMcp.test.ts tests/boundary/mcp/comparisonUnknownsMcp.test.ts tests/boundary/mcp/reconstructionCoverageMcp.test.ts tests/boundary/mcp/toolSchemaValidity.test.ts` — 157 tests passed across ten files; runtime/skill/catalog build passed. - `npm run check` — blocked by a pre-existing lint error in unchanged `tests/boundary/providers/ghidra/ghidraLauncher.test.ts:431`: function length 176 exceeds 150. Independently reproduced with `npm run lint` on a clean `43738bec5` worktree; both report 1 error and 1052 warnings. Type/format/metadata tasks that completed passed. - Normal pre-push `npm run check:fast` — stopped on the same unchanged Ghidra lint error. After reproducing it on clean main and running the unaffected checks separately, the fork pushes used command-scoped `HUSKY=0`; no global hook configuration was changed. - `npm run typecheck`, `npm run knip`, `npm run verify:module-boundaries`, `npm run verify:architecture-guards` — passed separately after the aggregate check stopped. - `npm run verify:test-discovery` — all 1061 test files discovered exactly once. - `npm run docs:check` — passed generated checks and both README consistency tests. - `git diff --check` — passed. - Independent static review — no findings; reviewer did not independently run tests. ## Review follow-up on the final head The two Copilot suggestions were submitted against `967d1c1e`, and were checked against current `7d242abbe71fdb197cb9455973a60751e13bde79`: - Teardown binding: `tests/fixtures/applicationMcpHarness.ts:46-48` defines `close` as an async arrow closure over `client`, `server`, and `session`, with no `this` dependency. The installed Vitest 5.0.3 runner's `callTestHooks` awaits the callback; its timeout wrapper follows the returned thenable. Passing this callback directly preserves its promise and cleanup. Adding a wrapper would not repair an existing failure. - Test grouping: this file has no suite-level hooks or configuration on which the new cases depend. `tests/boundary/mcp/functionComparisonCoverageMcp.test.ts` also uses a top-level test, and repository guidance does not mandate a `describe` wrapper. Focused verification selects exact files; the cases have explicit test names. No functional filtering or reporting defect was established, so no cosmetic-only regrouping was added. - `npm run test:focused -- tests/boundary/mcp/functionComparisonMcp.test.ts` was rerun on the final head: all eight tests passed. No source change or new commit was needed for these two suggestions. Greptile passed on the final head and GitHub Codex review completed. Copilot produced a Lite review, but its full agentic review did **not** run because a runner was unavailable before timeout; this is unavailable coverage, not a completed full review. CI remains pending and is reported separately. ## Compatibility, safety, and release - Breaking changes or migration steps: normal comparison records remain valid. Regenerate contradictory comparisons from retained source Evidence before using them for derived claims. - Real Hopper/Ghidra, browser, or OS coverage: no engine/browser changes; macOS MCP harness only. - Package or release metadata impact: none. - Security, privacy, process, or containment review: no additional authority, target effects, or loss of imported data. The unrelated baseline lint failure remains visible above. ## 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. - [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:06:22 +00:00
Sign in to join this conversation.
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#5401
No description provided.