Repository navigation
Three VICE suites pass or fail for reasons unrelated to what they measure (test_finished_verify's 16 negatives are vacuous on UCI) #161
Description
Three VICE suites currently pass or fail for reasons unrelated to what they claim to measure. All three share one shape: the rig does not establish an invariant the routine relies on, and the invariant differs by build configuration. Found during the adversarial review of #160; none is caused by that PR.
1. test_finished_verify.py — 16 negative cases pass vacuously on UCI
The good news first: server-Finished verification is intact. Proved, not inferred — adding two lines to call_verify_finished that set tls_hs_ptr = tls_rec_buf before the jsr, against an unmodified PRG, gives 18/18. Nothing else changed.
The cause. src/tls_keyschedule.s:727-732:
tls_verify_finished:
.ifndef TLS_STREAM_DEFRAME
jsr tls_hs_ptr_reset
.endifUnder TLS_STREAM_DEFRAME (UCI) the reset is compiled out, because the deframer owns tls_hs_ptr ($3E-$3F) and sets it per message before dispatch. The compare is lda (tls_hs_ptr),y at offset 4. A direct DMA caller with no deframer in the loop leaves tls_hs_ptr holding whatever was there last, so the routine compares an unrelated 32 bytes while the rig writes and reads back tls_rec_buf+4.
tools/test_finished_verify.py:309 runs bare make, i.e. ip65, where tls_hs_ptr_reset still fires and the suite is correct. It misbehaves only under C64_SKIP_BUILD=1 against a UCI PRG.
The uncomfortable half. On a UCI build the two positive cases fail loudly — but the 16 negative cases pass vacuously. They compare against a stale pointer, so they would report "reject" regardless of what the routine did. If someone inverted the mismatch branch tomorrow, this suite would still show 16/18 on UCI and only the positives would move.
This suite exists because the 2026-08-12 audit found the Finished-mismatch abort had no test at all. On the backend that ships to hardware, it currently provides no coverage of that branch.
Fix: set tls_hs_ptr in the rig before the call. Unconditionally is fine — it is what the ip65 build does internally anyway.
2. A docstring that promises the opposite of what the code does
src/tls_keyschedule.s:712-716 tells direct callers the pointer "is reset to tls_rec_buf at entry (see tls_hs_ptr_reset)" and names tools/test_finished_verify.py as the beneficiary. That promise has been false under UCI since W1 landed.
This is the reason the failure survived: anyone debugging the suite would read the docstring, conclude the pointer was handled, and look elsewhere. Correct it in the same change.
3. test_tls_deframe.py — five Certificate cases rejected for the wrong reason
Not "the hostname check rejecting a self-generated cert". src/x509_name.s:60-64 opens:
lda tls_hostname_len
bne :+
sec
rtstls_hostname is populated by http_get at runtime, which the rig never calls. So tls_hostname_len == 0 and the routine rejects everything, correct SAN or not.
Confirmed directly: writing tls_hostname = "www.foo.bar" / tls_hostname_len = 11 makes the identical streamed Certificate that returns DF_ERR_CERT_FMT in the stock rig return msg, with the public key correctly extracted.
Fix: two write_bytes in the rig. Recovers six real Certificate assertions.
4. test_tls_deframer.py — wikipedia_shaped_flight, almost certainly the same
No tls_hostname reference anywhere in the file. Very likely identical to (3), but this was not confirmed directly and should not be assumed.
Why one issue rather than four
All four are the same defect class: a harness that does not set up a precondition, where the precondition's necessity is configuration-dependent and therefore invisible on the backend the suite was written against. Fixing them individually without naming the class invites the fifth instance.
Note the interaction with the test-suite work: these suites are not in the vacuous-exit set fixed by #158 — they exit non-zero, so a runner sees them as red. The problem is subtler: they are red for a reason that is not the reason they report, and one of them is simultaneously green for cases it is not testing.
Suggested order
test_finished_verify.py— highest value; converts 16 vacuous negatives into real ones on the shipping backend.- The
tls_keyschedule.sdocstring — free, and it is what hid the problem. test_tls_deframe.py— recovers six assertions for two lines.test_tls_deframer.py— confirm first, then fix.
Activity
JC-000 commentedon Aug 30, 2026
A triage rule for this class, from a sibling lane
The c64-wireguard lane hit the same defect shape today by a third route, and offered a framing that is more useful than the one in this issue's description.
They wrote a test whose assertion grepped a function's source for a keyword. It passed against the exact mutant it existed to catch, because the keyword also appeared in a nearby log message. Their generalisation: two copies of a fact, only one of them real.
That unifies three things found today, in three different substrates:
test_hs_sequence.pyat 11/11, because the out-of-range indexes landed on$4Cand$A9from a neighbouringjmpandlda #0— the reject survived by code layout, not logicThe common shape is not "stale documentation" or "weak test". It is: an assertion passes because something coincides with the truth, and nothing distinguishes coincidence from cause. A keyword in a log message, a byte in an adjacent instruction, and a sentence in a second file are the same bug wearing different clothes.
The rule worth adopting
Their prescription, which is mutation testing applied to prose:
Applied to the eight defects in #156, most fail it — the corrections are prose restatements with no mechanism that would notice them going stale again. So they will drift again, and the fix bought a correct file today rather than a file that stays correct.
Where a fact can be mechanically pinned, pin it instead of restating it. Two in-tree instances already exist and should be read as instances of a rule rather than one-off fixes:
.assert LIB_NISTCURVES_REU_BANK_COMB = $02(Load the Lim-Lee comb table from disk instead of computing it at boot #120) — a bank move becomes a link failure by name;build/flags.stamp(build: make the flag set a tracked dependency (build/flags.stamp) #159) — the build-flag invariant becomes enforced rather than remembered.Both replace a sentence someone must remember with a build error.
And the detection method
Both instances were found by deliberately breaking the thing and checking the alarm sounded — they mutation-tested their own test; our range-check hole surfaced only because the adversarial reviewer was told explicitly to mutate the fix. A passing suite never prompts that. It is cheap relative to what it catches, and it is the only method that has actually worked on this class today.
JC-000 commentedon Aug 30, 2026
Two qualifications to the rule above — both needed, from the same sibling lane
Recording these because the unqualified version of "pin it instead of restating it" is itself the kind of advice that goes green by coincidence.
1. A pin can pin the wrong thing
Pinning only helps when the pin carries the meaning. A pin on a value that merely happens to be right today — a size, an offset, a hash — recreates the same problem one level down: it passes because it coincides, and now it also has to be maintained.
The triage question is the same one that generated the rule: would this pin change if the fact did?
Applied to the pins currently in flight in this repo:
.assert LIB_NISTCURVES_REU_BANK_COMB = $02(#120).asserts onTLS_STATE_*(#160)build/flags.stamp(#159)CA65FLAGS/LD65FLAGS, so a knob added later is covered on the day it is written. A hand-maintained knob list would fail — and would have missedBACKEND, which reaches the link through$(CFG)and never appears as a-DThat last row is the same distinction arriving from a different direction, and it is the clearest in-tree example of a pin that looks strong and answers a narrower question than people read into it.
2. Mutation testing is a floor, not a ceiling
It proves an assertion can fail. It does not prove it fails for the right reason, and it says nothing about the mutant nobody thought of. The #160 range-guard mutants caught what the reviewer imagined; a fifth variant would have gone unexamined.
Worth stating explicitly wherever this gets written down, or mutation coverage becomes the next thing trusted further than it earns — which is precisely the failure this issue is about.
Why the machine-code instance is the canonical one
Of the three substrates, the
$4C/$A9case is the one to lead with, and for a reason I had not appreciated: it is the only one unstable under unrelated edits.That makes it worse than a missing test, because a missing test is at least honestly absent.
test: record the ip65 measurement at the finished_verify dispatch arm
Merge pull request #176 from JC-000/fix/161-vacuous-vice-suites