Skip to content

test(gates): assert native runtime-init list, gate ITs on readiness - #147

Merged
cuioss-oliver merged 12 commits into
mainfrom
feature/plan-42-runtime-init-and-readiness-gates
Aug 3, 2026
Merged

test(gates): assert native runtime-init list, gate ITs on readiness#147
cuioss-oliver merged 12 commits into
mainfrom
feature/plan-42-runtime-init-and-readiness-gates

Conversation

@cuioss-oliver

@cuioss-oliver cuioss-oliver commented Aug 3, 2026

Copy link
Copy Markdown
Collaborator

Intent

Two gates that read as coverage and provide none. One guards the native image, one guards the
integration-test suite. Both were observed in CI, not inferred.

D1 — make the native-image runtime-init registration list executable

application.properties carries a hand-maintained --initialize-at-run-time list under
quarkus.native.additional-build-args. Every BFF class holding a static final SecureRandom must
appear in it. The list was guarded only by an explanatory comment that named the exact failure mode
"instance of Random/SplittableRandom in the image heap" — and still did not prevent it.

NativeRuntimeInitRegistrationArchTest now asserts it in the fast local gate. Four legs, all green:

  • positive rule — every de.cuioss.sheriff.gateway.bff.. class declaring a static final SecureRandom field appears in the registration list. Phrased positively (should(condition)),
    never noClasses()/noFields().
  • non-vacuity guard — asserts the selection predicate resolves to at least one class today, the
    parsed list is non-empty, and the quarkus.native.additional-build-args key is present.
  • negative controlStaticSecureRandomSpecimen is a deliberately non-compliant fixture the
    rule must reject, proven via assertThrows.
  • matched positive controls — the predicate must NOT select SealedSessionCookieCodec (a
    per-instance private final SecureRandom) or CookieKeyMaterial (a local new SecureRandom()).

Two implementation details worth knowing:

  • Properties are read from target/classes/application.properties by path, guarded with
    Files.isRegularFile. getResourceAsStream("/application.properties") resolves the test
    file, because target/test-classes precedes target/classes on the Surefire classpath — the gate
    would have passed green against the wrong artifact.
  • Coverage is evaluated class → list, not list → class, so the one package-prefix entry
    (de.cuioss.sheriff.token.client.flow) cannot be mistaken for a BFF class. The test deliberately
    does not assert the detected set equals today's four FQCNs — freezing it would force an edit
    for every legitimate new registration.

D2 — gate the IT bring-up on readiness, and derive the Keycloak probe

start-integration-container.sh gated on /q/health/live. A gateway that is live but not ready
passed the wait, and the suite started against a half-initialised instance.

PLAN-31B had already collapsed the per-instance wait blocks into one Compose-model-derived loop, so
the plan's "do all four wait sites" directive was stale — this is a one-line probe swap in that
single loop, plus:

  • the retry budget was measured, not guessed. A 100 ms prober started before compose up -d
    (so it observes the transition rather than inferring it) ran against all six gateway instances
    brought up concurrently with Keycloak under 24-worker CPU contention. The live→ready delta is
    0.00s on every instance (worst absolute ready 9.14s). That is what ADR-0027 predicts — the
    jwks datum is a boot-time constructibility fact read from a cached instance, so readiness flips
    when liveness does. The budget stays at 30 attempts on that evidence; it is not shrunk toward
    9s, because CI runners are slower than the machine measured here. The three drifting literals are
    now one GATEWAY_READY_ATTEMPTS constant with the measurement recorded beside it.
  • the hardcoded https://localhost:1090 at both the Keycloak wait and the banner echo is gone —
    the Compose discovery block was hoisted above the first compose up and widened to emit a
    keycloak row, then split into KEYCLOAK_TARGET / READINESS_TARGETS.
  • a latent bug fixed en route: the Keycloak probe used -s without -f. /health/ready
    answers 503 while starting, so curl exited 0 and the wait cleared at port-accept, defeating the
    JWKS race guard that block exists for. Now -sf.

D3 — record the readiness contract

