test(corpus): sweep for vacuous assertions and settle carried findings by cuioss-oliver · Pull Request #151 · cuioss/API-Sheriff

cuioss-oliver · 2026-08-04T00:05:11Z

Summary

Censuses the API Sheriff test corpus for tests whose assertions cannot support the behaviour they are
credited with covering, publishes the full classification as a checked-in note
(doc/development/test-corpus-integrity.adoc), strengthens a capped subset highest-risk-first, and
settles four carried items in the same PR: the four PLAN-05 Sonar new_code findings, the two public
API elements whose load-bearing status was undetermined, and the pending .plan/marshal.json
configuration change.

The method is deliberately census-then-targeted-read, not a naive 135-file linear pass. A linear
pass over a corpus this size degrades into spot-checks — which is the exact apparent-vs-real-coverage
failure this sweep exists to catch. Every coverage claim below is reproducible from a published
census count; nowhere does this PR assert "the whole corpus was reviewed", because no such claim
could be verified.

Changes

D1 — Corpus sweep, classification, and the capped strengthening

Declared surface (the denominator)

Marker	Files	Occurrences
`@Test`	135	1271
`@ParameterizedTest`	35	97
Declared surface (union)	135	1368

Both annotations are part of the denominator. A @Test-only count under-states the real
test-method population by 97 methods (7.1%). After this change the union is 1364 (−5 @Test
converted to +2 @ParameterizedTest in PassthroughHostGuardStageTest, −1 for the single delete
verdict); the file count is unchanged at 135. Every census ran under complete coverage: 300 files
scanned, 0 unreadable, not truncated, 0 elided.

The stated cap, and why it was deliberately under-filled

The cap is 20 test methods — 8 mandatory + up to 12 discretionary — stated up front, before
selection.

Actually strengthened: 18 (8 mandatory + 10 discretionary), plus 1 delete.

The discretionary budget was not filled to twelve because the discretionary pool yielded only ten
strengthen verdicts; every remaining flagged method in the pool earned keep on the
matched-control test. Padding to twelve would have meant rewriting tests that were already correctly
scoped — churn, not integrity. This under-fill is a deliberate verdict-driven outcome, not an
omission or an unfinished quota.

Classification — one row per flagged method, exactly one verdict each

56 methods classified across 15 files (12 mutation-pool files + 3 control-sample files), every row
carrying a file:line citation.

TokenValidatorProducerTest — 7 flagged

Method	Line	Verdict	Evidence
`buildsFromHttpIssuerWithAudience`	67	strengthen	(d) name promises an audience contrast the assertion cannot distinguish
`buildsFromHttpIssuerWithoutAudience`	85	strengthen	(d) the mirror of the above; swapping the arrange blocks leaves both green
`listedHostIsExempted`	193	keep	matched control `exemptionIsScopedToTheListedHost`:211
`severalHostsAreAllowlisted`	229	keep	matched control `exemptionIsScopedToTheListedHost`:211
`allowlistSurvivesTheProducerPath`	247	strengthen	(d) claims the allowlist is carried through the full producer path; `assertNotNull(validator)` shows only that a validator was built
`omittedProfileNeverConsultsTheResolver`	302	keep	neutralised fixture — the registry throws on any lookup, so completing IS the proof
`completesForAValidlyConfiguredValidator`	413	keep	matched control `forcesEagerAssemblyByInvokingTheValidator`:398

PassthroughHostGuardStageTest — 7 flagged

Method	Line	Verdict	Evidence
`passesBenignHost`	116	strengthen	(d)/(c) mandatory — `java:S5976` triple 1
`passesNullHost`	126	strengthen	(d) mandatory — `java:S5976` triple 1
`passesSuffixLookalikeHost`	136	strengthen	(d) mandatory — `java:S5976` triple 1
`passesHostWithNonNumericPort`	146	strengthen	(d) mandatory — `java:S5976` triple 2
`passesHostWithEmptyPort`	157	strengthen	(d) mandatory — `java:S5976` triple 2
`passesHostWithMultipleColons`	167	strengthen	(d) mandatory — `java:S5976` triple 2
`inertWhenPassthroughSetEmpty`	185	keep	matched control `rejectsSmuggledHost`:54

ThoroughChecksStageTest — 9 flagged

Method	Line	Verdict	Evidence
`acceptsLegitimateRequestUnderDivergentFilter`	95	keep	matched control `rejectsDivergentFilterViolation`:80
`skipsReRunWhenRouteConfigEqualsDefault`	112	strengthen	(d) cannot distinguish "the re-run was skipped" from "the re-run ran and passed"
`fallsBackToBaselineWhenRouteDeclaresNoConfig`	123	strengthen	(d) cannot distinguish "fell back to the baseline" from "did nothing"
`admitsWildcardWhitelistMatch`	170	keep	matched control `rejectsWildcardSegmentCountMismatch`:180
`acceptsParameterValidationTheModeDisables`	329	keep	matched control `strictRouteStillRejectsWhatMinimalAccepts`:355
`skipsPipelineReRun`	343	keep	matched control `strictRouteStillRejectsWhatMinimalAccepts`:355
`skipsHeaderNameValidationUnderMinimal`	405	keep	matched control `rejectsHeaderNameUnderDivergentConfig`:387
`skipsHeaderNameReRunWhenConfigEqualsBaseline`	418	keep	neutralised fixture — the stage's own baseline refuses the name, so a re-run would throw
`admitsLongCookieOnCookieModeGateway`	525	keep	matched control `bearerOnlyGatewayStillRejectsLongCookie`:539

TlsEdgeProducerTest — 3 flagged (the sharpest instance in the corpus: all three test methods
wholly vacuous, none with a control)

