tests: land the review round's reproductions, with the refuted claims turned into what is true - #197
Merged
Merged
Conversation
… turned into what is true
The 24-agent review round produced 20 test files that were never committed, because 11 of their 31
reproductions had been refuted by an adversarial pass and shipping them would have put two mutually
exclusive assertions in the same suite - one of them asserts the opposite of a test that passes
today. Leaving all 20 out cost the 77 PASSING tests in them, which is the larger number: controls,
matrices and ordinary coverage the agents wrote alongside their claims.
So the refuted 11 are converted rather than deleted. Each drove a real scenario that was already
wired up; what was wrong was the conclusion. Each now pins the behaviour that was verified correct
and records why the claim did not hold:
resume a ticket issued before a rotation is REFUSED after it, and the client still served.
Ticket keys are per SSL_CTX; nginx, Apache and HAProxy retire them on reload too, and
the test below it depends on that - a ticket outliving a rotation would carry the old
verify verdict past the new anchors.
truncation the pipe reports both endings alike and the SESSION tells them apart. Faulting the
cut one would fault every client that merely disposes its SslStream.
identity a CN the decoder refuses is not named, even where the rendered subject cannot tell it
apart - PeerSubject is documented for people, PeerCommonName is what authorizes.
mtls/quic a DN or CN too long to record is reported as NO name, never a prefix. Fails closed on
purpose: a prefix can belong to someone else.
quic/fault a throwing HandshakeCompleted is logged and the connection keeps serving, which is
this runtime's policy for user-code faults everywhere else.
http3 a body cut mid-frame kills the connection with H3_FRAME_ERROR and the reader signals
an ordinary end - the contract all three body readers document identically.
parity omitting the host table is refused on QUIC and applied on TCP, pinned as a KNOWN
DIVERGENCE. Neither side is a bug; TCP's is documented and pinned green, so resolving
it is a deliberate breaking change rather than a fix, and this makes sure nobody
resolves it by accident.
lifetime deleted - the file already had a passing test for the only true part.
Two were better answered by fixing the code than by rewriting the test, which both the refuting
agent and the commit auditor independently recommended: TlsService.Start now range-checks
MinProtocolVersion and HandshakeTimeoutMs. Both resolved to something plausible rather than
failing - an undefined version through a not-Tls13 ternary to the TLS 1.2 floor, and a negative
timeout to no handshake sweep at all, silently removing the only bound on a peer that connects and
says nothing. No config binder validates an enum, so neither needed a cast to arrive.
Two identity doc comments were corrected to match: null from PeerSubject/PeerCommonName is a
refusal, not "the peer offered none".
The 19 surviving reproductions stay as Pending - they report PEND and fail the run if they start
passing. 419 passed, 0 failed, 20 pending, across all six suites.
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.
A 24-agent review round produced 20 test files that were never committed. 11 of their 31 reproductions had been refuted by an adversarial pass, and shipping those would have put two mutually exclusive assertions in one suite — one asserts the opposite of a test that passes today.
Leaving all 20 out was the wrong trade, because the refuted claims were the smaller half. Those files also hold 77 passing tests: controls, matrices and ordinary coverage written alongside the claims.
The refuted claims are converted, not deleted
Each drove a real scenario that was already wired up. What was wrong was the conclusion, so each now pins the behaviour that was verified correct and records why the claim did not hold.
resumetruncationidentitymtls/quic(×2)quic/faultHandshakeCompletedis logged and the connection keeps servinghttp3H3_FRAME_ERROR; the reader signals an ordinary endparitylifetimeThree of those are worth stating in full, because the refutation is the interesting part.
Resumption. Ticket keys are per
SSL_CTXandReplaceCertificatesbuilds new contexts, so a rotation retires them — which is what nginx, Apache and HAProxy do on reload unless an explicit ticket-key file says otherwise. The test immediately below it depends on that: anchors given as a path are re-read on every rotation, so a ticket that outlived one would carry the old verify verdict past the new anchors. The two could not both pass.Truncation.
TlsDecryptingPipeReaderdocuments the rule it was accused of breaking: close_notify is a clean end, a closed snapshot without one is the peer vanishing, and the difference is left to the caller throughTlsSession.Closed. Faulting the cut one would fault every client that merely disposes itsSslStreamwithout callingShutdownAsync— the ordinary polite close. A TLS fault is still kept and reported.Parity. Neither stack is wrong on its own. TCP's behaviour is documented on the parameter and pinned by a passing test, so raising it to QUIC's stricter contract is a deliberate breaking change that deletes that test — a decision, not a fix. The test now records that the divergence exists, so whichever way it is resolved, nobody resolves it by accident.
Two were better answered in code
TlsService.Startnow range-checksMinProtocolVersionandHandshakeTimeoutMs. Both previously resolved to something plausible instead of failing: an undefined version reached the TLS 1.2 floor through anot-Tls13ternary, and a negative timeout disabled the handshake sweep entirely, because both readers guard on> 0. That silently removes the only bound on a peer that connects and then says nothing, which is the part of a TLS server reachable before anything is authenticated. No config binder validates an enum —Enum.Parse<TlsProtocolVersion>("3")succeeds — so neither value needed a cast to arrive.Two identity doc comments were corrected to match the code:
nullfromPeerSubjectandPeerCommonNameis a refusal, not "the peer offered none".What stays open
The 19 surviving reproductions remain
runner.Pending. They report PEND while they still fail and fail the run the moment one starts passing, which is how three defects in this round were caught as fixed rather than being quietly promoted.All six suites: 419 passed, 0 failed, 20 pending.