doc/development/integration-test-topology.adoc gains a The Readiness Contract section recording
what the gate asserts, what GatewayReadinessCheck actually attests (and explicitly what it does
not — a post-boot JWKS outage does not take readiness DOWN), the measured budget with its
numbers, and why -plain-mgmt serving plain HTTP is a deliberate exception rather than a defect.
The missing api-sheriff-plain-mgmt row (10448/19005) was added to the variant table, which listed
four of five instances.

Scope Deviation Accepted

demo-client/scripts/start-dev-environment.sh was changed behaviourally, though the plan
declared it read-only for D2. Its gateway wait now probes /q/health/ready with the same hoisted
GATEWAY_READY_ATTEMPTS constant. Accepted by operator decision: it removes a readiness-contract
divergence between the two parallel bring-ups that the Q-Gate flagged, and it is covered by the same
containerised evidence. Disclosed here rather than folded in silently.

A style(imports) commit applies the pre-commit formatter's pending import ordering to eight
files this plan does not otherwise touch. verify -Ppre-commit rewrites them on every run —
pre-existing drift on main. Reverting it each time left the build-freshness ledger recording a
tree that was never the one pushed. Import ordering only, no behavioural change.

Verification

  • 90 containerised integration tests green end-to-end, all six gateway instances gated on
    /q/health/ready, run under CPU contention. A green single-instance or uncontended local run
    would not have been sufficient evidence here, and is not what is claimed.
  • verify -Ppre-commit green; full verify green.
  • NativeRuntimeInitRegistrationArchTest — 5 tests, all four legs confirmed present in the
    module-test log, not merely compiled.

Follow-ups deliberately not folded in

  • The topology SVG omits api-sheriff-plain-mgmt — labelled variant instances (4), with the
    <desc> repeating the omission. Same gap as the table, but closing it is diagram work (fifth
    member, 10448 · 19005 pair, count label, <desc>, both-theme re-render). Recorded in the doc.
  • benchmarks/pom.xml:314 hardcodes https://localhost:1090. Deriving it from the Compose
    model needs new Maven machinery; D2's criterion is script-scoped.
  • Security audit filed two findings (non-blocking): the arch gate's selection radius stops at
    bff while 14 other gateway packages exist, and chmod 0777 without the sticky bit on the
    bind-mounted log dirs.

🤖 Generated with Claude Code

https://claude.ai/code/session_01FM7Rt95uGJm1VapzbYXtmg

Summary by CodeRabbit

  • Improvements
    • Improved development and integration-test startup reliability with readiness checks, clearer progress reporting, and consistent retry handling.
    • Added support for validating gateway services across HTTP and HTTPS management configurations.
    • Improved container log-directory permissions for non-root environments.
  • Documentation
    • Expanded integration-topology documentation with gateway variants, service endpoints, readiness behavior, and startup measurements.
    • Added guidance for enforcing runtime initialization and deriving readiness probes from deployed service configuration.
  • Tests
    • Added architecture and integration coverage for runtime initialization and management-port readiness.

@sourcery-ai sourcery-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

coderabbitai Bot commented Aug 3, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

The PR adds architecture validation for static SecureRandom runtime initialization. It also changes development and integration startup scripts to derive probe targets from Compose metadata and use gateway readiness checks.

Changes

Runtime initialization validation

Layer / File(s) Summary
SecureRandom runtime-init architecture contract
api-sheriff/src/test/java/de/cuioss/sheriff/gateway/arch/..., doc/adr/0030-...
The ArchUnit test validates static-final SecureRandom fields against compiled runtime-init registrations. It includes non-vacuity, positive-control, negative-control, and specimen checks. ADR-0030 documents the fitness-function requirements.

Compose-derived readiness gates

Layer / File(s) Summary
Compose-derived readiness probing
integration-tests/scripts/start-integration-container.sh, demo-client/scripts/start-dev-environment.sh, integration-tests/src/test/java/...
Startup scripts derive gateway and Keycloak probe URLs from the resolved Compose model. Gateway checks use /q/health/ready, shared retry budgets, and scheme-specific certificate handling. The management-port assertion message identifies the HTTP probe.
Readiness contract and topology documentation
doc/adr/0031-..., doc/development/integration-test-topology.adoc
The documentation records the fifth gateway variant, readiness behavior, Compose-derived targets, retry measurements, plain-HTTP management, and rejected alternatives.

Estimated code review effort: 4 (Complex) | ~60 minutes

Possibly related PRs

