errors: Move friendly recovery messages into library by BenWestgate · Pull Request #158 · BenWestgate/python-codex32

BenWestgate · 2026-10-22T09:51:18Z

Move the CLI's _FRIENDLY_SET_ERRORS messages into the library exceptions so interactive recovery, redirected input, and direct API users receive the same diagnostic. This makes the library the single source of truth for share-set and checksum errors, as decided in #156.

What changes

Remove the CLI-only exception-message map; keep exception types, validation order, recovery arithmetic, and candidate/correction selection unchanged.
Give invalid threshold-0 recovery input its own correct diagnostic rather than describing it as different numbers of shares.
Preserve existing friendly CLI output for checksum, incompatible profiles, differing thresholds/identifiers/lengths, duplicate indices, and shared-secret entry.
Extend tests to assert direct library error messages for bad checksums and each incompatible share-set case.

Security review scope
Review error handling in bip93.py and _cli_input.py: ensure all invalid inputs remain rejected, no wrong share sets can be combined, error messages do not expose backup data, and interactive recovery never substitutes diagnostics for confirmation. The diff does not change predicates, branch conditions, interpolation, or correction behavior.

Stacking
Draft PR stacked on #157 (itself stacked on #155); retarget after upstream merges. PR #146 contains an earlier similar B3 cleanup and will need deduplication/rebase before that work is merged.

Refs #156

Verification (22 October 2026)

Python package CI run 39758625436: all 10 matrix jobs passed, including normal and optimized pytest, strict mypy, Ruff lint/format, wheel checks, and correction verification. Ubuntu/Python 3.12 reported 909 passed.
Local focused checks: 89 tests passed (test_bip93.py, test_cli_lengths.py); Ruff lint and formatting passed.
Security diff scan on 50cbc19..9404991: complete, 0 findings (scan b8ed210a-5db6-4492-98ff-779f20a44e89); review 5612446714.

Move friendly recovery and checksum diagnostics from the CLI exception map to the underlying library errors. Keep all exception types and validation decisions unchanged so callers of the public API and interactive commands see the same message. Distinguish an invalid threshold-zero recovery input from two valid shares with mismatched thresholds, and assert the messages in the existing malformed-checksum and share-set tests. Refs #156

BenWestgate

Security diff review — PR #158 at 9404991 (base 50cbc19).

Result: no actionable security findings in the scoped patch. The completed Codex Security diff scan reviewed the two changed source files against the repository security invariants, including checksum-validation order, incompatible-share rejection, correction confirmation, and diagnostic confidentiality.

bip93.py: the new text changes only the arguments to existing exception types; checksum verification, threshold tests, share-set validation, interpolation, and output reparse are unmodified. A threshold-0 recovery input now gets the correct distinct diagnostic.
_cli_input.py: removal of _FRIENDLY_SET_ERRORS delegates rendering to library exceptions; CLI rejection and retry branches, parse-error handling, correction eligibility, and exact-string confirmation are unchanged.
Data exposure: messages contain no seeds, share contents, private keys, or application prefix values.
Regression evidence: direct checksum and sharing tests now assert the exception messages; 89 focused tests passed locally. Ruff lint and format checks pass. Nine GitHub Actions jobs passed at review time; the Ubuntu/Python 3.13 optimized-mode job was still running.

The scan is a focused diff review, not an audit of unrelated code. Keep this PR draft until the full matrix is green and the stacked #155 → #157 base PRs are merged/rebased. PR #146 includes the earlier B3 implementation; deduplicate that overlap before integrating both branches.

Security report: completed scan b8ed210a-5db6-4492-98ff-779f20a44e89 (0 findings).

BenWestgate

cACK

a few questions about making the messages more dynamic and relevant.

BenWestgate · 2026-10-22T11:22:46Z

    Header._from_symbols(body[:6])
    if not bech32_verify_checksum(hrp, encoded, checksum):
-        raise InvalidChecksum(f"invalid {checksum.kind} checksum")
+        raise InvalidChecksum("The checksum does not match.")


should we keep {checksum.kind} or not?

Drop it. Which checksum applies follows from the string's length, and naming it ("invalid Long codex32 checksum") gives the user nothing to act on. #161 drops it too, so both codex32 and bech32 checksum failures read "The checksum does not match." The kind field stays because tests/test_crc.py reads it.

