Repository navigation
cli: Give no edit hints when no correction is found - #155
BenWestgate wants to merge 3 commits into
Conversation
BenWestgate
left a comment
BenWestgate
left a comment
There was a problem hiding this comment.
Security review — PR #155, head 09c25873df4b6b7c53a14bb9e19a0a5f648484d9 (base 9b99862eb14d15d86977cec202a6d39abcac2a88).
Result: No security vulnerabilities identified in the changed code. Reviewed all four changed files and traced error handling through _parse, _suggestions, _redirected, _interactive, _creation_source, _validate_operational_artifact, and _confirm_correction.
- Failed parsing/correction now suppresses edit hints; candidate selection, validated-artifact checks, recovery-set compatibility, whole-card correction confirmation, and Bitcoin Core wallet initialization are unchanged.
checkdeliberately keeps detailed validation errors and does not offer a correction. Valid but incompatible or duplicate shares still produce operational rejection reasons.create --existingis restricted to interactive terminals, and unsuccessful correction remains a rejection; no failed input becomes accepted as recovery material.- The exact-head Python package workflow (run 39717465370) passed all reported Linux, macOS, Windows, and compatibility jobs. New tests cover bad threshold/length/character input, redirected entry, and existing-seed retry.
Risk: High potential impact if wallet-recovery validation were broken, but low regression likelihood from this message-only change. Revert is straightforward; no persisted format or security policy changes.
Review limitation: This is a source-based review of GitHub's exact PR diff and revision. Local test execution and the managed Codex Security scan were unavailable because the PR base commit is absent locally (the isolated clone also failed DNS resolution). CI did complete successfully at this head.
Disposition: No security blocker found in the reviewed diff. Normal human review remains appropriate before merging.
When an entry didn't parse and the correction search found no unique repair, secret, share, wallet, xprv and create --existing printed the parse error: a bad character's position, a threshold hint after the prefilled MS1, or how many characters were missing. Each invites a guess, and a wrong guess adds an error that can make a wrong correction findable. Say only "No unique correction found. Check your backup and re-enter it carefully.", as `correct` already does. Reasons about which valid card was entered (wrong application, duplicate share) stay. `check` keeps its diagnosis, since it never offers a correction, using the standard parse messages: the prefill-specific threshold and length wording only repeated them. Claude-Session: https://claude.ai/code/session_01Q1TNm1YeYW8K9W2eBvxmCD
0273aac to
377f2bc
Compare
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. |
BenWestgate
left a comment
BenWestgate
left a comment
There was a problem hiding this comment.
AI-assisted current-head review.
Code ACK 377f2bc (static). The generic no-correction message suppresses recovery edit hints; the parser and acceptance/confirmation paths remain unchanged. Existing tests cover both interactive and redirected errors. Tests not rerun here.
BenWestgate
left a comment
BenWestgate
left a comment
There was a problem hiding this comment.
Focused current-head security follow-up — 377f2bc955cb2904543842a5b4d16224475927c7.
No additional security findings. This complements the earlier security review of 09c2587 (review 5609013587). I compared the old reviewed revision and the current squashed head and rechecked the current four-file PR diff. The final delta removes the redundant _prefixed_input_error diagnostic helper and its length-hint constant, with matching tests; src/codex32/cli.py is byte-identical to the reviewed revision. Both revisions retain the same NO_CORRECTION fallback, the same correction candidate checks, and the same operator confirmation. The affected code still rejects unparseable input when correction finds no unique candidate; check continues to expose diagnostics without offering repair.
The checksum/parser, validated-artifact recovery boundary, eligibility and wallet-import confirmation flow, and low-discrimination disclosure policy are not modified by this delta. Exact-head Python package CI passed (run 39726897661). This is a static exact-head review of the changed behavior; tests were not rerun locally against this PR's head.
Security ACK for 377f2bc; no additional focused security review is needed for #155 at this head. Any later head change needs fresh assessment.
|
@codex review |
|
Codex Review: Didn't find any major issues. What shall we delve into next? 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". |
BenWestgate
left a comment
BenWestgate
left a comment
There was a problem hiding this comment.
Looks good except for this. We can't correct or validate hex besides that it is hex and the proper length and character set. It's okay to tell them if they entered a non-hex character. But yikes, people should not be typing the hex in they should be pasting it or better yet redirecting it in to avoid mistakes, or we should drop the hex feature. Your call.
Accept only the BIP93 seed sizes before any correction search. Reject other hexadecimal lengths with a clear prompt and reset the field instead of suggesting edits. Keep correction diagnostics specific to codex32 strings. Validation: 244 CLI tests; 20 focused tests under optimized Python; Ruff and diff checks pass. refs #155
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
@codex review |
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. 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: when an entry in
secret,share,wallet,xprvorcreate --existingdidn't parse and no unique correction was found, the rejection named the problem. For example: "The character 'b' is not allowed (position 48)", "After the prefilled MS1, enter a threshold…", or "The string is 9 characters short of…".After: those rejections say only "No unique correction found. Check your backup and re-enter it carefully.", as
correctalready does.A hint invites a guess. A wrong guess adds an error, and the next search may then find a wrong correction that leads to a wallet nobody can recover. Re-reading the backup is the only safe fix. Reasons about which valid card was entered (wrong application, duplicate share) stay.
checkkeeps its full diagnosis, since it never offers a correction. This follows the discussion that closed #101.How: the interactive and redirected entry paths, and
create --existing, swap the parse error for one message when the correction search finds no unique repair.checknow uses the standard parse messages ("The threshold must be 0 or a number from 2 through 9…", "This input has 39 characters…"), and the prefill-specific wording that repeated them is gone. Library: 4,894 to 4,859 logical lines.Checks: 909 tests pass, plus
python -O -m pytest, mypy, ruff, verify_correction_constants and differential_correction. Security review: no findings (review 5609013587 on 09c2587; the squashed head 377f2bc adds only the deletion of the repeatedcheckwording).🤖 Generated with Claude Code
https://claude.ai/code/session_01Q1TNm1YeYW8K9W2eBvxmCD