Skip to content

fix(kustomize): refuse a folder kustomize cannot build, and read the override chain from the renderer#232

Merged
sunib merged 4 commits into
mainfrom
feat/kustomize-chain-from-render
Jul 14, 2026
Merged

fix(kustomize): refuse a folder kustomize cannot build, and read the override chain from the renderer#232
sunib merged 4 commits into
mainfrom
feat/kustomize-chain-from-render

Conversation

@sunib

@sunib sunib commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

What this does

The override chain — which kustomization's images:/replicas: entries govern a document — was reconstructed by a hand-written DFS over the resources graph, re-deriving kustomize's build order, its cycle rules and its diamond behaviour. It is now read off kustomize's own provenance: every rendered object carries alpha.config.kubernetes.io/transformations, the ordered list of which kustomization configured which builtin transformer. That is the chain, handed to us by the thing that applies it.

The chain is now attributed per object rather than per file, which also closes a latent hazard: a Deployment and a Service sharing one file used to be given the same chain.

The safety hole this surfaced

A folder kustomize cannot build was being silently accepted. The old parse never tried to build anything, so both of these sailed through the acceptance gate:

  • a resources: entry that does not resolve (a manifest moved or renamed);
  • a diamond — one render root reaching a shared base through two overlays — which kustomize rejects outright: "may not add resource with an already registered id".

Neither folder can be deployed by Flux or Argo. And accepting them is not merely untidy — it disarmed the guard protecting them. The override chain, and therefore the write-fan-in precondition, is derived from the render. A root that does not build yields no chain, so no ambiguity is recorded, so the fan-in guard never fires — and the operator would write straight through into a base shared by two render paths. That is the one edit fan-in = 1 exists to forbid.

A render root that fails to build now refuses the GitTarget, quoting kustomize's own error. Three fixtures in our own test suite turned out to be unbuildable, which is how this was found.

LoadRestrictionsNone, which is what Flux uses

The in-memory filesystem contains only the scanned tree, so the filesystem is the jail — which is precisely why Flux can build with LoadRestrictionsNone safely, and why we can too.

RootOnly was the wrong kind of strict: it refuses to load a file from outside the render root (resources: [../shared.yaml]), which Flux renders happily. We would fail to build a root that deploys in production, see no chain for it, and quietly stop enforcing write-fan-in on the file it shares. Refusing to look is not a safety property.

The honest part

Net so far: +36 lines, not a deletion. The new chain-derivation code is about the size of the walk it replaced, because it carries the safety logic the walk didn't have.

The 400-line prize — overrides_projection.go's renderImage, imageSuppliers, simulateImageRender, isReplicaKind — is still standing. I deliberately have not touched it yet. It needs the rendered object plus supplier attribution (differential probing) plus a re-render verifier, and it changes SplitDesiredForOverrides's signature, which means rewriting the 12 tests that are my regression net. That is the single most safety-critical code in the repo — it decides what gets committed to someone's Git repo — and I'm deep into a long session. I'd rather hand over a green, coherent PR that fixes a real safety hole than a rushed rewrite of the write path.

The projection swap is the next PR, with the foundation now fully in place.

Behaviour changes

Documented in docs/UPGRADING.md. A folder whose render root does not build is now refused (GitPathAccepted=False / UnsupportedContent) instead of being written into.

Test fixtures updated rather than worked around:

  • TestAccept_MultipleRetainedSorted referenced a deploy.yaml that did not exist at the path its kustomization pointed at.
  • TestPlanFlush_AcceptsPlainKustomizeFolder never seeded the cm.yaml its kustomization declared.
  • The write-fan-in tests seeded a diamond, which is now refused at the gate and so no longer reaches the write-boundary preconditions they exist to test. They are re-pointed at two separate render roots sharing one base — a shape that does build and does disagree, which is exactly what the fan-in guard is for.

Validation

task fmt · generate · manifests · vet · lint · lint-docs · test all pass. Corpus baseline (support-today.md) unchanged. task test-e2e: green.

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Bug Fixes

    • Kustomize render roots are now fully built and validated, improving detection of invalid or incomplete configurations.
    • Git targets with build failures are refused with GitPathAccepted=False and actionable error details.
    • Invalid image patterns, cycles, shared-base configurations, and unresolved resources are handled consistently.
    • Image and replica overrides are attributed more accurately, with ambiguous edits safely withheld.
  • Documentation

    • Added upgrade guidance and migration steps for resolving newly detected Kustomize build failures.
    • Added design documentation covering acceptance boundaries, render attribution, render-root scoping, and values-file handling.