Method	Line	Verdict	Evidence
`startsAndStopsFrontListener`	70	strengthen	(d) cannot distinguish "the front listener started" from "it was never started"
`noFrontListenerWhenPassthroughEmpty`	101	strengthen	(d) cannot show the listener was not started
`skipsUnresolvedAlias`	118	strengthen	(d) cannot show the relay map is empty

BasicChecksStageTest — 7 flagged

Method	Line	Verdict	Evidence
`acceptsLegitimatePath`	111	strengthen	(b)/(d) name claims it records the single canonical path; `assertNotNull` never says which
`doesNotValidateParameterNamesOrValues`	124	keep	the arrange is a value the post-route stage rejects, so the admission is discriminating
`acceptsCookieHeaderAtBudget`	191	keep	matched control `rejectsCookieHeaderBeyondCap`:213
`strictBaselineAcceptsCookieHeaderAtBudget`	202	keep	matched control `strictBaselineRejectsCookieHeaderBeyondCap`:226
`admitsBearerTokenAboveBaselineCap`	300	keep	matched control `rejectsAuthorizationBeyondConfiguredCap`:315
`matchesHeaderNameCaseInsensitively`	328	keep	a case-sensitive implementation rejects, so the test can fail
`extendedAsciiIsAdmittedUnderAPermissiveBaseline`	389	keep	is itself the declared matched control for :356 and :372

ConfigProducerTest — 3 flagged

Method	Line	Verdict	Evidence
`shouldBootWhenTheDeclaredBodyCapEqualsTheFrameworkLimit`	259	keep	matched control `shouldRefuseBootWhenADeclaredBodyCapExceedsTheFrameworkLimit`:239 — a boundary pair, the #148 precedent
`shouldAssembleEagerlyOnStartupForValidConfig`	287	strengthen	(d) cannot distinguish eager assembly at `onStartup` from lazy assembly at the later `gatewayConfig()` call
`shouldResolvePassthroughSniAliasValueRatherThanHostKey`	335	keep	matched control `shouldFailBootWhenPassthroughSniAliasIsUnresolvable`:346

BffRuntimeProducerTest — 1 flagged

Method	Line	Verdict	Evidence
`shouldAssembleWithoutDiscovery`	97	strengthen	(d) claims assembly resolves no OIDC discovery; not throwing cannot show discovery was skipped

RouteRuntimeAssemblerTest — 1 flagged (F2, partially vacuous)

Method	Line	Verdict	Evidence
`shouldAssembleSessionRoutes`	163	strengthen	(b) three further scenarios each assemble a route table and discard the result, asserted only by `assertDoesNotThrow`

AuthenticationStageTest — 3 flagged

Method	Line	Verdict	Evidence
`passesRequireNone`	66	delete	strictly subsumed by `passesRequireNoneWithoutResolvingValidator`:77 — same arrange plus a throwing validator provider
`passesRequireNoneWithoutResolvingValidator`	77	keep	neutralised fixture — the provider raises when resolved, so completing proves the validator was never resolved
`acceptsValidBearerToken`	91	keep	matched controls at :103, :118, :133

CsrfDefenceTest — 4 flagged, all keep

Method	Line	Verdict	Evidence
`safeMethodsBypass`	73	keep	matched control `bothHeadersAbsentRejected`:126
`unsafeMethodTrustedOriginAccepted`	83	keep	matched control `unsafeMethodUntrustedOriginRejected`:92
`trustedOriginMatchIsCaseInsensitive`	100	keep	matched control :92
`absentOriginSameOriginFetchAccepted`	109	keep	matched control `absentOriginCrossSiteFetchRejected`:118

GatewayEdgeRouteTest — 5 flagged, all keep

Method	Line	Verdict	Evidence
`bootsCleanlyOverEmptyRouteTable`	121	keep	(a) construction test, correctly scoped; `registersCatchAllRoute`:132 asserts on the assembled edge
`bootsSessionAuthRoute`	147	keep	regression guard for a removed boot rejection — restoring it fails this test
`bootsGrpcProtocol`	161	keep	same shape, gRPC
`bootsWebSocketProtocol`	174	keep	same shape, WebSocket
`bootsSessionAuthWebSocketRoute`	187	keep	same shape, WebSocket + session

SessionAuthenticationStageTest — 0 flagged. Five assertDoesNotThrow occurrences, every one
followed by a substantive assertEquals. Clears the density gate; contributes nothing to the
vacuity population.

Control-sample rows (3 un-ranked files read in full) — 6 flagged, all keep

Method	File	Line	Verdict	Evidence
`acceptsWellFramedPost`	`FramingGateTest`	139	keep	seven matched rejection controls in the same class
`onAdmitsGetWithContentLengthBody`	`FramingGateTest`	187	keep	eight "STILL rejects" controls pin the single relaxed leg
`onAdmitsCleanGet`	`FramingGateTest`	271	keep	same control block
`emptyAllowlistEnforcesNothing`	`OriginValidationStageTest`	66	keep	matched control `foreignOriginRejected`:92
`allowlistedOriginProceeds`	`OriginValidationStageTest`	75	keep	matched control `foreignOriginRejected`:92
`matchingIsCaseInsensitiveOnHost`	`OriginValidationStageTest`	83	keep	matched control :92

Classification totals

Population	Methods
Flagged in the mutation pool (12 files, read in full)	50
Flagged in the control sample (3 files, read in full)	6
Total classified	56
— verdict `strengthen`	18
— verdict `keep`	37
— verdict `delete`	1

The countable backlog

43 files carrying 140 vacuous-shape marker occurrences were censused but not individually
classified. That is the standing backlog and the honest measure of what this sweep did not cover.
It is derived arithmetically from the published census, not estimated:

Population	Files	Marker occurrences
Marker-carrying files in the D1 surface (`assertDoesNotThrow` ∪ `assertNotNull`)	58	267
— read in full: the mutation pool	12	113
— read in full: the control sample	3	14
— censused, not individually classified (the backlog)	43	140