🚥 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 two main changes: native runtime-init coverage and readiness-gated integration tests.

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

cuioss-review-bot Bot commented Aug 3, 2026

Copy link
Copy Markdown

PR Reviewer Guide 🔍

(Review updated until commit f5b50d2)

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

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (1)
api-sheriff/src/test/java/de/cuioss/sheriff/gateway/arch/NativeRuntimeInitRegistrationArchTest.java (1)

395-398: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Derive the BFF package selector from one constant.

BFF_PACKAGE_PATTERN drives the ArchUnit rule at line 171. residesInBffPackage reimplements the same scope in plain Java for selectedOwnerNames(). The two agree today, but they are independent string literals, so an edit to one silently desynchronizes the non-vacuity guard from the rule it protects. The failure message at lines 213-216 names BFF_PACKAGE_PATTERN while the value it reports on comes from residesInBffPackage.

Derive the pattern from BFF_PACKAGE so one edit moves both.

♻️ Proposed refactor
     private static final String BFF_PACKAGE = "de.cuioss.sheriff.gateway.bff";
-    private static final String BFF_PACKAGE_PATTERN = "de.cuioss.sheriff.gateway.bff..";
+    private static final String BFF_PACKAGE_PATTERN = BFF_PACKAGE + "..";

ℹ️ Review info
⚙️ Run configuration

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

Review profile: CHILL

Plan: Pro Plus

Run ID: 54c29dfe-9268-4890-8273-a682f64028f0

📥 Commits

Reviewing files that changed from the base of the PR and between 0e14620 and f33af09.

📒 Files selected for processing (14)
  • api-sheriff/src/main/java/de/cuioss/sheriff/gateway/auth/JwksTrustProfileResolver.java
  • api-sheriff/src/main/java/de/cuioss/sheriff/gateway/config/load/ConfigLoader.java
  • api-sheriff/src/test/java/de/cuioss/sheriff/gateway/arch/NativeRuntimeInitRegistrationArchTest.java
  • api-sheriff/src/test/java/de/cuioss/sheriff/gateway/arch/specimen/StaticSecureRandomSpecimen.java
  • api-sheriff/src/test/java/de/cuioss/sheriff/gateway/auth/JwksTrustProfileResolverTest.java
  • api-sheriff/src/test/java/de/cuioss/sheriff/gateway/auth/MountedTlsMapKeyTest.java
  • api-sheriff/src/test/java/de/cuioss/sheriff/gateway/auth/TestTlsConfigurationRegistry.java
  • api-sheriff/src/test/java/de/cuioss/sheriff/gateway/config/SingleSourceTlsContractTest.java
  • api-sheriff/src/test/java/de/cuioss/sheriff/gateway/quarkus/DefaultProfileReadinessTest.java
  • api-sheriff/src/test/java/de/cuioss/sheriff/gateway/quarkus/ShippedApplicationPropertiesTest.java
  • demo-client/scripts/start-dev-environment.sh
  • doc/development/integration-test-topology.adoc
  • integration-tests/scripts/start-integration-container.sh
  • integration-tests/src/test/java/de/cuioss/sheriff/gateway/integration/ManagementPlainHttpActivationWiringTest.java
💤 Files with no reviewable changes (5)
  • api-sheriff/src/test/java/de/cuioss/sheriff/gateway/quarkus/ShippedApplicationPropertiesTest.java
  • api-sheriff/src/test/java/de/cuioss/sheriff/gateway/auth/TestTlsConfigurationRegistry.java
  • api-sheriff/src/test/java/de/cuioss/sheriff/gateway/quarkus/DefaultProfileReadinessTest.java
  • api-sheriff/src/test/java/de/cuioss/sheriff/gateway/auth/MountedTlsMapKeyTest.java
  • api-sheriff/src/test/java/de/cuioss/sheriff/gateway/config/SingleSourceTlsContractTest.java

@cuioss-oliver

Copy link
Copy Markdown
Collaborator Author

Triage dispositions

In reply to comment_id: PRR_kwDOPatrT88AAAABII-xpw

