wallet: Check existing seed before sharing - #81
BenWestgate wants to merge 5 commits into
Conversation
BenWestgate
added
bug
![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: 1af1e2c951
ℹ️ 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".
BenWestgate
added
area: security
ChatGPT Codex Connector
|
Current-head agent review ACK for |
1899011 to
0793590
Compare
ff0f3b2 to
7d5ebfd
Compare
Reassign the expected_fingerprint argument instead of copying it into a local, and give existing_secret its None default before the source checks instead of in an else branch. Behavior is unchanged. The installed package drops from 5161 to 5159 logical review lines, which keeps the integrated #7/#42/#57/#46/#80/#81 tip under the <5200 budget. Security: the record gate still runs before any card is generated or shown, and interrupts at that gate still raise _WalletSetupInterrupted. Validation: ruff check, ruff format --check, mypy src/codex32, and pytest (918 passed, with and without -O). Refs #81, #38. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018az69UX4773mYohXAtE8kD
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7d5ebfd518
ℹ️ 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".
|
AI-assisted release-gate recheck at current head |
7686cb0 to
aa2d333
Compare
a29753e to
23a2c05
Compare
Reassign the expected_fingerprint argument instead of copying it into a local, and give existing_secret its None default before the source checks instead of in an else branch. Behavior is unchanged. The installed package drops from 5161 to 5159 logical review lines, which keeps the integrated #7/#42/#57/#46/#80/#81 tip under the <5200 budget. Security: the record gate still runs before any card is generated or shown, and interrupts at that gate still raise _WalletSetupInterrupted. Validation: ruff check, ruff format --check, mypy src/codex32, and pytest (918 passed, with and without -O). Refs #81, #38. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018az69UX4773mYohXAtE8kD
aa2d333 to
9f88b21
Compare
BenWestgate
left a comment
BenWestgate
left a comment
There was a problem hiding this comment.
Exact-head release-gate re-review: ACK 9f88b21. ms32 create --existing now makes the wallet-record/no-record decision immediately after the supplied master seed is parsed, before any replacement card is generated or displayed; a mismatch/interruption cannot begin the sharing ceremony. The explicit no-record fallback remains, and raw-seed re-sharing reuses the identifier shown at that safety screen. The two later follow-ups are also sound: identifier reuse fixes the safety-screen/output mismatch, and the removed stdin guard is unreachable after the command preflight. The combined stack passes 935 tests normally and under python -O; focused create-existing/identity tests pass and the final tip remains 5,193 <5200. No code blocker found. Agent/Claude-authored commits still require responsible-human rewrite/squash before integration.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
23a2c05 to
a0ab769
Compare
Reassign the expected_fingerprint argument instead of copying it into a local, and give existing_secret its None default before the source checks instead of in an else branch. Behavior is unchanged. The installed package drops from 5161 to 5159 logical review lines, which keeps the integrated #7/#42/#57/#46/#80/#81 tip under the <5200 budget. Security: the record gate still runs before any card is generated or shown, and interrupts at that gate still raise _WalletSetupInterrupted. Validation: ruff check, ruff format --check, mypy src/codex32, and pytest (918 passed, with and without -O). Refs #81, #38. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018az69UX4773mYohXAtE8kD
9f88b21 to
1d5b6f5
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.
Codex current-head release-gate re-review: ACK 1d5b6f5.
ms32 create --existing now makes the wallet-record/no-record decision before any replacement card is generated or displayed. The two prior correctness findings remain fixed: Ctrl-C/EOF at that early gate preserves the existing backup, and raw-hex re-sharing reuses the identifier shown at the safety screen for the resulting share set. Both inline threads are resolved, and exact-head Python-package run 664 succeeded.
No code blocker found. The agent/Claude-authored commits still require responsible-human rewrite/squash before integration.
An existing hex seed or codex32 master secret previously reached the wallet-record fingerprint check only after new recovery cards had been generated and confirmed. Check the typed record immediately after parsing the source, before any card output or ceremony. Preserve the explicit recordless path at the same early decision point, and pass the checked result through to wallet initialization so it is not prompted twice. Cover matching, mismatching, and recordless flows for both source encodings. Refs #30.
Translate Ctrl-C or EOF at the early wallet-record gate for ms32 create --existing into the existing wallet-setup interruption path. This keeps an operator from being told to invalidate a pre-existing recovery card before any new share ceremony has started. Add a focused regression proving the interruption occurs before share creation or output and preserves the valid-backup message.
Reassign the expected_fingerprint argument instead of copying it into a local, and give existing_secret its None default before the source checks instead of in an else branch. Behavior is unchanged. The installed package drops from 5161 to 5159 logical review lines, which keeps the integrated #7/#42/#57/#46/#80/#81 tip under the <5200 budget. Security: the record gate still runs before any card is generated or shown, and interrupts at that gate still raise _WalletSetupInterrupted. Validation: ruff check, ruff format --check, mypy src/codex32, and pytest (918 passed, with and without -O). Refs #81, #38. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018az69UX4773mYohXAtE8kD
A raw seed imported with create --existing was assigned a temporary random identifier for the no-record safety screen, then assigned a different random identifier when the new share set was created. Reuse the first identifier as the share-set identifier so the safety screen describes the backup that will actually be produced.\n\nExtend the recordless-creation regression to require the displayed, emitted, and imported identifiers to agree.\n\nRefs #30
1d5b6f5 to
b2aafde
Compare
a0ab769 to
3a35463
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. 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.
AI-assisted current-head review: ACK b2aafde. Existing-seed identity is checked before any new card output; mismatch, interruption, no-record, and raw-seed identifier paths are covered. Exact-head package CI is green.
This branch has not been deployed
2 participants
Refs #30. Focused follow-up to #57 after #105, #99 and #80.
What and why
ms32 create --existingchecks a supplied hex seed or codex32 master secret against the fingerprint on the separate wallet record before generating or showing a new recovery card. A mismatch can be corrected before a share ceremony begins. The explicit no-record path presents the recovered fingerprint and backup identifier for visual confirmation at the same early point. Wallet initialization receives the already checked result and does not prompt twice.Ctrl-C or EOF during this early check reports that the existing backup remains valid. For a raw hex seed, the identifier shown during the no-record decision is the identifier used for the new shares and imported secret. Fresh
ms32 createstill asks the operator to record its newly generated fingerprint.Review shape
Current head
b2aafdeis the same five focused #81 commits mechanically replayed directly onto refreshed #80 (3a35463), which sits on #99. The existing inline findings remain resolved.Verification
All 303 CLI/Core tests pass normally and under
python -Oon the refreshed stack, including the early-record, mismatch, interruption, and no-record regressions. The exact-head GitHub matrices are green,git diff --checkis clean, and Codex found no major issue on reviewed commitb2aafdeb40. The composed refreshed stack through #95 also passes all 941 tests normally and all 941 underpython -O, plus Ruff, format, strict mypy, and correction-constant verification.Human integration order is #57 → #105 → #99 → #80 → #81 → #95. The agent/Claude-authored commits need responsible-human review/rewrite or squash under repository policy before integration.