docs(gate): land corpus lessons and add non-blocking rewrite report - #200
Merged
Conversation
|
Warning Review limit reached
Next review available in: 17 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository: cuioss/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThe pull request documents browser-control coverage, adds non-gating OpenRewrite reporting to Maven CI, and expands contributor guidance for build gates, configuration assertions, and SonarCloud scope. ChangesBrowser-Control Test Coverage
Build-Gate Reporting and Discipline
Coverage and Scope Policies
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 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 f28aee4)
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The awk-based extraction of recipe attribution from
rewrite-run.login therewrite-reportjob is tightly coupled to the current OpenRewrite/Maven log format; consider making the parsing more robust (e.g., anchoring on explicit markers or using a machine-readable output mode if available) to avoid breakage on minor log format changes. - The new
rewrite-reportjob runsrewrite:runon every workflow invocation, adding a non-trivial Maven execution; consider scoping this job to specific branches or events (e.g.,pull_requestonly, or main-protected branches) to keep CI time under control while still getting the intended visibility.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The awk-based extraction of recipe attribution from `rewrite-run.log` in the `rewrite-report` job is tightly coupled to the current OpenRewrite/Maven log format; consider making the parsing more robust (e.g., anchoring on explicit markers or using a machine-readable output mode if available) to avoid breakage on minor log format changes.
- The new `rewrite-report` job runs `rewrite:run` on every workflow invocation, adding a non-trivial Maven execution; consider scoping this job to specific branches or events (e.g., `pull_request` only, or main-protected branches) to keep CI time under control while still getting the intended visibility.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: cuioss/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 346a03d1-fdb4-44b1-abe1-8519148ecdb6
📒 Files selected for processing (8)
.claude/skills/run-integration-tests/SKILL.md.github/workflows/maven.ymlCLAUDE.mddemo-client/doc/playwright-suite.adocdoc/development/README.adocdoc/development/build-gate-discipline.adocdoc/development/declared-limit-assertion-coverage.adocdoc/development/sonar-quality-gate.adoc
Collaborator
Author
|
/review |
Create doc/development/build-gate-discipline.adoc as a peer of the Sonar quality-gate policy. It records the three mechanisms in this repository that mutate the working tree while the gate exits 0 -- OpenRewrite's import-group churn (and its poisoning of the change-ledger freshness stamp), the pre-commit formatter's non-idempotency, and the SimplifyTestThrows reversion reached via JUnit5BestPractices -- the three consequences a contributor acts on, 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. Add the matching row to the contributor guide's Contents table. Co-Authored-By: Claude <[email protected]>
Add one section to the declared-limit assertion-coverage note carrying the three rules that motivate the matrix: the deletion discriminator (a parsed, schema-validated key with no production consumer is a silent security no-op, tested by asking whether deleting the key would turn anything red, with the note's own inert TLS protocol-floor row as the worked instance); that wiring an inert key is a behavioural flip for every consumer, so the first failing test is a sample of the dependant set rather than the whole of it, and a new assertion extends SingleSourceTlsContractTest instead of splitting the contract across a second file; and that an opt-in runtime feature needs a deployment-activation test, with TlsEdgeActivationWiringTest as the reference shape. The three marked blocks DescriptorInventoryWiringTest parses as its expected inventory are untouched; the integration-tests module suite is green. Co-Authored-By: Claude <[email protected]>
Add one section to the Sonar quality-gate policy stating the two rules that answer the same question from opposite ends. A cosmetic sweep must stay out of the interior of uncovered branches, because Sonar's new-code window is the diff rather than its semantics -- a pure rename inside an uncovered catch block drags pre-existing uncovered lines into the pull request's coverage measurement. A finding inside the scanned surface but outside the declared write-boundary is dispositioned taken_into_account and reported as a named follow-up, never silently absorbed, because the widening is invisible in a gate result that only ever gets greener. Close with the page's own hygiene rule: cross-references here name documents and rules, never file:line anchors. The four-anchor deferral table that belongs here by subject is deliberately not landed -- its anchors no longer resolve and one now points at a comment recording the finding as fixed, so it is recorded as an omission with its reason rather than restored as a stale list. Co-Authored-By: Claude <[email protected]>
… pointers The Playwright doc already carries the browser-policy rule and its generalisation; only the enumerated class was short. Add the two members it omitted -- cookie partitioning, and the navigation context a response arrives in (top-level redirect GET versus cross-site form POST) -- and state that this suite is the only gate in the repository covering that class. The navigation member is the one the page's own worked defect turned on, so the list now matches the story it sits under. Give the run-integration-tests skill the two cross-references it was missing: that a green RestAssured run here is not evidence about that class, and that a trimmed bring-up already has a working script rather than needing a Compose profile. Both are pointers -- each rule stays in the document that owns it. Co-Authored-By: Claude <[email protected]>
CLAUDE.md is the scarcest surface in the repository, so only statements that apply to most work earn a line here. Three do, and each cross-references its detail page rather than restating it. Beside the -Werror prose in the Pre-Commit Process, where a reader already has the build gate in mind: a successful build is not evidence that work happened, and a gate that exits 0 can still have changed your files -- so a review-bot suggestion is verified by surviving the gate rather than by being implemented, and unrelated churn is reverted rather than committed. The mechanisms, the operational consequences and the recipe-scoping trap stay in doc/development/build-gate-discipline.adoc. With the testing standards: a configuration key that parses is not a configuration key that acts, discriminated by asking whether deleting the key would turn any test red. The worked instance stays in doc/development/declared-limit-assertion-coverage.adoc. Co-Authored-By: Claude <[email protected]>
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. Add a rewrite-report job to the Maven workflow that runs the rewriting gate and prints, into the job summary, the post-gate git status --porcelain listing and every "Changes have been made to <file> by:" block with its recipe chain. The job is patterned on supply-chain-scan: no needs:, hardened runner, checkout without persisted credentials, and deliberately non-gating on its own verdict. It carries no continue-on-error, so a genuine Maven or tooling failure still turns the check red; only the dirty verdict is suppressed, and it is suppressed structurally -- no step asserts cleanliness, so there is no failing path to mask. shell: bash is load-bearing on both run steps: it selects pipefail, and without it the tee would swallow a Maven failure. -Ppre-commit is load-bearing too, and the leading comment says so: the recipe list that reaches all three mechanisms is the pre-commit profile's activeRecipes override, and without the profile the goal reports "Using active recipe(s) []" and exonerates OpenRewrite for free. The same comment records why this lane is deliberately not the positively-phrased fitness function ADR-0030 prescribes, and the condition under which it could become one. The job commits nothing and pushes nothing; the mutated tree lives only in the runner. Co-Authored-By: Claude <[email protected]>
The page asserted that main is not at the OpenRewrite recipes' fixed point and recorded reaching it as unpaid debt. A whole-reactor verify -Ppre-commit run drives rewrite:run with the full pre-commit recipe list in every module, produces zero 'Changes have been made to' attributions and leaves the tree clean, so the premise was false. Restate every fixed-point passage as the current observed state plus the drift risk: the fixed point is a property of the tree at a point in time, lost silently the moment a change the recipes want to rewrite lands. The three mechanisms, the three operational consequences and the revert rule are unchanged; the revert rule's reasoning is re-grounded on that drift risk rather than on standing debt.
The job stays a report, not a gate; only its leading comment changes. The old rationale claimed a fail-on-dirty assertion would be red on every run for as long as main is not the formatter's fixed point, which is false - the tree is observed clean at the active recipes' fixed point. The real reason to stay non-gating is that the observation is not a guarantee: a hard gate would block every pull request from the moment any drift lands, including the ones that did not cause it. The comment now also names the condition under which the ADR-0030 fitness-function shape becomes the right call.
The rewrite-report job's ADR-0030 divergence comment restated, near-verbatim, the argument already carried by doc/development/build-gate-discipline.adoc while its own header comment declares that adoc as the home for the story. Collapse it to what is genuinely site-unique: the ADR-0030 summary, the divergence flag, the one-clause reason, the pointer, and the prohibition against converting the job into a hard gate. Comments only, no behaviour change. Also correct doc/development/README.adoc's "twelve-file descriptor surface" to thirteen. The page it describes states thirteen and that count is machine-checked by DescriptorInventoryWiringTest, so the README line was provably stale. Co-Authored-By: Claude <[email protected]>
The rewrite-report job died on its first CI run with
No plugin found for prefix 'rewrite'
after Maven Central answered 429 for org.openrewrite.maven/maven-metadata.xml.
Resolving a goal PREFIX requires that metadata fetch even though the plugin is
declared in the pre-commit profile, so the job carried a network dependency it
does not otherwise need and never reached the gate at all.
Invoke org.openrewrite.maven:rewrite-maven-plugin:run by full coordinates
instead. The version still resolves from the profile's own plugin declaration,
and no prefix metadata is fetched.
Verified locally: the run reports the full 19-recipe active list (not the
empty-recipe false negative the profile omission produces), exits 0, and leaves
the tree clean.
The job stays non-gating for the DIRTY VERDICT only; a genuine tooling failure
turning it red is the designed behaviour, and this run was exactly that.
Co-Authored-By: Claude <[email protected]>
The rewriting gate teed its log to `rewrite-run.log` in the checkout root. That path is not gitignored, so the next step's `git status --porcelain` printed `?? rewrite-run.log` under "Working tree after the gate" — the job whose entire purpose is to attribute tree mutations to OpenRewrite was manufacturing one mutation of its own and reporting it beside the real ones, with nothing to tell a reader the artefact from the finding. It also broke the CLAUDE.md rule that generated files live under `.plan/temp/`. The log now goes to `.plan/temp/rewrite-run.log`, which `.gitignore:20` already excludes, and the awk reads that same path. No new ignore rule. Harden the attribution parser while here. It is coupled to OpenRewrite's log wording and indentation, and that coupling cannot be removed short of reimplementing the plugin's reporting — but the SILENCE can be. When either moves, the matcher stops matching and the empty attribution block reads exactly like "no recipe changed anything": a confident report of nothing having happened. So the header and recipe-line match counts are now cross-checked against the working tree, and two drift shapes are called out as a BROKEN PARSER rather than a clean run — a dirty tree with zero matched headers (wording moved), and headers with no recipe lines beneath them (indentation moved). Both banners name the file to fix and point the reader at the raw gate log meanwhile. The job's non-gating shape is untouched: no `continue-on-error`, no `needs:`, `-Ppre-commit` and the full-coordinate plugin invocation unchanged, and the every-change trigger set deliberately kept (decisions 231a6b, 4d66ab, 671ad2). Co-Authored-By: Claude <[email protected]>
The deletion discriminator overstated its own conclusion: "if the key were deleted entirely, would any test go red? If the answer is no, the control is not in effect, whatever the descriptor says." A green suite after deletion proves the key is not TEST-PROVEN. It says nothing about whether a production reader consumes it, and the inference fails two ways — a production consumer with no asserting test, and a default or inherited value that preserves the behaviour once the explicit key is gone. The page refuted itself three rows up. Its own matrix lists G7 (strict preset path length cap, enforced by `BasicChecksStage.validatePath`), G8 (`tls.alpn`, enforced by `TlsServerCustomizer`) and G9 (`oidc.session.ttl_seconds`, enforced by the session store) each with `none` in the asserting-test column. Applying the one-line test to those three yielded "not in effect" for three controls the same page says ARE enforced — the claim-versus-mechanism defect this document exists to catch, sitting inside the document. The discriminator now stops where its evidence stops: not test-proven, with the two green-but-live shapes named and the G7/G8/G9 rows cited as the instance, and production consumption confirmed separately by tracing the key to its reader. The G11 worked instance stays and gains the second step that actually settled it — the trace found no reader. The sibling paragraph had the same root cause: "Some test fails" assumed a test exists to fail. Where the surface has no asserting test, wiring an inert key flips behaviour with nothing going red at all. Its real point — the first failure is a SAMPLE of the dependant set, not its extent — is unchanged. CLAUDE.md:133 carried the compressed form of the same invalid inference and is restated to match. The pointer must not outrun the text it points at. The silent-security-no-op warning, the matrix rows and the GAP statuses are untouched; only the strength of the deletion test's conclusion was wrong. Co-Authored-By: Claude <[email protected]>
`playwright-suite.adoc:88` called the suite "the only gate in this repository that covers that class". It gates nothing. `.github/workflows/demo-client-e2e.yml` triggers on `push: branches: [main]` and `workflow_dispatch` only — there is no `pull_request` trigger, so a pull request that breaks SameSite, Secure, the __Host- prefix, cookie partitioning, CORS, Referrer-Policy, CSP or navigation context can merge with this suite never having run. The file already said so, 338 lines later: "It is deliberately not attached to any `pull_request` trigger". Line 88 and line 426 could not both be true, and a reader who stopped at the first one came away believing a control was enforced that is not. The mechanism is right and stays. The opt-in design is deliberate and argued in the workflow's own leading comment: the suite builds a native image, starts containers and downloads a browser toolchain, and putting that on the default PR path would slow every PR for a demo-client signal. No `pull_request` trigger is added here. The claim is what changes. Line 88 now says the suite is the only COVERAGE of that class and that it gates nothing, and states the non-gating half at the point of claim rather than 338 lines downstream — so the later passage reads as elaboration instead of refutation. The sibling claim in `.claude/skills/run-integration-tests/SKILL.md:36` gets the same correction. Co-Authored-By: Claude <[email protected]>
…osition Two precision defects in text this branch authored. Neither changes the policy. The cosmetic-sweep section opens correctly — "Sonar's new-code window is the diff" — and then contradicts itself three lines later, claiming a cosmetic edit inside an uncovered branch "drags every pre-existing uncovered line of that branch into the pull request's new-code coverage measurement". Pull-request analysis is scoped to the CHANGED lines; unchanged siblings in the same branch do not enter the measurement. The warning underneath is real and survives intact: a rename or reformat inside an uncovered branch turns the lines it touched into uncovered new code, which is enough on its own to fail the new-coverage condition on lines the change did not author. It is now stated over the changed lines, so the section agrees with its own opening sentence. The prevention remedy is unchanged. The `taken_into_account` passage asserted a disposition and a "named follow-up" without saying where either is recorded, leaving a reader unable to check that the rule is real. It is real, but only half of it is mechanised, and the halves are now stated separately: the disposition is persisted outside this repository, by plan-marshall's triage tooling, in the plan's findings ledger via `manage-findings resolve --resolution taken_into_account`; the named follow-up has NO automated enforcement here — no workflow check, no plan-validation rule — and is labelled a process rule. No CI check is invented to back it. A section whose whole argument is that a green gate cannot report a scope breach must not itself assert an enforcement that does not exist. Co-Authored-By: Claude <[email protected]>
…mment The comment added in 320f09c cited `.gitignore:20` for the rule that excludes the rewrite log from the checkout. The coordinate is accurate today and decays without any signal the moment a line is inserted above it — and this same changeset lands the section in doc/development/sonar-quality-gate.adoc that condemns exactly that: cross-references here name documents, not lines. The claim is preserved; only the decaying coordinate is gone. Co-Authored-By: Claude <[email protected]>
cuioss-oliver
force-pushed
the
feature/plan-v02-17-lessons-into-source
branch
from
August 9, 2026 18:33
f28aee4 to
e03b6a6
Compare
cuioss-oliver
enabled auto-merge
August 9, 2026 18:40
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.
Summary
Lands the twelve
repository sourcelessons drained from theapi-sheriff-roadmapcorpus into thenarrowest document or gate whose reader is the person about to make the mistake. One new
operator-approved page receives the build-gate cluster, five existing documents receive the rest,
and the one mechanism that can be enforced rather than merely stated becomes a non-gating
OpenRewrite dirty-tree report job in CI.
The change is subtractive as well as additive: lessons found already present in repository source at
HEAD are recorded as already-covered rather than restated, and where a rule was substantially
present already, only the missing members of its enumerated class were added — not a second copy of
the rule.
Changes
New page (the one approved new document)
doc/development/build-gate-discipline.adoc— the build-gate cluster: the quality gate'srewriting behaviour and its operational consequences, the single build-invocation rule, and the
two surviving halves of the fixed-point lesson together with the recorded discard verdict for the
half that did not survive verification.
doc/development/README.adoc— one row in the== Contentstable for the new page. The tableindexes every page in the directory, so omitting the row would silently misrepresent the set.
Existing documents
CLAUDE.md— three always-on statements only, integrated into the Pre-Commit Process sectionbeside the existing
-Werrorprose rather than appended blind: the general gate-mutation rule,the class statement it belongs to, and the one-line declared-versus-asserted discriminator.
doc/development/declared-limit-assertion-coverage.adoc— the declared-versus-asserted rulesbehind the coverage matrix, folded into the page that already owns that subject.
doc/development/sonar-quality-gate.adoc— the unowned-duplicated-state rule about line-numbercross-references.
demo-client/doc/playwright-suite.adoc— the browser-policy rule completed to its full enumeratedclass.
.claude/skills/run-integration-tests/SKILL.md— the two owed pointers: the compose--no-depscross-reference, and a pointer to the browser-policy boundary (the rule itself stays in the
Playwright doc, since this skill's subject is the
integration-testsstack, not demo-client).CI
.github/workflows/maven.yml— a newrewrite-reportjob that runs the rewriting gate andreports the resulting dirty tree plus the OpenRewrite recipe attribution. It is deliberately
non-gating, patterned on the existing
supply-chain-scanjob, including theexit-code: 0-not-continue-on-errordistinction that keeps a broken tool red while suppressingthe verdict. No CI lane ran the rewriting gate before this PR, so this job is what first brings
the OpenRewrite mutation under CI observation.
Test Plan
verify -Ppre-commit) green at6a8100a, tree clean post-gaterewrite-reportjob observed on this PR's first CI run (the job is new and has never executed)Related Issues
None.
Generated by plan-finalize skill
Intent
The problem. Twelve hard-won lessons lived only in a drained lessons corpus. A lesson nobody
reads changes no behaviour: the person about to re-make the mistake reads
CLAUDE.md, adoc/development/page, or a CI failure — never the corpus. Landing them is what turns recordedexperience into something that actually intercepts the next occurrence.
The chosen approach. Each rule goes into the narrowest document whose reader is the person at
risk, and nowhere else — stated once, cross-referenced elsewhere, never restated. Where a lesson can
be enforced rather than merely written down, it is enforced: the OpenRewrite dirty-tree finding
becomes a real CI job instead of a paragraph. Every premise was re-probed at HEAD before landing, so
two lessons already satisfied by repository source are recorded as already-covered instead of
duplicated, one refuted claim is discarded with the verdict recorded, and no landed text carries a
file:lineanchor — that anchor style is itself one of the lessons being landed, and this planfound a live instance of it going stale mid-flight.
The new CI job is deliberately non-gating.
mainis not the formatter's fixed point today, so ablocking form would be red on every run and would be disabled within a week; a report that is always
visible and never blocks is the form that survives. It follows the existing
[Intent truncated — 1373 of 2155 characters shown; full outline in the plan workspace]
Summary by CodeRabbit
Documentation
CI