feat: flexible manifests#164
Conversation
…e document can count on pushback in the next commit).
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds manifest indexing, a decide/apply in-document YAML edit engine, render/report integration, writer placement and in-place edits, extensive unit and E2E tests, and CI/Taskfile/docs/tooling updates. ChangesManifest edit, report, and writer flow
Sequence Diagram(s)sequenceDiagram
participant Client as BuildReport / Apply caller
participant Indexer as manifestedit.IndexFiles
participant Decider as manifestedit.Decide
participant Applier as manifestedit.Apply
participant Worktree as Git worktree
Client->>Indexer: index Git files (Inventory)
Client->>Decider: build Comparison for desired object
Decider->>Client: Decision (no-change/patch/replace/delete/skip)
alt apply path (EditInPlace)
Client->>Applier: Apply Decision (EditInPlace)
Applier->>Worktree: return updated file content or skip
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces the manifestedit package and the manifestreport integration layer to enable file-agnostic placement and in-place manifest editing, allowing the operator to preserve hand-authored comments and formatting during updates. The changes are well-supported by new unit and E2E tests. Feedback on the implementation highlights several opportunities to add defensive nil checks to prevent potential nil pointer dereferences in git.go, render.go, and report.go. Additionally, some temporary scratchpad URLs left at the end of docs/TODO.md should be cleaned up.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| func preserveExistingFormatting( | ||
| writer eventContentWriter, | ||
| event Event, | ||
| filePath string, | ||
| existingContent []byte, | ||
| ) ([]byte, bool) { | ||
| if writer.isSensitiveIdentifier(event.Identifier) { | ||
| return nil, false | ||
| } |
There was a problem hiding this comment.
Add a defensive nil check for event.Object to prevent potential nil pointer dereference panics if the event payload is empty or incomplete.
func preserveExistingFormatting(
writer eventContentWriter,
event Event,
filePath string,
existingContent []byte,
) ([]byte, bool) {
if event.Object == nil {
return nil, false
}
if writer.isSensitiveIdentifier(event.Identifier) {
return nil, false
}| func EditInPlace(path string, existing []byte, obj *unstructured.Unstructured) ([]byte, bool) { | ||
| inv, _ := manifestedit.IndexFile(path, existing) | ||
| loc, found := inv.Location(identityOf(obj)) |
There was a problem hiding this comment.
Add a defensive nil check for obj to prevent a nil pointer dereference panic when calling identityOf(obj).
| func EditInPlace(path string, existing []byte, obj *unstructured.Unstructured) ([]byte, bool) { | |
| inv, _ := manifestedit.IndexFile(path, existing) | |
| loc, found := inv.Location(identityOf(obj)) | |
| func EditInPlace(path string, existing []byte, obj *unstructured.Unstructured) ([]byte, bool) { | |
| if obj == nil { | |
| return nil, false | |
| } | |
| inv, _ := manifestedit.IndexFile(path, existing) | |
| loc, found := inv.Location(identityOf(obj)) |
| for _, obj := range desired { | ||
| id := identityOf(obj) | ||
| desiredSeen[id] = true |
There was a problem hiding this comment.
Add a defensive nil check for obj in the loop to prevent a nil pointer dereference panic when calling identityOf(obj).
for _, obj := range desired {
if obj == nil {
continue
}
id := identityOf(obj)
desiredSeen[id] = true| Replace with https://docs.victoriametrics.com/helm/victoria-metrics-operator/ (so that it's also helm and so that we can have proper deps) | ||
| https://kubernetes.io/docs/reference/using-api/api-concepts/#resource-versions | ||
| https://sessionize.com/kcpcon-2026/ No newline at end of file |
There was a problem hiding this comment.
These lines appear to be temporary scratchpad notes or raw URLs that were accidentally left in the file. They should be removed to keep the documentation clean.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/git/git.go (1)
604-623:⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy liftDon't delete the whole file for a single-resource prune.
With flexible manifest placement, one file can legitimately contain multiple manifests. This path always unlinks
fullPath, so deleting one resource will also delete any sibling documents that share the file. Route deletes through the manifestedit delete path and only remove the file when the target document is the last remaining manifest.🤖 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/git.go` around lines 604 - 623, handleDeleteOperation currently unconditionally unlinks fullPath and stages its deletion (os.Remove + worktree.Remove), which will remove sibling manifests stored in the same file; instead, route deletions through the manifest edit/delete pathway and only delete the underlying file when the removed document is the last manifest in that file. Modify handleDeleteOperation to call the manifest-edit delete routine (the existing manifest edit/delete handler used elsewhere in the codebase) for the single-resource remove, determine the number of manifests in fullPath (parse the file to count documents) and only call os.Remove and worktree.Remove when the count is 1 (last manifest); otherwise only update the file contents via the manifest edit path and stage that change in the repo (do not unlink fullPath).
🤖 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/future/file-agnostic-placement.md`:
- Around line 1-24: Fix the spelling/grammar in the intro and scope bullets by
correcting the listed misspellings and normalizing phrasing: change “Kuberntes”
→ “Kubernetes”, “doesnt” → “doesn't”, “workin” → “working”, “seperator” →
“separator”, “entirly” → “entirely”, “everyhing” → “everything”, “insigt” →
“insight”, “Boundries” → “Boundaries”, and “respnsiblity” → “responsibility”;
also review surrounding sentences for punctuation/capitalization (e.g., “ok, if
configured well ofcourse” → “OK, if configured well, of course”) and apply
consistent grammar across the intro and bullet list so the document reads
clearly and professionally.
In `@docs/future/manifestedit-integration-readonly-reconcile.md`:
- Line 3: The status line currently reads "Status: in progress (the read-only
report is implemented; writer wiring is deferred)" which conflicts with later
notes that writer wiring has landed; update that status line text to reflect
that writer wiring is implemented/landed (for example "Status: in progress
(read-only report implemented; writer wiring landed)") and make the same
correction in the other duplicate status block later in the document that
describes writer wiring so both places consistently state that writer wiring is
landed rather than deferred.
In `@docs/TODO.md`:
- Around line 96-98: The dangling reference notes containing the three URLs
(https://docs.victoriametrics.com/helm/victoria-metrics-operator/,
https://kubernetes.io/docs/reference/using-api/api-concepts/#resource-versions,
and https://sessionize.com/kcpcon-2026/) should not remain as loose lines in
TODO.md; either delete them or move them into a structured section (e.g., an
"External Links" or "Backlog" bullet list) with a one-line context for each link
explaining why it’s listed (e.g., "helm chart for victoria-metrics operator",
"k8s API resource versions reference", "kcpcon 2026 session link") so the notes
are purposeful and discoverable.
In `@docs/wildcard-ci-failure-findings.md`:
- Around line 31-35: Several fenced code blocks are missing language identifiers
and violate markdownlint rule MD040; update each triple-backtick fence
containing the samples like the "Timed out after 30s." block and the "docker
builder prune -f" block (and the other similar blocks) to include an explicit
language tag (e.g., ```text or ```bash). For each occurrence mentioned in the
comment, open the fence that begins before the shown snippet and change ``` to
```text for plain output snippets and ```bash for shell commands so all code
fences comply with MD040.
In `@internal/git/git.go`:
- Around line 712-726: Currently preserveExistingFormatting canonicalizes the
whole file first which treats multi-document files as unparseable and causes a
wholesale overwrite; instead, attempt an in-place edit first by calling
manifestreport.EditInPlace(filePath, existingContent, event.Object) and if it
returns a non-nil []byte, return that result; only if EditInPlace returns
nil/false should you call canonicalizeManifestForComparison(existingContent),
and if that canonicalization returns an error then do NOT permit a full-file
overwrite (return nil,false) — only allow the fallback wholesale write if
canonicalization succeeds and proves the file contains exactly the single
canonical document for this event; keep the existing sensitive check via
writer.isSensitiveIdentifier(event.Identifier).
In `@internal/git/manifestedit/framing.go`:
- Around line 53-61: The hasDocEndMarker function currently only recognizes a
bare "..." line; update it to accept lines where "..." is followed by optional
whitespace and/or an inline comment (e.g., "... # keep"). In function
hasDocEndMarker, after trimming right-side whitespace, replace the equality
check (line == "...") with a check that the line starts with "..." and that the
character after the three dots is either end-of-string or one of allowed
separators (space, tab or '#'); return true in that case so inline-commented end
markers are preserved.
In `@internal/git/manifestedit/manifestedit_test.go`:
- Around line 522-526: The test is indexing the wrong filename
("bomb.sops.yaml") so inv.Records may be empty; change the call to IndexFile to
use the actual aliased manifest path used by the fixture (e.g. "bomb.yaml") so
the alias-bomb case is exercised, then keep the loop over inv.Records asserting
r.Editable is false (and optionally assert r.Path == "bomb.yaml" or the expected
path) to ensure the intended record is present and non-editable; update the
string in the IndexFile call and (if added) the path assertion against
inv.Records entries.
In `@internal/git/manifestedit/merge.go`:
- Around line 109-125: mergeValue currently treats replaceNode failures as
success by always returning ok=true; change each branch that calls replaceNode
(the map, sequence, and default branches) to capture the boolean result from
replaceNode and propagate failure by returning (false, false) when replaceNode
returns false, otherwise return (result, true); update the calls in mergeValue
so a failed unencodable replacement bubbles up to applyPatch as a merge failure
instead of being treated as successful.
In `@internal/git/manifestedit/patch.go`:
- Around line 36-44: PatchDocument currently treats a nil desired as a delete
because Comparison.Desired == nil signals deletion; at the top of PatchDocument
validate that the desired argument is not nil and fail fast if it is (do not
call NewDocument/Comparison/Decide/Apply). Return an error-style response by
producing an appropriate EditResult (empty/zero) and a Diagnostic marking the
call as invalid (severity error) with a clear message like "desired must not be
nil" so accidental destructive calls are prevented.
In `@test/e2e/inplace_edit_e2e_test.go`:
- Around line 207-210: Check that secret.Data contains non-empty entries for
"username" and "password" before calling base64.StdEncoding.DecodeString:
validate secret.Data["username"] and secret.Data["password"] are present and not
empty, and use Expect to fail early with clear messages like "missing Git
username secret" / "missing Git password secret" if absent; only then call
base64.StdEncoding.DecodeString and keep the existing
Expect(err).NotTo(HaveOccurred(), ...) checks for decoding errors.
- Around line 160-173: Do not embed plaintext credentials into the remote URL or
expose raw git output on failure: remove the parsed.User = url.UserPassword(...)
and the call that sets the credentialized URL via
mustGit("remote","set-url",...), and instead perform authenticated operations by
invoking gitRun/fetch with a controlled auth environment (e.g. set GIT_ASKPASS
to a small script that prints the password, set GIT_USERNAME or use credential
helper, and set GIT_TERMINAL_PROMPT=0) so credentials are supplied at runtime
but never written into the remote; also change mustGit so the Expect on git
errors does not interpolate raw git output (replace the detailed out string with
a generic failure message) to avoid leaking secrets in logs.
---
Outside diff comments:
In `@internal/git/git.go`:
- Around line 604-623: handleDeleteOperation currently unconditionally unlinks
fullPath and stages its deletion (os.Remove + worktree.Remove), which will
remove sibling manifests stored in the same file; instead, route deletions
through the manifest edit/delete pathway and only delete the underlying file
when the removed document is the last manifest in that file. Modify
handleDeleteOperation to call the manifest-edit delete routine (the existing
manifest edit/delete handler used elsewhere in the codebase) for the
single-resource remove, determine the number of manifests in fullPath (parse the
file to count documents) and only call os.Remove and worktree.Remove when the
count is 1 (last manifest); otherwise only update the file contents via the
manifest edit path and stage that change in the repo (do not unlink fullPath).
🪄 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: d8db06e3-c19e-4d5e-93bf-231f0f8b4f81
📒 Files selected for processing (39)
docs/TODO.mddocs/facts/resource-versions.mddocs/future/file-agnostic-placement.mddocs/future/ha-gittarget-distribution-plan.mddocs/future/manifest-inventory-file-agnostic-placement.mddocs/future/manifest-parser-poc.mddocs/future/manifestedit-abstraction-plan.mddocs/future/manifestedit-field-ownership-spike.mddocs/future/manifestedit-integration-readonly-reconcile.mddocs/wildcard-ci-failure-findings.mdinternal/git/git.gointernal/git/inplace_edit_test.gointernal/git/manifestedit/DECISION.mdinternal/git/manifestedit/additions_test.gointernal/git/manifestedit/comments_chomp_test.gointernal/git/manifestedit/convergence_test.gointernal/git/manifestedit/decision.gointernal/git/manifestedit/decision_test.gointernal/git/manifestedit/delete.gointernal/git/manifestedit/edge_test.gointernal/git/manifestedit/framing.gointernal/git/manifestedit/index.gointernal/git/manifestedit/keyedlist_test.gointernal/git/manifestedit/limitations_test.gointernal/git/manifestedit/manifestedit_test.gointernal/git/manifestedit/merge.gointernal/git/manifestedit/patch.gointernal/git/manifestedit/scan.gointernal/git/manifestedit/split.gointernal/git/manifestedit/testdata/corpus/configmap-script.yamlinternal/git/manifestedit/testdata/corpus/deployment.yamlinternal/git/manifestedit/testdata/corpus/multidoc.yamlinternal/git/manifestedit/types.gointernal/manifestreport/editinplace_test.gointernal/manifestreport/render.gointernal/manifestreport/render_contract_test.gointernal/manifestreport/report.gointernal/manifestreport/report_test.gotest/e2e/inplace_edit_e2e_test.go
| The whole cool thing of KRM and the Kubernetes API is that the exact content of the YAML file contains everything needed to reflect the actual resource inside the Kuberntes API. The actual location of the file doesnt matter. | ||
|
|
||
| At this moment gitops-reverser has a fixed convention on where it expects/places files. Obviously the placement of new files is something that always needs some rule (either hardcoded or fixed). But why would it be important to start workin? For normal GitOps application like ArgoCD or Flux it's also not important (ok, if configured well offcourse). | ||
|
|
||
| There is some interesting problems to this: | ||
| * It's normal/allowed in YAML files to place multiple resources in one file, the `/n---/n` trick is used as seperator. | ||
| * For some resources it would be very beneficial to NOT write the namespace, actually all my examples until now have the problem that it's not entirly logical to also place the namespace. It might be a nice option on a GitTarget to drop it. | ||
| * Most examples also would have benefitted from having a simple folder based set of 'bootstrap' files. I already do this for the .sops.yaml file, but why not for a simple kustomization.yaml? Which allows to easily hook it up to normal GitOps tools and deploy it into a different namespace? | ||
| * It would be very logical for people to hook up a cloud version of this to an existing Gitrepo: I will never be able to support everyhing but to be so strict in where files are to placed is madness, then it will certainly not work. | ||
|
|
||
| Requirements: | ||
| * Parse an existing folder and parse all yaml/KRM that can be found in it. | ||
| * Have a notion / index of this (can be in memory) so that I can write back changes or updates at the right place. | ||
| * Recognize a kustomization / the current structure and be able to not write the namespace. | ||
| * Be able to (or even require GitOps tooling!) to stream the initial set of yamls into some location so that we can start looking for changes. Not sure how to cope with this one, it's again a form if bidirectional GitOps. | ||
| * Detect helm shizzle and ignore it with a good error message. | ||
| * Detect Kustomize and only support the very basic constructs in both directions (what would that exactly look like). | ||
|
|
||
| The dream would to be able to point gitops-reverser (potenially combined with flux for example) at a folder and to be able to provide a GitOps API for it without even thinking. People would have insigt in which objects are detected, they can edit them and a pull request is automatically created out of it. | ||
|
|
||
| Boundries: | ||
| * I don't believe that gitops-reverser should get knowledge on things like GitHub (so creating that PR is a respnsiblity for another layer, but pushing changes to a branch is fine). | ||
| * We really shouldnt get into the details of kustomize or helm to soon: we can also start with a clean folder of yaml manifests. | ||
|
|
There was a problem hiding this comment.
Clean up spelling/grammar in the intro and scope bullets.
There are multiple visible typos (e.g., “Kuberntes”, “doesnt”, “workin”, “seperator”, “entirly”, “everyhing”, “insigt”, “Boundries”, “respnsiblity”). Please normalize this section before merge so the design doc is easier to consume.
🧰 Tools
🪛 LanguageTool
[grammar] ~1-~1: Ensure spelling is correct
Context: ... reflect the actual resource inside the Kuberntes API. The actual location of the file do...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[style] ~3-~3: Consider an alternative for the often overused word ‘important’.
Context: ...ation like ArgoCD or Flux it's also not important (ok, if configured well offcourse). Th...
(NOT_IMPORTANT)
[grammar] ~7-~7: Ensure spelling is correct
Context: ...ntil now have the problem that it's not entirly logical to also place the namespace. It...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~8-~8: Use a hyphen to join words.
Context: ...e benefitted from having a simple folder based set of 'bootstrap' files. I alread...
(QB_NEW_EN_HYPHEN)
[grammar] ~9-~9: Ensure spelling is correct
Context: ...itrepo: I will never be able to support everyhing but to be so strict in where files are ...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~19-~19: Ensure spelling is correct
Context: ...ithout even thinking. People would have insigt in which objects are detected, they can ed...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~20-~20: Ensure spelling is correct
Context: ...st is automatically created out of it. Boundries: * I don't believe that gitops-reverser ...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~22-~22: Ensure spelling is correct
Context: ...s like GitHub (so creating that PR is a respnsiblity for another layer, but pushing changes ...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🤖 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/future/file-agnostic-placement.md` around lines 1 - 24, Fix the
spelling/grammar in the intro and scope bullets by correcting the listed
misspellings and normalizing phrasing: change “Kuberntes” → “Kubernetes”,
“doesnt” → “doesn't”, “workin” → “working”, “seperator” → “separator”, “entirly”
→ “entirely”, “everyhing” → “everything”, “insigt” → “insight”, “Boundries” →
“Boundaries”, and “respnsiblity” → “responsibility”; also review surrounding
sentences for punctuation/capitalization (e.g., “ok, if configured well
ofcourse” → “OK, if configured well, of course”) and apply consistent grammar
across the intro and bullet list so the document reads clearly and
professionally.
| @@ -0,0 +1,132 @@ | |||
| # Step 6: the read-only, inventory-driven reconcile | |||
|
|
|||
| > Status: in progress (the read-only report is implemented; writer wiring is deferred) | |||
There was a problem hiding this comment.
Align status line with current rollout state.
Line 3 says writer wiring is deferred, but later you document that writer wiring is already landed (narrowly). Please update the status line so readers don’t get conflicting guidance.
Also applies to: 104-115
🤖 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/future/manifestedit-integration-readonly-reconcile.md` at line 3, The
status line currently reads "Status: in progress (the read-only report is
implemented; writer wiring is deferred)" which conflicts with later notes that
writer wiring has landed; update that status line text to reflect that writer
wiring is implemented/landed (for example "Status: in progress (read-only report
implemented; writer wiring landed)") and make the same correction in the other
duplicate status block later in the document that describes writer wiring so
both places consistently state that writer wiring is landed rather than
deferred.
| ``` | ||
| Timed out after 30s. | ||
| target A's commit for iso-cm-N must be a [CREATE] event commit | ||
| Expected <string>: to contain substring <string>: [CREATE] | ||
| ``` |
There was a problem hiding this comment.
Add language identifiers to fenced code blocks to satisfy markdownlint.
These fences are missing a language tag (MD040). Please annotate each block (e.g., text, bash) to keep docs lint-clean.
Example fix pattern
-```
+```text
Timed out after 30s.
...
-```
+```-```
+```bash
docker builder prune -f
docker image prune -f
-```
+```Also applies to: 40-42, 51-57, 113-117, 143-148, 198-204, 208-210, 216-220, 228-231
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 31-31: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 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/wildcard-ci-failure-findings.md` around lines 31 - 35, Several fenced
code blocks are missing language identifiers and violate markdownlint rule
MD040; update each triple-backtick fence containing the samples like the "Timed
out after 30s." block and the "docker builder prune -f" block (and the other
similar blocks) to include an explicit language tag (e.g., ```text or ```bash).
For each occurrence mentioned in the comment, open the fence that begins before
the shown snippet and change ``` to ```text for plain output snippets and
```bash for shell commands so all code fences comply with MD040.
| func PatchDocument( | ||
| content []byte, | ||
| documentIndex int, | ||
| desired *unstructured.Unstructured, | ||
| opts EditOptions, | ||
| ) (EditResult, []Diagnostic) { | ||
| git, _ := NewDocument(content, documentIndex) | ||
| c := Comparison{Git: git, Desired: desired, Options: opts} | ||
| return Apply(c, Decide(c)) |
There was a problem hiding this comment.
Reject nil desired objects in PatchDocument.
Comparison.Desired == nil means delete, so PatchDocument(..., nil, ...) will remove the document instead of failing fast. Since deletion already has a dedicated API, this wrapper should guard against accidental destructive calls.
Suggested fix
func PatchDocument(
content []byte,
documentIndex int,
desired *unstructured.Unstructured,
opts EditOptions,
) (EditResult, []Diagnostic) {
+ if desired == nil {
+ return EditResult{Content: content, Mode: EditSkipped}, []Diagnostic{{
+ Level: DiagError,
+ Message: "PatchDocument requires a desired object; use DeleteDocument for deletions",
+ DocumentIndex: documentIndex,
+ }}
+ }
git, _ := NewDocument(content, documentIndex)
c := Comparison{Git: git, Desired: desired, Options: opts}
return Apply(c, Decide(c))
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func PatchDocument( | |
| content []byte, | |
| documentIndex int, | |
| desired *unstructured.Unstructured, | |
| opts EditOptions, | |
| ) (EditResult, []Diagnostic) { | |
| git, _ := NewDocument(content, documentIndex) | |
| c := Comparison{Git: git, Desired: desired, Options: opts} | |
| return Apply(c, Decide(c)) | |
| func PatchDocument( | |
| content []byte, | |
| documentIndex int, | |
| desired *unstructured.Unstructured, | |
| opts EditOptions, | |
| ) (EditResult, []Diagnostic) { | |
| if desired == nil { | |
| return EditResult{Content: content, Mode: EditSkipped}, []Diagnostic{{ | |
| Level: DiagError, | |
| Message: "PatchDocument requires a desired object; use DeleteDocument for deletions", | |
| DocumentIndex: documentIndex, | |
| }} | |
| } | |
| git, _ := NewDocument(content, documentIndex) | |
| c := Comparison{Git: git, Desired: desired, Options: opts} | |
| return Apply(c, Decide(c)) | |
| } |
🤖 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/manifestedit/patch.go` around lines 36 - 44, PatchDocument
currently treats a nil desired as a delete because Comparison.Desired == nil
signals deletion; at the top of PatchDocument validate that the desired argument
is not nil and fail fast if it is (do not call
NewDocument/Comparison/Decide/Apply). Return an error-style response by
producing an appropriate EditResult (empty/zero) and a Diagnostic marking the
call as invalid (severity error) with a clear message like "desired must not be
nil" so accidental destructive calls are prevented.
| username, password := inplaceReadGitCredentials(namespace, repo.GitSecretHTTP) | ||
| originOut, err := gitRun(repo.CheckoutDir, "remote", "get-url", "origin") | ||
| Expect(err).NotTo(HaveOccurred(), "failed to read origin URL") | ||
| parsed, err := url.Parse(strings.TrimSpace(originOut)) | ||
| Expect(err).NotTo(HaveOccurred(), "failed to parse origin URL") | ||
| parsed.User = url.UserPassword(username, password) | ||
|
|
||
| mustGit := func(args ...string) { | ||
| out, gitErr := gitRun(repo.CheckoutDir, args...) | ||
| Expect(gitErr).NotTo(HaveOccurred(), fmt.Sprintf("git %s: %s", strings.Join(args, " "), out)) | ||
| } | ||
|
|
||
| mustGit("remote", "set-url", "origin", parsed.String()) | ||
| mustGit("fetch", "origin", "main") |
There was a problem hiding this comment.
Avoid leaking Git credentials through remote URL + error output.
This helper writes username:password into origin and then includes raw git output on failure; a fetch/push error can expose credentials in CI logs.
🔒 Suggested hardening
func seedCommentIntoRepoFile(repo *RepoArtifacts, namespace, relPath, comment string) {
GinkgoHelper()
username, password := inplaceReadGitCredentials(namespace, repo.GitSecretHTTP)
originOut, err := gitRun(repo.CheckoutDir, "remote", "get-url", "origin")
Expect(err).NotTo(HaveOccurred(), "failed to read origin URL")
- parsed, err := url.Parse(strings.TrimSpace(originOut))
+ originURL := strings.TrimSpace(originOut)
+ parsed, err := url.Parse(originURL)
Expect(err).NotTo(HaveOccurred(), "failed to parse origin URL")
parsed.User = url.UserPassword(username, password)
+ defer func() {
+ _, _ = gitRun(repo.CheckoutDir, "remote", "set-url", "origin", originURL)
+ }()
mustGit := func(args ...string) {
- out, gitErr := gitRun(repo.CheckoutDir, args...)
- Expect(gitErr).NotTo(HaveOccurred(), fmt.Sprintf("git %s: %s", strings.Join(args, " "), out))
+ _, gitErr := gitRun(repo.CheckoutDir, args...)
+ Expect(gitErr).NotTo(HaveOccurred(), fmt.Sprintf("git %s failed", strings.Join(args, " ")))
}🤖 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/inplace_edit_e2e_test.go` around lines 160 - 173, Do not embed
plaintext credentials into the remote URL or expose raw git output on failure:
remove the parsed.User = url.UserPassword(...) and the call that sets the
credentialized URL via mustGit("remote","set-url",...), and instead perform
authenticated operations by invoking gitRun/fetch with a controlled auth
environment (e.g. set GIT_ASKPASS to a small script that prints the password,
set GIT_USERNAME or use credential helper, and set GIT_TERMINAL_PROMPT=0) so
credentials are supplied at runtime but never written into the remote; also
change mustGit so the Expect on git errors does not interpolate raw git output
(replace the detailed out string with a generic failure message) to avoid
leaking secrets in logs.
| username, err := base64.StdEncoding.DecodeString(secret.Data["username"]) | ||
| Expect(err).NotTo(HaveOccurred(), "failed to decode Git username") | ||
| password, err := base64.StdEncoding.DecodeString(secret.Data["password"]) | ||
| Expect(err).NotTo(HaveOccurred(), "failed to decode Git password") |
There was a problem hiding this comment.
Validate required secret keys before decoding.
If data.username/data.password is missing, base64 decode of "" succeeds and shifts failure downstream with a less clear error.
✅ Suggested guard clauses
- username, err := base64.StdEncoding.DecodeString(secret.Data["username"])
+ usernameB64, ok := secret.Data["username"]
+ Expect(ok).To(BeTrue(), "Git Secret missing data.username")
+ username, err := base64.StdEncoding.DecodeString(usernameB64)
Expect(err).NotTo(HaveOccurred(), "failed to decode Git username")
- password, err := base64.StdEncoding.DecodeString(secret.Data["password"])
+ passwordB64, ok := secret.Data["password"]
+ Expect(ok).To(BeTrue(), "Git Secret missing data.password")
+ password, err := base64.StdEncoding.DecodeString(passwordB64)
Expect(err).NotTo(HaveOccurred(), "failed to decode Git password")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| username, err := base64.StdEncoding.DecodeString(secret.Data["username"]) | |
| Expect(err).NotTo(HaveOccurred(), "failed to decode Git username") | |
| password, err := base64.StdEncoding.DecodeString(secret.Data["password"]) | |
| Expect(err).NotTo(HaveOccurred(), "failed to decode Git password") | |
| usernameB64, ok := secret.Data["username"] | |
| Expect(ok).To(BeTrue(), "Git Secret missing data.username") | |
| username, err := base64.StdEncoding.DecodeString(usernameB64) | |
| Expect(err).NotTo(HaveOccurred(), "failed to decode Git username") | |
| passwordB64, ok := secret.Data["password"] | |
| Expect(ok).To(BeTrue(), "Git Secret missing data.password") | |
| password, err := base64.StdEncoding.DecodeString(passwordB64) | |
| Expect(err).NotTo(HaveOccurred(), "failed to decode Git password") |
🤖 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/inplace_edit_e2e_test.go` around lines 207 - 210, Check that
secret.Data contains non-empty entries for "username" and "password" before
calling base64.StdEncoding.DecodeString: validate secret.Data["username"] and
secret.Data["password"] are present and not empty, and use Expect to fail early
with clear messages like "missing Git username secret" / "missing Git password
secret" if absent; only then call base64.StdEncoding.DecodeString and keep the
existing Expect(err).NotTo(HaveOccurred(), ...) checks for decoding errors.
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Overview
Files Reviewed (12 files)
Reviewed by laguna-m.1-20260312:free · 3,643,340 tokens |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
internal/git/git.go (1)
822-827:⚠️ Potential issue | 🟠 Major | ⚡ Quick winTry the in-place edit before the whole-file canonicalization gate.
This is still the same multi-doc overwrite hazard from the earlier review: whole-file canonicalization rejects shared manifest files,
preserveExistingFormatting()returnsfalse, and the caller falls back to rewriting the entire file with a single resource.Proposed fix
func preserveExistingFormatting( writer eventContentWriter, event Event, filePath string, existingContent []byte, ) ([]byte, bool) { if writer.isSensitiveIdentifier(event.Identifier) { return nil, false } + if edited, ok := manifestreport.EditInPlace(filePath, existingContent, event.Object); ok { + return edited, true + } canonical, err := canonicalizeManifestForComparison(existingContent) if err != nil || bytes.Equal(existingContent, canonical) { // Unparseable or already canonical: nothing hand-authored to preserve. return nil, false } - return manifestreport.EditInPlace(filePath, existingContent, event.Object) + return nil, false }🤖 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/git.go` around lines 822 - 827, The current logic canonicalizes the whole file before attempting an in-place edit, which causes multi-doc overwrite when shared manifest files are rejected; change the order so you first attempt manifestreport.EditInPlace(filePath, existingContent, event.Object) and if that returns a non-nil result/indicates the edit succeeded return that immediately, otherwise fall back to canonicalizeManifestForComparison(existingContent) and the existing bytes.Equal check. Update the code paths around canonicalizeManifestForComparison and the caller that relied on preserveExistingFormatting() so the in-place edit is attempted prior to the whole-file canonicalization gate.
🤖 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/e2e-aggregated-apiserver-test-design.md`:
- Around line 398-400: The doc contradicts itself about a "smoke" split:
reconcile/remove the lingering smoke guidance so selection is consistent—either
remove the "Labels: aggregated-api, smoke" reference in S2 or update it to match
the stated behavior that only "task test-e2e" runs the whole package and "task
test-e2e-aggregated-api" is the focused entry point; update mentions of "smoke"
(e.g., the S2 label line) to only list "aggregated-api" or add a clear note
explaining how "smoke" interacts with "task test-e2e" and "task
test-e2e-aggregated-api" to eliminate the conflicting guidance.
In `@internal/git/git.go`:
- Around line 619-622: manifestIdentity currently returns fallback when
event.Object is absent, which causes DELETE events (that only carry the
identifier) to always use the canonical path and miss moved manifests; change
the logic where id, ok := manifestIdentity(event.Object) handles !ok by
performing match-first resolution via locate(id) (calling locate with the
manifest identifier) and, if locate finds a real path, return that path instead
of fallback, otherwise keep returning fallback—update the code around
manifestIdentity and the locate call to ensure DELETEs resolve moved manifests
during reconcile.
- Around line 593-639: The cached inventories in manifestLocator.byBase (built
by inventoryFor using manifestedit.IndexDir) can become stale after
edits/deletes that shift document indexes in multi-doc files; update the code to
invalidate or refresh the cache for the affected base when you mutate a
multi-document file. Specifically, after any operation that edits or deletes a
document in a file (the code paths that apply writer.filePathForIdentifier
results or perform edits/deletes based on manifestTarget.documentIndex), clear
or refresh l.byBase[base] (or update its DocumentIndex entries) so subsequent
calls to inventoryFor(base) and locate(...) see the current manifestedit
locations and don't use stale documentIndex values. Ensure the invalidation
happens for the same base used to compute manifestTarget.filePath/documentIndex.
---
Duplicate comments:
In `@internal/git/git.go`:
- Around line 822-827: The current logic canonicalizes the whole file before
attempting an in-place edit, which causes multi-doc overwrite when shared
manifest files are rejected; change the order so you first attempt
manifestreport.EditInPlace(filePath, existingContent, event.Object) and if that
returns a non-nil result/indicates the edit succeeded return that immediately,
otherwise fall back to canonicalizeManifestForComparison(existingContent) and
the existing bytes.Equal check. Update the code paths around
canonicalizeManifestForComparison and the caller that relied on
preserveExistingFormatting() so the in-place edit is attempted prior to the
whole-file canonicalization gate.
🪄 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: 539d8cb7-c267-4a80-a3a0-79cab8dd6eca
📒 Files selected for processing (31)
.github/workflows/ci.ymlTiltfiledocs/ci/image-loading.mddocs/design/e2e-aggregated-apiserver-test-design.mddocs/design/e2e-full-suite-shared-state-investigation.mddocs/design/e2e-speedup-plan.mddocs/design/e2e-test-design.mddocs/finished/design-rule-change-snapshot-trigger.mddocs/finished/gittarget-isolation-on-rule-change.mddocs/future/manifestedit-new-file-placement-spike.mddocs/future/manifestedit-writer-followups.mddocs/wildcard-ci-failure-findings.mdinternal/git/commit_executor.gointernal/git/git.gointernal/git/inplace_edit_test.gointernal/git/known_placement_bugs_test.gointernal/manifestreport/noneditable_desired_bug_test.gointernal/manifestreport/report.gotest/e2e/E2E_DEBUGGING.mdtest/e2e/Taskfile.ymltest/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/controller_basics_e2e_test.gotest/e2e/crd_lifecycle_e2e_test.gotest/e2e/gitprovider_validation_e2e_test.gotest/e2e/gittarget_isolation_e2e_test.gotest/e2e/restart_snapshot_e2e_test.gotest/e2e/signing_e2e_test.gotest/e2e/watchrule_configmap_secret_e2e_test.go
💤 Files with no reviewable changes (1)
- Tiltfile
✅ Files skipped from review due to trivial changes (11)
- test/e2e/commit_author_attribution_e2e_test.go
- test/e2e/restart_snapshot_e2e_test.go
- docs/finished/gittarget-isolation-on-rule-change.md
- docs/design/e2e-speedup-plan.md
- docs/ci/image-loading.md
- test/e2e/gitprovider_validation_e2e_test.go
- test/e2e/watchrule_configmap_secret_e2e_test.go
- docs/finished/design-rule-change-snapshot-trigger.md
- test/e2e/controller_basics_e2e_test.go
- docs/design/e2e-full-suite-shared-state-investigation.md
- docs/wildcard-ci-failure-findings.md
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/git/inplace_edit_test.go
- internal/manifestreport/report.go
| There is no separate smoke label split; `task test-e2e` runs the whole package, | ||
| and `task test-e2e-aggregated-api` remains the focused entry point for this | ||
| category. |
There was a problem hiding this comment.
Remove the remaining smoke guidance or reconcile it here.
This section says there is no smoke-label split, but S2 below still documents Labels: aggregated-api, smoke. As written, the doc gives two different answers about how aggregated-api coverage is selected.
🤖 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/e2e-aggregated-apiserver-test-design.md` around lines 398 - 400,
The doc contradicts itself about a "smoke" split: reconcile/remove the lingering
smoke guidance so selection is consistent—either remove the "Labels:
aggregated-api, smoke" reference in S2 or update it to match the stated behavior
that only "task test-e2e" runs the whole package and "task
test-e2e-aggregated-api" is the focused entry point; update mentions of "smoke"
(e.g., the S2 label line) to only list "aggregated-api" or add a clear note
explaining how "smoke" interacts with "task test-e2e" and "task
test-e2e-aggregated-api" to eliminate the conflicting guidance.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
docs/design/manifest/gittarget-repository-validity-and-placement.md (3)
100-105: ⚡ Quick winReconsider error categorization for append-to-existing guards.
Lines 102-104 state that writes are "refused and surfaced as a repository validity problem" when the target file is "invalid YAML, non-editable, encrypted, or outside the discovery scope." These conditions represent different error classes:
- Invalid YAML or non-editable: genuine repository validity concerns
- Encrypted or outside discovery scope: placement policy conflicts, not repository corruption
Surfacing an encrypted file or out-of-scope file as a "repository validity problem" may confuse operators who interpret validity as meaning the repository content is broken. Consider distinguishing between repository validity failures (corrupt/unsafe content) and placement policy violations (cannot write to this location per GitTarget rules).
🤖 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/gittarget-repository-validity-and-placement.md` around lines 100 - 105, Update the wording in the "append-to-existing" guard behavior to distinguish error classes: keep "repository validity problem" for cases of invalid YAML and non-editable files (i.e., corrupt/unsafe repo content), but classify "encrypted" and "outside the discovery scope" as placement policy violations under GitTarget rules (i.e., write refused due to placement constraints). Edit the text that currently lumps all four conditions into "repository validity problem" so it explicitly maps invalid YAML/non-editable → repository validity, and encrypted/out-of-scope → placement policy violation (or similar terminology used elsewhere, e.g., "placement/permission error" or "GitTarget placement violation") to avoid operator confusion.
197-220: ⚡ Quick winClarify kubebuilder validation scope for
newFilePath.Step 1 (line 203) mentions adding "kubebuilder validation for relative path template output constraints where possible; runtime validation still owns rendered paths."
It would help implementers to explicitly state which constraints can be enforced at the schema level vs. runtime. For example:
- Schema level: non-empty string, basic Go template syntax validation (if tooling supports it)
- Runtime only: rendered path is relative, clean, stays under
spec.path, doesn't escape via..This prevents ambiguity about what "where possible" means and ensures the CRD validation webhook provides useful early feedback.
🤖 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/gittarget-repository-validity-and-placement.md` around lines 197 - 220, Update the doc to explicitly list which `newFilePath` constraints belong in the CRD schema vs runtime: state that kubebuilder validation should cover non-empty string and any static/template-syntax checks your tooling can perform (referencing GitTargetSpec.NewFilePath and template validation), while runtime validation in code (e.g., in the BranchWorker/manifest scanner that renders templates) must enforce that the rendered path is relative, cleaned (no `.`/`..` escape), stays under the target `spec.path`, and meets any OS/filesystem constraints; also mention that GitTargetDiscoverySpec.Recurse remains unrelated to path rendering and that webhook/schema validation cannot guarantee rendered-path containment so runtime checks are required.
140-145: 💤 Low valueConsider adding context for internal component references.
Line 143 mentions "deletes its
FolderReconciler" without explaining what a FolderReconciler is. For a design document that may be read by contributors unfamiliar with the internal architecture, a brief explanation or link would improve clarity.🤖 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/gittarget-repository-validity-and-placement.md` around lines 140 - 145, The sentence referencing "deletes its `FolderReconciler`" is unclear to readers unfamiliar with internal components: add a brief parenthetical definition or hyperlink immediately after the term explaining that FolderReconciler is the internal component responsible for watching and reconciling files/folders for that GitTarget (or link to its design/README), and similarly ensure `GitTarget` and "branch worker" have brief clarifying phrases on first use (e.g., GitTarget = configuration object representing a repo/branch to be processed; branch worker = the per-branch goroutine/worker that processes events) so the paragraph is self-contained and clear to external contributors.
🤖 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/manifest/gittarget-repository-validity-and-placement.md`:
- Around line 296-305: Decide and document whether all invalid YAML should set
the GitTarget RepositoryValid condition to false or only the subset that
resembles KRM and cannot be safely classified; update the design doc (affecting
the RepositoryValid condition reasons/messages and the step 3 "repository
validity gate" behavior referenced around the GitTarget implementation) to
record the chosen rule, the rationale, and concrete examples (parse-errors vs.
non-KRM-but-parsable) so the implementer can unambiguously implement and test
RepositoryValid handling and the related status reasons/messages.
- Around line 53-66: The documentation lacks the concrete default newFilePath
template and doesn't define how `.GroupPath` transforms dotted API groups; add
the default template string for `newFilePath` (evaluated relative to
`spec.path`) showing the canonical layout and clearly describe `.GroupPath`
transformation (replace dots with slashes, omit for core resources). Reference
`newFilePath`, `spec.path`, and the `.GroupPath`/`.Group` template variables in
the updated paragraph and update the variables table entry for `.GroupPath` to
state "dots replaced with slashes, omitted for core resources."
- Around line 146-174: Update the doc to clearly distinguish POC behavior from
production: state that manifestedit.Identity (as defined in
manifestedit/types.go) is the YAML/content identity used today, that
identityFromNode (in manifestedit/index.go) extracts
GVK+metadata.namespace+metadata.name and resolveDuplicates currently performs
first-occurrence-wins on that manifest identity, and then describe exactly how
API-side identity (GVR) mapping will be added in production (e.g., use
RESTMapper/GVK→GVR lookup), what constitutes “mapping unavailable” (e.g.,
discovery failure or missing CRD), and the failure mode (whether controller will
block or degrade to manifest-identity-only diagnostics) so readers can tell POC
behavior versus intended production behavior.
---
Nitpick comments:
In `@docs/design/manifest/gittarget-repository-validity-and-placement.md`:
- Around line 100-105: Update the wording in the "append-to-existing" guard
behavior to distinguish error classes: keep "repository validity problem" for
cases of invalid YAML and non-editable files (i.e., corrupt/unsafe repo
content), but classify "encrypted" and "outside the discovery scope" as
placement policy violations under GitTarget rules (i.e., write refused due to
placement constraints). Edit the text that currently lumps all four conditions
into "repository validity problem" so it explicitly maps invalid
YAML/non-editable → repository validity, and encrypted/out-of-scope → placement
policy violation (or similar terminology used elsewhere, e.g.,
"placement/permission error" or "GitTarget placement violation") to avoid
operator confusion.
- Around line 197-220: Update the doc to explicitly list which `newFilePath`
constraints belong in the CRD schema vs runtime: state that kubebuilder
validation should cover non-empty string and any static/template-syntax checks
your tooling can perform (referencing GitTargetSpec.NewFilePath and template
validation), while runtime validation in code (e.g., in the
BranchWorker/manifest scanner that renders templates) must enforce that the
rendered path is relative, cleaned (no `.`/`..` escape), stays under the target
`spec.path`, and meets any OS/filesystem constraints; also mention that
GitTargetDiscoverySpec.Recurse remains unrelated to path rendering and that
webhook/schema validation cannot guarantee rendered-path containment so runtime
checks are required.
- Around line 140-145: The sentence referencing "deletes its `FolderReconciler`"
is unclear to readers unfamiliar with internal components: add a brief
parenthetical definition or hyperlink immediately after the term explaining that
FolderReconciler is the internal component responsible for watching and
reconciling files/folders for that GitTarget (or link to its design/README), and
similarly ensure `GitTarget` and "branch worker" have brief clarifying phrases
on first use (e.g., GitTarget = configuration object representing a repo/branch
to be processed; branch worker = the per-branch goroutine/worker that processes
events) so the paragraph is self-contained and clear to external contributors.
🪄 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: 077d1851-dc43-4680-aae2-f66a2d250574
📒 Files selected for processing (12)
docs/TODO.mddocs/design/manifest/file-agnostic-placement.mddocs/design/manifest/gittarget-repository-validity-and-placement.mddocs/design/manifest/manifest-inventory-file-agnostic-placement.mddocs/design/manifest/manifest-parser-poc.mddocs/design/manifest/manifestedit-abstraction-plan.mddocs/design/manifest/manifestedit-field-ownership-spike.mddocs/design/manifest/manifestedit-integration-readonly-reconcile.mddocs/design/manifest/manifestedit-new-file-placement-spike.mddocs/design/manifest/manifestedit-writer-followups.mddocs/design/manifest/pr164-review-completion.mdinternal/git/manifestedit/DECISION.md
💤 Files with no reviewable changes (2)
- docs/design/manifest/file-agnostic-placement.md
- docs/design/manifest/manifestedit-abstraction-plan.md
✅ Files skipped from review due to trivial changes (9)
- docs/design/manifest/pr164-review-completion.md
- docs/design/manifest/manifestedit-new-file-placement-spike.md
- docs/design/manifest/manifestedit-writer-followups.md
- docs/design/manifest/manifestedit-integration-readonly-reconcile.md
- docs/TODO.md
- docs/design/manifest/manifestedit-field-ownership-spike.md
- docs/design/manifest/manifest-parser-poc.md
- internal/git/manifestedit/DECISION.md
- docs/design/manifest/manifest-inventory-file-agnostic-placement.md
| `newFilePath` replaces any enum-style choice for new-file placement. It is a | ||
| single template evaluated relative to `spec.path`. The default renders the | ||
| current canonical layout. Suggested template variables: | ||
|
|
||
| | Variable | Meaning | | ||
| |---|---| | ||
| | `.Group` | API group, empty for core resources | | ||
| | `.GroupPath` | API group as a path segment, omitted for core resources | | ||
| | `.Version` | API version | | ||
| | `.Resource` | plural resource name | | ||
| | `.Kind` | manifest kind when available | | ||
| | `.Namespace` | namespace, empty for cluster-scoped resources | | ||
| | `.Name` | object name | | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Clarify the default newFilePath template and GroupPath transformation.
The document references "the current canonical layout" as the default but doesn't show the actual template string. Additionally, the .GroupPath variable description doesn't specify how dots in API groups (e.g., apps.k8s.io) are transformed into path segments.
📝 Suggested additions
Add the default template after line 54:
single template evaluated relative to `spec.path`. The default template is:
{{ .GroupPath }}/{{ .Version }}/{{ .Resource }}/{{ .Namespace }}/{{ .Name }}.yaml
This renders the current canonical layout. Suggested template variables:Clarify the GroupPath transformation in the table at line 60:
| `.GroupPath` | API group as a path segment (dots replaced with slashes), omitted for core resources |🤖 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/gittarget-repository-validity-and-placement.md` around
lines 53 - 66, The documentation lacks the concrete default newFilePath template
and doesn't define how `.GroupPath` transforms dotted API groups; add the
default template string for `newFilePath` (evaluated relative to `spec.path`)
showing the canonical layout and clearly describe `.GroupPath` transformation
(replace dots with slashes, omit for core resources). Reference `newFilePath`,
`spec.path`, and the `.GroupPath`/`.Group` template variables in the updated
paragraph and update the variables table entry for `.GroupPath` to state "dots
replaced with slashes, omitted for core resources."
| ## Open questions | ||
|
|
||
| - Should invalid YAML block `RepositoryValid`, or only the subset that looks like | ||
| KRM and cannot be safely classified? | ||
| - How much duplicate detail belongs in GitTarget status before it becomes too | ||
| large? A bounded summary plus first N examples is likely enough. | ||
| - Should a branch push notification be added so external invalidating pushes are | ||
| observed faster than the normal reconcile/poll interval? | ||
| - Should `newFilePath` support conditionals, or only simple variable expansion | ||
| plus path cleanup? |
There was a problem hiding this comment.
Resolve the invalid YAML blocking question before implementation.
The first open question (lines 298-300) asks whether invalid YAML should block RepositoryValid or only "the subset that looks like KRM and cannot be safely classified." This is a critical design decision that will affect the implementation of the repository validity gate (step 3, lines 210-213).
Deferring this question risks:
- Rework if the chosen behavior doesn't match operator expectations
- Inconsistent error handling between YAML that fails to parse vs. YAML that parses but isn't valid KRM
- Unclear test coverage since the boundary of "invalid" isn't defined
Suggest making a decision and documenting the rationale before proceeding with implementation, especially since this affects the RepositoryValid condition reasons and messages proposed in lines 110-123.
🤖 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/gittarget-repository-validity-and-placement.md` around
lines 296 - 305, Decide and document whether all invalid YAML should set the
GitTarget RepositoryValid condition to false or only the subset that resembles
KRM and cannot be safely classified; update the design doc (affecting the
RepositoryValid condition reasons/messages and the step 3 "repository validity
gate" behavior referenced around the GitTarget implementation) to record the
chosen rule, the rationale, and concrete examples (parse-errors vs.
non-KRM-but-parsable) so the implementer can unambiguously implement and test
RepositoryValid handling and the related status reasons/messages.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/wildcard-ci-failure-findings.md`:
- Around line 258-263: The fenced code block lacks a language identifier
(violates MD040); update the opening fence for the shown block to use a language
tag (e.g., change ``` to ```text) so the block becomes ```text ... ``` while
leaving the content and closing fence unchanged; locate the block in
docs/wildcard-ci-failure-findings.md matching the snippet starting "aborting
cluster snapshot..." and add the language tag to the opening backticks.
In `@internal/watch/manager.go`:
- Around line 630-633: The current code in GetClusterStateForGitDest (via
listResourcesForGVR) treats any apierrors.IsNotFound(err) as "type no longer
served" and skips the entire gvr; change that to inspect the error's
metav1.Status().Details and only skip/continue when the Details indicate the
missing object corresponds to the requested GVR (e.g.,
Details.Kind/Group/Name/Resource matches the gvr), otherwise treat it as a
namespace-missing case and surface/abort the list failure as before; update the
logic around the IsNotFound(err) check in
listResourcesForGVR/GetClusterStateForGitDest (referencing gvr and err) and add
a regression test that simulates a namespaced List returning NotFound for the
namespace (missing namespaces) to ensure other namespaces' resources for the
same gvr are not dropped.
🪄 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: e58d0ca9-afdd-4b81-8c69-92f68a944999
📒 Files selected for processing (5)
docs/design/watchrule-wildcard-and-resolution-semantics.mddocs/future/watchrule-wildcard-support-plan.mddocs/wildcard-ci-failure-findings.mdinternal/watch/manager.gointernal/watch/manager_snapshot_test.go
✅ Files skipped from review due to trivial changes (1)
- docs/design/watchrule-wildcard-and-resolution-semantics.md
| ``` | ||
| aborting cluster snapshot for .../watchrule-wildcard-expansion-test-dest: | ||
| failed to list bi-directional.e2e.example.com/v1, Resource=icecreamorders: | ||
| the server could not find the requested resource | ||
| "snapshot replay did not complete, leaving target pending" | ||
| ``` |
There was a problem hiding this comment.
Add language identifier to fenced code block.
The code block is missing a language tag, violating markdownlint rule MD040. This is consistent with similar issues flagged in past reviews for this file.
📝 Proposed fix
-```
+```text
aborting cluster snapshot for .../watchrule-wildcard-expansion-test-dest:
failed to list bi-directional.e2e.example.com/v1, Resource=icecreamorders:
the server could not find the requested resource
"snapshot replay did not complete, leaving target pending"
-```
+```🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 258-258: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 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/wildcard-ci-failure-findings.md` around lines 258 - 263, The fenced code
block lacks a language identifier (violates MD040); update the opening fence for
the shown block to use a language tag (e.g., change ``` to ```text) so the block
becomes ```text ... ``` while leaving the content and closing fence unchanged;
locate the block in docs/wildcard-ci-failure-findings.md matching the snippet
starting "aborting cluster snapshot..." and add the language tag to the opening
backticks.
…nto poc/manifestedit # Conflicts: # docs/design/manifest/implementation-plan.md
…re (annoying to have these file edits every time).
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| // target document, never the whole file. | ||
| func (wb *writeBatch) patchExisting(ctx context.Context, event Event, ref manifestanalyzer.RecordRef) error { | ||
| buf := wb.buffer(ref.FilePath) | ||
| gitDoc, _ := manifestedit.NewDocumentAt(ref.FilePath, buf.current, ref.DocumentIndex) |
There was a problem hiding this comment.
WARNING: Error from manifestedit.NewDocumentAt is ignored
If NewDocumentAt fails (e.g., document index out of bounds after a prior DELETE in the same multi-document file), the error is discarded and a zero-value gitDoc is passed to manifestedit.Apply, which may panic or produce incorrect results. The error should be checked and returned.
| if buf.current == nil { | ||
| return nil | ||
| } | ||
| res, _ := manifestedit.DeleteDocument(buf.current, ref.DocumentIndex) |
There was a problem hiding this comment.
WARNING: Error from manifestedit.DeleteDocument is ignored
If DeleteDocument fails (e.g., document index out of bounds), the error is discarded. The code proceeds with a zero-value res, which has FileEmpty=false and Content=nil, potentially causing the buffer to be set to nil (marking the file for deletion) when it shouldn't be. The error should be checked and returned.
| if err != nil { | ||
| return err | ||
| } | ||
| if d.Type()&os.ModeSymlink != 0 { |
There was a problem hiding this comment.
WARNING: Symlink handling discrepancy - comment says "never followed" but file symlinks are followed
The code only skips directory symlinks (returning filepath.SkipDir). File symlinks fall through to os.ReadFile(p), which follows the symlink and reads the target. This contradicts the comment and could allow reading arbitrary files outside the worktree if a malicious symlink is present in the Git repo. Either skip all symlinks or validate symlink targets stay within the worktree root.
| if d.IsDir() || !isYAMLManifest(p) { | ||
| return nil | ||
| } | ||
| rel, relErr := filepath.Rel(absBase, p) |
There was a problem hiding this comment.
WARNING: Potential path traversal via symlink
filepath.Rel(absBase, p) uses the symlink path (p), not the resolved target. If a file symlink points outside the worktree, Rel returns a path without ".." (since p is inside absBase), but os.ReadFile(p) follows the symlink and reads the target outside the worktree. When writing back, writeAndStageFile would follow the symlink and write to the external target. Consider using os.Lstat to detect symlinks and skip them, or resolve and validate paths stay within the worktree.
| writer: writer, | ||
| mapper: mapper, | ||
| store: store, | ||
| docLoc: store.DocumentLocations(), |
There was a problem hiding this comment.
WARNING: docLoc indices become stale after DELETE in multi-document file
docLoc is computed once at batch start from the initial store. If a batch contains a DELETE for document 0 in a multi-document file followed by an UPDATE for document 1 (same file), the DELETE removes document 0 (shifting indices), but the UPDATE still uses the stale docLoc index (1) which is now out of bounds. The openWindow coalesces by resource identity, so different resources in the same multi-doc file are NOT coalesced and can appear in the same batch. Consider recomputing indices after mutations, collecting all actions before applying, or tracking index shifts per file.
… repo root must be deliberate
1 participant
Support reading a a random dir with files, don't force a certain path structure
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests