From d03e91e5667d6e25dbea99040611203d79176ede Mon Sep 17 00:00:00 2001 From: Simon Koudijs Date: Sun, 26 Jul 2026 20:25:47 +0000 Subject: [PATCH 1/6] docs: review the configuration surface against a structured repository MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Record the review of docs/configuration.md and docs/architecture.md as a future-work proposal, and fix the factual errors it turned up: - cluster-scoped resources use the literal `_cluster/`, not `cluster/` (internal/types/identifier.go, internal/manifestanalyzer/placement.go) - the placement section linked the same file twice under two stale `design/manifest/...` labels, one claiming to be a different document - the type-followability link carried a stale label - the design-document list linked architecture.md to itself; it now points at the support contract, which the list was missing The proposal itself argues the engine learned a lot about a folder — layout, render roots, read-only context, ambiguity, a support verdict — while the API still exposes one bit of it, and orders the work that would close that gap. Co-Authored-By: Claude Opus 5 (1M context) --- docs/INDEX.md | 7 +- docs/architecture.md | 7 +- docs/configuration.md | 4 +- ...fig-surface-for-a-structured-repository.md | 406 ++++++++++++++++++ 4 files changed, 418 insertions(+), 6 deletions(-) create mode 100644 docs/future/config-surface-for-a-structured-repository.md diff --git a/docs/INDEX.md b/docs/INDEX.md index 13b64344..f65b8a4f 100644 --- a/docs/INDEX.md +++ b/docs/INDEX.md @@ -90,7 +90,12 @@ grouping strategies nothing else covers. [`ha-gittarget-distribution-plan.md`](future/ha-gittarget-distribution-plan.md) is the HA plan `architecture.md` cites three times (and the reason Redis is required). [`least-privilege-remaining-work.md`](future/least-privilege-remaining-work.md) has -three open RBAC items. Five more ideas sit beside them. +three open RBAC items. +[`config-surface-for-a-structured-repository.md`](future/config-surface-for-a-structured-repository.md) +reviews the configuration docs and argues the API never caught up with what the +folder analysis learned — a look-before-you-write mode, a `status.layout` +projection, an inference switch, and moving `commitWindow` onto the GitTarget. +Five more ideas sit beside them. ## History — [`finished/`](finished/) diff --git a/docs/architecture.md b/docs/architecture.md index 19cb3733..70937804 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -530,7 +530,8 @@ team-a-config/ # GitTarget spec.path The **built-in default** path is `{spec.path}/{namespace}/{group}/{resource}/{name}.yaml`, namespace first, the API group omitted for core resources, no version segment, and a `.sops.yaml` suffix for -sensitive resources; a cluster-scoped resource uses the literal `cluster/` in place of the namespace. +sensitive resources; a cluster-scoped resource uses the literal `_cluster/` in place of the namespace +(an illegal Kubernetes namespace name, so it can never clash with a real one). But that default is only the cold-start seed: a new resource first follows its **siblings'** existing layout, and a `GitTarget` can declare its own placement policy. Details and the placement policy are in [File Placement](#file-placement). @@ -859,7 +860,7 @@ refreshes that source. ### TypeRegistry and followability - **Source**: [internal/typeset/](../internal/typeset/) -- **Design**: [design/manifest/version2/type-followability.md](spec/type-followability.md) +- **Design**: [type-followability.md](spec/type-followability.md) `internal/typeset` is the single decision surface for "can this type be followed?" Each `TypeRecord` carries GVK/GVR identity, scope and preferred version facts, origin classification, subresource facts @@ -1278,7 +1279,7 @@ Deeper dives live under [docs/design/](design/): - [Typeset owns discovery grace](spec/typeset-owns-discovery-grace.md) - [Kubernetes API resource catalog](facts/kubernetes-api-resource-catalog.md) - [GitTarget status design](spec/status-conditions-guide.md) -- [GitTarget lifecycle and repo architecture](architecture.md) +- [The support contract: what we edit, what we refuse, and why](design/support-boundary/support-contract.md) - [Git credentials interop](finished/git-credentials-interop.md) - [SOPS/age key management](finished/sops-repo-bootstrap-and-key-management-architecture.md) - [Commit signing](commit-signing.md) diff --git a/docs/configuration.md b/docs/configuration.md index bf36fbd0..340d4e38 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -661,8 +661,8 @@ The boundaries that keep it predictable: want all ConfigMaps bundled even though none exist yet" is a job for `byType` below. The full ladder, tie-break rules, and edge cases are in -[design/manifest/version2/gittarget-new-file-placement-rules.md](spec/gittarget-new-file-placement-rules.md); -the vision behind it is [design/manifest/file-agnostic-placement.md](spec/gittarget-new-file-placement-rules.md). +[gittarget-new-file-placement-rules.md](spec/gittarget-new-file-placement-rules.md), which also carries +the vision the sibling-inference step serves. #### Declaring a layout (`byType` / `default`) diff --git a/docs/future/config-surface-for-a-structured-repository.md b/docs/future/config-surface-for-a-structured-repository.md new file mode 100644 index 00000000..eba775e7 --- /dev/null +++ b/docs/future/config-surface-for-a-structured-repository.md @@ -0,0 +1,406 @@ +# The configuration surface, now that a folder has structure + +> Status: proposal — review findings and an ordered plan. Nothing here binds until scheduled. +> Date: 2026-07-24 +> Companion to [flux-maintainer-review-status-and-config-model.md](flux-maintainer-review-status-and-config-model.md), +> whose §4 "Then (API surface)" block (F6, F9, F10) this plan absorbs and extends. + +## The one-sentence finding + +The product's premise — *the kube-apiserver writes back into Git* — is sound and worth +building on. What has drifted is that **`spec.path` is still one string**, while the folder +it names has quietly acquired structure: a layout, render roots, read-only context, build +directives, an inferred placement convention, and a support verdict. The operator now knows +a great deal about that folder, and the API offers the user **no way to say what they expect, +no way to see what was understood, and no way to look before writing.** + +Everything below follows from that one gap. + +--- + +## Part A — Review of `docs/configuration.md` and `docs/architecture.md` + +### A1. Factual errors (fixed on this branch) + +| Where | Was | Now | +|---|---|---| +| `architecture.md:533` | cluster-scoped resources use the literal `` `cluster/` `` | `` `_cluster/` `` — verified against `internal/manifestanalyzer/placement.go:418`, `internal/types/identifier.go:66`, and `configuration.md:700`, which all say `_cluster` | +| `configuration.md:664-665` | two links, both labelled `design/manifest/…` (a path layout that no longer exists), both resolving to the *same* file, one of them claiming to be a different document ("the vision behind it") | one correctly-labelled link | +| `architecture.md:862` | link label `design/manifest/version2/type-followability.md` | correct label | +| `architecture.md:1281` | "GitTarget lifecycle and repo architecture" linked to `architecture.md` — i.e. to itself | links to the support contract, which is what that list was missing | + +### A2. `configuration.md` is stale against the CRD it documents + +The GitTarget status list (`configuration.md:515-524`) names `Ready`, `Reconciling`, `Stalled`, +`Validated`, `EncryptionConfigured`, `StreamsRunning`, `GitPathAccepted`. The CRD's own printer +columns (`gittarget_types.go:283-297`) also ship **`RenderMatchesLive`**, **`GitProviderReady`**, +and **`ClusterProviderReady`**. `RenderMatchesLive` is the condition that carries the entire +kustomize-verification story — the newest and least obvious part of the product — and the +configuration guide does not mention it once. A user who hits it has nowhere to read what it means. + +### A3. The structural problem: three documents are wearing one trench coat + +`configuration.md` is ~1150 lines and is simultaneously + +- a **tutorial** (the setup flow, the quickstart pointers), +- a **field reference** (every spec field of six kinds), and +- a **design-rationale record** (why the provider scopes differ, why omitted ≠ empty, why the + sweep fails closed, why `default` is never auto-created). + +The rationale is genuinely good — better than what Flux has written down, as the maintainer +review says — but it is interleaved with the reference at a 1:1 ratio, so the reference is +unusable at speed and the rationale is unfindable on purpose. Concretely, the reader who wants +to answer *"which folder do I point this at?"* has to get through ~460 lines of Git credential +dialects, known-hosts precedence, commit templates and signing before reaching `GitTarget` at all. + +Recommendation (not urgent, but do it before there are users): + +- `configuration.md` → **reference only**: one section per kind, fields, defaults, a table, an + example. Rationale moves out to `docs/spec/`, linked inline as "why:". +- A new `docs/onboarding.md` → **the tutorial**: pick a repo, scan it, pick a folder, apply, verify. + This is where Part B's missing product step lives. +- Credentials interop (`configuration.md:95-136`) is a *guide*, not configuration — it belongs + beside `github-setup-guide.md`. + +### A4. Three surfaces are described as if they were one + +`configuration.md`'s "Audit ingestion settings" section (line 1027 onward) freely mixes: + +- CRD fields (`ClusterProvider.spec.attribution.auditRoute`), +- Helm values (`attribution.enabled`, `queue.redis.addr`), +- controller flags (`--author-attribution`, `--author-attribution-grace`, `--redis-key-prefix`), + +with no visual marker for which is which. `attribution.enabled` and `--author-attribution` are +the *same knob* under two names in the same paragraph. There is a `docs/config-flag-conventions.md` +already; this section should adopt its vocabulary and label every knob with its surface. + +### A5. `architecture.md` is in good shape + +It is long but it is *one* document with one job, and the ground-rules → mental-model → detail +progression works. Two notes: + +- The "Design documents" list at the end (`:1263`) omits the entire + `docs/design/support-boundary/` tree, which is now where the product's most consequential + reasoning lives. `support-contract.md` should be linked from the top of the file, not the bottom. +- "Operational boundaries" (`:1210`) is the most honest section in the repo and should be + linked from the README. It is currently only reachable by reading 1200 lines. + +--- + +## Part B — What the config surface is missing, and why + +### B0. The direction is right; the API did not keep up + +The doubt in the original question — *"analysing git folders, deciding where to place files, +understanding Kustomize … it's all very cool, but it requires more understanding to pick which +folder to reverse"* — is correct, and it is not an argument against the work. It is an argument +that the work **finished in the engine and never reached the API**. + +The engine now answers, per folder: what layout is this, which directories are render roots, +which files are read-only build context, which are build directives, which documents are +ambiguous, is this folder supportable at all, and where would a new resource go. Every one of +those answers exists in `internal/manifestanalyzer` and in the `manifest-analyzer` CLI. The +GitTarget API exposes exactly **one bit** of it: `GitPathAccepted`. + +That asymmetry produces four concrete problems. + +### B1. There is no "look before you write" + +For a product whose job is *writing to your GitOps repository*, first contact is: apply a +GitTarget, and it starts committing. `spec.prune.mode` guards deletions and is well designed — +but nothing guards the *initial* write, which for a populated folder is the scary one. + +**Proposal: `GitTarget.spec.mode`.** + +```yaml +spec: + mode: Observe # Observe | Write (default Write, or Observe — see below) +``` + +`Observe` runs the entire pipeline — watches, acceptance gate, layout analysis, placement +resolution, plan construction — and reports what it *would* do, without touching the remote. +It is the field the current direction most obviously demands, because the folder analysis is +now the interesting part and it is invisible until after the first commit. + +This is **not** `spec.suspend` (F6 in the maintainer review). Suspend means *stop reconciling +this object*; Observe means *keep reconciling and keep telling me, just don't write*. Ship both; +they answer different questions and Flux users expect `suspend` by name. + +Whether `Observe` should be the **default** is the interesting call. Arguments for: a +reverse-GitOps tool defaulting to "writes to your repo on apply" is a startling default, and +there are no users yet to break. Arguments against: it adds a step to the quickstart, which is +currently the product's best asset. Recommendation: **default `Write`, but make the chart's +`quickstart` render `Observe`** — the fast path stays fast, and the deliberate path is safe. + +### B2. A refusal is a boolean, not a diagnosis + +`GitPathAccepted=False` + reason `UnsupportedContent` + a message string is the whole +report. What the operator actually computed — layout kind, render roots, external bases, +tolerated context files, ambiguous documents — reaches the user only through the message, +or through `-v1` logs, or by running a separate CLI against a separate checkout. + +**Proposal: `GitTarget.status.layout`**, bounded and count-based, exactly like the existing +`status.streams` and `status.retention` precedents: + +```yaml +status: + layout: + kind: KustomizeOverlay # PlainManifests | KustomizeRoot | KustomizeOverlay | Mixed | Empty + renderRoots: 1 + externalBases: 1 # rendered as read-only context, outside spec.path + contextFiles: 3 # rendered, never written + buildDirectives: 1 # kustomization.yaml / .sops.yaml — retained, never swept + ambiguousDocuments: 0 # reachable from >1 render root; degrade to plain in-place edit + observedTime: "2026-07-24T…" +``` + +This is the highest-value, lowest-risk item in this document. The analyzer computes all of it +already; this is a projection, not new logic. It turns "the operator did something clever with +my repo" into "the operator says my repo is an overlay with one external base and three context +files", which is the difference between trusting the product and not. + +`ambiguousDocuments` deserves special mention. Per `configuration.md:779-784`, a document +reachable from more than one render root with differing override chains **silently degrades** +to plain in-place editing, recorded only as a store diagnostic at debug verbosity. That is a +correctness-relevant fallback that is currently invisible in the API. It is exactly the class +of surprise the original question is worried about. + +### B3. Inference cannot be turned off + +Sibling inference (`configuration.md:626-661`) is good, careful work — the namespace-agnosticism +proof, the sensitive/plaintext firewall, the deterministic tie-break. But it is **unconditional**. +A user who wants determinism — "if I didn't declare it, use the canonical path, do not read my +repo's mind" — has no field to say so. + +**Proposal: `GitTarget.spec.placement.mode`.** + +| Value | Ladder that runs | +|---|---| +| `Infer` (default, today's behaviour) | `byType` → `default` → sibling inference → canonical | +| `Declared` | `byType` → `default` → canonical. Inference off. | +| `Strict` | `byType` → `default` only; a type with no declared route is **refused**, not guessed. | + +One enum, no new machinery — the ladder already exists, this just truncates it. `Strict` is what +a platform team standardising a repo layout across many clusters will want, and it converts a +silent surprise into a loud condition. + +### B4. `commitWindow` is on the wrong object + +Flagged in the maintainer review (§3) and worth acting on now. The split today: + +| `GitProvider` (namespaced) holds | Which is really about | +|---|---| +| `url`, `secretRef`, `knownHostsRef`, `allowedBranches` | **the connection** ✅ | +| `commit.committer`, `commit.signing` | **the repo's identity on the platform** ✅ | +| `push.commitWindow` | **a workload's write cadence** ❌ | +| `commit.message.*Template` | **a workload's commit style** ❌ | + +Consequence: two GitTargets sharing one repository — a fast-moving app folder and a +cluster-RBAC mirror — cannot have different batching or different subjects. That is not +hypothetical; it is the normal shape as soon as a second GitTarget exists. + +**Proposal:** move `push.commitWindow` and `commit.message` to `GitTarget`. Keep `committer` +and `signing` on `GitProvider` (they are properties of the key registered with the Git host). + +Implementation note, so this is not underestimated: the commit window is owned by the +`BranchWorker`, which is keyed per `(provider namespace, provider, branch)` and **shared across +GitTargets**. But the open window is already keyed by `(author, GitTarget)` +(`architecture.md:974-980`) — so the duration becomes a property of the open window rather than +of the worker. The worker keeps serializing the branch; only the timer moves. That is a real +change but a contained one, and it is materially easier now than after a release. + +Do it in the same breaking wave as F6/F12, not separately. + +### B5. Onboarding is a product step the docs pretend does not exist + +`docs/design/support-boundary/repo-discovery-and-onboarding-scan.md` is explicit and correct: +**the operator never discovers.** A GitTarget is told exactly one subtree. Argo and Flux scan; +we deliberately do not. + +That architectural call is right — and it makes onboarding a **step the product owes the user**, +performed by `manifest-analyzer --mode scan-repo`, which answers precisely the question the +original doubt raises: *which folders can become GitTargets, what layout is each, which are +supported and why not, and what GitTarget/WatchRule would express each one.* + +That CLI is mentioned in `docs/style-guide.md` and `docs/UPGRADING.md`. It appears in +`configuration.md` **zero times** and in `architecture.md` **zero times**. The tool that closes +the gap exists and is undocumented where the gap is felt. + +**Proposal:** `docs/onboarding.md`, step 0 of every non-quickstart install, with the scan output +of a real repo and the GitTargets it suggests. Link it from the README, from `configuration.md`'s +`spec.path` section, and from `architecture.md`'s configuration model. + +### B6. Absorb the outstanding maintainer-review items into this wave + +Still open from the companion review, all API-surface, all cheaper now than after release: + +- **F6** — `spec.suspend` on GitTarget / WatchRule / ClusterWatchRule / GitProvider; + `spec.interval` on GitProvider at minimum; jitter the requeue; + `reconcile.configbutler.ai/requestedAt` + `status.lastHandledReconcileAt`. +- **F10** — CommitRequest lifecycle: `ttlSecondsAfterFinished` or an ownerRef, plus the + `delete` verb. Today they accumulate in etcd forever and the controller cannot reap them. +- **F9** — verify the stored `scope: Namespaced` status-write path on the minimum supported + Kubernetes version (one envtest). +- **F12 remainder** — unify the six near-identical reference shapes onto `fluxcd/pkg/apis/meta` + where they match. + +Plus one this review adds: **the `ProviderNotFound` message for the literal name `default`** +should name the fix (`clusterProvider.createDefault` in the chart, or commit the object). It is +the most likely first-run support ticket and it is a one-line message change. + +--- + +## Part C — Worked examples + +Three repositories, run through today's configuration and through the proposal. These are the +shapes the layout corpus (`test/fixtures/gitops-layouts/`) already covers. + +### C1. Per-environment folders — the `commitWindow` problem + +```text +repo/ + clusters/prod/… # ~40 objects, changes hourly + clusters/staging/… # ~40 objects, changes constantly during the day + platform/rbac/… # ClusterRoles, changes monthly +``` + +Three GitTargets, one GitProvider, one branch. Today they **must** share +`push.commitWindow: 5s` and one set of message templates. + +What you actually want: `staging` batching at `60s` (a `kubectl apply -k` storm should be one +commit, not twelve), `platform/rbac` at `0s` (every RBAC change is individually meaningful and +should be individually reviewable in `git log`), `prod` at the `5s` default. + +Today: impossible without three GitProviders pointing at the same URL — which means three +credential Secrets, three signing configurations, and three `ls-remote` probes per interval +against the same host, for a batching preference. **This is the clearest argument for B4.** + +With B4: + +```yaml +kind: GitTarget +metadata: {name: platform-rbac} +spec: + providerRef: {name: platform-repo} + branch: main + path: platform/rbac + push: + commitWindow: "0s" # every RBAC change is its own commit + commit: + message: + groupTemplate: "rbac: {{.Author}} changed {{.Count}} object(s)" +``` + +### C2. `base` + `overlays` — the folder-choice problem + +```text +apps/podinfo/ + base/ kustomization.yaml + deployment.yaml + overlays/prod/ kustomization.yaml (resources: [../../base], images:, replicas:) + overlays/test/ kustomization.yaml (resources: [../../base], images:) +``` + +**Which folder is the GitTarget?** The API gives no help, and the two plausible answers behave +very differently: + +- `path: apps/podinfo/overlays/prod` — the supported, designed case. The base is read as + read-only context; writes stay in the overlay; a `kubectl set image` lands on the overlay's + own `images:` entry. This is what render-root scoping shipped for. +- `path: apps/podinfo` — **also accepted.** Two render roots inside one target. `base/deployment.yaml` + is now reachable from both, with differing override chains, so per `configuration.md:779-784` + it is **ambiguous** and the writer silently degrades to plain in-place editing of the base — + which means a `kubectl scale` in *prod* rewrites the **base**, changing test too. + +Both report `GitPathAccepted=True` and `Ready=True`. Nothing in `kubectl get gittarget` +distinguishes them. The degradation is a debug log line. + +That is the single most important example in this document, and it is the concrete form of the +original doubt. The proposal addresses it on three sides: + +1. **B5 / scan-repo** tells you `apps/podinfo/overlays/prod` and `…/test` are the GitTargets, + before you apply anything. +2. **B2 / `status.layout`** makes the wrong choice visible after you apply: + `kind: Mixed, renderRoots: 2, ambiguousDocuments: 1`. +3. **B1 / `mode: Observe`** means the wrong choice costs you a status read, not a commit that + changed the wrong environment. + +Optionally a fourth: a `spec.expect.layout: KustomizeOverlay` that refuses when the observed +layout differs. I would **not** ship that yet — `status.layout` first, and see whether anyone +wants to gate on it. Publish the observation before inventing the assertion. + +### C3. A bundle file and a new namespace — the inference problem + +```text +clusters/prod/ + all.yaml # 9 ConfigMaps, one multi-document file + team-a/secrets/db.sops.yaml +``` + +Today (`configuration.md:638-646`): a new ConfigMap is **appended to `all.yaml`**, including one +in a brand-new namespace, because the bundle has proven itself namespace-agnostic. A new Secret +goes to `team-a/secrets/`. Both are the right calls, and both are *inferred from mutable repo +state*. + +The failure mode is not the logic; it is that a **human editing the repo changes the operator's +behaviour without touching any Kubernetes object.** Delete `all.yaml`'s last ConfigMap and the +next new ConfigMap takes the canonical path instead. Nothing in the GitTarget changed. Nothing +in its status says the placement basis moved. + +With B3, a team that finds this unacceptable writes one field: + +```yaml +spec: + placement: + mode: Declared # never read the repo's mind + byType: + v1/configmaps: "{namespace}/configmaps.yaml" + v1/secrets: "{namespace}/secrets/{name}.yaml" +``` + +…and gets the same practical layout, permanently, from a declaration rather than an inference. +Inference stays the default and stays excellent for "point me at an existing repo and it just +works" — which remains the product's best demo. + +--- + +## Part D — Ordered plan + +### Wave 0 — docs, no code (this branch has started it) + +1. ✅ Fix the four factual/link errors (A1). +2. Add `RenderMatchesLive`, `GitProviderReady`, `ClusterProviderReady` to `configuration.md`'s + status list, with one line each (A2). +3. Write `docs/onboarding.md` around `manifest-analyzer --mode scan-repo`, with the C2 repo as + the worked example. Link it from README, `configuration.md`, `architecture.md` (B5). +4. Label every knob in the attribution section with its surface: CRD field / Helm value / + controller flag (A4). +5. Link `support-contract.md` from the top of `architecture.md` and add the support-boundary + tree to its design-document list (A5). + +### Wave 1 — the breaking API wave, before any release + +All of these change or add a spec field. Do them together, in one `feat(api)!` sequence, while +still `v1alpha3` and while there are no users. + +1. **B4** — `push.commitWindow` and `commit.message` move `GitProvider` → `GitTarget`. +2. **B1** — `GitTarget.spec.mode: Observe|Write`; chart quickstart renders `Observe`. +3. **F6** — `spec.suspend` on the four reconciled kinds; `spec.interval` on GitProvider; + requeue jitter; `reconcile.configbutler.ai/requestedAt` + `status.lastHandledReconcileAt`. +4. **B3** — `spec.placement.mode: Infer|Declared|Strict`. +5. **F10** — CommitRequest TTL/ownerRef + the `delete` verb. + +### Wave 2 — legibility, non-breaking, ships any time after + +1. **B2** — `GitTarget.status.layout`, including `ambiguousDocuments`. Highest value per line + of code in this document. +2. The `default` ClusterProvider not-found message names the fix (B6). +3. **F9** — the one envtest. +4. `configuration.md` splits into reference + rationale + guides (A3). + +### Explicitly not now + +- `spec.expect.layout` — publish the observation (B2) before inventing the assertion. +- Making `Observe` the global default — revisit once there is one real user's onboarding to + watch. +- Ordered placement rules (Option A of the placement design) — `byType` + `mode` covers the + demand we can actually see. From bac9ab5ff13719106c41404ca7d06ff4d4deceee Mon Sep 17 00:00:00 2001 From: Simon Koudijs Date: Sun, 26 Jul 2026 21:23:17 +0000 Subject: [PATCH 2/6] docs(analyzer): record three consumer asks against the published contract MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Our first downstream consumer links pkg/manifestanalyzer, execs the CLI from a second tool, and runs the reverser as an image — so it feels every seam in what we publish. All three of its asks are one complaint: a distinction our doc comments assert, carried by prose nothing tests, encoded downstream after it went stale. Written for an implementing party, with each claim checked against the tree: - RefusalReason carries no permanence, and the code space was never two-valued — issuesToReasons sets Code from every IssueKind, so a folder refused for invalid-yaml was reported to its owner as "not supported yet". Includes our classification of all eighteen kinds, which no consumer can maintain, and isolates the three that need a decision rather than a lookup. - The JSON report names no analyzer version, the CLI has no --version, and no binary ships — three pins on one release, none of them recorded. - ResourceIdentifier.Key() is a cross-product identity contract with no test at all; its format can change today with every gate green. Also fixes the stale comment the first ask blames: LayoutKustomizeOverlay still said such folders were "refused today, with a forward-looking reason", four lines above the constant saying the scanner adopts them. That sentence is what the consumer's code was written from. Co-Authored-By: Claude Opus 5 (1M context) --- docs/INDEX.md | 1 + .../design/analyzer-consumer-contract-asks.md | 375 ++++++++++++++++++ pkg/manifestanalyzer/repo.go | 6 +- 3 files changed, 380 insertions(+), 2 deletions(-) create mode 100644 docs/design/analyzer-consumer-contract-asks.md diff --git a/docs/INDEX.md b/docs/INDEX.md index f65b8a4f..b8e06ec4 100644 --- a/docs/INDEX.md +++ b/docs/INDEX.md @@ -71,6 +71,7 @@ Twelve other open items: |---|---| | [`docs-linting.md`](design/docs-linting.md) | how to mechanize [`style-guide.md`](style-guide.md) with markdownlint-cli2 and Vale. Both are wired into `task lint`, gated on the files [`.docs-lint-scope`](../.docs-lint-scope) lists rather than the whole tree: 102 of 174 files fail markdownlint and 148 of 174 fail Vale, so the two backlogs need different gates. Open: how the scope list grows to cover the tree, the `MD013` limit, and whether `AGENTS.md` and the chart READMEs are in scope | | [`attribution-fact-identity.md`](design/attribution-fact-identity.md) | several `ClusterProvider`s may name one physical cluster, but a kube-apiserver posts audit to one route, so only one of those names is ever fed and every other one authors `unknown (attribution unresolved)`. Proposes a declared `spec.attribution.auditRoute` that partitions the facts instead of `metadata.name`, so several providers can share one cluster's facts while cloned clusters stay separate, ingestion loses its last Kubernetes read, and a misrouted provider becomes loud. Renames the key infix and the annotation-key flag to the same word | +| [`analyzer-consumer-contract-asks.md`](design/analyzer-consumer-contract-asks.md) | three asks from the first downstream consumer of `pkg/manifestanalyzer`, each the same failure at a different layer — a distinction our code knows, published as prose no test defends. Permanence on a `RefusalReason` (they shipped "not supported yet" for `invalid-yaml`, because one deprecated code was the only signal the type offered); the analyzer's version in its own JSON report, plus a signed release binary; and `ResourceIdentifier.Key()`'s string format as a documented contract with a golden test — it has **no test at all** today. Carries our classification of all eighteen `IssueKind` values; open: the per-construct split for `unsupported-kustomize`, and whether `Key()` or `ToGitPath()` is the identity when a preferred version bumps | | [`watch-and-catalog-architecture.md`](design/watch-and-catalog-architecture.md) | the target three-layer watch model — **needs a human call before building** | | [`metrics-observability-plan.md`](design/metrics-observability-plan.md) | the watch-stage metrics do not exist yet | | [`reconcile-triggering.md`](design/reconcile-triggering.md) | which controllers still fail to wake up | diff --git a/docs/design/analyzer-consumer-contract-asks.md b/docs/design/analyzer-consumer-contract-asks.md new file mode 100644 index 00000000..033e6bb3 --- /dev/null +++ b/docs/design/analyzer-consumer-contract-asks.md @@ -0,0 +1,375 @@ +# Three asks from a downstream consumer of the analyzer + +> Status: accepted as work, unscheduled. Written for an implementing party — every +> claim below was checked against the tree at `v0.39.1`, and each ask ends with the +> decisions only a maintainer can make. +> Date: 2026-07-26 +> Companion to [config-surface-for-a-structured-repository.md](../future/config-surface-for-a-structured-repository.md), +> which makes the same argument one layer up: the engine learned something and the +> contract did not carry it. + +The three asks come from a product that consumes GitOps Reverser two ways at once — it +links `pkg/manifestanalyzer` as a module, and it execs `manifest-analyzer --mode +scan-repo --format json` from a second tool that does not link us at all. It runs the +reverser itself as an image, pinned to the module by two constants with a build-breaking +test on their side (their `internal/cluster/cluster.go`). That makes it +the first consumer that feels every seam in our published contract, and all three asks +are the same complaint: **a fact our own doc comments assert is carried by prose that +nothing tests, so it drifts silently and a consumer encodes the stale version.** + +One of them already shipped a wrong sentence to a real user because of it. That is the +bar these asks are answering, and it is why the fix in each case is *data plus a test*, +not better prose. + +--- + +## Ask 1 — put the permanence of a refusal in the data + +### What they asked for + +A third field on `RefusalReason`, filled in by the check that raised it, saying whether +the refusal can ever stop being one. Plus the same field on `Issue`, which is the same +taxonomy one level down. + +### Why — verified + +The consumer's onboarding wizard shows a human every folder in their repository and why +each one cannot be picked. *"Not supported yet"* is a wait; *"cannot be synced"* is a +redesign; and the two sentences were split on the only signal the type offers, a match on +one code: + +```go +if r.Code == manifestanalyzer.ReasonRefusedStructural { + fc.Permanent = true // everything else fell through to "not supported yet" +} +``` + +They took that from our doc comments, and both halves of what they read are now wrong. + +**The axis went degenerate.** `ReasonOverlayFanOutUnsupported` is marked `// Deprecated: +no longer emitted` at [`pkg/manifestanalyzer/repo.go:42`](../../pkg/manifestanalyzer/repo.go) +— render-root scoping shipped and external-base overlays are adopted. That was the right +change, and it deleted one side of a two-valued distinction a consumer had already +encoded, with no signal that it had happened. + +**The stale comment that caused it is still there.** `LayoutKustomizeOverlay` at +[`pkg/manifestanalyzer/repo.go:23`](../../pkg/manifestanalyzer/repo.go) says such folders +are "Refused today, with a forward-looking reason", four lines above the constant saying +the scanner now adopts them. That is the sentence their code was written from. **Fixed in +the same commit as this document** — but the fix is worth nothing on its own, because +nothing stops the next one. + +**The code space was never two-valued.** `RefusalReason.Code` is populated from exactly +two places: the literal `ReasonRefusedStructural`, and `issuesToReasons` +([`internal/manifestanalyzer/scan_repo.go:409`](../../internal/manifestanalyzer/scan_repo.go)), +which sets `Code: string(iss.Kind)` for **every** acceptance issue. So the live value set +is the eighteen `IssueKind` constants plus `refused-structural` — and most of them are +neither permanent nor pending. They are **fixable today by the person looking at the +screen**. A folder refused for `invalid-yaml` is one broken document from working, and its +owner was told "not supported yet". + +They are correct that no consumer can maintain this table outside our repo. We added +fifteen codes without any of them carrying permanence. + +### The shape + +Additive, and inert for anyone who ignores it: + +```go +// Permanence says whether a refusal can ever stop being one. It is set by the check +// that raised the refusal, because only that check knows. Consumers MUST treat an +// unrecognised or absent value as PermanenceUnknown and say nothing about the future. +type Permanence string + +const ( + PermanenceUnknown Permanence = "" // not classified; say nothing + PermanenceFixable Permanence = "fixable" // change the repo or the GitTarget + PermanencePending Permanence = "pending-upstream" // a future release may accept it + PermanencePermanent Permanence = "permanent" // the support boundary; never a "not yet" +) + +// Actor names who can act on a fixable refusal. Empty when unclassified or when +// nobody can act. +type Actor string + +const ( + ActorUnknown Actor = "" + ActorAuthor Actor = "repository-author" // the person who owns the files + ActorPlatform Actor = "platform-operator" // the person who owns the GitTarget +) + +type RefusalReason struct { + Code string `json:"code"` + Detail string `json:"detail"` + // Permanence is empty when the check did not classify itself. + Permanence Permanence `json:"permanence,omitempty"` + // Actor is empty unless Permanence is PermanenceFixable. + Actor Actor `json:"actor,omitempty"` +} +``` + +`Issue` takes both fields for the same reason: a policy that treats an issue as blocking +is making this decision already, without the data to make it. + +Ship the actor field. The consumer offered it as optional; it is the cheaper half of the +same constant, and two of our codes are fixable **only** by the platform operator, whom +their wizard does not have on the screen. Without it, `out-of-scope` renders as "fix your +repository" to someone who cannot. + +### The classification — our answers + +This is the part no consumer can write, so it is written here in full. Derived from each +kind's own doc comment and its raise site. + +| Code | Permanence | Actor | Basis | +|---|---|---|---| +| `invalid-yaml` | fixable | author | the document does not parse | +| `duplicate-identity` | fixable | author | two documents claim one identity | +| `impure-managed-file` | fixable | author | split the file | +| `mixed-managed-allowlisted` | fixable | author | move the kustomization to its own file | +| `ignore-shadows-managed` | fixable | author | narrow the `.gittargetignore` pattern | +| `non-krm-yaml` | fixable | author | remove it, or ignore it | +| `foreign-file` | fixable | author | remove it, or ignore it | +| `foreign-symlink` | fixable | author | the *rule* is permanent, the *folder* is not — remove the link | +| `foreign-submodule` | fixable | author | as above; relocate the submodule | +| `out-of-scope` | fixable | **platform** | widen the GitTarget's scope | +| `write-escapes-scope` | fixable | **platform** | widen `spec.path`, or re-place the write | +| `render-does-not-match-live` | fixable | **platform** | a diverged live value is out-of-band substitution, not a render artifact | +| `write-fan-in` | **pending** | — | the doc comment says per-render-root scoping generalizes this | +| `unplaceable-edit` | **permanent** | — | its comment argues the alternative is measurably wrong, not merely risky | +| `refused-structural` | **permanent** | — | the support boundary by definition | + +Note what the `foreign-symlink` row settles, because it is the rule for every future +check: **permanence classifies the folder's prospects, not the rule's.** A rule we will +never relax can still produce a refusal the author clears in one commit. Classify what the +reader can do, since that is the sentence the field exists to write. + +### Three that need a decision, not a lookup + +**`unsupported-kustomize` — must be classified per construct at the raise site.** This is +the case that proves the whole ask: one code, both answers. Its doc comment lists +generators, components, Helm inflation, replacements, transformers, name prefixes and +remote bases; `v0.37.0` shipped exactly the kind of loosening that moves one of those, and +`patches:` already moved. Suggested starting split, for the implementer to confirm +construct by construct: + +- remote bases, Helm inflation → `pending-upstream` +- generators, replacements, transformers, name prefix/suffix → **decide**; they are the + constructs whose output the writer cannot map back to source at all, which reads + permanent, but so did `patches:` before it moved + +A static per-code map cannot express this. The field can, and it is why the field goes on +the emitted reason rather than into a table beside the constants. + +**`unresolved-krm` — splits at the raise site.** A kind absent because its CRD is not +installed is `fixable`/`platform`. A kind that is ambiguous, unserved, or missing a verb is +`pending` at best. One code, two answers, same argument as above. + +**`kustomize-render-refused` — decide what it classifies.** It refuses a *write*, not a +folder, so "can this folder ever be picked" is the wrong question for it. Either classify +it `permanent` (the oracle will always refuse a write it cannot vouch for) or leave it +`Unknown` deliberately and say so in its comment. + +### Implementation notes + +- The constant goes at the **raise site**, not into a map beside the type. A map is the + thing that drifted; a field on the emitted value cannot be raised without a decision. +- `issuesToReasons` becomes the projection for both new fields — it is already the single + choke point through which every issue becomes a reason. +- Add a test that **every** `IssueKind` constant is classified, failing on a new + unclassified kind. That test is the ask. Without it this document is prose again in two + releases, which is how we got here. +- `PermanenceUnknown` must stay the zero value so an unclassified path degrades to silence + rather than to a confident wrong sentence. + +### Meanwhile, downstream + +They have flipped their default from "not supported yet" to unknown — an unrecognised code +now says only "this folder cannot be picked" — and deleted their `Permanent bool`. That is +worse for their user than the truth and is the honest maximum from `{Code, Detail}`. + +--- + +## Ask 2 — a JSON document from a binary is a contract with no compile-time signal + +### What they asked for + +Four things, in their order of value: + +1. The analyzer's own version **in the report**. +2. A `manifest-analyzer --version` flag. +3. One sentence on what a `SchemaVersion` bump asserts, and whether a reader should refuse + a version it does not know. +4. The analyzer binary attached to the release we already sign. + +### Why — verified + +Our package doc invites this consumer by name: *"Exec the binary if Go is not your +language; import this package if it is."* When they link the package, a rename is a build +failure. When they exec the binary, the document is all they hold, and it does not say +what produced it. `RepoReport` and `FolderReport` carry `schemaVersion` and nothing else +about provenance; the flag set in `cmd/manifest-analyzer/main.go` is `--mode`, `--format`, +`--policy`, `--kubeconfig`, `--context`, with no `--version`. + +Their exec'd binary is a **third** pin on the same release, alongside the module and the +image that their build already guards. It is installed by `go install` and recorded +nowhere. A report from one release consumed against a writer from another is a wrong +answer that looks entirely normal. + +`--version` alone does not close that — it is a second exec and an assumption that it was +the same binary. **The field in the report is the fix.** The flag is the convenience. + +### The shape, and why it is nearly free + +The machinery exists. [`cmd/buildinfo.go`](../../cmd/buildinfo.go) holds +ldflags-injected `version`/`gitCommit`/`buildDate` and serves them on `/build-info`; the +`Dockerfile` passes `-X main.version=${VERSION}`. + +For the `go install github.com/ConfigButler/gitops-reverser/cmd/manifest-analyzer@vX.Y.Z` +path our own package doc recommends, **ldflags do not apply at all** — +`runtime/debug.ReadBuildInfo()` returns the module version for free, with no build change +and no release-workflow change. Use ldflags when set, fall back to `ReadBuildInfo`, and +emit `"dev"` for a plain `go build`. + +```go +type ReportProvenance struct { + // AnalyzerVersion is the release that produced this report: "v0.39.1", or "dev" + // for an unreleased build. Informational — do not gate on it. + AnalyzerVersion string `json:"analyzerVersion,omitempty"` +} +``` + +Both reports take it. Adding a field does not bump `SchemaVersion`, which is exactly what +our own stability note tells consumers to expect. + +### On `SchemaVersion` — the missing half + +Our doc states the consumer's duties well: pin a version, ignore unknown fields, do not +switch on prose. It never states what a **bump asserts**. Write that sentence, and answer +these three: + +- Does a bump mean fields were removed, that a field's meaning changed, or either? +- Should a reader that knows `v1` hard-fail on `v2`, or attempt a best-effort parse? +- Since adding a field never bumps it, what is left that *does*? + +Until that exists every consumer's version handling is a guess, and they will not all +guess alike. + +### On the release asset + +Their devcontainer pins every tool from a release asset — `task`, `kubectl`, `kustomize`, +`helm`, `k3d`, `flux`, `flux-operator` with `sha256sum -c`. The analyzer is the only one +that cannot be, because we publish no binary; the tool that runs it does `go install` from +source. That is the single unverifiable link in an otherwise pinned toolchain, and it is +the tool deciding which folders they offer a tenant. + +They explicitly do **not** want a `sha256sums` file. `v0.39.1` already ships `crds.yaml`, +`install.yaml` and `sbom.spdx.json`, each with a `.intoto.jsonl` provenance attestation +and a `.sigstore.json` bundle ([`.github/workflows/release.yml`](../../.github/workflows/release.yml)). +Adding the analyzer binary to that job makes it verifiable with `gh attestation verify` — +stronger than a checksum, and machinery we already run. + +Decision for the implementer: **which platforms.** `linux/amd64` and `linux/arm64` covers +their devcontainer; `darwin/arm64` is the obvious third. Each is another matrix leg on a +job that already signs. + +--- + +## Ask 3 — `ResourceIdentifier.Key()` is a cross-product identity contract + +### What they asked for + +Document the **string format** of `ResourceIdentifier.Key()` where a consumer can find it, +and put a golden test on the exact strings **in our repo**. + +**Explicitly not: export the type.** Worth recording why, because the generous answer is +the wrong one. Importing anything from our module puts a `require` line in *their* go.mod, +and minimal version selection then raises *their* build to whatever that tool pins — +defeating the exact-release pin their build test guards. A mirrored struct plus a format +test is their design, not a stopgap. **A stable documented format is worth more to them +than an importable type, and costs us less.** + +### Why — verified + +Two products must agree on what "the same resource" is or every join between them is +silently wrong: one keys a row on our identity, the other reports a verdict about it. +Today that agreement is enforced by a byte-for-byte test **on their side**, against a type +in **our** `internal/`. It breaks for them, late, after a version bump they chose, and +never for us. + +And they are right that we have nothing. +[`internal/types/identifier_test.go`](../../internal/types/identifier_test.go) covers +`ToGitPath`, `IsClusterScoped` and `String`. **`Key()` has no test at all.** We can change +its format today and every gate stays green. + +### Be specific about which `Key()` + +There are two, and "the key format" is ambiguous: + +```go +// internal/types/identifier.go:40 — the one this ask is about +func (r ResourceIdentifier) Key() string // "{group}/{version}/{resource}/{namespace}/{name}" + // cluster-scoped: the namespace segment is dropped + // core group: empty, so the key leads with "/v1/secrets/…" + +// internal/types/reference.go:40 — not this one +func (r ResourceReference) Key() string // "namespace/name" +``` + +The golden test pins all three shapes, because those are the cases a reimplementation gets +wrong: + +| case | expected | +|---|---| +| namespaced, grouped | `apps/v1/deployments/prod/api` | +| cluster-scoped, grouped | `rbac.authorization.k8s.io/v1/clusterroles/admin` | +| namespaced, core group | `/v1/secrets/prod/db` | + +The four-segment cluster-scoped form is the sharp edge: `Key()` **drops** the namespace +segment rather than emitting an empty one, so a naive reimplementation that always joins +five parts produces `…/clusterroles//admin` and never joins. + +### The trap worth naming + +`Key()` includes `Version`; `ToGitPath()` deliberately excludes it, for a documented and +correct reason — the operator writes one version per object, so a version segment would +churn the path on a preferred-version bump. + +Both are right for their own job. Together they mean **our two identity functions disagree +about whether a preferred-version bump is the same resource.** A cross-product join keyed +on `Key()` splits in two when a CRD's storage version moves, while the Git path correctly +does not move at all. + +Decide which of the two **is** the identity and say so where both are defined. The +consumer would rather have a versionless key beside the current one than build their own — +which is the cheap answer if the decision goes that way, and it also removes the last +reason for anyone to reimplement either function. + +### Scope + +Small, and none of it constrains a pre-1.0 module: + +- Godoc on `Key()` naming the format as public, with the three shapes. +- A table-driven golden test on the exact strings, in `internal/types`. +- One line in the package doc of `pkg/manifestanalyzer` pointing at the format, since that + is where a consumer looks. +- The version-identity decision, recorded beside both methods. + +A golden test in our repo turns their late breakage into a red CI run at the moment +someone makes the change, which is where the decision is being taken. It forbids nothing. +It makes the change deliberate. + +--- + +## Why these three belong together + +Each is the same failure at a different layer: a distinction our code knows, published as +prose no test defends. + +- Ask 1 — the check knows whether a refusal is forever; the type carries a code. +- Ask 2 — the binary knows its version; the document carries a schema marker. +- Ask 3 — `Key()` is an identity contract; the format lives in a comment. + +Each fix is a value plus a test that fails when the value stops being true. The cost is a +constant per site. What it buys is the end of a class of bug where we are correct, the +consumer is careful, and the user still gets a wrong sentence. diff --git a/pkg/manifestanalyzer/repo.go b/pkg/manifestanalyzer/repo.go index 8144f9fc..b40a7a05 100644 --- a/pkg/manifestanalyzer/repo.go +++ b/pkg/manifestanalyzer/repo.go @@ -21,8 +21,10 @@ const ( // resources graph stays within its own subtree. Accepted. LayoutKustomizeSingle Layout = "kustomize-single" // LayoutKustomizeOverlay is a render root reaching a base outside its own subtree - // (the classic base/ + overlays/{env} shape). Refused today, with a forward-looking - // reason: it becomes accepted when render-root scoping ships. + // (the classic base/ + overlays/{env} shape). Accepted: render-root scoping shipped, + // so the base is read as read-only context and writes stay in the overlay. The + // candidate's editable count shows how much of what it renders it can own. An overlay + // refused for a real fault carries that fault's own code, never a forward-looking one. LayoutKustomizeOverlay Layout = "kustomize-overlay" // LayoutRefusedStructural is a render root whose kustomization uses a construct the // writer cannot map back to editable source. This is the permanent support boundary, From 793f156a2e6d09b2689b00f2454412d5c7936a94 Mon Sep 17 00:00:00 2001 From: Simon Koudijs Date: Mon, 27 Jul 2026 04:06:31 +0000 Subject: [PATCH 3/6] docs(analyzer): fix the kind count, the version field, and the inference example MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review findings from PR #272, all four verified against the tree before fixing: - The kind count was wrong and hid a real gap. internal/manifestanalyzer defines seventeen IssueKinds, not eighteen, and pkg/manifestanalyzer re-declares only fourteen — so kustomize-render-refused, render-does-not-match-live and unplaceable-edit can reach a consumer through issuesToReasons with no exported constant to match on. Recorded as a second gap to close in the same pass. - A per-constant classification test cannot reach the per-construct branches of unsupported-kustomize, unresolved-krm and kustomize-render-refused, which is the original bug one level down. The ask now requires per-emission-path coverage. - analyzerVersion was specified as a nested, omitempty field, which contradicts the ask it serves: a report must always say what produced it. It is now a non-empty top-level string on both reports, with the fallback chain stated. - The C3 inference example did not satisfy its own precondition. A cohort admits a never-seen namespace only once it holds more than one (configuration.md 652-655), so a bundle of nine ConfigMaps in one namespace inverts the answer. The example now spans two namespaces, and its failure mode is the sharper one: the file is still there and still full of ConfigMaps. Co-Authored-By: Claude Opus 5 (1M context) --- docs/INDEX.md | 4 +- .../design/analyzer-consumer-contract-asks.md | 59 ++++++++++++++++--- ...fig-surface-for-a-structured-repository.md | 23 +++++--- 3 files changed, 68 insertions(+), 18 deletions(-) diff --git a/docs/INDEX.md b/docs/INDEX.md index b8e06ec4..e5a05645 100644 --- a/docs/INDEX.md +++ b/docs/INDEX.md @@ -65,13 +65,13 @@ says what we support and refuse** — and then its kustomize field taxonomy, the write boundary, the orchestrator/expansion line, and how secrets are handled. -Twelve other open items: +Thirteen other open items: | Doc | Open question | |---|---| | [`docs-linting.md`](design/docs-linting.md) | how to mechanize [`style-guide.md`](style-guide.md) with markdownlint-cli2 and Vale. Both are wired into `task lint`, gated on the files [`.docs-lint-scope`](../.docs-lint-scope) lists rather than the whole tree: 102 of 174 files fail markdownlint and 148 of 174 fail Vale, so the two backlogs need different gates. Open: how the scope list grows to cover the tree, the `MD013` limit, and whether `AGENTS.md` and the chart READMEs are in scope | | [`attribution-fact-identity.md`](design/attribution-fact-identity.md) | several `ClusterProvider`s may name one physical cluster, but a kube-apiserver posts audit to one route, so only one of those names is ever fed and every other one authors `unknown (attribution unresolved)`. Proposes a declared `spec.attribution.auditRoute` that partitions the facts instead of `metadata.name`, so several providers can share one cluster's facts while cloned clusters stay separate, ingestion loses its last Kubernetes read, and a misrouted provider becomes loud. Renames the key infix and the annotation-key flag to the same word | -| [`analyzer-consumer-contract-asks.md`](design/analyzer-consumer-contract-asks.md) | three asks from the first downstream consumer of `pkg/manifestanalyzer`, each the same failure at a different layer — a distinction our code knows, published as prose no test defends. Permanence on a `RefusalReason` (they shipped "not supported yet" for `invalid-yaml`, because one deprecated code was the only signal the type offered); the analyzer's version in its own JSON report, plus a signed release binary; and `ResourceIdentifier.Key()`'s string format as a documented contract with a golden test — it has **no test at all** today. Carries our classification of all eighteen `IssueKind` values; open: the per-construct split for `unsupported-kustomize`, and whether `Key()` or `ToGitPath()` is the identity when a preferred version bumps | +| [`analyzer-consumer-contract-asks.md`](design/analyzer-consumer-contract-asks.md) | three asks from the first downstream consumer of `pkg/manifestanalyzer`, each the same failure at a different layer — a distinction our code knows, published as prose no test defends. Permanence on a `RefusalReason` (they shipped "not supported yet" for `invalid-yaml`, because one deprecated code was the only signal the type offered); the analyzer's version in its own JSON report, plus a signed release binary; and `ResourceIdentifier.Key()`'s string format as a documented contract with a golden test — it has **no test at all** today. Carries our classification of all seventeen internal `IssueKind` values (the public package exports only fourteen, and three codes can reach a consumer with no constant to match on); open: the per-construct split for `unsupported-kustomize`, and whether `Key()` or `ToGitPath()` is the identity when a preferred version bumps | | [`watch-and-catalog-architecture.md`](design/watch-and-catalog-architecture.md) | the target three-layer watch model — **needs a human call before building** | | [`metrics-observability-plan.md`](design/metrics-observability-plan.md) | the watch-stage metrics do not exist yet | | [`reconcile-triggering.md`](design/reconcile-triggering.md) | which controllers still fail to wake up | diff --git a/docs/design/analyzer-consumer-contract-asks.md b/docs/design/analyzer-consumer-contract-asks.md index 033e6bb3..9529e1d4 100644 --- a/docs/design/analyzer-consumer-contract-asks.md +++ b/docs/design/analyzer-consumer-contract-asks.md @@ -63,10 +63,24 @@ nothing stops the next one. two places: the literal `ReasonRefusedStructural`, and `issuesToReasons` ([`internal/manifestanalyzer/scan_repo.go:409`](../../internal/manifestanalyzer/scan_repo.go)), which sets `Code: string(iss.Kind)` for **every** acceptance issue. So the live value set -is the eighteen `IssueKind` constants plus `refused-structural` — and most of them are -neither permanent nor pending. They are **fixable today by the person looking at the -screen**. A folder refused for `invalid-yaml` is one broken document from working, and its -owner was told "not supported yet". +is the **seventeen** internal `IssueKind` constants plus `refused-structural` — and most of +them are neither permanent nor pending. They are **fixable today by the person looking at +the screen**. A folder refused for `invalid-yaml` is one broken document from working, and +its owner was told "not supported yet". + +**A second gap surfaced while counting them, and it needs fixing in the same pass.** +`internal/manifestanalyzer` defines seventeen kinds; `pkg/manifestanalyzer` re-declares +**fourteen**. Because `issuesToReasons` copies the internal string through verbatim, three +codes can reach a consumer that has no exported constant to match on: + +- `kustomize-render-refused` +- `render-does-not-match-live` +- `unplaceable-edit` + +A consumer doing the correct thing — matching on our published constants — silently fails +to recognise all three. Either export them or state that they never reach `ScanRepo`; the +implementer should confirm which by checking whether these write-time kinds can appear in a +structure-only scan. Whatever the answer, the counts must stop disagreeing. They are correct that no consumer can maintain this table outside our repo. We added fifteen codes without any of them carrying permanence. @@ -121,6 +135,11 @@ repository" to someone who cannot. This is the part no consumer can write, so it is written here in full. Derived from each kind's own doc comment and its raise site. +The fourteen rows below plus the three deferred to the next section account for all +seventeen internal kinds; `refused-structural` is the eighteenth value and is not an +`IssueKind`. Check that arithmetic when adding a kind — it is the only thing keeping this +table honest until the test below exists. + | Code | Permanence | Actor | Basis | |---|---|---|---| | `invalid-yaml` | fixable | author | the document does not parse | @@ -179,6 +198,14 @@ it `permanent` (the oracle will always refuse a write it cannot vouch for) or le - Add a test that **every** `IssueKind` constant is classified, failing on a new unclassified kind. That test is the ask. Without it this document is prose again in two releases, which is how we got here. +- One test per constant is **not sufficient**, and the three decision cases above are why: + `unsupported-kustomize`, `unresolved-krm` and `kustomize-render-refused` each classify + differently depending on which branch raised them, so a per-constant test passes while a + new branch emits `PermanenceUnknown`. Cover each emission path, not each constant. This + is the one place the ask costs more than a constant per site, and skipping it reproduces + the original bug one level down. +- Reconcile the internal and public kind sets in the same pass, so the classification test + covers the codes a consumer can actually receive rather than the subset we export. - `PermanenceUnknown` must stay the zero value so an unclassified path degrades to silence rather than to a confident wrong sentence. @@ -231,16 +258,30 @@ path our own package doc recommends, **ldflags do not apply at all** — and no release-workflow change. Use ldflags when set, fall back to `ReadBuildInfo`, and emit `"dev"` for a plain `go build`. +A **top-level string on each report**, beside `schemaVersion` — not a nested object, and +not `omitempty`. The point of the ask is that a report always says what produced it, so a +shape that permits omission fails it: + ```go -type ReportProvenance struct { +type RepoReport struct { + SchemaVersion string `json:"schemaVersion"` // AnalyzerVersion is the release that produced this report: "v0.39.1", or "dev" - // for an unreleased build. Informational — do not gate on it. - AnalyzerVersion string `json:"analyzerVersion,omitempty"` + // for a build that carries no version. Never empty. Informational — a consumer + // records it to trace an answer back, and does not gate on it. + AnalyzerVersion string `json:"analyzerVersion"` + // … unchanged } ``` -Both reports take it. Adding a field does not bump `SchemaVersion`, which is exactly what -our own stability note tells consumers to expect. +`FolderReport` takes the identical field in the same position. The fallback chain is +ldflags → `debug.ReadBuildInfo()` → the literal `"dev"`, so the field is non-empty on every +path including `go run`. A consumer may then treat an absent `analyzerVersion` as "produced +before this shipped" rather than having to distinguish that from "produced by a build that +did not know its own version". + +Adding a field does not bump `SchemaVersion` — which is what our own stability note tells +consumers to expect, and a reason to answer the `SchemaVersion` question below in the same +release rather than after it. ### On `SchemaVersion` — the missing half diff --git a/docs/future/config-surface-for-a-structured-repository.md b/docs/future/config-surface-for-a-structured-repository.md index eba775e7..3a8f8dec 100644 --- a/docs/future/config-surface-for-a-structured-repository.md +++ b/docs/future/config-surface-for-a-structured-repository.md @@ -332,19 +332,28 @@ wants to gate on it. Publish the observation before inventing the assertion. ```text clusters/prod/ - all.yaml # 9 ConfigMaps, one multi-document file + all.yaml # 9 ConfigMaps in one multi-document file, + # spanning namespaces team-a and team-b team-a/secrets/db.sops.yaml ``` +The two namespaces are load-bearing, not decoration. Per `configuration.md:652-655`, a resource +in a namespace the target has never written joins an existing cohort **only** when that cohort +has proven itself namespace-agnostic by already holding more than one — one directory holding one +namespace is indistinguishable from a per-namespace layout whose second namespace has not arrived +yet. So with a single-namespace `all.yaml` the answer below inverts, and the new ConfigMap takes +the canonical path. + Today (`configuration.md:638-646`): a new ConfigMap is **appended to `all.yaml`**, including one -in a brand-new namespace, because the bundle has proven itself namespace-agnostic. A new Secret -goes to `team-a/secrets/`. Both are the right calls, and both are *inferred from mutable repo -state*. +in a brand-new `billing` namespace, because the bundle has cleared that bar. A new Secret goes to +`team-a/secrets/`. Both are the right calls, and both are *inferred from mutable repo state*. The failure mode is not the logic; it is that a **human editing the repo changes the operator's -behaviour without touching any Kubernetes object.** Delete `all.yaml`'s last ConfigMap and the -next new ConfigMap takes the canonical path instead. Nothing in the GitTarget changed. Nothing -in its status says the placement basis moved. +behaviour without touching any Kubernetes object.** Delete every `team-b` ConfigMap from +`all.yaml` and the bundle stops being namespace-agnostic, so the next ConfigMap in a new namespace +takes the canonical path instead. Nothing in the GitTarget changed. Nothing in its status says the +placement basis moved — and this is a sharper version of the same point, because the file the +inference reads is still there and still full of ConfigMaps. With B3, a team that finds this unacceptable writes one field: From a62af9e2fcec9b36e51ee1e26f068cb095114e6e Mon Sep 17 00:00:00 2001 From: Simon Koudijs Date: Mon, 27 Jul 2026 04:15:59 +0000 Subject: [PATCH 4/6] docs(analyzer): recommend a KRM envelope for the analyzer report MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit apiVersion + kind replaces schemaVersion and dissolves the three open questions about what a bump asserts: the Kubernetes API conventions already answer them, in a document every consumer of a GitOps tool has read. The spec/status split turns out to be honest here — spec is the scan request, status the observation — which also gives today's floating "informational" root field a home. Carved out: no metadata (a report has no identity, and the field invites an apply that will never work — kpt's ResourceList sets the same precedent), and never served or registered. Costs one breaking change to the JSON contract, which argues for doing it inside Ask 2 while there is one consumer to coordinate with. It does not replace Ask 2: apiVersion versions the contract, analyzerVersion records the build. Co-Authored-By: Claude Opus 5 (1M context) --- .../design/analyzer-consumer-contract-asks.md | 60 +++++++++++++++++++ 1 file changed, 60 insertions(+) diff --git a/docs/design/analyzer-consumer-contract-asks.md b/docs/design/analyzer-consumer-contract-asks.md index 9529e1d4..0ac6fa60 100644 --- a/docs/design/analyzer-consumer-contract-asks.md +++ b/docs/design/analyzer-consumer-contract-asks.md @@ -296,6 +296,66 @@ these three: Until that exists every consumer's version handling is a guess, and they will not all guess alike. +### Recommended: make the report a KRM document, and delete the question + +The three questions above are ones the Kubernetes API conventions already answer, in a +document every consumer of a GitOps tool has read. So stop writing our own versioning +policy and adopt theirs — give the report an `apiVersion` and a `kind`: + +```yaml +apiVersion: manifestanalyzer.configbutler.ai/v1alpha1 +kind: RepoReport +spec: # what was asked for + root: /repo + mode: scan-repo +status: # what was found + analyzerVersion: v0.39.1 + candidates: [...] + summary: {...} +``` + +`apiVersion` replaces `schemaVersion` outright. "What does a bump assert" becomes the +published alpha/beta/GA contract; "should a reader hard-fail on a version it does not know" +becomes yes, by the same rule every Kubernetes client already follows. We answer three open +questions by citing a document instead of writing one. + +Four further things fall out, none of which the bespoke shape gives us: + +- **The `spec`/`status` split is honest here**, which was the surprise. `spec` is the scan + request (root, mode, policy) and `status` is the observation. Our own CRDs are built that + way, so the report reads like the rest of the product. Today's `root` field floats at the + top level marked "Informational" precisely because there is nowhere for a request to go. +- **YAML becomes the obvious serialization**, which makes `--format yaml` a natural third + option: diffable, reviewable, and committable with the tooling the user already runs. +- **Existing tooling works** — `yq`, `jq`, `kubectl --dry-run` shape assumptions, and + apimachinery's `TypeMeta` for anyone who does link us. +- **It is on-message.** A product whose thesis is that cluster state belongs in Git as KRM + should not emit a bespoke JSON envelope to describe it. + +### Two things to carve out, and one cost + +**No `metadata`.** A KRM document invites `metadata.name`, and a report has no identity — it +observes a path at an instant. A synthesized name is noise, and worse, it suggests the +document can be applied. Kpt's own `kind: ResourceList` carries `apiVersion`, `kind`, `items` +and `results` with no `metadata`, so the envelope-without-metadata shape has precedent. + +**Never served, never registered.** No CRD, no group registration, not applyable. State that +in the type's doc comment, because the shape will make someone try. The related failure is +mild but worth knowing: a report saved into a watched folder is refused either way — today +as `foreign-file`, and as a KRM document as `unresolved-krm`, which reads as "we tried to +manage this and could not" rather than "this is not ours". A marginally worse message, not a +blocker. + +**The cost is one breaking change to the JSON contract**, and our only consumer holds golden +fixtures generated from it. That argues for doing it **now** and inside Ask 2 rather than +after: they are asking us for version clarity in this same document, there is exactly one +consumer to coordinate with, and every later release makes it dearer. It does not disturb +their mirrored-struct design — if anything `TypeMeta` is one more thing they can mirror. + +**It does not replace Ask 2.** `apiVersion` versions the *contract*; `analyzerVersion` +records the *build*. Items 1, 2 and 4 stand unchanged; only the `SchemaVersion` question +above dissolves. + ### On the release asset Their devcontainer pins every tool from a release asset — `task`, `kubectl`, `kustomize`, From 08c87196de12e28d4e401a645cbcdc02ef446c2c Mon Sep 17 00:00:00 2001 From: Simon Koudijs Date: Mon, 27 Jul 2026 04:27:49 +0000 Subject: [PATCH 5/6] docs(analyzer): close the gaps against the second consumer team's four issues MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A second team filed four issues against v0.39.1, verified by measurement rather than by reading. Three of their asks were already covered; five points were not: - The block a consumer reads first is the one that misleads. "Refusal reason codes a candidate may carry" lists two of eighteen, one retired, and nothing says RefusalReason.Code draws from IssueKind — a more direct cause of their wrong sentence than the Layout comment was. Type the field, fix the header. - Their permanence enum has two values; ours keeps four. A two-valued enum drops the not-yet axis, which is the exact distinction that went degenerate. - --version must print the SchemaVersion it emits, not only the release. - Their generator{name,version} beats our flat analyzerVersion: a piped report needs to say which tool, not only which release. - Key()'s golden test needs a fourth case, core cluster-scoped (/v1/nodes/node-1), where group and namespace are both empty and degenerate in opposite ways. All four shapes confirmed by running them. The two teams disagree on one point — attestation versus checksums — so the doc now says do both: the attestation is the stronger claim, the checksum is what a Dockerfile can consume without a token. Also records that the minimal-version-selection diamond is explicitly not our bug, so nobody restructures the module to solve it. Co-Authored-By: Claude Opus 5 (1M context) --- .../design/analyzer-consumer-contract-asks.md | 119 ++++++++++++++---- 1 file changed, 98 insertions(+), 21 deletions(-) diff --git a/docs/design/analyzer-consumer-contract-asks.md b/docs/design/analyzer-consumer-contract-asks.md index 0ac6fa60..18760f2b 100644 --- a/docs/design/analyzer-consumer-contract-asks.md +++ b/docs/design/analyzer-consumer-contract-asks.md @@ -8,18 +8,26 @@ > which makes the same argument one layer up: the engine learned something and the > contract did not carry it. -The three asks come from a product that consumes GitOps Reverser two ways at once — it -links `pkg/manifestanalyzer` as a module, and it execs `manifest-analyzer --mode -scan-repo --format json` from a second tool that does not link us at all. It runs the -reverser itself as an image, pinned to the module by two constants with a build-breaking -test on their side (their `internal/cluster/cluster.go`). That makes it -the first consumer that feels every seam in our published contract, and all three asks -are the same complaint: **a fact our own doc comments assert is carried by prose that -nothing tests, so it drifts silently and a consumer encodes the stale version.** - -One of them already shipped a wrong sentence to a real user because of it. That is the -bar these asks are answering, and it is why the fix in each case is *data plus a test*, -not better prose. +The asks come from **two independent consumer teams**, arriving separately and landing on +the same seams. One links `pkg/manifestanalyzer` as a module and execs `manifest-analyzer +--mode scan-repo --format json` from a second tool that does not link us; it runs the +reverser as an image pinned to the module by two constants with a build-breaking test on +its side. The other verified everything against `v0.39.1` by measurement — running the +binary over a corpus and reading the JSON — and filed four issues. + +That convergence is the strongest evidence in this document. Two teams that never spoke to +each other hit the same three seams, and every one is the same complaint: **a fact our doc +comments assert, carried by prose nothing tests, so it drifts and a consumer encodes the +stale version.** One of them shipped a wrong sentence to a real user because of it. + +The second team's four issues map onto this document as: their #1 → Ask 1, their #2 → the +`LayoutKustomizeOverlay` fix already made, their #3 → Ask 2, their #4 → Ask 3. Where they +asked for something narrower than what is written here, this document says so. + +One thing they were explicit about, so it is recorded before someone "fixes" it: **the +minimal-version-selection diamond is not our bug.** It existed because their tool linked +our module; it stopped linking, and that is the end of it. Do not restructure the module +to solve it. --- @@ -82,6 +90,32 @@ to recognise all three. Either export them or state that they never reach `ScanR implementer should confirm which by checking whether these write-time kinds can appear in a structure-only scan. Whatever the answer, the counts must stop disagreeing. +**And the block a consumer reads first is the one that misleads.** The second team measured +a real corpus and got back `unsupported-kustomize` and `non-krm-yaml` as refusal codes, then +went looking for them. `repo.go` has this: + +```go +// Refusal reason codes a candidate may carry. +const ( + ReasonOverlayFanOutUnsupported = "overlay-fan-out-unsupported" // Deprecated: no longer emitted + ReasonRefusedStructural = "refused-structural" +) +``` + +A block headed "Refusal reason codes a candidate may carry" that lists **two of eighteen**, +one of them retired. The codes they measured do exist as `IssueKind` constants — but nothing +says `RefusalReason.Code` draws from `IssueKind`, so a reader who finds this block reasonably +concludes it is the enumeration and treats everything else as unknown. That is a more direct +cause of their wrong sentence than the `Layout` comment was. + +Two fixes, both cheap: + +- **Type it.** `RefusalReason.Code` becomes `IssueKind` rather than `string`, which makes the + relationship compile-checked instead of stated. If that is too strong for a JSON-facing + field, document it: *"Code is an [IssueKind] value, or [ReasonRefusedStructural]."* +- **Fix the block's header** so it says what it holds: reason codes that are *not* issue + kinds. As written it claims to be the enumeration, and it is not. + They are correct that no consumer can maintain this table outside our repo. We added fifteen codes without any of them carrying permanence. @@ -125,6 +159,19 @@ type RefusalReason struct { `Issue` takes both fields for the same reason: a policy that treats an issue as blocking is making this decision already, without the data to make it. +**Four values, where the second team proposed two** (`permanent | conditional`). Take the +four deliberately: a two-valued enum drops the *not-yet* axis, which is the exact +distinction that went degenerate when `ReasonOverlayFanOutUnsupported` was retired and the +exact one that made their table wrong. Collapsing `pending-upstream` into `conditional` +tells a user to go fix something no user can fix. And `PermanenceUnknown` has to exist as +the zero value regardless, or an unclassified path emits a confident wrong answer instead +of silence. Their `retryable` naming is worth considering for the `fixable` value; the axis +count is the part not to compromise on. + +They also note that adding a field is additive and need not move `SchemaVersion`. Correct, +and it stays correct under the KRM envelope recommended in Ask 2 — that envelope is a +separate, deliberate breaking change, and this field should not wait for it. + Ship the actor field. The consumer offered it as optional; it is the cheaper half of the same constant, and two of our codes are fixable **only** by the platform operator, whom their wizard does not have on the screen. Without it, `out-of-scope` renders as "fix your @@ -224,11 +271,16 @@ worse for their user than the truth and is the honest maximum from `{Code, Detai Four things, in their order of value: 1. The analyzer's own version **in the report**. -2. A `manifest-analyzer --version` flag. +2. A `manifest-analyzer --version` flag — printing the release version **and the + `SchemaVersion` it emits**, so one exec answers both questions. 3. One sentence on what a `SchemaVersion` bump asserts, and whether a reader should refuse a version it does not know. 4. The analyzer binary attached to the release we already sign. +The second team measured the current flag set to confirm the gap: `manifest-analyzer +--version` returns `flag provided but not defined: -version`, and the full set is +`-context`, `-format`, `-kubeconfig`, `-mode`, `-policy`. + ### Why — verified Our package doc invites this consumer by name: *"Exec the binary if Go is not your @@ -273,6 +325,14 @@ type RepoReport struct { } ``` +The second team proposed a `generator: {name, version}` object instead, on the grounds that +it is "the version of this that survives being piped into another tool". **Prefer their +shape**, and put the tool's name in it: a bare `analyzerVersion` says which release without +saying which *tool*, and a report that has been piped somewhere is exactly where that +ambiguity bites. Under the KRM envelope below it becomes `status.generator: {name, version}`, +where `kind` names the document and `generator.name` names what produced it — two different +facts that a flat string conflates. + `FolderReport` takes the identical field in the same position. The fallback chain is ldflags → `debug.ReadBuildInfo()` → the literal `"dev"`, so the field is non-empty on every path including `go run`. A consumer may then treat an absent `analyzerVersion` as "produced @@ -364,11 +424,19 @@ that cannot be, because we publish no binary; the tool that runs it does `go ins source. That is the single unverifiable link in an otherwise pinned toolchain, and it is the tool deciding which folders they offer a tenant. -They explicitly do **not** want a `sha256sums` file. `v0.39.1` already ships `crds.yaml`, -`install.yaml` and `sbom.spdx.json`, each with a `.intoto.jsonl` provenance attestation -and a `.sigstore.json` bundle ([`.github/workflows/release.yml`](../../.github/workflows/release.yml)). -Adding the analyzer binary to that job makes it verifiable with `gh attestation verify` — -stronger than a checksum, and machinery we already run. +**The two teams differ here, and the resolution is to do both.** The first explicitly does +*not* want a `sha256sums` file, because attestation is stronger. The second asks for +"publishing the binary with checksums, if that is cheap alongside the existing release job" +— its devcontainer verifies every other tool that way, so checksums are the mechanism its +existing tooling already speaks. + +`v0.39.1` already ships `crds.yaml`, `install.yaml` and `sbom.spdx.json`, each with a +`.intoto.jsonl` provenance attestation and a `.sigstore.json` bundle +([`.github/workflows/release.yml`](../../.github/workflows/release.yml)). Attach the binary +to that job — verifiable with `gh attestation verify` — **and** emit a `sha256sums` file +beside it. The attestation is the stronger claim; the checksum is the one a `curl | sha256sum +-c` line in a Dockerfile can consume without a GitHub token. Neither team is served by +choosing. Decision for the implementer: **which platforms.** `linux/amd64` and `linux/arm64` covers their devcontainer; `darwin/arm64` is the obvious third. Each is another matrix leg on a @@ -417,18 +485,27 @@ func (r ResourceIdentifier) Key() string // "{group}/{version}/{resource}/{nam func (r ResourceReference) Key() string // "namespace/name" ``` -The golden test pins all three shapes, because those are the cases a reimplementation gets -wrong: +The golden test pins **four** shapes — the second team named the fourth, and it is the one +this document first missed. All four were confirmed by running them: | case | expected | |---|---| | namespaced, grouped | `apps/v1/deployments/prod/api` | | cluster-scoped, grouped | `rbac.authorization.k8s.io/v1/clusterroles/admin` | | namespaced, core group | `/v1/secrets/prod/db` | +| **cluster-scoped, core group** | `/v1/nodes/node-1` | The four-segment cluster-scoped form is the sharp edge: `Key()` **drops** the namespace segment rather than emitting an empty one, so a naive reimplementation that always joins -five parts produces `…/clusterroles//admin` and never joins. +five parts produces `…/clusterroles//admin` and never joins. The core cluster-scoped case +is the sharpest of the four, since both the group and the namespace are empty — one +degenerates to a leading `/` and the other vanishes, and a reimplementation has to get two +opposite rules right in one string. + +Their ask includes wording, and it is worth taking verbatim: the test carries a comment +saying the format is **depended on across product boundaries, so changing it is a breaking +change rather than a refactor**. That sentence is what turns a red test from an obstacle +into a decision point. ### The trap worth naming From 9cd1f431732521b56d50fb98bda8b31f4f5f19dd Mon Sep 17 00:00:00 2001 From: Simon Koudijs Date: Mon, 27 Jul 2026 04:44:04 +0000 Subject: [PATCH 6/6] docs(analyzer): record the two decisions that unblock implementation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The KRM envelope is adopted rather than recommended: apiVersion/kind replaces schemaVersion, spec carries the scan request and status the findings. Taken as a deliberate breaking change now, while there are two consumers and both are asking for version clarity in this same document. The provenance field settles as status.generator{name,version} — the second team's shape, because a piped report needs to say which tool and not only which release. The three unclassified refusal codes are decided during implementation: the session proposes a permanence per raise site and raises each as a review comment for the maintainer to accept or overturn. They never ship as PermanenceUnknown, and they no longer block the other fourteen. Also folds the SchemaVersion section into the envelope decision that answers it, rather than leaving three open questions beside their own answer. Co-Authored-By: Claude Opus 5 (1M context) --- .../design/analyzer-consumer-contract-asks.md | 88 ++++++++++--------- 1 file changed, 48 insertions(+), 40 deletions(-) diff --git a/docs/design/analyzer-consumer-contract-asks.md b/docs/design/analyzer-consumer-contract-asks.md index 18760f2b..d2835e1e 100644 --- a/docs/design/analyzer-consumer-contract-asks.md +++ b/docs/design/analyzer-consumer-contract-asks.md @@ -1,13 +1,26 @@ # Three asks from a downstream consumer of the analyzer -> Status: accepted as work, unscheduled. Written for an implementing party — every -> claim below was checked against the tree at `v0.39.1`, and each ask ends with the -> decisions only a maintainer can make. -> Date: 2026-07-26 +> Status: accepted as work, scheduled as three independent PRs. Written for an implementing +> party — every claim below was checked against the tree at `v0.39.1`. +> Date: 2026-07-26, decisions recorded 2026-07-27. > Companion to [config-surface-for-a-structured-repository.md](../future/config-surface-for-a-structured-repository.md), > which makes the same argument one layer up: the engine learned something and the > contract did not carry it. +**Two decisions are settled and are not open in implementation:** + +1. **The KRM envelope is adopted** (Ask 2). `apiVersion`/`kind` replaces `schemaVersion`, + `spec` carries the scan request and `status` the findings. This is a deliberate breaking + change to the JSON contract, taken now because there are two consumers and both are + asking for version clarity in this same document. +2. **The three unclassified refusal codes are decided during implementation** (Ask 1). The + implementing session proposes a permanence per *raise site* from the code, and raises + each as a review comment on its PR for the maintainer to accept or overturn. They do not + ship as `PermanenceUnknown`, and they do not block the rest of Ask 1. + +Each ask is one PR, in the order below — Ask 3 is unblocked, Ask 1 is scaffolding plus +fourteen settled classifications, Ask 2 carries the breaking change. + The asks come from **two independent consumer teams**, arriving separately and landing on the same seams. One links `pkg/manifestanalyzer` as a module and execs `manifest-analyzer --mode scan-repo --format json` from a second tool that does not link us; it runs the @@ -212,6 +225,12 @@ reader can do, since that is the sentence the field exists to write. ### Three that need a decision, not a lookup +**How these three get settled — decided.** The implementing session reads each raise site, +proposes a permanence for it, and raises the proposal as a review comment on its own PR for +the maintainer to accept or overturn. They ship classified, never as `PermanenceUnknown`, +and they do not hold up the fourteen above. The starting positions below are inputs to that +proposal, not conclusions. + **`unsupported-kustomize` — must be classified per construct at the raise site.** This is the case that proves the whole ask: one code, both answers. Its doc comment lists generators, components, Helm inflation, replacements, transformers, name prefixes and @@ -310,53 +329,42 @@ path our own package doc recommends, **ldflags do not apply at all** — and no release-workflow change. Use ldflags when set, fall back to `ReadBuildInfo`, and emit `"dev"` for a plain `go build`. -A **top-level string on each report**, beside `schemaVersion` — not a nested object, and -not `omitempty`. The point of the ask is that a report always says what produced it, so a -shape that permits omission fails it: +**The settled shape is `status.generator`**, an object, never `omitempty`. The second team +proposed `{name, version}` on the grounds that it is "the version of this that survives +being piped into another tool", and their shape wins: a bare version string says which +release without saying which *tool*, and a piped report is exactly where that bites. `kind` +names the document; `generator.name` names what produced it — two different facts a flat +string conflates. ```go -type RepoReport struct { - SchemaVersion string `json:"schemaVersion"` - // AnalyzerVersion is the release that produced this report: "v0.39.1", or "dev" - // for a build that carries no version. Never empty. Informational — a consumer - // records it to trace an answer back, and does not gate on it. - AnalyzerVersion string `json:"analyzerVersion"` - // … unchanged +// Generator names the build that produced a report. Never empty: a report that cannot +// say what produced it is the failure this field exists to prevent. +type Generator struct { + // Name is the producing tool, e.g. "manifest-analyzer". + Name string `json:"name"` + // Version is the release, e.g. "v0.39.1", or "dev" for a build carrying no version. + Version string `json:"version"` } ``` -The second team proposed a `generator: {name, version}` object instead, on the grounds that -it is "the version of this that survives being piped into another tool". **Prefer their -shape**, and put the tool's name in it: a bare `analyzerVersion` says which release without -saying which *tool*, and a report that has been piped somewhere is exactly where that -ambiguity bites. Under the KRM envelope below it becomes `status.generator: {name, version}`, -where `kind` names the document and `generator.name` names what produced it — two different -facts that a flat string conflates. - -`FolderReport` takes the identical field in the same position. The fallback chain is -ldflags → `debug.ReadBuildInfo()` → the literal `"dev"`, so the field is non-empty on every -path including `go run`. A consumer may then treat an absent `analyzerVersion` as "produced -before this shipped" rather than having to distinguish that from "produced by a build that -did not know its own version". - -Adding a field does not bump `SchemaVersion` — which is what our own stability note tells -consumers to expect, and a reason to answer the `SchemaVersion` question below in the same -release rather than after it. +Both reports carry it at `status.generator`. The fallback chain is ldflags → +`debug.ReadBuildInfo()` → the literal `"dev"`, so it is non-empty on every path including +`go run`. A consumer may then read an absent `generator` as "produced before this shipped" +without having to distinguish that from "produced by a build that did not know itself". -### On `SchemaVersion` — the missing half +### On `SchemaVersion` — the question the envelope answers Our doc states the consumer's duties well: pin a version, ignore unknown fields, do not -switch on prose. It never states what a **bump asserts**. Write that sentence, and answer -these three: +switch on prose. It never states what a **bump asserts**, which left three questions open: - Does a bump mean fields were removed, that a field's meaning changed, or either? - Should a reader that knows `v1` hard-fail on `v2`, or attempt a best-effort parse? - Since adding a field never bumps it, what is left that *does*? -Until that exists every consumer's version handling is a guess, and they will not all -guess alike. +**All three are answered by adopting the KRM envelope below**, which is why that decision +was taken rather than writing a bespoke policy to answer them one at a time. -### Recommended: make the report a KRM document, and delete the question +### Decided: the report becomes a KRM document, and the question dissolves The three questions above are ones the Kubernetes API conventions already answer, in a document every consumer of a GitOps tool has read. So stop writing our own versioning @@ -369,7 +377,7 @@ spec: # what was asked for root: /repo mode: scan-repo status: # what was found - analyzerVersion: v0.39.1 + generator: {name: manifest-analyzer, version: v0.39.1} candidates: [...] summary: {...} ``` @@ -412,7 +420,7 @@ after: they are asking us for version clarity in this same document, there is ex consumer to coordinate with, and every later release makes it dearer. It does not disturb their mirrored-struct design — if anything `TypeMeta` is one more thing they can mirror. -**It does not replace Ask 2.** `apiVersion` versions the *contract*; `analyzerVersion` +**It does not replace Ask 2.** `apiVersion` versions the *contract*; `status.generator` records the *build*. Items 1, 2 and 4 stand unchanged; only the `SchemaVersion` question above dissolves. @@ -527,7 +535,7 @@ reason for anyone to reimplement either function. Small, and none of it constrains a pre-1.0 module: -- Godoc on `Key()` naming the format as public, with the three shapes. +- Godoc on `Key()` naming the format as public, with the four shapes. - A table-driven golden test on the exact strings, in `internal/types`. - One line in the package doc of `pkg/manifestanalyzer` pointing at the format, since that is where a consumer looks.