Every backlog entry is enumerated by name in the note with its measured marker density and the gate
that excluded it (24 files / 73 occurrences under api-sheriff, 19 files / 67 occurrences under
integration-tests). It is not "various remaining tests" — draining it is bounded, countable work.

The inversion audit

A strengthened assertion is worth the diff only if it fails when the behaviour it claims is broken.
Three transient production mutations were applied, the affected suites run, and the mutations
reverted — each targeting a different strengthening technique:

Transient mutation	Observed
`PassthroughHostGuardStage.normalize` strips any `:suffix` rather than only a numeric port	the three colon rows failed with `PASSTHROUGH_HOST_SMUGGLED`; the other rows and every attribution control correctly stayed green
`TlsEdgeProducer.onStartup` returns before creating the front listener	`startsAndStopsFrontListener` failed: expected `<true>` but was `<false>`
`TokenValidatorProducer.toValidationIssuer` drops the `audienceValidationDisabled(true)` opt-out	`audienceLessIssuerDisablesAudienceValidation` failed with the exact library refusal the opt-out branch exists to prevent

Each mutation is invisible to the pre-change form of the same tests — all of them stay green
under all three mutations. That contrast is the point of the sweep.

D2 — All four carried PLAN-05 Sonar findings, disposed

"Still open" was not a permitted outcome. All four are fixed in code, none suppressed, none
marked won't-fix/false-positive in the Sonar UI:

Finding	Location	Disposition
`java:S3776` cognitive complexity	`ClientHelloSniParser.parse`	Fixed in production code — the method decomposed so the construct no longer trips the rule
`java:S7467` unnamed pattern	`ClientHelloSniParser` catch clause	Fixed in production code — unnamed pattern `_` applied
`java:S5976` (triple 1)	`PassthroughHostGuardStageTest`	Fixed inside D1's strengthening pass — parameterized and strengthened
`java:S5976` (triple 2)	`PassthroughHostGuardStageTest`	Fixed inside D1's strengthening pass — parameterized and strengthened

The two java:S5976 rows required both operations. Parameterizing alone would have closed the
Sonar rule while leaving all six methods vacuous under D1's own taxonomy — the gate would go green
and the rows would look handled. That is precisely the failure mode this sweep exists to catch, so
parameterize-and-strengthen was treated as one indivisible fix.

The live gate was read first (a code check alone cannot prove gate state). Both ClientHelloSniParser
line anchors carried by the source premises were off by one (parse at 80 not 79; the catch at 171
not 170) — the constructs were present and the findings were not stale; only the line numbers had
drifted.

Four ADDITIONAL live-gate findings — logged, deliberately NOT fixed

Reading the live gate surfaced four findings beyond the four carried ones. All four sit outside this
plan's write boundary and are reported here rather than absorbed; absorbing them would have been
a scope breach, not a judgement call:

Finding	Location
`java:S6539`	`RouteTableBuilder:80`
`java:S5738`	`ConfigFailFastTest:59`
`java:S5738`	`ConfigProducerTest:179`
`java:S135`	`BackchannelLogoutEndpoint:137`

D3 — The two undetermined public API elements, per-element verdicts

Both branches (remove / keep-with-recorded-rationale) were live. They resolved differently:

FramingGate() (the no-arg constructor) — REMOVED.
Zero production consumers; its sole caller was its own test. Deleted outright per the pre-1.0 rule
(remove, never deprecate). FramingGateTest was updated to the boot-resolved constructor, and the
module compiling with the no-arg form gone is itself the proof that no consumer survived.

SealedSessionCookieCodec.Unsealed — KEPT, with the rationale recorded in-source.
It is load-bearing: it is the payload wrapper of the return type of the public
Optional<Unsealed> unseal(String), consumed via the method reference codec::unseal inside
CookieSessionBinding.resolve() (lines 110–113). Two facts are now written into its Javadoc so a
future sweep cannot re-open the question from the same wrong evidence:

the production call site uses the method-reference form, so a call-form-only search reports it
as unconsumed (see the methodology note below);
the Optional<Unsealed> return is not PLAN-36 residue — ADR-0033 retires Optional only from
stored positions (fields, declared parameters, record components); a computed method return
is explicitly sanctioned.

D4 — The carried `.plan/marshal.json` configuration change

The pending configuration change is carried into this PR as committed hunks on this branch. Two
caveats, both load-bearing for anyone reviewing or later diffing this file:

(a) main's working-tree copy of .plan/marshal.json was MUTATED MID-RUN.
This branch deliberately ships the lessons-capture lane as "off" — the operator's init-time
intent, captured verbatim at plan-init as blob 60ed916. Meanwhile main's current on-disk
working-tree value reads "minimal". Anyone diffing this branch against main's working tree later
would otherwise read that difference as a defect introduced by this PR. It is not: the branch value
is the intended one, and the divergence is an artefact of the mid-run mutation of the main checkout.

(b) provisioned_version ships verbatim at 0.1.1288 and is STALE on arrival.
The installed bundle is 0.1.1292. The value is carried verbatim by operator ruling; re-deriving
it would mean running marshall-steward, which was explicitly out of this plan's scope. It is
recorded here so the staleness reads as a known, deliberate carry rather than an oversight.

Methodology note — a census on `name(` misses `::name`

This generalises well beyond this PR and is now a standing rule for the corpus.