Accepted. Two independent literals that must agree is a real desync hazard, and your observation that the gateIsNonVacuous failure message at lines 213-216 names BFF_PACKAGE_PATTERN while reporting a value produced by residesInBffPackage is exactly the symptom. Will be addressed by TASK-4; see follow-up commit on this branch. Note the fix lands slightly differently than the proposed diff: a security-audit finding on the same file is widening the gate radius from the bff package to the whole de.cuioss.sheriff.gateway tree, so the single derived constant will be the widened base package rather than BFF_PACKAGE + '..'. The desync you identified is closed either way, and collapsing the duplicate is what makes that widening a one-place edit.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
integration-tests/scripts/start-integration-container.sh (1)

364-366: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Derive the Keycloak public target too.

The new KEYCLOAK_HEALTH_URL is model-derived. The application URL printed immediately above still embeds port 1443. If integration-tests/docker-compose.yml changes the published host port for container port 8443, readiness will work but the banner will point to the wrong target. Add the public port to the discovery output and build the banner from that value, as demo-client/scripts/start-dev-environment.sh already does.

Suggested direction
-echo "  🔑 Keycloak:       https://localhost:1443/auth"
+echo "  🔑 Keycloak:       https://localhost:${KEYCLOAK_PUBLIC_PORT}/auth"

KEYCLOAK_PUBLIC_PORT must come from the Compose discovery block.

As per path instructions, values that mirror an authoritative definition must be derived at build or run time.

Source: Path instructions


ℹ️ Review info
⚙️ Run configuration

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

Review profile: CHILL

Plan: Pro Plus

Run ID: 08777d05-e1ff-4739-a967-a0fac7ca5de8

📥 Commits

Reviewing files that changed from the base of the PR and between f33af09 and f5b50d2.

📒 Files selected for processing (3)
  • api-sheriff/src/test/java/de/cuioss/sheriff/gateway/arch/NativeRuntimeInitRegistrationArchTest.java
  • demo-client/scripts/start-dev-environment.sh
  • integration-tests/scripts/start-integration-container.sh

Comment thread integration-tests/scripts/start-integration-container.sh
@cuioss-oliver

Copy link
Copy Markdown
Collaborator Author

/review

@cuioss-oliver

Copy link
Copy Markdown
Collaborator Author

Triage dispositions

In reply to comment_id: PRR_kwDOPatrT88AAAABII-xpw

Accepted. Two independent literals that must agree is a real desync hazard, and your observation that the gateIsNonVacuous failure message at lines 213-216 names BFF_PACKAGE_PATTERN while reporting a value produced by residesInBffPackage is exactly the symptom. Will be addressed by TASK-4; see follow-up commit on this branch. Note the fix lands slightly differently than the proposed diff: a security-audit finding on the same file is widening the gate radius from the bff package to the whole de.cuioss.sheriff.gateway tree, so the single derived constant will be the widened base package rather than BFF_PACKAGE + '..'. The desync you identified is closed either way, and collapsing the duplicate is what makes that widening a one-place edit.

In reply to comment_id: PRR_kwDOPatrT88AAAABIJjy1g

Declined as out of scope, not as wrong. Your observation is factually correct: KEYCLOAK_HEALTH_URL at line 366 is Compose-model-derived while the banner one line above at 361 still embeds 1443 literally. That literal was left in deliberately. At refine time the operator scoped this plan's hardcoded-host correction to Keycloak's MANAGEMENT port only - the two sites the plan names - and explicitly ruled the remaining literals out of scope: go-httpbin 18080, nginx-static 18081 and this Keycloak auth URL 1443 all stay hardcoded. Adding KEYCLOAK_PUBLIC_PORT to the discovery block and rebuilding the banner from it would reverse that scope decision and widen the discovery block beyond what was authorised, so it is left for a follow-up rather than folded into this PR. The drift you describe is also bounded and cosmetic here: the readiness gate this PR adds is fully model-derived, so republishing container port 8443 on a different host port would break only the printed convenience link, never the gate itself.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2


ℹ️ Review info
⚙️ Run configuration

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

Review profile: CHILL

Plan: Pro Plus

Run ID: a1c4d31c-4d5f-4e56-a25a-0faefb6e22a0

📥 Commits

Reviewing files that changed from the base of the PR and between f5b50d2 and 601a142.

