fix(placement)!: repository state must not move where we write, and a refused resource must not be registered - #291
Merged
Merged
Conversation
📝 WalkthroughWalkthroughThe change removes sibling-layout inference for new Git documents, adds deterministic kustomize-root and canonical fallbacks, introduces structured placement refusals and telemetry, updates Git write attribution, and revises tests, schemas, specifications, upgrade guidance, and observability documentation. ChangesNew-file placement
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant SourceEvents
participant BranchWorker
participant PlacementResolver
participant GitWorktree
participant Telemetry
SourceEvents->>BranchWorker: submit create or resync events
BranchWorker->>PlacementResolver: resolve new document placement
PlacementResolver-->>BranchWorker: return declared, kustomize-root, or canonical path
BranchWorker->>GitWorktree: write document and update kustomization resources
BranchWorker->>Telemetry: record placement, refusal, or entry outcome
Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
Contributor
Author
|
Base automatically changed from
fix/analyzer-refusal-doc-and-single-encoder
to
main
July 30, 2026 06:04
…e the operator writes Option C's sibling-cohort ladder is gone: `resolveInferred` through `allSameDir`, about a third of `placement.go`, plus the tests that pinned each rung. A new document's destination now comes from the GitTarget's declared `placement.byType`/`default`, or from the folder having exactly one supported kustomization root, or from the built-in canonical path — never from where the repository happens to keep the other documents of the same type. The argument is in docs/design/open-asks-priority.md and it is not primarily about the bug: inference let a human's edit to a repository change the operator's behaviour with no Kubernetes object changing and nothing in status recording the move. The bug is the evidence. Its namespace-agnosticism guard was vacuous on the singleton branch for a period, so a new namespace's object was appended into the first namespace's file, which then genuinely spanned two namespaces, which legitimized the bundle for every later object, which collapsed a whole type into one file. A rule inferred from mutable state has failure modes that feed themselves, and the fix for that instance did not make the class safe. The kustomize-root fallback stays, because it is not inference. A file no kustomization can reach is not oddly placed, it is never rendered; placing the new document beside the folder's one root follows from there being one root. More than one is still ambiguous and still declines. Two things came out of doing it — - **Namespace inheritance moved to the governing kustomization, where it belongs.** "Omit metadata.namespace, the context supplies it" used to be read off a sibling's bytes, so it only ever fired for an inferred placement; a DECLARED path into the same directory silently wrote a `namespace:` line the folder's own documents omit. It is now decided once, in `finishPlacement`, for every resolved path. - **And it must match, which the old kustomize-root path never checked.** Omitting the namespace hands it to kustomize, so a transformer naming a DIFFERENT namespace would render the document as another object entirely. The explicit line now stays in that case, and the render oracle reports a folder that cannot express the object instead of the mirror quietly claiming one it does not hold. `PlacementResult.Cohort` is deleted with the ladder, and `PlacementSource` gains `kustomize_root` in place of `inferred`, which now names one mechanism rather than two. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…refused
Placement had two signals and neither was reachable: a log line at the skip site,
and `ResyncStats.PlacementSkipped` — a field in a resync summary, not a series.
The spec's own P8 said the "why did it land there?" trace was mandatory and it was
never built. With sibling inference deleted, a hand-authored layout needs a
`placement.byType` line, so the question "which target and which type is missing
one" has to be answerable without reading the folder.
Three counters, all labelled by `{gittarget_namespace, gittarget_name, group,
version, resource}` — the GitTarget that owns the write and the exact shape of a
`placement.byType` key, so a series reads as the line the target needs:
- **`placements_total{source, disposition}`** — one increment per new document
actually written. `source` is declared / kustomize_root / canonical;
`disposition` is new_file / appended. `source="canonical"` is the missing-rule
signal, and `kustomize_root` is deliberately NOT lumped in with it: a folder
with one render root is placing files where they build, which is the correct
answer with no declaration at all.
- **`placement_refusals_total{reason}`** — one increment per resource the writer
declined to place, from a closed reason set (`invalid_path`,
`sensitive_append`, `plaintext_onto_encrypted`, `mixed_sensitivity_new_file`,
`multi_document_target`). Every increment is a resource absent from the mirror.
- **`placement_kustomization_entries_total{outcome}`** — added / no_change /
failed for the `resources:` entry a new file needs. `failed` is the invisible
one: the document is committed and the entry is not, so kustomize never builds
the file — it is in Git, it looks mirrored, and nothing applies it.
Three decisions worth stating:
- **The two counters partition the population.** A placement is recorded after the
write lands, not at resolution, and a refusal is recorded instead — never both.
A refusal as a `source` value would have let a dashboard count a skipped Secret
as a successful placement.
- **The reason is typed, not a matched message.** `PlacementRefusedError` carries a
bounded `Reason`, so the label cannot drift when an error string is reworded, and
the two writer-side refusals share the analyzer's label domain.
- **The GitTarget labels are the point.** The design doc argued against leading
with a bare `placement_fell_back_total` precisely because "it happened
somewhere" is not actionable. The label keys are `gittarget_*` rather than
`namespace`/`name` for the pod-scrape reason `TargetReconcileCompletedTotal`
documents.
The resync path carries the same labels, taken from the resolved target metadata
rather than the synthesised events: which of the two paths created a file is not
something the operator chose, so it must not change whether the placement is visible.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… rule
The spec binds the code, so the ladder cannot be deleted from one and left in the
other. `gittarget-new-file-placement-rules.md` now documents three steps —
declared, the folder's one kustomize root, canonical — and keeps Option C's
sections as history, because the argument is worth having on the page and because
its own P1–P10 risk list is the case for the removal. Each risk is annotated with
what became of it: P1, P2, P3, P4, P6 and P8 are one property stated six times and
are retired; P7, P9 and P10 are facts about the code that remains. P8 in
particular stays visible — the explainability that spec made mandatory was never
built, and the smaller ladder gets the smaller obligation it deserves.
The kustomize-root fallback keeps its section and gains the namespace-match rule,
stated as a safety property rather than a convention: omitting `metadata.namespace`
hands the namespace to kustomize, so a transformer naming a different namespace
would render a different object than the one being mirrored.
User-facing:
- **`configuration.md`** replaces the "following the existing layout" section with
the three-step ladder, a "knowing when you need a rule" section built on
`placements_total{source="canonical"}`, and the refusal and kustomization-entry
counters with what each reason means for a policy.
- **`UPGRADING.md`** carries the behaviour change: who is affected (a hand-authored
folder, not one this operator created), the one `byType` line that buys the old
behaviour back, the query that says whether it affects you, and why there is no
`spec.placement.mode` to switch it back on.
- **`interpreting-metrics.md`** documents the three counters with a `source` table
saying which values need attention (`kustomize_root` does not — a folder with one
render root is placing files where they build), and the label-cardinality reasons.
- **`architecture.md`** and **`installing-apps-as-krm.md`** stop describing a step
that no longer runs.
`open-asks-priority.md` strikes the entry and corrects itself where building it
proved the argument wrong: it had argued *against* leading with a Prometheus
counter, on the grounds that "it happened somewhere" is not actionable. That
objection was to the labels, and it does not survive them naming the GitTarget and
the type key. "What the deletion taught" records the rest — namespace inheritance
was a second implementation of a rule that belonged to the governing kustomization,
and the write path's missing GitTarget identity is the same fact that explains why
placement had no metrics and cannot easily have an Event.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…vary unparam is right: every caller passed the same name and namespace, so the parameters were documentation of an intent the tests do not have. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…t a broken folder The manifest-folder spec asserted the committed `kustomization.yaml` was byte-identical to the fixture's. That assertion was pinning a side effect of sibling inference, and it failed as soon as the inference was gone. The namespace holds a ConfigMap nobody in the test created — the cluster's own `kube-root-ca.crt` — and the WatchRule selects every ConfigMap, so the operator has a watched resource with no document in Git. Placement now gives it a file beside the folder's one kustomization and registers it in `resources:`, which is the documented kustomize-root behaviour and what the new-file-placement spec asserts directly. Inference used to append that resource to the existing bundle instead, and the bundle was already listed, so the build file happened to stay byte-identical. The property this spec is about is that a hand-authored build file is not reordered, reformatted, or shortened. That is now asserted directly: every line the fixture wrote survives in order, and anything added is a `resources:` entry. It also states the reasoning in the helper, so nobody re-tightens it to equality and rediscovers this. `docs/UPGRADING.md` gains the concrete shape a user of a kustomize folder will see: a new file plus an entry, where a bundle used to grow and the build file did not change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
sunib
force-pushed
the
feat/delete-sibling-inference
branch
from
July 30, 2026 06:32
660878f to
7aade9c
Compare
….layout A decision record for the review questions on #291, written so the answers are arguable rather than asserted. Everything it decides lands in that same PR. **Keep `canonical`, and split `declared`.** Renaming the built-in path to `default` collides with `spec.placement.default`, which is the opposite thing: a declaration. A reader of `source="default"` could not tell whether their catch-all matched or whether nothing matched at all. The resolution the metric was actually missing is the other one, so `declared` becomes `byType` and `default` and the prose stops calling the built-in path "the built-in default". **No CRD default for `placement.default`, and the reason is concrete rather than stylistic.** The idea is clearer for a reader of one object and I gave it too little credit at first, so the document states it at full strength and then prices it. Two prices: the versionless template we would default to is judged NOT identity-complete by `validateSecretSafety`, so the CRD's own default would turn `Validated=False` on for every target without an explicit Secret route; and a persisted default becomes the user's data, which costs us the ability to improve the built-in path for existing targets and the ability to distinguish "the user asked for this" from "we suggested it" ever again. The order for revisiting it is written down rather than left as a no. **`status.layout` instead**, with five worked examples: greenfield, a kustomize overlay, brownfield missing one rule, two ambiguous roots, and a refusal from an operator-configured sensitive type the static gate cannot see. It answers the same question from a derived field, so it cannot fork from the code and improves with it, and it says the thing a spec field structurally cannot: what the operator understood about the folder. Three findings changed a decision: - `IdentityCompletePlacementTemplate` requires `{version}` for a non-narrowed template, which contradicts the versionless-path decision and rejects templates that cannot collide two identities. A bug on its own terms, and the precondition for any future spec default. - The data-plane-to-status seam the queue doc said did not exist does exist: `MarkTargetRetention` enqueues the GitTarget on a change, which is exactly the missing enqueue that made an Event look expensive. - Two supported kustomizations still decline to canonical, where no root reaches the file. It is committed, looks mirrored, and is applied by nothing, and the entries counter cannot see it because no entry is attempted. `{kindLower}` over a `toLower` function, because a function syntax is a language and the spec's own "keep it small" already forbids one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/git/plan_flush.go (1)
378-401: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDefer the kustomization resource entry until the write succeeds.
appendKustomizationResourcemutates the kustomization file beforeplaceNewDocument; ifplaceNewDocumentlater returnsupsertSkippedUnsafefor mixed sensitivity or a multi-document target, the commit can include aresources:entry for a file this placement refused to write. That makeskustomize buildfail for the folder and makesplacement_kustomization_entries_total{outcome="added"}count a refused resource. Add the kustomization entry only afterwroteBytes(outcome), or roll backbuf.currenton the skip path.Suggested ordering change
- if placement.Kustomization != nil { - wb.appendKustomizationResource(ctx, event, placement) - } - // A destination that infers its namespace from build context... @@ outcome, refusal, err := wb.placeNewDocument(ctx, event, placement, sensitive) if err != nil || !wroteBytes(outcome) { @@ return outcome, err } + if placement.Kustomization != nil { + wb.appendKustomizationResource(ctx, event, placement) + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/git/plan_flush.go` around lines 378 - 401, Move the appendKustomizationResource call out of the pre-write block and invoke it only after placeNewDocument returns successfully with wroteBytes(outcome). Keep the existing placement.NamespaceInherited namespace-clearing behavior before the write, and ensure skipped or refused placements do not mutate the kustomization or increment its added-entry metric.
🧹 Nitpick comments (3)
internal/manifestanalyzer/placement_test.go (1)
267-269: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueStale rationale in the failure message.
The assertion message still attributes inheritance to the sibling's bytes; the header comment (and the implementation) now derive it from the kustomization's
namespace:transformer.♻️ Wording fix
- t.Fatalf("got %+v, want NamespaceInherited since the sibling omits metadata.namespace", res) + t.Fatalf("got %+v, want NamespaceInherited: the kustomization's namespace: transformer supplies it", res)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/manifestanalyzer/placement_test.go` around lines 267 - 269, Update the failure message in the NamespaceInherited assertion within the placement test to attribute inheritance to the kustomization namespace transformer rather than the sibling omitting metadata.namespace. Keep the assertion and expected behavior unchanged.internal/git/placement_metrics.go (2)
142-152: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
"unclassified"is a label value outside the documented closed set.
PlacementRefusalReasonis described as a bounded label domain ininternal/manifestanalyzer/placement.go, andPlacementRefusalsTotal's doc ininternal/telemetry/exporter.goenumerates the reasons without this one. Defining it alongside the others (or at least naming it as a const here and mentioning it in the exporter doc) keeps the domain discoverable for dashboard authors.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/git/placement_metrics.go` around lines 142 - 152, The fallback returned by placementRefusalReason is not part of the documented PlacementRefusalReason domain. Define the unclassified value alongside the bounded refusal-reason constants in internal/manifestanalyzer/placement.go, and update the PlacementRefusalsTotal documentation in internal/telemetry/exporter.go to enumerate it; then reuse that defined constant in placementRefusalReason instead of the raw string.
94-98: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
appendonto a returned slice is safe today only becauseattrs()allocates exactly.
placementTarget.attrs()returns a fresh len==cap slice, so eachappendreallocates and no label set can be corrupted. Ifattrs()ever returns a pre-sized or shared backing array, these three sites silently overwrite each other's attributes. Aslices.Concator explicitmake([]attribute.KeyValue, 0, n)would remove the dependency on that invariant.Also applies to: 115-116, 138-138
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/git/placement_metrics.go` around lines 94 - 98, The attribute construction in the placement metrics code depends on placementTarget.attrs returning an unshared, exact-capacity slice. Update the attribute assembly at all three sites, including the flows around placementTarget.attrs, to use independent concatenation or explicitly allocated capacity before appending resource and metric attributes, preventing writes from mutating shared backing arrays.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/configuration.md`:
- Around line 613-628: Align placement documentation with the shipped contract
across docs/configuration.md lines 613-628, docs/architecture.md lines 541-543,
docs/installing-apps-as-krm.md lines 156-168, and docs/UPGRADING.md lines 12-18:
remove all existing-layout or sibling-inference claims, require exactly one
supported kustomization root where applicable, and consistently document the
canonical {namespace}/{group}/{resource}/{name}.yaml path, {namespaceOrCluster}
behavior, omitted core-resource group, no version segment, and .sops.yaml suffix
for sensitive resources.
In `@docs/interpreting-metrics.md`:
- Around line 209-211: Update the sentence immediately before the query in the
metrics documentation to use “This should be zero:” instead of the subjectless
“Should be zero:”.
In `@docs/spec/gittarget-new-file-placement-rules.md`:
- Around line 1304-1314: Update the “Surface placement outcomes” specification
to state that placements_total increments only for every successful placement or
new document, not every resolution. Preserve the existing refusal, Kustomization
entry, skip logging, and resync-summary requirements unchanged.
---
Outside diff comments:
In `@internal/git/plan_flush.go`:
- Around line 378-401: Move the appendKustomizationResource call out of the
pre-write block and invoke it only after placeNewDocument returns successfully
with wroteBytes(outcome). Keep the existing placement.NamespaceInherited
namespace-clearing behavior before the write, and ensure skipped or refused
placements do not mutate the kustomization or increment its added-entry metric.
---
Nitpick comments:
In `@internal/git/placement_metrics.go`:
- Around line 142-152: The fallback returned by placementRefusalReason is not
part of the documented PlacementRefusalReason domain. Define the unclassified
value alongside the bounded refusal-reason constants in
internal/manifestanalyzer/placement.go, and update the PlacementRefusalsTotal
documentation in internal/telemetry/exporter.go to enumerate it; then reuse that
defined constant in placementRefusalReason instead of the raw string.
- Around line 94-98: The attribute construction in the placement metrics code
depends on placementTarget.attrs returning an unshared, exact-capacity slice.
Update the attribute assembly at all three sites, including the flows around
placementTarget.attrs, to use independent concatenation or explicitly allocated
capacity before appending resource and metric attributes, preventing writes from
mutating shared backing arrays.
In `@internal/manifestanalyzer/placement_test.go`:
- Around line 267-269: Update the failure message in the NamespaceInherited
assertion within the placement test to attribute inheritance to the
kustomization namespace transformer rather than the sibling omitting
metadata.namespace. Keep the assertion and expected behavior unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 1a105146-958d-4034-9ba1-9199074e7b01
📒 Files selected for processing (23)
api/v1alpha3/gittarget_types.goconfig/crd/bases/configbutler.ai_gittargets.yamldocs/INDEX.mddocs/UPGRADING.mddocs/architecture.mddocs/configuration.mddocs/design/metrics-observability-plan.mddocs/design/open-asks-priority.mddocs/installing-apps-as-krm.mddocs/interpreting-metrics.mddocs/spec/gittarget-new-file-placement-rules.mdinternal/git/pending_writes.gointernal/git/placement_metrics.gointernal/git/placement_metrics_test.gointernal/git/placement_test.gointernal/git/plan_flush.gointernal/git/resync_flush.gointernal/git/resync_flush_test.gointernal/git/types.gointernal/manifestanalyzer/placement.gointernal/manifestanalyzer/placement_test.gointernal/telemetry/exporter.gotest/e2e/inplace_edit_e2e_test.go
…nder root, not etcd Review pushed back on two of the three arguments in this document and was right about both, so the record now leads with the objection that survives. - **"Just default the Secret route too" works, and is a trap.** A defaulted `byType["v1/secrets"]` is narrowed to one type, so it satisfies identity-completeness and unblocks the bundling-default check. But Kubernetes defaulting applies to an ABSENT field and never merges, so a user writing any `byType` entry of their own replaces the whole map, silently drops the Secret route, and flips the object to `Validated=False` on an edit about ConfigMaps. - **The persistence argument was overstated.** A default is persisted on every spec-writing apply and applied in memory on read, but NOT by our own status writes (GitTarget has a status subresource, verified). And freezing the built-in path per target is arguably desirable, since placement is already create-time and non-retroactive. `metadata.managedFields` even records that the server set the value, so "indistinguishable from a declaration" was false. What is left is spec bloat, which decides nothing. - **The objection that does decide it is structural.** `resolveDeclared` returns on any non-empty declared template, and the kustomize-root step runs after it. A defaulted `default` is never empty, so the render-root step becomes unreachable and every new file in an overlay takes the canonical path: in Git, looking mirrored, rendered by nothing. That is the exact failure the render-root step exists to prevent. The repairs invert something load-bearing — the render root beating a real declaration, or placement depending on field-ownership metadata. It also sharpens why status is the right shape rather than the cautious one: status can show the ladder without collapsing it, and a spec default can only express the ladder by flattening it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…dered file today
Review turned the argument about a hypothetical CRD default into a live bug, which
is the most useful finding on the page.
`governingKustomization` decides whether a new file gets a `resources:` entry by
looking in exactly two places: the kustomization in the file's own directory, and
the write scope's root — the latter only when render-root scoping is in force. So a
`byType` entry pointing into a subdirectory of a self-contained kustomize folder
("configmaps/{name}.yaml") produces a file no kustomization lists. It is committed,
it looks mirrored, kustomize never builds it, and because no entry is attempted
`placement_kustomization_entries_total` cannot see it either. An overlay reading a
base is registered correctly, by accident of a branch added for another reason.
The fix replaces both cases with one rule: walk up to the nearest kustomization
inside the write jail. It cannot escape the jail by construction, the relative entry
is what `appendKustomizationResource` already computes, and the already-listed check
is path-based so it stays idempotent. It goes first in the plan, because it is a
correctness fix rather than an observability improvement.
It also undercuts F9, and the page now says so instead of keeping an argument that
has been weakened. With the ancestor walk, defaulting `placement.default` would no
longer produce unrendered files. What survives is narrower and structural: a nested
tree registered inside an overlay renders but is the worse layout, and no template
can express "beside the folder's one supported kustomization", so a defaulted
template consumes the slot in front of the one step that exists because a path
cannot say what it says. The recommendation is unchanged; its grounds are smaller.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…overstated arguments A second review found five things wrong with this page, and four of them stand. - `defaultSource: BuiltInCanonical` contradicted the `source: KustomizeRoot` on its own example. It is now `effectiveFallbackSource` with `DeclaredDefault | KustomizeRoot | Canonical`, which answers "what happens to a type I have not named?" in one word instead of describing a different axis. - The retention roll-up proves an enqueue mechanism exists, not that placement status will be fresh. Retention reports on every resync; placement is sparse and may never fire for a stable target. The field is now explicitly two halves: a CURRENT half derived from the last scan and stamped with `observedRevision`, and a HISTORICAL half accumulated since it. - `newFiles` was wrong for an append and `fallbackTypes` was loaded language for a folder whose canonical layout is intentional: `placedResources` and `canonicalTypes`, both defined as historical. - `metadata.managedFields` is field-management bookkeeping, not durable provenance, so it cannot separate a declaration from a schema default. The claim is gone rather than merely hedged. - "Keep writing, make it loud" was a policy choice stated as a consequence. It is open again: a committed manifest nothing applies manufactures a false appearance of convergence, which is the failure class this project ranks first. What is not open is that the signal must not be a third outcome on a counter that counts entry ATTEMPTS. F5b's phrasing is narrowed too: the mechanism is that a default applies to an absent field and is never re-merged per key, not that any write replaces the map. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The placement questions kept dead-ending because the primitive is wrong. A path template cannot say "beside this folder's one kustomization" (which is why that rung is not a template), cannot be read at a glance, and cannot bring a folder into existence. So the proposal is to declare the LAYOUT. `spec.layout.kind` with `Auto`, `Kustomize`, `Tree`, `Flat` and `Template`, plus `byType` overrides valid under every kind. Two rules carry the value: - **whatever chose the path, the file is registered with the kustomization that governs it.** F10 stops being a bug that one `byType` line reproduces and becomes something the model cannot express; - **a structural kind excludes a blanket `default`.** "Kustomize folder AND a nested canonical tree" is statable today, and broken; here it is refused by validation. That also dissolves the defaulting argument this document set out from. Defaults were never the problem: defaulting a PATH was, because a path is the one thing that cannot say "look at the folder". `kind: Auto` is a safe CRD default because it NAMES the structural rule rather than standing in front of it, and it is declared inference, which is the difference between it and the inference we deleted. `kind: Kustomize` with `create: true` answers the bootstrapping ask: the first write commits a folder `kubectl apply -k` can build, rather than a file that happens to be YAML. Its boundary is stated too, because it is the obvious place for scope creep: the layout may create only what its own invariant requires. A repository template is a separate object with a separate lifecycle. Seven worked examples, a status shape carrying `declaredKind` beside the resolved `kind` so declared inference never reads as a user's decision, metric labels, and a mechanical migration for every current configuration (the one behavior change being that a declared template stops silently disabling the render root). On whether the layout should be its own CRD: no, and the decisive argument is the one this release is about. A shared object that changes where N folders write, with nothing on the GitTarget recording it, is structurally the same defect as sibling inference with a different actor. It also adds a readiness chain, cross-namespace authorization, and a third place to look, to share four lines that a generator already repeats for free. The reuse pressure is concentrated in a large `byType` map, so that is what we would share first, projected into status so the target still shows what it is doing. The trigger for revisiting is written down. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… GitTarget work The maintainer review's still-open block (F6, F9, F10, F12's reference nit, §3's pushbacks) and the queue's Tier 2 items (B4, B1, #5, #6) are all `feat(api)!` on the same object as the layout model, so they are cheaper together. That is the weaker half of the argument. The stronger half is that four of them are one decision seen from different angles: **the folder is described on the GitTarget, and the connection describes only the connection.** `spec.layout` says what the folder is, `spec.mode` whether we write it, `spec.suspend` whether we write it now, and `commitWindow`/`commit.message` how those writes are batched and phrased. The last pair lives on `GitProvider` today, which is why §3 says that object is doing three jobs. Shipping the layout alone asserts the principle with one field while another contradicts it. Two findings change the layout design rather than accompanying it, which is the reason to combine rather than merely batch: - **`spec.mode: Observe` becomes how a layout is adopted.** Placement only affects documents that do not exist yet, so a user declaring `kind: Kustomize` on a real repository has nothing to preview. Observe plus `status.layout` is a dry run: resolve the layout, publish what it would do, write nothing, then flip to Write. It also gives Observe a purpose beyond being a switch nobody uses. - **`spec.interval` is what keeps that status fresh.** The scan-derived half of `status.layout` has a hole: a scan happens on a write or a resync, so a stable target may publish a revision from last week. A periodic observation pass closes it, and neither piece was proposed for this reason. Also recorded: `suspend` is a precondition rather than a rider, because a layout that creates a `kustomization.yaml` needs a stop button; F7 already shipped the EventRecorder the placement Event was said to be too expensive for, so that open question is now cheap; layout is mutable like `prune`, and deciding that now keeps #6 from reopening it; F9 stays OUTSIDE the wave because its answer constrains the enum work; and the version stays `v1alpha3` with a loud rejection for `spec.placement` rather than paying for a conversion path while we have one consumer. What rides along without a synergy claim is listed as such: #5, F10, the reference types, the `TooManyStreams` cap, and the ClusterProvider "default" message. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/design/placement-visibility-and-declared-defaults.md`:
- Around line 24-26: Synchronize the ambiguous kustomize-root policy across both
documentation sites: in
docs/design/placement-visibility-and-declared-defaults.md lines 24-26 and its
later ambiguous-root examples, replace “keep writing” or fallback behavior with
refusal; in docs/INDEX.md lines 72-75, update the summary to state that multiple
roots are rejected and document the actual Event and metric behavior.
- Around line 8-10: Revise the scope statement in the document so it does not
claim that all decisions, particularly status.layout and API/CRD schema changes,
land in the current PR. Mark those items as proposals or explicitly exclude them
from the current PR while retaining the same-PR claim only for behavior actually
shipped here.
In `@docs/future/flux-maintainer-review-status-and-config-model.md`:
- Around line 18-25: Update the active “Suggested order” section in the
documentation to remove F9 from the upcoming breaking-change sequence, matching
the introduction’s decision to keep F9 outside the API wave; only retain it if
that section is explicitly labeled as historical.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 9a61622c-9de8-42f0-a4e7-47212cd033a5
📒 Files selected for processing (5)
docs/INDEX.mddocs/design/gittarget-api-wave.mddocs/design/gittarget-layout-model.mddocs/design/placement-visibility-and-declared-defaults.mddocs/future/flux-maintainer-review-status-and-config-model.md
…immutable Review connected four things this design had left apart, and each one changes it. **Namespace scope belongs to the layout.** A folder that omits the namespace from its paths is a folder for one namespace, and that assumption has to be carried by the object that owns the folder. `layout.scope: SingleNamespace|MultiNamespace` is a STRUCTURAL claim; `spec.allowedSourceNamespaces` is an AUTHORIZATION bound; they are different questions about the same folder and admission now checks them against each other. It cannot be derived instead: the matcher may be absent (which `NamespaceMatcher` defines as "no policy declared", not "one namespace"), and the namespaces that arrive come from N WatchRule objects that do not own the folder, so a derived assumption could be invalidated later by an edit elsewhere. Declaring it turns that invalidation into a counted refusal naming both namespaces instead of a collision. **Whether the namespace is written into the file is inference today, and it is the one inference an empty folder cannot perform.** `writeNamespace: FromContext|Always|Never` makes it declarable. `Never` needs a guarantee, because omitting the namespace hands the object to whatever namespace the applier is pointed at. And this closes the bootstrap loop: `create: true` plus `SingleNamespace` lets the operator write `namespace: team-a` into the kustomization it creates and then legitimately omit it from every file. The convention is established rather than guessed. **The layout is immutable, with a widening exception**, which moves it from `prune`'s company to `path`'s. The deciding fact is checkable and I had not checked it: GitTarget has NO finalizer, so deleting one leaves the folder untouched and re-creating it at the same path re-adopts every document by identity. Changing a layout by recreating the object costs status and a moment of mirroring, not data, where `prune`'s mutability argument was that a recreate would destroy what cannot be rebuilt. A mutable layout would leave a folder permanently half one structure and half another, with nothing recording which file came from which. `Flat` to `Tree` widening is allowed because it cannot lose identity-completeness; narrowing is what collides. **And `Auto` resolves once and pins.** Immutability of a field that says "look at the folder" pins nothing: delete the `kustomization.yaml` and `Auto` would silently become `Tree`, which is the defect this release deleted, re-entering through a default value. Pinning also settles whether `Auto` may be the default at all. It may, and the quickstart stays four fields. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A markdownlint failure I pushed past: the previous commit's chain piped lint output through tail, so the shell saw tail's exit status and committed anyway. The row's `FromContext|Always|Never` was read as extra table cells. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This was referenced Jul 30, 2026
…r than pending Attribution was documented across seven files while it was being built, and the one named after the deletecollection expander outlived its subject: it still documented the `result=` label and `attribution_collection_degraded_total`, both of which the fact-stream switchover replaced. Six design records plus that spec are folded into `docs/spec/attribution.md`, which binds and is Vale-gated. Every metric name, tier constant and flag in it was checked against the tree first. What survives elsewhere is the reasoning trail that is still worth reading (`finished/attribution-fact-stream.md`) and the one decision still open (`design/attribution-removal-wait-options.md`). Go and test comments that cited the deleted files now cite the spec; nothing executable changed. The placement page claimed eight items would land in PR #291. None of them did. It shipped the sibling-inference deletion, the three placement counters and the namespace-transformer safety fix, so the page now says which two of its questions were answered by shipping and which six are decided and unbuilt, filed as #295 (correctness) and #296 (visibility). The declared-path-in-a-kustomize-subdirectory bug moves up to Tier 1 in the queue. One ordinary `byType` line silently produces a file that is in Git and rendered by nothing, with nothing in status or the counters saying so, which is this page's own definition of the product being silently wrong. It had been written down as a finding rather than ranked because it was found while arguing about metric names. The layout model and the API wave are postponed to a later deployment and tracked as #293 and #294. 0.41.0 already replaces the whole attribution model and breaks placement; a third breaking dimension, on the shape of GitTarget itself, is a separate conversation. Every Tier 2 entry that changes a GitTarget field is now marked wave-bound rather than independently schedulable, and Tier 1 is explicitly kept free of the wave so it does not wait for it. Net 2,886 lines of markdown deleted, 794 added. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/design/gittarget-layout-model.md`:
- Around line 158-162: Require an explicit namespace source before documenting
or using writeNamespace: Never: in docs/design/gittarget-layout-model.md lines
158-162, declare the namespace/supplier for the bootstrap-created Kustomize
configuration; in docs/design/gittarget-api-wave.md lines 148-151, show the
namespace declaration or make the configuration fail closed; and in
docs/design/gittarget-layout-model.md lines 273-280, use Always or explicitly
declare the build/applier namespace supplier.
In `@docs/finished/redis-key-schema-v3.md`:
- Line 187: Update the paragraph in redis-key-schema-v3.md that claims
deletecollection expands one “:last” entry per member; either mark this
historical explanation as superseded or rewrite it to describe the current
single-collection-fact model, and remove or adjust the attribution spec link so
it does not present the outdated claim as authoritative.
In `@docs/spec/attribution.md`:
- Line 49: Correct the wording in the current contract by replacing “applyable
Git manifest” with “applicable Git manifest,” while preserving the rest of the
sentence unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 9743f708-ec1d-4f6e-a7df-9c6316db4a76
📒 Files selected for processing (28)
.docs-lint-scopedocs/INDEX.mddocs/TODO.mddocs/architecture.mddocs/design/attribution-branch-findings.mddocs/design/attribution-deletion-intent-actor.mddocs/design/attribution-fact-identity.mddocs/design/attribution-metrics-proposal.mddocs/design/attribution-publish-and-join.mddocs/design/attribution-removal-wait-options.mddocs/design/attribution-wait-poll-vs-push.mddocs/design/docs-linting.mddocs/design/gittarget-api-wave.mddocs/design/gittarget-layout-model.mddocs/design/metrics-observability-plan.mddocs/design/open-asks-priority.mddocs/design/placement-visibility-and-declared-defaults.mddocs/finished/attribution-fact-stream.mddocs/finished/redis-key-schema-v3.mddocs/spec/README.mddocs/spec/attribution.mddocs/spec/deletecollection-attribution-expander.mdinternal/queue/author_fact.gointernal/queue/fact_index_store.gointernal/watch/target_watch.gotest/e2e/audit_route_attribution_e2e_test.gotest/e2e/deletecollection_intent_e2e_test.gotest/mutationlab/README.md
💤 Files with no reviewable changes (7)
- docs/design/attribution-metrics-proposal.md
- docs/spec/deletecollection-attribution-expander.md
- docs/design/attribution-branch-findings.md
- docs/design/attribution-publish-and-join.md
- docs/design/attribution-deletion-intent-actor.md
- docs/design/attribution-wait-poll-vs-push.md
- docs/design/attribution-fact-identity.md
🚧 Files skipped from review as they are similar to previous changes (5)
- docs/architecture.md
- docs/design/metrics-observability-plan.md
- docs/design/open-asks-priority.md
- docs/design/placement-visibility-and-declared-defaults.md
- docs/INDEX.md
…e kustomization `appendKustomizationResource` ran before `placeNewDocument`, so a placement the writer then refused still gained a `resources:` entry. The reachable case is the multi-document refusal: the file exists, holds a document we cannot account for, and we decline to own it — then registered it into the folder's render anyway, counted as `outcome="added"`, which is the value that is supposed to mean "the file we just wrote will build". The mixed-sensitivity refusal cannot reach it, because it requires a document already written at that path in the same batch, so its entry is legitimate. That is why the fix is pinned by a multi-document fixture and asserts the kustomization is byte-identical after the refusal rather than only that the counter is zero. Moved after `wroteBytes(outcome)`, where `recordPlacement` already sits and for the same reason. Review follow-ups in the same pass: - `configuration.md` had the last two live claims that omitting `spec.placement` follows the repository's existing layout. Sibling inference is gone; it takes the folder's one kustomization root or the canonical path. - `UPGRADING.md` gives the canonical path in full, since it is the page read to predict where a file lands: `_cluster/`, the omitted core group, no version segment, `.sops.yaml`. - The placement spec said every resolution increments `placements_total`, which its own test contradicts. It is every successful placement. - The maintainer review still scheduled F9 inside the API wave while its own introduction says F9 is deliberately outside it. - `redis-key-schema-v3.md` describes the deleted expander. Repointing its link at the new spec in the previous commit made that claim look authoritative, so it is marked superseded and says what replaced it. - The layout model's `Flat` example used `writeNamespace: Never`, which its own table forbids without a namespace guarantor — and `Flat` has no kustomization to write `namespace:` into. It is `Always`. The sharper gap is recorded as an open question: `SingleNamespace` constrains cardinality and never names the namespace, which is exactly what bootstrapping needs. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
Author
Review responseNine findings. Six fixed, one of them a real bug with a test. Two pushed back on, with the reason. The one that mattered:
|
sunib
changed the title
Contributor
Author
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
#290 has merged, so this now targets
maindirectly.Two placement defects, one of them by design. The larger one is that repository state decided where the operator writes: sibling inference read the folder's existing layout, so a human's edit to the repository changed the operator's behaviour with no Kubernetes object changing and nothing in status recording it. That is the Tier 1 item #290's queue reconciliation left ranked, and it is deleted here. The smaller one was found in review of this very PR: a resource the writer refused still had its file registered with the folder's kustomization. Both are fixes, which is why this is
fix(placement)!rather thanfeat.It also ships the placement observability the spec made mandatory and never built, and a documentation consolidation described under docs: attribution is documented in one place instead of seven, and the breaking
GitTargetwork is postponed to a later deployment and filed as issues rather than left reading as pending.The deletion
resolveInferredthroughallSameDirare gone — about a third ofplacement.go, plus the tests that pinned each rung. A resource with no document in Git yet now gets its path from exactly three things:placements_total{source}placement.byType/.defaultdeclaredkustomize_root{namespaceOrCluster}/{group}/{resource}/{name}.yamlcanonicalThe argument is not primarily the bug. Inference let a human's edit to the repository change where the operator writes, with no Kubernetes object changing and nothing in status recording the move. The bug is the evidence: its namespace-agnosticism guard was vacuous on the singleton branch for a period, so a new namespace's object was appended into the first namespace's file, which then genuinely spanned two namespaces, which legitimized the bundle for every later object, which collapsed a whole type into one file. That fix was right, and it did not make the class safe — a rule inferred from mutable state has failure modes that feed themselves. Two further reasons, both from the spec's own risk list: the explainability P8 declared mandatory was never built, and P4's per-namespace layout — the one most likely to be hand-authored — was the one inference could not extend anyway, so the user had to declare it regardless.
The kustomize-root fallback stays, because it is not inference: a file no kustomization can reach is not oddly placed, it is never rendered. More than one supported kustomization is still ambiguous and still declines.
No
spec.placement.modeenum. An off-switch for a removed feature is a permanent API field bought to solve a temporary problem. The config-surface proposal's B3 is answered by the deletion, and so is consumer ask #10.Two things came out of building it that the design note did not contain:
metadata.namespace, the build context supplies it" was read off a sibling's bytes, so it only ever fired for an inferred placement — a declared path into the same kustomize directory wrote anamespace:line every other document in that folder omits. It now comes from the kustomization that governs the destination, which is the thing that actually decides the rendered namespace, so the declared path is fixed for free.namespace:transformer was set, never whether it named the resource's own namespace. Omitting the namespace hands it to kustomize, so a transformer naming a different namespace rendered the document as a different object — the mirror claiming to hold a resource it does not. It now writes the namespace explicitly there and lets the render oracle report a folder that cannot express the object.The refusal that registered its file anyway
Found reviewing this PR, in this PR's own new code, and fixed in
84f96b73.writeNewDocumentcalledappendKustomizationResourcebeforeplaceNewDocument. The second of those can still decline to write — and when it did, theresources:entry had already been added. So a resource the operator refused to place had its file registered with the folder's kustomization anyway, and the entry was countedoutcome="added", which is the value that is supposed to mean the file we just wrote will build.Which refusal reaches it is the whole question, and it is not the obvious one. The review suggested the failure was
kustomize buildbreaking on an entry naming a file that was never written. That case is unreachable: the mixed-sensitivity refusal only fires whenbuf.current != nil, which means a document was already written at that path in this batch, so its entry is legitimate.The reachable one is the multi-document refusal. There
buf.original != nil: the file already existed in the repository, holds a document the writer cannot account for, and the writer declines to overwrite it precisely so it does not drop someone else's content. Registering it anyway put foreign content into the folder's render on our say-so — the one thing the refusal existed to avoid.The fix moves the call after
wroteBytes(outcome), which is exactly whererecordPlacementalready sits, and for the same reason: that is the point at which the document is really in the mirror.Two notes on how it is pinned, because the first attempt did not pin anything:
LocateNew, which returns before the old call site. A test that passes both ways pins nothing, so the fixture was rebuilt around a genuine multi-document target:The metrics
Placement had two signals and neither was reachable: a log line, and
ResyncStats.PlacementSkipped— a field in a resync summary. With inference gone, "which target and which type needs abyTypeline" has to be answerable without reading the folder.Three counters, all labelled
{gittarget_namespace, gittarget_name, group, version, resource}— the target that owns the write, and the exact shape of aplacement.byTypekey, so one series reads as the line that is missing:placements_total{source, disposition}— one per new document written.dispositionisnew_file/appended;source="canonical"is the missing-rule signal, andkustomize_rootis deliberately not lumped in with it, or every well-formed overlay would read as misconfigured.placement_refusals_total{reason}— one per resource the writer declined to place, from a closed set (invalid_path,sensitive_append,plaintext_onto_encrypted,mixed_sensitivity_new_file,multi_document_target). Every increment is a resource absent from the mirror.placement_kustomization_entries_total{outcome}—added/no_change/failed.failedis the invisible one: the document is committed and itsresources:entry is not, so kustomize never builds the file. It is in Git, it looks mirrored, and nothing applies it.Three decisions worth naming:
sourcevalue would let a dashboard count a skipped Secret as a successful placement.PlacementRefusedError.Reason), not matched from a message, so the label cannot drift when an error string is reworded.open-asks-priority.mdargued against leading with a counter —placement_fell_back_total"says it happened somewhere, not which type in which target". That objection was to the labels, and it does not survive them being fixed. The doc now says so rather than quietly shipping the opposite of its own recommendation. The Event on the GitTarget andstatus.layoutare still the right split for timeliness and durability, and both are still unbuilt.Compatibility
A behaviour change, with a
docs/UPGRADING.mdentry. A target whose repository this operator created is unaffected (that folder was already canonical, which inference also produced). A target pointed at a hand-authored folder is affected: a new resource of a type that folder already holds now takes the canonical path instead of joining the existing file. Nothing already in Git moves — an existing document is still edited in place, forever. OnebyTypeline restores the old behaviour and says on the page what used to be a guess. Kustomize folders need no declaration.No CRD schema change (two field descriptions moved).
PlacementResult.CohortandPlacementSourceInferredare gone frominternal/;kustomize_rootreplacesinferred, which now names one mechanism rather than two.Docs
The spec binds the code, so
gittarget-new-file-placement-rules.mdis rewritten: three steps live, Option C kept as history with each of P1–P10 annotated by what became of it (P1/P2/P3/P4/P6/P8 are one property stated six times and are retired; P7/P9/P10 are facts about the code that remains).configuration.md,architecture.md,installing-apps-as-krm.md,interpreting-metrics.md, the metrics plan,INDEX.mdand the queue doc follow.One attribution spec, folded from seven documents
Attribution was documented across seven files while it was being built, and the one named after the
deletecollectionexpander outlived its subject — it still documented theresult=label andattribution_collection_degraded_total, both of which the fact-stream switchover replaced. Six design records plus that spec are folded intodocs/spec/attribution.md, which binds and is Vale-gated: deletion-at-intent, the publish and join halves, the tier ladder, theauditRoutepartition, the transports, and what the metric surface deliberately cannot answer.Every metric name, tier constant and flag in it was checked against the tree before it was written. What survives elsewhere is the reasoning trail still worth reading (
finished/attribution-fact-stream.md) and the one decision still open (design/attribution-removal-wait-options.md). Go and test comments that cited the deleted files now cite the spec; nothing executable changed — the whole Go diff in this commit is six doc-reference comments.Net 2,886 lines of markdown deleted, 794 added.
The placement page said eight things would land here. None of them did
placement-visibility-and-declared-defaults.mdclaimed its full build list landed in this PR. What landed is the deletion, the three counters and the namespace-transformer fix. The page now says which two of its questions were answered by shipping and which six are decided and unbuilt, filed as:status.layout, the ambiguous render root, thedeclaredmetric split,{kindLower}, canonical-as-template.One re-ranking. The declared-path-in-a-subdirectory bug moves to Tier 1. One ordinary
byTypeline silently produces a file that is in Git and rendered by nothing, with nothing in status or the counters saying so — which is the queue's own definition of the product being silently wrong. It had been written down as a finding rather than ranked, because it was found while arguing about metric names. It is not fixed here.The GitTarget wave is postponed, not pending
The layout model and the breaking API wave are filed as #293 (
spec.layout: declare what the folder is) and #294 (the wave:spec.mode,spec.suspend, thecommitWindowmove offGitProvider, CommitRequest lifecycle). 0.41.0 already replaces the whole attribution model and breaks placement; a third breaking dimension, on the shape ofGitTargetitself, is a separate conversation.The queue reflects that: every Tier 2 entry that changes a
GitTargetfield is marked wave-bound rather than independently schedulable, and Tier 1 is explicitly kept free of the wave so it does not wait for it.Validation
task fmt,task vet,task lint,task test(coverage 78.4%, baseline 78.5%, within tolerance),task lint-docs(doccheckresolves every reference across 203 markdown and 492 Go files), andtask gitops-layouts-baseline(no movement — placement is not part of the scan).task test-e2ewas not completed locally for the docs commit. A local run reachedSynchronizedAfterSuite PASSEDbut its shell was killed before Ginkgo wrote the report, so there is no valid local record and I am not claiming one. The suite passed locally for the code in this PR before that commit; the commit added no executable change, so the CI e2e legs are the gate for it.One note for whoever reads a red run here: an earlier CI run failed Unit tests on a single envtest timeout (
GitTarget Controller Security / Should recreate encryption secret when it is deleted while GitTarget still exists, 45s). The preceding run on the same branch was green, the commits between them were docs-only, andtask testpasses locally. That is a flake, not this diff.🤖 Generated with Claude Code
Summary by CodeRabbit
Changed
kustomization.yamlroot (when unambiguous), then the built-in canonical path.resources:accordingly.Observability
placements_total,placement_refusals_total(with reasons), andplacement_kustomization_entries_total.Documentation