feat(analyzer)!: make the scan report a KRM document that says what it knows - #275
Merged
Merged
Conversation
A consumer's onboarding wizard had to split "not supported yet" from "cannot be synced" on the only signal the type offered, a match on one code — and both halves of what it read were wrong. The axis went degenerate when overlay-fan-out-unsupported was retired, and most of the live code space is neither permanent nor pending: a folder refused for invalid-yaml is one broken document from working, and its owner was told to wait for a release. AcceptanceIssue (and the RefusalReason it projects into) now carries a Permanence and, when someone can act, an Actor. Both are set at the RAISE SITE, because only the check that raised the refusal knows: three kinds answer differently depending on which branch fired. - unsupported-kustomize is classified per construct, from the feature set the parse already computed and now carries on the retention: fixable for a build file the author broke, pending for a remote base or Helm inflation, permanent for a generator or a transformer. One code, three answers. - unresolved-krm splits where the registry answer is still in hand: a GVK nothing serves is a CRD the platform operator installs; a GVK that is served but ambiguous, denied, or missing a verb is nobody's to clear today. - refused-structural is classified from the same feature set, so the repo scan and the folder gate cannot hand a consumer two different answers about one directory. - The write path classifies its four too, since they reach a user through GitTarget status with the same guess to make. The tests are the actual ask. TestEveryIssueKindIsClassified reads the IssueKind constants out of the package source, so a new kind fails rather than reaching a consumer as silence; TestAcceptanceEmissionPathsAreClassified drives each emission path through the real gate, which is what a per-constant test cannot do; and TestEveryUnsupportedKustomizeFeatureIsClassified walks kustomize's own struct, so a kustomize bump cannot quietly reintroduce PermanenceUnknown. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…l code The engine defines seventeen refusal kinds; this package re-declared fourteen. Since the projection copies the internal string through verbatim, three codes could reach a consumer that had no exported constant to match on — a consumer doing the correct thing, matching on our published constants, silently failed to recognise all three. All seventeen are now published, and a test reads both sets out of the source so the counts cannot disagree again. Issue and RefusalReason gain Permanence and Actor, so a tool can tell a wait from a redesign from a one-commit fix without inferring it from the code. RefusalReason.Code is now typed IssueKind rather than string, which makes "a candidate's refusal is the acceptance gate's own issue" compile-checked rather than merely stated. The const block above it claimed to enumerate the codes a candidate may carry while listing two of eighteen, one of them retired; it now says what it actually holds — the codes that are NOT issue kinds. BREAKING CHANGE: RefusalReason.Code is typed IssueKind, not string. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…produced it
A JSON document from a binary is a contract with no compile-time signal. A tool
that execs the analyzer holds the document and nothing else: it could not tell
which release produced it, and our own SchemaVersion never said what a bump
asserts or whether a reader should refuse a version it does not know.
Both reports are now KRM documents — apiVersion and kind, the scan request in
spec, the findings in status — so those three questions are answered by citing
the Kubernetes API conventions instead of writing a bespoke policy. apiVersion
replaces schemaVersion outright. The document is never served, never
registered, and carries no metadata: it observes a path at an instant and has
no identity.
status.generator names the build ({name, version}), never empty: ldflags when
the release build sets them, otherwise the module version runtime/debug records
for `go install ...@vX.Y.Z`, otherwise "dev". A bare version string would say
which release without saying which tool, and a piped report is where that
bites.
manifest-analyzer --version prints that release and the report apiVersion
together, so one exec answers both pin questions rather than two execs and an
assumption they were the same binary.
The release job builds, checksums, signs and attests the binary for
linux/amd64, linux/arm64 and darwin/arm64. Both verification mechanisms ship:
`gh attestation verify` is the stronger claim, and a sha256sums file is what a
`curl | sha256sum -c` line in a Dockerfile can consume without a token. The job
also asserts the built binary reports the release it was built from, so a
broken ldflags path fails the release instead of shipping a binary that lies
about itself.
BREAKING CHANGE: schemaVersion is replaced by apiVersion/kind, and the report
fields move under spec/status.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
UPGRADING gets the migration for the breaking JSON change, and the discovery design doc's "report shape as shipped" section now matches what ships: the KRM envelope, and the permanence/actor pair on every refusal reason. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe analyzer now emits versioned KRM-style folder and repository reports, including solvability metadata, rendered type inventories, read graphs, YAML output, and generator provenance. CLI, tests, documentation, fixtures, and release automation were updated accordingly. ChangesAnalyzer contract and classification
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/UPGRADING.md`:
- Around line 34-35: Update the non-applyable report wording in the upgrading
documentation to say it is “not intended to be applied,” while preserving the
surrounding explanation that it is never served or registered and therefore has
no metadata.
- Around line 45-47: Update the documentation text introducing permanence and
actor in UPGRADING.md to avoid claiming that every refusal is classified; state
that refusals may provide these fields or explicitly limit the claim to
classified refusals, while preserving the existing field definitions and
reference.
In `@internal/git/write_refusal_permanence_test.go`:
- Around line 62-104: The new tests use descriptive names without the required
underscore-separated convention. Rename TestUnplaceableEditRefusalIsPermanent,
TestRenderFidelityRefusalIsFixableByThePlatform, and
TestPathScopeRefusalIsFixableByThePlatform to TestFunctionName_Scenario form
while preserving their existing test behavior.
In `@pkg/manifestanalyzer/repo.go`:
- Around line 35-39: Update the comments around the refusal reason constants and
RefusalReason.Code to distinguish values not raised by the acceptance gate from
values that are not of type IssueKind. State that these refusal codes are
declared as IssueKind but originate outside gate-generated Issues, and revise
the Code documentation accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 9ebd49df-8667-4752-9f22-abf2b91e5e06
📒 Files selected for processing (28)
.coverage-baseline.github/workflows/release.ymlcmd/manifest-analyzer/main.gocmd/manifest-analyzer/main_test.godocs/UPGRADING.mddocs/design/support-boundary/repo-discovery-and-onboarding-scan.mdinternal/git/kustomize_oracle_test.gointernal/git/plan_flush.gointernal/git/write_boundary_precondition_test.gointernal/git/write_refusal_permanence_test.gointernal/manifestanalyzer/acceptance.gointernal/manifestanalyzer/analyzer.gointernal/manifestanalyzer/gittargetignore.gointernal/manifestanalyzer/kustomization_parse.gointernal/manifestanalyzer/permanence.gointernal/manifestanalyzer/permanence_test.gointernal/manifestanalyzer/scan_repo.gointernal/manifestanalyzer/store.gointernal/manifestanalyzer/testdata/scan-repo/unsupported/helm-inflation.golden.jsoninternal/manifestanalyzer/testdata/scan-repo/unsupported/plain-nonkrm.golden.jsoninternal/manifestanalyzer/testdata/scan-repo/unsupported/unsupported-kustomize.golden.jsonpkg/manifestanalyzer/contract.gopkg/manifestanalyzer/contract_test.gopkg/manifestanalyzer/doc.gopkg/manifestanalyzer/folder.gopkg/manifestanalyzer/folder_test.gopkg/manifestanalyzer/repo.gopkg/manifestanalyzer/repo_test.go
…he envelope Three review findings. The .gittargetignore guard has two halves and only the parse-time one was classified. A pattern narrow enough to pass the initial denylist can still match a write planned later, and that refusal reaches the same reader with the same question to answer, so it now carries fixable/repository-author too. The new test drives exactly that path: a narrow pattern, no parse-time issue, a planned write that hits it. The layout baseline read the scan report at the top level and got nothing back under the KRM envelope, so every fixture rendered as "no candidate folders reported" — a whole corpus of behaviour silently blanked. It reads through .status now, and records each refusal's permanence in the process: a refusal that changes permanence has changed what a user is told to do about it, which is a boundary move like any other and belongs in this diff. UPGRADING claimed the envelope for `--format json` generally. It is the two published scan modes; `--mode analyze` renders the engine's own structural report, which was never part of the contract. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…n words Permanence asked the reader to hold a taxonomy in their head — permanent, pending-upstream, fixable — before they could read a single refusal. The solvability of a refusal belongs in the data, and the data should answer the question a person actually has: can this be solved? `Solvability` is `yes` or `no`, with `Actor` naming who can solve a `yes`. Making this a field means an issue cannot be emitted without explicitly deciding whether it can be solved and, when applicable, by whom. The third value is gone on purpose. `pending-upstream` bought the reader nothing — a folder they cannot pick today is one they cannot pick today, and the action is identical — while putting a roadmap promise in a machine-readable field, which ages into a lie in one direction or the other. So `no` now means "nobody can solve this with this release", never "not ever": a refusal a later release learns to accept simply reports differently on the next scan, because the consumer reads this value rather than a table it cached. What that gives up is the wait-versus-redesign split; what it keeps is the half that mattered, telling the person on the screen when the fix is theirs to make. Renames throughout: the JSON key, both actor constants, the emission-path tests, the goldens, the layout baseline, and the design record — which keeps the reasoning for the four-value shape it originally argued for, so a later change knows what it is overturning. UPGRADING is rewritten in the present tense, and AGENTS.md now says why: an upgrade guide is read after the release ships, so "Unreleased" and "next minor" are false by the time anyone sees them. BREAKING CHANGE: the refusal field is `solvability` (`yes`/`no`), not `permanence` (`fixable`/`pending-upstream`/`permanent`). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
sunib
changed the title
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/design/analyzer-consumer-contract-asks.md`:
- Around line 232-234: Update the refused-structural row in the contract table
to document construct-dependent yes/no raise-site classification: unparseable
structural refusals are yes for the repository author, while unsupported
constructs remain no. Align the wording with the later explanation and committed
fixtures.
In `@docs/INDEX.md`:
- Line 74: The analyzer contract row in INDEX.md still lists the per-construct
unsupported-kustomize split as open. Update the row for
analyzer-consumer-contract-asks.md to remove that completed item and retain only
the remaining Ask 3 work in its open-items description, preserving the rest of
the entry.
In `@hack/gitops-layouts-baseline.sh`:
- Around line 120-122: The reading rule must account for the renderer’s
supported unclassified solvability output. In hack/gitops-layouts-baseline.sh
lines 120-122, document unclassified as the fallback when solvability is absent;
mirror the same corrected rule in test/fixtures/gitops-layouts/support-today.md
lines 22-24.
In `@pkg/manifestanalyzer/folder.go`:
- Around line 93-95: Update the SolvabilityNo documentation in
pkg/manifestanalyzer/folder.go lines 93-95 to describe that nobody can solve the
specific refusal with the current release, without stating that the folder
cannot be adopted. Apply the same current-release, refusal-scoped wording to
pkg/manifestanalyzer/repo.go lines 62-64, replacing the permanent “can ever stop
being one” implication.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 5a7b1ce3-40f2-48bc-a060-4109ae9cf3af
📒 Files selected for processing (26)
AGENTS.mdcmd/manifest-analyzer/main_test.godocs/INDEX.mddocs/UPGRADING.mddocs/design/analyzer-consumer-contract-asks.mddocs/design/support-boundary/repo-discovery-and-onboarding-scan.mdhack/gitops-layouts-baseline.shinternal/git/kustomize_oracle_test.gointernal/git/plan_flush.gointernal/git/write_boundary_precondition_test.gointernal/git/write_refusal_solvability_test.gointernal/manifestanalyzer/acceptance.gointernal/manifestanalyzer/analyzer.gointernal/manifestanalyzer/gittargetignore.gointernal/manifestanalyzer/scan_repo.gointernal/manifestanalyzer/solvability.gointernal/manifestanalyzer/solvability_test.gointernal/manifestanalyzer/store.gointernal/manifestanalyzer/testdata/scan-repo/unsupported/helm-inflation.golden.jsoninternal/manifestanalyzer/testdata/scan-repo/unsupported/plain-nonkrm.golden.jsoninternal/manifestanalyzer/testdata/scan-repo/unsupported/unsupported-kustomize.golden.jsonpkg/manifestanalyzer/contract_test.gopkg/manifestanalyzer/doc.gopkg/manifestanalyzer/folder.gopkg/manifestanalyzer/repo.gotest/fixtures/gitops-layouts/support-today.md
🚧 Files skipped from review as they are similar to previous changes (9)
- internal/manifestanalyzer/testdata/scan-repo/unsupported/plain-nonkrm.golden.json
- internal/git/kustomize_oracle_test.go
- pkg/manifestanalyzer/doc.go
- internal/git/write_boundary_precondition_test.go
- pkg/manifestanalyzer/contract_test.go
- cmd/manifest-analyzer/main_test.go
- internal/manifestanalyzer/store.go
- docs/UPGRADING.md
- internal/manifestanalyzer/acceptance.go
sunib
changed the title
…a boolean Two changes that arrived together, both about the contract being legible. **You can see the document now.** Nothing in the repo showed one: the *.golden.json files pin the ENGINE's report, which has no envelope, and every public test asserted one field at a time — while the engine's own test comment claimed "pkg/manifestanalyzer owns the machine-readable contract and pins it separately". It did not. Both report kinds now have a checked-in YAML golden, regenerated from a real scan, so a reader sees what they are about to parse and a reviewer sees a change to the envelope as a diff. The folder one is refused two ways on purpose, so both answers appear side by side. `--format yaml` prints the same document, through the report's own writer so the CLI cannot serialize a shape the Go contract would not. The engine modes reject it by name: they render an internal report, and a contract-looking flag must not print a non-contract document. **Solvable is a boolean.** The string enum asked a reader to learn a vocabulary before they could read a single refusal; `solvable: true` with an actor, or `solvable: false`, asks nothing. It also removes a wart the goldens exposed: "yes" and "no" are booleans in YAML 1.1, so the enum had to be emitted quoted forever. What that gives up is worth stating rather than discovering later. The enum's zero value meant "nobody decided", so a raise site that forgot degraded to silence and a test could detect it; `false` is both the honest answer for most refusals and what a forgotten raise site produces, and at runtime the two are now indistinguishable. What still guards it is that the classification table is checked against the SOURCE — a new IssueKind fails until someone decides what it answers, and every unmodelled kustomize construct fails until it appears in the map — so the omission is caught where it is still visible, at the constant. Two consequences follow from the same fact. `solvable` is emitted ALWAYS, never omitempty, so "false" and "absent" stay distinguishable on the wire even though Go cannot tell them apart: a report from a build predating the field is the one case where "nobody said" is readable. And a feature set holding anything unrecognised now reports not solvable, where the enum skipped unknowns and could still answer "solvable" — with no third state, sending an author to fix a typo while an unclassified construct still blocks the folder is the worse error. The design record keeps the enum's argument and says why it was traded away. BREAKING CHANGE: the refusal field is the boolean `solvable`, not the string `solvability`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
sunib
force-pushed
the
feat/analyzer-asks1-2-permanence-and-krm-envelope
branch
from
July 27, 2026 11:11
7aebfac to
1d6b194
Compare
sunib
changed the title
…much A tool provisioning from a folder scan has to settle two things before it acts, and both gate a step that is not cheap to undo. A type the destination does not serve fails the forward apply with "no matches for kind" and then waits for the next resync rather than degrading — so the schema has to be installed before the folder is picked. And replacing a targetNamespace override means naming the exact namespace set, where inferredNamespace is one name and a folder can render into several. Candidate gains Kinds and Namespaces, both as the folder RENDERS. That is the part only this engine can answer: a consumer can scan apiVersion/kind headers itself, but the moment a layout transform is involved the two answers diverge silently, in the direction of provisioning something the writer then refuses. An overlay renders a base it does not contain, into a namespace its own files never mention; kustomize's namespace transformer OVERRIDES metadata.namespace, so the folder that says one namespace lands in another. It costs no new render. The scan already builds every render root to derive the override chains, so the inventory is a projection of objects that are already in hand — kustomize's own output, not our reading of the files. A root that fails to build reports neither set: we cannot know what it renders, and saying nothing is better than guessing. A cluster-scoped object contributes no namespace rather than an empty string, which would read as a namespace named "". Additive: a consumer that does not read the fields is unaffected. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/design/analyzer-consumer-contract-asks.md`:
- Around line 154-161: Update the published RefusalReason example so Code uses
the IssueKind type instead of string, while preserving its JSON field name and
the existing Detail, Solvable, and Actor fields.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 6c07e3ca-d915-45f5-a984-f484a1c202af
📒 Files selected for processing (48)
cmd/manifest-analyzer/main.gocmd/manifest-analyzer/main_test.godocs/INDEX.mddocs/UPGRADING.mddocs/design/analyzer-consumer-contract-asks.mddocs/design/support-boundary/repo-discovery-and-onboarding-scan.mdhack/gitops-layouts-baseline.shinternal/git/kustomize_oracle_test.gointernal/git/plan_flush.gointernal/git/write_boundary_precondition_test.gointernal/git/write_refusal_solvable_test.gointernal/manifestanalyzer/acceptance.gointernal/manifestanalyzer/analyzer.gointernal/manifestanalyzer/gittargetignore.gointernal/manifestanalyzer/kustomize_render_hostile_test.gointernal/manifestanalyzer/kustomize_render_test.gointernal/manifestanalyzer/override_chain.gointernal/manifestanalyzer/overrides_projection_test.gointernal/manifestanalyzer/overrides_test.gointernal/manifestanalyzer/render_inventory_test.gointernal/manifestanalyzer/scan_repo.gointernal/manifestanalyzer/solvable.gointernal/manifestanalyzer/solvable_test.gointernal/manifestanalyzer/source_form_test.gointernal/manifestanalyzer/store.gointernal/manifestanalyzer/testdata/scan-repo/supported/argocd-external-helm-values.golden.jsoninternal/manifestanalyzer/testdata/scan-repo/supported/base-overlays.golden.jsoninternal/manifestanalyzer/testdata/scan-repo/supported/fleet-root.golden.jsoninternal/manifestanalyzer/testdata/scan-repo/supported/flux-helmrelease-values.golden.jsoninternal/manifestanalyzer/testdata/scan-repo/supported/helm-release-document.golden.jsoninternal/manifestanalyzer/testdata/scan-repo/supported/kustomize-single.golden.jsoninternal/manifestanalyzer/testdata/scan-repo/supported/overlapping.golden.jsoninternal/manifestanalyzer/testdata/scan-repo/supported/overlay-nested-base.golden.jsoninternal/manifestanalyzer/testdata/scan-repo/supported/overlay-parked-base.golden.jsoninternal/manifestanalyzer/testdata/scan-repo/supported/plain-per-env.golden.jsoninternal/manifestanalyzer/testdata/scan-repo/unsupported/helm-inflation.golden.jsoninternal/manifestanalyzer/testdata/scan-repo/unsupported/plain-nonkrm.golden.jsoninternal/manifestanalyzer/testdata/scan-repo/unsupported/unsupported-kustomize.golden.jsonpkg/manifestanalyzer/contract.gopkg/manifestanalyzer/contract_test.gopkg/manifestanalyzer/doc.gopkg/manifestanalyzer/folder.gopkg/manifestanalyzer/golden_test.gopkg/manifestanalyzer/repo.gopkg/manifestanalyzer/repo_test.gopkg/manifestanalyzer/testdata/folder-report.golden.yamlpkg/manifestanalyzer/testdata/repo-report.golden.yamltest/fixtures/gitops-layouts/support-today.md
🚧 Files skipped from review as they are similar to previous changes (17)
- internal/manifestanalyzer/testdata/scan-repo/unsupported/unsupported-kustomize.golden.json
- internal/manifestanalyzer/testdata/scan-repo/unsupported/helm-inflation.golden.json
- internal/git/kustomize_oracle_test.go
- docs/INDEX.md
- pkg/manifestanalyzer/contract.go
- internal/git/write_boundary_precondition_test.go
- pkg/manifestanalyzer/doc.go
- internal/git/plan_flush.go
- cmd/manifest-analyzer/main.go
- pkg/manifestanalyzer/repo_test.go
- docs/UPGRADING.md
- internal/manifestanalyzer/acceptance.go
- docs/design/support-boundary/repo-discovery-and-onboarding-scan.md
- internal/manifestanalyzer/analyzer.go
- cmd/manifest-analyzer/main_test.go
- hack/gitops-layouts-baseline.sh
- pkg/manifestanalyzer/repo.go
…namespace
A set of types beside a set of namespaces reads as every combination of the two.
A folder rendering a Deployment into storefront and a Service into payments
published four pairs, two of which exist in no repository — and the stated use
for these fields is one watch rule per (type, namespace), so the union would
have authorized watches matching nothing. The pairing is the answer, not a
detail of how it is spelled.
renderedTypes:
byNamespace:
storefront: [apps/v1/Deployment, v1/Service, v1/ConfigMap]
payments: [apps/v1/Deployment, v1/Service]
namespaceUndeclared: [v1/ConfigMap]
Types are canonical GVK strings, which is GVK.String() and the spelling
summary.byGvk already uses, so the report gains no second vocabulary.
The second correction is a claim I had no right to make. "A cluster-scoped
object contributes no namespace" describes what I inferred; what the code
observes is an object that rendered WITHOUT one, which is either cluster-scoped
or namespaced and relying on the applier's default. Telling those apart needs
API discovery and a structure-only scan has none, so both go to
namespaceUndeclared. A clusterScoped key is reserved for the discovery-aware
scan that can fill it, and until then is ABSENT rather than empty: an empty list
would read as "this folder has no cluster-scoped types", which is the same
species of confident wrong answer as the cross-product it replaces.
A type may appear in both byNamespace and namespaceUndeclared. Two ConfigMaps,
one placed and one floating, is an ordinary folder.
BREAKING CHANGE: candidate.kinds and candidate.namespaces are replaced by
candidate.renderedTypes.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
It was reserved for a discovery-aware scan that does not exist, and omitempty meant it never appeared in a document — so the Go API and the wire disagreed about what the report contains, which is a worse thing to publish than a missing name. Adding it later is a new field and additive, and by then we may want a different shape than a name chosen now for something nobody has designed. The limitation it was standing in for is stated where a reader actually is: on namespaceUndeclared, which is named for what was observed rather than for what might be inferred from it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The shape block still declared RefusalReason.Code as string while the published type is IssueKind — the one thing in that block a consumer would copy verbatim, since mirroring our structs is the design the downstream tool chose. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
It was an implementation brief written against v0.39.1 and read as pending work in docs/design/, where the index lists documents by their OPEN question. It has none left. INDEX draws the line for this repo: prose about code that no longer exists gets deleted, and the rest is kept for why-is-it-like-this. The code this argues about is exactly what shipped, and the reasoning is not reconstructible from it — why the field is a boolean rather than the enum this document proposed, why the report carries no metadata, why classification sits at the raise site, why both attestation and a checksums file. So it moves rather than goes. Annotated, not rewritten. A finished document's value is the record, including the four-value shape that was overturned, so the header says what shipped and where each live definition now is, and leaves the argument intact underneath. The three questions it left open are answered in place. Dropped only the downstream team's stopgap, which described their tree at a moment that has passed. Also: the older UPGRADING entry that introduced schemaVersion now says it was replaced, so a reader landing there is not told to read a field that is gone. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…guess `summary.fleetRoot` required top-level clusters/ + apps/ + infra/, so it read false on the layout Flux's own docs and reference repository use (infrastructure/) and true on any repository that happens to use those three names. The better evidence this doc already prescribed — a flux-system/ directory holding gotk-sync.yaml — is still not enough, and that is why this is a deletion rather than a fix. Fleet-rootness is not a property of a repository: a directory is a cluster entry point because some cluster is POINTED at it, by a FluxInstance.spec.sync.path or a flux bootstrap somebody ran once. Bootstrap a cluster at clusters/staging in Flux's reference repo and nothing in the tree distinguishes it from clusters/production. The same repository is zero cluster entry points, one, or twelve, depending on who syncs it, and a read-only scan sees none of that. Nothing branched on it — isFleetRoot computed it, RepoSummary carried it, the public package copied it, and one line of text printed it — and the repo root is not offered as a candidate either way. Every one of the 19 corpus fixtures reported false, so the regenerated baseline loses only that line. The test that read as though it enforced something asserted a coincidence: the root is not a candidate because it holds no documents of its own, which is what it now says. A field that can only ever be a guess invites consumers to branch on it, and they would be wrong on repositories no test here ever sees. The question returns as NotAnEditingTarget, decided from the documents a folder holds. BREAKING CHANGE: `manifest-analyzer --mode scan-repo` no longer emits status.summary.fleetRoot, the text report no longer prints fleet-root=true, and pkg/manifestanalyzer's RepoSummary has no FleetRoot field. There is no replacement key; the field never changed which folders the scan offers. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
readScope already knew which folders a candidate renders from and did not own. It
reached a consumer as one line of text and no reverse edge, so the report's most
confusing accept — acceptedByOperator with editable 0 — was explicable only by a
reader who already knew to connect two fields.
Adds readBy per candidate and summary.readEdges repo-wide. Both are projections of
one relation, and the corpus-wide test asserts they cannot disagree: the edges are
exactly the union of the readScopes, and every readBy has a matching edge. The
identities are the point — ReachedByMultipleRenderRoots already answered the same
question with the identities thrown away.
readScope now covers every out-of-subtree build input rather than base directories
only. It is the directory projection of renderScopePaths, the function the scoped
acceptance gate renders from, because a build reads a folder through a base, through
`resources: ../shared/deployment.yaml`, and through `patches: [{path: ../../p.yaml}]`,
and only the first is a kustomize base. A second enumeration of "what does this build
load" drifts from the one the writer obeys: the external patch was invisible to the
first version of this graph for exactly that reason.
Both node classes are covered without a second node list. A folder some kustomization
references is never a render root — being referenced is what disqualifies it — so most
edges end at a directory no candidate list mentions, and those are the edge targets
absent from candidates[]: a set difference needing no render knowledge. An earlier cut
published them as readOnlyBases, which was wrong twice over, since a patch directory is
neither a base nor read-only in the sense the name implies. readBy is non-empty in the
remaining case: a plain folder holding a file a distant kustomization lists under
resources: is a candidate another folder genuinely depends on.
Two fixtures, since a field nothing can populate is worse than no field:
shared-file-reference carries both node classes at once, external-patch pins the edge
that only a patch entry creates. The corpus baseline now records the graph, where
`apps/staging reads apps/base/frontend (not a candidate)` explains its own editable 0.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
docs/UPGRADING.md (1)
88-93: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winValidate both
apiVersionandkind.Line 90 only replaces
schemaVersionwithapiVersion, butkinddistinguishesFolderReportfromRepoReport. Validate both before decodingspec/statusto avoid accepting the wrong report shape.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/UPGRADING.md` around lines 88 - 93, Update the Migration guidance to require validating both apiVersion and kind before decoding spec/status. State that apiVersion must be known and kind must match the expected report type, distinguishing FolderReport from RepoReport; retain the existing field-access migration instructions.docs/design/support-boundary/repo-discovery-and-onboarding-scan.md (1)
266-272: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winKeep the report examples on one live schema.
This new section describes the KRM envelope under
status.candidates, but the earlier example at Lines 186-207 still shows a flat pre-envelope object with the retiredoverlay-fan-out-unsupportedreason and nosolvable/actor. Both are presented as first-cut output, so consumers can copy the wrong JSON. Update, remove, or explicitly mark the earlier sketch historical.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/design/support-boundary/repo-discovery-and-onboarding-scan.md` around lines 266 - 272, Update the earlier report example near the initial output sketch to match the shipped KRM envelope and current candidate schema, including status.candidates and refusalReasons fields solvable and actor. Alternatively remove it or explicitly label it historical so readers do not treat the retired flat format or overlay-fan-out-unsupported reason as current.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/design/support-boundary/orchestrator-knowledge-boundary.md`:
- Around line 220-222: Revise the sentence describing NotAnEditingTarget so it
does not claim the question is currently answered. State that it is the intended
place, or would be the place, to determine the result from documents rather than
directory names, while preserving the surrounding explanation about the deleted
guess.
In `@docs/UPGRADING.md`:
- Around line 626-629: Move the historical schemaVersion note from the
Unreleased upgrade section into the appropriate shipped release entry in
docs/UPGRADING.md. Keep the note’s historical wording and context intact, while
ensuring the Unreleased entry contains only current upgrade guidance and no
Unreleased or next-minor labeling for this historical change.
In `@pkg/manifestanalyzer/repo_test.go`:
- Around line 222-251: Replace the tautological final loop in
requireReadGraphConsistent with a reverse-transpose assertion: for each read
edge whose target is a candidate, verify that the target candidate’s ReadBy
contains the edge source. Preserve the existing fromScope/fromEdges and
ReadBy-to-edge checks, and remove readersOf if this is its only remaining usage.
---
Outside diff comments:
In `@docs/design/support-boundary/repo-discovery-and-onboarding-scan.md`:
- Around line 266-272: Update the earlier report example near the initial output
sketch to match the shipped KRM envelope and current candidate schema, including
status.candidates and refusalReasons fields solvable and actor. Alternatively
remove it or explicitly label it historical so readers do not treat the retired
flat format or overlay-fan-out-unsupported reason as current.
In `@docs/UPGRADING.md`:
- Around line 88-93: Update the Migration guidance to require validating both
apiVersion and kind before decoding spec/status. State that apiVersion must be
known and kind must match the expected report type, distinguishing FolderReport
from RepoReport; retain the existing field-access migration instructions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 5d2e4649-6f57-4704-ae6b-4aa77694e5eb
📒 Files selected for processing (29)
cmd/manifest-analyzer/main_test.godocs/INDEX.mddocs/UPGRADING.mddocs/design/support-boundary/orchestrator-knowledge-boundary.mddocs/design/support-boundary/repo-discovery-and-onboarding-scan.mddocs/finished/analyzer-consumer-contract-asks.mdhack/gitops-layouts-baseline.shinternal/git/write_refusal_solvable_test.gointernal/manifestanalyzer/scan_repo.gointernal/manifestanalyzer/solvable_test.gointernal/manifestanalyzer/testdata/scan-repo/supported/base-overlays.golden.jsoninternal/manifestanalyzer/testdata/scan-repo/supported/external-patch.golden.jsoninternal/manifestanalyzer/testdata/scan-repo/supported/external-patch/base/deployment.yamlinternal/manifestanalyzer/testdata/scan-repo/supported/external-patch/base/kustomization.yamlinternal/manifestanalyzer/testdata/scan-repo/supported/external-patch/overlays/test/kustomization.yamlinternal/manifestanalyzer/testdata/scan-repo/supported/external-patch/patches/replicas.yamlinternal/manifestanalyzer/testdata/scan-repo/supported/fleet-root.golden.jsoninternal/manifestanalyzer/testdata/scan-repo/supported/overlay-nested-base.golden.jsoninternal/manifestanalyzer/testdata/scan-repo/supported/overlay-parked-base.golden.jsoninternal/manifestanalyzer/testdata/scan-repo/supported/shared-file-reference.golden.jsoninternal/manifestanalyzer/testdata/scan-repo/supported/shared-file-reference/base/deployment.yamlinternal/manifestanalyzer/testdata/scan-repo/supported/shared-file-reference/base/kustomization.yamlinternal/manifestanalyzer/testdata/scan-repo/supported/shared-file-reference/overlays/test/kustomization.yamlinternal/manifestanalyzer/testdata/scan-repo/supported/shared-file-reference/shared/config.yamlinternal/manifestanalyzer/testdata/scan-repo/supported/shared-file-reference/shared/standalone.yamlpkg/manifestanalyzer/contract_test.gopkg/manifestanalyzer/repo.gopkg/manifestanalyzer/repo_test.gotest/fixtures/gitops-layouts/support-today.md
🚧 Files skipped from review as they are similar to previous changes (7)
- internal/manifestanalyzer/testdata/scan-repo/supported/overlay-parked-base.golden.json
- internal/manifestanalyzer/testdata/scan-repo/supported/overlay-nested-base.golden.json
- pkg/manifestanalyzer/contract_test.go
- pkg/manifestanalyzer/repo.go
- cmd/manifest-analyzer/main_test.go
- test/fixtures/gitops-layouts/support-today.md
- internal/manifestanalyzer/solvable_test.go
Comment on lines
+220
to
+222
| `OverriddenBy` is 05 and 07. `NotAnEditingTarget` is what `fleetRoot` was reaching | ||
| for, decided from documents rather than directory names — and it is now the only | ||
| place that question is answered, the guess having been deleted. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Do not describe NotAnEditingTarget as already answering the question.
This says the question is now answered, while the same document says orchestrator interpreters are not built yet. Use “intended place” or “would answer” to avoid implying shipped behavior.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/design/support-boundary/orchestrator-knowledge-boundary.md` around lines
220 - 222, Revise the sentence describing NotAnEditingTarget so it does not
claim the question is currently answered. State that it is the intended place,
or would be the place, to determine the result from documents rather than
directory names, while preserving the surrounding explanation about the deleted
guess.
"read but never offered" described the section correctly and at twice the length. The
corpus baseline already marks these directories "(not a candidate)", which is the same
fact in the words a reader meets first.
Also clears every Vale suggestion in the two gated docs — six fillers ("actually",
"simply", "really", "just", "genuinely") whose removal changes no sentence's meaning,
which is the test the rule asks you to apply — and one corrective apposition in each
file, restated positively.
Two Correctives warnings remain, one per file, and they are density warnings rather
than defects: the rule budgets three "X, not Y" constructions per file and these hold
18 and 11. Clearing them is a prose pass over docs this branch does not otherwise
touch, and several of the constructions are load-bearing distinctions carrying their
own headings ("Why audit, not admission, attributes a mirrored change"). Left for a
change that is about those documents.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
sunib
changed the title
This was referenced Jul 27, 2026
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.
Implements Ask 1 and Ask 2 of the analyzer consumer-contract brief (now retired to
docs/finished/), the later ask for what is
in a candidate folder, and two follow-up asks: delete the fleet-root guess, and expose the
folder read graph. Ask 3 shipped in #273.
The through-line: the report says what it knows, says what it cannot know, and guesses
nothing. Every change below is one of those three.
Every refusal says whether it can be solved
IssueandRefusalReasoncarrysolvable(always present) andactor(
repository-author/platform-operator, set only when solvable). Both are decided at theraise site, never in a table beside the constants — a map is the thing that drifted, and a
field on the emitted value cannot be raised without a decision. That matters because one code
has two answers:
unsupported-kustomizeis classified per construct: solvable by the author for a buildfile they broke (unparseable, malformed
images/replicas, a patch outside the tree, a rootkustomize cannot build); not solvable for generators, transformers, replacements,
vars,validators, name prefix/suffix, inline patches, and the context this release does not fetch.A folder is solvable only when every construct is.
unresolved-krmsplits where the registry's answer is still in hand: a GVK nothing servesis solvable by the platform operator (install the CRD); a served-but-ambiguous one is not.
It describes this release and promises nothing about the future — read it per scan rather
than caching a mapping from it.
The tests are the ask:
TestEveryIssueKindIsClassifiedreads the constants out of the packagesource, so a new kind fails until someone decides what it answers;
TestAcceptanceEmissionPathsAreClassifieddrives each path through the real gate;TestEveryUnsupportedKustomizeFeatureIsClassifiedwalks kustomize's own struct so a bump cannotadd an unclassified construct.
pkg/manifestanalyzeralso publishes all seventeen kinds — it declared fourteen, and the threea live write raises reached a consumer with no constant to match on.
The report is a KRM document that says what produced it
apiVersionreplacesschemaVersionoutright, so "what does a bump assert" and "must a readerrefuse a version it does not know" are answered by citing the Kubernetes API conventions instead
of writing a policy. No
metadata; never served, never applyable.status.generatoris never empty: ldflags, else the module versiondebugrecords forgo install ...@vX.Y.Z, else"dev".--versionprints that release and the reportapiVersiontogether, so one exec answersboth pin questions.
--format yamlprints the same document, through the report's own writer. Two checked-ingoldens (
pkg/manifestanalyzer/testdata/) show a whole document — nothing in the repo did,and the
*.golden.jsoncorpus pins the engine's report, not this one.linux/amd64,linux/arm64anddarwin/arm64, and fails if the built binary does not report the release itwas built from.
A candidate says what is in it
Both gate a provisioning step that is not cheap to undo: a type the destination does not serve
fails the forward apply with
no matches for kindrather than degrading, and naming allowedsource namespaces (or one watch rule per pair) needs the exact pairs.
The pairing is the contract. A type set beside a namespace set reads as every combination of
the two, which would authorize watches matching nothing. It is what the folder renders, off
the real kustomize build the scan already runs — so a base outside the subtree is included and a
namespace:transformer is applied. It costs no new render.namespaceUndeclaredis not"cluster-scoped": telling those apart needs API discovery this scan does not have, so it is
named for what was observed.
The folder read graph, whole
readScopealready knew which folders a candidate renders from and does not own. It reached aconsumer as one line of text and no reverse edge, so the report's most confusing accept —
acceptedByOperator: truewithresources.editable: 0— was explicable only by a reader whoalready knew to connect two fields.
readScope,readByandsummary.readEdgesare derivedfrom one function, and a corpus-wide test asserts they cannot disagree: the edges are exactly
the union of the readScopes, and every
readByhas a matching edge.readScopenow covers every out-of-subtree build input, not base directories only. It isthe directory projection of
renderScopePaths, the function the scoped acceptance gate rendersfrom, because a build reads a folder through a base, through
resources: ../shared/deployment.yaml, and throughpatches: [{path: ../../p.yaml}]— andonly the first is a kustomize base. An external patch was invisible to the first cut of this
graph for exactly that reason.
is never a render root — being referenced is what disqualifies it — so most edges end at a
directory no candidate list mentions. Those are the edge targets absent from
candidates[], aset difference needing no render knowledge.
readByis non-empty in the remaining case: aplain folder holding a file a distant kustomization lists under
resources:, which is acandidate another folder genuinely depends on, and adopting it is the disruptive choice.
shared-file-referencecarries both node classes at once,external-patchpins the edge onlya
patches:entry creates. The corpus baseline now records the graph, whereapps/staging reads apps/base/frontend (not a candidate)explains its owneditable: 0.summary.fleetRootis deleted, because it was a guessIt required top-level
clusters/+apps/+infra/, so it readfalseon the layout Flux'sown documentation and reference repository use (
infrastructure/) andtrueon any repositorythat happens to use those three names. The better evidence — a
flux-system/directory holdinggotk-sync.yaml— is still not enough, which is why this is a deletion and not a fix:fleet-rootness is not a property of a repository. A directory is a cluster entry point
because some cluster is pointed at it, by a
FluxInstance.spec.sync.pathor aflux bootstrapsomebody ran once, and a read-only scan of the tree cannot see that. The same repository is zero
cluster entry points, one, or twelve, depending on who syncs it.
Nothing branched on it, here or downstream, and the repository root is not offered as a candidate
with or without it — all 19 corpus fixtures reported
false. The question returns asNotAnEditingTarget, decided from the documents a folder holds, inorchestrator-knowledge-boundary.md.
Two calls for you
refused-structuralis classified per construct, deviating from the settled table.Implementing the row verbatim made two surfaces disagree about one directory: a root whose
build file merely does not parse read "not solvable" one way and "the author's to fix" the
other. One-line revert in
refusedStructuralReasonif you disagree — but theunsupported-kustomizesplit collapses with it.kustomize-render-refusedis not solvable. The doc left it open; the narrow claim is thetrue one — the oracle refuses a write it cannot vouch for, and neither side can make it vouch.
Breaking changes
schemaVersion→apiVersion/kind, fields move underspec/status,RefusalReason.Codeistyped
IssueKind, andsummary.fleetRootis gone with no replacement key. Migration indocs/UPGRADING.md, which is written in the present tense with no
Unreleasedmarker — AGENTS.md now says why: an upgrade guide is read after therelease ships.
Validation
task lint,task test(coverage 77.9%, baseline 78.0%, within tolerance) andtask test-e2e(69 passed, 0 failed) green. Two
HouseStyle.Correctiveswarnings remain indocs/architecture.mdand
docs/configuration.md; they are per-file density warnings on documents this branch does nototherwise touch, and every Vale suggestion is cleared.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
manifest-analyzer --version.Documentation