morluto/rea

Fork 0

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

[PR #7176] [CLOSED] fix(artifacts): name the compare_artifacts side of a rejected summary #7693

New issue

Closed

opened 2026-11-03 13:39:43 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-11-03 13:39:43 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/7176
Author: @hannesstrydom664-hash
Created: 10/25/2026
Status: ❌ Closed

Base: main ← Head: fix/compare-artifacts-side-path-7089

📝 Commits (2)

e0ae9f4 fix(artifacts): name the compare_artifacts side of a rejected summary
55be6de Merge branch 'main' into fix/compare-artifacts-side-path-7089

📊 Changes

4 files changed (+118 additions, -15 deletions)

View changed files

📝 src/application/artifacts/ArtifactComparisonEvidence.test.ts (+70 -14)
📝 src/application/artifacts/ArtifactComparisonEvidence.ts (+3 -1)
📝 src/server/registerArtifactComparisonTool.ts (+2 -0)
📝 tests/process-global/derivedOperationFailures.test.ts (+43 -0)

📄 Description

Summary

Fixes #7089

When compare_artifacts rejects an inspect_analysis_view Evidence record whose normalized_result is not an artifact inventory summary, resolveArtifactComparisonEvidence previously emitted an AnalysisInputError with issues: [{ path: ["evidence"], reason: "invalid_value", ... }]. Because compare_artifacts accepts left and right (not evidence), ["evidence"] was a nonexistent input path. This change threads side: "left" | "right" into resolveArtifactComparisonEvidence so rejected summaries report path: ["left"] or path: ["right"].

Problem and expected behavior

Smallest trigger: Call compare_artifacts (or resolveArtifactComparisonEvidence) with left or right set to an inspect_analysis_view summary whose normalized_result is not an artifact inventory summary (e.g., { view: "other" }).
Violated invariant: AnalysisInputError.issues[].path must name an actual input parameter of compare_artifacts (["left"] or ["right"]) rather than the internal variable name ["evidence"], matching parseSuppliedEvidenceInput(args.left, ["left"], ...) and parseSuppliedEvidenceInput(args.right, ["right"], ...) in src/server/registerArtifactComparisonTool.ts.
Expected behavior: Rejecting a non-inventory inspect_analysis_view summary on left reports path: ["left"], and rejecting one on right reports path: ["right"].

Change and scope

Follow the contribution scope.
Not applicable (ordinary defect fix within existing compare_artifacts input validation boundaries).

src/application/artifacts/ArtifactComparisonEvidence.ts: Add side: "left" | "right" parameter to resolveArtifactComparisonEvidence and use path: [side] when constructing AnalysisInputError.
src/server/registerArtifactComparisonTool.ts: Pass "left" and "right" to resolveArtifactComparisonEvidence for suppliedLeft and suppliedRight.
src/application/artifacts/ArtifactComparisonEvidence.test.ts: Update existing unit calls and add a parameterized it.each(["left", "right"] as const) test verifying that non-inventory summaries report path: [side].
tests/process-global/derivedOperationFailures.test.ts: Add a parameterized it.each(["left", "right"] as const) MCP tool boundary test verifying that compare_artifacts returns invalid_request with issues: [{ path: [side], reason: "invalid_value" }] and retains no Evidence records on failure.

Contract and boundary impact

Semantic owner and earliest changed stage: resolveArtifactComparisonEvidence (src/application/artifacts/ArtifactComparisonEvidence.ts), consumed by registerArtifactComparisonTool (src/server/registerArtifactComparisonTool.ts).
Contribution scope fit and linked maintainer decision: Not applicable (bug fix for #7089 within existing compare_artifacts error contract).
CLI and MCP/tool-catalog contract: Fixes compare_artifacts invalid_request issue path from nonexistent ["evidence"] to ["left"] or ["right"]. No tool schema or catalog change.
Provider, bridge, target-format, or platform compatibility: None.
Evidence, artifact, provenance, or reconstruction contract: Failed compare_artifacts calls continue to retain zero Evidence records; only the AnalysisInputError issue path is corrected.
Target selection, authority, upstream behavior, and cleanup details (if applicable): Not applicable.
Generated metadata (docs/public/product-catalog.json), package, or installation impact: None.

Expected effects (check known effects; use documentation-only on its own).
Unchecked items do not establish absence; leave uncertainties visible.

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

Tests added or updated:
- src/application/artifacts/ArtifactComparisonEvidence.test.ts (it.each(["left", "right"] as const)("names the %s compare_artifacts side when a summary is not an artifact inventory", ...))
- tests/process-global/derivedOperationFailures.test.ts (it.each(["left", "right"] as const)("classifies a non-inventory inspect_analysis_view summary at %s", ...))
Base reproduction or other evidence:
- Executed: Prior to this change, src/application/artifacts/ArtifactComparisonEvidence.ts:27 hardcoded path: ["evidence"], causing both the unit and MCP boundary assertions for path: ["left"] and path: ["right"] to fail on main.

User-visible CLI/MCP output (if applicable):

MCP compare_artifacts error payload now reports:

{
  "code": "invalid_request",
  "details": {
    "issues": [
      {
        "path": ["left"],
        "reason": "invalid_value",
        "message": "Artifact comparison accepts only an artifact inventory summary from inspect_analysis_view."
      }
    ]
  }
}

(and "path": ["right"] when supplied on right).

Remaining proof gaps: None.

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

npx vitest run src/application/artifacts/ArtifactComparisonEvidence.test.ts tests/process-global/derivedOperationFailures.test.ts — 2 test files passed, 84 tests passed (8/8 in ArtifactComparisonEvidence.test.ts, 76/76 in derivedOperationFailures.test.ts).
npm run typecheck — passed (tsc --noEmit, 0 errors).
npm run format:check — passed (oxfmt --check .).
npm run lint — passed (oxlint 0 warnings/0 errors; verify:module-boundaries passed).
npm run verify:test-discovery — passed.
npm run verify:architecture-guards — passed.
npm run scan:todos — passed.
npm run jscpd — passed (0% duplicated lines).

Compatibility, safety, and release

Breaking changes or migration steps: None. Fixes an inaccurate error path (["evidence"] -> ["left"] / ["right"]).
Real Hopper/Ghidra, browser, or OS coverage: Not applicable (pure domain/application Evidence validation and MCP tool boundary test).
Package or release metadata impact: None.
Security, privacy, process, or containment review: None.

Author readiness

Human owner: @hannesstrydom664-hash

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/7176 **Author:** [@hannesstrydom664-hash](https://github.com/hannesstrydom664-hash) **Created:** 10/25/2026 **Status:** ❌ Closed **Base:** `main` ← **Head:** `fix/compare-artifacts-side-path-7089` --- ### 📝 Commits (2) - [`e0ae9f4`](https://github.com/morluto/rea/commit/e0ae9f4369aa2608b70fd2a472fcbfba4490e93b) fix(artifacts): name the compare_artifacts side of a rejected summary - [`55be6de`](https://github.com/morluto/rea/commit/55be6dedf384bf432a344bcc929f262803f9ea03) Merge branch 'main' into fix/compare-artifacts-side-path-7089 ### 📊 Changes **4 files changed** (+118 additions, -15 deletions) <details> <summary>View changed files</summary> 📝 `src/application/artifacts/ArtifactComparisonEvidence.test.ts` (+70 -14) 📝 `src/application/artifacts/ArtifactComparisonEvidence.ts` (+3 -1) 📝 `src/server/registerArtifactComparisonTool.ts` (+2 -0) 📝 `tests/process-global/derivedOperationFailures.test.ts` (+43 -0) </details> ### 📄 Description ## Summary Fixes #7089 When `compare_artifacts` rejects an `inspect_analysis_view` Evidence record whose `normalized_result` is not an artifact inventory summary, `resolveArtifactComparisonEvidence` previously emitted an `AnalysisInputError` with `issues: [{ path: ["evidence"], reason: "invalid_value", ... }]`. Because `compare_artifacts` accepts `left` and `right` (not `evidence`), `["evidence"]` was a nonexistent input path. This change threads `side: "left" | "right"` into `resolveArtifactComparisonEvidence` so rejected summaries report `path: ["left"]` or `path: ["right"]`. ## Problem and expected behavior - **Smallest trigger:** Call `compare_artifacts` (or `resolveArtifactComparisonEvidence`) with `left` or `right` set to an `inspect_analysis_view` summary whose `normalized_result` is not an artifact inventory summary (e.g., `{ view: "other" }`). - **Violated invariant:** `AnalysisInputError.issues[].path` must name an actual input parameter of `compare_artifacts` (`["left"]` or `["right"]`) rather than the internal variable name `["evidence"]`, matching `parseSuppliedEvidenceInput(args.left, ["left"], ...)` and `parseSuppliedEvidenceInput(args.right, ["right"], ...)` in `src/server/registerArtifactComparisonTool.ts`. - **Expected behavior:** Rejecting a non-inventory `inspect_analysis_view` summary on `left` reports `path: ["left"]`, and rejecting one on `right` reports `path: ["right"]`. ## Change and scope Follow the [contribution scope](https://github.com/morluto/rea/blob/main/CONTRIBUTING.md#contribution-scope). Not applicable (ordinary defect fix within existing `compare_artifacts` input validation boundaries). - `src/application/artifacts/ArtifactComparisonEvidence.ts`: Add `side: "left" | "right"` parameter to `resolveArtifactComparisonEvidence` and use `path: [side]` when constructing `AnalysisInputError`. - `src/server/registerArtifactComparisonTool.ts`: Pass `"left"` and `"right"` to `resolveArtifactComparisonEvidence` for `suppliedLeft` and `suppliedRight`. - `src/application/artifacts/ArtifactComparisonEvidence.test.ts`: Update existing unit calls and add a parameterized `it.each(["left", "right"] as const)` test verifying that non-inventory summaries report `path: [side]`. - `tests/process-global/derivedOperationFailures.test.ts`: Add a parameterized `it.each(["left", "right"] as const)` MCP tool boundary test verifying that `compare_artifacts` returns `invalid_request` with `issues: [{ path: [side], reason: "invalid_value" }]` and retains no Evidence records on failure. ## Contract and boundary impact - Semantic owner and earliest changed stage: `resolveArtifactComparisonEvidence` (`src/application/artifacts/ArtifactComparisonEvidence.ts`), consumed by `registerArtifactComparisonTool` (`src/server/registerArtifactComparisonTool.ts`). - Contribution scope fit and linked maintainer decision: Not applicable (bug fix for #7089 within existing `compare_artifacts` error contract). - CLI and MCP/tool-catalog contract: Fixes `compare_artifacts` `invalid_request` issue path from nonexistent `["evidence"]` to `["left"]` or `["right"]`. No tool schema or catalog change. - Provider, bridge, target-format, or platform compatibility: None. - Evidence, artifact, provenance, or reconstruction contract: Failed `compare_artifacts` calls continue to retain zero Evidence records; only the `AnalysisInputError` issue path is corrected. - Target selection, authority, upstream behavior, and cleanup details (if applicable): Not applicable. - Generated metadata (`docs/public/product-catalog.json`), package, or installation impact: None. Expected effects (check known effects; use documentation-only on its own). Unchecked items do not establish absence; leave uncertainties visible. - [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 - Tests added or updated: - `src/application/artifacts/ArtifactComparisonEvidence.test.ts` (`it.each(["left", "right"] as const)("names the %s compare_artifacts side when a summary is not an artifact inventory", ...)`) - `tests/process-global/derivedOperationFailures.test.ts` (`it.each(["left", "right"] as const)("classifies a non-inventory inspect_analysis_view summary at %s", ...)`) - Base reproduction or other evidence: - Executed: Prior to this change, `src/application/artifacts/ArtifactComparisonEvidence.ts:27` hardcoded `path: ["evidence"]`, causing both the unit and MCP boundary assertions for `path: ["left"]` and `path: ["right"]` to fail on `main`. - User-visible CLI/MCP output (if applicable): - MCP `compare_artifacts` error payload now reports: ```json { "code": "invalid_request", "details": { "issues": [ { "path": ["left"], "reason": "invalid_value", "message": "Artifact comparison accepts only an artifact inventory summary from inspect_analysis_view." } ] } } ``` (and `"path": ["right"]` when supplied on `right`). - Remaining proof gaps: None. 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 - `npx vitest run src/application/artifacts/ArtifactComparisonEvidence.test.ts tests/process-global/derivedOperationFailures.test.ts` — 2 test files passed, 84 tests passed (`8/8` in `ArtifactComparisonEvidence.test.ts`, `76/76` in `derivedOperationFailures.test.ts`). - `npm run typecheck` — passed (`tsc --noEmit`, 0 errors). - `npm run format:check` — passed (`oxfmt --check .`). - `npm run lint` — passed (`oxlint` 0 warnings/0 errors; `verify:module-boundaries` passed). - `npm run verify:test-discovery` — passed. - `npm run verify:architecture-guards` — passed. - `npm run scan:todos` — passed. - `npm run jscpd` — passed (`0%` duplicated lines). ## Compatibility, safety, and release - Breaking changes or migration steps: None. Fixes an inaccurate error path (`["evidence"]` -> `["left"]` / `["right"]`). - Real Hopper/Ghidra, browser, or OS coverage: Not applicable (pure domain/application Evidence validation and MCP tool boundary test). - Package or release metadata impact: None. - Security, privacy, process, or containment review: None. ## Author readiness Human owner: @hannesstrydom664-hash - [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. - [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-11-03 13:39:43 +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#7693

No description provided.

Rows
Columns