fix(test): loopback-bind test listeners and land socket instrumentation - #255
Open
cuioss-oliver wants to merge 38 commits into
Open
fix(test): loopback-bind test listeners and land socket instrumentation#255cuioss-oliver wants to merge 38 commits into
cuioss-oliver wants to merge 38 commits into
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @cuioss-oliver, you've used your own review budget of 250,000 diff characters for the last 7 days.
You can request another review in 4 days and 17 hours by commenting @sourcery-ai review. Upgrade to get a review now.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe test suite now uses explicit loopback bindings, captures socket state in timeout diagnostics, and enforces bind usage with ArchUnit. Documentation records the measured macOS mechanism. Review-plan metadata replaces one required review bot. ChangesLoopback bind policy
Review plan metadata
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: 🔵 Low · up to The bind fix is otherwise ready, but add coverage for all documented wildcard-host forms and clarify Linux socket output so future guard regressions and timeout investigations remain reliable. 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
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. Comment |
PR Reviewer Guide 🔍(Review updated until commit e32394a)
|
Collaborator
Author
|
@coderabbitai review |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: cuioss/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Team
Run ID: 73d9a70f-310e-4f4e-932f-0c8af6be0d89
📒 Files selected for processing (20)
.plan/marshal.json.plan/project-architecture/_project.jsonapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/arch/LoopbackEphemeralBindArchTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/arch/specimen/LoopbackEphemeralBindSpecimen.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/arch/specimen/WildcardEphemeralBindSpecimen.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/edge/GatewayEdgePipelineTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/edge/GatewayEdgeRouteBffWiringTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/edge/GatewayEdgeRouteTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/edge/GrpcDispatchStageTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/edge/GrpcStatusMapperTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/edge/ReservedBodyCeilingTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/edge/WebSocketRelayStageTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/testsupport/Awaits.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/testsupport/AwaitsTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/testsupport/LoopbackHost.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/testsupport/SocketSnapshot.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/tls/PassthroughRelayTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/tls/SniFrontListenerTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/tls/TlsEdgeProducerTest.javadoc/development/build-gate-discipline.adoc
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.
|
cuioss-oliver
added a commit
that referenced
this pull request
Sep 3, 2026
`listen(0, "0.0.0.0")` and `listen(0, LoopbackHost.ADDRESS)` are the same call target — same name, same parameter types — so the ArchUnit rule accepted both while the first rebuilds exactly the wildcard ephemeral bind the guard exists to refuse. The discriminator is the argument's text, which lives in the source rather than the bytecode, so a wider ArchUnit rule cannot reach it. Adds a source sweep for the three wildcard host spellings (0.0.0.0, ::, empty) with its own matched controls: a positive control asserting the sweep finds the specimen's deliberate literal, a negative control asserting it leaves the loopback-bound spelling alone, and a non-vacuity check on the scanned source set. Records the bytecode rule's structural limit in its Javadoc so its green is not read as more than it proves. Falsifiability demonstrated: reverting one guarded call site to `listen(0, "0.0.0.0")` failed the sweep with exactly one violation naming file and line, while the other eight probes stayed green. Reported by CodeRabbit on PR #255. Co-Authored-By: plan-marshall <noreply@cuioss.de>
cuioss-oliver
added a commit
that referenced
this pull request
Sep 3, 2026
… section `SocketSnapshot.available()` proves only that both binaries are executable, and `capture()` renders their output into one section. The probe scanned the whole section, so an `lsof` address satisfied it even when `netstat` contributed no matching row — the probe would pass on a half-degraded capture while claiming the OS snapshot saw the socket. Splits the assertion per tool. `SocketSnapshot` now owns the two sub-section banners as constants and renders from them, so a probe that narrows a message to one tool's rows cannot drift from the rendering it narrows. No degradation carve-out is needed: the degraded netstat rendering names its ports as a bare set rather than as address tokens, and the probe matches only address tokens, so it does not mistake the degradation note for a real row. Reported by CodeRabbit on PR #255. Co-Authored-By: plan-marshall <noreply@cuioss.de>
Collaborator
Author
|
/review |
Replace 20 bare `.listen(0)` wildcard binds across the nine live-socket fixtures with `.listen(0, LoopbackHost.ADDRESS)`, and route the 24 `127.0.0.1` literals in those same files through that one new shared constant (`testsupport/LoopbackHost`). A wildcard ephemeral bind that is later dialled on loopback is the measured exposure this closes; every dial target in these fixtures was already `127.0.0.1`, so binding the listener to loopback is behaviour-preserving rather than a semantic change. The constant is deliberately not named `Test*` (Surefire's default includes match `**/Test*.java`) and lives in `testsupport` rather than inside any one feature package. TlsEdgeProducerTest keeps its four `new ServerSocket(0)` wildcard binds and gains comments explaining why. Production `SniFrontListener.start()` binds the wildcard via `netServer.listen(publicPort)`, so a loopback-only holder would stop colliding with it and those controls would silently stop controlling anything. None of the four dials the port it holds. ReservedBodyCeilingTest converts only its three transport dial targets; its eight `localhost` occurrences (the Host: header, the redirect_uri and the Javadoc mentions) are deliberately unchanged, since a drop there is the signature of the blanket replace this change forbids. No file under api-sheriff/src/main/** is modified, and no await ceiling is widened. Co-Authored-By: plan-marshall <noreply@cuioss.de>
A thread dump says what the JVM was doing; it cannot say whether the awaited work ever reached the machine. Every Awaits ceiling now also carries the kernel's own view of this process's TCP sockets, including the per-socket queue depths that separate "the bytes arrived and nobody was told" from "nothing was ever sent". Adds testsupport/SocketSnapshot.java: a diagnostic-only capture over lsof and netstat that degrades to a stated note on every failure path, bounds each child process, and can never throw or fail a test. Wires it into Awaits.timedOut and adds two disjoint matched controls in AwaitsTest — one asserting the section is present at all, one asserting the capture names a loopback socket this JVM actually holds, abstaining rather than failing where the binaries are absent. The three-leg mutation probe was re-run after applying, not assumed: dropping the snapshot reddens exactly the two snapshot probes, dropping the thread dump reddens exactly the three dump-carrying probes, and zeroing the elapsed measurement reddens exactly the three elapsed probes. The legs stay disjoint, so a single red names which mechanism was lost. The degraded path was exercised directly by making netstat unresolvable: the suite stays green with one abstention, confirming available() routes the content probe through its assumption skip while the section header is still emitted so the presence probe holds. CONNECT_CEILING_SECONDS and TEARDOWN_CEILING_SECONDS are unchanged. Co-Authored-By: plan-marshall <noreply@cuioss.de>
…eue reading The stall section asserted a lost macOS kqueue readiness event as the current reading. That inference was refuted on 2026-09-03: `.listen(0)` binds the dual-stack wildcard, macOS lets it coexist with a foreign `127.0.0.1`-specific listener on the same ephemeral port, and BSD most-specific-match then routes the fixture's own client to that other process. Retitle the section around the measured mechanism and carry its four controls with their scopes: the system-wide `lsof` that named the foreign owner at `rxbytes=146 / txbytes=0`, the E1 squat-then-wildcard-bind reproduction, the E2 `BindException` negative control, and the 179-listeners / 20 000-allocation rate pair (1.0925 % predicted, 1.0931 % measured, loopback arm 0). Keep the superseded reading rather than deleting it, since how it failed is the useful part: the dumps were read correctly and the step from "no thread had work" to "a notification was lost" assumed the work had been queued at all. Add the elimination to the 2026-09-01 rules-out list and state why it belongs there rather than in the 2026-09-02 list. Record that the TIME_WAIT listener form was refuted correctly and still left the listener socket looking answered-for. Restate the limit against the new mechanism, keep both power caveats, and keep the three unsettled items unsettled: the 5.036 s TEARDOWN occurrence in `ReservedBodyCeilingTest`, the inference that CI is clean for want of such listeners, and whether the mechanism accounts for every historical occurrence. Update the contributor guidance for the landed fix without claiming closure: the quarter-of-runs figure is pre-fix, no post-fix rate has been measured, and the `SocketSnapshot` block is now the first thing to read on a recurrence. The `-T1` section heading and the `@Nested` `Tests run: 0` CI-log trap are unchanged. Co-Authored-By: plan-marshall <noreply@cuioss.de>
…itness function Binding all 20 sites to loopback fixes the tree as it stands; nothing stopped the next fixture from reintroducing the exposure. `LoopbackEphemeralBindArchTest` does, over the api-sheriff test tree. The rule keys on the overload signature, never the port literal — ArchUnit reads bytecode and cannot see the `0`, but it can see which overload was called, and the bare single-int forms bind the wildcard whatever the port. Both spellings are banned together, `ServerSocket(int)` and `listen(int)`, since a guard against one leaves the other free to reintroduce the identical bind under another name. Phrased positively — `classes().should(...)` emitting `violated` — because under the `no...` form ArchUnit inverts event polarity and the rule would pass vacuously, the same reasoning `NoStoredOptionalArchTest` records. All four ADR-0030 control legs ship with it: - Non-vacuity: the guarded selection resolves to more than zero classes, the specimen package resolves, and at least one selected class is seen calling the host-bound `listen(int, String)` form. That third leg is what makes a zero-violation result mean "no bare bind" rather than "no bind calls scanned". - Negative control: `WildcardEphemeralBindSpecimen` carries both banned spellings and must be rejected, proven with `assertThrows` and `allowEmptyShould(true)` so an unresolved specimen fails loudly instead of passing quietly. - Matched positive controls: `LoopbackEphemeralBindSpecimen` is the host-bound near-miss and must be accepted; production `SniFrontListener` must stay unselected, asserted only after re-establishing that it does still call `listen(int)` — "not selected" is trivially true of a class that stopped matching at all. - Carve-outs: the specimen package, and `TlsEdgeProducerTest` with its nested classes, whose four sites hold or probe a port for production's own wildcard bind and are never dialled. The justification stays recorded at each site. Demonstrated red before being accepted green: with one real fixture site temporarily reverted to `listen(0)`, the rule failed with exactly one violation naming the class, file and line; reverted forward-only, the five tests are green. Co-Authored-By: plan-marshall <noreply@cuioss.de>
The -Ppre-commit gate reformatted the continuation indentation of the loopback-bound listen calls and the client request introduced by deliverable 1. Committed as-authored-by-the-gate so the tree is settled and the next gate run is a no-op. Co-Authored-By: plan-marshall <noreply@cuioss.de>
The -Ppre-commit gate widened the snapshot probe's throws clause to Exception, realigned its try-with-resources continuation, and de-indented the omitted-rows string concatenation in SocketSnapshot. Committed so the tree is settled and the next gate run is a no-op. Co-Authored-By: plan-marshall <noreply@cuioss.de>
The -Ppre-commit gate dropped the redundant test prefix from the rule's method name, flipped three equals calls to constant-first form, and adjusted import spacing in the two specimens. Committed so the tree is settled and the next gate run is a no-op. Co-Authored-By: plan-marshall <noreply@cuioss.de>
… login `required_bots` named `cuioss-review-bot`, which is PR-Agent's GitHub author login. The bot registry keys by `bot_kind`, and that reviewer's declaration in `automatic-review/standards/pr-agent.md` reads `bot_kind: pr-agent` with `author_login: cuioss-review-bot`. No `cuioss-review-bot` bot definition exists, so the token could never be proven a participant: PR-Agent would review the diff, be credited as `pr-agent`, and the quorum would still report it absent — failing identically on every pass and every future PR. Fallout from #249, which correctly renamed the GitHub App identity but carried the rename into this token as well. CLAUDE.md already documents the intended value as `coderabbit,pr-agent`. Co-Authored-By: plan-marshall <noreply@cuioss.de>
`listen(0, "0.0.0.0")` and `listen(0, LoopbackHost.ADDRESS)` are the same call target — same name, same parameter types — so the ArchUnit rule accepted both while the first rebuilds exactly the wildcard ephemeral bind the guard exists to refuse. The discriminator is the argument's text, which lives in the source rather than the bytecode, so a wider ArchUnit rule cannot reach it. Adds a source sweep for the three wildcard host spellings (0.0.0.0, ::, empty) with its own matched controls: a positive control asserting the sweep finds the specimen's deliberate literal, a negative control asserting it leaves the loopback-bound spelling alone, and a non-vacuity check on the scanned source set. Records the bytecode rule's structural limit in its Javadoc so its green is not read as more than it proves. Falsifiability demonstrated: reverting one guarded call site to `listen(0, "0.0.0.0")` failed the sweep with exactly one violation naming file and line, while the other eight probes stayed green. Reported by CodeRabbit on PR #255. Co-Authored-By: plan-marshall <noreply@cuioss.de>
… section `SocketSnapshot.available()` proves only that both binaries are executable, and `capture()` renders their output into one section. The probe scanned the whole section, so an `lsof` address satisfied it even when `netstat` contributed no matching row — the probe would pass on a half-degraded capture while claiming the OS snapshot saw the socket. Splits the assertion per tool. `SocketSnapshot` now owns the two sub-section banners as constants and renders from them, so a probe that narrows a message to one tool's rows cannot drift from the rendering it narrows. No degradation carve-out is needed: the degraded netstat rendering names its ports as a bare set rather than as address tokens, and the probe matches only address tokens, so it does not mistake the degradation note for a real row. Reported by CodeRabbit on PR #255. Co-Authored-By: plan-marshall <noreply@cuioss.de>
The -Ppre-commit gate widened the four new sweep probes' throws clauses from IOException to Exception, matching the rewrite it applied to the socket instrumentation earlier on this branch. Committed so the tree is settled and the next gate run is a no-op. Co-Authored-By: plan-marshall <noreply@cuioss.de>
PR #254 added ForwardedTrustFromEnvironment after this branch forked, carrying two bare `listen(0)` binds and a local LOOPBACK = "127.0.0.1" constant. Both are the exposure class this branch exists to remove: the listeners bind the dual-stack wildcard while the fixture dials loopback. Found by this branch's own ArchUnit guard, which CI ran against the merge result. That is the guard working as intended on code the branch never saw — the sites are invisible from the branch tip and appear only once the two histories are combined. Routes the local constant through LoopbackHost.ADDRESS rather than leaving a second spelling of the same address in the tree. Co-Authored-By: plan-marshall <noreply@cuioss.de>
The netstat half of SocketSnapshot was macOS-only in both halves of its contract, and neither was platform-guarded: - `netstat -anv -p tcp` is macOS syntax. Linux reads `-p` as show-program, taking no argument, so the invocation never selected TCP there. `-ant` is the Linux spelling that does. - The port pattern matched macOS's dot separator (`127.0.0.1.59120`). Linux prints a colon (`127.0.0.1:59120`). Selected per platform rather than unioned: a dot-matching pattern applied to Linux output matches the dots inside the IPv4 address, so `127.0.0.1:59120` would yield a spurious port `1`. Accepting either separator everywhere would manufacture wrong ports rather than widen coverage. The section banner no longer names the argv, since a banner carrying one platform's flags stops matching on the other and would silently turn a probe that narrows a message to these rows into one that finds nothing. The argv actually used is rendered inside the section instead. This refutes a documented assumption of this plan: netstat was believed absent from ubuntu-latest, with lsof present. CI shows netstat IS present, so available() returns true and the netstat half runs — it simply produced nothing usable. The previous section-wide assertion hid that behind lsof's output; the per-tool assertion added earlier in this branch is what surfaced it. Co-Authored-By: plan-marshall <noreply@cuioss.de>
…w CI caught it The page described the capture as `netstat -anv -p tcp`, which is the macOS spelling only. Corrects that to name both platforms' invocations, and adds the finding itself: two beliefs were wrong at once — that netstat is absent from ubuntu-latest, and that a section-wide assertion demonstrated the capture had worked — and each hid the other. Written up because it is the same failure shape this page already documents elsewhere: a check whose green is satisfiable by a neighbour is not a check on the thing it names. Co-Authored-By: plan-marshall <noreply@cuioss.de>
The class doc still named `netstat -anv -p tcp` as the command, which is the macOS spelling only, and presented the Recv-Q/Send-Q/rxbytes reading as platform-neutral when those columns are macOS's. Names both invocations and scopes the column reading to macOS. Also settles the formatter's array-initialiser rewrite from the gate run. Co-Authored-By: plan-marshall <noreply@cuioss.de>
cuioss-oliver
force-pushed
the
feature/loopback-stall-fix-and-instrumentation
branch
from
September 3, 2026 22:42
cf72c95 to
8a01904
Compare
Collaborator
Author
|
/review |
Collaborator
Author
|
@coderabbitai full review |
✅ Action performedFull review finished. |
Collaborator
Author
|
@coderabbitai full review |
|
Collaborator
Author
|
@coderabbitai full review |
|
Collaborator
Author
|
@coderabbitai full review |
|
The build-gate page said the fix converted "All 20 such sites across 9 files". It converted 22: PR #254 landed two more bare listen(0) sites in GatewayEdgeRouteTest after this branch forked, and rebasing brought them in. Re-derived from the tree rather than trusted — 22 host-bound listen calls across the nine fixture files, excluding the specimens this branch added and the two occurrences that are Javadoc text rather than bind sites. This one matters more than the earlier stale counts: it is the claim a reader checks the deliverable against, and it UNDERSTATED the work, so a reader verifying it would have found more sites than the page admits to and had no way to tell whether the surplus was intended. Second, from the simplify re-run: the wrapped-line assertion's message said joining the specimen "leaves five violations" while the count assertion three lines above asserts four. A leftover from the round that briefly had five specimens. No coverage change. Co-Authored-By: plan-marshall <noreply@cuioss.de>
Collaborator
Author
|
@coderabbitai full review |
|
Collaborator
Author
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 3
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: cuioss/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Team
Run ID: 92742a2c-bc2c-4ff9-b733-01bb288b12f7
📒 Files selected for processing (19)
.plan/marshal.jsonapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/arch/LoopbackEphemeralBindArchTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/arch/specimen/LoopbackEphemeralBindSpecimen.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/arch/specimen/WildcardEphemeralBindSpecimen.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/edge/GatewayEdgePipelineTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/edge/GatewayEdgeRouteBffWiringTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/edge/GatewayEdgeRouteTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/edge/GrpcDispatchStageTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/edge/GrpcStatusMapperTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/edge/ReservedBodyCeilingTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/edge/WebSocketRelayStageTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/testsupport/Awaits.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/testsupport/AwaitsTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/testsupport/LoopbackHost.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/testsupport/SocketSnapshot.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/tls/PassthroughRelayTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/tls/SniFrontListenerTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/tls/TlsEdgeProducerTest.javadoc/development/build-gate-discipline.adoc
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.
The section said saturation, deadlock and a blocked handler were "excluded" by two thread dumps. Two dumps are two instants: they show no sampled thread in those states AT those moments, and say nothing about work that ran or blocked between them. On a page whose subject is not drawing conclusions from evidence that cannot support them, that was the wrong claim to leave standing. Rewritten to put the weight where it belongs — the live socket capture and the E1/E2 controls observe the mechanism directly, while the dumps are consistent with it. Consistency is the right weight for a sample. Two smaller corrections alongside: - The arch test's carve-out note still said "three of the four sites" are nested. freePort's re-probe made five; the nested three are the collision holders, so the claim is now about them rather than about a total. Sixth stale count on this branch, all from the same move. - The sweep description named one wildcard host form and its nesting limit but not the full set. It refuses 0.0.0.0, :: and the empty host, and tolerates whitespace before listen — a contributor reading only the doc would have under-estimated what the guard covers. Reported by CodeRabbit on PR #255. Co-Authored-By: plan-marshall <noreply@cuioss.de>
Collaborator
Author
|
@coderabbitai full review |
Collaborator
Author
|
/review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: cuioss/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Team
Run ID: e5b11fe8-64f2-4530-9d1f-738f832166ac
📒 Files selected for processing (19)
.plan/marshal.jsonapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/arch/LoopbackEphemeralBindArchTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/arch/specimen/LoopbackEphemeralBindSpecimen.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/arch/specimen/WildcardEphemeralBindSpecimen.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/edge/GatewayEdgePipelineTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/edge/GatewayEdgeRouteBffWiringTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/edge/GatewayEdgeRouteTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/edge/GrpcDispatchStageTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/edge/GrpcStatusMapperTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/edge/ReservedBodyCeilingTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/edge/WebSocketRelayStageTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/testsupport/Awaits.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/testsupport/AwaitsTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/testsupport/LoopbackHost.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/testsupport/SocketSnapshot.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/tls/PassthroughRelayTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/tls/SniFrontListenerTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/tls/TlsEdgeProducerTest.javadoc/development/build-gate-discipline.adoc
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.
…sweep `server./* c */listen(0, "0.0.0.0")` is legal Java and slipped a whitespace-only separator, while the bytecode rule accepts it for the usual reason — the target is still listen(int, String). The bypass therefore sat open in both halves of the guard. The separator now accepts whitespace, block comments and line comments. The block-comment branch is written `[^*]|\*(?!/)` so it consumes without backtracking, and the alternatives cannot overlap, so this does not reintroduce the S8786 ambiguity fixed earlier on this branch. Asserted against literal strings rather than source specimens, following the lesson from the spaced-selector round: a shape the formatter may normalise cannot be pinned in source. Three assertions — both comment spellings match, and a comment-interleaved LOOPBACK bind still does NOT match, so tolerating comments did not cost the discrimination the sweep exists for. Residual limit recorded in place rather than implied: a comment INSIDE the argument list is still out of reach, and closing that needs a Java lexer — disproportionate machinery for the secondary half of a guard whose primary half is the bytecode rule. Reported by CodeRabbit on PR #255. Co-Authored-By: plan-marshall <noreply@cuioss.de>
Introducing SEPARATOR placed its declaration between WILDCARD_HOST_LISTEN's Javadoc and the field that Javadoc documents, orphaning the doc and leaving the pattern undocumented. Moved SEPARATOR above it. java:S8491, and the second time this exact insertion mistake has occurred on this branch — the first was readBounded landing between loopbackRows' Javadoc and its method. Both times the symptom read as a formatting nit and was in fact a detached doc. Co-Authored-By: plan-marshall <noreply@cuioss.de>
Collaborator
Author
|
@coderabbitai full review |
Collaborator
Author
|
/review |
✅ Action performedFull review finished. |
Collaborator
Author
|
@coderabbitai full review |
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: cuioss/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Team
Run ID: e5c1f028-1cfe-46b7-a469-e109dd1e449b
📒 Files selected for processing (19)
.plan/marshal.jsonapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/arch/LoopbackEphemeralBindArchTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/arch/specimen/LoopbackEphemeralBindSpecimen.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/arch/specimen/WildcardEphemeralBindSpecimen.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/edge/GatewayEdgePipelineTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/edge/GatewayEdgeRouteBffWiringTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/edge/GatewayEdgeRouteTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/edge/GrpcDispatchStageTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/edge/GrpcStatusMapperTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/edge/ReservedBodyCeilingTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/edge/WebSocketRelayStageTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/testsupport/Awaits.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/testsupport/AwaitsTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/testsupport/LoopbackHost.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/testsupport/SocketSnapshot.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/tls/PassthroughRelayTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/tls/SniFrontListenerTest.javaapi-sheriff/src/test/java/de/cuioss/sheriff/gateway/tls/TlsEdgeProducerTest.javadoc/development/build-gate-discipline.adoc
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.
|
…nner overclaiming Every specimen in the source file uses "0.0.0.0", so the `::` and empty-host alternatives could have been deleted from WILDCARD_HOST_LISTEN and every control would have stayed green while the guard went on claiming to reject all three. The controls now assert each form directly, plus a negative on "127.0.0.1" — without which all three positives would pass for a pattern that matched ANY host string. Second: NETSTAT_SUBSECTION promised "Recv-Q/Send-Q, rxbytes/txbytes and process:pid discriminate" on every platform. Those are macOS `-v` columns; Linux `-ant` does not produce them, so the header named discriminators the rows below it did not contain. The banner is now a bare marker and the column note is rendered per platform, saying plainly which fields are absent on Linux. That is the third label-overclaims-content correction in this file. The pattern is consistent enough to be worth naming: a header written against the platform the author was standing on. Reported by CodeRabbit on PR #255. Co-Authored-By: plan-marshall <noreply@cuioss.de>
…s the columns The class Javadoc lays out three diagnostic readings built on Recv-Q/Send-Q, rxbytes/txbytes and process:pid, presented as universally available. The commit before this one established that the byte counters and process:pid are macOS `-v` columns that Linux's `-ant` does not produce — so a Linux reader following this guide would attempt the "nothing was ever put on the wire" reading using columns that are not there, and that reading specifically needs the cumulative counters to tell "never sent" from "already drained". Now states that only the first reading is available on Linux, and points the reader at the rendered capture's own column line before applying any of it. Found by looking for the sibling rather than waiting for it to be reported: correcting a header and leaving the prose that depends on it is the same fix-one-site-miss-the-other pattern this branch has produced repeatedly, and three instances of it in this file were caught by review rather than by me. Co-Authored-By: plan-marshall <noreply@cuioss.de>
Collaborator
Author
|
@coderabbitai full review |
Collaborator
Author
|
/review |
Action performedFull review triggered. |
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
1 participant
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.
Intent
Every live-socket fixture in the
api-sherifftest tree bound its ephemeral server with thesingle-argument
listen(0)overload — the dual-stack wildcard — while every client in those samefixtures dialled
127.0.0.1. That mismatch is the measured cause of the macOS-local 30-second teststall: a wildcard ephemeral bind can be won by a foreign loopback listener, and the awaiting client
then burns its full connect ceiling.
This PR binds all 20 such sites to loopback through one shared
testsupportconstant, lands thesocket-state instrumentation that captures the kernel's own view at every timeout, replaces the
refuted surviving-hypothesis section of the build-gate discipline doc with the measured mechanism,
closes out the originating lesson, and adds an ArchUnit fitness function plus a source sweep that
refuse the reintroduction of every wildcard spelling.
The fix is behaviour-preserving: every dial target across all nine affected fixtures was already
127.0.0.1, so binding the listener to loopback changes no test semantics.Deliverables
LoopbackHost.ADDRESS; all 20 bare.listen(0)sites across ninefixtures bind explicitly to loopback, and the 24 inline
127.0.0.1literals route through it.SocketSnapshotlands in the test tree;Awaitscarries theOS socket view, a thread dump, and elapsed time on every timeout.
kqueue-readiness reading is retained and labelled
SUPERSEDED, with an account of how it failed.2026-08-29-16-002updated as a record-only, purely additive change.LoopbackEphemeralBindArchTestbans both bare bind spellingsat the bytecode level, and a companion source sweep catches the wildcard host literal the
bytecode rule structurally cannot see (
listen(0, "0.0.0.0")andlisten(0, LoopbackHost.ADDRESS)are the same call target). Four specimens pin the sweep's reach: single-line, wrapped across
lines, variable port, and nested port call.
Verification
.listen(0)count 0 across 972 files scanned, nounreadable or elided paths;
127.0.0.1in the nine fixtures 24 → 0;new ServerSocket(0)exactly4, all in
TlsEdgeProducerTest.listen(0)failed the ArchUnit rulewith one violation naming file and line; wrapping one across three lines failed the sweep the same
way. Other probes stayed green each time, so both discriminate rather than blanket-fail.
What CI established that local runs could not
The Linux lane refuted a documented assumption of this plan, and the correction is part of the diff.
The plan recorded that
netstatis absent fromubuntu-latest(withlsofpresent), so thecapture would abstain there. CI showed the opposite:
netstatis present, so the availabilityprobe returned true and the netstat half ran — while its invocation (
-anv -p tcp) and port patternwere macOS-only, so it produced nothing usable on Linux. The netstat half had never worked there.
It stayed invisible because the probe asserting the capture had worked scanned the combined
section, so
lsofoutput satisfied it. Splitting that assertion per tool — itself a CodeRabbitfinding on this PR — is what turned a silent degradation into a red build. Both the invocation and
the port pattern are now platform-selected, and the doc records the episode.
Review
Five review rounds; every finding is fixed, replied to individually, and its thread resolved. One
suggestion was declined with reasoning on the thread:
required_botsnames the registrybot_kind(pr-agent), not the reviewer's GitHub author login (cuioss-review-bot) — see thethread on
.plan/marshal.jsonfor why the login form cannot satisfy the participation quorum.Notably, all four rounds after the first found defects in the guard added during review, not in
the deliverables above, which have been stable since they landed.
Summary by CodeRabbit
Bug Fixes
Tests
Documentation