api: Expose reference-vector helpers - #53
BenWestgate wants to merge 2 commits into
Conversation
|
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.
ACK 7311db8
- Pure renames plus
checksum_for_body_lengthfactored out of_from_partswith the same logic. I ran the api.md example; it parses as aSecret.__all__is 24. - These names become a supported v1 surface. That's the point of #49, but it's a commitment worth accepting on purpose.
- Sequencing: it conflicts with #42 and #57 (mechanical renames only).
- Nit: the new docstrings have a blank line after them, unlike the neighbouring functions.
|
Merge-order note from #42 review: #42 introduces |
|
Review follow-up: the blank-line docstring nit is valid but non-functional. I’m leaving the current one-commit API diff intact until #42 and #57 land because #53 already has mechanical rename conflicts with both; remove the extra blank lines in that single conflict-refresh commit rather than creating another pre-conflict churn commit. Human review order: #42 and #57 before #53. |
|
Second merge-order/API-boundary note from #13 review: after #13 lands, deduplicate the two ASCII-only case-fold implementations by adding one private |
|
Review-submission follow-up: ACK stands. The blank-line-after-docstring nit is style-only; handle it when this branch is refreshed after #13/#42 so the conflict resolution stays one mechanical API-boundary pass. That same refresh should (1) centralize private |
|
Release-gate sequencing note: keep this after #13, #42, and #57 because the current conflicts are mechanical renames. One concrete helper-boundary cleanup should be folded into this pass: #13 currently has equivalent ASCII-only lowercase helpers in |
|
Agent API-boundary review at current head |
Promote the module-level checksum and u5 conversion interfaces needed by reference-vector authors while keeping the package-level API narrow. Document and test the supported vector workflow and justify the remaining private cross-module couplings.\n\nValidation: 866 normal and 866 optimized tests; mypy; Ruff; production size budget.\n\nfixes #49
The newly supported chars_to_u5 interface must not case-fold Unicode into valid Bech32 symbols. Lower only ASCII characters so lookalikes such as the Kelvin sign remain invalid, matching the normalization boundary enforced elsewhere.
316ea11 to
cde312b
Compare
BenWestgate
left a comment
There was a problem hiding this comment.
Codex current-head re-review: code ACK cde312b.
The refreshed diff preserves the narrow package-level surface while publishing the module-level primitives needed for external reference-vector construction. checksum_for_body_length() and checksum_for_encoded_length() preserve the regular/long threshold, the expanded-length 94/95 gap, and the 1023-symbol ceiling. The renamed u5 helpers preserve range checks, and chars_to_u5() uses ASCII-only lowercase so Unicode lookalikes such as Kelvin sign remain invalid. The shared private _ascii_lower centralization also preserves the earlier confirmation behavior. Focused tests exercise vector construction entirely through the new supported names and retain the checksum/interpolation invariants. No inline review threads remain.
I found no code blocker. GitHub currently reports no workflow run/status attached to this exact head, so this is a code-review ACK only; the PR body's exact-head local validation (952 normal/optimized, 57/57 differential, Ruff/mypy, 5,199 < 5,200) is not being represented here as GitHub CI evidence. The Codex-authored second commit must still be rewritten/squashed by the responsible human before integration.
BenWestgate
left a comment
There was a problem hiding this comment.
Codex verification follow-up for current head cde312b6ab5c56b90426bdcdb1aa721199c46a87: the earlier review's statement that no exact-head workflow run was attached was an API-wrapper blind spot. GitHub Actions run 36985866683 is attached to this commit and all 10 Python-package matrix jobs completed successfully. The Ubuntu 3.13 job also passed the optimized suite, correction-constant verifier, strict mypy, Ruff check/format, frozen differential verifier, build, and wheel-environment verification. The prior current-head code ACK therefore now has exact-head GitHub CI evidence as well. The separate dynamic github-advanced-security agent job failure is not the package workflow; its Processing Request step failed while ordinary CodeQL analysis succeeded. Human authorship/rewrite for the Codex-authored follow-up remains.
Closes #49.
Promote the module-level checksum specifications, u5 conversion helpers, and checksum-selection helpers required to construct and verify codex32 reference vectors without underscore-prefixed imports. These are supported at their owning modules; this PR does not add them to package-level
codex32.__all__.docs/developer/api.mddocuments the supported vector workflow and the remaining intentionally private cross-module couplings. Internal benchmarks may continue to use implementation-private names when they are explicitly testing internals.The supported
chars_to_u5()API lowercases ASCII only. Unicode lookalikes such as Kelvin signKstay invalid instead of being case-folded into Bech32k. The ASCII-only helper now lives once inbech32.pyand is shared by the CLI input path.Review stack
Base: #52 (
codex/5-release-qualification). Current head:cde312b.This is the planned one-time refresh after the overlapping restore/foundation/security/release stack settled. Mechanical conflict resolution preserves the reviewed mixed-case correction state, the Core-native process boundary, the 83-character BIP173 HRP rule, and the supported module-level vector API. Package-level
codex32.__all__remains 23 names.The diff remains two logical review units:
d6679cc— the human-authored vector-API publication, replayed on the settled base;cde312b— the Unicode-lookalike follow-up plus the mechanical ASCII-lowercase centralization required by the settled CLI/Core code.The second commit remains Codex-authored and must be rewritten/squashed under the responsible human author before merge, per the repository authorship policy. The PR therefore remains draft until that human step is performed.
Validation
On current head
cde312b:python -O: 952 passed;python -O;__all__: 23 names;git diff --check: clean.Exact-head GitHub Python-package run
36985866683completed successfully oncde312b; all 10 matrix jobs passed. The Ubuntu 3.13 leg also passed the optimized suite, correction-constant verifier, strict mypy, Ruff check/format, frozen differential verifier, build, and wheel-environment verification. A separate dynamicgithub-advanced-securityagent job failed in its Processing Request step while ordinary CodeQL analysis succeeded; that is not a package-test failure.Current-head Codex review ACKs the code, and there are no unresolved inline review threads. No automated code-review or verification gap remains. Human authorship/rewrite and integration remain.
Disclosure: AI tools assisted with the mechanical restack and conflict analysis. Human responsibility is required for final authorship and integration.