📒 Files selected for processing (2)
  • doc/adr/0028-Comment-only_invariants_become_positively-phrased_fitness_functions_shipped_with_a_four-leg_control_set.adoc
  • doc/adr/0029-Host-side_readiness_gates_derive_the_probe_URL_from_the_resolved_Compose_model_and_assert_readiness.adoc

cuioss-oliver and others added 12 commits August 3, 2026 11:44
…gistered

GraalVM initializes static state at build time, so a `static final
SecureRandom` in a BFF class must carry a matching
`--initialize-at-run-time=` entry in `quarkus.native.additional-build-args`.
Nothing linked the Java field to that properties entry, so adding a field
without the registration was a silent omission surfacing only during a
native build — or, in the seeded-heap case, not at all.

NativeRuntimeInitRegistrationArchTest closes the gap in the JVM test cycle
with four legs: a positive `fields().should(...)` rule (never the
`noFields()` form, which would invert event polarity and pass vacuously),
a non-vacuity guard over the field selection / config key / parsed list,
a negative control against the new StaticSecureRandomSpecimen, and two
matched positive controls proving the selection excludes the per-instance
field in SealedSessionCookieCodec and the local construction in
CookieKeyMaterial.

Coverage is evaluated class -> list and the detected set is deliberately
not frozen to today's names, so a legitimate new registration needs no
edit here. The properties file is read from `target/classes` rather than
the classpath so a dependency's own application.properties cannot win the
lookup.

Co-Authored-By: Claude <noreply@anthropic.com>
The gateway wait cleared on /q/health/live, which answers as soon as the
process is up — strictly earlier than the point at which the suite may
drive the instance. It now probes /q/health/ready, so the gate asserts
what GatewayReadinessCheck attests: the configuration document is bound
and the @GatewayValidator-qualified TokenValidator resolved.

The retry budget is now ONE named constant read by the loop bound, the
last-attempt comparison and the progress echo — previously three literal
30s that could drift apart. Its value is measured, not chosen: a 100 ms
prober started before `compose up -d` (so it observes the transition
rather than inferring it) recorded a live-to-ready delta of 0.00s on all
six instances under CPU contention, with a worst time-to-ready of 9.14s.
That is what ADR-0027 predicts — the `jwks` datum is a boot-time
constructibility fact, so readiness flips when liveness does — so the
switch needs no extra budget and 30 attempts is retained on evidence.

The Keycloak wait and the closing banner no longer hardcode
https://localhost:1090. The Compose-model discovery moved ahead of the
first `compose up` and now emits a keycloak row beside the api-sheriff*
rows (with the symmetric missing-row guard); the rows are split into
KEYCLOAK_TARGET and READINESS_TARGETS, mirroring the demo script's
IDP_TARGET / GATEWAY_TARGETS split, and the probe URL and its -k are
derived from the service's management-scheme label.

The Keycloak probe also gains -f. Without it curl exits 0 on the 503
/health/ready answers while Keycloak starts, so the wait cleared as soon
as the port ACCEPTED — the very race the gate exists to remove.

demo-client's dev bring-up carried the same live-probe and the same three
drifting literals and is folded in identically. Verified end-to-end: the
containerised suite runs 90 ITs green with all six instances gated on
readiness.

Co-Authored-By: Claude <noreply@anthropic.com>
Applied by the pre-commit OpenRewrite pass to the package comparison in
NativeRuntimeInitRegistrationArchTest: the constant is now the receiver,
so the call cannot NPE on a null argument. Behaviour is unchanged.

Co-Authored-By: Claude <noreply@anthropic.com>
Adds a Readiness Contract section to the integration-test topology note
covering four facts a contributor otherwise has to reverse-engineer.

What the gate asserts: /q/health/ready, in the single Compose-derived wait
loop, and why liveness is not the gate.

What GatewayReadinessCheck actually attests: boot-time constructibility of
the @GatewayValidator-qualified TokenValidator, read from a cached
instance rather than fetched per probe. Stated plainly, because it reads
stronger than it is — a later-unreachable IdP, a stalled rotation and an
expired signing key do NOT take readiness DOWN, and belong in the
container log rather than the probe. ADR-0027 is cross-referenced for the
exclusion decision behind that narrowness.

Where the retry budget came from: the measured per-instance live and ready
figures under contention, the 0.00s delta, and the headroom factor stated
against the 9.14s worst case — a note claiming "measured" without the
numbers would not be a contract.

