morluto/rea

Fork 0

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

[PR #4452] [MERGED] fix(evidence): classify ledger duplicate and missing review Evidence as caller input #6252

New issue

Closed

opened 2026-10-31 01:13:03 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-31 01:13:03 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/4452
Author: @amyanger
Created: 10/16/2026
Status: ✅ Merged
Merged: 10/16/2026
Merged by: @morluto

Base: main ← Head: fix/4444-ledger-input-errors

📝 Commits (1)

48d78b9 fix(evidence): classify ledger duplicate and missing review Evidence as caller input

📊 Changes

4 files changed (+111 additions, -8 deletions)

View changed files

📝 src/application/ReconstructionObligationCandidates.ts (+9 -3)
📝 src/application/ReconstructionObligationLedgerService.ts (+13 -3)
📝 src/domain/evidenceBundle.ts (+10 -2)
📝 tests/boundary/mcp/reconstructionObligationLedgerMcp.test.ts (+79 -0)

📄 Description

Summary

Fixes #4444.

build_reconstruction_obligation_ledger reported two caller-input mistakes as execution_failure / unreadable_output with no details, telling the caller to run rea doctor. Both now return invalid_request / invalid_input with the reason in details.issues, matching evaluate_reconstruction_coverage.

Problem and expected behavior

Two inputs reached the service's catch as plain TypeErrors and fell through to AnalysisProtocolError:

A reviewed obligation whose review_evidence_ids names a record that is not in evidence_bundle.records (what you get by pasting the sample obligation from docs/reconstruction-obligation-ledgers.md into the sample request).
An Evidence bundle that lists the same record twice.

Both are caller input, so the caller should get an input error naming the problem.

Change and scope

This follows the approach from 3a34ad37b (#3219): one named error class per caller-input condition, thrown at its source and matched explicitly in the service. That commit kept unrelated TypeErrors as protocol failures on purpose, so this PR does not copy the blanket TypeError mapping used by ReconstructionCoverageService.

src/domain/evidenceBundle.ts: add DuplicateEvidenceRecordError (extends TypeError, same message) and throw it at both duplicate-ID checks.
src/application/ReconstructionObligationCandidates.ts: add MissingReviewEvidenceError (extends TypeError, same message) for the missing review Evidence check.
src/application/ReconstructionObligationLedgerService.ts: map both to workflowInputError, next to NonCanonicalEvidenceBundleError.

Both classes extend TypeError and keep the existing messages, so any caller matching on TypeError or the message text is unaffected.

Not included: the residual-unknown graph TypeErrors in evidenceBundle.ts that the issue mentions as possibly affected. I did not reproduce those, so I left them out rather than reclassify them untested. They could follow the same pattern in a follow-up.

Contribution scope: ordinary fix within existing boundaries; not applicable.

Contract and boundary impact

Semantic owner and earliest changed stage: reconstruction obligation ledger service, error classification after request parsing.
Contribution scope fit and linked maintainer decision: not applicable (bug fix within existing boundaries).
CLI and MCP/tool-catalog contract: no schema change. The error code/category for the two inputs above changes from unreadable_output / execution_failure to invalid_request / invalid_input, which is the documented classification for caller input. The CLI (src/cli/applicationCommands.ts) and MCP tool (src/server/registerApplicationTools/obligationLedger.ts) both call buildReconstructionObligationLedgerEvidenceValidated.
Provider, bridge, target-format, or platform compatibility: none.
Evidence, artifact, provenance, or reconstruction contract: none. Valid requests produce the same ledger.
Target selection, authority, upstream behavior, and cleanup details: not applicable.
Generated metadata, package, or installation impact: none.

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.

(No target interaction. This only changes how invalid request JSON is classified.)

Evidence and regression coverage

Tests added or updated: tests/boundary/mcp/reconstructionObligationLedgerMcp.test.ts adds one test covering both inputs through the real MCP server. Setup for the new test lives in module-level helpers so the existing tests are unchanged.

Base reproduction: with the service change reverted, the new test fails with the reported classification:

×  returns duplicate records and missing review Evidence as caller input errors
+     "category": "execution_failure",
+     "code": "unreadable_output",

User-visible MCP output after the fix (asserted in the test):

{"error":{"code":"invalid_request","category":"invalid_input",
  "details":{"operation":"build_reconstruction_obligation_ledger",
    "issues":[{"message":"Evidence bundle contains duplicate record IDs", ...}]}}}

and Reviewed obligation Evidence is missing: ev_aaaa... for the second case.

Remaining proof gaps: I did not run the built CLI end to end. The CLI uses the same service function, and the MCP test exercises the real server path.
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

Node 24.18.0 (.nvmrc), macOS arm64.

npm run test:focused -- tests/boundary/mcp/reconstructionObligationLedgerMcp.test.ts src/domain/evidenceBundle.test.ts src/application/ReconstructionObligationLedger.authority.test.ts src/application/ReconstructionObligationLedger.closure.test.ts: 22 passed.
Same MCP test with only the service change reverted: new test fails with unreadable_output (above).
npm run check: 6/6 tasks successful (typecheck, lint, format, knip, metadata), 0 lint errors; verify:architecture-guards passed.

Compatibility, safety, and release

Breaking changes or migration steps: none. Error classes extend TypeError with unchanged messages.
Real Hopper/Ghidra, browser, or OS coverage: not applicable.
Package or release metadata impact: none.
Security, privacy, process, or containment review: not applicable.

Author readiness

Human owner: @amyanger

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/4452 **Author:** [@amyanger](https://github.com/amyanger) **Created:** 10/16/2026 **Status:** ✅ Merged **Merged:** 10/16/2026 **Merged by:** [@morluto](https://github.com/morluto) **Base:** `main` ← **Head:** `fix/4444-ledger-input-errors` --- ### 📝 Commits (1) - [`48d78b9`](https://github.com/morluto/rea/commit/48d78b9b47cec6fdc5700ec1887a1033c77ad361) fix(evidence): classify ledger duplicate and missing review Evidence as caller input ### 📊 Changes **4 files changed** (+111 additions, -8 deletions) <details> <summary>View changed files</summary> 📝 `src/application/ReconstructionObligationCandidates.ts` (+9 -3) 📝 `src/application/ReconstructionObligationLedgerService.ts` (+13 -3) 📝 `src/domain/evidenceBundle.ts` (+10 -2) 📝 `tests/boundary/mcp/reconstructionObligationLedgerMcp.test.ts` (+79 -0) </details> ### 📄 Description ## Summary Fixes #4444. `build_reconstruction_obligation_ledger` reported two caller-input mistakes as `execution_failure` / `unreadable_output` with no `details`, telling the caller to run `rea doctor`. Both now return `invalid_request` / `invalid_input` with the reason in `details.issues`, matching `evaluate_reconstruction_coverage`. ## Problem and expected behavior Two inputs reached the service's catch as plain `TypeError`s and fell through to `AnalysisProtocolError`: 1. A reviewed obligation whose `review_evidence_ids` names a record that is not in `evidence_bundle.records` (what you get by pasting the sample obligation from `docs/reconstruction-obligation-ledgers.md` into the sample request). 2. An Evidence bundle that lists the same record twice. Both are caller input, so the caller should get an input error naming the problem. ## Change and scope This follows the approach from 3a34ad37b (#3219): one named error class per caller-input condition, thrown at its source and matched explicitly in the service. That commit kept unrelated `TypeError`s as protocol failures on purpose, so this PR does not copy the blanket `TypeError` mapping used by `ReconstructionCoverageService`. - `src/domain/evidenceBundle.ts`: add `DuplicateEvidenceRecordError` (extends `TypeError`, same message) and throw it at both duplicate-ID checks. - `src/application/ReconstructionObligationCandidates.ts`: add `MissingReviewEvidenceError` (extends `TypeError`, same message) for the missing review Evidence check. - `src/application/ReconstructionObligationLedgerService.ts`: map both to `workflowInputError`, next to `NonCanonicalEvidenceBundleError`. Both classes extend `TypeError` and keep the existing messages, so any caller matching on `TypeError` or the message text is unaffected. Not included: the residual-unknown graph `TypeError`s in `evidenceBundle.ts` that the issue mentions as possibly affected. I did not reproduce those, so I left them out rather than reclassify them untested. They could follow the same pattern in a follow-up. Contribution scope: ordinary fix within existing boundaries; not applicable. ## Contract and boundary impact - Semantic owner and earliest changed stage: reconstruction obligation ledger service, error classification after request parsing. - Contribution scope fit and linked maintainer decision: not applicable (bug fix within existing boundaries). - CLI and MCP/tool-catalog contract: no schema change. The error code/category for the two inputs above changes from `unreadable_output` / `execution_failure` to `invalid_request` / `invalid_input`, which is the documented classification for caller input. The CLI (`src/cli/applicationCommands.ts`) and MCP tool (`src/server/registerApplicationTools/obligationLedger.ts`) both call `buildReconstructionObligationLedgerEvidenceValidated`. - Provider, bridge, target-format, or platform compatibility: none. - Evidence, artifact, provenance, or reconstruction contract: none. Valid requests produce the same ledger. - Target selection, authority, upstream behavior, and cleanup details: not applicable. - Generated metadata, package, or installation impact: none. 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. (No target interaction. This only changes how invalid request JSON is classified.) ## Evidence and regression coverage - Tests added or updated: `tests/boundary/mcp/reconstructionObligationLedgerMcp.test.ts` adds one test covering both inputs through the real MCP server. Setup for the new test lives in module-level helpers so the existing tests are unchanged. - Base reproduction: with the service change reverted, the new test fails with the reported classification: ``` × returns duplicate records and missing review Evidence as caller input errors + "category": "execution_failure", + "code": "unreadable_output", ``` - User-visible MCP output after the fix (asserted in the test): ```json {"error":{"code":"invalid_request","category":"invalid_input", "details":{"operation":"build_reconstruction_obligation_ledger", "issues":[{"message":"Evidence bundle contains duplicate record IDs", ...}]}}} ``` and `Reviewed obligation Evidence is missing: ev_aaaa...` for the second case. - Remaining proof gaps: I did not run the built CLI end to end. The CLI uses the same service function, and the MCP test exercises the real server path. - [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 Node 24.18.0 (`.nvmrc`), macOS arm64. - `npm run test:focused -- tests/boundary/mcp/reconstructionObligationLedgerMcp.test.ts src/domain/evidenceBundle.test.ts src/application/ReconstructionObligationLedger.authority.test.ts src/application/ReconstructionObligationLedger.closure.test.ts`: 22 passed. - Same MCP test with only the service change reverted: new test fails with `unreadable_output` (above). - `npm run check`: 6/6 tasks successful (typecheck, lint, format, knip, metadata), 0 lint errors; `verify:architecture-guards` passed. ## Compatibility, safety, and release - Breaking changes or migration steps: none. Error classes extend `TypeError` with unchanged messages. - Real Hopper/Ghidra, browser, or OS coverage: not applicable. - Package or release metadata impact: none. - Security, privacy, process, or containment review: not applicable. ## Author readiness Human owner: @amyanger - [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-10-31 01:13:03 +00:00

closed this issue
added the
pull-request
label

bsakaguchi referenced this issue

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

[PR #6258] [MERGED] fix(cli): exclude declared sensitive values from parser diagnostics #7233

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#6252

No description provided.

Rows
Columns