diff --git a/.claude/skills/run-integration-tests/SKILL.md b/.claude/skills/run-integration-tests/SKILL.md index c6e3cbcc..907947c4 100644 --- a/.claude/skills/run-integration-tests/SKILL.md +++ b/.claude/skills/run-integration-tests/SKILL.md @@ -31,6 +31,11 @@ Use a 10-minute Bash timeout. On a loaded machine the whole cycle can exceed the ``` 3. Curl the health endpoints (below) and read `docker compose logs api-sheriff`. Re-run only step 3 while iterating. +## Two pointers out of this stack + +- **A green run here is not evidence about browser-enforced controls.** These suites drive the gateway with RestAssured. `demo-client/doc/playwright-suite.adoc` states why that leaves a whole class of controls outside their reach, enumerates the class, and carries the Playwright suite that is the only coverage in this repository for it — a suite that gates nothing, since it runs on pushes to `main` and on demand but never on a pull request. Read it before concluding anything about that class from a green run here. +- **Need a subset of this stack rather than all of it?** `demo-client/scripts/start-dev-environment.sh` already carries the working invocation for a trimmed three-container bring-up. Do not reach for a Compose profile instead: the standing prohibition, with its reasoning, is in `demo-client/doc/playwright-suite.adoc`. + ## Port & health map | Service | Container | Host | Notes | diff --git a/.github/workflows/maven.yml b/.github/workflows/maven.yml index 5ad9156c..973069b2 100644 --- a/.github/workflows/maven.yml +++ b/.github/workflows/maven.yml @@ -156,3 +156,135 @@ jobs: trivy-filesystem.sarif retention-days: 30 if-no-files-found: error + + # OPENREWRITE DIRTY-TREE REPORT — deliberately NON-GATING. + # + # Three mechanisms in this repository mutate the working tree while the gate exits 0, and all three + # are invisible in the build result by construction. They are documented in + # doc/development/build-gate-discipline.adoc; this job is the only part of that story a machine can + # carry, so it PRINTS what the rewriting gate changed and which recipe changed it, and never judges. + # + # `-Ppre-commit` IS LOAD-BEARING. The recipe list that reaches all three mechanisms is the + # pre-commit profile's `activeRecipes` override in the root pom.xml. Without the profile, + # `rewrite:run` reports `Using active recipe(s) []`, changes nothing, and this job becomes a false + # green that appears to exonerate OpenRewrite. Do not "simplify" the profile away. + # + # No CI lane runs the rewriting gate today, so this job has to invoke it before it has a dirty tree + # to report. That is CI time this repository did not previously spend; the invocation is kept to the + # narrowest form that still produces all three mutations. + # + # DELIBERATE DIVERGENCE FROM ADR-0030, which holds that a repository invariant asserted only by an + # explanatory comment becomes a positively-phrased, machine-checked fitness function. This lane is + # deliberately NOT that shape, because the fixed point is observed rather than held. The argument, + # and the condition under which ADR-0030's shape becomes the right call, are in + # doc/development/build-gate-discipline.adoc under "The enforcement" — read it before converting + # this job into a hard fail-on-dirty gate. + # + # No `needs:` — patterned on supply-chain-scan above; the job neither delays nor is delayed by + # `build`. It commits nothing and pushes nothing: the mutated tree exists only inside the runner. + rewrite-report: + name: OpenRewrite dirty-tree report (non-gating) + runs-on: ubuntu-latest + timeout-minutes: 30 + permissions: + contents: read + + steps: + - name: Harden the runner (Audit all outbound calls) + uses: step-security/harden-runner@b09bb98e06d4d774595224525879c09bc6e98c40 # v2.20.1 + with: + egress-policy: audit + + - name: Checkout code + uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + persist-credentials: false + + - name: Set up JDK 25 + uses: actions/setup-java@b6effb05e454b25005698d916606bdc6ffcbf961 # v5.7.0 + with: + java-version: '25' + distribution: 'temurin' + cache: maven + + # `shell: bash` is required, not decorative: it selects `-eo pipefail`, whereas the default + # `bash -e` leaves pipefail OFF and the `| tee` below would swallow a Maven failure. A genuine + # gate or tooling failure must still turn this check red — only the DIRTY VERDICT is non-gating, + # which is why `continue-on-error` is deliberately not used on this job or any of its steps. + # The goal is invoked by FULL COORDINATES, not by the `rewrite:` prefix. A prefix has to be + # resolved through `org.openrewrite.maven/maven-metadata.xml` on Central, which is a network + # fetch this job does not otherwise need — and when Central answers 429 the prefix does not + # resolve, the gate never runs, and the job dies with `No plugin found for prefix 'rewrite'`. + # Full coordinates resolve the version from the pre-commit profile's own plugin declaration. + # The log goes to `.plan/temp/`, which `.gitignore` already excludes, NOT to the checkout + # root. A log in the root is picked up by the `git status --porcelain` below as `?? rewrite-run.log` + # — the job would manufacture one tree mutation of its own and report it next to the real ones, + # leaving a reader unable to tell the artefact from the finding. + - name: Run the rewriting gate + shell: bash + run: | + mkdir -p .plan/temp + ./mvnw -B -ntp -Ppre-commit org.openrewrite.maven:rewrite-maven-plugin:run -DskipTests 2>&1 | tee .plan/temp/rewrite-run.log + + # Report, without a verdict. Nothing here asserts cleanliness, so there is no `exit 1` path: + # a dirty tree produces a longer summary and a green check, which is the whole point. + # + # The attribution parser is coupled to OpenRewrite's log wording and to its indentation, and + # that coupling cannot be removed short of reimplementing the plugin's reporting. What CAN be + # removed is the SILENCE. When the wording or the indentation moves, the matcher stops matching + # and an empty attribution block reads exactly like "no recipe changed anything" — a confident + # report of nothing having happened. So the match counts are cross-checked against the working + # tree: a dirty tree with zero matched headers, or headers with no recipe lines beneath them, + # is reported as a BROKEN PARSER and never as a clean run. + - name: Publish the dirty-tree report to the job summary + shell: bash + run: | + tree_status="$(git status --porcelain)" + counts="$(awk -v out=.plan/temp/attribution.txt ' + /Changes have been made to / { block = 1; headers++; print > out; next } + block && /^\[INFO\][[:space:]][[:space:]]+/ { detail++; print > out; next } + block && /^[[:space:]][[:space:]]+/ { detail++; print > out; next } + { block = 0 } + END { print headers + 0, detail + 0 } + ' .plan/temp/rewrite-run.log)" + headers="${counts% *}" + detail="${counts#* }" + { + echo "## OpenRewrite dirty-tree report (non-gating)" + echo + echo "The findings below NEVER fail this check. They record what the rewriting gate" + echo "changed in the runner's working tree while exiting 0. See" + echo "\`doc/development/build-gate-discipline.adoc\` for what to do about them." + echo + echo "### Working tree after the gate" + echo '```' + if [ -n "$tree_status" ]; then + printf '%s\n' "$tree_status" + else + echo "(clean - the gate rewrote nothing)" + fi + echo '```' + echo + echo "### Recipe attribution" + echo '```' + if [ "$headers" -gt 0 ]; then + cat .plan/temp/attribution.txt + else + echo "(no recipe reported a change)" + fi + echo '```' + if [ "$headers" -eq 0 ] && [ -n "$tree_status" ]; then + echo + echo "> **Parser drift, not a clean run.** The tree above is dirty, yet no" + echo "> \`Changes have been made to\` line matched. Do NOT read the empty attribution as" + echo "> \"no recipe ran\" — OpenRewrite's log wording has most likely moved. Attribute the" + echo "> changes from the raw gate log in the previous step, and fix the awk matcher in" + echo "> \`.github/workflows/maven.yml\`." + elif [ "$headers" -gt 0 ] && [ "$detail" -eq 0 ]; then + echo + echo "> **Parser drift, partial match.** $headers change header(s) matched but no recipe" + echo "> line beneath them did, so the attribution above names no recipe. OpenRewrite's log" + echo "> indentation has most likely moved; fix the awk matcher in" + echo "> \`.github/workflows/maven.yml\`." + fi + } >> "$GITHUB_STEP_SUMMARY" diff --git a/CLAUDE.md b/CLAUDE.md index 177ef174..faa84ff1 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -67,6 +67,17 @@ turned on over was retired. A `@SuppressWarnings` added to get back to green hol while leaving it reporting success, which is worse than not having it — and it collides with the Pre-1.0 rule below that forbids carrying deprecated code at all. +**A successful build is not evidence that work happened.** `BUILD SUCCESS` says the build +completed — not that it compiled what you changed, ran what you wrote, or kept what you fixed. The +gate below and every specific case documented off it are instances of that one rule. + +**A gate that exits 0 can still have changed your files**, and three different mechanisms in this +repository do. So a review-bot suggestion is verified by *surviving* the gate, never by being +implemented: run the gate, then `git status --porcelain`, and attribute a dirty tree before +reverting it. Revert unrelated churn; keep the rewrite only for files the branch itself authored. +See `doc/development/build-gate-discipline.adoc` for the three mechanisms, the three operational +consequences and the recipe-scoping trap. + **Documentation-only commits skip both.** A commit whose entire footprint is prose or agent instructions cannot change build output, so a Maven run proves nothing and only burns minutes. Skip when **every** changed file is one of: @@ -119,6 +130,7 @@ one is a red `main`. - Minimum 80% coverage - CUI Test Generator for test data (`@GeneratorsSource` preferred) - **Forbidden**: Mockito, PowerMock, Hamcrest +- **A configuration key that parses is not a configuration key that acts.** Ask: *if the key were deleted entirely, would any test go red?* If not, the control it names is not test-proven — that is all a green suite settles, so trace the key to its production reader before concluding anything about whether it is in effect — see `doc/development/declared-limit-assertion-coverage.adoc` ### Javadoc diff --git a/demo-client/doc/playwright-suite.adoc b/demo-client/doc/playwright-suite.adoc index 27f90aaf..2dce603b 100644 --- a/demo-client/doc/playwright-suite.adoc +++ b/demo-client/doc/playwright-suite.adoc @@ -81,11 +81,18 @@ regardless. The suite was therefore structurally incapable of observing this def green throughout. It was not a weak test; it was a test of something else. Generalise it before adding the next control: *any control whose enforcement lives in the browser -- -`SameSite`, `Secure`, the `__Host-` prefix, CORS, `Referrer-Policy`, CSP -- cannot be proven by a -programmatic HTTP client, however faithful.* Only a real browser applies the policy. When you add or -change one, add the assertion to this Playwright suite; a green `Bff*IT` run tells you nothing about -it. That limitation is now recorded on `BffKeycloakLoginFlow` itself, so the next reader meets it at -the helper rather than in this document. +`SameSite`, `Secure`, the `__Host-` prefix, cookie partitioning, CORS, `Referrer-Policy`, CSP, and +the navigation context a response arrives in (a top-level redirect GET versus a cross-site form +POST) -- cannot be proven by a programmatic HTTP client, however faithful.* Only a real browser +applies the policy. The last member is the one the defect above turned on, and it is listed here so +the class matches the story rather than trailing it. *This Playwright suite is the only suite in +this repository that covers that class -- and it gates nothing.* It runs on pushes to `main` and on +demand, never on a pull request, so a pull request that breaks one of those controls can merge with +this suite never having run. Coverage and enforcement are separate things here, and only the first +one exists. When you add or change one of those controls, add the assertion here and run the suite +deliberately; a green `Bff*IT` run tells you nothing about it. That RestAssured limitation is now +recorded on `BffKeycloakLoginFlow` itself, so the next reader meets it at the helper rather than in +this document. ==== [#_module_layout] diff --git a/doc/development/README.adoc b/doc/development/README.adoc index 5515d556..cdeb6265 100644 --- a/doc/development/README.adoc +++ b/doc/development/README.adoc @@ -30,6 +30,15 @@ to *run* the finished gateway). merging over a red or stale-green gate, PR-new-code vs post-merge project-gate auditability, and the fix-by-default / suppress-with-rationale escape hatch. +| link:build-gate-discipline.adoc[Build-Gate Discipline -- A Green Gate That Changed Your Files] +| The three mechanisms that mutate the working tree while the gate exits 0 -- OpenRewrite's + import-group churn, the pre-commit formatter's non-idempotency, and the `SimplifyTestThrows` + reversion -- the three consequences a contributor acts on (verify by surviving the gate, + attribute a dirty tree before reverting it, never hand-edit formatter-owned formatting), the + revert rule with the objection to it and the standing fixed-point debt, the recipe-scoping trap, + the non-gating `rewrite-report` job that reports all of it, the incremental-build rule, and the + recorded discard of the fully-qualified-refspec fix. + | link:release-process.adoc[Release Process] | How a release is cut -- merge a version change in `.github/project.yml`, which is the publishing act once the centrally-owned version-changed guard lets it through, with `workflow_dispatch` as the @@ -71,7 +80,7 @@ to *run* the finished gateway). `docker-compose.yml` and the mounted `gateway.yaml`, which stay the authoritative sources. | link:declared-limit-assertion-coverage.adoc[Declared-Limit Assertion Coverage] -| Which declared gateway limit has a boundary assertion and which does not -- the twelve-file +| Which declared gateway limit has a boundary assertion and which does not -- the thirteen-file descriptor surface, the strict preset's resolved caps that appear in no yaml, one row per limit naming its enforcing component and its asserting test, the standing gap remainder, and the two report-only findings -- that the body-cap wiring guard's glob never reaches the `endpoints/` tree, diff --git a/doc/development/build-gate-discipline.adoc b/doc/development/build-gate-discipline.adoc new file mode 100644 index 00000000..99541423 --- /dev/null +++ b/doc/development/build-gate-discipline.adoc @@ -0,0 +1,202 @@ += Build-Gate Discipline -- A Green Gate That Changed Your Files +:toc: +:toclevels: 2 +:sectnums: + +link:../../CLAUDE.md[`CLAUDE.md`] states the class, in its *Pre-Commit Process* section: a gate that +exits 0 can still have changed your files. This page is the detail behind that line -- which +mechanisms in this repository do it, what a contributor does about them, and the two neighbouring +rules about reading build output honestly. The class statement is not restated here; the +enforcement is named at the end. + +== The three mechanisms that exit 0 while mutating the tree + +Three distinct mechanisms in this repository mutate the working tree during a green +`verify -Ppre-commit` run. None of them is visible in the build result -- that is the whole +difficulty. `BUILD SUCCESS` is printed either way. + +Where the tree stands today: a whole-reactor `verify -Ppre-commit` drives `rewrite:run` with the +full `pre-commit` recipe list in every module, produces *zero* `Changes have been made to` +attributions, and leaves the working tree clean. The repository is at the active recipes' fixed +point. That is a property of the tree at a point in time, not a guarantee -- the fixed point is +lost the moment a change lands that the recipes want to rewrite, and nothing announces the loss. +The three mechanisms below are what happens on that run. They are dormant, not retired, and every +rule on this page is written for the run on which they wake up. + +=== OpenRewrite import-group churn + +The `pre-commit` profile's `activeRecipes` override in the root `pom.xml` activates +`org.openrewrite.java.OrderImports` and `org.openrewrite.java.RemoveUnusedImports`. Against a tree +that is not already at their fixed point, they rewrite import blocks across roughly *170 files* -- +almost all of them untouched by whatever branch is running the gate. + +This one has a second-order effect that is easy to miss. The plan-marshall change-ledger stamps +`worktree_sha` at *build end*, so it records the *churned* tree. Reverting the churn restores the +pre-build sha, and the freshness check then reports `stale` for a commit whose gate has just +passed. A "the worktree was never verified" refusal immediately after a green gate is this +interaction, not a lost build. + +=== The pre-commit formatter is not idempotent + +`org.openrewrite.java.format.AutoFormat` and its companions do not converge in one pass: on a tree +away from their fixed point, a run takes A to B, and the next run takes B to C rather than leaving +B alone. What was observed when this was diagnosed was a `main` that gave every run real work to +do, and the work was not stable. + +`main` is at the fixed point today, so the recipes have nothing to do and the instability has +nothing to act on. It comes back with the first change the recipes want to rewrite. The practical +consequence, on any run that does have work to do, is that "run the formatter again and see if it +settles" is not a diagnostic here. It will not settle. + +=== `SimplifyTestThrows` silently reverts an accepted narrowing + +`org.openrewrite.java.testing.junit5.JUnit5BestPractices` -- also in the `pre-commit` profile's +recipe list -- reaches `SimplifyTestThrows`, which broadens a narrowed `throws` clause on a test +method back to `throws Exception`. When a review bot asks for a narrower clause and the narrowing +is implemented and then run through the gate, the recipe reverts it, and the gate exits 0 over the +reverted form. The change is gone and every visible signal says the work landed. + +== The three operational consequences + +=== Verify a review-bot suggestion by surviving the gate, not by implementing it + +Implementing a suggestion is not evidence that the suggestion is in the tree. Run the gate, then +run `git status --porcelain`, and read the result: a file you edited that now shows as modified +against your own commit has been rewritten by the gate, and a file that shows nothing may have been +rewritten back to where it started. A suggestion is verified only by *surviving the gate*. + +=== Attribute a post-gate dirty tree before reverting it + +The reflex on a dirty tree is "the usual import churn, revert it and move on". That reflex is what +loses a `SimplifyTestThrows` reversion, because it discards the evidence unread. + +OpenRewrite prints its attribution into the build log, as a +`Changes have been made to by:` block followed by the recipe chain responsible. Read that +block before reverting anything: it distinguishes the three mechanisms above from each other and +from a genuine change of yours. + +One trap in the tooling around that log: the `build-maven` rewrite-log parser can report +`verdict: not_observed` for a build in which `rewrite:run` demonstrably ran, because it keys on a +findings marker rather than on the recipe-attribution blocks. That verdict is *not* evidence that +OpenRewrite was inactive. When it disagrees with the log, the log wins. + +=== Never hand-edit formatter-owned whitespace, wrapping or import order + +Whitespace, line wrapping and import order belong to the formatter. Editing them by hand puts you +in a loop against a mechanism that is not idempotent: two rounds of "reformat by hand, re-run the +gate, get a different result" is not bad luck, it is a proof of the non-idempotency described +above. Stop at that point and revert the cosmetic edits rather than trying to find the arrangement +the formatter will accept -- there is not one. + +== The revert rule, and the objection to it + +*Revert unrelated churn; keep the rewrite only for files the branch itself authored.* A scoped pull +request carries the files its change is about. + +The countervailing argument is real and should be stated rather than ignored: on a tree away from +the fixed point the reverted diff *will reappear on the next run*, so reverting it is work that has +to be repeated. That is true, and it is not a reason to commit the churn. Paying that cost once per +pull request is cheaper than polluting every scoped pull request with roughly 170 unrelated files +-- which destroys the reviewability of each one and buries the actual change. + +The rule stands even though the tree is at the fixed point today, and it stands *because* of how +that fixed point is held: it is a property of the current tree rather than a guarantee about future +ones, and it is lost silently the first time a change the recipes want to rewrite lands. On that +run the churn is back and the revert-versus-commit question is live again, with the same answer. +Running `git status --porcelain` after the gate is what tells a contributor which kind of run they +are on -- there is no other signal, because the gate exits 0 either way. + +== The recipe-scoping trap + +A reproduction that drops the profile does not reproduce anything. `rewrite:run` *without* +`-Ppre-commit` reports `Using active recipe(s) []` and changes no file, because the recipe list +that reaches all three mechanisms is the `pre-commit` profile's `activeRecipes` override. + +The failure mode is a false negative that reads as an exoneration: a narrower, faster invocation +comes back clean, and OpenRewrite appears to be innocent of a mutation it did in fact perform. +`-Ppre-commit` is load-bearing in every reproduction of anything on this page. + +== The enforcement + +All three mechanisms are invisible in the build result by construction, so the one thing that can +be done about them mechanically is to *report* them. link:../../.github/workflows/maven.yml[`.github/workflows/maven.yml`] +carries a `rewrite-report` job, named "OpenRewrite dirty-tree report (non-gating)", which runs the +rewriting gate itself and then prints, into the job summary, the post-gate `git status --porcelain` +listing and every `Changes have been made to by:` block with its recipe chain. + +The job *deliberately never fails the build on a dirty verdict*. The tree is observed clean at the +fixed point today, but that is not guaranteed to hold, and a hard fail-on-dirty gate would block +every pull request from the moment any drift lands -- including the pull requests that did not +cause it. So the job reports the dirty set and the recipe attribution and leaves the disposition to +the author. A genuine Maven or tooling failure still turns the check red; only the dirty verdict is +non-gating. + +The condition under which the ADR-0030 fitness-function shape becomes the right call is a fixed +point that is *held* rather than merely observed: once drift is caught and corrected by the change +that introduces it, a positively-phrased "the tree is clean after the rewriting gate" assertion +stops being a blanket blocker and becomes an invariant worth asserting. + +== A build whose output says it did nothing has verified nothing + +This is the specific case of the general rule in link:../../CLAUDE.md[`CLAUDE.md`] -- a successful +build is not evidence that work happened -- applied to Maven's incremental compilation. + +Maven's `testCompile` compares test-source timestamps against the compiled test classes. A change +confined to `src/main/java`, or a pure signature change, leaves every test source older than its +class file, so `testCompile` skips compilation entirely and prints +`Nothing to compile - all classes are up to date`. Plain `test` inherits the same defect, because +Surefire runs whatever is already sitting in `target/test-classes`. + +*After any production-only or signature change, run `clean test-compile` -- or `clean verify`.* +That marker line in the output is the detection signal: a build that reports it has verified +nothing about the change, whatever its exit code says. + +Two instances of this repository's own history are the evidence for the single rule, not two +separate rules: a production-only change that looked green and yielded *142 compilation errors* +under `clean test-compile`, and a signature change that yielded *26*. + +== Two rules about reading a check's output + +=== An ERROR from a guard is a statement about the guard, not about its subject + +When a guard, gate or analysis step reports an error, the first reading is almost always "the thing +being checked is broken". Frequently the correct reading is "the check could not evaluate" -- a +missing input, an unresolvable path, a tool that never started. + +Establish that the check actually ran before treating its output as a finding about its subject. +Establish the same before treating a *green* as evidence: a check that silently could not run +reports success indistinguishable from a real pass, and that is the more dangerous half of this +rule. An error at least draws attention to itself. + +=== A plausible root cause is not the root cause + +A story that explains every symptom is not thereby true. An explanation that fits all the available +facts is a hypothesis with good fit, and it survives exactly until someone re-derives it against +the implementing source -- at which point a comfortable, load-bearing explanation can turn out to +describe code that does not exist. + +Before a diagnosis is acted on, read the implementation it claims to describe. + +== Recorded discard: the fully-qualified-refspec fix + +A proposal to make the release tag check use fully-qualified refspecs is recorded here as +*discarded*, so it is not re-opened from memory. + +It has nothing left to apply to. The project-local release skill's tag check now uses +`git fetch --tags --force` together with `git ls-remote --exit-code --tags`, and its only surviving +`merge-base` use is an ancestry assertion rather than a resolution against a prunable +remote-tracking ref -- which was the failure the refspec change existed to prevent. + +Its companion prescription, "run the guard checks serially", is likewise *not* landed, and +deliberately so: it was refuted by the correction that followed it, which found that serial +execution merely hid the problem rather than fixing it. + +== See also + +* link:../../CLAUDE.md[`CLAUDE.md`] -- the class statement, the general build-success rule, and the + pre-commit process this page details. +* link:../../.github/workflows/maven.yml[`.github/workflows/maven.yml`] -- the non-gating + `rewrite-report` job. +* link:sonar-quality-gate.adoc[SonarCloud Quality Gate -- Compliance Policy] -- the other blocking + gate a contributor answers to, and what a plan's declared footprint does about its findings. +* link:README.adoc[Contributor Guide] -- the index for this documentation layer. diff --git a/doc/development/declared-limit-assertion-coverage.adoc b/doc/development/declared-limit-assertion-coverage.adoc index 26c545b3..3574b406 100644 --- a/doc/development/declared-limit-assertion-coverage.adoc +++ b/doc/development/declared-limit-assertion-coverage.adoc @@ -541,3 +541,64 @@ This table is prose, and prose drifts the moment a descriptor changes. Three pro When you add a limit-shaped key to any of the thirteen files, add its row here in the same change -- and give it a status that is honest, including `GAP`. + +== Declared Is Not Asserted: Three Rules Behind the Matrix + +The matrix answers *which declared limit has an asserting test*. These three rules are the *why* +behind that question -- what a key with no asserting test actually is, what happens the day it +acquires one, and which shape of test settles the question for an opt-in feature. Together they are +the reason this note counts gaps instead of trusting the descriptors. + +=== The deletion discriminator + +*A configuration key that parses is not a configuration key that acts.* A key that is spelled +correctly, passes schema validation and reads exactly like a security control can still have no +production consumer at all -- in which case it is a *silent security no-op*: the descriptor reads +correctly, the schema accepts it, the build is green, and the control it names applies to nothing. +Every visible signal agrees, and every one of them is about the declaration rather than the +behaviour. + +The one-line test is: *if the key were deleted entirely, would any test go red?* If the answer is +no, the control is *not test-proven* -- and that is the whole of what the test settles. It does not +settle whether the control is in effect. Two shapes come back green and are still live: a key with a +production reader but no asserting test -- rows *G7*, *G8* and *G9* of the matrix above are exactly +that, each naming an enforcer and `none` in the asserting-test column -- and a key whose default or +inherited value carries the same behaviour once the explicit declaration is gone. So the deletion +test is the first of two steps, not the verdict. Confirm production consumption separately, by +tracing the key to the reader that consumes it. + +This note carries its own worked instance. The `tls.min_version` protocol floor and the +`tls.cipher_suites` restriction (row *G11*) were declared, valid and inert -- they had *zero* +production readers until `TlsServerCustomizer` became one, so they constrained no listener at all +while the descriptor, the schema and the build all reported health. Deleting either key at that +point would have turned nothing red. The deletion test is what raised the suspicion; the trace to a +production reader -- finding none -- is what settled it. + +=== Wiring an inert key is a behavioural flip for every consumer + +The day an inert key becomes live, every consumer that reads the surface it governs changes +behaviour at once. An existing test may fail, and the trap is to treat that one failure as the scope +of the change. Where the surface carries no asserting test at all -- rows *G7*, *G8* and *G9* again +-- the flip lands with nothing going red, and the trap has no trigger to spring. + +*The test that fails first is a sample of the dependant set, not the whole of it.* Fixing it closes +the investigation while the un-sampled dependants stay broken, and they surface later and further +from the change that caused them -- by which point the wiring commit no longer looks like a +suspect. Enumerate the dependants before declaring the wiring done, rather than letting the test +suite enumerate them for you one failure at a time. + +When the contract needs a new assertion, *extend `SingleSourceTlsContractTest`* -- or its siblings +`CipherSuiteFixtureWiringTest` and `ManagementPlainHttpActivationWiringTest` -- rather than writing +a new contract test beside them. The guards already exist, and a second one splits the contract +across two files that then drift apart. + +=== An opt-in runtime feature needs a deployment-activation test + +Unit tests prove `config -> behaviour`: given this configuration, the component does this. They do +not prove that *the deployment sets that configuration*. An opt-in feature can be fully unit-tested +and entirely off in every running instance, and no unit test can see the difference. + +The shape that closes the other half is the `*ActivationWiringTest` family this note already +indexes, with `TlsEdgeActivationWiringTest` as the reference: it asserts the *deployed descriptor +actually turns the feature on*. That is the assertion a unit test structurally cannot reach, and it +is why a feature landing without one is recorded here as a gap rather than as coverage. diff --git a/doc/development/sonar-quality-gate.adoc b/doc/development/sonar-quality-gate.adoc index 6548cec1..e532dd66 100644 --- a/doc/development/sonar-quality-gate.adoc +++ b/doc/development/sonar-quality-gate.adoc @@ -62,6 +62,65 @@ Reaching zero by *silently marking issues won't-fix or false-positive in the Son *not acceptable* -- that decision leaves no trace in the codebase and cannot be reviewed in a pull request. Every deviation from a clean gate must be visible and justified in the source. +== The plan's footprint governs in both directions + +Two rules answer the same question from opposite ends: *what does a change do about the lines and +findings its own declared footprint does not cover?* One keeps unrelated lines out of the +measurement; the other keeps the diff from being pulled out past its boundary. + +=== Do not let a cosmetic sweep pull unrelated lines in + +Sonar's new-code window is *the diff*, not the semantics of the diff. A line touched for any reason +enters the new-code measurement, and the gate does not ask whether the change authored the +behaviour on it. + +The consequence is sharper than it sounds, and the sentence above also bounds it: a *purely +cosmetic* edit inside an already-uncovered branch turns *the lines it changed* into uncovered new +code. Pull-request analysis is scoped to the changed lines, so the untouched siblings in that branch +never enter the measurement -- the window really is the diff -- but the changed lines are enough on +their own. A pure rename inside an uncovered `catch` block can fail the new-coverage condition on +lines the change did not author and does not test. + +The remedy is prevention rather than repair. Keep a cosmetic sweep -- a rename, a reformat, a +comment tidy -- *out of the interior of uncovered branches*. Where the sweep has already landed and +the condition is red, the honest options are to cover the branch or to back the cosmetic edit out; +neither is cheap, which is why the rule is stated as prevention. + +=== Do not let a finding pull your diff out + +The mirror case: a finding lands *inside* the scanned surface but *outside* the plan's declared +write-boundary. + +That finding is dispositioned `taken_into_account` and reported as a *named follow-up* -- never +silently absorbed into the current change. + +The two halves of that rule are backed very differently, and the difference is worth stating. The +*disposition* is mechanised, though not in this repository: plan-marshall's triage tooling records it +in the plan's findings ledger via `manage-findings resolve --resolution taken_into_account`, one of +that store's resolution values, so the decision survives as an artefact rather than an intention. +The *named follow-up* is not mechanised at all. No workflow check and no plan-validation rule here +asserts that the follow-up was actually filed; it is a process rule, kept by whoever runs the +triage. + +Naming that boundary is not a caveat, it is the point. Absorbing the finding widens the blast radius +past the boundary the specification declared, and the widening is *invisible in the gate result*, +because a gate only ever gets greener as more is fixed. A green gate over a change twice the size it +was scoped to is still a scope breach; the gate simply cannot report it. A section built on that +observation must not itself claim an enforcement it does not have. + +=== Cross-references here name documents, not lines + +This policy names *documents and rules*. It does not carry `file:line` anchors, and neither should +anything written into it. A line number is unowned duplicated state: it decays the moment the file +it points into is edited, and it decays *without any signal* -- nothing fails, the reference simply +starts pointing somewhere else. + +The evidence this section deliberately omits is the worked example. An earlier four-anchor deferral +table belongs here by subject and is *not* landed: its anchors no longer resolve, and one of them +now points at a comment recording that the finding was *fixed* rather than deferred -- so +re-landing the table would ship a stale deferral list that reads as current. Where a deferral list +is wanted, re-derive it from the live gate rather than restoring a decayed one. + == See also * link:../../.github/project.yml[`.github/project.yml`] -- the repository's SonarCloud declaration.