Why -plain-mgmt serves plain HTTP: the supported ADR-0025 downgrade path,
carried structurally by the management-scheme label, not a defect.

Also fixes a count the table already had wrong: docker-compose.yml
declares five variant instances and the table listed four, omitting
api-sheriff-plain-mgmt (10448 / 19005). The diagram was checked against
this change — it depicts no fact this change alters, so it is deliberately
not redrawn — and that check surfaced the same omission in the SVG, which
is recorded as a follow-up rather than folded in here.

Co-Authored-By: Claude <noreply@anthropic.com>
Q-Gate finding b47875: demo-client/scripts/start-dev-environment.sh still
gates on /q/health/live with an unmeasured 30-attempt budget, so the two
parallel bring-ups diverge on the readiness contract from this change onward.
Record that as a stated decision with its rationale and a follow-up pointer,
rather than leaving the topology note reading as if it described both.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FM7Rt95uGJm1VapzbYXtmg
The topology note claimed the demo bring-up still diverged on the readiness
contract, but the same changeset had already switched it to /q/health/ready
with the same hoisted budget constant — so the section documented a divergence
that no longer existed and promised a follow-up already done. Rewritten to
state what is actually true.

Both scripts' comments now point at the topology note's measurement table
instead of restating its figures, so two copies of one measurement cannot
drift apart.

Comment and prose only; no behavioural change.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FM7Rt95uGJm1VapzbYXtmg
…ntrol

Three triaged findings against the runtime-init SecureRandom arch gate, all in
one file.

Gate radius (security-audit 6eff4d): the rule selected only
de.cuioss.sheriff.gateway.bff.., so a static final SecureRandom added in any of
the 14 other gateway packages -- several crypto-adjacent -- was never selected,
never reached --initialize-at-run-time, and GraalVM would initialise its holder
at build time and bake a seeded generator into the image heap (CWE-336 /
CWE-1204). Widened to the whole gateway tree. All four current holders are in
bff and registered, so this passes today at no cost.

Duplicated selector (b10e3b): BFF_PACKAGE_PATTERN drove the ArchUnit rule while
residesInBffPackage reimplemented the same scope as an independent literal --
they agreed until one was edited, and the non-vacuity failure message named one
while reporting a value from the other. Collapsed to a single GATED_CLASS
predicate shared verbatim by the rule and selectedOwnerNames().

Widening puts the negative-control specimen under the gated prefix, where
DO_NOT_INCLUDE_TESTS was the only thing holding it out of the production
selection. GATED_CLASS now carves the specimen package out explicitly, so that
ImportOption is no longer the single load-bearing guard.

Vacuous control (8482f8): selectionExcludesLocalSecureRandomConstruction
asserted exclusion only, so it also passed green if CookieKeyMaterial were
renamed, moved or deleted. It now asserts the near-miss is real first -- the
class still references SecureRandom and still declares no SecureRandom field --
matching the rigor of the sibling per-instance control.

Verified by a real run: all 5 tests green, including the negative control, which
still detects the unregistered specimen.

Co-Authored-By: Claude <noreply@anthropic.com>
FINDING 695f3d (security-audit, CWE-732). Both bring-up scripts create the
bind-mounted Quarkus log directory and chmod it 0777.

The world write is genuinely required and is preserved: the gateway container
runs as the distroless nonroot user while the directory is created by the
differently-numbered build user, so without it the file sink dies with
"FileNotFoundException: /logs/quarkus.log (Permission denied)". Both sites
already narrow the grant to a dedicated quarkus-logs subdirectory rather than
the whole target tree, which stays as it is.

What was missing is the sticky bit. Without it any local account on a shared CI
runner or developer host can delete or replace quarkus.log -- the file CI
uploads as a failure-diagnosis artifact -- so the evidence read after a failed
run is locally tamperable. 1777 keeps exactly the world write the container
needs while restricting unlink and rename to the file owner and the directory
owner.

Also corrects the container uid both comment blocks asserted. They claimed uid
1001; the image is quay.io/quarkus/quarkus-distroless-image with USER nonroot,
and the log files this run produced are owned by 65532. The sticky-bit rationale
is recorded in the same comment blocks that already explain the world-write
grant, rather than left as a bare mode change.

