feat(git): write-boundary L1/L2 preconditions, WriteBoundaryRefused status, and go 1.26.5 refresh#216
Merged
Merged
Conversation
…dits (Track 1) Records the Track-1 design decisions: the two-layer write boundary (L1 write-scope jail + L2 fan-in=1), the GitTarget granularity fork (Option A for launch, C kept open for shared-defaults editing, B rejected as the operator write model), and cross-environment editing (product promotion now, base-as- variant later and only for shared defaults). Includes the who-renders split for floating/external sources and mermaid overviews + action-to-output examples. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ditions Make the two write-boundary invariants explicit and tested rather than emergent, before any byte is written (Track 1 prerequisite for F2): - L1 (pathScopePrecondition): every planned write stays inside the GitTarget write scope; an absolute or ".."-escaping path refuses the flush (IssueWriteEscapesScope). Reads may reach shared context (../../base); writes never leave the scope. Defense-in-depth alongside ignoreShadowPrecondition. - L2 (fanInPrecondition): a planned in-place edit of a source file more than one kustomize render path reaches with override entries at stake (the diamond hole) is now refused (IssueWriteFanIn) instead of falling back to write- through. It fires only on an actual planned write, so the legitimate base- sharing layout (base doc is NamespaceNone, never dirty) is not refused; F2 render-root scoping generalizes the check. Store-side signal via ManifestStore.OverridesAmbiguousAt, derived from build-time diagnostics. Both surface as the umbrella UnsupportedContent GitTarget reason (message names the path + cause). Unit + end-to-end tests; unit coverage 75.2% -> 75.3%. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…2/GO-2026-5856
- golang.org/x/crypto v0.53.0 -> v0.54.0 drops the unmaintained openpgp package
from the module graph, clearing GO-2026-5932. govulncheck and `go mod why`
confirm our code neither imports nor calls it (go-git already uses the
ProtonMail/go-crypto fork).
- go 1.26.4 -> 1.26.5 (go.mod, Dockerfile, .devcontainer/Dockerfile,
test/mutationlab/Dockerfile; base-image digests re-pinned to the 1.26.5
index) fixes the reachable crypto/tls ECH privacy leak GO-2026-5856.
- k8s.io/utils and transitive golang.org/x/{mod,sync,sys,term,text,tools}
refreshed by go mod tidy.
govulncheck: 0 vulnerabilities affecting our code. lint/vet/unit tests green
under go1.26.5.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughAdds write-boundary refusal kinds and enforcement, wires refusal reporting into GitTarget status updates, updates related design docs, and bumps the Go 1.26.5 toolchain and pinned images. ChangesWrite-boundary enforcement and refusal reporting
GitTarget and write-boundary design docs
Go 1.26.5 toolchain bump
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant BranchWorker
participant WatchManager
participant GitTargetController
BranchWorker->>WatchManager: ReportGitPathRefusal(...)
WatchManager->>GitTargetController: MarkTargetGitPathRefused(...)
GitTargetController-->>WatchManager: status reason updated
WatchManager-->>BranchWorker: refusal recorded
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/git/plan_flush.go (1)
935-940: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winNormalize the planned path before the fan-in lookup.
writePathEscapesScopeallows non-escaping paths likefoo/../bar, butfanInPreconditionchecks ambiguity using the rawrelwhile the later write path is normalized. If a non-canonical key can enterwb.buffers, the L2 refusal can miss an ambiguous target.Suggested hardening
func writePathEscapesScope(rel string) bool { if rel == "" || path.IsAbs(rel) { return true } clean := path.Clean(rel) return clean == ".." || strings.HasPrefix(clean, "../") } @@ - if wb.store.OverridesAmbiguousAt(rel) { + if wb.store.OverridesAmbiguousAt(path.Clean(rel)) {Please also verify that all
writeBatch.bufferskeys are clean/slash-normalized at construction time.Also applies to: 959-967
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/git/plan_flush.go` around lines 935 - 940, Normalize and validate the planned relative path before it reaches the fan-in ambiguity check in fanInPrecondition, since writePathEscapesScope and the later write path use a cleaned value while the lookup still uses the raw rel. Update the writeBatch construction path so wb.buffers keys are always clean and slash-normalized up front, and make fanInPrecondition use the same normalized form when checking for ambiguous targets. Keep the fix centered around writePathEscapesScope, fanInPrecondition, and writeBatch.buffers.
🤖 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.
Nitpick comments:
In `@internal/git/plan_flush.go`:
- Around line 935-940: Normalize and validate the planned relative path before
it reaches the fan-in ambiguity check in fanInPrecondition, since
writePathEscapesScope and the later write path use a cleaned value while the
lookup still uses the raw rel. Update the writeBatch construction path so
wb.buffers keys are always clean and slash-normalized up front, and make
fanInPrecondition use the same normalized form when checking for ambiguous
targets. Keep the fix centered around writePathEscapesScope, fanInPrecondition,
and writeBatch.buffers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 484a1758-be0a-46d3-80c1-a789fbed5e05
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (12)
.coverage-baseline.devcontainer/DockerfileDockerfiledocs/design/gitops-api/gittarget-granularity-and-cross-environment-edits.mdgo.modinternal/git/plan_flush.gointernal/git/write_boundary_precondition_test.gointernal/manifestanalyzer/acceptance.gointernal/manifestanalyzer/analyzer_test.gointernal/manifestanalyzer/overrides.gointernal/manifestanalyzer/overrides_test.gotest/mutationlab/Dockerfile
sunib
changed the title
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
…BoundaryRefused A write-plan refusal on a live-event path was logged and dropped. The resync path carries its refusal back on a result channel the router inspects, but a live commit window is finalized on a timer with no channel to return anything on, so an ambiguous write was correctly prevented while the GitTarget stayed healthy — the user's apply appeared to take effect and Git never moved. Both live paths were affected: the open window and the atomic request. Give the branch worker a PathRefusalReporter hook, installed by the watch manager, and call it from both paths when errors.As recovers an AcceptanceRefusedError. Every other error keeps its existing write-fault handling. Recovery stays with the resync path: a live write that happens to avoid the offending file proves nothing about the rest of the subtree. Name the two write-boundary refusals rather than hiding them behind the umbrella UnsupportedContent: a refusal composed purely of write-escapes-scope and/or write-fan-in issues now reports WriteBoundaryRefused, which is in the controller's stalled-reason set. UnsupportedContent means the folder holds content we cannot manage; WriteBoundaryRefused means the folder is fine and the edit had nowhere safe to land. AllIssuesOfKind becomes variadic AllIssuesOfKinds to express "all issues are among this set". Tests cover the path end to end, since the gap was that nothing connected the preconditions to the user-visible surface: a real diamond repo driven through the real event loop (reporter fires, branch tip unmoved), the watch mapping to GitPathAccepted=False, and the controller mapping to Stalled=True. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
sunib
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/git/write_boundary_precondition_test.go`:
- Around line 166-177: Update diamondGitTarget to use the production-valid
repository-root path convention instead of an empty string. The issue is in the
GitTargetSpec construction within diamondGitTarget, where Path is set to an
invalid empty value. Change the Path field to "." so the test matches the
documented GitTargetSpec.Path behavior and stays aligned with any future
validation in configv1alpha3.GitTargetSpec.
🪄 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: 41b17aaf-00c9-4445-a147-8802dc0ae7c7
📒 Files selected for processing (19)
cmd/main.godocs/design/gitops-api/README.mddocs/design/gitops-api/finished/f1-images-replicas-edit-through.mddocs/design/gitops-api/gittarget-granularity-and-cross-environment-edits.mddocs/design/gitops-api/kustomize-support-boundary-and-product-model.mdinternal/controller/gittarget_controller.gointernal/controller/gittarget_status_test.gointernal/controller/stream_status.gointernal/git/branch_worker.gointernal/git/git_path_refusal.gointernal/git/git_path_refusal_test.gointernal/git/worker_manager.gointernal/git/write_boundary_precondition_test.gointernal/manifestanalyzer/acceptance.gointernal/manifestanalyzer/acceptance_refusal.gointernal/watch/event_router.gointernal/watch/event_router_test.gointernal/watch/git_path_acceptance.gointernal/watch/git_path_acceptance_test.go
✅ Files skipped from review due to trivial changes (3)
- docs/design/gitops-api/finished/f1-images-replicas-edit-through.md
- internal/controller/gittarget_controller.go
- internal/controller/gittarget_status_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/manifestanalyzer/acceptance.go
Comment on lines
+166
to
+177
| // diamondGitTarget is the GitTarget the live-path test's events name, rooted at the repo root | ||
| // so the whole diamond is inside its write scope. | ||
| func diamondGitTarget(providerName, branch string) *configv1alpha3.GitTarget { | ||
| return &configv1alpha3.GitTarget{ | ||
| ObjectMeta: metav1.ObjectMeta{Name: "podinfo-test", Namespace: "default"}, | ||
| Spec: configv1alpha3.GitTargetSpec{ | ||
| ProviderRef: configv1alpha3.GitProviderReference{Name: providerName}, | ||
| Branch: branch, | ||
| Path: "", | ||
| }, | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use Path: "." instead of Path: "" in diamondGitTarget.
The GitTargetSpec.Path field documentation states an empty string is rejected (+kubebuilder:validation:MinLength=1), and "." is the documented convention for deliberately targeting the repository root. While the test bypasses API validation by constructing the object directly, using the production-valid value improves test fidelity and prevents breakage if code-level path validation is added later.
🔧 Proposed fix
func diamondGitTarget(providerName, branch string) *configv1alpha3.GitTarget {
return &configv1alpha3.GitTarget{
ObjectMeta: metav1.ObjectMeta{Name: "podinfo-test", Namespace: "default"},
Spec: configv1alpha3.GitTargetSpec{
ProviderRef: configv1alpha3.GitProviderReference{Name: providerName},
Branch: branch,
- Path: "",
+ Path: ".",
},
}
}📝 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
| // diamondGitTarget is the GitTarget the live-path test's events name, rooted at the repo root | |
| // so the whole diamond is inside its write scope. | |
| func diamondGitTarget(providerName, branch string) *configv1alpha3.GitTarget { | |
| return &configv1alpha3.GitTarget{ | |
| ObjectMeta: metav1.ObjectMeta{Name: "podinfo-test", Namespace: "default"}, | |
| Spec: configv1alpha3.GitTargetSpec{ | |
| ProviderRef: configv1alpha3.GitProviderReference{Name: providerName}, | |
| Branch: branch, | |
| Path: "", | |
| }, | |
| } | |
| } | |
| // diamondGitTarget is the GitTarget the live-path test's events name, rooted at the repo root | |
| // so the whole diamond is inside its write scope. | |
| func diamondGitTarget(providerName, branch string) *configv1alpha3.GitTarget { | |
| return &configv1alpha3.GitTarget{ | |
| ObjectMeta: metav1.ObjectMeta{Name: "podinfo-test", Namespace: "default"}, | |
| Spec: configv1alpha3.GitTargetSpec{ | |
| ProviderRef: configv1alpha3.GitProviderReference{Name: providerName}, | |
| Branch: branch, | |
| Path: ".", | |
| }, | |
| } | |
| } |
🤖 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/write_boundary_precondition_test.go` around lines 166 - 177,
Update diamondGitTarget to use the production-valid repository-root path
convention instead of an empty string. The issue is in the GitTargetSpec
construction within diamondGitTarget, where Path is set to an invalid empty
value. Change the Path field to "." so the test matches the documented
GitTargetSpec.Path behavior and stays aligned with any future validation in
configv1alpha3.GitTargetSpec.
This was referenced Jul 14, 2026
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.
Summary
Track-1 write-boundary hardening for F2 — the two invariants become explicit,
tested preconditions and reach the user as GitTarget status — plus a
go 1.26.5 / dependency refresh that clears the Scorecard vulnerability findings.
Write-boundary hardening
Design:
gittarget-granularity-and-cross-environment-edits.md§1.pathScopePrecondition) — every planned write stays insidespec.path; an escaping path refuses the flush (IssueWriteEscapesScope).Defense-in-depth: planned paths are base-relative by construction, but this is
the invariant "base is read-only" rests on under layout A, so it is asserted
rather than assumed.
fanInPrecondition) — the former ambiguous-override write-through isnow a refusal (
IssueWriteFanIn), firing only on an actual write into a filemore than one render path reaches (the diamond hole); the legitimate
base-sharing layout is left untouched for F2.
Both run before any byte is touched, so a violation aborts the whole flush and
commits nothing.
Refusals now reach GitTarget status
A refused write plan on a live-event path used to be logged and dropped. The
resync path carries its refusal back on a result channel the router inspects,
but a live commit window is finalized on a timer with no channel to return
anything on — so an ambiguous write was correctly prevented while the
GitTarget stayed healthy. That is the worse failure: the user's
kubectl applyappears to have taken effect and Git never moves. Both live paths were affected
(open window and atomic request).
git.PathRefusalReporterhook on the branch worker, installed by thewatch manager (
SetPathRefusalReporter, wired incmd/main.go). Both livepaths call it when
errors.Asrecovers anAcceptanceRefusedError; everyother error keeps its existing write-fault handling.
event-level metadata (
atomicRefusalTarget) —buildAtomicPendingWriteonlystamps request-level metadata onto events when the request carries it.
logged loudly and deliberately not recorded: the acceptance map is keyed by
namespace/name, so a half-empty reference would file under a key noGitTarget reads and every such refusal would collide there.
offending file proves nothing about the rest of the subtree.
New reason:
WriteBoundaryRefusedA refusal composed purely of
write-escapes-scopeand/orwrite-fan-inissues now reports
WriteBoundaryRefusedinstead of the umbrellaUnsupportedContent; any mix with content issues still falls back. Both are inthe controller's stalled-reason set, so either way the GitTarget surfaces as
GitPathAccepted=False/Stalled=True/ kstatusFailed.The distinction:
UnsupportedContentmeans this folder holds content we cannotmanage;
WriteBoundaryRefusedmeans the folder is fine, this edit had nowheresafe to land.
Tests
The gap was that nothing connected the preconditions to the user-visible
surface, so the coverage is end to end:
internal/git— a real diamond repo driven through the real event loop: thereporter fires with the right GitTarget, and the local branch tip is unmoved
(the refused write commits nothing). Verified load-bearing by neutering the
fix, where it fails. Plus unit coverage for target resolution, the
unattributable guard, and non-refusal pass-through.
internal/watch— the refusal becomesGitPathAccepted=Falsewith reasonWriteBoundaryRefused, plus a compile-time check that theManagermethodstill satisfies the hook signature so the
main.gowiring cannot drift.internal/controller— the reason carries through toStalled=True.Docs
Corrected, and sharpened on one distinction the docs were blurring — the hard
refusal that ships here is not the per-edit accounting that does not exist yet
(
FullyReflectedand the unreflected set appear in zero Go files):GitPathAccepted=False,Stalled=True,WriteBoundaryRefusedFullyReflectedaccountingkustomize-support-boundary-and-product-model.mdnow describe the refusal that shipped; the pre-F2/F4 prerequisite is marked done.
the ⚠ "emergent/buggy" callouts, and the §1 diagram (which claimed an
unreflected-set record and
FullyReflected=False) are corrected. The diagramnow states the true granularity: whole-flush abort, one target-level condition.
README.mdgains the write boundary under"What already works", and its launch-path paragraph no longer implies per-edit
reporting exists today.
is marked superseded where it documents the write-through fallback this
branch removed.
images:/replicas:entry is the L2-safe destination for an edit that wouldotherwise want to write
base/, that it silently shadows the base, and aproposal to notify on the override created transition (commit-message
trailer preferred over a target-scoped condition). Deferred to its own F-item.
rejected) and the who-renders split for floating sources.
Known blind spot (documented, not fixed)
fanInPreconditionkeys offreasonAmbiguousOverrides, so it catches a sharedfile reached by competing override chains. A file shared by two render roots
with no
images:/replicas:contention is not flagged — harmless under layoutA (such a doc is
NamespaceNone, never matches a live object, never dirty), butit is what must close before layout B could be offered. Generalizing to "any
file reachable from more than one render root" is F2 render-root scoping (§5).
Security refresh
x/cryptov0.53 → v0.54 drops the unmaintained openpgp package, clearingGO-2026-5932 (never reachable — go-git uses the ProtonMail fork).
GO-2026-5856; base-image digests re-pinned.
govulncheck: 0.Validation
task lint(0 issues) /task vet/task testgreen under go1.26.5; fulllocal
task test-e2epassed (55 passed, 0 failed, 8 skipped). Unit coverage75.3%, matching the committed
.coverage-baseline.🤖 Generated with Claude Code