Three triage fixes: a diagnostic that could not print, a test that documented the wrong build, and five stale region margins - #224
Merged
Conversation
Four `set -eo pipefail` swallow sites in tools/integration/build_nistcurves_p256.sh, each one killing the shell inside the error path written for exactly that condition, so the caller gets a bare non-zero exit and no message. 1. `lib_preflight_fail`'s `pin=` line (the reported #217 defect) was the only one of three adjacent `git` calls without `|| true`. A PROJECT_ROOT that is not a git repository makes `git ls-tree` exit 128, pipefail carries it through `awk`, and the shell dies two lines before the `cat >&2` heredoc — in exactly the context (`tools/test_build_flags_stamp.py`'s tempdir "Farm" tree, which has no `.git`) where the #124 remedy message is most useful. RED, Farm-shaped tree (non-repo root, libs -> uninitialised submodule): $ bash <farm>/tools/integration/build_nistcurves_p256.sh onchip exit=128 <- no output whatsoever $ cd <farm> && git ls-tree HEAD libs/nistcurves fatal: not a git repository (or any of the parent directories): .git ls-tree rc=128 GREEN: the full #124 message, exit=1. 2. `ZP_MEMBER="$( ar65 t ... | grep '^zp_config' )"` — same shape, worse consequence: with no matching member `grep` exits 1, the assignment dies, and the `0)` arm that reports it never runs. A silently dropped zp_config member is the #124 runtime-corruption class (zp_ptr2 reverts to $fd and collides with zp_temp/zp_count, no link error). RED (fake ar65 filtering `^zp_config` out of the listing): $ AR65=<fake> bash tools/integration/build_nistcurves_p256.sh onchip [p256/onchip] building libs/nistcurves lib-p256-verify-onchip ... exit=1 <- the 0) arm never printed GREEN: ERROR: no zp_config member in nistcurves-p256-verify-onchip.a exit=1 3. `check_zp_slot`'s `got=$(od65 ... | awk ...)` — a failing od65 kills the script and the `${got:-<absent>}` empty case is unreachable. RED (OD65 -> a stub that exits 3): $ OD65=<stub> bash tools/integration/build_nistcurves_p256.sh onchip exit=3 <- no message GREEN: ERROR: zp_config_p256verify.o exports nistcurves_zp_ptr2 = <absent>, expected 0x0000003D (CONTRACT_ZP_DEFINES did not take) exit=1 4. With `pin` empty, the diagnosis branches then asserted a WRONG cause. A non-repo root whose libs/nistcurves is populated (a symlink into a real checkout, as the test farms use) leaves head non-empty and pin empty, so `[ "$head" != "$pin" ]` is trivially true and the message blamed a stale submodule and prescribed a `git submodule update` that cannot run there. An empty pin now gets its own branch, first, with its own remedy text. RED (stub repo at libs/nistcurves without src/zp_config.s, non-repo root): submodule checkout : 88a12b5 this repo pins : The submodule working tree is NOT the commit this repo pins. Fix it with: git submodule update --init --recursive GREEN: This repo's gitlink for libs/nistcurves could not be read, so there is nothing to compare the checkout against: either <root> is not a git checkout, or its HEAD carries no libs/nistcurves gitlink. Fix it wherever this tree's libs/nistcurves comes from, with: git submodule update --init --recursive Running that against THIS root will not help: there is no gitlink here to update, whatever the reason. The new branch is narrowed to what was actually tested. An earlier draft blamed a non-repo root outright; that over-claims, because a REAL checkout whose HEAD carries no libs/nistcurves gitlink reaches the same branch with `.git` present — constructed and confirmed (a git repo containing only tools/, with libs/nistcurves a stub checkout): this repo pins : ... either <root> is not a git checkout, or its HEAD carries no libs/nistcurves gitlink. And where BOTH conditions hold (the #217 Farm shape: non-repo root, empty submodule directory) the message now states both, so the original issue's case keeps its most actionable sentence: submodule checkout : <unknown> this repo pins : ... either <root> is not a git checkout, or its HEAD carries no libs/nistcurves gitlink. The submodule is not checked out at all, either. Also fixes the literal `\n` in the zp_config.s call-site string — the one site using `\n` inside a `cat <<EOF` heredoc, visible mangled in the RED capture above and rendering as a real line break in the GREEN one. MEASURED CORRECTION to the reported analysis: `case "$(... | grep -c .)"` is NOT a fourth instance. A command substitution in a `case` word has its status discarded, so `set -e` never sees it. Measured both ways — with the assignment guarded and that line left bare, the `0)` arm still runs and prints (capture 2 above is from exactly that build). It is the ASSIGNMENT that is fatal, not the pipeline shape. The line is therefore left bare and the asymmetry documented in place, so nobody adds a cargo-cult guard here or copies the bare form into an assignment where it does bite. Healthy-path control: the same script run against an initialised checkout completes normally (`ZP overrides verified in zp_config_p256verify.o; bare zp_* suppressed across 18 members`, exit 0). All five profiles build to PRGs byte-identical to master (5/5, hashes in the PR body). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
tools/test_tls_p384_negotiation.py attributed sub-test [1b]'s C=1 to
"ecdsa_verify_384_tls's DER parse rejecting the 48-zero-byte dummy
signature". That is the ENABLE_P384_VERIFY=1 mechanism. In the build the
test runs against the flag is off, ecdsa_verify's P-384 arm is a bare
`sec`, and ecdsa_verify_384_tls is neither called nor linked.
Confirmed from artifacts, not reworded:
- src/crypto/ecdsa_verify.s: the arm is
.ifdef ENABLE_P384_VERIFY
jsr ecdsa_verify_384_tls
.else
sec ; C=1 — unsupported curve, clean reject
.endif
jmp @done
- Makefile:388-393 — P384_SRCS (ecdsa_verify_384.s, p384_force_link.s)
are `filter-out`ed from CRYPTO_SRCS_EFFECTIVE unless the flag is 1,
so the object never reaches the link line.
- In the very PRG this run tested (default ip65 build), ecdsa_verify is
at $6B41 and reads
20 95 3B AD 7F B6 F0 04 38 4C 54 6B
jsr lda beq sec jmp @done
and `ecdsa_verify_384_tls` appears 0 times in build/c64-https.map.
- Same shape on BACKEND=uci USE_NISTCURVES_ONCHIP_COMB=1
($69C8: 20 81 28 AD 7F B0 F0 04 38 4C DB 69).
All eight stale sites in the file are corrected, not just the two [1b]
docstrings:
- :11-13 module docstring said the branch is "a `sec / rts` stub that
Phase 4a fills in" — Phase 4a landed and is gated, not pending.
- :26-40 [1b] summary docstring (the reported site).
- :250-258 test_cert_verify_p384_dispatch docstring (the reported site).
- :265 "Phase 4a's dispatcher composes ..." now says "when armed".
- :335-343 the FAIL-branch comment and its message string
"(DER rejection / stub rejection)" — the retracted mechanism,
sitting in the branch a reader reaches when the test FAILS.
Now "(gated-off P-384 arm should `sec`)".
- :348 PASS string said "Phase 4a dispatcher reached"; the dispatcher
reached is ecdsa_verify, so it now says so.
- :294 "value irrelevant under the stub" — the same retired
vocabulary; the 48 dummy bytes are never read because the
gated-off arm rejects on curve_id before any signature parse.
- :6-8 pre-existing, inside the docstring this commit edits: "This
file verifies\nalongside ecdsa_secp256r1_sha256 (0x0403) in
the ClientHello" had lost the clause naming what is verified,
leaving the sentence without an object. Restored to say the
ClientHello advertises 0x0403 and NOT 0x0503, which is what
[1a] asserts.
Byte-count correction from review: the earlier wording called the arm
"three bytes". `38 4C 54 6B` is four, and the gated arm proper is one
(`38`) — the `jmp @done` is shared with the P-256 path. Reworded to say
that rather than count.
src/tls_cert.s:670-674 carries the same stale mechanism. Left out because
this is a docs/test commit and that is source — not for any cost reason;
an earlier draft argued one and it did not survive checking. Follow-up.
Test after the edits:
$ python3 tools/test_tls_p384_negotiation.py
[1a] PASS: schemes advertised = 0x0403 (0x0503 correctly absent)
[1b] PASS: cv_sig_scheme=1, ecdsa_curve_id=1, C=1
(ecdsa_verify dispatcher reached)
Passed: 2/2 Failed: 0/2
Comments and strings only; all five profiles byte-identical to master.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…stale (#193) CLAUDE.md contradicted itself on three region margins, each in the same way: #211's fix moved ~27 B into HTTP_AUX_CODE2, one paragraph was updated and the others were not. This is load-bearing, not a doc nit — tools/uci/_memory_policy.py hands rig scratch out of the comb CRYPTO_OVERLAY tail, so an over-stated figure is a silent crypto-corrupting collision, and :805 sits in the paragraph that reads as the authoritative ip65 sizing block. All measured here off build/c64-https.map, per profile, P-384 objects gated out, at the v0.14.0 pin. uci-comb (make BACKEND=uci USE_NISTCURVES_ONCHIP_COMB=1; ld65 line confirms cfg/c64-https-uci-onchip.cfg, and neither ecdsa_verify_384.o nor p384_force_link.o is on it): cfg:163 CRYPTO_OVERLAY: start = $4200, size = $1E00 -> ends $5FFF map LIB_NISTCURVES_MUL_CODE 005EE0 005F81 (last tenant; next segment UCI_BSS starts 006000, i.e. CRYPTO_HOT) $5FFF - $5F81 = $7E = 126 B free :558 already said 126 B; :560 and :755 said 153 B. Corrected. ip65 (measured on BOTH ip65 profiles — plain `make`, REU_BANKS_USED=0003, and `make USE_NISTCURVES_ONCHIP=1`, REU_BANKS_USED=0000; the two figures below are identical across them, but see the CRYPTO_RESIDENT note further down, which is NOT): cfg:90 LOADER: start = $0801, size = $17FF -> ends $1FFF map CODE 00080D 001FEE (last tenant) $1FFF - $1FEE = $11 = 17 B free :311 and :805 said 21 B; :625 said 17 B. Corrected. cfg:103 CRYPTO_OVERLAY: start = $4F8C, size = $1074 -> ends $5FFF map HTTP_AUX_CODE2 005EBF 005F82 (last tenant) $5FFF - $5F82 = $7D = 125 B free :805 said 152 B; :626 said 125 B. Corrected. Re-measured and NOT changed as figures, because they were already right: NET_CODE $2000 + $2000 -> $3FFF; HTTPS_TARGET_RODATA ends $3FC7 $3FFF - $3FC7 = $38 = 56 B (matches :804; same on both ip65 profiles) CRYPTO_RESIDENT $6000 + $4000 -> $9FFF; LIB_NISTCURVES_MUL_CODE ends $9F6E; $9FFF - $9F6E = $91 = 145 B (matches :805) CRYPTO_RESIDENT is the one figure here that is NOT profile-independent, and the paragraph carrying it named no profile: ip65 REU LIB_NISTCURVES_MUL_CODE 009EF2 009F18 ($27) -> 231 B free ip65 onchip LIB_NISTCURVES_MUL_CODE 009ECD 009F6E ($A2) -> 145 B free 145 B is the ip65-onchip value, i.e. the shipped c64-https-ip65-onchip.prg, so the number was correct and conservative but presented as universal in a file whose own maintenance note says margins are per profile. The block is now labelled ip65-onchip and states which of its figures the REU profile shares (LOADER, CRYPTO_OVERLAY, NET_CODE) and which it does not. Also corrected, same cause, lower stakes: :820 named "HTTP_AUX_CODE2's 169 B". That segment is $C4 = 196 B today (169 B was its pre-#211 size). The surrounding claim is unaffected and stands — LIB_NISTCURVES_MUL_CODE is $A2 = 162 B on ip65-onchip and is still the smallest segment in the CRYPTO_OVERLAY+CRYPTO_RESIDENT pool. :612's warning ("took ip65's LOADER from 21 B free to zero; the next byte anyone added would not link") kept its 21 B, which was correct for that episode, and now states the 17 B measured today alongside it. The 4 B difference is now measured rather than left open: rebuilding ip65-onchip with ONLY src/http.s reverted to 60022de moves http.o's CODE from $35E to $35A and the whole CODE segment from $17E2 to $17DE, i.e. CODE ends $1FEA and LOADER is back at 21 B free. So the shrink is http.o's own CODE growth in #211's fix. Attribution is to the module and the commit; nobody has disassembled which four bytes, and the file says exactly that. Also corrected, same block, same family, found in review: :828 said "LOADER_OVERFLOW carries ~125 B of http.s that outgrew LOADER". src/http.s has no `.segment "LOADER_OVERFLOW"` at all — src/http.s:1096-1097 records the eviction ("W4: moved from LOADER_OVERFLOW to HTTP_AUX_CODE"). The segment is 332 B and its measured tenants are boot.o 110 B, vic.o 18 B, crypto_swap.o 204 B ($6E + $12 + $CC = $14C). Wrong module and wrong number; the line now names the three real tenants. Prose only; all five profiles byte-identical to master. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.
Closes #217, closes #208, and closes the CLAUDE.md sub-item of #193.
One commit each; disjoint files. All five profiles build to PRGs byte-identical to master (5/5) — nothing here can reach an image.
e3ef15e— #217, and three more of the same defectUnder
set -eo pipefail, four error paths intools/integration/build_nistcurves_p256.shkilled the shell inside the branch written for exactly the condition being reported, leaving a bare non-zero exit and no message.lib_preflight_fail'spin=line was the only one of three adjacentgitcalls without|| true. REDexit=128, zero output; GREEN the full Build error in build_nistcurves_p256.sh #124 remedy message,exit=1.ZP_MEMBER="$(ar65 t … | grep '^zp_config')"— with no matching membergrepexits 1, the assignment dies, and the0)arm never runs. A silently droppedzp_configmember is the Build error in build_nistcurves_p256.sh #124 runtime-corruption class:zp_ptr2reverts to$fdand collides withzp_temp/zp_count, with no link error. REDexit=1silent; GREENERROR: no zp_config member in ….check_zp_slot'sgot=$(od65 … | awk …)— a failingod65makes the${got:-<absent>}message unreachable. REDexit=3silent; GREEN the full export mismatch.pinempty, the diagnosis branches asserted a wrong cause: a populated submodule under a non-repo root blamed a stale checkout and prescribed agit submodule updatethat cannot run there. Emptypinnow has its own first branch and its own remedy text, and the original both-empty case keeps its "not checked out at all" sentence as an independent, appended condition rather than losing it to the reordering.Plus the literal
\nthat rendered mangled in exactly the message it belonged to.A measured correction to the review that prompted this.
case "$(… | grep -c .)"is not a fourth instance: a command substitution in acaseword has its status discarded andset -enever sees it. Verified three ways (arm matched, no arm matched,false | grep -c .) plus end-to-end with the assignment guarded and this line bare — the0)arm runs and prints. It is the assignment that is fatal, not the pipeline shape. The line is left bare and the asymmetry documented in place, deliberately rather than given a|| truethat would do nothing: a dead guard there would read as evidence the mechanism was understood. The reviewer who raised it independently retracted after reproducing all three variants.Every assignment site in the script is now guarded (
grep -nE '=\$\(|="\$\('→ 53, 152/153/160/168, 439, 452), with the twofor m in $(ar65 t …)loops correctly left bare as word-list expansions.efe2dc2— #208: the P-384 negotiation test described an armed build[1b]'sC=1was attributed toecdsa_verify_384_tls's DER parse. That is theENABLE_P384_VERIFY=1mechanism; unarmed — every build anyone runs — the carry comes from a baresecin the gated-off arm, andecdsa_verify_384.sis filtered out of the link entirely.Verified four independent ways rather than reworded: the
.ifdefinsrc/crypto/ecdsa_verify.s;Makefile:388-393'sfilter-out; the caller insrc/tls_cert.stail-callingjmp ecdsa_verifywithcurve_id=1; and the bytes of the PRG the passing run tested ($6B41: 20 95 3B AD 7F B6 F0 04 38 4C 54 6B, withecdsa_verify_384_tlsappearing 0 times in all five maps).Review confirmed this is the sole mechanism by patching
$6B4938→18(SEC→CLC) in the PRG:[1b]went red, restoring it went green. No third source of the carry.Six sites corrected, not the two originally identified — including the module docstring's "still a
sec / rtsstub that Phase 4a fills in", the FAIL-branch comment and message string (now(gated-off P-384 arm should sec)), and the PASS string, which claimed "Phase 4a dispatcher reached" when the dispatcher reached isecdsa_verify. Both the FAIL and PASS strings are exercised — PASS by the normal run, FAIL by the mutation above. Suite still 2/2.src/tls_cert.s:670-674carries the same retired vocabulary and is deliberately left alone: this is a docs/test commit and that is source. Filed as follow-up.7ae1804— #193: every post-#211 region margin, re-measured#211 moved ~27 B into
HTTP_AUX_CODE2. One paragraph was updated and three were not, leaving CLAUDE.md contradicting itself on three margins — twice within two lines of each other. Load-bearing:tools/uci/_memory_policy.pysizes rig scratch from the combCRYPTO_OVERLAYtail, and the ip65 block reads as the authoritative sizing paragraph.LOADER($1FFF − $1FEE)CRYPTO_OVERLAY($5FFF − $5F82)CRYPTO_OVERLAY($5FFF − $5F81)Also corrected:
HTTP_AUX_CODE2is 196 B, not the pre-#211 169 B.LOADER_OVERFLOWno longer claims to carry "~125 B ofhttp.s".http.shas no.segment "LOADER_OVERFLOW"at all —src/http.s:1096-1097records the eviction itself. The real tenants areboot.o(110 B),vic.o(18 B) andcrypto_swap.o(204 B) = 332 B.The ip65 figures are now labelled
ip65-onchip — the shipped product, because they are not all profile-independent:CRYPTO_RESIDENTis 231 B on the REU profile against 145 B on onchip (LIB_NISTCURVES_MUL_CODEat$27vs$A2). LOADER,CRYPTO_OVERLAYandNET_CODEgenuinely are identical across the two, and the block now says which is which. This is precisely the per-profile hazard CLAUDE.md's own maintenance note describes, and the previous wording generalised from three matching figures to a fourth that does not.The 21 → 17 B LOADER shrink is attributed to
http.o'sCODEgrowth from #211's own fix, isolated by reverting onlysrc/http.sto60022deand rebuilding (Size=00035A→00035E, whole-segmentCODE0x17DE→0x17E2). Attribution is to the module and the commit; nobody disassembled which four bytes, and the file says so.Evidence
#217 is red-green on four reproduced failures plus a healthy-path control at exit 0. #208 and #193 are documentation corrections whose claims are each backed by a build artifact quoted in the commit — a map segment row, a module list, PRG bytes — never by another document.
Two adversarial rounds. Round 1 found the commit fixing CLAUDE.md's self-contradiction had left two more of identical origin; round 2 found a third (
CRYPTO_RESIDENT) and a stale attribution (LOADER_OVERFLOW), both now closed, and retracted its owncase-line finding.🤖 Generated with Claude Code