Repository navigation
correct: Say why the entry was invalid when suggesting a repair - #101
BenWestgate wants to merge 6 commits into
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b4d15aeeb6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
a7efaae to
054e8d9
Compare
This comment has been minimized.
This comment has been minimized.
Gate restore and existing-seed wallet initialization on the independently recorded BIP32 master fingerprint before any Bitcoin Core wallet mutation. Keep the correction path from disclosing or reusing a fingerprint derived from the candidate being authenticated. Fixes #30.
054e8d9 to
115f2c2
Compare
Every correction plan returned its target set, that same set as primary, an empty reduced set, and a true timed flag. Only the targets and primary set were consumed. Derive primary from targets at the call site and remove the other fields. The search engine also accepted reduced without reading it, so remove that argument and update its test and benchmark callers. Search order and capture accounting remain unchanged. Refs #46.
The preceding all-isinstance check rejects every non-share, so the list-comprehension predicate in recovery could never discard an item. Pass the validated list directly, using a type cast to express the established invariant to mypy. Recovery still copies and validates the sequence internally. Refs #46.
`correct` and the "Possible correction" prompt in secret, share, wallet and create --existing showed only the repair. Print one line to stderr first, "Invalid: <reason>", using the reason `check` already gives. Callers pass the parse error they already caught, so nothing is parsed twice. A mixed-case string now says codex32 strings are all uppercase or all lowercase and that either case recovers the same wallet. Closes #85 Claude-Session: https://claude.ai/code/session_015CuLXqAvAovfoVcUmmogwa
The mixed-case message said either case "recovers the same wallet", but `_parse` shows it for every profile, including shares and application prefixes with no wallet. Say that both cases decode to the same data. Claude-Session: https://claude.ai/code/session_015CuLXqAvAovfoVcUmmogwa
e852899 to
dc4bf7b
Compare
This comment has been minimized.
This comment has been minimized.
1 similar comment
This comment has been minimized.
This comment has been minimized.
BenWestgate
left a comment
BenWestgate
left a comment
There was a problem hiding this comment.
AI-generated review (Codex), posted at the maintainer's request.
Not ACKing 96ac5aa yet: this raises the installed-code budget from 5,200 to 5,250 without explicit authorization. The invalid-reason UX change otherwise looks sound.
BenWestgate
added
area: cli
96ac5aa to
dc4bf7b
Compare
This comment has been minimized.
This comment has been minimized.
BenWestgate
left a comment
BenWestgate
left a comment
There was a problem hiding this comment.
AI-generated current-head follow-up review (Codex), posted at the maintainer's request.
ACK dc4bf7bbc223 now that the unrelated <5250 cap commit has been removed.
The invalid-input reason is taken from the parse error the caller already caught, so the change does not add a second parsing/validation path. The message remains behind the existing low-discrimination disclosure gate, and the follow-up wording is profile-neutral (“same data”) rather than incorrectly assuming a wallet. The tested malformed-header/data/length and mixed-case cases cover the intended user-facing boundary.
No correctness blocker found in the focused behavior diff. The PR is stacked on #105 and preserves <5200; run final CI on the actual integrated #105 + #101 tip before freeze. Responsible-human authorship/review policy still applies.
This comment has been minimized.
This comment has been minimized.
BenWestgate
left a comment
BenWestgate
left a comment
There was a problem hiding this comment.
Codex current-head review at 65a886a: no findings. Since the prior ACK, the only new commit brings in #105’s already exact-head-reviewed cleanup; the invalid-reason behavior and disclosure-gate placement are unchanged.
BenWestgate
left a comment
BenWestgate
left a comment
There was a problem hiding this comment.
AI-generated current-head stack review (Codex), posted at the maintainer's request.
Code ACK 65a886a735b8 for the actual #105 + #101 head. #105 is now an ancestor of this branch; comparing #105 (361feb7) to the head yields only the four reviewed invalid-reason behavior/test files. The unrelated <5250 cap change is absent.
The reason text still comes from the already-caught parse error, remains behind the existing low-discrimination disclosure gate, and uses profile-neutral mixed-case wording. The known #104 call-shape overlap remains documented if that post-v1 enhancement is later integrated.
Fresh current-head Python-package run #755 is still queued at the time of this review. Treat this as an exact-head code ACK with CI pending. Responsible-human authorship/review still applies before integration.
|
V1 disposition (Codex, 2026-10-04): defer this post-audit UX enhancement from the v1 candidate. The invalid-reason behavior has a current-head Codex code ACK and fits under the library budget on #105. It is not required to close the original mixed-case audit finding, which is already fixed by #42. A temporary attempt to combine #100 and #101 exposed a real late-stack overlap conflict, so carrying this into v1 would enlarge the frozen-candidate review scope without closing an audit finding. Keep #101 open for post-v1, where it can be rebased with #100/#104 as appropriate. Human authorship/review remains required when integrated. |
BenWestgate
removed
the
gate: adversarial review
03b0f8a to
9ea48de
Compare
This branch has not been deployed
2 participants
Requested by Ben · project thread
Problem
correct, and the embedded “Possible correction” prompt in recovery commands, showed a repaired string without first explaining why the entered text was invalid.Change
Print one
Invalid: <reason>line using the parse error already available to the caller. Mixed-case input uses the profile-neutral wording that codex32 strings are all uppercase or all lowercase and both cases decode to the same data. The reason is printed only after the low-discriminationYESgate, so noninteractive gate failure still emits only its operational error.The tested cases cover mixed case, invalid threshold/index symbols, invalid data characters, and an extra character.
create --existingkeeps using the library’s own rejection reason.Review stack
Base: #105 (
codex/remove-unreachable-v1-branches) at361feb7.The unrelated
<5250cap commit was removed and its old tip is preserved onarchive/101-pre-restack-20261004. #105 was merged into this feature branch through stack-maintenance PR #122, so current head65a886ais an actual descendant of #105. Comparing #105 to the head shows only the four reviewed behavior/test files.The combined tree is 5,188 installed production logical lines under
<5200. Exact-head Python-package run 755 succeeded on this combined head. #104 overlaps the_confirm_correctioncall shape and must preserve both arguments if #104 is later accepted.Validation
The behavior commit
dc4bf7bhas a Codex ACK. Prior validation passed 931 tests plus Ruff, strict mypy and focused optimized tests; exact-head Python-package run 755 is green. The review finding that the original mixed-case wording incorrectly said “same wallet” for non-wallet profiles is fixed.Closes #85.
AI-assisted stack maintenance and review. Responsible-human review/authorship policy still applies before integration.