You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
Commit 65abf54
Browse filesBrowse the repository at this point in the historyBrowse files
feat(commit): let a GitTarget frame a save request's message (#364)
* feat(commit): let a GitTarget frame a save request's message
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>
* fix(e2e): retry a seed push that loses to the controller, and tighten 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>
* fix(commit): bind framing to the grouped kind, and mint a fresh probe
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>
---------
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
0 commit comments