feat(gitops-api): place new resources to match the repo's existing layout#202
Merged
Conversation
… design Records the product-direction refinements agreed for the GitOps API workstream: the kustomize field taxonomy and supported-layout allowlist, the write-fan-in-=1 invariant for overlays, the mirror-mode/intent-mode topology, and the three-tier unreflectable-edit accounting (per-edit FullyReflected reporting plus the opt-in F6 admission preflight). Updates the feature ladder and launch use cases in README.md accordingly. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ayout (F4)
Implements the F4 new-file placement design
(docs/design/manifest/version2/gittarget-new-file-placement-rules.md): a
resource with no existing document in Git is no longer always written to
the canonical {group}/{version}/{resource}/{namespace}/{name}.yaml path.
Placement now resolves in order:
1. GitTargetSpec.Placement (Option B) — a declared type-map/default
template per sensitive/normal class, rendered via a small
brace-variable path-template language.
2. Sibling inference (Option C, steps 1/2 — same type+namespace, then
same type any namespace): a new resource follows the layout already
established by its siblings (one-per-file vs. a shared bundle),
with a deterministic tie-break and the P4 safety rule that a
per-namespace-segmented layout is never guessed for an unseen
namespace. Step 3 (same namespace, any type) is deliberately not
implemented, per the design doc's own P5 risk analysis.
3. A narrow kustomize-root fallback: when the whole GitTarget subtree
is governed by exactly one supported kustomization, a genuinely new
type still lands beside that kustomization's other files rather
than in the unreachable canonical tree.
4. The canonical path, unchanged, when nothing else applies.
Folds in the "add to the right kustomize file" half of F4: when a new
file lands in a directory a supported kustomization already governs,
its resources: entry is added in the same commit
(manifestedit.AppendKustomizationResource), preserving comments and
order and never touching an unsupported kustomization.
A file already holding a document the writer tolerates but cannot fully
account for (e.g. a YAML anchor) is excluded from both bundle and
singleton-style candidacy, so placement can never silently join or
overwrite content it does not understand — writeWholeFile's existing
multi-document guard remains the backstop.
The declared policy is validated statically (unknown template
variables, byType key syntax, sensitive-template SOPS suffix and
identity-completeness) as part of the GitTarget Validated gate, before
any repository scan.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThis PR adds GitTarget placement policy support for new resources, including API/CRD schema changes, controller validation, placement resolution, write-path wiring, kustomization ChangesF4 Placement Feature
Estimated code review effort: 4 (Complex) | ~75 minutes Sequence Diagram(s)sequenceDiagram
participant CommitExecutor
participant WriteBatch as writeBatch
participant PlacementEngine as manifestanalyzer.LocateNew
participant ManifestStore
participant KustomizationEdit as manifestedit.AppendKustomizationResource
CommitExecutor->>WriteBatch: executePendingWrite(ctx, targets)
WriteBatch->>WriteBatch: applyUpsert -> createNew(event)
WriteBatch->>PlacementEngine: LocateNew(store, policy, request)
PlacementEngine->>ManifestStore: inspect documents and kustomizations
ManifestStore-->>PlacementEngine: placement candidates and kustomization info
PlacementEngine-->>WriteBatch: PlacementResult(path, append, namespace, kustomization)
alt append existing bundle
WriteBatch->>WriteBatch: appendNewDocument / appendYAMLDocument
else write new file
WriteBatch->>WriteBatch: create content for resolved path
end
opt add kustomization entry
WriteBatch->>KustomizationEdit: AppendKustomizationResource(path, content, entry)
KustomizationEdit-->>WriteBatch: EditPatched / EditNoChange / EditSkipped
end
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! |
…ntext
The e2e "Manager Manifest Folder Editing" spec caught a real bug: a
built-in, cluster-injected ConfigMap (kube-root-ca.crt) watched by too
broad a WatchRule has no existing document, so it went through F4's new
sibling-inference path — and landed appended into a hand-authored
multi-document bundle whose other members infer their namespace from a
kustomization's namespace: transformer. Because createNew wrote the
live object's bytes verbatim, the appended document carried an explicit
metadata.namespace, breaking the "no namespace: in this file"
convention every sibling in that bundle already followed and failing
the test's own comment-and-content assertions.
patchExisting already strips metadata.namespace before patching an
existing document whose NamespaceSource is kustomize-inherited
(DocumentModel.NamespaceInheritedFromContext); createNew had no
equivalent for a document that doesn't exist yet. This is exactly the
gap the design doc's own Option C test plan calls out ("the new file
inherits its sibling's NamespaceSource") and that I missed testing for
in the original implementation.
Fixes it by threading a NamespaceInherited signal through
PlacementResult: cohortDestination now tracks one representative
sibling per candidate destination and reports whether it infers its
namespace from context, and resolveKustomizeRoot reports the same when
the GitTarget's one kustomization declares a namespace: transformer.
createNew strips the new document's metadata.namespace whenever
NamespaceInherited is true, before building its content.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Contributor
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
Adds targeted tests for the branches Codecov's patch-coverage report flagged as missing, each proving a real behavior rather than chasing the number for its own sake: - ValidPlacementTemplateSyntax / placementVariableNames had no direct test in their own package (the controller test exercising them cross-package doesn't count toward manifestanalyzer's coverage). - resolveKustomizeRoot: two supported kustomizations under one GitTarget is ambiguous (falls to canonical); the fallback also works for a sensitive resource. - A declared template with an unknown variable falls through to inference/canonical instead of erroring the whole placement. - cohortMembers never conflates a sensitive and a normal document of the same type sharing one namespace (sensitivity is an encryption fact, not a type fact). - A tainted sibling (a document tolerated despite a disallowed construct, e.g. a YAML anchor) is never joined as a bundle or singleton-style destination. - fileIsAppendSafe / spansMultipleNamespaces direct nil/edge-case tests. - placementVars for a grouped, cluster-scoped resource. - resolvePlacementPolicy (CRD spec -> analyzer policy) had no direct test at all. - createNew's placement-error skip path (a misconfigured sensitive template colliding with an existing file) proven end-to-end through the real flush path: no crash, no write, existing file untouched. - appendYAMLDocument's three branches (empty existing, missing/present trailing newline) tested directly. - appendKustomizationResource's vanished-buffer guard and its skip-with-diagnostic path (a kustomization accepted by the analyzer whose resources: field is a malformed non-sequence) proven end to end: the resource's own file still lands, the kustomization is left byte-for-byte untouched. - evaluateValidatedGate's new branch tested against the reconciler method itself (not just the pure validatePlacementPolicy function), using the existing fake-client unit-test pattern: an otherwise-valid GitTarget with an invalid placement policy fails Validated with reason InvalidConfig. Unit coverage 74.8% -> 75.0%; .coverage-baseline bumped accordingly. Co-Authored-By: Claude Sonnet 5 <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: 2
🧹 Nitpick comments (2)
api/v1alpha3/gittarget_types.go (1)
91-118: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winConsider adding kubebuilder validation tags for the new placement fields.
Only
+optionalmarkers are present; no+kubebuilder:validation:*tags (e.g. pattern forByTypekeys,MinLengthonDefault) were added. Full template/identity-completeness validation clearly needs controller logic, but basic shape checks at the CRD/admission level would catch obvious misconfigurations earlier than the reconcile-timeValidatedcondition.As per coding guidelines, "For API changes, add kubebuilder validation tags, include JSON tags and field descriptions, and update CRDs with
task manifests."🤖 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 `@api/v1alpha3/gittarget_types.go` around lines 91 - 118, Add kubebuilder validation tags to the new placement API fields in GitTargetPlacementSpec and GitTargetPlacementClass so basic misconfigurations are rejected at admission time. Update the struct tags on Sensitive, Normal, ByType, and Default with appropriate validation such as key/value shape constraints for ByType and non-empty constraints for Default, while keeping the existing descriptions and JSON tags intact. After updating the types, regenerate the CRDs and manifests so the new validation rules are included.Source: Coding guidelines
internal/manifestanalyzer/placement.go (1)
162-181: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNo test coverage for the ambiguous multi-kustomization case.
resolveKustomizeRootcorrectly declines when more than one supported kustomization exists under the scanned root (Lines 168-170), butplacement_test.godoesn't appear to exercise this branch. Given this is the safety rule the function's own doc comment leans on heavily ("More than one supported kustomization … is ambiguous and declines rather than guessing"), a regression test would be valuable. As per path instructions,**/*.gochanges should be "Cover[ed] … with tests and ensure total coverage does not regress."🤖 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.go` around lines 162 - 181, Add test coverage for the ambiguous multi-kustomization path in resolveKustomizeRoot: create a case in placement_test.go where store.Kustomizations contains more than one supported KustomizationInfo and verify the function returns the declined result. Use resolveKustomizeRoot and PlacementRequest in the test so the branch that returns false when multiple supported kustomizations are present is explicitly covered and total coverage does not regress.Source: Path instructions
🤖 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 `@internal/controller/gittarget_placement_validation.go`:
- Around line 32-49: The error returned by validatePlacementClass is
non-deterministic because it ranges directly over class.ByType, so different
invalid entries can surface on each reconcile. Update validatePlacementClass to
collect and sort the ByType keys before validating them, then iterate in that
stable order so the first reported error and the Validated condition message
remain deterministic across runs.
In `@internal/git/plan_flush.go`:
- Around line 211-254: The unsafe-placement path in createNew is currently
hidden because it returns upsertNoChange, which makes applyResyncPlan treat it
like a normal no-op. Update createNew to return a distinct upsertOutcome for the
LocateNew error case, and wire that outcome through applyResyncPlan so
ResyncStats can count it separately instead of relying only on the Info log. Use
the existing outcome handling around upsertNoChange, plan.Counts(), and
stats.Skipped as the place to extend the reconcile status reporting.
---
Nitpick comments:
In `@api/v1alpha3/gittarget_types.go`:
- Around line 91-118: Add kubebuilder validation tags to the new placement API
fields in GitTargetPlacementSpec and GitTargetPlacementClass so basic
misconfigurations are rejected at admission time. Update the struct tags on
Sensitive, Normal, ByType, and Default with appropriate validation such as
key/value shape constraints for ByType and non-empty constraints for Default,
while keeping the existing descriptions and JSON tags intact. After updating the
types, regenerate the CRDs and manifests so the new validation rules are
included.
In `@internal/manifestanalyzer/placement.go`:
- Around line 162-181: Add test coverage for the ambiguous multi-kustomization
path in resolveKustomizeRoot: create a case in placement_test.go where
store.Kustomizations contains more than one supported KustomizationInfo and
verify the function returns the declined result. Use resolveKustomizeRoot and
PlacementRequest in the test so the branch that returns false when multiple
supported kustomizations are present is explicitly covered and total coverage
does not regress.
🪄 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: 939c6455-1380-4817-adfa-5b310c7b15d3
📒 Files selected for processing (30)
.coverage-baselineapi/v1alpha3/gittarget_types.goapi/v1alpha3/zz_generated.deepcopy.goconfig/crd/bases/configbutler.ai_gittargets.yamldocs/design/gitops-api/README.mddocs/design/gitops-api/kustomize-support-boundary-and-product-model.mddocs/design/gitops-api/unreflectable-edits-and-write-gating.mdinternal/controller/gittarget_controller.gointernal/controller/gittarget_placement_validation.gointernal/controller/gittarget_placement_validation_test.gointernal/git/acceptance_gate_test.gointernal/git/commit_executor.gointernal/git/fieldpatch_flush_test.gointernal/git/inplace_edit_test.gointernal/git/inplace_overrides_test.gointernal/git/manifestedit/kustomization.gointernal/git/manifestedit/kustomization_test.gointernal/git/pending_writes.gointernal/git/placement_test.gointernal/git/plan_flush.gointernal/git/plan_flush_test.gointernal/git/resync_flush.gointernal/git/resync_flush_test.gointernal/git/types.gointernal/manifestanalyzer/placement.gointernal/manifestanalyzer/placement_test.gointernal/manifestanalyzer/store.gotest/e2e/f4_placement_e2e_test.gotest/e2e/fixtures/f4-placement-folder/deployment.yamltest/e2e/fixtures/f4-placement-folder/kustomization.yaml
Addresses two review blockers on the F4 new-file placement work.
Fix batch-collision data loss: LocateNew resolves every event against
the pre-batch store snapshot (by design, for P2 order-independence), so
when two new resources in the same commit/resync batch render to the
same brand-new path, neither placement decision can see the other
coming. createNew routed both through writeWholeFile, and the second
call silently discarded the first resource's document — the design doc
explicitly requires this case to become a deterministic multi-document
file instead ("Collision and append behavior"). writeBatch now tracks
cold-bundle membership per path and rebuilds the file, sorted by
resource identity, on every collision, so the result is independent of
event arrival order; a sensitive resource colliding with another new
resource in the same batch is refused rather than merged, matching the
existing pre-batch sensitive-collision rule. Covers both the
steady-state and resync entry points.
Fix incomplete path validation: a declared placement template's own
literal text was validated only for known variable names, so a
misconfigured or malicious template like "../outside.yaml" could
render a path that escapes the GitTarget's spec.path once flush joins
base and the resolved path. Adds two layers: ValidPlacementTemplatePath
statically rejects a bad template's literal text (parent traversal,
absolute path, backslash separators, wrong suffix) at the GitTarget's
Validated gate, before any resource can trigger a write; and the new
ValidateResolvedPlacementPath runs inside finishPlacement as a runtime
backstop against every resolution path (declared, inferred, the
kustomize-root fallback, and canonical alike) so a bad path can never
reach the writer even if static validation is bypassed or stale.
Also documents the kustomize-root fallback in the design doc: the
implementation added this rule during F4 build-out to keep a brand-new
resource type inside its kustomization-managed folder rather than
falling through to the (unreachable, from the kustomization's
resources: graph) canonical tree, but the doc still described step 4 as
a silent "no match -> canonical" — it now records the fallback as a
deliberate, scoped rule with its own rationale, matching the
implementation.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Normal placement (byType/default) moves to the top level of GitTargetPlacementSpec instead of a nested `normal:` block; only `sensitive:` stays nested as the guarded override. The common case (no encryption in play) now reads as byType/default directly under placement, with sensitive/nested only where the extra ceremony actually earns its keep. Internal PlacementPolicy/PlacementPolicyClass and all placement logic are unaffected — this is a pure spec-shape change, covered by updating the conversion and validation tests to the new field paths. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…tivity as write-safety)
Drop the placement CRD's `sensitive:` block entirely. GitTargetPlacementSpec
is now just `{byType, default}`: a single declared map consulted for every
resource, sensitive or not. Sensitivity stops being a second placement
namespace the user has to configure and becomes purely a controller-owned
write-safety property. This is the smaller, additive-later surface — a
`sensitive:` block can be reintroduced non-breakingly if a real need for a
distinct sensitive *path* ever appears; removing it later would not be.
Dropping the API split removes no part of the encryption guarantee, because
every piece of it already lived outside the placement block and stays there:
content is encrypted by classification (SensitiveResourcePolicy), sibling
inference never crosses the CauseEncrypted boundary, a sensitive resource is
never appended to an existing file, and the canonical fallback stays SOPS.
What B1's split additionally provided — a Secret can never reach a shared or
plaintext file — is preserved by moving it from "structural (two maps)" to one
static check plus two write-time guards, so it now holds for every sensitive
type (core and operator-configured), not only those a user listed:
- Validated gate: an explicit byType["v1/secrets"] route must be
identity-complete, and a bundling default (e.g. "all.yaml") is rejected
unless such a route exists, so core Secrets can never fall through a bundle.
- Write-time guard 1: a sensitive and a plaintext resource that collide on a
brand-new file are never co-mingled, whichever event arrives first.
- Write-time guard 2: a plaintext resource is refused (not appended, not
overwritten) when its path already holds an encrypted document.
The residual (an operator-configured additional sensitive type under a bundling
default with no byType entry) fails safe: skipped with a diagnostic, never
mixed. Documented, with an open question on promoting it to a gate rejection.
The design doc is updated to record B2 as decided/implemented with these notes;
tests cover both new guards (analyzer + git level) and the B2 validation rules.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
docs/design/manifest/version2/gittarget-new-file-placement-rules.md (1)
837-842: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winClarify the narrowed-template rule for cluster-scoped resources
The narrowed-to-one-type case still needs a scope variable:
IdentityCompletePlacementTemplaterejects{name}-only templates, and the test expects{namespace}or{namespaceOrCluster}even for narrowed entries. Update the cluster-scoped example here to match that rule.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/design/manifest/version2/gittarget-new-file-placement-rules.md` around lines 837 - 842, The narrowed-template guidance for cluster-scoped resources is inconsistent with IdentityCompletePlacementTemplate expectations. Update the cluster-scoped example in the placement rules section so it includes the required scope variable, using {namespaceOrCluster} or the equivalent scope placeholder rather than a {name}-only template, and keep the wording aligned with the narrowed-to-one-type rule described alongside the namespaced example.internal/manifestanalyzer/placement.go (2)
207-231: 🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick winReject existing-file collisions independently of append eligibility.
Line 211 and Line 225 only guard when
res.Appendis true. If the target file exists but is not append-safe,Appendremains false and the caller can take the whole-file write path for the same path, bypassing the sensitive/encrypted-file collision refusal.Proposed fix
fm, exists := store.FilesByPath[resolvedPath] - if exists && fileIsAppendSafe(fm) { + if exists && fileIsAppendSafe(fm) { res.Append = true } - if req.Sensitive && res.Append { + if req.Sensitive && exists { return PlacementResult{}, fmt.Errorf( - "placement for sensitive resource %s resolved to %q, which already holds a document; "+ - "sensitive resources are never appended to an existing file", + "placement for sensitive resource %s resolved to %q, which already exists; "+ + "sensitive resources are never placed into an existing file", req.Identifier.String(), resolvedPath, ) } @@ - if res.Append && !req.Sensitive && fileHoldsEncryptedDocument(fm) { + if exists && !req.Sensitive && fileHoldsEncryptedDocument(fm) { return PlacementResult{}, fmt.Errorf( "placement for resource %s resolved to %q, which already holds an encrypted document; "+ "a plaintext resource is never appended to an encrypted file",Please add a regression test for the
exists && !fileIsAppendSafe(fm)collision branch. As per coding guidelines, “Cover new Go code with tests and ensure total coverage does not regress.”🤖 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.go` around lines 207 - 231, Reject existing-file collisions even when append is not allowed: in `placement.go`, the guards around `res.Append` in `Placement` only run for append-safe files, so an existing target can still fall through to whole-file writes. Update the collision checks near `fileIsAppendSafe`, `fileHoldsEncryptedDocument`, and the `PlacementResult` construction so existing-file/sensitive-encrypted conflicts are refused independently of append eligibility, then add a regression test covering the `exists && !fileIsAppendSafe(fm)` branch.Source: Coding guidelines
302-318: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winKeep template validation and rendering on the same normalized path.
Line 467 validates a trimmed template, but Line 316 renders the original string; also, static validation currently lets non-clean literals like
./name.yamlordir//name.yamlpass the Validated gate and fail later per resource.Proposed fix
func resolveDeclared(policy *PlacementPolicy, req PlacementRequest, vars map[string]string) (string, bool, error) { if policy == nil { return "", false, nil } key := PlacementTypeKey(req.Identifier.Group, req.Identifier.Version, req.Identifier.Resource) + byTypeTemplate := strings.TrimSpace(policy.ByType[key]) + defaultTemplate := strings.TrimSpace(policy.Default) var tmpl string switch { - case strings.TrimSpace(policy.ByType[key]) != "": - tmpl = policy.ByType[key] - case strings.TrimSpace(policy.Default) != "": - tmpl = policy.Default + case byTypeTemplate != "": + tmpl = byTypeTemplate + case defaultTemplate != "": + tmpl = defaultTemplate default: return "", false, nil }if strings.HasPrefix(trimmed, "/") { return fmt.Errorf("placement template %q must be relative, not absolute", tmpl) } + if cleaned := path.Clean(trimmed); cleaned != trimmed || cleaned == "." || strings.HasPrefix(cleaned, "../") { + return fmt.Errorf("placement template %q must be a clean relative path", tmpl) + } for _, segment := range strings.Split(trimmed, "/") { if segment == ".." { return fmt.Errorf("placement template %q must not contain a \"..\" path segment", tmpl) }Also applies to: 466-486
🤖 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.go` around lines 302 - 318, Template validation and rendering are using different inputs, so make them share one normalized path in resolveDeclared and the validation logic around the Validated gate. Trim/clean the chosen template once, use that same normalized value for both RenderPlacementTemplate and the static validation check, and tighten validation so literals like ./name.yaml and dir//name.yaml are rejected before per-resource rendering. Keep the behavior consistent for PlacementPolicy.ByType and PlacementPolicy.Default so the same normalized template is what gets validated and rendered.
♻️ Duplicate comments (1)
internal/controller/gittarget_placement_validation.go (1)
33-53: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winNon-deterministic error message: unsorted
ByTypemap iteration reintroduced.Line 37 ranges over
spec.ByTypewithout sorting keys. With multiple invalidbyTypeentries, the specific error surfaced (and theValidatedcondition message set byevaluateValidatedGate) will vary across reconciles for an unchanged spec — the same non-determinism previously flagged onvalidatePlacementClassbefore the B1→B2 API simplification. As per coding guidelines,internal/controller/**/*.gomust "implement idempotent reconciliation logic."🔧 Proposed fix: sort keys for deterministic ordering
+import "sort" + func validatePlacementPolicy(spec *configbutleraiv1alpha3.GitTargetPlacementSpec) (bool, string) { if spec == nil { return true, "" } - for key, tmpl := range spec.ByType { + keys := make([]string, 0, len(spec.ByType)) + for key := range spec.ByType { + keys = append(keys, key) + } + sort.Strings(keys) + for _, key := range keys { + tmpl := spec.ByType[key] if !validPlacementTypeKeySyntax(key) { return false, fmt.Sprintf( "placement byType key %q is not a valid \"[group/]version/resource\" type key", key, ) } if msg, bad := validatePlacementTemplate(tmpl); bad { return false, fmt.Sprintf("placement byType[%q]: %s", key, msg) } }🤖 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/controller/gittarget_placement_validation.go` around lines 33 - 53, The issue is that validatePlacementPolicy iterates over spec.ByType directly, which makes the first returned validation error depend on map iteration order. Update validatePlacementPolicy to collect and sort the ByType keys before validating each entry so the error surfaced is deterministic across reconciles. Keep the existing validatePlacementTemplate and validPlacementTypeKeySyntax checks, but perform them in sorted key order so evaluateValidatedGate sees a stable message for the same spec.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@docs/design/manifest/version2/gittarget-new-file-placement-rules.md`:
- Around line 837-842: The narrowed-template guidance for cluster-scoped
resources is inconsistent with IdentityCompletePlacementTemplate expectations.
Update the cluster-scoped example in the placement rules section so it includes
the required scope variable, using {namespaceOrCluster} or the equivalent scope
placeholder rather than a {name}-only template, and keep the wording aligned
with the narrowed-to-one-type rule described alongside the namespaced example.
In `@internal/manifestanalyzer/placement.go`:
- Around line 207-231: Reject existing-file collisions even when append is not
allowed: in `placement.go`, the guards around `res.Append` in `Placement` only
run for append-safe files, so an existing target can still fall through to
whole-file writes. Update the collision checks near `fileIsAppendSafe`,
`fileHoldsEncryptedDocument`, and the `PlacementResult` construction so
existing-file/sensitive-encrypted conflicts are refused independently of append
eligibility, then add a regression test covering the `exists &&
!fileIsAppendSafe(fm)` branch.
- Around line 302-318: Template validation and rendering are using different
inputs, so make them share one normalized path in resolveDeclared and the
validation logic around the Validated gate. Trim/clean the chosen template once,
use that same normalized value for both RenderPlacementTemplate and the static
validation check, and tighten validation so literals like ./name.yaml and
dir//name.yaml are rejected before per-resource rendering. Keep the behavior
consistent for PlacementPolicy.ByType and PlacementPolicy.Default so the same
normalized template is what gets validated and rendered.
---
Duplicate comments:
In `@internal/controller/gittarget_placement_validation.go`:
- Around line 33-53: The issue is that validatePlacementPolicy iterates over
spec.ByType directly, which makes the first returned validation error depend on
map iteration order. Update validatePlacementPolicy to collect and sort the
ByType keys before validating each entry so the error surfaced is deterministic
across reconciles. Keep the existing validatePlacementTemplate and
validPlacementTypeKeySyntax checks, but perform them in sorted key order so
evaluateValidatedGate sees a stable message for the same spec.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e9cd03d9-8ca6-467f-8360-778caf19d70d
📒 Files selected for processing (13)
.coverage-baselineapi/v1alpha3/gittarget_types.goapi/v1alpha3/zz_generated.deepcopy.goconfig/crd/bases/configbutler.ai_gittargets.yamldocs/design/manifest/version2/gittarget-new-file-placement-rules.mdinternal/controller/gittarget_placement_validation.gointernal/controller/gittarget_placement_validation_test.gointernal/git/pending_writes.gointernal/git/pending_writes_test.gointernal/git/placement_test.gointernal/git/plan_flush.gointernal/manifestanalyzer/placement.gointernal/manifestanalyzer/placement_test.go
✅ Files skipped from review due to trivial changes (2)
- .coverage-baseline
- api/v1alpha3/zz_generated.deepcopy.go
🚧 Files skipped from review as they are similar to previous changes (3)
- internal/git/pending_writes.go
- api/v1alpha3/gittarget_types.go
- internal/git/plan_flush.go
sunib
changed the title
…surface skips
Three changes from the F4 review round, all on the new-file placement path.
1. New default placement path (ResourceIdentifier.ToGitPath). A brand-new
resource's canonical fallback is now namespace-first with no version segment:
{namespace}/{group}/{resource}/{name}.yaml (group omitted for core, literal
"cluster/" for cluster-scoped, .sops.yaml for sensitive), replacing the old
REST-first {group}/{version}/{resource}/{namespace}/{name}. It reads
namespace-first the way a human browses, and dropping the version segment
avoids churn on a preferred-version bump (the operator writes one version per
object). This is only the cold-start seed: sibling inference still follows an
existing layout, and existing files are match-first by identity, so the change
never moves a file already in Git.
2. Blocker: root GitTargets silently ignored declared placement on live events.
groupEventsByBase keys events by sanitizePath(event.Path), which collapses a
root target's "." to "", but placementPolicyForBase compared that against the
raw spec.path ("."), so the lookup returned nil and root targets fell back to
sibling/canonical placement — diverging from resync, which resolves
target.Placement directly. Fixed by sanitizing md.Path in the comparison.
3. Blocker: fail-safe placement skips were not observable. A resource the writer
refuses to place unsafely (placement unresolvable, or a co-mingling
sensitive/plaintext write) returned upsertNoChange, indistinguishable from a
genuine no-op, so resync stats never counted it. Added a distinct
upsertSkippedUnsafe outcome and a ResyncStats.PlacementSkipped counter, logged
per-resource and surfaced in the resync summary. It is deliberately not a
dedicated GitTarget status condition in v1; the design doc's claim is narrowed
to match, with an open question on adding a bounded status surface.
Docs updated to today's behavior: architecture.md (What it writes / File
Placement / boundaries), configuration.md (new spec.placement section), and the
F4 design doc (canonical layout, residual claim, implementation sketch). README
simplified (trimmed repeated Redis-required/managed-path restatements). All unit
tests and e2e file-path assertions updated to the new shape; commit-message
assertions (from resource identity, unchanged) left as-is.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/types/identifier.go (1)
58-63: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReuse
IsClusterScoped()instead of re-deriving scope inline.The scope check here duplicates the exact logic of
IsClusterScoped()(line 74-76). Consider calling it for consistency and to avoid drift if the "cluster-scoped" definition ever changes.♻️ Suggested refactor
func (r ResourceIdentifier) ToGitPath() string { - scope := r.Namespace - if scope == "" { + scope := r.Namespace + if r.IsClusterScoped() { // Cluster-scoped resource: the scope segment is the literal "cluster", // matching the {namespaceOrCluster} placement template variable. scope = "cluster" }🤖 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/types/identifier.go` around lines 58 - 63, The scope selection in the identifier formatting logic duplicates the cluster-scoped check already implemented in IsClusterScoped(), so replace the inline namespace-empty check in the identifier path with a call to that method. Update the code around the scope computation in the Identifier-related method to reuse IsClusterScoped() for consistency and to keep the cluster scope definition centralized if it changes later.
🤖 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 `@test/e2e/watchrule_configmap_secret_e2e_test.go`:
- Around line 511-521: The namespace guard in
assertLatestCommitTouchesNoNamespaces is still checking the old
e2e/configmap-test/v1/configmaps base path, while the generated files now use
e2e/configmap-test/<ns>/configmaps. Update the forbidden-path construction in
that helper to match the new namespace-based layout, using the same path pattern
as the nearby ConfigMap assertions in watchrule_configmap_secret_e2e_test.go so
the check targets the actual files.
---
Nitpick comments:
In `@internal/types/identifier.go`:
- Around line 58-63: The scope selection in the identifier formatting logic
duplicates the cluster-scoped check already implemented in IsClusterScoped(), so
replace the inline namespace-empty check in the identifier path with a call to
that method. Update the code around the scope computation in the
Identifier-related method to reuse IsClusterScoped() for consistency and to keep
the cluster scope definition centralized if it changes later.
🪄 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: ce5b8c8d-20fd-4a99-b9a3-b2bb79d29b55
📒 Files selected for processing (35)
README.mddocs/architecture.mddocs/configuration.mddocs/design/manifest/version2/gittarget-new-file-placement-rules.mdinternal/git/branch_worker_split_test.gointernal/git/branch_worker_test.gointernal/git/commit_executor_test.gointernal/git/commit_test.gointernal/git/git_operations_test.gointernal/git/git_test.gointernal/git/gittargetignore_writer_test.gointernal/git/pending_writes.gointernal/git/pending_writes_test.gointernal/git/plan_flush.gointernal/git/resync_flush.gointernal/git/resync_flush_test.gointernal/git/secret_write_test.gointernal/git/types.gointernal/manifestanalyzer/placement_test.gointernal/manifestanalyzer/plan_test.gointernal/manifestanalyzer/render_test.gointernal/types/identifier.gointernal/types/identifier_test.gotest/e2e/aggregated_apiserver_e2e_test.gotest/e2e/commit_author_attribution_e2e_test.gotest/e2e/commit_request_e2e_test.gotest/e2e/commit_window_batching_e2e_test.gotest/e2e/deletecollection_intent_e2e_test.gotest/e2e/deployment_scale_author_attribution_e2e_test.gotest/e2e/deployment_scale_subresource_e2e_test.gotest/e2e/f4_placement_e2e_test.gotest/e2e/gittarget_isolation_e2e_test.gotest/e2e/inplace_edit_e2e_test.gotest/e2e/signing_e2e_test.gotest/e2e/watchrule_configmap_secret_e2e_test.go
✅ Files skipped from review due to trivial changes (3)
- internal/manifestanalyzer/plan_test.go
- internal/manifestanalyzer/render_test.go
- README.md
🚧 Files skipped from review as they are similar to previous changes (7)
- internal/git/pending_writes.go
- internal/git/resync_flush.go
- test/e2e/f4_placement_e2e_test.go
- internal/git/resync_flush_test.go
- internal/manifestanalyzer/placement_test.go
- internal/git/plan_flush.go
- docs/design/manifest/version2/gittarget-new-file-placement-rules.md
…s, seeds) The first pass at updating e2e file-path assertions for the namespace-first default path missed several forms my initial grep did not cover, surfaced by the e2e legs on 931a412: - custom-group resources (IceCreamOrder): the iceCreamInstanceDir helper baked in the old "<group>/v1/icecreamorders" prefix; replaced with iceCreamInstancePath building the new "<namespace>/<group>/icecreamorders/<name>.yaml" and updated all call sites (crd-lifecycle, restart-reconcile, wildcard watchrule, bi-directional); - cluster-scoped CRDs now mirror under "cluster/apiextensions.k8s.io/ customresourcedefinitions/" (crd-lifecycle assertions + bi-directional seed); - multi-segment filepath.Join builders that split "v1"/"configmaps"/ns as separate args (commit-window seed+burst, quickstart-framework configmap+secret, bi-directional live secret, demo coffeeconfig); - assertLatestCommitTouchesNoNamespaces: its base path dropped the trailing "/v1/configmaps" since the namespace segment now leads directly under spec.path. Commit-message assertions (from ResourceIdentifier.String(), e.g. "[CREATE] v1/configmaps/name") are unchanged and deliberately left as-is. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…path segment
"cluster" is a legal DNS-1123 namespace name, so placing a cluster-scoped
resource under a top-level "cluster/" folder is ambiguous: a reader can't tell
it from a namespace, and a real namespace named "cluster" would land its
resources in the very same folder. "_cluster" is an illegal namespace name
(DNS-1123 forbids "_"), so the sentinel can never collide with a real namespace
and self-documents as "not a namespace".
Applied consistently to both places that emit the scope segment: the built-in
canonical ResourceIdentifier.ToGitPath and the {namespaceOrCluster} placement
template variable (so a declared template — including the recommended
identity-complete sensitive default — is collision-safe too). The {scope}
template variable stays the readable "cluster"/"namespaced" descriptor, since it
is a label, not a value that stands in the namespace position.
Updates the unit + e2e assertions (including cluster-scoped CRD mirror paths) and
the docs (design doc variable table/examples, architecture, configuration).
Match-first means existing cluster-scoped files are found by identity, so this
only affects newly-created files.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/e2e/icecream.go (1)
23-39: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winExtract a full-path helper for CRD mirror files to match
iceCreamInstancePath.
iceCreamInstancePathreturns a complete relative mirror path, buticeCreamCRDMirrorFileonly returns the filename. As a result, every caller that needs the CRD mirror path re-derives the"_cluster/apiextensions.k8s.io/customresourcedefinitions/"prefix by hand (seen 3x incrd_lifecycle_e2e_test.goand once, split across path segments, inbi_directional_e2e_test.go). This duplication is exactly the kind of divergence risk this PR is already fixing by centralizingiceCreamInstancePath— a typo or future path-convention change (as already happened once,cluster→_cluster) would need to be applied in 4 places instead of one.♻️ Proposed helper to eliminate duplication
func iceCreamCRDMirrorFile(group string) string { return iceCreamCRDName(group) + ".yaml" } + +// iceCreamCRDMirrorPath returns the full relative mirror path for one +// IceCreamOrder CRD, including the cluster-scoped +// "_cluster/apiextensions.k8s.io/customresourcedefinitions/" prefix. +func iceCreamCRDMirrorPath(group string) string { + return "_cluster/apiextensions.k8s.io/customresourcedefinitions/" + iceCreamCRDMirrorFile(group) +}Callers in
crd_lifecycle_e2e_test.go(lines 140, 632, 644, 665) andbi_directional_e2e_test.go(lines 419-430) could then useiceCreamCRDMirrorPath(group)instead of hand-rolling the prefix.🤖 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 `@test/e2e/icecream.go` around lines 23 - 39, Add a full-path helper for CRD mirror files alongside iceCreamCRDName and iceCreamInstancePath, so callers do not manually rebuild the _cluster/apiextensions.k8s.io/customresourcedefinitions prefix. Update iceCreamCRDMirrorFile in test/e2e/icecream.go to expose the complete mirror path (or add a new iceCreamCRDMirrorPath helper) and then replace the repeated hardcoded path construction in crd_lifecycle_e2e_test.go and bi_directional_e2e_test.go with that shared helper.
🤖 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.
Outside diff comments:
In `@test/e2e/icecream.go`:
- Around line 23-39: Add a full-path helper for CRD mirror files alongside
iceCreamCRDName and iceCreamInstancePath, so callers do not manually rebuild the
_cluster/apiextensions.k8s.io/customresourcedefinitions prefix. Update
iceCreamCRDMirrorFile in test/e2e/icecream.go to expose the complete mirror path
(or add a new iceCreamCRDMirrorPath helper) and then replace the repeated
hardcoded path construction in crd_lifecycle_e2e_test.go and
bi_directional_e2e_test.go with that shared helper.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 66628ab0-0c1b-4c96-af49-3cc87a6ad54c
📒 Files selected for processing (17)
docs/architecture.mddocs/configuration.mddocs/design/manifest/version2/gittarget-new-file-placement-rules.mdinternal/git/commit_test.gointernal/git/git_test.gointernal/manifestanalyzer/placement.gointernal/manifestanalyzer/placement_test.gointernal/types/identifier.gointernal/types/identifier_test.gotest/e2e/bi_directional_e2e_test.gotest/e2e/commit_window_batching_e2e_test.gotest/e2e/crd_lifecycle_e2e_test.gotest/e2e/demo_e2e_test.gotest/e2e/icecream.gotest/e2e/quickstart_framework_e2e_test.gotest/e2e/restart_reconcile_e2e_test.gotest/e2e/watchrule_configmap_secret_e2e_test.go
✅ Files skipped from review due to trivial changes (3)
- test/e2e/demo_e2e_test.go
- internal/git/commit_test.go
- internal/git/git_test.go
🚧 Files skipped from review as they are similar to previous changes (9)
- internal/types/identifier_test.go
- docs/configuration.md
- test/e2e/commit_window_batching_e2e_test.go
- docs/architecture.md
- internal/types/identifier.go
- test/e2e/watchrule_configmap_secret_e2e_test.go
- internal/manifestanalyzer/placement_test.go
- internal/manifestanalyzer/placement.go
- docs/design/manifest/version2/gittarget-new-file-placement-rules.md
The spec.placement section read as "magic". Expand it with:
- an explicit resolution ladder (byType → default → sibling inference → canonical
fallback) so the order of precedence is clear;
- a worked sibling-inference example (a brownfield repo with a ConfigMap bundle
and one-per-file Secrets) showing the two observed decisions — which directory,
and one-file-vs-bundle — plus the boundaries (sensitive never joins plaintext,
deterministic tie-break, can only continue an existing layout);
- a full template-variable table with example values for all eleven variables,
and a callout on the {namespace} vs {namespaceOrCluster} gotcha (namespace is
empty for cluster-scoped resources, so its segment vanishes — use
namespaceOrCluster to keep the _cluster/ segment);
- links to the design doc (full ladder/risks) and the file-agnostic-placement
vision doc.
Docs-only change.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ath tradeoff
Review feedback on F4 placement:
- Medium: validPlacementTypeKeySyntax now rejects byType keys with surrounding
whitespace (p != strings.TrimSpace(p)). A padded key like "apps/v1/deployments "
cleared the old trim-then-check gate but never matched the resolver's exact GVR
key, so placement silently fell through instead of erroring. Add regression cases.
- High, resolved as a documented tradeoff (not a code change): the versionless
cold-start path means objects differing only by API version share a file.
Document that multi-version watches wanting separate files must include {version}
in a placement template — in the user-facing WatchRule.apiVersions CRD
description and the placement godoc. byType keys stay versioned and exact.
- Low: correct three stale comments still describing the old
{group}/{version}/{resource}/{namespace}/{name} layout.
Regenerated the watchrules CRD.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this adds
When GitOps Reverser writes a resource to Git that isn't there yet, it used to
always drop it at one fixed path. This PR makes it place new resources to fit
how your repository is already organized.
spec.placementfield lets youdeclare where new files of a given type should go, using simple path
templates. Sensitive resources (Secrets) always get their own file, never
shared.
what its neighbours already do (one file per resource, or a shared bundle) in
the same folder, instead of imposing a fixed structure.
kustomization.yaml, itsresources:list is updated in the same commit,preserving comments and order.
path (now namespace-first and versionless).
New files are never silently appended to a file whose contents the writer can't
fully account for.
Testing
fmt/generate/manifests/vet/lint/testall green (unit +controller integration suite). Added unit, integration, and e2e coverage for
placement policies, layout inference, kustomize updates, and the new path
layout.
🤖 Generated with Claude Code
Summary by CodeRabbit
GitTarget.spec.placement(byType+default) to control where brand-new resources are written, including safe behavior for sensitive resources and kustomize overlay placement with idempotentkustomization.yamlresourcesupdates._cluster-based convention.