Verified by a real containerised run, not by inspection: verify
-Pintegration-tests green, the directory comes back drwxrwxrwt, and all six
per-service logs were created fresh by the container through it. Rotation also
worked (the .log.N files), which is the direct evidence that the sticky bit does
not block the owner's own unlink -- and mvn clean is unaffected because it runs
as the user owning the directory.

Co-Authored-By: Claude <noreply@anthropic.com>
The arch gate had two implementations of self-or-descendant dotted-prefix
containment — `residesIn` and the inline test in `isRegistered` — each with its
own paragraph explaining the same "the dot is appended deliberately" rationale.
Collapsed into one `isWithin`, applying to the prefix rule the same argument the
file already makes for `GATED_CLASS`: one shared thing, not two expressions of
one intent.

Also corrects the sticky-bit comment's claim that the script uploads quarkus.log
as a CI artifact. It does not — the workflow uploads only failsafe-reports. The
tamper-evidence argument stands without the false premise.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FM7Rt95uGJm1VapzbYXtmg
ADR-0028 records the fitness-function authoring contract this plan applied:
an invariant a comment cannot enforce becomes a positively-phrased rule shipped
with four control legs (non-vacuity guard, negative control, matched positive
controls from real code, explicit specimen carve-out), because a vacuous gate is
worse than the comment it replaces.

ADR-0029 records the bring-up gating rule: derive scheme and published port from
the resolved Compose model rather than restating them, gate on readiness rather
than liveness, and size the retry budget from a worst case measured under
contention.

Both Proposed. Numbered 28/29 above the pre-existing 0026/0027 duplicate-number
collisions in doc/adr/.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FM7Rt95uGJm1VapzbYXtmg
Review caught both. The script does restate one port literal — the container-side
management port used as the binding selector — so "no host and no port number" was
too strong; narrowed to the published host port, with the container port named as
a platform-level contract rather than a per-instance fact.

And the retry-budget constant is per-script, not shared across the two bring-up
scripts. Scoped the claim accordingly and moved the duplication into Consequences
where it belongs, rather than leaving a shared-constant claim the code does not
support.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FM7Rt95uGJm1VapzbYXtmg
Two ADRs numbered 0028 and 0029 landed on main while this branch was open, so
the numbers chosen here now collide. Renumbered to the next free pair rather
than adding a third and fourth duplicate to a corpus that already carries
duplicate 0026 and 0027.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FM7Rt95uGJm1VapzbYXtmg
@cuioss-oliver
cuioss-oliver force-pushed the feature/plan-42-runtime-init-and-readiness-gates branch from 851f423 to 1fe95a2 Compare August 3, 2026 10:00

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (1)
integration-tests/scripts/start-integration-container.sh (1)

201-215: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Centralize the Keycloak retry count, matching the pattern just introduced for GATEWAY_READY_ATTEMPTS.

Line 203, line 208, and line 213 each hardcode 120 separately. This is the identical drift risk the file's own comments describe for the pre-consolidation gateway retry logic. Introduce a KEYCLOAK_READY_ATTEMPTS variable and read it in the loop bound, the last-attempt comparison, and both messages.

♻️ Proposed fix
+KEYCLOAK_READY_ATTEMPTS=120
+
 echo "⏳ Waiting for Keycloak to be ready (management ${KC_MGMT_SCHEME} on ${KC_MGMT_PORT})..."
-for i in {1..120}; do
+for ((i = 1; i <= KEYCLOAK_READY_ATTEMPTS; i++)); do
     if curl "${KEYCLOAK_PROBE_OPTS[@]}" "${KEYCLOAK_HEALTH_URL}" > /dev/null 2>&1; then
         echo "✅ Keycloak is ready!"
         break
     fi
-    if [ "$i" -eq 120 ]; then
-        echo "❌ Keycloak did not answer ${KEYCLOAK_HEALTH_URL} within 120 attempts"
+    if [ "$i" -eq "$KEYCLOAK_READY_ATTEMPTS" ]; then
+        echo "❌ Keycloak did not answer ${KEYCLOAK_HEALTH_URL} within ${KEYCLOAK_READY_ATTEMPTS} attempts"
         echo "Check logs with: ${COMPOSE_BASE} logs keycloak"
         exit 1
     fi
