📝 docs: stop the docs teaching API shapes the CRDs reject - #330
Merged
Conversation
…chemas Apply the strict typed decode plus committed-CRD-schema validation that test/samples guards config/samples/ with to every YAML example under docs/: fenced blocks in Markdown pages and standalone .yaml files. Complete objects are validated in full and cannot opt out. Fragments, which are legitimate in docs, must declare a per-block '# kollect-doc:' contract -- undirected fragments fail, and there is no file-level marker. Red on main: docs/crds/kollectclusterscope.md and docs/examples/kollecttarget_trivy-high.yaml carry a spec.sinkRefs the API never had, docs/crds/kollectinventory.md uses the bare-string sink-ref form the CRD rejects, and docs/examples/multi-cluster-fleet.md documents a KollectDatabaseSink that cannot apply.
The YAML schema gate cannot see a removed field in a table row, a mermaid edge label, a glossary entry, or a shell comment -- and a reader copies from a table as readily as from a fence. sinkRefs is the seed case. ADR-0414 split the scope allowlist per sink family; because structural CRD schemas prune unknown fields instead of rejecting them, a scope written from the old docs applies cleanly, loses its allowlist silently, and internal/scope/scope.go then treats the empty allowlist as no restriction at all -- a wider scope than the page depicts, with no error anywhere. Live pages get zero tolerance; docs/adr and docs/rfc may keep the token with same-line superseding framing, matching docs_adr_kollectsink_retcon_test.sh. Red on main across 14 lines in 6 pages.
… sinkRefs KollectScope and KollectClusterScope never had a combined spec.sinkRefs. The scope pages, the trivy example, the glossary and the platform-decisions page all taught it anyway, and a structural CRD schema prunes the unknown field instead of rejecting it: the object applies, the allowlist vanishes, and internal/scope treats the empty allowlist as no restriction. A reader following those pages got a wider scope than the page depicted, silently. Both scope pages now spell out that an empty allowlist means no restriction for that family, which is the part that made the old wording dangerous rather than merely wrong. ADR-0203 and ADR-0413 keep their examples -- retconning a decision record destroys it -- and carry same-line ADR-0414 framing instead.
A partial snippet is legitimate in docs, but 'it is only a snippet' cannot be the reason the gate never looks at it -- that is how a fragment came to teach the bare-string sink-ref form the CRD rejects. Every fragment now names its own contract on its first line: which kind it abbreviates, or why it is not a kollect object at all (Helm values, CI config, workload metadata), or -- on ADR/RFC pages only -- that it records superseded or never-implemented surface. The marker is per-block, so no page can be silenced wholesale, and it renders in the code block, so the reader is told too. ADR-0604 and the attribute-metrics RFC sketch fields that were never implemented; they are marked proposed rather than rewritten. Discovery floors set to the measured corpus: 70 blocks, 29 complete objects, 10 validated fragments.
Four pages taught the bare-string sink-ref form -- including the one directly above a `kubectl apply -k config/samples/advanced/` instruction. The CRDs require objects with a name, so a reader following those pages hits a rejection on the first apply. multi-cluster-fleet.md was worse than a shape error: its KollectDatabaseSink had no spec.type and used postgres.host/database, which the API never had, and its snapshot sink put repoURL and pathTemplate under git: instead of at spec level. Neither object could be created.
task lint:markdown installs docs/node_modules, whose vendored dependabot and workflow YAML flooded the gate with undirected-fragment failures on any host that had run the markdown lint first.
… not impose Naming one family allowlist and leaving the other two empty reads as "export is capped" but leaves those families unrestricted -- the same silent widening this lane exists to close, reintroduced by the previous commit's own fix. The cluster ceiling example, whose whole subject is the ceiling, now populates all three families and says why. The trivy example genuinely caps only snapshot export, so it says that instead of implying more. Neither gate can see this: the schema gate checks shape, the prose gate checks tokens. Recorded as an explicit non-coverage note on the gate.
Step 4 of the Helm-release walkthrough rendered databaseSinkRefs as a bare string four lines above a kubectl apply, and misdescribed the sample it names -- that file uses the object form. Neither gate could see it: gate 1 parses fences only, and gate 2's capital-S guard deliberately excludes databaseSinkRefs. Also: ADR-0413 carried a stray duplicate directive line that rendered as garbage in the published fence, and the cluster-scope page cited internal/scope/scope.go for a claim the cluster path proves in ValidateClusterInventoryClusterScopeSinkRefs (internal/scope/collection.go:230).
Three holes in the gate's own escape hatches, all found by the independent reviewer. A directive was read from the first line of the FENCE and then applied to every document in it, so the current-API KollectTarget sharing ADR-0203's fence with a superseded KollectScope was validated by nothing -- injecting spec.totallyBogusTargetField was green. Directives are now parsed per document; the same injection is red. The fragment floor sat at 8 against a corpus of 10, so two fragments could be reverted to the bare-string form this lane just fixed and relabelled 'kollect-doc: ignore' while staying green. It is pinned at 10: headroom here is not slack, it is the exploit, and nothing verifies an ignore reason is honest. The non-coverage list also pointed at hack/test/docs_scope_sinkrefs_field_test.sh as the compensating control for prose. No such file ships -- the real one is docs_removed_api_fields_test.sh.
hack/docs/verify.sh runs task lint:markdown -- and its npm ci --prefix docs -- before it invokes this gate, so docs/node_modules is present when the gate runs: 167 vendored files against an authored corpus of 130. That broke the contract in both directions. The non-vacuity floor of 100 was satisfiable by build output alone: with every authored page deleted the old find still counted 167 and passed, where the pruned find counts 0 and fails. And the recursive grep swept node_modules too, so a dependency that happened to vendor the token would red the build with no fix available to this repo. Gate 1 already pruned node_modules; that asymmetry was the bug.
Two leftovers from the review round. The Helm-release Step 4 fix replaced one inaccurate shape with another: it quoted databaseSinkRefs as an inline flow mapping, but the sample it cites uses block form. Prose that quotes a rendering has to match the file; this one now describes the contract instead of inventing a rendering, which is what the finding was actually about. minValidatedKollectDocs was left at 26 while the per-document directive fix raised the real count to 30. That is the same headroom-is-an-exploit hole the fragment floor had: four complete examples could be redirected to `superseded` or `proposed` on an ADR page and stay green. Pinned at 30 -- relabelling the ADR-0203 KollectTarget now reds the build.
… ways Two escape hatches from the second review round, both in this file. N1 -- the fence pattern was anchored on the bare "yaml" spelling, so any fence carrying an attribute was never discovered: not validated, not counted, no warning. mkdocs.yml enables pymdownx.superfences and attr_list, so those spellings are idiomatic here and render as YAML, and markdownlint has MD040 disabled with no rule on info strings, so nothing else caught them either. Proven before the fix on a live product page: a fragment with the bare-string databaseSinkRefs form this lane exists to eliminate, and a complete KollectTarget carrying an unknown field, both passed every gate clean. The self-reinforcing part is what made it P1. An author whose fragment failed with "undirected YAML fragment" only had to add a fence attribute for the complaint to disappear -- the gate taught its own bypass -- and no floor could catch it, because an undiscovered block is not counted either. Discovery now reads the first token of the info string. Tilde fences are admitted and the closer widened symmetrically with the opener, so a tilde opener cannot swallow the rest of a page. Fence bodies are no longer rescanned for openers. Pure no-op on the corpus: 70 blocks, 30 complete documents, 10 fragments, all unchanged. N2 -- the two counts described as "pinned" compared with < while their messages said "expected exactly", so additions passed silently and the claim that any corpus change reds the build was false in one direction. They are equalities now, failing both ways, and the constants are renamed off the min prefix to stop promising a bound they do not implement. The fence-variant class is also named in the non-coverage list, because an undiscovered block fails silently and that is the part this repo keeps forgetting.
Three rounds of this defect class, and every regression was found by a human running exploits by hand against the live corpus. The gate validates 130 pages and had no test of its own, so a scanner that quietly stopped recognising a markup variant reported a vacuous green -- and no floor could notice, because an undiscovered block is not validated, not counted, and cannot move the block count either. 28 discovery cases pinning extracted bodies rather than counts, plus unit tables for the info-string parser and the closer rule. Cases that expect NO block are as load-bearing as the rest: over-discovery would red the corpus on ordinary prose. Red on three of them, which are this round's findings: a title attribute containing backticks, a blockquoted fence, and the desync that the first of those produces -- a skipped fence that also swallows every legitimate block after it. Each case carries the reason it exists, so a future edit that changes the behaviour has to argue with a rationale rather than a bare expectation.
The walker enforced CommonMark's "no backtick in a backtick fence's info string" rule. pymdownx.superfences, which is what actually renders this site, does not implement that rule -- it lexes a title attribute containing backticks as YAML like any other block. So the block was skipped. Skipping it was the smaller half. The guard used `continue`, so the walker never advanced past the closer and resumed mid-body: every legitimate block after it on the same page was lost too. Verified on a live page -- a complete KollectInventory carrying an unknown field, immediately after such a fence, passed both gates green. The guard is gone, and the comment now states the invariant it violated: whatever the reason for not collecting a fence, always advance past its closer. Blockquoted fences are now discovered as well. They render as ordinary highlighted YAML and no markdownlint rule forbids them, so leaving them out was a silent hole rather than a limitation. Indent handling for unquoted fences is unchanged byte-for-byte; only a quoted fence takes the new path, which it needs because a blank line in a blockquote is written ">" with no trailing space. The non-coverage comment claimed the title form was already admitted, which was false, and pointed the next fixer at infoStringLanguage -- the one place neither defect lived. It now points at the walker, defers to fence_scanner_test.go as the authoritative list, drops the over-declared HTML-block gap (md_in_html is enabled, so those fences ARE found), and records that attribute lists resolve to their first class. Pure no-op on the corpus: 70 blocks, 30 complete documents, 10 fragments.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
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.
What was wrong
docs/documented asinkRefsfield that does not exist onKollectScopeorKollectClusterScope. ADR-0414 split the sink references per family, and thecombined field went away with it — but the pages kept teaching it.
The failure mode is quiet, and that is what makes it worth a lane of its own:
reader who copied the documented example got
configuredback fromkubectl apply. No warning, no error.allowlist.
internal/scope/treats an empty allowlist as no restriction at all.So the example granted a wider scope than the page depicted, and every layer
between the reader and the cluster reported success. A documentation bug that
resolves to a permission bug.
What this changes
Corpus. Every published YAML example is corrected to the shapes the shipped
CRDs actually accept: the per-family sink allowlists, not the removed
sinkRefs. Prose that named the old field is corrected alongside the examples,and the scope examples no longer depict a ceiling they do not impose.
Two standing gates, so this cannot come back:
test/docs/docs_yaml_schema_test.go— strict typed decode plus validationagainst the committed CRD schemas, over all 70 YAML examples discovered on
the site. Every fragment now declares a
kollect-doccontract so the gateknows what each block is meant to be.
hack/test/docs_removed_api_fields_test.sh— a prose/token gate for removedAPI field names, catching what a YAML gate structurally cannot see: a field
named in a sentence rather than in a fence.
Both are wired into
hack/docs/verify.sh, so Docs CI cannot omit them.And a gate on the gate.
test/docs/fence_scanner_test.gopins the fencescanner itself with 75 assertions. A discovery gate that silently stops
discovering is worse than no gate, because it is green.
Review history
Four review rounds, each of which found a real defect — twice a live
sinkRefspayload sitting on a published page with every gate green. That is the whole
argument for the third test file.
The final round verified the red step exactly (3 of 28 discovery cases failed on
the unfixed scanner), mutated the scanner 13 ways and confirmed 11 are caught by
specific named cases, and render-diffed all 130
docs/**/*.mdpages against theCI-pinned toolchain: 69 discovered blocks vs 69 rendered, zero pages
differing — no undiscovered YAML block exists on any live page.
Seven residual findings are filed forward as DOCS-FENCE-01, matching how
GATE-HARDEN-01 and GATE-COMMENT-01 were handled.
Scope
docs/**,test/docs/,hack/test/,hack/docs/verify.sh. Nointernal/, noapi/, nogo.mod/go.sum.