[PR #4481] [CLOSED] fix(reconstruction): report ledger caller-input mistakes as invalid input #6266

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

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/4481
Author: @luigiPinna
Created: 10/16/2026
Status: ❌ Closed

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


📝 Commits (2)

  • aa937ec fix(reconstruction): report ledger caller-input mistakes as invalid input
  • 0abc2c4 fix(reconstruction): tag ledger caller-input failures at their owners

📊 Changes

7 files changed (+292 additions, -194 deletions)

View changed files

📝 src/application/ReconstructionObligationCandidates.test.ts (+16 -2)
📝 src/application/ReconstructionObligationCandidates.ts (+9 -3)
📝 src/application/ReconstructionObligationLedgerService.ts (+14 -5)
📝 src/domain/evidenceBundle.test.ts (+17 -0)
📝 src/domain/evidenceBundle.ts (+9 -1)
📝 tests/boundary/cli/reconstructionObligationLedgerCli.test.ts (+120 -108)
📝 tests/boundary/mcp/reconstructionObligationLedgerMcp.test.ts (+107 -75)

📄 Description

Summary

Fixes #4444. build_reconstruction_obligation_ledger returned an opaque unreadable_output ("run rea doctor") for two caller-input mistakes: a cited review_evidence_id that is absent from evidence_bundle.records, and a bundle containing the same record ID twice. The first is what a user hits by following docs/reconstruction-obligation-ledgers.md literally. Both now return invalid_request / invalid_input with the offending reason in details.issues, matching evaluate_reconstruction_coverage.

Problem and expected behavior

buildReconstructionObligationLedgerEvidenceValidated (src/application/ReconstructionObligationLedgerService.ts) mapped only NonCanonicalEvidenceBundleError and z.ZodError to workflowInputError; every other TypeError became AnalysisProtocolError, presented as unreadable_output with no details. The two inputs raise plain TypeErrors on the ledger path: Reviewed obligation Evidence is missing: <id> (ReconstructionObligationCandidates.ts) and Evidence bundle contains duplicate record IDs (evidenceBundle.ts, via deriveReconstructionObligationCandidates). The sibling ReconstructionCoverageService already classifies cause instanceof TypeError || cause instanceof z.ZodError as caller input.

Change and scope

The catch block now mirrors ReconstructionCoverageService: TypeError or z.ZodError → workflowInputError; anything else stays AnalysisProtocolError. Because NonCanonicalEvidenceBundleError extends TypeError, its explicit branch and import are removed as redundant; the non-canonical case is unchanged. The unexpected-error fallback is intentionally kept as unreadable_output rather than widened, so genuine internal failures still read as internal. No schema, catalog, provider, or domain throw-site change. This also brings the other caller-reachable TypeErrors on this path (e.g. the residual-unknown graph checks noted in the issue) under the same invalid_input classification.

Contract and boundary impact

  • Semantic owner and earliest changed stage: the ledger application service owns error classification; the TypeErrors originate in the obligation-candidate derivation and evidence-bundle authentication it calls.

  • Contribution scope fit and linked maintainer decision: ordinary correctness fix within an existing read-only workflow; not applicable.

  • CLI and MCP/tool-catalog contract: unchanged; only the error envelope for already-failing inputs changes (unreadable_output → invalid_request).

  • Provider, bridge, target-format, or platform compatibility: provider-independent.

  • Evidence, artifact, provenance, or reconstruction contract: a malformed caller bundle is now reported as caller input with the reason, not as an internal unreadable result. Valid ledgers are unchanged.

  • Generated metadata, package, or installation impact: none.

  • Read existing files or captured data.

Evidence and regression coverage

  • src/application/ReconstructionObligationLedgerService.test.ts (new, services lane): asserts the returned Result error is AnalysisInputError with the expected issue for (a) absent review evidence and (b) duplicate record, plus a non-canonical control. On unmodified src, the two new cases fail (AnalysisProtocolError); with the fix they pass; reverting the fix fails them again.
  • tests/boundary/cli/reconstructionObligationLedgerCli.test.ts: adds the two cases at the CLI boundary (asserting code: invalid_request, category: invalid_input, and the details.issues message). On a main build the two new cases return unreadable_output and fail; after the fix all four pass. The existing non-canonical and empty-ledger tests were refactored onto a shared runLedgerCli helper with no behavior change.
  • Remaining proof gaps: verified on the main/source CLI; not run against the stdio MCP adapter or a published rea-agents build (same shared service, so source-derived parity). The broadened TypeError branch is the same heuristic the sibling service uses; the more precise alternative (typed input errors at the two throw sites) is a larger domain-boundary change left for maintainer direction.

Validation performed

  • npx vitest run --project services src/application/ReconstructionObligationLedgerService.test.ts — 3 passed (2/3 fail on unmodified source; revert reproduces).
  • npx vitest run --project cli-boundary tests/boundary/cli/reconstructionObligationLedgerCli.test.ts — 4 passed (2 fail on a main build).
  • npx vitest run --project services — 84 files, 1089 passed.
  • npm run typecheck, oxlint, oxfmt --check, npm run verify:module-boundaries, npm run verify:architecture-guards, npx knip — all clean.

Compatibility, safety, and release

  • Breaking changes or migration steps: none. Previously-failing inputs still fail; only the error code/category/details improve. Callers keying on unreadable_output for these two inputs would now see invalid_request (retryable: true).
  • Real Hopper/Ghidra, browser, or OS coverage: not applicable (offline classification change). macOS arm64, Node 24.15.
  • Package or release metadata impact: none.
  • Security, privacy, process, or containment review: no new target interaction; the issue message is the existing TypeError text (bundle/obligation IDs the caller supplied).

Author readiness

Human owner: @luigiPinna

  • 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. (error-envelope change is covered by the CLI-boundary test; raw before/after JSON not pasted here.)
  • I checked the final diff for secrets, unrelated cleanup, and unsupported claims.

🤖 Generated with Claude Code


🔄 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/4481 **Author:** [@luigiPinna](https://github.com/luigiPinna) **Created:** 10/16/2026 **Status:** ❌ Closed **Base:** `main` ← **Head:** `fix/obligation-ledger-input-errors-4444` --- ### 📝 Commits (2) - [`aa937ec`](https://github.com/morluto/rea/commit/aa937ec97db3a202090fdc94fb9f7564cc04ca3a) fix(reconstruction): report ledger caller-input mistakes as invalid input - [`0abc2c4`](https://github.com/morluto/rea/commit/0abc2c405372b11995dfa854cd2bf2882b3afdda) fix(reconstruction): tag ledger caller-input failures at their owners ### 📊 Changes **7 files changed** (+292 additions, -194 deletions) <details> <summary>View changed files</summary> 📝 `src/application/ReconstructionObligationCandidates.test.ts` (+16 -2) 📝 `src/application/ReconstructionObligationCandidates.ts` (+9 -3) 📝 `src/application/ReconstructionObligationLedgerService.ts` (+14 -5) 📝 `src/domain/evidenceBundle.test.ts` (+17 -0) 📝 `src/domain/evidenceBundle.ts` (+9 -1) 📝 `tests/boundary/cli/reconstructionObligationLedgerCli.test.ts` (+120 -108) 📝 `tests/boundary/mcp/reconstructionObligationLedgerMcp.test.ts` (+107 -75) </details> ### 📄 Description ## Summary Fixes #4444. `build_reconstruction_obligation_ledger` returned an opaque `unreadable_output` ("run `rea doctor`") for two caller-input mistakes: a cited `review_evidence_id` that is absent from `evidence_bundle.records`, and a bundle containing the same record ID twice. The first is what a user hits by following `docs/reconstruction-obligation-ledgers.md` literally. Both now return `invalid_request` / `invalid_input` with the offending reason in `details.issues`, matching `evaluate_reconstruction_coverage`. ## Problem and expected behavior `buildReconstructionObligationLedgerEvidenceValidated` (`src/application/ReconstructionObligationLedgerService.ts`) mapped only `NonCanonicalEvidenceBundleError` and `z.ZodError` to `workflowInputError`; every other `TypeError` became `AnalysisProtocolError`, presented as `unreadable_output` with no `details`. The two inputs raise plain `TypeError`s on the ledger path: `Reviewed obligation Evidence is missing: <id>` (`ReconstructionObligationCandidates.ts`) and `Evidence bundle contains duplicate record IDs` (`evidenceBundle.ts`, via `deriveReconstructionObligationCandidates`). The sibling `ReconstructionCoverageService` already classifies `cause instanceof TypeError || cause instanceof z.ZodError` as caller input. ## Change and scope The catch block now mirrors `ReconstructionCoverageService`: `TypeError` or `z.ZodError` → `workflowInputError`; anything else stays `AnalysisProtocolError`. Because `NonCanonicalEvidenceBundleError extends TypeError`, its explicit branch and import are removed as redundant; the non-canonical case is unchanged. The unexpected-error fallback is intentionally kept as `unreadable_output` rather than widened, so genuine internal failures still read as internal. No schema, catalog, provider, or domain throw-site change. This also brings the other caller-reachable `TypeError`s on this path (e.g. the residual-unknown graph checks noted in the issue) under the same `invalid_input` classification. ## Contract and boundary impact - Semantic owner and earliest changed stage: the ledger application service owns error classification; the `TypeError`s originate in the obligation-candidate derivation and evidence-bundle authentication it calls. - Contribution scope fit and linked maintainer decision: ordinary correctness fix within an existing read-only workflow; not applicable. - CLI and MCP/tool-catalog contract: unchanged; only the error envelope for already-failing inputs changes (`unreadable_output` → `invalid_request`). - Provider, bridge, target-format, or platform compatibility: provider-independent. - Evidence, artifact, provenance, or reconstruction contract: a malformed caller bundle is now reported as caller input with the reason, not as an internal unreadable result. Valid ledgers are unchanged. - Generated metadata, package, or installation impact: none. - [x] Read existing files or captured data. ## Evidence and regression coverage - `src/application/ReconstructionObligationLedgerService.test.ts` (new, `services` lane): asserts the returned `Result` error is `AnalysisInputError` with the expected issue for (a) absent review evidence and (b) duplicate record, plus a non-canonical control. On unmodified `src`, the two new cases fail (`AnalysisProtocolError`); with the fix they pass; reverting the fix fails them again. - `tests/boundary/cli/reconstructionObligationLedgerCli.test.ts`: adds the two cases at the CLI boundary (asserting `code: invalid_request`, `category: invalid_input`, and the `details.issues` message). On a `main` build the two new cases return `unreadable_output` and fail; after the fix all four pass. The existing non-canonical and empty-ledger tests were refactored onto a shared `runLedgerCli` helper with no behavior change. - Remaining proof gaps: verified on the `main`/source CLI; not run against the stdio MCP adapter or a published `rea-agents` build (same shared service, so source-derived parity). The broadened `TypeError` branch is the same heuristic the sibling service uses; the more precise alternative (typed input errors at the two throw sites) is a larger domain-boundary change left for maintainer direction. ## Validation performed - `npx vitest run --project services src/application/ReconstructionObligationLedgerService.test.ts` — 3 passed (2/3 fail on unmodified source; revert reproduces). - `npx vitest run --project cli-boundary tests/boundary/cli/reconstructionObligationLedgerCli.test.ts` — 4 passed (2 fail on a `main` build). - `npx vitest run --project services` — 84 files, 1089 passed. - `npm run typecheck`, `oxlint`, `oxfmt --check`, `npm run verify:module-boundaries`, `npm run verify:architecture-guards`, `npx knip` — all clean. ## Compatibility, safety, and release - Breaking changes or migration steps: none. Previously-failing inputs still fail; only the error code/category/details improve. Callers keying on `unreadable_output` for these two inputs would now see `invalid_request` (`retryable: true`). - Real Hopper/Ghidra, browser, or OS coverage: not applicable (offline classification change). macOS arm64, Node 24.15. - Package or release metadata impact: none. - Security, privacy, process, or containment review: no new target interaction; the issue message is the existing `TypeError` text (bundle/obligation IDs the caller supplied). ## Author readiness Human owner: @luigiPinna - [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. - [ ] User-visible CLI/MCP changes include representative output. _(error-envelope change is covered by the CLI-boundary test; raw before/after JSON not pasted here.)_ - [x] I checked the final diff for secrets, unrelated cleanup, and unsupported claims. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --- <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:09 +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#6266
No description provided.