Every symbol-reachability census must search both the call form name( and the
method-reference form ::name before any "unused" / "unreached" verdict is recorded.

This bit concretely during this plan. A literal unseal( search reported no production consumer
for SealedSessionCookieCodec.unseal, while the real production call site is codec::unseal inside
CookieSessionBinding.resolve(). Acting on that census would have removed a live public symbol —
the exact apparent-vs-real failure this sweep is about, turned on the sweep's own method. The
constraint is written into the note as load-bearing, not as an aside.

Reported, NOT fixed — two production defects outside the write boundary

This sweep's production write boundary is exactly three files
(ClientHelloSniParser.java, SealedSessionCookieCodec.java, FramingGate.java). Two genuine
production defects were found outside it. Absorbing either would have been a scope breach, so
both are reported here and in the note:

1. EgressPolicy.equals does not carry the host allowlist.
An EgressPolicy built with allowedEgressHost("localhost") compares equal to
EgressPolicy.secureDefault() — both render as EgressPolicy(allowLoopback=false). The allowlist is
invisible to equals/toString. Consequence: assertEquals(EgressPolicy.secureDefault(), …) cannot
detect an unintended widening, and the inverse assertion cannot detect a dropped allowlist at all.
The type is third-party (de.cuioss.sheriff.token.commons.transport). The strengthened row therefore
asserts behaviourally via policy.check(uri) rather than by equality.

2. TokenValidator exposes no view of its issuer configs.
Its public surface is createAccessToken/createIdToken/createRefreshToken, close,
getSecurityEventCounter, getPerformanceMonitor — there is no accessor for the IssuerConfig list
it was built from. The producer's audience decision (expected audience vs the explicit opt-out) is
therefore structurally unobservable on the built validator. That is why the strengthened
TokenValidatorProducerTest pair asserts the audience posture behaviourally, by validating a
real generated token over an offline file JWKS source, rather than structurally.

Production write boundary held. D1 modified no production file at all.

Test Plan

Module tests: test -pl api-sheriff -am — green
Quality gate: verify -Ppre-commit — zero errors/warnings
Full verify: verify — green
Inversion audit: three transient production mutations each turned the intended strengthened
assertion red, and were reverted (see D1 above)
ClientHelloSniParserTest passes unmodified, proving the java:S3776 refactor is
behaviour-preserving
Post-change re-census run over both annotations under complete coverage (300 files, 0
unreadable, 0 elided): declared surface 1368 → 1364

Related Issues

None.

Intent

The problem. A large share of this corpus asserts nothing that can fail. A test that calls
production code and asserts only assertDoesNotThrow(...) or assertNotNull(result) reports as
coverage while proving almost nothing: it stays green when the behaviour its @DisplayName promises
is deleted, inverted, or never implemented. The highest-risk shape is a test named or documented
for a behaviour its assertions do not exercise — the name reads as a specification and the green
result reads as its proof. The corpus is large enough that "we read everything and it looked fine" is
not a claim anyone can check.

The chosen approach, and why that shape. Census-then-targeted-read, not a linear 135-file pass. A
linear pass over a corpus this size degrades into spot-checks — which would reproduce the exact
apparent-vs-real-coverage failure this sweep exists to catch. So: mechanical censuses with published,
reproducible counts; a targeted read of 100% of the ranked candidates plus a control sample of the
un-ranked remainder to test whether the ranking actually selected the vacuous ones; one terminal
verdict per flagged method with a file:line citation; a cap stated before selection; and the
un-covered remainder recorded as a countable backlog with every file named, rather than as an
anecdote. A keep verdict must be attributable — a matched negative control or a

[Intent truncated — 1396 of 2280 characters shown; full outline in the plan workspace]

Summary by CodeRabbit

Security & Reliability
- Improved validation for TLS handshakes, request framing, host headers, route paths, and body-size limits.
- Strengthened token audience validation and preserved secure network-access defaults.
- Avoided unnecessary token-validator initialization on unauthenticated routes.
- Improved runtime assembly and listener startup/shutdown verification.
Configuration
- Updated workflow provisioning and finalized step behavior.
- GET requests with content-length bodies now require explicit enablement.
Documentation
- Added contributor guidance and findings on test coverage and assertion integrity.

Census the 135-file / 1364-method API Sheriff test corpus for assertions that cannot support the behaviour their names claim, publish the full classification as a checked-in note, and strengthen a capped subset highest-risk-first. Note (new): doc/development/test-corpus-integrity.adoc, indexed from doc/development/README.adoc. It carries the marker censuses with their complete-coverage evidence, the four vacuity shapes and three verdicts, the two selection gates and every higher-density file they excluded, one verdict row per flagged method with a file:line citation, and the standing backlog counted as 43 files carrying 140 marker occurrences. Strengthened: 18 methods (8 mandatory + 10 discretionary) against a cap of 20. The six BenignPassThrough tests in PassthroughHostGuardStageTest are both parameterized (closing java:S5976) and strengthened, since parameterizing alone would turn Sonar green while leaving all six vacuous. One test is deleted as strictly subsumed by a stronger sibling. Findings recorded in the note: marker density is a screen, not a verdict (three high-density files yielded zero strengthenings); EgressPolicy equality does not carry the host allowlist; TokenValidator exposes no view of its issuer configs. No production file is modified by this deliverable. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NJ5pHcAVTxjFJneB7cyke8

Three transient production mutations, one per strengthening technique, were applied together and reverted after the run. Each broke exactly the strengthened assertions it targeted, and none is visible to the pre-change form of the same test - which is the contrast the sweep exists to establish. The audit ran after the deliverable was committed so the revert is a checkout against committed content rather than a destructive discard of uncommitted work. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NJ5pHcAVTxjFJneB7cyke8

Read the authoritative cuioss SonarCloud gate BEFORE any code change. All four PLAN-05 findings were genuinely open, with the re-anchored line numbers exact: java:S3776 at ClientHelloSniParser.java:80 (cognitive complexity 21 > 15), java:S7467 at :171, and the two java:S5976 rows at PassthroughHostGuardStageTest.java:116 and :146. None had been resolved without a code change, so there is no silent-dismissal finding to report. java:S3776 - the while(true) reassembly loop interleaved four concerns. Each is now a named private helper returning the terminal Result it implies, or null to consume the record: recordHeaderVerdict, recordBodyVerdict, reassembledVerdict. parse() reads as the reassembly loop it is. The duplicated tail check after an INCOMPLETE span is removed because it restates the loop head's own first check, so looping back is byte-for-byte equivalent. Every fail-closed Result.parsed(null) path survives unchanged. java:S7467 - the unused catch binding becomes the unnamed variable. Structure-only: ClientHelloSniParserTest passes with ZERO edits, which is the behaviour-preservation harness this deliverable is verified by. The two S5976 rows were closed by deliverable 1's parameterize-and-strengthen pass. Four further live-gate findings are open and outside this plan's scope; they are reported in decision.log, not absorbed: java:S6539 RouteTableBuilder.java:80, java:S5738 ConfigFailFastTest.java:59, java:S5738 ConfigProducerTest.java:179, java:S135 BackchannelLogoutEndpoint.java:137. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NJ5pHcAVTxjFJneB7cyke8

Both consumer sweeps were re-confirmed against the current tree searching BOTH the call form and the ::name method-reference form, which is what decides them in opposite directions. no-arg FramingGate() - REMOVED. Sweeping `new FramingGate()` together with `FramingGate::` over 926 files finds exactly one consumer, FramingGateTest:40, and no production one; the only production instantiation is the boolean constructor at GatewayEdgeRoute. All four clean-break conditions hold, so under the plan-level breaking compatibility and the pre-1.0 remove-never-deprecate rule it goes directly, with no deprecation marker and no transitionary comment. The single call site becomes new FramingGate(false), which is value-identical: the removed constructor delegated to this(false), so the strict default posture is preserved exactly and no assertion changes. SealedSessionCookieCodec.Unsealed - KEPT, with the reason recorded in its Javadoc. It is the payload wrapper of unseal()'s return, consumed in production by the readSealedValue(...).flatMap(codec::unseal).filter(...).map(...) chain in CookieSessionBinding.resolve. That site is the METHOD-REFERENCE form, so a call-form-only search reports this API as unused - a false verdict that would have justified an unsafe removal. The Javadoc now records both that consumer chain and the ADR-0033 computed-vs-stored Optional distinction, so a future sweep does not re-flag Optional<Unsealed> as PLAN-36 residue. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NJ5pHcAVTxjFJneB7cyke8

The phase-5 worktree is cut from main's committed HEAD, so it does not inherit main's uncommitted working-tree changes. This re-applies the pending .plan/marshal.json edit inside the worktree so it ships rather than being silently lost. Carried as a patch (git diff --output, then git apply) from the main checkout, never hand-retyped, and the main checkout was not written to. Three hunks: - default:lessons-capture and default:finalize-step-preference-emitter are relocated to sit after default:branch-cleanup. A canonical key re-ordering with byte-identical values, semantically a no-op. - provisioned_version 0.1.1286 -> 0.1.1288. REPORTED, not corrected: provisioned_version 0.1.1288 is already stale against the installed plan-marshall bundle 0.1.1292. Re-deriving it would mean running marshall-steward, which is outside this plan's scope, so it is carried verbatim. REPORTED, needs operator confirmation: the operator-supplied backup of this diff carried default:lessons-capture at lane "off", while the main checkout's current working tree carries it at lane "minimal" - main drifted after that backup was taken. D4's success criteria name the main checkout as the source of truth, so "minimal" is what shipped here. If "off" was intended, this hunk is wrong and must be corrected before merge. See decision.log. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NJ5pHcAVTxjFJneB7cyke8

…g change Corrects the one hunk of the D4 carry that was taken from a rewritten source. The carry in 9907dc0 read .plan/marshal.json from the main checkout, which D4's success criteria name as the source of truth, and shipped default:lessons-capture at lane "minimal". Main's working tree had been REWRITTEN mid-run: the orchestrator's init-time capture of the same pending change is blob 474e4cc..60ed916 and carries lane "off", while main now shows 474e4cc..a84f140 with "minimal". The plan's own handshake rows corroborate a mid-run config write (config_hash 93acf2ec -> 6951111f -> 577b902 -> e8e8b3e). "Matches main" was therefore not the same as "matches the operator's change". The operator's init-time intent is authoritative, so lane "off" is what ships. Verification is exact rather than asserted: after this change .plan/marshal.json hashes to blob 60ed916 - byte-identical to the post-image of the init-time capture. Applied through the manage-config step setter, so no JSON was hand-retyped and no other key was touched; the diff is one line. Everything else in the D4 carry is unchanged: the block relocation, finalize-step-preference-emitter at preference_min_recurrence 2 / lane minimal, and provisioned_version verbatim at 0.1.1288. No marshall-steward run. REPORTED, unchanged: provisioned_version 0.1.1288 is stale against the installed plan-marshall bundle 0.1.1292; re-deriving it is out of scope. REPORTED, new: main's .plan/marshal.json was mutated mid-run, so this branch deliberately differs from main's CURRENT on-disk value for lessons-capture.lane. Anyone diffing the branch against main's working tree later would otherwise read that difference as an error in this PR. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NJ5pHcAVTxjFJneB7cyke8

@SuppressWarnings

…nert NOSONAR marker Post-implementation simplification sweep over the plan's changeset. TokenValidatorProducerTest: allowlistSurvivesTheProducerPath carried two near-identical ~13-line builder blocks differing only by the .allowedEgressHosts(...) call. Collapsed into a producerPathEgressPolicy helper alongside the existing egressPolicyFor. Both assertions and every explanatory comment are preserved verbatim; no assertion was weakened. TlsEdgeProducerTest: awaitNotListening carried two suppression mechanisms for java:S2925 — an inert "// NOSONAR java:S2925" on a standalone comment line (NOSONAR only suppresses on the line it sits on, and the Thread.sleep is six lines below) plus the @SuppressWarnings that actually suppresses. Dropped the inert marker, kept the rationale prose and the annotation. Two anti-patterns were reported rather than fixed, both out of scope: a naming shadow at ClientHelloSniParser.java:113 (a rename would re-open D2's behaviour-preserving, zero-test-edit constraint for cosmetic churn), and pre-existing five-argument call duplication in RouteRuntimeAssemblerTest that predates this plan. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NJ5pHcAVTxjFJneB7cyke8

sourcery-ai

Sorry @cuioss-oliver, you have reached your weekly rate limit of 500000 diff characters.

Please try again later or upgrade to continue using Sourcery

coderabbitai · 2026-08-04T00:05:34Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository: cuioss/coderabbit/.coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 8a83894d-b0c5-4f44-8730-f6ae30f1819b

📥 Commits

Reviewing files that changed from the base of the PR and between f802043 and 5742019.

📒 Files selected for processing (4)

api-sheriff/src/main/java/de/cuioss/sheriff/gateway/tls/ClientHelloSniParser.java
api-sheriff/src/test/java/de/cuioss/sheriff/gateway/edge/RouteRuntimeAssemblerTest.java
api-sheriff/src/test/java/de/cuioss/sheriff/gateway/tls/ClientHelloSniParserTest.java
api-sheriff/src/test/java/de/cuioss/sheriff/gateway/tls/TlsEdgeProducerTest.java

🚧 Files skipped from review as they are similar to previous changes (3)

api-sheriff/src/test/java/de/cuioss/sheriff/gateway/edge/RouteRuntimeAssemblerTest.java
api-sheriff/src/test/java/de/cuioss/sheriff/gateway/tls/TlsEdgeProducerTest.java
api-sheriff/src/main/java/de/cuioss/sheriff/gateway/tls/ClientHelloSniParser.java

📝 Walkthrough

Walkthrough

The change refactors TLS validation, removes the default FramingGate constructor, strengthens authentication, pipeline, runtime, and TLS tests, adds test-integrity documentation, and updates finalize workflow metadata.

Changes

Gateway validation and test integrity

Layer / File(s)	Summary
TLS ClientHello validation `api-sheriff/src/main/java/.../ClientHelloSniParser.java`, `api-sheriff/src/test/java/.../ClientHelloSniParserTest.java`	TLS record and handshake checks use dedicated helpers. Tests cover truncated headers and maximum-buffer boundaries.
Authentication and validator assembly tests `api-sheriff/src/test/java/.../auth/*`	Tests cover lazy validator resolution, audience enforcement, offline JWKS validation, and egress allowlist propagation.
Pipeline contracts and checks `api-sheriff/src/main/java/.../FramingGate.java`, `api-sheriff/src/test/java/.../pipeline/*`	`FramingGate` requires an explicit option. Pipeline tests strengthen path, Host, configuration reuse, and body-size assertions.
Runtime startup and listener tests `api-sheriff/src/test/java/.../{edge,quarkus,tls}/*`	Tests validate assembled routes, deferred discovery, eager startup, and TLS listener lifecycle behavior.
Test-integrity documentation `api-sheriff/src/main/java/.../SealedSessionCookieCodec.java`, `doc/development/*`	Documentation describes `Unsealed` usage and records test-corpus assertion and vacuity analysis.

Workflow metadata

Layer / File(s)	Summary
Finalize workflow configuration `.plan/marshal.json`	Finalize workflow steps were relocated. Lessons capture uses the `off` lane, preference emission uses the minimal lane, and the provisioned version is `0.1.1288`.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Possibly related PRs

cuioss/API-Sheriff#71: Both changes address TLS SNI fail-closed behavior and request-framing controls.
cuioss/API-Sheriff#131: Both changes strengthen body-size enforcement tests.
cuioss/API-Sheriff#146: Both changes modify FramingGate construction and GET body handling configuration.

🚥 Pre-merge checks | ✅ 2

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly summarizes the main changes: the test-corpus sweep for vacuous assertions and resolution of carried findings.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands.}

