Remove pre-review CLI dead code - #46
Conversation
|
Agent handoff: before declaring the pre-review cleanup complete, read the ignored local checklist at |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
BenWestgate
left a comment
There was a problem hiding this comment.
AI-generated review (Claude), posted at the maintainer's request.
Concept ACK. Code looks right; not ACKing 7e7c649 until the merge commits are gone.
- Removed branches are unreachable:
_createalready requires a TTY on stdin/stdout, and_connected_corenever returnsNone.ambiguouswas alwaysFalse. - Rebase away
825b863/ccabfee. - The PR body points to
docs/planning/v1-pre-review-cleanup.md, which is local and ignored, so reviewers can't read it. Inline the non-goals or drop the pointer. - Conflicts with #53 and #57.
|
Review follow-up: the PR body no longer points reviewers at the ignored local planning file; the non-goals are now inline. The remaining NACK item is the branch history itself: rebase away the two base-refresh merge commits before human review. I’m leaving #46 behind #45 in the review order until that rewrite is done. |
5a826bd to
eb7cfdd
Compare
7e7c649 to
28b06c6
Compare
|
Release-gate follow-up: the earlier history NACK is resolved. Current head |
d1ecc82 to
c943e9a
Compare
|
Release-gate refresh note: final #45 is now |
|
Final-refresh addendum: disposable replay of this single cleanup commit on the current combined #12/#13/#59/#7/#51/#33/#42/#45/#57 stack resolves only the expected Do not raise the budget in this PR. The post-prerequisite cleanup refresh must either recover at least 99 real review lines while preserving #42/#57 behavior or compose with a separate focused cleanup that does, then rerun the exact combined-tree size test. |
19a59dd to
3d5e60d
Compare
bfd8b1b to
794f898
Compare
28b06c6 to
9a3b845
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
|
AI-assisted release-gate recheck at exact head |
Remove unreachable creation guards and the permanently false correction ambiguity field, align the CLI test Core stub with production, and move reference-only correction helpers out of the installed package.\n\nSecurity: fail-closed correction and wallet behavior are unchanged.\n\nRefs #38.
0793590 to
0867a1b
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
There was a problem hiding this comment.
Release-gate ACK 0867a1b.
I re-reviewed the refreshed one-commit cleanup on current #42. The earlier nACK was mechanical/staleness only; current head removes the unreachable _create guards, permanently-false ambiguity field, drifted Core test-double state, and reference-only installed helpers without changing correction policy or wallet behavior. The branch is now one Ben Westgate-authored commit with no unresolved inline threads. Exact-head local validation passed 898 tests normally and under python -O, all 232 CLI tests, strict mypy, Ruff check/format, correction-constant verification, all 57 frozen differential cases, and git diff --check.
No code blocker remains from this review. The dependency-aware order is #42 → #46 → #57; placing this mechanical cleanup before #57 keeps the installed-source budget below the authorized <5200 cap at every intermediate tip.
|
Agent exact-head review on
No code blocker found. One non-blocking commit-message nit remains: the commit body contains literal |
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
Every correction plan returned its target set, that same set as primary, an empty reduced set, and a true timed flag. Only the targets and primary set were consumed. Derive primary from targets at the call site and remove the other fields. The search engine also accepted reduced without reading it, so remove that argument and update its test and benchmark callers. Search order and capture accounting remain unchanged. Refs #46.
The preceding all-isinstance check rejects every non-share, so the list-comprehension predicate in recovery could never discard an item. Pass the validated list directly, using a type cast to express the established invariant to mypy. Recovery still copies and validates the sequence internally. Refs #46.
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
Every correction plan returned its target set, that same set as primary, an empty reduced set, and a true timed flag. Only the targets and primary set were consumed. Derive primary from targets at the call site and remove the other fields. The search engine also accepted reduced without reading it, so remove that argument and update its test and benchmark callers. Search order and capture accounting remain unchanged. Refs #46.
The preceding all-isinstance check rejects every non-share, so the list-comprehension predicate in recovery could never discard an item. Pass the validated list directly, using a type cast to express the established invariant to mypy. Recovery still copies and validates the sequence internally. Refs #46.
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
Remove unreachable creation guards and the permanently false correction ambiguity field, align the CLI test Core stub with production, and move reference-only correction helpers out of the installed package. Security: fail-closed correction and wallet behavior are unchanged. Refs #38.
Every correction plan returned its target set, that same set as primary, an empty reduced set, and a true timed flag. Only the targets and primary set were consumed. Derive primary from targets at the call site and remove the other fields. The search engine also accepted reduced without reading it, so remove that argument and update its test and benchmark callers. Search order and capture accounting remain unchanged. Refs #46.
The preceding all-isinstance check rejects every non-share, so the list-comprehension predicate in recovery could never discard an item. Pass the validated list directly, using a type cast to express the established invariant to mypy. Recovery still copies and validates the sequence internally. Refs #46.
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
Remove the mechanical cleanup items recorded by the adversarial-review triage and reduce installed review surface without changing user-visible behavior:
_createterminal guard;_correction_candidates()contract;initialize(private=...)state that production does not expose;src/codex32/indel.pytotools/correction_reference.py;pyproject.toml.Why this now precedes #57
The current Core-native
reviewability-v1base increased installed source enough that the focused #57 restore-authentication change reaches 5,228 production logical lines by itself, exceeding the maintainer-authorized<5200gate. The already-reviewed mechanical cleanup is independent of that security behavior and reduces the #42 tip from 5,124 to 5,030 lines. Applying it first lets #57 remain focused and keeps every intermediate commit inside the existing budget without raising the cap or taking a risky refactor merely to save lines.Current head
0867a1bis one focused Ben Westgate-authored cleanup commit stacked directly on #42 (4be79a7). Conflict resolution deliberately preserved #42's current wallet behavior; #57-specific record/fingerprint behavior is no longer bundled into this cleanup.Validation
<5200;python -O;git diff --checkis clean.Human review order is now #42 → #46 → refreshed #57. #80 and #81 follow #57. This ordering is mechanical: it exists to keep the authorized review-size invariant true at every step.
Intentional non-goals: no correction-policy change, wallet-identity change, helper/API publication, or fuzzing-policy change. Those are owned by #42/#45, #57/#81, #53, and #40/#47 respectively.
AI assistance was used for the user-authorized restack/conflict validation; the squash/restack does not substitute for responsible maintainer review.