…override chain from the renderer

Two changes, and the second is why the first matters.

1. The override chain — which kustomization's images:/replicas: entries govern a
   document — was reconstructed by a hand-written DFS over the resources graph,
   re-deriving kustomize's build order, its cycle rules and its diamond behaviour.
   It is now read off kustomize's own provenance: every rendered object carries
   alpha.config.kubernetes.io/transformations, the ordered list of which
   kustomization configured which builtin transformer. That IS the chain, handed to
   us by the thing that applies it.

   It is now attributed per OBJECT rather than per file, which closes a latent
   hazard: a Deployment and a Service sharing one file were given the same chain.

2. A folder whose render root kustomize cannot BUILD was being silently accepted,
   because the old parse never tried to build anything. Both of these passed the
   acceptance gate:

     - a resources: entry that does not resolve (a manifest moved or renamed);
     - a diamond — one render root reaching a shared base through two overlays —
       which kustomize rejects outright: "may not add resource with an already
       registered id".

   Neither folder can be deployed by Flux or Argo. And accepting them did not just
   look untidy, it DISARMED THE GUARD PROTECTING THEM: the override chain, and
   therefore the write-fan-in precondition, is derived from the render. A root that
   does not build yields no chain, so no ambiguity is recorded, so the fan-in guard
   never fires — and the operator would write straight through into a base shared by
   two render paths, which is the one edit fan-in = 1 exists to forbid.

   A render root that fails to build now refuses the GitTarget, quoting kustomize's
   own error. Three fixtures in our own test suite turned out to be unbuildable,
   which is how this was found.

Also: LoadRestrictionsNone, which is what Flux itself builds with. The in-memory
filesystem contains only the scanned tree, so the filesystem is the jail — precisely
why Flux can use it safely. RootOnly was the wrong kind of strict: it refuses to load
a FILE from outside the render root (resources: [../shared.yaml]) which Flux renders
happily, so we would fail to build a root that deploys in production, see no chain
for it, and quietly stop enforcing write-fan-in on the file it shares. Refusing to
look is not a safety property.

The write-fan-in tests seeded a diamond, which is now refused at the gate and no
longer reaches the write-boundary preconditions they exist to test. They are
re-pointed at two separate render roots sharing one base — a shape that does build
and does disagree, which is exactly what the fan-in guard is for.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Jul 14, 2026

Copy link
Copy Markdown

Review Change Stack

Warning

Review limit reached

@sunib, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 43 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: c9c54cbc-1298-414b-8d67-9cd5d8ffecf4

📥 Commits

Reviewing files that changed from the base of the PR and between de0d5e4 and 5ea7019.

📒 Files selected for processing (6)
  • docs/design/support-boundary/acceptance-precision.md
  • docs/design/support-boundary/generated-repo-map.md
  • docs/design/support-boundary/patching-kustomize.md
  • docs/design/support-boundary/render-attribution.md
  • docs/design/support-boundary/render-root-scoping.md
  • docs/design/support-boundary/values-file-projection.md
📝 Walkthrough

Walkthrough

The analyzer now builds every Kustomize render root, refuses unbuildable content, records render failures, and derives identity-qualified override chains. Tests and upgrade documentation cover these behaviors, while new design documents define broader support-boundary changes.

Changes

Kustomize render validation