cuioss-review-bot · 2026-08-04T00:06:13Z

PR Reviewer Guide 🔍

(Review updated until commit `5742019`)

🧪 PR contains tests
🔒 No security concerns identified
⚡ No major issues detected

coderabbitai

Actionable comments posted: 2

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository: cuioss/coderabbit/.coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 0fb87f82-8542-448b-9476-b7bda3bc1c12

📥 Commits

Reviewing files that changed from the base of the PR and between ec6f7e4 and f802043.

📒 Files selected for processing (16)

.plan/marshal.json
api-sheriff/src/main/java/de/cuioss/sheriff/gateway/bff/cookie/SealedSessionCookieCodec.java
api-sheriff/src/main/java/de/cuioss/sheriff/gateway/pipeline/FramingGate.java
api-sheriff/src/main/java/de/cuioss/sheriff/gateway/tls/ClientHelloSniParser.java
api-sheriff/src/test/java/de/cuioss/sheriff/gateway/auth/AuthenticationStageTest.java
api-sheriff/src/test/java/de/cuioss/sheriff/gateway/auth/TokenValidatorProducerTest.java
api-sheriff/src/test/java/de/cuioss/sheriff/gateway/edge/RouteRuntimeAssemblerTest.java
api-sheriff/src/test/java/de/cuioss/sheriff/gateway/pipeline/BasicChecksStageTest.java
api-sheriff/src/test/java/de/cuioss/sheriff/gateway/pipeline/FramingGateTest.java
api-sheriff/src/test/java/de/cuioss/sheriff/gateway/pipeline/PassthroughHostGuardStageTest.java
api-sheriff/src/test/java/de/cuioss/sheriff/gateway/pipeline/ThoroughChecksStageTest.java
api-sheriff/src/test/java/de/cuioss/sheriff/gateway/quarkus/BffRuntimeProducerTest.java
api-sheriff/src/test/java/de/cuioss/sheriff/gateway/quarkus/ConfigProducerTest.java
api-sheriff/src/test/java/de/cuioss/sheriff/gateway/tls/TlsEdgeProducerTest.java
doc/development/README.adoc
doc/development/test-corpus-integrity.adoc

