feat(commit): let a GitTarget frame a save request's message - #364
Merged
sunib merged 3 commits intoSep 15, 2026
Merged
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (17)
📝 WalkthroughWalkthroughThe change adds optional ChangesRequest message framing
E2E fixture push retry
Estimated code review effort: 4 (Complex) | ~45 minutes Change: Feature Sequence Diagram(s)sequenceDiagram
participant CommitRequest
participant BranchWorker
participant requestTemplate
participant GitCommit
CommitRequest->>BranchWorker: provide literal message
BranchWorker->>requestTemplate: render with RequestMessage and live data
requestTemplate->>BranchWorker: return framed message or render error
BranchWorker->>GitCommit: commit framed message or verbatim fallback
✨ 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! |
A CommitRequest's message REPLACED the commit template rather than feeding it,
so supplying one cost the resource body liveTemplate would have produced. "Why I
saved" and "what was saved" were mutually exclusive: the commit said the reason
and stopped naming the resources.
The obvious fix is to template `CommitRequest.spec.message`, and it is the wrong
one. That spec is immutable, its bytes are committed verbatim, and nothing parses
them, which together are what make a request auditable against the commit it
produced. Templating it would also cross a trust boundary: a GitTarget is
operator-owned, while a CommitRequest is created by whoever holds create on it in
a namespace.
Decisively, it could not be validated. Commit templates run with
missingkey=error, and ValidateCommitConfig exists to render every one of them
against sample data at GitTarget admission precisely because "failing at
admission is the difference between a rejected GitTarget and a commit that dies
mid-window months later". A CommitRequest gets no such protection: a runtime
fault surfaces at finalize, where it costs the whole window (other authors'
retained events included), on an immutable object nobody can repair.
So it is inverted. The operator's template sees the request's message instead.
`GitTarget.spec.commit.message.requestTemplate` renders for a window a request
attached to, with the message arriving as `.RequestMessage` and committed
unaltered. Omit it and behaviour is byte-for-byte what it was.
Three properties make it safe rather than merely convenient:
- The request is still never parsed. Nothing a requester writes is executed;
they own the content, the operator owns the wording around it.
- A requestTemplate that never renders `.RequestMessage` is REJECTED. Without
this the feature reintroduces the exact fault it was designed around — a
target could frame the reason away entirely and the commit would still count
as request-sourced. The check renders the template and looks for a probe in
the OUTPUT rather than scanning the source, so `{{.RequestMessage}}`, a
pipeline, and a variable all pass for the right reason: the message reached
the commit.
- Both live templates are now validated against ONE shared set of window
shapes. Validating requestTemplate against a single sample was not enough and
the tests caught it: the labelled sample carries `team`, so a template
reading that label passed admission and would have fallen back at runtime for
every resource without it. They face identical windows in production;
checking one more thoroughly than the other only moves which template fails
late.
A requestTemplate that fails to render at finalize commits the request's message
verbatim rather than losing the window. That deviates from liveTemplate, where a
render failure fails the write, and the deviation is the point: here a correct,
non-lossy answer is always in hand because the literal already passed admission,
so failing would discard the requester's save and everyone else's retained events
to punish a formatting mistake. framedRequestMessage returns no error at all, and
cannot — "this failed" is not an outcome it can report.
That fallback is a SUCCESSFUL commit, so nothing is refused and no condition
moves, which would leave "the template works" and "the template silently stopped
applying" indistinguishable. git_commits_total therefore gains
`commit_request_framed` and `commit_request_fallback`; `commit_request` keeps its
meaning, so existing dashboards are unaffected. The source is stamped onto the
write at commit time the way CommitSHA already is, because publishCommitsForPush
runs after the push and recomputing it from the write cannot know a render
failed.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… framing
Review follow-ups, plus the flake the first e2e run exposed.
seedRenderedFolderIntoRepo pushed straight into a branch the controller is
already mirroring, with no retry. It reads the remote tip, builds a commit on it
and pushes under the remote's compare-and-swap, so a controller commit landing in
that window rejects it:
! [remote rejected] HEAD -> main (incorrect old value provided)
That is the test's own push failing, not the controller, and losing the race says
nothing about the behaviour under test. Each attempt now rebuilds the commit on
the new tip. The helper is shared by six spec files, so one unretried push was a
flake source for all of them; it surfaced as the unsupported-folder spec failing
locally while the same spec passed on CI's full-manager leg for the same commit.
The attempt returns errors rather than asserting: an Expect inside Eventually
would fail the spec on the first lost race, which is exactly what the retry is
there to survive. A retry landing on a tip that already carries the identical
content stages nothing, and `git commit` with nothing staged is an error — that
case is success, because the fixture is in Git, which is all the caller asked.
Three review points on the framing change itself:
- The requestTemplate arm now validates the literal message too, instead of
going straight to the render. The literal reaches Git on BOTH outcomes of
that arm (framed on success, bare on the fallback), so framing must not
become a route that smuggles a message past the check the verbatim arm
enforces. The controller validates earlier, so this is not reachable through
the normal flow today — it is the kind of invariant that stops holding the
moment a second producer of PendingWrite appears. Tested on both arms
together, so framing cannot change what is accepted.
- The CRD field description is back to the contract: when it renders, what
fields it gets, that the message is literal, and what omitting or dropping it
does. The rationale moves to a block above the doc comment, separated by the
blank line that keeps controller-gen from putting it in the schema
(AGENTS.md). Verified the way that section prescribes: regenerated to a
scratch dir and compared with every description stripped — zero structural
diffs, so no kubebuilder marker was displaced.
- docs/configuration.md listed RequestMessage among liveTemplate's fields,
which is misleading: it is always empty there, because liveTemplate only runs
for windows that have no request message. Moved to requestTemplate, with the
reason stated so nobody reaches for {{if .RequestMessage}} and waits for it
to fire.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
sunib
force-pushed
the
feat/commit-request-message-composition
branch
from
September 15, 2026 08:33
ae2252f to
bc88b0b
Compare
Contributor
Author
|
@coderabbitai review Auto-review was skipped here because this PR is stacked on |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '== PR metadata =='
gh pr view 364 --repo ConfigButler/gitops-reverser --json number,title,baseRefName,headRefName,commits,files --jq '{number,title,baseRefName,headRefName,commits: [.commits[] | {oid,messageHeadline}], files: [.files[] | {path,additions,deletions}]}'
echo '== Diff against PR base branch =='
git diff --stat origin/fix/watch-reconnect-severity...HEAD
git diff --name-only origin/fix/watch-reconnect-severity...HEAD
echo '== Changed Go symbols =='
fd -e go . | xargs -r ast-grep outline --items all --type function,method,struct,interface 2>/dev/null | head -n 400
echo '== Diff =='
git diff --find-renames --find-copies --unified=80 origin/fix/watch-reconnect-severity...HEADLength of output: 3022 🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="ConfigButler/gitops-reverser"
base_sha=$(gh pr view 364 --repo "$repo" --json baseRefOid --jq .baseRefOid)
head_sha=$(gh pr view 364 --repo "$repo" --json headRefOid --jq .headRefOid)
echo "base_sha=$base_sha"
echo "head_sha=$head_sha"
echo '== API comparison file list =='
gh api "repos/$repo/compare/$base_sha...$head_sha" --paginate --jq '.files[] | [.filename, .status, .additions, .deletions] | `@tsv`'
echo '== API comparison patch =='
gh api "repos/$repo/compare/$base_sha...$head_sha" --paginate --jq '.files[] | "\n===== \(.filename) =====\n\(.patch // "(binary or no patch)")"'
echo '== Go source outlines for changed implementation files =='
for f in api/v1alpha3/gitprovider_types.go internal/git/commit.go internal/git/commit_executor.go internal/git/open_window.go internal/git/pending_writes.go internal/git/resync_flush.go internal/git/types.go internal/git/request_template_test.go test/e2e/inplace_edit_e2e_test.go; do
if [ -f "$f" ]; then
echo "===== $f ====="
ast-grep outline "$f" --items all --type function,method,struct,interface || true
fi
doneLength of output: 71139 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '== Request-template implementation =='
sed -n '100,245p' internal/git/commit.go
sed -n '60,285p' internal/git/commit_executor.go
sed -n '760,825p' internal/git/types.go
sed -n '1,115p' internal/git/pending_writes.go
echo '== Validation call sites and CommitMessage producers =='
rg -n -C 4 --type go 'ValidateCommitConfig\s*\(|CommitMessage:' api internal test -g '!**/*_test.go'
rg -n -C 4 --type go '\.CommitMessage\s*=|CommitMessage\s*:' internal -g '!**/*_test.go'
echo '== Commit publication and message source =='
rg -n -C 8 --type go '\bpublishCommitsForPush\s*\(|\bmessageSource\s*\(' internal/git
echo '== Relevant API validation =='
rg -n -C 8 --type go 'CommitMessageSpec|requestTemplate|ValidateCommit' api internal/controller* internal -g '!**/*_test.go' | head -n 500Length of output: 86264 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '== Commit modes and WriteRequest producers =='
rg -n -C 6 --type go 'CommitMode(?:\s*:|\s*=)|WriteRequest\s*\{' . -g '!**/*_test.go'
echo '== Atomic pending-write and enqueue routing =='
rg -n -C 10 --type go '\bbuildAtomicPendingWrite\s*\(|\bbuildGroupedPendingWrite\s*\(|\bEnqueueRequest\s*\(|\benqueueRequest\s*\(' internal -g '!**/*_test.go'
echo '== CommitRequest attachment and effective message =='
sed -n '1030,1120p' internal/git/branch_worker.go
rg -n -C 8 --type go 'effectiveMessage|pendingMessage|CommitRequest' internal/git/branch_worker.go internal/controller -g '!**/*_test.go'
echo '== Existing tests with atomic message overrides =='
rg -n -C 6 --type go 'PendingWriteAtomic|CommitModeAtomic|CommitMessage:' internal -g '*_test.go' | head -n 800Length of output: 143363
I found two blocking issues:
|
Two holes in the requestTemplate work, both reported by CodeRabbit on #364. The validation probe was a package constant, so the check could be satisfied without doing the thing it verifies: a template emitting that literal string would pass while never referencing .RequestMessage. Nobody writes that on purpose, but the whole argument for rejecting a message-dropping template was that the guarantee must hold by construction rather than by good intentions, and a spoofable sentinel does not carry that. The probe is now minted per validation from crypto/rand, so rendering the message is the only way to pass. Framing was bound to "carries a message" rather than to the kind. A CommitRequest attaches its message to an open live window, which finalizes as PendingWriteCommit — but an atomic snapshot can carry a message too (WriteRequest.CommitMessage), and it was being framed with a live context written for save requests, rerouting the documented atomic path through the wrong template. Not reachable in production today, since the only WriteRequest producer hardcodes an empty message and EnqueueRequest has no production callers; that is exactly the kind of invariant that stops holding the moment a second producer appears, which is the same reason the literal validator was added to this arm earlier. An atomic write with a message now keeps the behaviour it had before requestTemplate existed: committed verbatim. Tests cover both: a template faking either probe shape is rejected, successive probes differ, and atomic/resync writes resolve exactly as they did before. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Came out of the question "does
CommitRequest.spec.messagesupport templating, and should it?" — no, and no. But the question points at a real gap, and the fix runs the other way round.The gap
A save request's message replaces the commit template rather than feeding it, so supplying one costs the resource body
liveTemplatewould have produced. "Why I saved" and "what was saved" are mutually exclusive today: the commit states the reason and stops naming the resources.Why not just template the request
Three reasons, strongest last.
Wrong trust boundary.
liveTemplate/reconcileTemplatelive on the GitTarget, which an operator owns, and describe house style for the org's Git history. ACommitRequestis created by whoever holdscreateon it in a namespace — the save-button population, deliberately much wider. That set supplying message content is the design; that set defining formatting policy is not.It breaks the literal contract, which exists for auditability. The spec is immutable (
self == oldSelf), validation never truncates, surrounding spaces are preserved. Every one of those says the same thing: what you typed is what lands in Git, so the object and the commit agree byte for byte.Decisively: it could not be validated. Commit templates run with
missingkey=error, andValidateCommitConfigexists to render every template against sample data at GitTarget admission precisely because:A
CommitRequestcannot be given that. Syntax could be checked at admission, but a runtime fault surfaces at finalize — and there the blast radius is not the request, it is the whole window, including other authors' retained events. The spec is immutable, so nobody can repair the request that killed it.The inversion
Don't template the request. Let the operator's template see the request's message.
requestTemplatesetrequestTemplate, with.RequestMessagerequestTemplateliveTemplatereconcileTemplateFully backward compatible: omit
requestTemplateand nothing moves.Why an explicit field rather than just exposing
.RequestMessageThe cheaper-looking alternative is to add
.RequestMessageto the live context, add no API field, and let people write{{if .RequestMessage}}…{{end}}insideliveTemplate.It doesn't work. Today a request message skips the template entirely, so making
liveTemplatesee it means no longer skipping — and then any existing target whoseliveTemplatedoesn't mention.RequestMessagewould silently drop the user's save message. That is a worse regression than the gap being closed.requestTemplatebeing set is the opt-in signal, and it is self-documenting.A
requestTemplatethat drops the message is rejectedWithout this rule the design reintroduces the exact fault the alternative above was rejected for, moved one level up: a target could set
requestTemplate: "chore: sync {{.Count}} resources"and every save message would silently vanish while the commit still counted as request-sourced.The check renders the template and looks for a probe in the output, rather than scanning the source. A scan for the literal
{{.RequestMessage}}would reject{{.RequestMessage | printf "%s"}}, a variable assignment, and every other legitimate spelling; the probe accepts all of them for the right reason — the message reached the commit. Covered in both directions by tests.Residual, stated plainly: sample execution cannot prove every branch, so a template that drops the message only under
{{if eq .Count 1}}still passes. That is the caveatdocs/configuration.mdalready documents for the existing templates, now extended to this one.Both live templates share one sample set
liveTemplateandrequestTemplateare validated against the same window shapes.This was not the first implementation, and the tests caught it: validating
requestTemplateagainst a single sample was not enough, becausesampleLabeledObject()carries ateamlabel. A template reading.Labels.teampassed admission and would have fallen back at runtime for every resource lacking it — exactly the failure admission exists to prevent. They face identical windows in production; checking one more thoroughly than the other only changes which template fails late.Render failure falls back, and is counted
A
requestTemplatethat fails at finalize commits the request's message verbatim rather than losing the window. This deviates fromliveTemplate, where a render failure fails the write, and the deviation is the point: a correct, non-lossy answer is always in hand here because the literal already passedValidateLiteralCommitMessageat admission. Failing would discard the requester's save and everyone else's retained events to punish a formatting mistake.framedRequestMessagereturns no error at all, and cannot — "this failed" is not an outcome it can report. (unparampointed this out; the signature now states it rather than the linter being suppressed.)A fallback is a successful commit, so nothing is refused and no condition moves. Without a counter, "the template works" and "the template silently stopped applying" read identically. So
git_commits_totalgains twomessage_sourcevalues:commit_requestrequestTemplateconfigured. Unchanged, so existing dashboards keep their meaning.commit_request_framedrequestTemplaterendered.commit_request_fallbackrequestTemplatefailed; the literal was committed.rate(…fallback) > 0is the alert andframed / (framed + fallback)the health ratio, both indocs/interpreting-metrics.md.This splits from
commit_requesteven though the text originated with the request either way. The existingmessageResolution.label()comment groups by how text is produced (which is why resync and atomic sharereconcile) — but a fallback is not a production detail, it is the operator's configured policy not having applied, which is a different question and one an operator actually asks.The source is stamped onto the write at commit time the same way
CommitSHAalready is, becausepublishCommitsForPushruns after the push and recomputing the source from the write cannot know a render failed earlier.Considered and not done: a GitTarget condition
The seam exists (
commitFailureRefusedis surfaced as a condition). Left out on proportionality: the marker check plus the shared sample renders now catch syntax faults,missingkeyfaults, and the drop-the-message mistake at admission, where the operator is already looking. What survives to runtime is a narrow residual, mirroring is unaffected, and commits keep flowing. Ifcommit_request_fallbackever shows real traffic, that is the evidence for promoting it — noted in the code rather than left as a decision made once and forgotten.Incidental finding
CommitMessageSpec's doc comment claims the "identically-shapedGitProvider.spec.commit.messageis retained only to reject a manifest that still sets it there". There is no such field —CommitMessageSpecis embedded only byGitTargetCommitSpec, andGitProvider.spec.commitcarries justCommitterandSigning. The generated CRD diff confirms it:requestTemplatelands ongittargetsonly. Left alone as out of scope; flagged so the next reader doesn't trust the comment.Testing
internal/git/request_template_test.go, plus signature updates to existing callers:requestTemplatecommits the literal byte for byte, leading and trailing spaces included — the test that guards the CommitRequest contract.commit_request_fallback.messageSourcekeeps plain / framed / fallback distinct, including the commit-time stamp.requestTemplateoutranks the verbatim arm; a window with no request message is untouched.requestTemplatehas no built-in default, and the overlay trims like the others.Full gate green:
fmt/generate/manifests/vet/lint/test(coverage holds at the 78.0% baseline) /test-e2e(85 passed, 0 failed).Flake found and fixed along the way
The first local e2e run failed
Manager Unsupported Folder Refusalatunsupported_folder_e2e_test.go:108, inside the sharedseedRenderedFolderIntoRepohelper:That is the test's own push, rejected by compare-and-swap. The spec brings the GitTarget to healthy and actively mirroring, then pushes straight into the same branch via a helper that does
fetch→reset --hard→commit→pushwith no retry. Any controller commit landing in that window rejects the seed push.Confirmed pre-existing, not caused by this change, by CI rather than by argument: the
full-managerleg — which runslabel: "manager", exactly this spec's label — passed ond6d84aa8, the feature commit without the race fix. Same spec, same code, clean cluster. The supporting reasoning also holds:requestTemplateappears nowhere in the e2e suite or fixtures, soresolveMessage()takes an identical arm for every e2e commit and messages are byte-identical.Fixed rather than left, because the helper is shared by six spec files, so one unretried push was a latent flake source for all of them. Each attempt now rebuilds the commit on the new tip. Two details:
ExpectinsideEventuallywould fail the spec on the first lost race, which is precisely what the retry exists to survive.git commitwith nothing staged is an error. That case is success: the fixture is in Git, which is all the caller asked for.Review round (second commit)
ValidateLiteralCommitMessage. The literal reaches Git on both outcomes of that arm — framed on success, bare on the fallback — so framing must not become a route that smuggles a message past the check the verbatim arm enforces. Not reachable through the normal flow (the controller validates earlier); it is the invariant that stops holding once a second producer ofPendingWriteexists. Tested on both arms together, so framing cannot change what is accepted.AGENTS.md. Verified as that section prescribes — regenerated to a scratch dir and compared with everydescriptionstripped: zero structural diffs, so no+kubebuilder:marker was displaced.RequestMessageremoved from theliveTemplatefield list. It is always empty there, becauseliveTemplateonly runs for windows with no request message. The docs now say so, so nobody reaches for{{if .RequestMessage}}and waits for it to fire.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Metrics
Documentation
Bug Fixes