correct: Say why the entry was invalid when suggesting a repair - #101
BenWestgate wants to merge 3 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
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
054e8d9 to
115f2c2
Compare
`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
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
1 similar comment
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
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
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
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.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
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
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.