💤 Files with no reviewable changes (2)

api-sheriff/src/main/java/de/cuioss/sheriff/gateway/pipeline/FramingGate.java
api-sheriff/src/test/java/de/cuioss/sheriff/gateway/auth/AuthenticationStageTest.java

The cuioss SonarCloud gate failed at f802043 with three new-code findings. All three are closed here; the bug-class one was analysed before it was touched. javabugs:S6466 (ClientHelloSniParser.java:253, BUG) - re-derived every call path into uint16 and uint24 rather than trusting the earlier "the header guard already returned null" claim. Three call sites exist and every one is guarded: * parse() reaches uint16(bytes, pos + 3) only after recordHeaderVerdict returned null, which happens solely when bytes.length - pos >= RECORD_HEADER_LENGTH, so pos + 4 <= bytes.length - 1. pos starts at 0 and only advances to a recordEnd that recordBodyVerdict already bounded by MAX_CLIENT_HELLO_BYTES, so it is never negative and cannot overflow. * Cursor.readUint16() reaches uint16(data, position) only after require(2) asserted position + 2 <= data.length, raising MalformedHelloException otherwise. position starts at HANDSHAKE_HEADER_LENGTH and never decreases - seek() refuses a backwards target - so it is never negative. * completeHandshake() reaches uint24(handshake, 1) only after returning INCOMPLETE for handshake.length < HANDSHAKE_HEADER_LENGTH, so offsets 1..3 are always inside the buffer. The out-of-bounds access is therefore unreachable: no defect shipped and no behavioural fix is warranted. What the S3776 split did change is where the guards live - they moved from inline in the reassembly loop into named helpers, which is exactly what put them out of reach of the symbolic-execution engine. Suppressed in-code with // NOSONAR javabugs:S6466 on the two flagged reads, with the full per-call-site bounds contract written up as Javadoc on uint16 (and the matching one-liner on uint24) so a reviewer can challenge the claim rather than take it. Two regression tests turn that prose into executable assertions, both pinning the guard at the exact one-byte-short-of-a-record-header boundary - index 4 present in neither case, which is the precise read Sonar flagged: one on the first record (pos == 0) and one on the loop-back to a later record (pos > 0), the tail check the S3776 split delegated to the loop head. Remove the length guard and both throw ArrayIndexOutOfBoundsException instead of asking for more data. java:S7467 (TlsEdgeProducerTest.java:201, :213) - both catch bindings are unused, so both become the unnamed pattern. The refused-connection branch keeps its meaning in a comment now that the binding name no longer carries it. No assertion was weakened; these are the D1-strengthened tests and their strength is untouched. Both gates green with zero errors: verify -Ppre-commit and verify. Neither left any rewrite churn outside the three-file footprint. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

