Repository navigation
wallet: Check existing seed before sharing - #81
BenWestgate wants to merge 7 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
security
ChatGPT Codex Connector
|
Current-head agent review ACK for |
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 |
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.
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
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.
ada987b to
fad6b69
Compare
When RIPEMD-160 is unavailable, a valid standard Bails identifier cannot be checked. Preserve the Bails-alpha SHA-256 result and distinguish that inconclusive state from a completed identifier mismatch, so the no-record restore prompt does not claim the cards are wrong. Keep the independent fingerprint and explicit operator-confirmation boundary unchanged. Refs #79
The no-record restore flow can no longer claim a standard Bails identifier mismatch when RIPEMD-160 is unavailable. Record that platform-dependent inconclusive outcome in both the security model and invariant so reviewers can distinguish it from a completed comparison. Refs #79
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
fad6b69 to
06ac7e1
Compare
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
@codex review |
|
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. 👍 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". |
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.