-    echo "⏳ Waiting for Keycloak... (attempt $i/120)"
+    echo "⏳ Waiting for Keycloak... (attempt $i/${KEYCLOAK_READY_ATTEMPTS})"
     sleep 1
 done

ℹ️ Review info
⚙️ Run configuration

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

Review profile: CHILL

Plan: Pro Plus

Run ID: 4b2011c9-047f-49a5-8d66-0913cb034476

📥 Commits

Reviewing files that changed from the base of the PR and between 601a142 and 1fe95a2.

📒 Files selected for processing (8)
  • api-sheriff/src/test/java/de/cuioss/sheriff/gateway/arch/NativeRuntimeInitRegistrationArchTest.java
  • api-sheriff/src/test/java/de/cuioss/sheriff/gateway/arch/specimen/StaticSecureRandomSpecimen.java
  • demo-client/scripts/start-dev-environment.sh
  • doc/adr/0030-Comment-only_invariants_become_positively-phrased_fitness_functions_shipped_with_a_four-leg_control_set.adoc
  • doc/adr/0031-Host-side_readiness_gates_derive_the_probe_URL_from_the_resolved_Compose_model_and_assert_readiness.adoc
  • doc/development/integration-test-topology.adoc
  • integration-tests/scripts/start-integration-container.sh
  • integration-tests/src/test/java/de/cuioss/sheriff/gateway/integration/ManagementPlainHttpActivationWiringTest.java
🚧 Files skipped from review as they are similar to previous changes (4)
  • integration-tests/src/test/java/de/cuioss/sheriff/gateway/integration/ManagementPlainHttpActivationWiringTest.java
  • api-sheriff/src/test/java/de/cuioss/sheriff/gateway/arch/specimen/StaticSecureRandomSpecimen.java
  • api-sheriff/src/test/java/de/cuioss/sheriff/gateway/arch/NativeRuntimeInitRegistrationArchTest.java
  • doc/development/integration-test-topology.adoc

@@ -0,0 +1,148 @@
= ADR-0031: Host-side readiness gates derive the probe URL from the resolved Compose model and assert readiness

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Narrow the probe-URL derivation claim.

integration-tests/scripts/start-integration-container.sh constructs KEYCLOAK_HEALTH_URL with the hardcoded host localhost. The Compose model supplies the service, scheme, and published port. It does not supply the host used by this host-side probe.

Change Line 1 and Lines 51-60 to state that the scheme and published port are derived. Document localhost as the fixed host-side invariant.

As per path instructions, a stated derived behavior must have a mechanism in the implementing source file. The current source does not derive the host.

Also applies to: 51-60

Source: Path instructions

Comment on lines +67 to +76
**Derive the retry budget from measurement, and record the measurement.** The
budget is traceable to an observed worst-case time-to-ready, measured with all
instances brought up concurrently on a machine under load, plus a stated headroom
factor. The measured figures and the factor are recorded in developer documentation
alongside the gate, so a later reader can re-evaluate the budget rather than guess
at it. Within a script, the retry budget is a single constant read by every site
that needs it -- the loop bound, the last-attempt comparison, the progress
message -- so the number cannot drift between spellings. The constant is
per-script: two bring-up scripts that gate independently each declare their own,
and that duplication is stated as a consequence below rather than claimed away.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Align the retry-budget rule with the implementation.

The ADR states that one constant drives every retry site in a script. integration-tests/scripts/start-integration-container.sh uses literal 120 values for the Keycloak wait and literal 30 values for another wait, including loop bounds, final-attempt checks, and messages.

If the gates require different budgets, document one named constant per gate. Otherwise, make every applicable wait read the same named constant. Update Lines 111-116 to describe the actual scope of the duplication.

As per path instructions, the stated centralized behavior must have a mechanism in the implementing source file. The current source repeats numeric literals.

Also applies to: 111-116

Source: Path instructions

@cuioss-oliver
cuioss-oliver added this pull request to the merge queue Aug 3, 2026
Merged via the queue into main with commit 0e7c8d3 Aug 3, 2026
29 checks passed
@cuioss-oliver
cuioss-oliver deleted the feature/plan-42-runtime-init-and-readiness-gates branch August 3, 2026 10:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant