chore: move the layout corpus into test/, execute shape 7's refusal, and give every design page one status label - #332
Open
sunib wants to merge 10 commits into
Open
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 190 files, which is 90 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 (190)
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✅ All modified and coverable lines are covered by tests. 📢 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>
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.
Why
You asked whether the release contains what it promises, whether the examples were all
migrated, and whether we actually execute the shapes we describe. Auditing that turned up
three real defects and one filing problem, all of which are the kind that only show up when
something reads the documents rather than a person.
The release itself checks out: every field-level promise in
UPGRADING.mdis backed by thegenerated CRDs, the types, the chart and the code that consumes them. This PR is the cleanup
around it.
What changed
The layout corpus moved to
test/fixtures/layout-corpus/The shapes and the ecosystem examples lived in
docs/layout/. That was true when they wereprose and stopped being true once
TestLayoutCorpusstarted executing them: every folder isseeded, written through the real plan-then-flush path, and diffed against a committed patch.
They are a test, so they now live with the other executed fixtures.
They land beside
test/fixtures/gitops-layouts/rather than at the top oftest/, becausethose two are mistaken for each other regularly. A new
test/fixtures/README.mdstates thedistinction 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 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. Links were resolved rather than substituted, so the 37 that escape
the moved tree were recomputed and the ones that stay inside it were left alone.
Shape 7's refusal was unexecuted, and wrong
expected-shared-layer-status.yamlwas committed, linked from its README as "the wholeresult", and asserted by nothing. Wiring it in showed it described behavior we do not have.
It claimed the refusal names
layers/observability. The writer saysbase/deployment.yaml,because the layer's patch and the base's Deployment share an identity, so 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 honest conclusion is a negative one, and the
fixture and README now say it: a shared layer above a base does not change the answer.
The fixture was also unreadable by the harness, asserting
Readywhere every other statusfixture asserts
GitPathAcceptedplusStalled, which alone shows nothing had parsed it.Three things stop it recurring:
TestLayoutCorpus_EveryExpectationIsAssertedcloses thecorpus over its expectations (the existing guard was folder-level, so a folder could be
executed and still carry an expectation no row named), the controller's
Stalledprojectionnow covers this fixture, and WatchRule fixtures decode with
UnmarshalStrictlike theGitTargets beside them.
One example was on a field that does not exist
prerequisites/config/gitprovider.yamlsetspec.commit.author. Not a field onGitProvider, pruned in silence on apply. It predates this branch and drifted because thatfolder has no
input/, which is the case the corpus guard deliberately skips.TestExamplesDecodeStrictlycloses that over the filesystem: every checked-in manifest of ourown kinds under
config/samples, the corpus,test/playgroundandtest/e2e/setupisstrict-decoded against the real v1alpha3 types. 45 of 46 were already clean. This matters more
after the breaking wave than before it, because the five fields it removed are pruned rather
than refused.
Every design page now carries one status label
Two header conventions were in use: five pages had YAML frontmatter, 41 had a blockquote
stanza, and two had both and disagreed with themselves. Nothing reads the frontmatter, so its
related:lists had gone stale unnoticed, two of them pointing at a file that does not exist.Labels are now a closed set of five, ordered by lifecycle: design, design, decided,
partly built, built, deferred.
INDEX.mdsaid "Eighteen other open items" above atable of twenty, four of which were labelled built. in their own row. It is now split into
sixteen open and four built-and-kept.
Nothing moved to
finished/. Every shipped design page indesign/is cited by path fromGo source, which is exactly the case the index's own exception exists for. The labels were
what was missing, not the filing.
Validation
task fmt,task generate,task manifests,task vet,task lint,task testandtask test-e2eall pass. Unit coverage 77.0% at baseline. The corpus is now 14 scenarios withone deliberate skip (shape 8's
images:authoring, gated on track C).Not done
docs/layout/shapes/README.mddocumentsgittarget-test.yamland homelab-flux'sgittarget-media.yaml/watchrule-media.yamlas illustrative configs that no scenario runs.Left alone: they are referenced from prose as alternatives, not as expectations.
prose here follows it; fixing the rest would be an unreviewable diff and belongs on its own.
Second half: 805 comment lines cut
Production Go was 27% comments (16,072 of 59,162 lines), with 309 doc-path references
across the source. Three passes, in descending order of how safe each one is.
API design rationale — 185 lines, provably safe
api/v1alpha3carried two comment blocks per field, separated by a blank line. Thesecond becomes the CRD description a user reads through
kubectl explain; the first neverleaves the source — twenty of them opened with the same boilerplate line saying exactly
that. That makes the internal half mechanically separable from the API documentation, and
the cut verifiable rather than argued:
config/crd/andzz_generated.deepcopy.goarebyte-identical after regeneration.
One 18-line block explaining that
commitWindowmoved offGitProviderwas a near-verbatimsecond copy of
UPGRADING.md. It is now two lines ending in a citation. That is the patternthroughout: where a comment duplicated a document, it now cites it.
Write-path and analyzer essays — 411 lines
plan_flush.go,placement.go,store.go,branch_worker.go,overrides_projection.go,scan_repo.go,plan.go,kustomization_parse.go,acceptance.go,fact_index.go,author_fact.go,internal/git/types.go,internal/watch/owner.go.Every invariant that would cost a bug to rediscover survives, in about a third of the
words: why a patch file is a build input and not a manifest, why the allowlist is
filename-based, why tag and digest clear each other in kustomize's own transformer, why
aligning two lists by position is measurably wrong, why a nil
DesiredResourcecannot beskipped in a sweeping planner, why
AttributionNotAttemptedmust be the empty string.Doc references — 47 of 309
Removed where the citation was sequencing, backlog or an investigation log. Kept where the
document is the contract rather than the reasoning:
spec/,facts/,layout/, and thesupport-boundarypages cited by section.What I did NOT cut, deliberately
kubectl explainshows, not sourcecommentary. Cutting them would silently shrink the API reference.
no longer/used to/previouslymatch 243comments, but most state a current fact correctly ("
Namespacedis no longer a publicchoice anywhere in the API"). A sweep would have deleted correct documentation, so every
cut here was read first.
Remaining headroom
~6,700 lines in 694 production comment blocks and ~2,613 in 267 test blocks. The same bar
would yield roughly another 2,000–2,500 lines. This pass took the largest essays in about a
dozen files rather than sweeping all of them.
🤖 Generated with Claude Code