cli: Name the real Bitcoin Core requirement when it is missing - #99
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: 3b738eb71e
ℹ️ 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
Without bitcoin-cli every ms32 command said "Install a reviewed bitcoin-cli before creating a backup", even commands that create nothing, and the codex32 hint didn't say what it leaves out. Say that Bitcoin Core 32 or newer must run with RPC enabled and that an unsynced regtest or signet node is enough for practice. Then say what the command uses Core for: the master fingerprint and correction ranking for secret, share and correct (with the codex32 fallback and what it omits), or giving Core the master key for create and wallet. ms32 correct now connects before its search instead of after it, so a missing Core no longer costs up to ten seconds of discarded work. Closes #84 Claude-Session: https://claude.ai/code/session_015CuLXqAvAovfoVcUmmogwa
Connecting before the search made two problems easier to hit. With damaged input piped to `ms32 correct` and two Core networks running, the network prompt read the exhausted pipe forever. With stderr redirected, "Using Bitcoin Core on ..." and a blank line came before `interactive confirmation required`, which the security model says must be the only message. Ask for a network only when stdin is a terminal; a pipe now gets "More than one local Bitcoin Core network is running." Print Core's messages only when stderr is a terminal. The gate test's fake now reports like the real one, so it catches the extra output. Claude-Session: https://claude.ai/code/session_015CuLXqAvAovfoVcUmmogwa
d422478 to
c583b24
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. |
|
@codex review |
|
CACK, will review the code after the bots say it is ready for me. |
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. 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-generated review (Codex), posted at the maintainer's request.
Not ACKing 2569980 yet: this raises the installed-code budget from 5,200 to 5,250 without the explicit authorization AGENTS.md requires. The Core error-handling change otherwise looks sound.
BenWestgate
added
area: cli
2569980 to
c583b24
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 c583b2479359 for the focused #84 behavior now that the unrelated <5250 cap commit has been removed.
The error paths name Bitcoin Core 32+/RPC clearly; Core-independent codex32 fallbacks are described only for secret/share/correct; wallet/create correctly have no fallback. ms32 correct now establishes the Core boundary before spending the correction-search deadline, while already-valid input still returns before connecting. The redirected-input change prevents stdin from being consumed by Core network selection and suppresses incidental selection chatter on redirected stderr.
The behavior is covered by the missing-Core/fallback, wallet no-fallback, multi-network pipe, and Core preflight regressions. No correctness blocker found in the five-file diff.
The PR is now stacked on #105 and preserves <5200; run the final combined-tip CI after human integration/restack before freezing the candidate. 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.
AI-generated current-head stack review (Codex), posted at the maintainer's request.
Code ACK 70c5da6267bf for the actual #105 + #99 head. #105 is now an ancestor of this branch; comparing #105 (361feb7) to the current head yields exactly the five previously reviewed #84 files: _bitcoin_core.py, cli.py, test_bitcoin_core.py, test_cli.py, and test_correction_disclosure.py. The unrelated <5250 cap change is absent.
The behavior review remains unchanged: Core 32+/RPC requirements and Core-independent fallbacks are correctly described, ms32 correct establishes the Core boundary before damaged-input search, and redirected input cannot answer Core network selection.
Fresh current-head Python-package run #749 is still queued at the time of this review; current-head Bitcoin Core fixture run #37 has succeeded. Treat this as an exact-head code ACK with Python CI pending, not a claim that the matrix is already green. Responsible-human authorship/review still applies before integration.
This branch has not been deployed
2 participants
Problem
Without
bitcoin-cli,ms32 secret,share,correct,walletandcreateused the generic message “Install a reviewed bitcoin-cli before creating a backup.” It did not say what Core was needed for, what thecodex32 <command>fallback omits, or thatms32 correctwould spend up to ten seconds searching before discovering Core was unavailable.Change
The Core failure now names Bitcoin Core 32+, RPC, and signet/regtest practice.
secret,share, andcorrectexplain that Core supplies the master fingerprint/correction ranking and point to the Core-independentcodex32command.createandwalletstate that they give Core the master key and therefore have no fallback.ms32 correctconnects before beginning a damaged master-seed search. Core network selection is kept off redirected stdin/stderr so a pipe cannot accidentally answer an interactive network prompt.Review stack
Base: #105 (
codex/remove-unreachable-v1-branches) at361feb7.The unrelated
<5250size-budget commit was removed and its old tip is preserved onarchive/99-pre-restack-20261004. #105 is an ancestor of current head70c5da6; comparing #105 to this head shows only the five reviewed #84 behavior/test files.The combined tree is 5,187 installed production logical lines under the existing
<5200gate.Validation
The focused behavior had a Codex ACK before stacking, and current head
70c5da6has a Codex stack ACK. Current-head Python-package run 756 and Bitcoin Core wallet-fixture run 38 both succeeded. Tests cover all three Core-independent fallback messages, the no-fallback wallet/setup path, earlycorrectCore connection, missingbitcoin-cli, no local Core 32+ RPC, and redirected input not answering network selection.No automated verification remains before human review. Human integration order is #57 → #105 → #99, then refresh #80 once onto this settled Core/CLI tip and carry #81/#95 forward.
Closes #84.
AI-assisted stack maintenance and review. Responsible-human review/authorship policy still applies before integration.