fix(kustomize): stop writing the build's own output into the build's input#234
Merged
Conversation
…input
The writer mirrors a live object into the file that produced it. Under kustomize
that file is not what the cluster runs, and mirroring the live object straight
back writes the BUILD'S OUTPUT into the build's INPUT.
Measured, on a folder we accept today, with nothing changed in the cluster and
nothing changed in the render: a kustomization declaring `labels:` +
`commonAnnotations:` and nothing else commits the overlay's `env: prod` and
`owner: platform` into the base manifest, on the first reconcile of an unchanged
folder. Every reconcile of an in-sync folder produced a commit, and the file was
left wrong. In a base shared by two overlays, the value baked in is one
environment's.
The fix is one rule, and it models no transformer:
WHERE THE LIVE OBJECT AND THE RENDER AGREE, THE SOURCE KEEPS ITS BYTES.
WHERE THEY DISAGREE, THE USER CHANGED SOMETHING, AND THAT IS WHAT WE WRITE.
Agreement means the build already produces exactly what the cluster runs, so the
source is — by construction — what produced it, and there is nothing to write.
Disagreement is the user's edit: it is routed to an images:/replicas: entry when
the dye says one supplies the field, and written through to the source otherwise
— where, if the build owns the field, the re-render refuses the flush.
Because it needs no model of labels, of namespace, or of a patch, it closes all
of them at once. It is also the gate on tolerating `patches:` at all: a patched
base would otherwise absorb one environment's values, and no re-render can catch
that (the patch re-imposes its value, so the render comes out identical).
Two behavior changes go with it:
- the re-render now runs for ANY document a render root produces, not only one
an override chain governs. A change to a build-supplied field in a folder
with no images:/replicas: entries used to be committed and silently never
converge; it is now a reported refusal.
- a live change the projection cannot place is refused (`unplaceable-edit`):
the build and the user both rewrote one list whose elements carry no unique
`name:` to pair them by. Pairing by position is not a conservative guess, it
is measurably wrong — kustomize PREPENDS a container a patch adds.
The corpus no-op invariant now compares the WHOLE document, not just its images,
which is how a projection that quietly rewrote every field we had not modelled
passed it for as long as it did: 8 documents checked before, 53 now.
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (13)
📝 WalkthroughWalkthroughThe PR adds source-form reconciliation for Kustomize output, render-versus-live token detection, epoch-based write gating, GitTarget status reporting, refusal classification, tests, design documentation, and related CRD and coverage updates. ChangesSource-form reconciliation and render-fidelity gating
Estimated code review effort: 5 (Critical) | ~90 minutes Sequence Diagram(s)sequenceDiagram
participant LiveObject
participant EventRouter
participant FidelityGate
participant BranchWorker
participant GitTarget
LiveObject->>EventRouter: resync or live update
EventRouter->>FidelityGate: record clean or divergent scope result
FidelityGate-->>BranchWorker: allow or block normal Git writes
FidelityGate-->>GitTarget: publish RenderMatchesLive status
Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/render-root-scoping.md`:
- Around line 90-93: Update the `vars` support statement in the support-boundary
document to remove the contradiction: do not claim folders containing `vars` are
accepted today, and describe that behavior as historical or currently rejected
consistent with the parenthetical about `#229`.
In `@internal/manifestanalyzer/acceptance.go`:
- Around line 137-148: Fix the doc comment for IssueUnplaceableEdit by replacing
the malformed sentence with clear, grammatical wording that explains the source
and rendered list elements cannot be paired because they have no unique names.
Remove the stray colon and ensure the SourceFormRefusedError reference is
integrated naturally.
In `@internal/manifestanalyzer/source_form.go`:
- Around line 69-72: Complete the truncated error message returned by
SourceFormRefusedError.Error so it forms a grammatically complete refusal
explanation, preserving the existing context about the changed build/live
objects and unnamed elements. Ensure the full message is surfaced through
WriteBoundaryRefused without altering unrelated error handling.
🪄 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: e2b06bc3-7e98-4461-9c2e-be479a15b3e7
📒 Files selected for processing (16)
.coverage-baselinedocs/UPGRADING.mddocs/design/support-boundary/render-root-scoping.mdinternal/git/plan_flush.gointernal/git/source_form_test.gointernal/manifestanalyzer/acceptance.gointernal/manifestanalyzer/analyzer_test.gointernal/manifestanalyzer/kustomization_parse.gointernal/manifestanalyzer/kustomize_render_semantics_test.gointernal/manifestanalyzer/kustomize_render_test.gointernal/manifestanalyzer/overrides_attribution.gointernal/manifestanalyzer/overrides_projection.gointernal/manifestanalyzer/overrides_projection_test.gointernal/manifestanalyzer/source_form.gointernal/manifestanalyzer/source_form_test.gointernal/watch/event_router.go
sunib
added a commit
that referenced
this pull request
Jul 15, 2026
…e, fix the vars contradiction The SourceFormRefusedError message (surfaced to the user via WriteBoundaryRefused) and its IssueUnplaceableEdit doc comment both cut off mid-clause; complete them. And §2's vars bullet claimed the folder is accepted today while a parenthetical said it now refuses -- rewrite the passage in past tense (both leaks it cites are now closed, by #229 and by sourceForm).
…e, fix the vars contradiction The SourceFormRefusedError message (surfaced to the user via WriteBoundaryRefused) and its IssueUnplaceableEdit doc comment both cut off mid-clause; complete them. And §2's vars bullet claimed the folder is accepted today while a parenthetical said it now refuses -- rewrite the passage in past tense (both leaks it cites are now closed, by #229 and by sourceForm).
Contributor
Author
Review concern: we render kustomize, but not with the orchestrator's contextThis PR closes the leak where our render's output leaked into the source. But there is a broader, pre-existing version of the same blind spot it does not close, and we should stop implying it does: the folder we render is not the whole render spec. The orchestrator supplies render context that is not in the repo:
So The docs frame the residual as version skew only ( Proposed direction — support a little too little, not a little too much
None of this blocks this PR — the |
…lementation Bring the support-boundary design docs onto this PR — they are the design that sits next to this real implementation, so they belong together rather than in a separate PR: render-fidelity.md (our render is not the orchestrator's; the render-vs-live fence, the blocking RenderFaithful condition, and where to implement it), admission-consent.md, orchestrator-reconcile-trigger.md, and the kpt-and-krm-functions orientation note, plus their README index rows. Consolidated from PR #237, now closed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…atchesLive RenderMatchesLive names what it measures directly — does our render match the live object — better than the 'faithful' framing. The negative reason follows: RenderNotFaithful -> RenderDoesNotMatchLive, and the gate AcceptRenderFaithful -> AcceptRenderMatchesLive. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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/design/support-boundary/admission-consent.md`:
- Around line 120-125: Update the consent token design around the consequence
content hash to hash a canonical representation of the complete consequence,
including affected fields, old and new values, and source revision—not only
object/environment pairs. Include the relevant GitTarget and authorization scope
in the hashed input, and require authorization to be rechecked at flush time
before honoring consent.
In `@docs/design/support-boundary/render-fidelity.md`:
- Around line 224-234: The document overstates what RenderMatchesLive proves:
the described token-only check cannot establish full render fidelity or safely
serve as an adoption gate. Either rename and narrow the condition to token
fidelity throughout the affected sections, or implement the general
untouched-field precondition so Argo overrides, post-build changes, and version
skew are covered before retaining the full-fidelity claim.
🪄 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: 2a64f97c-7f80-440a-9771-c3bf1f7ae722
📒 Files selected for processing (5)
docs/design/support-boundary/README.mddocs/design/support-boundary/admission-consent.mddocs/design/support-boundary/kpt-and-krm-functions.mddocs/design/support-boundary/orchestrator-reconcile-trigger.mddocs/design/support-boundary/render-fidelity.md
…tation prompt Align the prose to the RenderMatchesLive framing (the "faithful/unfaithful" language follows the rename to "matches / diverges"), and mark §8's 5a-first as decided rather than open. Add next-prompt-render-matches-live-gate.md: a handoff for a fresh session to implement the token gate — the design, the code entry points (patchExisting + the resync path), the CRD lesson (measure render-vs-live, not the disk; the reverted structural check broke CRD mirroring), the test net, and the e2e-tail gotcha. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Incorporate the review findings:
- The predicate must compare the RENDER to live, not the Git source to live.
kustomize does not RESOLVE ${...}, but it does not PRESERVE every token-bearing
source field either: a supported labels/commonLabels transform overwrites
metadata.labels[...] via SetEntry, so `env: ${ENV}` under `labels: {env: prod}`
renders to `prod` == live. Git-vs-live would falsely refuse that faithful folder.
The render is dm.Rendered.Object for a kustomize doc, the Git doc for a plain one,
and reading the render also catches a token a patches: block injects. Fixed the
prompt (which had the git-vs-live shortcut + "does not need dm.Rendered") and §5a.
- Narrow the causal claim in the fact doc and the design: a rendered token that
differs from live proves only that our render did not produce the value — could be
Flux substitution, a live edit, admission, another controller. Refusal is safe
regardless; RenderDoesNotMatchLive is a fact, "must have been substituted" a guess.
- State the two integration requirements for the blocking gate: aggregate fidelity
across ALL scoped resyncs (a last-successful-GVR status masks a diverging type),
and stop opening write windows while failed (status-only refusal isn't a gate).
- Record the bias: blocking a shade too soon is fine; failing to block is not. The
simple token gate is the right first cut.
Also fixes the fact doc's external-sources markdown links (doccheck treats upstream
checkouts as untracked) to code spans, matching the convention in the sibling docs.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
internal/controller/gittarget_controller.go (1)
205-207: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
git.RenderFidelityStateconstants instead of string literals.Several assignments and comparisons use magic string literals (
"True","Unknown","False") rather than the exported state constants defined in thegitpackage. Using the constants ensures type safety and consistency across the codebase.
internal/controller/gittarget_controller.go#L205-L207: replace"True"withgit.RenderFidelityTrue.internal/controller/gittarget_controller.go#L221-L224: replace"Unknown"withgit.RenderFidelityUnknown.internal/controller/gittarget_status_test.go#L128-L128: replace"True"withgit.RenderFidelityTrue.internal/controller/gittarget_status_test.go#L159-L161: replace"Unknown"withgit.RenderFidelityUnknown.internal/controller/gittarget_status_test.go#L165-L167: replace"False"withgit.RenderFidelityFalse.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/controller/gittarget_controller.go` around lines 205 - 207, Replace the magic render-fidelity state strings with the exported git.RenderFidelityState constants: use git.RenderFidelityTrue and git.RenderFidelityUnknown in internal/controller/gittarget_controller.go at lines 205-207 and 221-224, and use git.RenderFidelityTrue, git.RenderFidelityUnknown, and git.RenderFidelityFalse in internal/controller/gittarget_status_test.go at lines 128, 159-161, and 165-167 respectively.internal/controller/gittarget_status_test.go (1)
153-153: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFollow test naming convention and consider a table-driven approach.
As per coding guidelines, tests should use the
TestFunctionName_Scenarionaming convention and employ a table-driven approach where appropriate. Consider renaming this test to reflect the scenario (e.g.,TestDeriveGitTargetDataPlaneStatusWithRenderFidelity_AllStates) and refactoring the sequential assertions into a table-driven structure to clearly map eachRenderFidelityStateto its expected outcomes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/controller/gittarget_status_test.go` at line 153, Rename TestDeriveGitTargetDataPlaneStatusWithRenderFidelity to follow the TestFunctionName_Scenario convention, using an all-states scenario name. Refactor its sequential assertions into a table-driven test that maps each RenderFidelityState to the expected status outcomes.Source: Coding guidelines
internal/manifestanalyzer/render_fidelity_test.go (1)
93-93: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove redundant
unstructured.Unstructuredwrapper.Wrapping the
objectmap in&unstructured.Unstructured{Object: object}and immediately accessing.Objectreturns the exact same map reference. You can returnobjectdirectly.💡 Proposed refactor
- return (&unstructured.Unstructured{Object: object}).Object + return object🤖 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/render_fidelity_test.go` at line 93, Update the return statement in the surrounding helper to return object directly, removing the redundant unstructured.Unstructured wrapper and immediate .Object access while preserving the same map reference.internal/watch/event_router.go (1)
225-227: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse static messages for structured logging. As flagged by static analysis, concatenating dynamic variables like
kindinto the primary log message string is an anti-pattern in structured logging. Keeping the primary log message static while passing dynamic variables as fields improves log indexability and safeguards against log injection.
internal/watch/event_router.go#L225-L227: Replace"per-type "+kind+" applied"with a static string (e.g.,"per-type applied") and add"kind", kindto the logging arguments.internal/watch/event_router.go#L259-L260: Replace"per-type "+kind+" found a render-vs-live divergence"with a static string and add"kind", kindto the logging arguments.🤖 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/watch/event_router.go` around lines 225 - 227, Update both logging sites in internal/watch/event_router.go: in the per-type applied log at lines 225-227 and the render-vs-live divergence log at lines 259-260, use static primary messages and add the dynamic kind value as a structured "kind" field alongside the existing fields.Source: Linters/SAST tools
🤖 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/orchestrator-reconcile-trigger.md`:
- Around line 194-195: Update the orchestrator apply barrier around the Rev→Git
“land pending intent” step to require effective-render fidelity before treating
drift as a no-op. Resume processing only when the applied result matches the
local render; otherwise return a named refusal such as TransformedOutOfBand,
preventing externally injected Flux/Argo transformations from being projected
into source templates.
- Around line 221-229: Define fail-closed handling for the finalize-and-push,
orchestrator trigger, and reconcile-wait flow: on any finalization or push
failure, rejected trigger, timeout, or stale/older reported revision, keep
live-event processing paused and do not resume until pending intent is durably
present in the remote state and the expected orchestrator revision has been
applied.
In `@internal/git/render_fidelity_gate.go`:
- Around line 195-221: Update reduceRenderFidelity so it evaluates every scope
and counts all finished clean scopes before returning a divergence status.
Preserve the existing sorted traversal and divergence details, but defer the
RenderFidelityFalse return until the loop completes, retaining the first
divergence for the returned status and accurate CleanScopes.
---
Nitpick comments:
In `@internal/controller/gittarget_controller.go`:
- Around line 205-207: Replace the magic render-fidelity state strings with the
exported git.RenderFidelityState constants: use git.RenderFidelityTrue and
git.RenderFidelityUnknown in internal/controller/gittarget_controller.go at
lines 205-207 and 221-224, and use git.RenderFidelityTrue,
git.RenderFidelityUnknown, and git.RenderFidelityFalse in
internal/controller/gittarget_status_test.go at lines 128, 159-161, and 165-167
respectively.
In `@internal/controller/gittarget_status_test.go`:
- Line 153: Rename TestDeriveGitTargetDataPlaneStatusWithRenderFidelity to
follow the TestFunctionName_Scenario convention, using an all-states scenario
name. Refactor its sequential assertions into a table-driven test that maps each
RenderFidelityState to the expected status outcomes.
In `@internal/manifestanalyzer/render_fidelity_test.go`:
- Line 93: Update the return statement in the surrounding helper to return
object directly, removing the redundant unstructured.Unstructured wrapper and
immediate .Object access while preserving the same map reference.
In `@internal/watch/event_router.go`:
- Around line 225-227: Update both logging sites in
internal/watch/event_router.go: in the per-type applied log at lines 225-227 and
the render-vs-live divergence log at lines 259-260, use static primary messages
and add the dynamic kind value as a structured "kind" field alongside the
existing fields.
🪄 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: ce97c339-ca90-4161-b714-7228210b1a6e
📒 Files selected for processing (64)
.coverage-baselineapi/v1alpha3/gittarget_types.goconfig/crd/bases/configbutler.ai_gittargets.yamldocs/design/support-boundary/README.mddocs/design/support-boundary/next-prompt-render-matches-live-gate.mddocs/design/support-boundary/orchestrator-reconcile-trigger.mddocs/design/support-boundary/render-fidelity-scenarios.mddocs/design/support-boundary/render-fidelity.mddocs/facts/kustomize-never-emits-dollar-brace.mdinternal/controller/constants.gointernal/controller/gittarget_controller.gointernal/controller/gittarget_status_test.gointernal/controller/stream_status.gointernal/git/branch_worker.gointernal/git/git_path_refusal.gointernal/git/plan_flush.gointernal/git/render_fidelity_gate.gointernal/git/render_fidelity_gate_test.gointernal/git/render_fidelity_test.gointernal/git/worker_manager.gointernal/manifestanalyzer/acceptance.gointernal/manifestanalyzer/analyzer.gointernal/manifestanalyzer/analyzer_test.gointernal/manifestanalyzer/render_fidelity.gointernal/manifestanalyzer/render_fidelity_test.gointernal/manifestanalyzer/testdata/render-fidelity/comment-only-token/git/configmap.yamlinternal/manifestanalyzer/testdata/render-fidelity/comment-only-token/live.yamlinternal/manifestanalyzer/testdata/render-fidelity/comment-only-token/want.yamlinternal/manifestanalyzer/testdata/render-fidelity/label-injects-render-token/git/configmap.yamlinternal/manifestanalyzer/testdata/render-fidelity/label-injects-render-token/git/kustomization.yamlinternal/manifestanalyzer/testdata/render-fidelity/label-injects-render-token/live.yamlinternal/manifestanalyzer/testdata/render-fidelity/label-injects-render-token/want.yamlinternal/manifestanalyzer/testdata/render-fidelity/label-overwrites-source-token/git/configmap.yamlinternal/manifestanalyzer/testdata/render-fidelity/label-overwrites-source-token/git/kustomization.yamlinternal/manifestanalyzer/testdata/render-fidelity/label-overwrites-source-token/live.yamlinternal/manifestanalyzer/testdata/render-fidelity/label-overwrites-source-token/want.yamlinternal/manifestanalyzer/testdata/render-fidelity/literal-crd-description/git/crd.yamlinternal/manifestanalyzer/testdata/render-fidelity/literal-crd-description/live.yamlinternal/manifestanalyzer/testdata/render-fidelity/literal-crd-description/want.yamlinternal/manifestanalyzer/testdata/render-fidelity/literal-kro-template/git/resourcegraphdefinition.yamlinternal/manifestanalyzer/testdata/render-fidelity/literal-kro-template/live.yamlinternal/manifestanalyzer/testdata/render-fidelity/literal-kro-template/want.yamlinternal/manifestanalyzer/testdata/render-fidelity/literal-nginx-config/git/configmap.yamlinternal/manifestanalyzer/testdata/render-fidelity/literal-nginx-config/live.yamlinternal/manifestanalyzer/testdata/render-fidelity/literal-nginx-config/want.yamlinternal/manifestanalyzer/testdata/render-fidelity/native-dollar-paren/git/configmap.yamlinternal/manifestanalyzer/testdata/render-fidelity/native-dollar-paren/live.yamlinternal/manifestanalyzer/testdata/render-fidelity/native-dollar-paren/want.yamlinternal/manifestanalyzer/testdata/render-fidelity/nested-list-token/git/deployment.yamlinternal/manifestanalyzer/testdata/render-fidelity/nested-list-token/live.yamlinternal/manifestanalyzer/testdata/render-fidelity/nested-list-token/want.yamlinternal/manifestanalyzer/testdata/render-fidelity/plain-postbuild-token/git/deployment.yamlinternal/manifestanalyzer/testdata/render-fidelity/plain-postbuild-token/live.yamlinternal/manifestanalyzer/testdata/render-fidelity/plain-postbuild-token/want.yamlinternal/manifestanalyzer/testdata/render-fidelity/token-field-removed-live/git/configmap.yamlinternal/manifestanalyzer/testdata/render-fidelity/token-field-removed-live/live.yamlinternal/manifestanalyzer/testdata/render-fidelity/token-field-removed-live/want.yamlinternal/watch/event_router.gointernal/watch/event_router_test.gointernal/watch/git_path_acceptance.gointernal/watch/git_path_acceptance_test.gointernal/watch/manager.gointernal/watch/render_fidelity_gate.gointernal/watch/target_watch.go
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/manifestanalyzer/analyzer_test.go
- docs/design/support-boundary/README.md
Comment on lines
+194
to
+195
| Note over Rev: detects the remote moved —<br/>holds live events back, replaying-gated FIFO | ||
| Rev->>Git: land pending intent — finalize the open window,<br/>push pendingWrites rebased onto the new tip<br/>(replay — a no-op if the drift already matches) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Require render-fidelity before treating the apply as a no-op.
This barrier assumes the orchestrator’s apply matches the local render, but Flux/Argo can add images, replicas, patches, namespaces, labels, suffixes, components, or substitutions unavailable locally. Those values could be misclassified as user edits and projected into source templates. Require an effective-render match or a named refusal such as TransformedOutOfBand before resuming processing.
🤖 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/support-boundary/orchestrator-reconcile-trigger.md` around lines
194 - 195, Update the orchestrator apply barrier around the Rev→Git “land
pending intent” step to require effective-render fidelity before treating drift
as a no-op. Resume processing only when the applied result matches the local
render; otherwise return a named refusal such as TransformedOutOfBand,
preventing externally injected Flux/Argo transformations from being projected
into source templates.
Comment on lines
+221
to
+229
| operator may hold **not-yet-remote** intent in two places: the **open commit window** (`openWindow` — | ||
| events coalesced in memory but not yet committed) and the **committed-but-unpushed queue** | ||
| (`pendingWrites`). Triggering the orchestrator reconcile *now* would apply the new origin over those | ||
| live edits on the cluster, erasing them before they reach Git. | ||
|
|
||
| So the ordering must be: **land pending intent to Git first, *then* trigger the reconcile, *then* | ||
| resume.** "Landing" it is the ordinary finalize-then-push — the open window is finalized into a commit, | ||
| joins `pendingWrites`, and the whole queue is pushed, rebased onto the new tip by the replay above (and | ||
| a no-op if the drift already matches). This is only safe because the intent is durably recoverable: the |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Define fail-closed behavior for barrier failures.
Specify what happens when finalization or push fails, the trigger is rejected, or the orchestrator times out or reports an older revision. Live-event processing must remain paused until pending intent is durably represented in the remote state and the expected orchestrator revision is applied; otherwise the stale-baseline and reconcile-echo hazards described here can reappear.
🤖 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/support-boundary/orchestrator-reconcile-trigger.md` around lines
221 - 229, Define fail-closed handling for the finalize-and-push, orchestrator
trigger, and reconcile-wait flow: on any finalization or push failure, rejected
trigger, timeout, or stale/older reported revision, keep live-event processing
paused and do not resume until pending intent is durably present in the remote
state and the expected orchestrator revision has been applied.
Comment on lines
+195
to
+221
| func reduceRenderFidelity(state renderFidelityTargetState) RenderFidelityStatus { | ||
| keys := make([]string, 0, len(state.scopes)) | ||
| for key := range state.scopes { | ||
| keys = append(keys, key) | ||
| } | ||
| sort.Strings(keys) | ||
| clean := 0 | ||
| for _, key := range keys { | ||
| result := state.scopes[key] | ||
| if !result.finished { | ||
| continue | ||
| } | ||
| if result.divergence != nil { | ||
| sample := *result.divergence | ||
| return RenderFidelityStatus{ | ||
| Epoch: state.epoch, | ||
| State: RenderFidelityFalse, | ||
| Reason: "RenderDoesNotMatchLive", | ||
| Message: "Rendered token " + sample.Token + " at " + sample.Field + " does not match live", | ||
| Divergence: &sample, | ||
| ScopeCount: len(state.scopes), CleanScopes: clean, | ||
| } | ||
| } | ||
| if result.clean { | ||
| clean++ | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Fix CleanScopes undercounting on divergence.
When a divergence is found, the loop exits early, causing CleanScopes to only reflect the number of clean scopes that happened to sort alphabetically before the divergent one, rather than the true count of evaluated clean scopes.
Evaluate all scopes to ensure an accurate count before returning.
🐛 Proposed fix
func reduceRenderFidelity(state renderFidelityTargetState) RenderFidelityStatus {
keys := make([]string, 0, len(state.scopes))
for key := range state.scopes {
keys = append(keys, key)
}
sort.Strings(keys)
clean := 0
+ var firstDivergence *manifestanalyzer.RenderDivergence
for _, key := range keys {
result := state.scopes[key]
if !result.finished {
continue
}
- if result.divergence != nil {
- sample := *result.divergence
- return RenderFidelityStatus{
- Epoch: state.epoch,
- State: RenderFidelityFalse,
- Reason: "RenderDoesNotMatchLive",
- Message: "Rendered token " + sample.Token + " at " + sample.Field + " does not match live",
- Divergence: &sample,
- ScopeCount: len(state.scopes), CleanScopes: clean,
- }
- }
+ if result.divergence != nil && firstDivergence == nil {
+ firstDivergence = result.divergence
+ }
if result.clean {
clean++
}
}
+ if firstDivergence != nil {
+ sample := *firstDivergence
+ return RenderFidelityStatus{
+ Epoch: state.epoch,
+ State: RenderFidelityFalse,
+ Reason: "RenderDoesNotMatchLive",
+ Message: "Rendered token " + sample.Token + " at " + sample.Field + " does not match live",
+ Divergence: &sample,
+ ScopeCount: len(state.scopes), CleanScopes: clean,
+ }
+ }
if clean != len(state.scopes) {📝 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.
Suggested change
| func reduceRenderFidelity(state renderFidelityTargetState) RenderFidelityStatus { | |
| keys := make([]string, 0, len(state.scopes)) | |
| for key := range state.scopes { | |
| keys = append(keys, key) | |
| } | |
| sort.Strings(keys) | |
| clean := 0 | |
| for _, key := range keys { | |
| result := state.scopes[key] | |
| if !result.finished { | |
| continue | |
| } | |
| if result.divergence != nil { | |
| sample := *result.divergence | |
| return RenderFidelityStatus{ | |
| Epoch: state.epoch, | |
| State: RenderFidelityFalse, | |
| Reason: "RenderDoesNotMatchLive", | |
| Message: "Rendered token " + sample.Token + " at " + sample.Field + " does not match live", | |
| Divergence: &sample, | |
| ScopeCount: len(state.scopes), CleanScopes: clean, | |
| } | |
| } | |
| if result.clean { | |
| clean++ | |
| } | |
| } | |
| sort.Strings(keys) | |
| clean := 0 | |
| var firstDivergence *manifestanalyzer.RenderDivergence | |
| for _, key := range keys { | |
| result := state.scopes[key] | |
| if !result.finished { | |
| continue | |
| } | |
| if result.divergence != nil && firstDivergence == nil { | |
| firstDivergence = result.divergence | |
| } | |
| if result.clean { | |
| clean++ | |
| } | |
| } | |
| if firstDivergence != nil { | |
| sample := *firstDivergence | |
| return RenderFidelityStatus{ | |
| Epoch: state.epoch, | |
| State: RenderFidelityFalse, | |
| Reason: "RenderDoesNotMatchLive", | |
| Message: "Rendered token " + sample.Token + " at " + sample.Field + " does not match live", | |
| Divergence: &sample, | |
| ScopeCount: len(state.scopes), CleanScopes: clean, | |
| } | |
| } |
🤖 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/render_fidelity_gate.go` around lines 195 - 221, Update
reduceRenderFidelity so it evaluates every scope and counts all finished clean
scopes before returning a divergence status. Preserve the existing sorted
traversal and divergence details, but defer the RenderFidelityFalse return until
the loop completes, retaining the first divergence for the returned status and
accurate CleanScopes.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The bug, measured
The writer mirrors a live object into the file that produced it. Under kustomize that file is not what the cluster runs — the build stands between them — so mirroring the live object straight back writes the build's own output into the build's input.
This is not hypothetical, and it is not about patches. It is live today, in a folder we accept today:
Nothing changed in the cluster. Nothing changed in the render. Every reconcile of an in-sync folder produced a commit, and the file was left wrong — delete the kustomization later and the injected values are now yours forever. In a base shared by two overlays, the value baked in is one environment's.
render-root-scoping.md§2 predicted exactly this ("the metadata-transformer leak, live today, in supported folders"). Nobody had run it.The fix
One rule, and it models no transformer:
Agreement means the build already produces exactly what the cluster runs, so the source is — by construction — the thing that produced it. Keeping it is not a heuristic; changing it would be writing a value we did not get from the user. Disagreement is the user's edit: routed to an
images:/replicas:entry when the dye says one supplies the field, written through to the source otherwise — where, if the build owns the field, the re-render refuses the flush.Because it needs no model of
commonLabels, ofnamespace, or of a patch, it closes all of them at once.Why this is the gate on
patches:I started on the
patches:milestone and found this in front of it. A patched base absorbs the overlay's values in exactly the same way — and no re-render can catch it: the patch re-imposes its own value, so the render comes out identical either way and the oracle passes.VerifyBatchRendersguards the render; only this guards the source.Tolerating
patches:lands next, on top of this.Behavior changes
images:/replicas:entries used to be committed and silently never converge (the transformer overrode it right back, forever). It is now a reported refusal —WriteBoundaryRefused, naming the file and the object.unplaceable-edit): the build and the user have both rewritten one list whose elements carry no uniquename:to pair them by. Pairing by position is not a conservative guess, it is measurably wrong — kustomize prepends a container a patch adds, so the source's element 0 is the render's element 1.Both are in
docs/UPGRADING.md.Measured, not reasoned
Every fact here came from asking kustomize with a throwaway probe, per the workstream's own rule:
labels:/commonAnnotations:on an in-sync folderimages:/replicas:ordering[PatchTransformer, ReplicaCountTransformer, ImageTagTransformer]— the transformers win, so a patch that sets a governed field is dead textThe last one is now a permanent fixture (
TestRender_ATransformerOverridesAPatchOnTheSameField) — it is the fact a future refactor will break.The net
TestProjection_InSyncCorpusFolderIsANoOpnow compares the whole document, not just its image slots — which is how a projection that quietly rewrote every field we had not modelled passed it for as long as it did. It went from 8 documents checked to 53, across every fixture in both corpora.Line delta, honestly
+430 source (
source_form.gois 250 of it), +410 test. It is not a small change for a projection that used to be "copy the live object". But the thing it replaces was not simpler, it was just wrong in a way that only showed up in files nobody diffed.Validation
task lint·task test(coverage 75.6% → 75.7%) ·task test-e2e(55/55) all pass.🤖 Generated with Claude Code
Summary by CodeRabbit
labels,commonLabels,commonAnnotations, andnamespacefrom being written back into source when they already match the live object.${...}token mismatches and ambiguous list placement.RenderMatchesLiveGitTarget condition (includingkubectltable column) to reflect render-vs-live fidelity and block normal writes while not established.