cuioss-oliver · 2026-08-04T01:22:10Z

Triage dispositions

In reply to comment_id: `PRR_kwDOPatrT88AAAABIQ154Q`

Review summary acknowledged — no separate action required. The body carries run configuration, the reviewed commit range (ec6f7e4..f802043) and the 16-file selection list, plus the count of actionable comments; it raises no code request of its own. Both actionable comments it announces were triaged individually and both were accepted as genuine: 28625f (ClientHelloSniParser bound-guard predicate) is addressed by TASK-008 and 094c4b (forced-H2 assertion in RouteRuntimeAssemblerTest) by TASK-009, each answered on its own thread. The two entries under 'Files with no reviewable changes' are expected: FramingGate.java and AuthenticationStageTest.java carry only removals in this PR (the unused no-arg FramingGate() constructor and an assertion-strengthening edit), which is the intended shape of the change.

…ount PR 151 CodeRabbit inline comment 28625f on ClientHelloSniParser.java:119. recordHeaderVerdict decided "keep buffering vs fail closed" with overBound(pos), where pos counts only the record bytes already CONSUMED. The invariant MAX_CLIENT_HELLO_BYTES declares is on the ACCUMULATED buffer: "the hard upper bound on the bytes buffered while reassembling a single ClientHello ... anything larger is treated as complete-with-no-SNI (fail closed) rather than buffered further". Those two are not the same number. recordBodyVerdict admits a record whose recordEnd equals MAX_CLIENT_HELLO_BYTES, so pos can advance to MAX - 1. The header branch fires while bytes.length - pos < RECORD_HEADER_LENGTH, so bytes.length can reach pos + 4 = MAX + 3 with overBound(pos) still false and parse() still returning needMoreData(). The caller was therefore told to keep buffering a buffer that had already passed the declared hard bound. The overshoot is self-terminating - one more byte completes the record header and recordBodyVerdict fails closed - so the class-level "buffering can never grow unbounded" claim always held. What did not hold is the constant's own contract, and a guard whose predicate does not match the scenario it exists for is a defect whether or not the blast radius is currently bounded. Fixed by anchoring the test on overBound(bytes.length). Since pos <= bytes.length always holds (pos only ever advances to a recordEnd that recordBodyVerdict has already confirmed is within the buffer), the buffer-anchored test subsumes the position-anchored one: every input that used to fail closed still does, and the 1..3 byte overshoot window now fails closed too. The rationale is written up on recordHeaderVerdict so a reviewer can challenge it rather than take it. This is NOT the javabugs:S6466 finding closed earlier in this PR. That one was about a uint16 read running past the array and was closed UNREACHABLE with a per-call-site bounds contract; index safety is untouched here. This is the SIZE bound, a distinct concern on the same method. Two regression tests pin the guard as a matched pair, both built from one fixture that puts an incomplete handshake record ahead of a partial next-record header - the shape that separates the two candidate anchors, because the bytes consumed stop at the first record's end while the bytes buffered run on: * a buffer of exactly MAX_CLIENT_HELLO_BYTES fails closed, though only MAX - 1 bytes have been consumed. This is the case the old anchor got wrong; it fails against the previous guard. * the identical shape one byte smaller still asks for more data, pinning the give-up point to MAX_CLIENT_HELLO_BYTES exactly so the assertion above cannot be satisfied by a guard that gives up early on every fragmented ClientHello. The pair is what makes either test meaningful: they differ by one buffered byte and disagree on the verdict, while the consumed position is under the bound in both - precisely the distinction the old predicate could not draw. Both gates green with zero errors: verify -Ppre-commit and verify. Neither left any rewrite churn outside the footprint. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NJ5pHcAVTxjFJneB7cyke8