Layer / File(s) Summary
Render-root build validation
internal/manifestanalyzer/kustomize_render.go, internal/manifestanalyzer/override_chain.go, internal/manifestanalyzer/scan_repo.go
Render roots and cycle targets are selected deterministically; remote resources, invalid image patterns, Kustomize errors, and build panics are converted into refusal errors.
Identity-qualified override attribution
internal/manifestanalyzer/override_chain.go, internal/manifestanalyzer/overrides.go, internal/manifestanalyzer/kustomize_render_test.go
Override chains are derived from transformer provenance and keyed by origin path, kind, and name; duplicate and conflicting chains are handled explicitly.
Store and acceptance integration
internal/manifestanalyzer/store.go, internal/manifestanalyzer/acceptance.go, internal/manifestanalyzer/acceptance_test.go, internal/git/*_test.go
Render failures become diagnostics and unsupported retained documents, while acceptance fixtures now provide buildable resources and render roots.
Render and acceptance regression coverage
internal/manifestanalyzer/*_test.go, .coverage-baseline
Tests cover hostile image patterns, panic recovery, cycles, duplicate provenance, unbuildable diamonds, ambiguity, rendered images, and updated coverage.
Upgrade documentation
docs/UPGRADING.md
The unreleased migration note documents render-root builds, refusal conditions, and remediation steps.

Support-boundary design

Layer / File(s) Summary
Acceptance-boundary specification
docs/design/support-boundary/acceptance-precision.md
Defines acceptance scope, unsupported Kustomize features, generated-output handling, refusal messaging, and rollout priorities.
Render attribution specification
docs/design/support-boundary/render-attribution.md
Compares attribution strategies and specifies dye-based attribution with real-render verification and a phased implementation plan.
Render-root scoping specification
docs/design/support-boundary/render-root-scoping.md
Defines oracle-based verification, overlay routing, scope rules, fan-in handling, and patch behavior.
Values-file projection specification
docs/design/support-boundary/values-file-projection.md
Defines referenced values-file context, synthetic editing projections, write-back constraints, and Helm capability boundaries.

Estimated code review effort: 4 (Complex) | ~60 minutes

Possibly related PRs

Sequence Diagram(s)

sequenceDiagram
  participant GitTarget
  participant ManifestAnalyzer
  participant Kustomize
  participant ManifestStore
  GitTarget->>ManifestAnalyzer: analyze folder
  ManifestAnalyzer->>Kustomize: build each render root
  Kustomize-->>ManifestAnalyzer: rendered objects or build failure
  ManifestAnalyzer->>ManifestStore: assignments and render diagnostics
  ManifestStore-->>GitTarget: accepted target or UnsupportedContent refusal
Loading
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Description check ⚠️ Warning The PR description is detailed but does not follow the required template and omits several required sections. Reformat it to the repository template and add the missing sections, especially Type of Change, Testing, Checklist, Related Issues, Screenshots, and Additional Notes.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title is concise and accurately summarizes the main change.
Docstring Coverage ✅ Passed Docstring coverage is 91.18% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/kustomize-chain-from-render

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@codecov

codecov Bot commented Jul 14, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 94.93671% with 8 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
internal/manifestanalyzer/override_chain.go 92.8% 4 Missing and 4 partials ⚠️

📢 Thoughts on this report? Let us know!

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

🧹 Nitpick comments (1)
internal/manifestanalyzer/kustomize_render_test.go (1)

52-52: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win

Compute renderChains once per subtest.

ourChainFor currently rebuilds every render root for every rendered object. Precompute the assignments before the object loop and make the helper perform only the map lookup.

Proposed refactor
 			kusts := parseKustomizations(root.files)
+			chains, _ := renderChains(root.files, kusts)
 			for _, ro := range rendered {
 				...
-				chain, ambiguous := ourChainFor(root.files, kusts, ro)
+				chain, ambiguous := ourChainFor(chains, ro)
 func ourChainFor(
-	files []manifestedit.FileContent,
-	kusts map[string]*kustomizationDoc,
+	chains map[chainKey]*overrideAssignment,
 	ro renderedObject,
 ) (*KustomizeOverrides, bool) {
-	chains, _ := renderChains(files, kusts)
 	a := chains[chainKey{

Also applies to: 99-112

🤖 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/kustomize_render_test.go` at line 52, Update each
subtest around ourChainFor to compute renderChains once before iterating over
rendered objects, then pass or reuse those precomputed assignments. Change
ourChainFor to perform only the corresponding map lookup, avoiding repeated
render-root reconstruction for every object while preserving the existing chain
and ambiguity results.
🤖 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/manifestanalyzer/override_chain.go`:
- Around line 48-68: The render-root discovery around renderRoots must handle
disconnected cyclic components instead of returning no roots and silently
skipping them. Update the rendering flow, including renderChains, to track
unvisited directories and either start each cycle from a deterministic
representative or emit the established build-failure diagnostic; preserve normal
root ordering and behavior. Add a regression test covering a standalone cycle
and ensure the new Go logic is covered.
- Around line 31-38: Extend chainKey and the rendering flow used by
resolveOverrides to include Location.DocumentIndex as a stable per-document
discriminator. Populate and reuse this index in both override assignment and
lookup so distinct documents from the same source, kind, and name retain
separate chains; do not use namespace as the discriminator.

In `@internal/manifestanalyzer/store.go`:
- Around line 410-415: Update the godoc for the exported retained-state field or
type associated with RetainedDocument.Unsupported to explicitly include
kustomize build failures alongside unsupported features and malformed overrides.
Also update unsupportedKustomizeRefusals so refusal messages identify build
failures as a possible cause, preserving existing explanations for the other
cases.
- Around line 422-428: Sort the paths from renderFailures before iterating and
appending to store.Diagnostics in the render-failure diagnostic block. Preserve
each path’s associated message and existing diagnostic fields, while ensuring
diagnostics are emitted in deterministic path order.

---

Nitpick comments:
In `@internal/manifestanalyzer/kustomize_render_test.go`:
- Line 52: Update each subtest around ourChainFor to compute renderChains once
before iterating over rendered objects, then pass or reuse those precomputed
assignments. Change ourChainFor to perform only the corresponding map lookup,
avoiding repeated render-root reconstruction for every object while preserving
the existing chain and ambiguity results.
🪄 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: 36f6a01e-e406-4d62-9600-dcc22cf5fee9

📥 Commits

Reviewing files that changed from the base of the PR and between 8a835f0 and dba61b9.

📒 Files selected for processing (10)
  • docs/UPGRADING.md
  • internal/git/acceptance_gate_test.go
  • internal/git/write_boundary_precondition_test.go
  • internal/manifestanalyzer/acceptance_test.go
  • internal/manifestanalyzer/kustomize_render.go
  • internal/manifestanalyzer/kustomize_render_test.go
  • internal/manifestanalyzer/override_chain.go
  • internal/manifestanalyzer/overrides.go
  • internal/manifestanalyzer/overrides_test.go
  • internal/manifestanalyzer/store.go

Comment thread internal/manifestanalyzer/override_chain.go
Comment thread internal/manifestanalyzer/override_chain.go
Comment thread internal/manifestanalyzer/store.go
Comment thread internal/manifestanalyzer/store.go Outdated
… the roots that have none

Two ways a folder could get past the refusal path that #232 introduced, both found by
probing kustomize rather than by reading it.

An images: entry's name: is a REGULAR EXPRESSION, and kustomize compiles it while
discarding the compile error (api/internal/image/image.go) before dereferencing the nil
*Regexp. So `- name: "ngin["` does not fail the build — it panics inside it, on bytes that
came straight out of a user's repository. Nothing reaches krusty on main; #232 is what puts
it on the store-build path, so this lands with it. controller-runtime recovers reconciler
panics by default, which does not make it safe: it turns a crash into panic → requeue →
panic, for as long as the repository stays as it is. The manifest-analyzer CLI has no such
net at all. Refuse the entry before the build, and put a recover() under krusty for the
panics we have not found yet: a build runs library code we do not own over bytes we do not
control, so a panic there has to become a refused folder.

A cycle (a → b → a) has NO render root — every directory in it is referenced by another —
so renderRoots returned nothing for it, renderChains built nothing, and no failure was
recorded. That is exactly the hole the refusal exists to close: no build means no chain, no
chain means no ambiguity, and no ambiguity means the write-fan-in guard never fires on a
folder kustomize cannot build at all. Cover every kustomization with some build attempt and
let kustomize give the verdict; it says "cycle detected", and Flux would say the same.

Also, from the same probing: kustomize builds one ImageTagTransformer PER images: ENTRY and
stamps them all with the same origin, so a kustomization with three entries leaves three
byte-identical transformation records on every object in the build — and chainOf added that
file's whole entry list once per record. Dedupe the records. The annotation's comments said
kustomize records "the transformers that touched each resource"; it does not, and it cannot:
it records every transformer that RAN, against every object in the build, modified or not
(api/resmap/reswrangler.go loops the whole ResMap with no diff check). Say what it does.

Review feedback from #232, all addressed: sort render-failure diagnostics (map iteration was
nondeterministic), tell users about build failures in the Unsupported godoc and in the
refusal message, and compute renderChains once per fixture in the differential test instead
of once per rendered object.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@sunib

sunib commented Jul 14, 2026

Copy link
Copy Markdown
Contributor Author

Pushed 5a85afa. All four review threads are answered and resolved; the nitpick is in too (renderChains is now computed once per fixture in TestRenderRoot_ImagesAgreeWithKustomize, not once per rendered object).

One thing the review did not find, and it outranks everything it did — this PR is what makes a panic reachable from user-controlled repository content.

An images: entry's name: is a regular expression, and kustomize compiles it while discarding the compile error (api/internal/image/image.go) before dereferencing the nil *Regexp:

pattern, _ := regexp.Compile("^" + name + "(:[a-zA-Z0-9_.{}-]*)?(@sha256:...)?$")

So - name: "ngin[" does not fail the build — it panics inside it. Measured:

RESULT4 malformed regex name -> *** PANIC *** invalid memory address or nil pointer dereference

krusty has no non-test caller on main, so this is unreachable today; this PR puts it on the store-build path. controller-runtime recovers reconciler panics by default, which does not make it safe — it turns a crash into panic → requeue → panic, for as long as the repository stays as it is, on a GitTarget nobody can fix except by editing the repo. The manifest-analyzer CLI has no such net and simply dies.

Two layers, both in this push:

  1. Refuse before the build. Validated against the exact pattern kustomize compiles, so we accept precisely what it accepts. The corollary is worth knowing: - name: "ngin." matches nginx, because it is a regex — it still builds, and there is a test pinning that.
  2. recover() under krusty. A build runs library code we do not own over bytes we do not control, so a panic there has to become a refused folder, not a wedged process. errBuildPanicked.

Also folded in from the same probing: kustomize builds one ImageTagTransformer per images: entry and stamps them all with the same origin, so three entries leave three byte-identical transformations records on every object in the build — and chainOf was appending that file's whole entry list once per record. Deduped. Relatedly, the comments claiming the annotation records "the transformers that touched each resource" were wrong: it records every transformer that ran, against every object in the build, modified or not (api/resmap/reswrangler.go loops the whole ResMap with no diff check). A ConfigMap no image transformer could touch still collects ImageTagTransformer records. The comments now say what it does.

UPGRADING.md covers the two new refusals (cycles, invalid name: regexes). Full sequence green: fmt → generate → manifests → vet → lint → test → test-e2e (54 passed / 0 failed / 11 skipped). Unit coverage 75.5% → 75.6%. Corpus baseline regenerated: zero rows moved — no fixture has a cycle or a regex name:, which is also why neither path was ever exercised.

🤖 Generated with Claude Code

…docs it argues from

The Go comment in kustomize_render_hostile_test.go points at render-attribution.md, so
doccheck refuses the reference until the doc is tracked — and the doc links to the three
design docs it builds on, so they land together or not at all.

render-attribution.md is the design for the last step of the kustomize-renderer workstream:
deleting the re-implemented transformers the write path still uses to decide which file an
edit belongs in. Its argument, in short:

- kustomize has NO field-level provenance, at any visibility level, and no DAG object even
  internally — accumulation is control flow, never a retained structure. So attribution
  ("which images: entry supplied this tag?") cannot be READ out of the renderer. It can only
  be inferred by questioning it, and every viable design is a query strategy.
- Leave-one-out probing (re-render with one entry removed; whatever moves is what it
  supplied) is measured WRONG, and would regress today's code: removal probes the VALUE, and
  values collide. It is blind whenever an overlay pins the tag the base already has — the
  steady state of a GitOps repo — and blind on ties.
- Dyeing every entry with a distinguishable nonce and rendering ONCE is the same idea with
  the flaw removed: it resolves ties, costs 2 builds instead of 1+N, and is the only
  field-level provenance available for kustomize at any price.
- Attribution may be heuristic. Verification may not — which is why the real re-render
  replaces simulateImageRender first, alone.

§6 is the bug ledger the measuring produced, and it is the justification for the deletion:
our matcher is string equality where kustomize's is a regex over the whole image string,
isReplicaKind misses ReplicationController, and collectContainerSlots collects
ephemeralContainers (kustomize won't touch them) while missing OCI volume images (it will).
All three exist only because the re-implementation exists. It also records P1 and C1, the
panic and the cycle fixed in 5a85afa.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 7

🧹 Nitpick comments (1)
internal/manifestanalyzer/kustomize_render.go (1)

126-139: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win

renderRoot re-parses all kustomizations on every render target.

renderRoot calls parseKustomizations(files) internally, even though its only caller (renderChains in override_chain.go) already computed kusts and passes only files. This re-parses the entire file set once per render target instead of once per store build — an O(targets × files) cost where O(files) suffices.

Thread the already-parsed kusts map through instead of re-deriving it per call.

♻️ Proposed fix
-func renderRoot(files []manifestedit.FileContent, rootDir string) ([]renderedObject, error) {
-	if err := refuseBeforeBuild(parseKustomizations(files), rootDir); err != nil {
+func renderRoot(files []manifestedit.FileContent, kusts map[string]*kustomizationDoc, rootDir string) ([]renderedObject, error) {
+	if err := refuseBeforeBuild(kusts, rootDir); err != nil {
 		return nil, err
 	}

And update the caller in override_chain.go:

-		rendered, err := renderRoot(files, rootDir)
+		rendered, err := renderRoot(files, kusts, rootDir)
🤖 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/kustomize_render.go` around lines 126 - 139, Update
renderRoot to accept the already-parsed kustomizations map and pass it to
refuseBeforeBuild instead of calling parseKustomizations(files). Modify the
renderChains caller in override_chain.go to supply its existing kusts map,
preserving the current rendering and error behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@docs/design/support-boundary/acceptance-precision.md`:
- Around line 131-134: Add the text language identifier to the fenced Markdown
block in acceptance-precision.md, changing the opening fence to use text while
preserving the generated-content comments and closing fence.

In `@docs/design/support-boundary/render-attribution.md`:
- Around line 238-241: Revise the attribution guidance around the “any knob”
claim to limit nonce dyeing to fields proven to be pure sinks. Require a
per-field proof that the dye cannot affect selectors, references, name
generation, validation, or unrelated output, and apply the baseline-first
guardrail described later before using the technique.
- Around line 52-65: Update docs/design/support-boundary/render-attribution.md
lines 52-65 to present the non-dependency discussion as historical pre-renderer
context and remove the present-tense claim that kustomize is not a dependency;
document the shipped renderer contract instead. In
docs/design/support-boundary/render-root-scoping.md lines 87-109, replace
LoadRestrictionsRootOnly guidance with the implemented LoadRestrictionsNone
behavior, in-memory filesystem jail, disabled plugins, and pre-build remote-base
validation.
- Around line 42-46: Revise the attribution design language around the
query-strategy discussion so it does not present simulateImageRender as a full
rendering oracle or blast-radius verifier. Describe its behavior as
image-specific verification against each planned source image and expected live
image, unless the implementation is expanded to compare all unrelated rendered
objects byte-for-byte.
- Around line 88-91: Add appropriate language identifiers to the fenced Markdown
blocks containing the attribution examples, including the blocks around the
shown example and the additional referenced sections. Use text for plain output
examples, or yaml where the contents are valid YAML, while preserving all
example content.
- Around line 387-391: Clarify the “Baseline first, then dye” guardrail so that
any dyed-build failure is a hard refusal: produce no attribution and perform no
write. Explicitly prohibit falling back to renderImage, leave-one-out probing,
or any other heuristic attribution path.

In `@docs/design/support-boundary/values-file-projection.md`:
- Around line 62-70: Update the values-file projection description to scope the
read-only file rule exclusively to Application.valueFiles. Describe
Application.valuesObject as inline YAML and HelmRelease.valuesFrom as a KRM
resource reference, documenting both as separate editable surfaces rather than
file-backed projections.

---

Nitpick comments:
In `@internal/manifestanalyzer/kustomize_render.go`:
- Around line 126-139: Update renderRoot to accept the already-parsed
kustomizations map and pass it to refuseBeforeBuild instead of calling
parseKustomizations(files). Modify the renderChains caller in override_chain.go
to supply its existing kusts map, preserving the current rendering and error
behavior.
🪄 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: 2deda266-d782-4bfe-a6a5-8da28a830da4

📥 Commits

Reviewing files that changed from the base of the PR and between dba61b9 and de0d5e4.

📒 Files selected for processing (13)
  • .coverage-baseline
  • docs/UPGRADING.md
  • docs/design/support-boundary/acceptance-precision.md
  • docs/design/support-boundary/render-attribution.md
  • docs/design/support-boundary/render-root-scoping.md
  • docs/design/support-boundary/values-file-projection.md
  • internal/manifestanalyzer/acceptance.go
  • internal/manifestanalyzer/kustomize_render.go
  • internal/manifestanalyzer/kustomize_render_hostile_test.go
  • internal/manifestanalyzer/kustomize_render_test.go
  • internal/manifestanalyzer/override_chain.go
  • internal/manifestanalyzer/scan_repo.go
  • internal/manifestanalyzer/store.go
🚧 Files skipped from review as they are similar to previous changes (2)
  • internal/manifestanalyzer/kustomize_render_test.go
  • docs/UPGRADING.md

Comment thread docs/design/support-boundary/acceptance-precision.md Outdated
Comment thread docs/design/support-boundary/render-attribution.md
Comment thread docs/design/support-boundary/render-attribution.md
Comment thread docs/design/support-boundary/render-attribution.md Outdated
Comment thread docs/design/support-boundary/render-attribution.md Outdated
Comment thread docs/design/support-boundary/render-attribution.md Outdated
Comment thread docs/design/support-boundary/values-file-projection.md Outdated
… shipped

Review feedback on #232, all of it fair, plus one thing it asked for.

**Accuracy.** The docs were written before the renderer landed and several passages had
gone stale in ways that mattered:

- render-root-scoping said `sigs.k8s.io/kustomize` "is not a dependency of this module.
  Zero hits in go.mod". It is one, since #229 — that is the argument the doc *won*. Marked
  historical, and the inventory table now says which rows are gone (the 17-key deny-list,
  replaced by reflection over kustomize's struct) and which are still standing (renderImage,
  isReplicaKind — with the divergences they cause).
- The sandbox table proposed `LoadRestrictionsRootOnly`. The shipped renderer uses
  `LoadRestrictionsNone`, deliberately, because RootOnly rejects `resources: [../shared.yaml]`
  which Flux renders happily — and failing to build a root silently disarms the write-fan-in
  guard. The contract as built is recorded rather than quietly corrected.
- The doc claimed simulateImageRender "already does" propose-re-render-compare. It does not:
  it replays OUR re-implemented chain, not kustomize, over only the images it planned, and
  never checks that the rest of the build is untouched. It shares a blind spot with the thing
  it verifies, which is the whole reason it has to die. Said plainly.
- values-file-projection conflated three different surfaces. Verified upstream: Argo's
  `helm.valueFiles` and Flux's `spec.chart.spec.valuesFiles` name a FILE; `valuesObject` is
  inline RawExtension in the Application; Flux's `valuesFrom` is a KRM object reference
  (`Secret/my-secret-values`). Only the first is a values file. Collapsing them would claim a
  file-projection capability over content living inside another document, or inside a Secret.

**render-attribution**, tightened where the review was right:

- "put a nonce in ANY knob" was an overclaim. Each new field costs a sink proof, and the
  fields worth reaching for next are the ones most likely to fail it: a replacements source is
  read by a selector, a generator literal feeds a name hash, a patch's containers[].name is a
  merge key.
- "refuse and fall back" was ambiguous, and the review's instinct was sharper than it knew.
  Falling back to write-through is NOT harmless: writing a live value into a source document
  whose field an entry governs does not converge — the entry overrides it on the next render,
  which is the same non-convergence that convicts leave-one-out in §2. What makes the fallback
  safe is that the verification re-render CATCHES it. Fallback means no attribution, never
  another heuristic.

**acceptance-precision** now contains only what is not implemented, as asked. `vars` is gone
from it: #229 closed that hole, and closed it structurally — the unsupported set is derived by
reflecting over kustomize's own struct, so a field we have never heard of refuses instead of
being tolerated. Every remaining claim re-verified against the code (labels/commonLabels are
still tolerated and still leak; Generated{path} is still emitted by nothing; the values file
still cannot be rescued by .gittargetignore because *.yaml is a catastrophic pattern).

Fenced blocks given language identifiers (MD040).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@sunib
sunib merged commit 960e3bd into main Jul 14, 2026
18 checks passed
@sunib
sunib deleted the feat/kustomize-chain-from-render branch July 14, 2026 15:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant