Repository navigation
Fix the five confirmed #142 review findings - #210
Draft
BenWestgate wants to merge 5 commits into
Draft
BenWestgate wants to merge 5 commits into
BenWestgate wants to merge 5 commits into
Conversation
Bech32 separates the HRP at the last "1". Correction only checked that
the input started with the context's "<hrp>1", so an entered "1" later
in the body was treated as a mutable explicit error and corrected away.
For example, an ms string with a "1" typed at position 15 actually
carries a different HRP, yet correct(CorrectionContext("ms", 48), ...)
returned the original ms artifact.
Reject the input during normalization unless its last "1" sits
immediately after the context HRP. This applies to every structural
and fixed search, public and CLI, since all of them normalize here.
Security: invariant 5 requires a syntactically present HRP and
separator to remain immutable correction context. Correction can no
longer move text from one namespace into another by repairing its
separator.
Validation: new regression test fails before and passes after;
ruff, mypy, and the correction test modules pass.
Claude-Session: https://claude.ai/code/session_01MDsbGqZmvVThoJrs4vzc4K
Mixed-case input is searched twice: once with minority-case symbols erased and once with the case normalized. Each _search_many() call built its own frontier, so each interpretation started a fresh admission budget and the combined admitted capture mass could exceed the mandatory bound of 1. For a 48-character ms string with 13 minority-case symbols, the erased reading admitted a whole-checksum completion while the normalized reading admitted its own layers; the returned candidate reported a cumulative mass of about 1.318. _frontier() now accepts the other interpretation's states. Their layers take part in the volume-ordered admission but are not returned, so the union of both calls is exactly one greedy admission over every layer, independent of search order. The required-only discovery passes compute that same full frontier and then restrict it, so a seeded candidate can only come from a layer the full search also admits. The public API and the CLI scheduler both pass the other reading. Security: restores invariant 5's single shared mass bound for mixed-case correction. Inputs whose explanation no longer fits the bound now yield no suggestion instead of an over-budget one. Validation: new public and CLI regression tests fail before and pass after; ruff, mypy, full pytest (914 passed), verify_correction_constants.py and differential_correction.py --verify pass. Claude-Session: https://claude.ai/code/session_01MDsbGqZmvVThoJrs4vzc4K
For mixed-case CLI input, the scheduler runs fixed and required work in discovery passes and then a full pass with optional_only=True. That pass skipped fixed BCH entirely, leaving the per-target fixed-witness map empty. _covered_candidates() reads a missing witness as a failed fixed decode and returns "no possible survivor", so optional layers that could give a known candidate a lower capture volume were pruned. Example: bytes(range(16)) encoded with identifier "test", with positions 11/12 and 23/34 swapped and position 40 capitalized, was reported with capture volume 2,663,936, while the identical lowercase input finds the adjacent-plus-distant swap at 83,248. Recompute each fixed witness in the optional pass as well. It is a single BCH decode per target; results are deduplicated against the seeded candidates, and admission is unchanged. Security: pruning may only drop work that cannot improve rank. A wrong proof could rank a worse explanation first or misstate the reported capture volume shown before the low-discrimination gate. Validation: new regression test (expected volume from independent arithmetic, 2 * 44 * C(44, 2)) fails before and passes after; ruff, mypy, competitor, mixed-case and disclosure tests pass. Claude-Session: https://claude.ai/code/session_01MDsbGqZmvVThoJrs4vzc4K
For an opaque HRP the CLI tries every length within -8..+8 of the entered text. Near the short/long checksum boundary some of those targets expand to the forbidden lengths 94 and 95, and _prepare() raised InvalidLength from _checksum_for_encoded_length() before any BCH work ran. A valid 90-character "zz" string with a single substituted checksum symbol therefore failed with "expanded codex32 lengths 94 and 95 are invalid" instead of being corrected. Filter planned targets through the same context validation the public unknown-length path already applies, so unsupported lengths are never searched or charged to the admission ledger. Security: none beyond availability; the remaining targets, ledger and confirmation boundaries are unchanged. Validation: new regression test fails before and passes after; ruff, mypy, CLI, CLI length and generic-HRP tests pass. Claude-Session: https://claude.ai/code/session_01MDsbGqZmvVThoJrs4vzc4K
The build job held contents: write and actions/checkout persisted that token in .git/config while "python -m build --no-isolation" executed repository code (the build backend and project hooks). Anything running during the build could therefore read a token able to modify releases and repository contents. Check out without persisting credentials, drop the build job to contents: read, and move "gh release upload" into a separate attach job. That job runs no repository code: it only downloads the built distributions and receives the token through GH_TOKEN. PyPI publishing still waits for the release attachment, as it did when both steps shared a job; gh is pointed at the repository explicitly because the attach job has no checkout. Security: limits the write-scoped token to a job that executes no project code, closing a path from a compromised build dependency or hook to release tampering. Validation: workflow YAML parses and the job graph and permissions are as intended (build: read, attach: write, publish: read + id-token). Not run on GitHub. Claude-Session: https://claude.ai/code/session_01MDsbGqZmvVThoJrs4vzc4K
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Before: in #142's review at 5fc90bc, Ben confirmed five findings that are still present in reviewability-v1:
1, even though Bech32 uses the last1as the separator.contents: writetoken.After: each finding has its own commit and a regression test that fails before the fix and passes after it. The CI change is the exception and has no test.
How:
49761a7indel._normalizerejects input unless its last1comes right after the context HRP. One side effect: a1typed in place oflin the data part is now rejected rather than corrected.950606e_frontiertakes the other case reading assharedlayers. They spend the same budget but are not returned. Required-only discovery now narrows the joint frontier. The 13×Prepro from the review now returns no candidate, which is fail-closed, but the public API still runs until its 10 s deadline before it gives up.43e74a0_search_competitorsrecomputes the fixed witness on every pass.23e613e_correction_candidatesskips target lengths that_validate_contextrejects, the same as the public unknown-length path.0ba725bThe build job is nowcontents: readwithpersist-credentials: false. A newattachjob runs no repository code and is the only job that can write to the release.publishneeds[build, attach].Checks: ruff, ruff format, mypy and
python -m pytest(916 passed, up from 911) all pass.python -O -m pytest,verify_correction_constants.pyanddifferential_correction.py --verifypass too. The publish workflow change hasn't been run on GitHub.This touches correction, so it needs a security review before it goes ready. #142 will be rebuilt on top of it.
Generated by Claude Code