PR 151 CodeRabbit inline comment 094c4b on RouteRuntimeAssemblerTest.java:194. The gRPC leg of shouldAssembleSessionRoutes ended on assertNotNull(grpc.getHttpClient(), "a gRPC route carries the forced-h2 upstream client") The message claims forced-h2; the assertion only proved non-null. The test's client factory is clientFactory = _ -> vertx.createHttpClient(); which DISCARDS its UpstreamTarget, so the leg passed identically whether or not the assembler asked for a forced-h2 client. The only thing that could have failed it is the factory returning null, which it never does. That is exactly the weak-assertion class this plan exists to remove, reintroduced by the very hunk that replaced the leg's original assertDoesNotThrow - so it is in scope and authored by this PR, not pre-existing. RouteRuntimeAssembler builds UpstreamTarget.of(resolvedUpstream, route.protocol() == Protocol.GRPC) at line 159, so forcedHttp2 is the observable that proves the gRPC branch ran. A capturing UpstreamClientFactory records the target the assembler hands it, which is the direct, non-reflective way to assert on the factory's INPUT rather than on its never-null output. The gRPC leg now asserts exactly one client was resolved and that its target carries forcedHttp2(). The WebSocket leg becomes the matched negative control: the identical capture over a non-gRPC route asserts forcedHttp2() is false. Without it the new assertion would be only half-pinned - assertTrue on a flag would still pass if the flag were hardcoded true, or if forced-h2 were requested for every protocol. The two legs assert opposite values on the same field through the same capture path, so neither can be vacuous. Nothing was weakened or deleted: the original assertNotNull and both existing assertEquals assertions on the WebSocket leg are preserved verbatim, with the new assertions added alongside them. Both gates green with zero errors: verify -Ppre-commit and verify. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NJ5pHcAVTxjFJneB7cyke8

cuioss-oliver · 2026-08-04T02:17:07Z

/review

cuioss-oliver and others added 7 commits August 4, 2026 00:37

sourcery-ai Bot reviewed Aug 4, 2026

View reviewed changes

coderabbitai Bot reviewed Aug 4, 2026

View reviewed changes

Comment thread api-sheriff/src/main/java/de/cuioss/sheriff/gateway/tls/ClientHelloSniParser.java

Comment thread api-sheriff/src/test/java/de/cuioss/sheriff/gateway/edge/RouteRuntimeAssemblerTest.java Outdated

cuioss-oliver and others added 2 commits August 4, 2026 03:42

cuioss-oliver added this pull request to the merge queue Aug 4, 2026

Merged via the queue into main with commit b39b271 Aug 4, 2026
26 checks passed

cuioss-oliver deleted the feature/plan-43-test-corpus-integrity branch August 4, 2026 02:37

Navigation Menu

Uh oh!

Uh oh!

test(corpus): sweep for vacuous assertions and settle carried findings - #151

test(corpus): sweep for vacuous assertions and settle carried findings#151
cuioss-oliver merged 10 commits into
mainfrom
feature/plan-43-test-corpus-integrity

cuioss-oliver commented Aug 4, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

sourcery-ai Bot left a comment

Uh oh!

coderabbitai Bot commented Aug 4, 2026 •

edited

Loading

Walkthrough

Changes

Uh oh!

cuioss-review-bot Bot commented Aug 4, 2026 •

edited

Loading

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

Uh oh!

cuioss-oliver commented Aug 4, 2026

Uh oh!

cuioss-oliver commented Aug 4, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Uh oh!

Conversation

cuioss-oliver commented Aug 4, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Changes

D1 — Corpus sweep, classification, and the capped strengthening

Declared surface (the denominator)

The stated cap, and why it was deliberately under-filled

Classification — one row per flagged method, exactly one verdict each

The countable backlog

The inversion audit

D2 — All four carried PLAN-05 Sonar findings, disposed

Four ADDITIONAL live-gate findings — logged, deliberately NOT fixed

D3 — The two undetermined public API elements, per-element verdicts

D4 — The carried .plan/marshal.json configuration change

Methodology note — a census on name( misses ::name

Reported, NOT fixed — two production defects outside the write boundary

Test Plan

Related Issues

Intent

Summary by CodeRabbit

Uh oh!

sourcery-ai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

coderabbitai Bot commented Aug 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Uh oh!

cuioss-review-bot Bot commented Aug 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Reviewer Guide 🔍

(Review updated until commit 5742019)

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

cuioss-oliver commented Aug 4, 2026

Triage dispositions

In reply to comment_id: PRR_kwDOPatrT88AAAABIQ154Q

Uh oh!

cuioss-oliver commented Aug 4, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

cuioss-oliver commented Aug 4, 2026 •

edited by coderabbitai Bot

Loading

D4 — The carried `.plan/marshal.json` configuration change

Methodology note — a census on `name(` misses `::name`

coderabbitai Bot commented Aug 4, 2026 •

edited

Loading

cuioss-review-bot Bot commented Aug 4, 2026 •

edited

Loading

(Review updated until commit `5742019`)

In reply to comment_id: `PRR_kwDOPatrT88AAAABIQ154Q`