Repository navigation
chore: move the layout corpus to test/, and tidy the docs and comments - #332
Merged
Merged
Conversation
…ainst the real types `docs/layout/specific-examples/prerequisites/config/gitprovider.yaml` set `spec.commit.author`, which is not a field on `GitProvider`. Applied to a cluster it was pruned in silence, so neither a reader nor a cluster could tell. That folder drifted because it has no `input/`, which is exactly the case the layout corpus's coverage guard skips. `TestExamplesDecodeStrictly` closes it over the filesystem instead: every checked-in manifest of our own kinds under `config/samples`, `docs/layout`, `test/playground` and `test/e2e/setup` is strict-decoded against the real v1alpha3 types, so a field the API does not have fails the build rather than disappearing on apply. This matters more after the breaking wave than it did before it: the five fields that release removed are pruned rather than refused, so a stale example is invisible at every layer except this one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… corpus it is confused with The shapes and the ecosystem examples lived in `docs/layout/`, which was true when they were prose and stopped being true when `TestLayoutCorpus` started executing them. Every folder is now seeded, written through the real plan-then-flush path and diffed against a committed patch, so it is a test, and it belongs where the other executed fixtures are. They land at `test/fixtures/layout-corpus/`, next to `test/fixtures/gitops-layouts/` rather than at the top of `test/`. The two are mistaken for each other regularly and the adjacency is the point: a new `test/fixtures/README.md` states the distinction in one table. `gitops-layouts/` is Git-in, real-world shapes we did not write, recording observations and never verdicts; `layout-corpus/` is Git-out, our own configuration and the exact patch we expect. The READMEs travelled with their fixtures. Splitting them would have recreated the drift the corpus exists to prevent: a page describing a folder it no longer sits in is a page that can be wrong without anything noticing. Links were resolved rather than substituted, so the 37 that escape the moved tree were recomputed from the new depth and the ones that stay inside it were left alone. `docs/layout/` keeps the argument (the two contracts Go source cites by path, and the model that produced them) and forwards to the evidence. Also adds `test/fixtures/layout-corpus/README.md`: how to run the corpus, what `-update` is and is not for, the four conventions that make it a specification rather than a snapshot, and how to add a scenario. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… behavior we do not have `expected-shared-layer-status.yaml` was committed, linked from its README as "the whole result", and asserted by nothing. Wiring it into the corpus showed it was also wrong. It claimed the refusal names `layers/observability` and its message reads "the changed field is expressed in apps/checkout/layers/observability, outside this target's path". The writer says `base/deployment.yaml` instead, because the layer's patch document and the base's Deployment share an identity and the manifest store keeps the base and drops the other as a duplicate. The edit is planned against the base and refused for escaping the write scope, which is the same refusal shape 8 produces from a repository with no layer in it at all. The fixture and the README now say that, including the negative conclusion it leads to: a shared layer above a base does not change the answer. The fixture was also unreadable by the harness, asserting a `Ready` condition where every other status fixture asserts `GitPathAccepted` plus `Stalled`. That alone shows nothing had ever parsed it. Three things stop this recurring: - `TestLayoutCorpus_EveryExpectationIsAsserted` closes the corpus over its expectations. The existing guard is folder-level, so a folder could be executed by one scenario and still carry an expectation no row named. An unasserted expectation is worse than an absent one: in review it reads as a pinned behavior and pins nothing. - The controller's `Stalled` projection now covers this fixture too, so all three of its conditions are asserted rather than one. - WatchRule fixtures decode with `UnmarshalStrict`, matching the GitTargets beside them. A stale field in a watchrule.yaml was previously ignored in silence. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…g built work as open Two header conventions were in use. Five pages carried YAML frontmatter (`status:`, `date:`, `related:`) and 41 carried a blockquote stanza; two pages carried both and disagreed with themselves, `watch-manager-ownership.md` reading `status: implemented` in the frontmatter and **built.** in the stanza. Nothing reads the frontmatter, so its `related:` lists had also gone stale unnoticed: two of them pointed at `crd-relationships.md`, which does not exist. The frontmatter is gone and its surviving links are folded into the stanza, which is the form the other 41 pages already used and the only one that renders. The labels are now a closed set of five, ordered by lifecycle: **design**, **design, decided**, **partly built**, **built**, **deferred**. Every page in `design/` carries exactly one, and four pages that had no status at all now say where they stand. `INDEX.md` said "Eighteen other open items" above a table of twenty, four of which were labelled **built.** in their own row. It is now split: sixteen open, and four built and kept in `design/` under the rule the index already states, because Go source cites them by path and `finished/` declares itself non-binding. Nothing moved to `finished/` as a result of this pass: every shipped design page here is cited from Go, which is exactly the case that exception exists for. The labels are what was missing, not the filing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The path substitution left four comments broken mid-clause or running past the column the rest of the file keeps to. No behavior change. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Important Review skippedToo many files! This PR contains 198 files, which is 98 over the limit of 100. To get a review, reduce the PR to 100 files or fewer by splitting it into smaller PRs or changing its base branch. Upgrade to a paid plan to raise the limit. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (198)
You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
…er reaches a reader Production Go was 27% comments. Three cuts, in descending order of how safe they are. **API design rationale (185 lines).** api/v1alpha3 carried two comment blocks per field, separated by a blank line: the second becomes the CRD description a user reads through `kubectl explain`, the first never leaves the source. Twenty of them opened with the same boilerplate sentence saying so. The detached halves are compressed to the fact a reader cannot derive (why *bool, why top level, why the field is retained purely to fail), and one 18-line block explaining that commitWindow moved off GitProvider became two lines citing UPGRADING.md, which already holds that story in full. `config/crd/` and zz_generated.deepcopy.go are byte-identical after regeneration, which is the whole reason this cut is first: it is provable rather than argued. **The densest write-path files (295 lines).** plan_flush.go, placement.go and internal/git/types.go carried multi-paragraph essays where a sentence does. Every non-obvious invariant survives — why pruneMode must be read through OrDefault, why the sweep scope must equal the gathered scope, why intentFor takes the live object and not the event — in about a third of the words. **Doc references (47 of 309).** Removed where the citation was sequencing, backlog or an investigation log: a reader chasing docs/design/target-watch-plan.md from a comment about cell identity learns nothing the comment did not already say. Kept everywhere the document is the contract rather than the reasoning: spec/, facts/, layout/, and the support-boundary pages cited by section, which define behavior the code cannot state. Nothing moved to a doc; nothing was deleted that only existed here. Where a comment duplicated a document, it now cites it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…vious core Second pass, same rule as the first: keep the fact a reader cannot derive from the code, drop the restatement around it. store.go, branch_worker.go, overrides_projection.go, scan_repo.go, plan.go, kustomization_parse.go and acceptance.go. What survives is every invariant that would cost a bug to rediscover — why a patch file is a build input and not a manifest, why the allowlist is filename-based (a real kustomization.yaml has no metadata.name, so a GVK match would never see it), why tag and digest clear each other in kustomize's own transformer, why aligning two lists by position is measurably wrong rather than merely risky, why a nil DesiredResource cannot be skipped in a sweeping planner. What goes is the narration: restating a signature in prose before explaining it, naming the milestone a function shipped in, and re-deriving a conclusion the next sentence states anyway. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Same rule. The tier-precedence argument in Lookup, the sticky removal pointer, why Name came back to AuthorFact after being dropped, and why the shared-snapshot refresh runs off the loop all survive; the narration around them does not. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…lly takes Review found the guard skipping three cases silently, each of which is a way an example goes stale rather than a way it belongs to somebody else: - **malformed YAML** returned nil from the probe unmarshal and was passed over; - **an unknown kind** in our own group fell through the switch to nil; - **an older API version** (`configbutler.ai/v1alpha2`) failed the exact-string match on `apiVersion` and read as an unrelated schema. Every document is now parsed before anything decides to skip it, and the group is compared on the parsed GroupVersion rather than a string match, so a document in `configbutler.ai` must name the version we serve and a kind we register. The neighbouring `examples.configbutler.ai` and `manifestanalyzer.configbutler.ai` groups still skip, which is why the comparison is on the group and not a prefix. Documents are read through k8s.io/apimachinery's YAML reader instead of splitting on "\n---\n". That split was wrong twice over: a document may open with a separator, and the sequence can appear inside a block scalar. Each of the four failure modes was verified by planting a fixture and watching the guard name it, rather than assumed from the code. `TestExampleKindsCoverTheScheme` closes the remaining hole: adding a CRD and an example for it in one change would otherwise leave that example unchecked, since an unregistered kind returns nil. Meta kinds are identified by their package, because apimachinery injects GetOptions and friends into every registered group version. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two review findings, both introduced by the status-stanza pass on this branch. `build-order.md` still opened its corpus section with "it is not wired up yet" and said the fixtures were "read today by nothing but a human: no Go file references either directory". Neither is true on this branch: `TestLayoutCorpus` executes fifteen scenarios over eleven folders, and the fixtures moved under `test/fixtures/` precisely because a test runs them. The section now records where each of its three rules landed, including the one that only came out when the corpus was wired up: shape 7's refusal fixture was committed and asserted by nothing, and turned out to describe a refusal the writer does not produce. `gittarget-configuration-freshness.md` said "Not an active implementation proposal," twice, from folding the old frontmatter into the blockquote. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…eal owners
The page sequenced five in-flight changes across three PRs. All three shipped, so the
question it answered ("what ships together, what can ship alone") no longer has a subject,
and about 150 of its 225 lines were an argument about an order already taken.
It had also gone stale in a way that misleads rather than merely ages: "Track A. All of it
is unbuilt" was false in every part. `spec.suspend`, `spec.placement`, `status.placement`,
`LayoutResolved`, the `Ambiguous` rule, `serializeNamespace`, `useKustomize`, the
one-source-namespace refusal and creating a `kustomization.yaml` are all in the tree.
Nothing is lost, because every open item it tracked is owned elsewhere and was already
written up in more detail there:
- track C, including that steps 2 and 3 shipped and the same one-and-a-half-to-two-week
ballpark, is in support-boundary/patch-authoring.md;
- the `SelfSubjectAccessReview` pass is in source-scope-simplification.md;
- the trimmed riders are in gittarget-api-wave.md.
Five referents moved to those owners rather than being dropped. The load-bearing one is the
corpus's live skip, which names the track's definition of done from Go and is checked by
`task lint-docs`: it now cites patch-authoring.md step 1, which is where that work actually
lives. `model.md` kept its PR table as a record and lost the cross-track pointer, since both
its PRs have shipped and the other two tracks were never dependencies of them.
The generated behavior reference the page proposed is dropped deliberately, not re-homed.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The guide already said how status writes are suppressed. It did not say which fields should
exist in the first place, so the rule was rediscovered per field: once on
status.placement ("No counters, those are metrics, and a counter in status is a status
write per event") and again on resolvedAtRevision, which is deliberately not re-stamped per
scan because that would write status once per commit to the branch.
Stated once now: status writes are bounded by configuration changes and health transitions,
never by data-plane throughput. It is a rule about a field's RATE, not its subject, which is
why a reconcile-request handshake passes (bounded by how often a human pokes an annotation)
and a per-type record does not (grows with types AND is rewritten per scan).
Three reasons, and only the first is cost: a resourceVersion bump invalidates every
watcher's cache, a status write feeds the self-triggering reconcile edge the next section
exists to suppress, and a value that never settles cannot be read as a claim about
convergence.
The table is the shipped fields as worked examples rather than invented ones, so the rule
can be checked against the code that already follows it.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
gitTargetRetentionStatus stamped metav1.Now() when a reported summary carried no ObservedTime. Nothing can produce that summary: reportRetention sets scope.reported and state.observed in the same mutation, and the projection is only reached when anyReported() is true, so a reported summary always carries a real timestamp. The only caller that reached the branch was the test asserting it, which had to construct a summary the producer never emits in order to do so. That assertion is gone; the test keeps its actual subject, which is that an ABSENT retention block and a reported zero mean different things. The invariant is now stated on RetentionSummary.ObservedTime, where it is established, rather than defended in the consumer. Defensive code for an impossible state is worse than none: it makes the state look reachable to the next reader, and the timestamp it would have written is exactly the shape docs/spec/status-conditions-guide.md now forbids, so it also read as a live example of the anti-pattern. Behavior is unchanged. Unit coverage returns to 77.0% from 76.9%, because the branch that went was one nothing executed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
sunib
changed the title
…c lock A reconcile whose status patch is rejected by the optimistic lock publishes nothing, and nothing re-enqueues it: the winning write is status-only and every For() carries a GenerationChangedPredicate to filter those out. The object is left holding an older answer until its periodic requeue, so anything reading it -- a WatchRule mirroring Ready, a spec waiting on a reason to advance -- reads the stale value until then. Two things kept that window open. stall() never asked writeLost(). It returned its gate's result directly and was the only status path that skipped the check, which is why a GitTarget that stalled on "provider not validated yet" kept publishing ValidationFailed for a full steady interval after it had already computed the recovery. It now routes its requeue through st.requeueAfter() like every other path. The recovery cadence also borrowed RequeueStreamSettleInterval (10s), and there is nothing to settle for: the patch was rejected against a resourceVersion this reconcile had already read, so the only thing that must happen before a retry succeeds is the informer cache catching up with a write that is already durable. RequeueWriteLostInterval (100ms) says that, and stops two unrelated cadences sharing one constant -- a coincidence that put the recovery in a dead heat with the 10s condition waits watching the object. Pulling on blockedGate.result showed it carried nothing: every override in the chain was RequeueSteadyInterval, which is already stall()'s fallback. It is now a plain duration set only by the encryption gate, and evaluateValidatedGate, validateProviderAndBranch and checkForConflicts each drop a return value and every ctrl.Result behind it. Also states, in commit(), what the conflict actually is. There is exactly one status writer per kind here and controller-runtime never runs two reconciles for one key concurrently, so it is not contention -- a reconcile reads rv=1 from the cache after its predecessor stored rv=2, and patches if-match rv=1. The old comment said "somebody else wrote the object", which sends the reader looking for a second writer that does not exist. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…eueAfter Both rule kinds returned RequeueStreamSettleInterval directly from the CONVERGING branch of commitRule, which is where a lost write is easiest to miss: a rule still coming up already polls on the fast settle loop, so the cadence looks close enough to correct. It is not the same thing. The settle interval is for watching streams converge; a write the API server rejected needs the cache to catch up, and until the retry lands the rule publishes the older answer — the same stale-reason race this series is closing, ten seconds wide. Each controller now selects its cadence and applies st.requeueAfter once, so the lost-write check cannot be bypassed by whichever branch picked the cadence. CommitRequest is deliberately untouched: it does not use reconcileStatus at all, polling its own interval with Status().Update. Also trims the commit() comment to the invariant. The measured incident it used to recite already lives in TestStatusCommit_LostRaceIsRecordedSoTheCallerComesBack, which is where a reader can act on it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…why one is exempt "Every controller in this package" was wrong: CommitRequestReconciler does not use this helper. The exposure needs BOTH halves — commit() dropping the write and a GenerationChangedPredicate filtering the status-only update that won — so the accurate scope is every controller using the helper. CommitRequest carries neither half, and the reason is worth stating where the next reader will ask it rather than leaving it as an absence, so its Reconcile now records it: no predicate on its For() (a status write re-enqueues it), uncached APIReader (the cache lag that loses the lock cannot re-drive work), and an opposite and deliberate write policy — re-running that reconcile would re-finalize an already-flushed window and mis-report NoOpenWindow, so writeTerminalStatus retries in place and gives up rather than requeue. Also carries the coverage baseline the new tests raised, 77.0% → 77.1%. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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
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.
The corpus is in the end a real test, so it moves out of
docs/intotest/fixtures/layout-corpus/. It sits next togitops-layouts/now, with a small READMEsaying which corpus is which, since those two get confused.
Checking the release against what it promises turned up three things:
spec.commit.author, which is not a field. It applied cleanly and didnothing. There is a test now that strict-decodes every example against the real types, so
a stale example fails the build instead of being pruned in silence.
described a refusal we do not produce. Fixture and README corrected.
their own status. Every page has one label now: design, design decided, partly built,
built, or deferred.
INDEX.mdno longer files built work under "open items".I also cut about 800 comment lines. Mostly the design rationale in
api/v1alpha3that neverreaches the CRD, and the long essays in the write path. Every non-obvious reason survives,
just shorter. Generated CRDs are byte identical.
build-order.mdis deleted: all three PRs it sequenced have shipped, and it still claimedtrack A was unbuilt. Its referents point at the pages that own that work.
The status principle is written down in
docs/spec/status-conditions-guide.md: statuswrites are bounded by config changes and health, never by throughput.
task lint,task testandtask test-e2eall pass.🤖 Generated with Claude Code