Repository navigation
bip93: Move the CLI's friendly share-set wording into the library - #161
BenWestgate wants to merge 2 commits into
Conversation
The CLI kept its own table of friendlier messages for library exceptions, and the GUI branch keeps a third copy. Put the wording in the library exceptions so every frontend shows the same text, and delete the CLI table. This also fixes the threshold-0 case: the CLI mapped it to "These strings require different numbers of shares." Mixed applications now show the library's "ms and bip39_12w cannot be combined". Refs #156. Claude-Session: https://claude.ai/code/session_01Q1TNm1YeYW8K9W2eBvxmCD
BenWestgate
left a comment
BenWestgate
left a comment
There was a problem hiding this comment.
AI-assisted current-head code/security review.
Code ACK f10a5c7 (static). The changes move user-facing error strings from the CLI mapping to the existing library exceptions without changing acceptance, checksum, or interpolation logic. Threshold-zero errors now describe the actual condition. Retest CLI and GUI together after merging stacked #155/#157; current head remains draft.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
AI-assisted triage: the earlier feedback is on #158 (review threads from Oct 22, 11:22–11:33 UTC): whether to retain |
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
|
Correction to the triage above: #161 is stacked on #157, not on #158. Both #158 (Codex, Generated by Claude Code |
BenWestgate
left a comment
BenWestgate
left a comment
There was a problem hiding this comment.
Focused code/security review — commit 10c8f9bbc9ab2acc59e78cc3b2aa440e255745ea.
No security findings in the PR diff or the additional 10c8f9b recovery refactor. recover_secret now calls _validate_recovery_prefix(..., require_exact=True); it still applies the same bounded validated-artifact checks, exact threshold count, compatibility and distinct-index checks, and SecretInRecoverySet rejection before interpolation. Error precedence is unchanged. The checksum checks and exception classes remain intact. New exceptions and CLI messages disclose no seed/share payload; the HRP mismatch reports only application prefixes.
Checked the repository security invariants, parser/CLI call sites, all six changed files, #158 feedback and its dispositions. The exact-head Python package CI workflow (run 39807698073) succeeded across its reported jobs. Local test execution against the PR checkout was unavailable because the local fetch failed DNS resolution for github.com; this is a static review backed by exact-head CI, not a claim that I reran the suite.
Security ACK for 10c8f9b. The stacked #155/#157 integration and combined CLI/GUI retest remain separate release gates.
|
@codex review |
|
Codex Review: Didn't find any major issues. Hooray! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Before: the CLI used its own
_FRIENDLY_SET_ERRORStable to swap terse library messages ("share identifiers do not match", "invalid Long codex32 checksum") for friendly ones. The GUI branch has a third copy inpages.py_incompatible. When a user triedsharewith an unshared secret, the table showed the wrong message: "These strings require different numbers of shares."After: the library exceptions use the friendly wording, so the CLI, a GUI and API callers all show the same text. The CLI table is gone. With an unshared secret,
sharenow says "An unshared secret (threshold 0) was never split into shares." Mixing applications shows the library's "ms and bip39_12w cannot be combined" (Ben OK'd that).How: I changed the messages in
bip93.pyandbech32.py, deleted the table and its imports in_cli_input.py, updated two pinned tests and added a test for the threshold-0 case. Installed logical lines go from 4,844 to 4,830. Exception types are unchanged.Stacked on #157 (itself stacked on #155). Refs #156. It touches checksum and recovery error messages only, not their logic, but per project policy it still needs a security review before it is marked ready.
Checks run locally:
python -m pytestandpython -O -m pytest(909 passed), ruff, mypy,verify_correction_constants,differential_correction --verify.Generated by Claude Code