Generated by Claude Code

BenWestgate · 2026-10-22T11:24:07Z

    for item in copied[1:]:
        if item.hrp != first.hrp:
-            raise MismatchedHrp(f"{first.hrp} and {item.hrp} cannot be combined")
+            raise MismatchedHrp("These strings are for different applications.")


did you like showing the mismatch or is the new string best?

this diff uses strings repeatedly but sometimes a more user friendly message may say shares when the provided data was all non-S

Showing the mismatch is better. #161 keeps the library's "ms and bip39_12w cannot be combined", which you said seemed fine earlier today. On shares versus strings: #161 (10c8f9b) now says "shares" when every input is an ordinary share and "strings" when a secret is among them.

Generated by Claude Code

BenWestgate · 2026-10-22T11:30:52Z

+            raise MismatchedThreshold("These strings require different numbers of shares.")
        if item.header.identifier != first.header.identifier:
-            raise MismatchedIdentifier("share identifiers do not match")
+            raise MismatchedIdentifier("These strings have different identifiers.")


this is a situation where strings or shares could be correct. i nit slightly prefer to use the one our set is.

Done in #161 (10c8f9b). The threshold, identifier and length mismatches all pick the noun from the set itself. "These strings require different numbers of shares" also became "These shares have different thresholds" (or "These strings have…") so that it reads well with either noun.

Generated by Claude Code

BenWestgate · 2026-10-22T11:31:32Z

        )
        if item_shape != (len(first.payload_symbols), checksum_length, encoded_length):
-            raise MismatchedPayloadLength("share payload/checksum lengths do not match")
+            raise MismatchedPayloadLength("These strings have different lengths.")


same shares sometimes, strings other times.

Same fix: the length mismatch uses the set's noun in #161 (10c8f9b).

Generated by Claude Code

BenWestgate · 2026-10-22T11:33:07Z

@@ -342,7 +342,7 @@ def recover_secret(shares: Sequence[Share]) -> Secret:
    """Recover S from exactly k compatible, distinct ordinary shares."""
    share_set = _validate_share_set(shares)
    if any(isinstance(item, Secret) for item in share_set.artifacts):
-        raise SecretInRecoverySet("recovery accepts ordinary shares only")
+        raise SecretInRecoverySet("Enter ordinary shares rather than the shared secret.")


why are we duplicating this logic?

We shouldn't. In #161 (10c8f9b), _validate_recovery_prefix takes require_exact and returns the share set, and recover_secret calls it, so the secret-in-recovery check lives in one place. The order of checks is unchanged. Full suite passes (910, normal and -O).

Generated by Claude Code

Say "shares" when every input is an ordinary share and "strings" when a secret is among them, and word the threshold mismatch as "different thresholds" so it reads well either way. recover_secret now reuses _validate_recovery_prefix instead of repeating its secret check. Addresses Ben's review of the duplicate #158. Claude-Session: https://claude.ai/code/session_01Q1TNm1YeYW8K9W2eBvxmCD

BenWestgate · 2026-10-22T19:27:47Z

Closing as superseded by #161 (head 10c8f9b). Both PRs remove the CLI-only friendly-error mapping in favor of library diagnostics, but #161 additionally addresses the review feedback here: simpler checksum wording, context-sensitive “shares”/“strings” messages, preserved application mismatch information, and centralized recovery-prefix validation. The feedback on this PR was answered in its review threads and is reflected in #161.

#161 received a focused security review of the exact 10c8f9b head (review 5619171073), with no findings, and its exact-head CI passed. Continue integration on #161, not this duplicate; preserve this PR's discussion as review history.

BenWestgate commented Oct 22, 2026

View reviewed changes

BenWestgate mentioned this pull request Oct 22, 2026

Remove duplicated user messages #156

Open

BenWestgate added this pull request to stack #162 October 22, 2026 11:18

BenWestgate commented Oct 22, 2026

View reviewed changes

BenWestgate mentioned this pull request Oct 22, 2026

bip93: Move the CLI's friendly share-set wording into the library #161

Open

BenWestgate closed this Oct 22, 2026

Conversation

BenWestgate commented Oct 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BenWestgate left a comment

Choose a reason for hiding this comment

Uh oh!

BenWestgate left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BenWestgate commented Oct 22, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

BenWestgate commented Oct 22, 2026 •

edited

Loading