[PR #2494] [MERGED] fix(domain): preserve retryable capability failures through redaction #5273

New issue

Closed

opened 2026-10-31 01:05:21 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-31 01:05:21 +00:00

Owner

📋 Pull Request Information

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

Base: main ← Head: fix/redaction-retryable-preservation

📝 Commits (1)

513d2d0 fix(domain): preserve retryable capability failures through redaction

📊 Changes

2 files changed (+19 additions, -0 deletions)

View changed files

📝 src/domain/explicitSensitiveFailure.test.ts (+18 -0)
📝 src/domain/explicitSensitiveFailure.ts (+1 -0)

📄 Description

Explicit-value redaction rebuilds each error from its fields, and AnalysisCapabilityUnavailableError.retryable was missing from the rebuild. Redaction therefore reset a documented semantic field to false.

Summary

redactExplicitFailure reconstructs typed failures field by field so that explicit sensitive literals are removed while the error stays projected as the same code. For AnalysisCapabilityUnavailableError it carried providerId, operation, reason and userMessage, but not retryable:

return new AnalysisCapabilityUnavailableError(
  error.providerId, error.operation, text(error.reason),
  {
    ...context,
    ...(error.userMessage === undefined ? {} : { userMessage: text(error.userMessage) }),
  },
);

retryable is a semantic field with its own doc comment ("Incomplete cleanup must settle before the operation can be retried") and it drives projectAnalysisError, the error category, and the remediation text the caller is shown. Because the constructor defaults it to false, redacting any capability failure silently converted a transient condition into a permanent one: the remediation changed from "Review details.reason, then retry the same analysis after the condition clears." to "This analysis is unavailable for the current target. Choose another analysis or target." The field was recently added to the error class, and the existing redaction test for this class covers userMessage only, so the loss was uncovered.

Change and scope

explicitSensitiveFailure passes the field through with the same conditional-spread shape already used for userMessage:

...(error.retryable ? { retryable: true } : {}),

Passing it is safe by construction: the constructor computes options?.retryable === true && !this.cleanupIncomplete, and cleanup is already part of the reconstructed context, so a value that was true before redaction stays true and a value that was false stays absent. No other class, projection, presentation or remediation path changes, and no new representation or wrapper is introduced.

The same rebuild covers AnalysisCancelledError, AnalysisTimeoutError, AnalysisAccessDeniedError, AnalysisArtifactChangedError, AnalysisInputError, AnalysisOutputError, AnalysisCapabilityUnavailableError, AnalysisResourceConstraintError, ProviderCleanupError and ProviderAdapterError; AnalysisUnsupportedTargetError is not in the chain and falls through to return error, so its path, reason and remediationAction are returned unredacted. I did not change that here: extending redaction to a class it has never covered is a privacy/evidence decision rather than a field-preservation fix, and the function's doc scopes it to "common acquisition/decoder failures". I am flagging it rather than folding it in.

Contract and boundary impact

Semantic owner and earliest changed stage: typed error reconstruction during explicit-value redaction. Projection, presentation, remediation selection, the error classes and every producer are untouched.
CLI and MCP/tool-catalog contract: no tool name, command, input field, output schema or error code changes. retryable keeps its documented meaning for redacted and unredacted failures alike; only a redacted transient capability failure no longer reports itself as permanent.
Provider, target-format or platform compatibility: none — provider-neutral error modeling.
Evidence, artifact, provenance or reconstruction contract: redaction still removes every explicit literal it removed before; the test asserts the redacted projection does not contain the literal. No Evidence value changes.
Process execution, authorization, cleanup or containment impact: none. The change does not alter cleanup tracking; it preserves the flag that reports whether cleanup already settled.
Generated metadata, package or installation impact: none.

Evidence and regression coverage

A new case in explicitSensitiveFailure.test.ts, "preserves a retryable capability failure through explicit-value redaction", builds a retryable capability failure whose reason embeds an explicit literal, asserts the unredacted projection is already retryable, then asserts the redacted error instance and its projection are both still retryable and still free of the literal.

Before the change it failed with expected false to be true; the 18 existing cases in that file passed throughout, which is why the field was uncovered. Verification on macOS 26.5.1 arm64 with Node.js 24.18.0 and npm 11.16.0, based on bb54082:

npm run test:focused over the redaction suite, the error-projection suite, the access-denied suite and the browser capture-failure suite: 66 passed, 0 failed.
npm run check:fast: passed.
npm run verify:test-discovery: all 1015 test files discovered exactly once.

Unavailable coverage: no Windows or Linux run and no real-provider run — the change is provider-neutral domain logic and the regression input is a constructed error.

_{🔄 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/2494 **Author:** [@XiaoBinGan](https://github.com/XiaoBinGan) **Created:** 10/13/2026 **Status:** ✅ Merged **Merged:** 10/13/2026 **Merged by:** [@morluto](https://github.com/morluto) **Base:** `main` ← **Head:** `fix/redaction-retryable-preservation` --- ### 📝 Commits (1) - [`513d2d0`](https://github.com/morluto/rea/commit/513d2d044ab354342969aafbf81e3f245cfea2ab) fix(domain): preserve retryable capability failures through redaction ### 📊 Changes **2 files changed** (+19 additions, -0 deletions) <details> <summary>View changed files</summary> 📝 `src/domain/explicitSensitiveFailure.test.ts` (+18 -0) 📝 `src/domain/explicitSensitiveFailure.ts` (+1 -0) </details> ### 📄 Description Explicit-value redaction rebuilds each error from its fields, and `AnalysisCapabilityUnavailableError.retryable` was missing from the rebuild. Redaction therefore reset a documented semantic field to `false`. ## Summary [`redactExplicitFailure`](https://github.com/morluto/rea/blob/bb54082/src/domain/explicitSensitiveFailure.ts) reconstructs typed failures field by field so that explicit sensitive literals are removed while the error stays projected as the same code. For `AnalysisCapabilityUnavailableError` it carried `providerId`, `operation`, `reason` and `userMessage`, but not `retryable`: ```ts return new AnalysisCapabilityUnavailableError( error.providerId, error.operation, text(error.reason), { ...context, ...(error.userMessage === undefined ? {} : { userMessage: text(error.userMessage) }), }, ); ``` `retryable` is a semantic field with its own doc comment ("Incomplete cleanup must settle before the operation can be retried") and it drives `projectAnalysisError`, the error category, and the remediation text the caller is shown. Because the constructor defaults it to `false`, redacting any capability failure silently converted a transient condition into a permanent one: the remediation changed from "Review details.reason, then retry the same analysis after the condition clears." to "This analysis is unavailable for the current target. Choose another analysis or target." The field was recently added to the error class, and the existing redaction test for this class covers `userMessage` only, so the loss was uncovered. ## Change and scope [`explicitSensitiveFailure`](https://github.com/XiaoBinGan/rea/blob/HEAD/src/domain/explicitSensitiveFailure.ts) passes the field through with the same conditional-spread shape already used for `userMessage`: ```ts ...(error.retryable ? { retryable: true } : {}), ``` Passing it is safe by construction: the constructor computes `options?.retryable === true && !this.cleanupIncomplete`, and `cleanup` is already part of the reconstructed `context`, so a value that was `true` before redaction stays `true` and a value that was `false` stays absent. No other class, projection, presentation or remediation path changes, and no new representation or wrapper is introduced. ## Related cleanup that remains The same rebuild covers `AnalysisCancelledError`, `AnalysisTimeoutError`, `AnalysisAccessDeniedError`, `AnalysisArtifactChangedError`, `AnalysisInputError`, `AnalysisOutputError`, `AnalysisCapabilityUnavailableError`, `AnalysisResourceConstraintError`, `ProviderCleanupError` and `ProviderAdapterError`; `AnalysisUnsupportedTargetError` is not in the chain and falls through to `return error`, so its `path`, `reason` and `remediationAction` are returned unredacted. I did not change that here: extending redaction to a class it has never covered is a privacy/evidence decision rather than a field-preservation fix, and the function's doc scopes it to "common acquisition/decoder failures". I am flagging it rather than folding it in. ## Contract and boundary impact - **Semantic owner and earliest changed stage:** typed error reconstruction during explicit-value redaction. Projection, presentation, remediation selection, the error classes and every producer are untouched. - **CLI and MCP/tool-catalog contract:** no tool name, command, input field, output schema or error code changes. `retryable` keeps its documented meaning for redacted and unredacted failures alike; only a redacted transient capability failure no longer reports itself as permanent. - **Provider, target-format or platform compatibility:** none — provider-neutral error modeling. - **Evidence, artifact, provenance or reconstruction contract:** redaction still removes every explicit literal it removed before; the test asserts the redacted projection does not contain the literal. No Evidence value changes. - **Process execution, authorization, cleanup or containment impact:** none. The change does not alter cleanup tracking; it preserves the flag that reports whether cleanup already settled. - **Generated metadata, package or installation impact:** none. ## Evidence and regression coverage A new case in [`explicitSensitiveFailure.test.ts`](https://github.com/XiaoBinGan/rea/blob/HEAD/src/domain/explicitSensitiveFailure.test.ts), "preserves a retryable capability failure through explicit-value redaction", builds a retryable capability failure whose reason embeds an explicit literal, asserts the unredacted projection is already `retryable`, then asserts the redacted error instance and its projection are both still `retryable` and still free of the literal. Before the change it failed with `expected false to be true`; the 18 existing cases in that file passed throughout, which is why the field was uncovered. Verification on macOS 26.5.1 arm64 with Node.js 24.18.0 and npm 11.16.0, based on `bb54082`: - `npm run test:focused` over the redaction suite, the error-projection suite, the access-denied suite and the browser capture-failure suite: 66 passed, 0 failed. - `npm run check:fast`: passed. - `npm run verify:test-discovery`: all 1015 test files discovered exactly once. Unavailable coverage: no Windows or Linux run and no real-provider run — the change is provider-neutral domain logic and the regression input is a constructed error. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-31 01:05:21 +00:00