Repository navigation
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 |
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.
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.
3a35463 to
0157a97
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
b2aafde to
9769a5f
Compare
This comment has been minimized.
This comment has been minimized.
|
@codex review Current head was mechanically restacked onto the refreshed audit stack after #57's documentation-only follow-up. Please review this exact head for correctness/regressions; the composed stack passes 941 tests normally and under |
This comment has been minimized.
This comment has been minimized.
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-assisted current-head review, posted at the maintainer's request.
ACK 9769a5f for code/security review. Existing-seed identity is still checked before any new card output; the prior interruption and raw-seed identifier findings remain fixed, and no-record behavior is unchanged. The fresh Python-package run is queued. No findings.
|
@codex review Please review current head |
This comment has been minimized.
This comment has been minimized.
0157a97 to
096b654
Compare
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
9769a5f to
ada987b
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: ACK ada987b. ms32 create --existing performs the recorded-fingerprint/no-record decision before any replacement card is generated or shown, carries the checked fingerprint into wallet initialization without a second prompt, and preserves the supplied seed across re-sharing. I also rechecked the deleted older piped-input guard: _create() now has an earlier unconditional interactive-terminal requirement, so piped seed input still fails before generation and the removed check is redundant. Exact-head Python-package CI is green and all review threads are resolved. No correctness or release blocker found.
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
ada987bis the same five reviewed #81 patches mechanically replayed directly onto current #80 (096b654). Every stable patch ID is unchanged; no behavior was rewritten during the restack. Existing inline findings remain resolved.Verification
Exact-head GitHub Python-package CI is green. The composed current stack through #95 passes all 941 tests normally and all 941 under
python -O, Ruff, strict mypy, correction-constant verification, andgit diff --check. The focused early-record, mismatch, interruption, and no-record regressions are included.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.