From 9b7730e92d3e2160cbb6e83b927712a4f988c4a7 Mon Sep 17 00:00:00 2001 From: Simon Koudijs Date: Tue, 15 Sep 2026 08:03:51 +0000 Subject: [PATCH 1/3] feat(commit): let a GitTarget frame a save request's message MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- api/v1alpha3/gitprovider_types.go | 13 ++ .../crd/bases/configbutler.ai_gittargets.yaml | 13 ++ docs/UPGRADING.md | 29 +++ docs/configuration.md | 50 ++++- docs/interpreting-metrics.md | 17 ++ docs/spec/commitrequest-design.md | 9 +- internal/git/commit.go | 93 ++++++++- internal/git/commit_executor.go | 93 +++++++-- internal/git/commit_executor_test.go | 14 +- internal/git/commit_metadata_fields_test.go | 14 +- internal/git/literal_message_test.go | 4 +- internal/git/open_window.go | 20 +- internal/git/pending_writes.go | 12 ++ internal/git/request_template_test.go | 190 ++++++++++++++++++ internal/git/resync_flush.go | 2 +- internal/git/types.go | 23 +++ 16 files changed, 543 insertions(+), 53 deletions(-) create mode 100644 internal/git/request_template_test.go diff --git a/api/v1alpha3/gitprovider_types.go b/api/v1alpha3/gitprovider_types.go index 68cdc9d1..4c807bc1 100644 --- a/api/v1alpha3/gitprovider_types.go +++ b/api/v1alpha3/gitprovider_types.go @@ -184,6 +184,19 @@ type CommitMessageSpec struct { // +optional LiveTemplate string `json:"liveTemplate,omitempty"` + // RequestTemplate optionally frames a CommitRequest's message instead of committing it + // verbatim. It renders only for a window a request's message is attached to, and receives the + // live fields plus RequestMessage, which carries that message unaltered. + // + // Omit it and a request's message is committed exactly as supplied, which is the default. + // Set it and the operator owns the wording around the message while the requester still owns + // the message itself: the request is never parsed as a template, so a save-button user cannot + // execute one. A template that never renders RequestMessage is REJECTED — framing the message + // is the whole point, and silently dropping a requester's stated reason is worse than having + // no template at all. + // +optional + RequestTemplate string `json:"requestTemplate,omitempty"` + // ReconcileTemplate formats atomic snapshots and resyncs. // Fields: Count, GitTarget, Group, Version, Resource, APIVersion, Namespace, Revision. // Type and Namespace fields are empty for whole-target snapshots. Revision can be empty. diff --git a/config/crd/bases/configbutler.ai_gittargets.yaml b/config/crd/bases/configbutler.ai_gittargets.yaml index 235f5f77..42e08e7f 100644 --- a/config/crd/bases/configbutler.ai_gittargets.yaml +++ b/config/crd/bases/configbutler.ai_gittargets.yaml @@ -172,6 +172,19 @@ spec: Type and Namespace fields are empty for whole-target snapshots. Revision can be empty. Guard optional fields so the message remains meaningful for every snapshot scope. type: string + requestTemplate: + description: |- + RequestTemplate optionally frames a CommitRequest's message instead of committing it + verbatim. It renders only for a window a request's message is attached to, and receives the + live fields plus RequestMessage, which carries that message unaltered. + + Omit it and a request's message is committed exactly as supplied, which is the default. + Set it and the operator owns the wording around the message while the requester still owns + the message itself: the request is never parsed as a template, so a save-button user cannot + execute one. A template that never renders RequestMessage is REJECTED — framing the message + is the whole point, and silently dropping a requester's stated reason is worse than having + no template at all. + type: string type: object x-kubernetes-validations: - message: eventTemplate is retired; migrate to liveTemplate diff --git a/docs/UPGRADING.md b/docs/UPGRADING.md index cefc9dbb..48bb5cb4 100644 --- a/docs/UPGRADING.md +++ b/docs/UPGRADING.md @@ -7,6 +7,35 @@ guidance that the changelog's breaking-change entries link to. We are pre-1.0, so breaking changes bump the **minor** version (release-please is configured with `bump-minor-pre-major`) rather than the major. Read the relevant entry before upgrading across it. +## Save messages can be framed by the GitTarget + +**Not breaking.** `GitTarget.spec.commit.message.requestTemplate` is new and optional; omit it and +`CommitRequest.spec.message` is committed verbatim exactly as before. + +Until now a save request's message REPLACED the commit template, so supplying one lost the resource +body `liveTemplate` would have produced. `requestTemplate` composes them: + +```yaml +spec: + commit: + message: + requestTemplate: |- + {{.RequestMessage}} + + {{range .Resources -}} + - [{{.Operation}}] {{.APIVersion}}/{{.Resource}}/{{.Namespace}}/{{.Name}} + {{end -}} +``` + +`CommitRequest.spec.message` stays literal and is still never parsed as a template — it arrives as +`.RequestMessage` and is committed unaltered. A `requestTemplate` that never renders it is rejected +with `Validated=False`. + +`git_commits_total` gains two `message_source` values, `commit_request_framed` and +`commit_request_fallback`. The existing `commit_request` keeps its meaning (a verbatim message with +no `requestTemplate` configured), so dashboards reading it are unaffected. Alert on the fallback +rate: it is a successful commit, so it is the only signal that a template has stopped applying. + ## Watch reconnects no longer report as failures **Not breaking, but two observable surfaces move.** Neither needs a manifest change; both may need diff --git a/docs/configuration.md b/docs/configuration.md index 9d021102..de377cf8 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -575,10 +575,13 @@ write rather than stopping the mirror. `reconcileTemplate` formats atomic snapshots and resyncs. Invalid templates report `Validated=False` with reason `InvalidConfig`; validation exercises singleton and mixed-operation windows, empty authors, and scoped and whole-target snapshots through the production renderer. -Sample execution cannot prove every possible conditional branch valid. +Sample execution cannot prove every possible conditional branch valid. That applies to +`requestTemplate`'s [message check](#framing-a-save-message) too: a template that drops the save +message only on some branch passes validation and falls back at commit time. | Input, in precedence order | Message source | |---|---| +| Attached [save request](#commitrequest) **and** `requestTemplate` set | `requestTemplate`, with the request's message as `.RequestMessage` | | Non-empty literal override, including an attached [save request](#commitrequest) | Exact supplied text | | Live window of any size | `liveTemplate` | | Atomic snapshot or resync | `reconcileTemplate` | @@ -600,7 +603,8 @@ spec: | Template | Fields | |---|---| -| `liveTemplate` | `Author`, `GitTarget`, `Count`, `Operations`, `Resources`, and the `LabelValues` / `LabelValue` accessors | +| `liveTemplate` | `Author`, `GitTarget`, `Count`, `Operations`, `Resources`, `RequestMessage`, and the `LabelValues` / `LabelValue` accessors | +| `requestTemplate` | the same fields, with `RequestMessage` carrying the save request's message | | Each `Resources` entry | `Operation`, `Group`, `Version`, `Resource`, `Kind`, `Namespace`, `Name`, `APIVersion`, `Labels`, and the `Label` accessor | | `reconcileTemplate` | `Count`, `GitTarget`, `Group`, `Version`, `Resource`, `APIVersion`, `Namespace`, `Revision` | @@ -615,6 +619,44 @@ message. Printing a resource entry directly keeps its `group/version/resource[/n display name or the `attribution-unresolved` Git author sentinel. The sentinel appears only in the Git author header when attribution ran without resolving an actor. Messages never change authorship. +##### Framing a save message + +By default a [save request](#commitrequest)'s message **replaces** the template, so supplying one +costs you the resource body `liveTemplate` would have produced: the commit says why, but no longer +says what. `requestTemplate` composes the two. + +```yaml +spec: + commit: + message: + requestTemplate: |- + {{.RequestMessage}} + + {{range .Resources -}} + - [{{.Operation}}] {{.APIVersion}}/{{.Resource}}/{{.Namespace}}/{{.Name}} + {{end -}} +``` + +It renders only for a window a save request attached to; every other window is unaffected. Omit it +and request messages are committed verbatim, exactly as before. + +The request is **never** parsed as a template. Its message arrives as `.RequestMessage` and is +committed unaltered, so a save-button user supplies the content while the operator owns the +wording around it. Nothing a requester writes is ever executed, and braces inside a request message +stay literal in every case. + +A `requestTemplate` that never renders `.RequestMessage` is **rejected** with `Validated=False`, +because dropping the requester's stated reason is the one thing this field must not do. Every +spelling that puts the message in the commit is accepted (`{{.RequestMessage}}`, a pipeline, or a +variable), because the check renders the template and looks for the message in the output rather +than scanning the template's text. + +`requestTemplate` is validated against the same window shapes as `liveTemplate`, so a template +reading a label some resources do not carry fails at admission rather than at commit time. If one +does fail to render in production, the request's message is committed verbatim instead of the +window being lost, and the commit is counted under `message_source="commit_request_fallback"`. +Alert on that rate: the commit itself succeeds and no condition moves, so it is the only signal. + ##### Kind, scope, and labels `Kind` is the commit-message spelling of the `{kind}` [placement variable](#template-variables), @@ -1614,7 +1656,9 @@ The entire spec is immutable. Create a new `CommitRequest` for each save attempt A present message accepts 1–1024 Unicode characters, including newline. All other ASCII control characters, including tab, carriage return, and DEL, are rejected, as is whitespace-only text. Accepted surrounding spaces are preserved. Braces such as `{{.Author}}` remain literal; omission -uses [the live template](#commit-message-templates). A rejected request leaves automatic mirroring +uses [the live template](#commit-message-templates). To frame a save message with what was saved, +configure [`requestTemplate`](#framing-a-save-message) on the GitTarget. The request is still never +parsed as a template. A rejected request leaves automatic mirroring available. The submitter chooses any semantic prefix; free-form messages are accepted. A request attaches to at most one matching open window. Normal flush triggers may close it early; diff --git a/docs/interpreting-metrics.md b/docs/interpreting-metrics.md index cbc6b737..42b84dbc 100644 --- a/docs/interpreting-metrics.md +++ b/docs/interpreting-metrics.md @@ -353,6 +353,23 @@ serve several GitTargets sharing a provider+branch, coalescing their writes into the worker is the honest attribution unit. `author_kind` is `user`, `serviceaccount`, `committer`, or `unresolved`; reconcile/resync commits and configured-author mode use `committer`. +`message_source` is `live`, `reconcile`, or one of three request values: `commit_request` (a save +request's message committed verbatim, no `requestTemplate` configured), `commit_request_framed` +(`requestTemplate` rendered), and `commit_request_fallback` (`requestTemplate` failed to render, so +the message was committed verbatim instead). + +The fallback is the one to alert on. It is a **successful** commit — nothing is refused and no +condition moves — so without this counter a `requestTemplate` that has quietly stopped applying +looks exactly like one that was never configured: + +```promql +sum by (provider_namespace, provider_name) ( + rate(gitopsreverser_git_commits_total{message_source="commit_request_fallback"}[15m]) +) > 0 +``` + +`framed / (framed + fallback)` is the health ratio for the feature. + **`unresolved` is the one to watch.** It means attribution RAN and did not name an actor, so the commit carries the `unknown (attribution unresolved)` author instead of a person. It is deliberately not folded into `user` (which would make a lost actor look like a named one, so a diff --git a/docs/spec/commitrequest-design.md b/docs/spec/commitrequest-design.md index 2111b7cc..30b8c7d6 100644 --- a/docs/spec/commitrequest-design.md +++ b/docs/spec/commitrequest-design.md @@ -31,8 +31,13 @@ at most one window and cannot rename a finalized commit, including one waiting f provides no ordering guarantee; use a non-zero window for custom save messages. The delay does not reserve a transaction. Competing requests keep the earliest-finalize-deadline selection policy. -`spec.message` is literal, including template-like text and surrounding spaces. Omission uses -`GitTarget.spec.commit.message.liveTemplate`. A present value accepts 1–1024 Unicode characters; +`spec.message` is literal, including template-like text and surrounding spaces. It is never parsed +as a template, so a request author cannot execute one. Omission uses +`GitTarget.spec.commit.message.liveTemplate`. A target may set +`GitTarget.spec.commit.message.requestTemplate` to frame the message with the window's resources; +the message still arrives unaltered, as `.RequestMessage`, and a template that does not render it +is rejected at admission, so the request's bytes always reach the commit. A `requestTemplate` that +fails to render commits the message verbatim rather than losing the window. A present value accepts 1–1024 Unicode characters; newline is allowed, other ASCII controls and whitespace-only text are rejected. Validation never truncates accepted text. A no-op still creates no commit. The message does not change Git identities. diff --git a/internal/git/commit.go b/internal/git/commit.go index b69f88f0..c8c154c3 100644 --- a/internal/git/commit.go +++ b/internal/git/commit.go @@ -4,6 +4,7 @@ package git import ( "bytes" + "errors" "fmt" "regexp" "strings" @@ -68,11 +69,20 @@ func renderReconcileCommitMessage( } func renderLiveCommitMessage(pendingWrite PendingWrite, config CommitConfig) (string, error) { - return renderCommitTemplate( - "live", - config.Message.LiveTemplate, - buildLiveCommitMessageData(pendingWrite.Author(), pendingWrite.Target().Name, pendingWrite.Events), - ) + return renderCommitTemplate("live", config.Message.LiveTemplate, pendingWrite.liveMessageData()) +} + +// renderRequestCommitMessage frames an attached CommitRequest's message with the target's +// requestTemplate. The literal rides in as .RequestMessage rather than being parsed, so nothing a +// requester wrote is ever executed. +func renderRequestCommitMessage(pendingWrite PendingWrite, config CommitConfig) (string, error) { + return renderCommitTemplate("request", config.Message.RequestTemplate, pendingWrite.liveMessageData()) +} + +// liveMessageData is the template context both live renders share. One builder, because a framed +// request commit is a live window that happens to carry a message — not a different kind of commit. +func (p PendingWrite) liveMessageData() LiveCommitMessageData { + return buildLiveCommitMessageData(p.Author(), p.Target().Name, p.CommitMessage, p.Events) } func renderCommitTemplate(name, text string, data any) (string, error) { @@ -139,6 +149,30 @@ func ValidateCommitConfig(config CommitConfig) error { // not carry "team", and failing at admission is the difference between a rejected GitTarget // and a commit that dies mid-window months later. "{{.Label \"team\"}}" renders empty and // passes both. + for _, events := range liveValidationSamples(sampleEvent) { + if _, err := renderLiveCommitMessage(PendingWrite{ + Kind: PendingWriteCommit, Events: events, + }, config); err != nil { + return err + } + if err := validateRequestTemplate(config, events); err != nil { + return err + } + } + + return nil +} + +// liveValidationSamples is the set of window shapes both live-message templates are validated +// against: growing windows, mixed operations, an empty author, and — the important half — a +// resource that carries an object with labels beside ones that carry neither, because these +// templates run with missingkey=error and it is the render WITHOUT the label that fails. +// +// It is shared so liveTemplate and requestTemplate cannot drift into being checked against +// different worlds. They face identical windows at runtime; checking one more thoroughly than the +// other just moves which template fails months later instead of at admission. +func liveValidationSamples(sampleEvent Event) [][]Event { + var samples [][]Event for _, author := range []string{"template-validator", ""} { var events []Event for _, operation := range []string{"CREATE", "UPDATE", "DELETE"} { @@ -150,14 +184,53 @@ func ValidateCommitConfig(config CommitConfig) error { event.Object = sampleLabeledObject() } events = append(events, event) - if _, err := renderLiveCommitMessage(PendingWrite{ - Kind: PendingWriteCommit, Events: events, - }, config); err != nil { - return err - } + samples = append(samples, append([]Event(nil), events...)) } } + return samples +} + +// requestTemplateProbe is the sentinel that requestTemplate validation renders as the request's +// message. It only has to be something no template could plausibly produce on its own. +const requestTemplateProbe = "" +// validateRequestTemplate checks that a configured requestTemplate renders, AND that it actually +// puts the request's message in the commit. +// +// The second half is the point. Without it the feature has a hole exactly as bad as the one it was +// designed to avoid: a target could set `requestTemplate: "chore: sync {{.Count}} resources"` and +// every save message would silently vanish — the requester writes a reason, the commit never +// carries it, and the commit is still counted as request-sourced. Framing the message is the whole +// purpose of the field, so a template that drops it is a mistake, not a configuration choice. +// +// It probes the RENDERED OUTPUT rather than scanning the template source. A scan for the literal +// "{{.RequestMessage}}" would reject `{{.RequestMessage | printf "%s"}}`, a template that assigns +// it to a variable first, and every other legitimate spelling — while the probe accepts all of them +// for the right reason: the message reached the commit. +// +// Sample execution cannot prove every branch: a template that drops the message only under, say, +// {{if eq .Count 1}} still passes. That is the same caveat docs/configuration.md already states for +// the other templates, not a new one. +func validateRequestTemplate(config CommitConfig, events []Event) error { + if config.Message.RequestTemplate == "" { + return nil + } + + rendered, err := renderRequestCommitMessage(PendingWrite{ + Kind: PendingWriteCommit, + CommitMessage: requestTemplateProbe, + Events: events, + }, config) + if err != nil { + return err + } + // EVERY sample must carry the message through, not merely one: the contract is that a + // requester's reason reaches the commit whatever the window happened to contain. + if !strings.Contains(rendered, requestTemplateProbe) { + return errors.New("requestTemplate must render {{.RequestMessage}}: as written it would " + + "drop the CommitRequest's message from the commit. Omit requestTemplate to commit that " + + "message verbatim") + } return nil } diff --git a/internal/git/commit_executor.go b/internal/git/commit_executor.go index 3c872041..f464df4e 100644 --- a/internal/git/commit_executor.go +++ b/internal/git/commit_executor.go @@ -30,11 +30,15 @@ func (w *BranchWorker) executePendingWrites( // SHA on push, and a rebase-replay (which re-runs this loop on the retained // writes) refreshes it to the post-rebase hash. for i := range pendingWrites { - created, hash, err := w.executePendingWrite(ctx, repo, worktree, pendingWrites[i]) + created, hash, source, err := w.executePendingWrite(ctx, repo, worktree, pendingWrites[i]) if err != nil { return commitsCreated, err } pendingWrites[i].CommitSHA = hash + // Stamped for the same reason the hash is: publishCommitsForPush runs after the push and + // would otherwise recompute the source from the write, which cannot know that a + // requestTemplate render failed back here at commit time. + pendingWrites[i].committedMessageSource = source commitsCreated += created } @@ -68,6 +72,14 @@ const ( messageResolutionPreRendered // messageResolutionRequest is a message a CommitRequest supplied, used verbatim. messageResolutionRequest + // messageResolutionRequestTemplate is a CommitRequest's message framed by the target's + // requestTemplate. The request's own bytes are never parsed — they ride in as .RequestMessage — + // so the formatting policy is the operator's while the message stays the requester's. + messageResolutionRequestTemplate + // messageResolutionRequestFallback is a requestTemplate that failed to render, so the literal + // was committed instead. Never returned by resolveMessage: it is decided at render time and + // stamped onto the write, because a silent, SUCCESSFUL commit is otherwise invisible. + messageResolutionRequestFallback // messageResolutionReconcileTemplate renders reconcileTemplate from the write's events. messageResolutionReconcileTemplate // messageResolutionLiveTemplate renders liveTemplate for a live window. @@ -80,6 +92,8 @@ func (p PendingWrite) resolveMessage() messageResolution { switch { case p.Kind == PendingWriteResync: return messageResolutionPreRendered + case p.CommitMessage != "" && p.CommitConfig.Message.RequestTemplate != "": + return messageResolutionRequestTemplate case p.CommitMessage != "": return messageResolutionRequest case p.Kind == PendingWriteAtomic: @@ -98,6 +112,10 @@ func (r messageResolution) label() string { switch r { case messageResolutionRequest: return messageSourceCommitRequest + case messageResolutionRequestTemplate: + return messageSourceCommitRequestFramed + case messageResolutionRequestFallback: + return messageSourceCommitRequestFallback case messageResolutionPreRendered, messageResolutionReconcileTemplate: return messageSourceReconcile case messageResolutionLiveTemplate: @@ -112,11 +130,22 @@ func (r messageResolution) label() string { } // messageSource is the commits_total `message_source` label for this write. +// +// It prefers the source stamped at commit time, because that is the only one that knows whether a +// requestTemplate actually rendered. An unstamped write — one that never reached the executor, as +// in a unit test — falls back to what the write itself implies. func (p PendingWrite) messageSource() string { + if p.committedMessageSource != messageResolutionUnsupported { + return p.committedMessageSource.label() + } return p.resolveMessage().label() } -func (p PendingWrite) commitMetadata() (string, *gogit.CommitOptions, error) { +// commitMetadata builds one commit's message and options, and reports the message source it +// actually committed under — which is not always what resolveMessage predicted, because a +// requestTemplate that fails to render falls back to the literal. The caller stamps that back onto +// the write so the commits counter tells the truth; see executePendingWrites. +func (p PendingWrite) commitMetadata() (string, *gogit.CommitOptions, messageResolution, error) { var message string var err error resolution := p.resolveMessage() @@ -126,20 +155,51 @@ func (p PendingWrite) commitMetadata() (string, *gogit.CommitOptions, error) { case messageResolutionRequest: message = p.CommitMessage err = ValidateLiteralCommitMessage(message) + case messageResolutionRequestTemplate: + message, resolution = p.framedRequestMessage() case messageResolutionReconcileTemplate: message, err = renderReconcileCommitMessageFromEvents(p.Events, p.Target().Name, p.CommitConfig) case messageResolutionLiveTemplate: message, err = renderLiveCommitMessage(p, p.CommitConfig) + case messageResolutionRequestFallback: + // resolveMessage never returns it; only framedRequestMessage produces it, below. + err = fmt.Errorf("unsupported pending write kind %q", p.Kind) case messageResolutionUnsupported: err = fmt.Errorf("unsupported pending write kind %q", p.Kind) default: err = fmt.Errorf("unsupported pending write kind %q", p.Kind) } if err != nil { - return "", nil, err + return "", nil, messageResolutionUnsupported, err } log.Log.V(1).Info("Selected commit message", "source", resolution.label()) - return message, commitOptionsFor(p, p.CommitConfig, p.Signer, time.Now()), nil + return message, commitOptionsFor(p, p.CommitConfig, p.Signer, time.Now()), resolution, nil +} + +// framedRequestMessage renders the target's requestTemplate around an attached CommitRequest's +// message, falling back to that message verbatim if the render fails. +// +// The fallback 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 ValidateLiteralCommitMessage at admission. Failing would discard the requester's +// save AND every other author's retained events in the same window, to punish a formatting mistake +// that ValidateCommitConfig should have caught at GitTarget admission and that the fallback makes +// harmless. +// +// It is never silent. The Error log names the target, and the returned resolution moves the commit +// to message_source="commit_request_fallback" — because a fallback is a SUCCESSFUL commit, so no +// condition moves, nothing is refused, and a rate is the only way an operator sees a template that +// has quietly stopped applying. +// +// It returns no error, and cannot: "this failed" is not an outcome here, which is the whole point. +func (p PendingWrite) framedRequestMessage() (string, messageResolution) { + framed, err := renderRequestCommitMessage(p, p.CommitConfig) + if err != nil { + log.Log.Error(err, "requestTemplate failed to render; committing the CommitRequest message verbatim", + "gitTarget", p.Target().Namespace+"/"+p.Target().Name) + return p.CommitMessage, messageResolutionRequestFallback + } + return framed, messageResolutionRequestTemplate } func (w *BranchWorker) executePendingWrite( @@ -147,20 +207,21 @@ func (w *BranchWorker) executePendingWrite( repo *gogit.Repository, worktree *gogit.Worktree, pendingWrite PendingWrite, -) (int, plumbing.Hash, error) { +) (int, plumbing.Hash, messageResolution, error) { switch pendingWrite.Kind { case PendingWriteResync: // Resync writes never carry a CommitRequest, so their commit hash is unused; // report ZeroHash to keep the per-write SHA bookkeeping uniform. created, err := w.executeResyncPendingWrite(ctx, repo, worktree, pendingWrite) - return created, plumbing.ZeroHash, err + return created, plumbing.ZeroHash, messageResolutionPreRendered, err case PendingWriteCommit, PendingWriteAtomic: default: - return 0, plumbing.ZeroHash, fmt.Errorf("unsupported pending write kind %q", pendingWrite.Kind) + return 0, plumbing.ZeroHash, messageResolutionUnsupported, + fmt.Errorf("unsupported pending write kind %q", pendingWrite.Kind) } if len(pendingWrite.Events) == 0 { - return 0, plumbing.ZeroHash, nil + return 0, plumbing.ZeroHash, messageResolutionUnsupported, nil } target := pendingWrite.Target() @@ -170,25 +231,27 @@ func (w *BranchWorker) executePendingWrite( encryptionPath, target.EncryptionConfig, ); err != nil { - return 0, plumbing.ZeroHash, fmt.Errorf("configure secret encryptor: %w", err) + return 0, plumbing.ZeroHash, messageResolutionUnsupported, + fmt.Errorf("configure secret encryptor: %w", err) } anyChanges, err := w.applyPendingWriteEvents(ctx, repo, worktree, pendingWrite.Events, pendingWrite.Targets) if err != nil { - return 0, plumbing.ZeroHash, err + return 0, plumbing.ZeroHash, messageResolutionUnsupported, err } if !anyChanges { - return 0, plumbing.ZeroHash, nil + return 0, plumbing.ZeroHash, messageResolutionUnsupported, nil } - commitMessage, commitOptions, err := pendingWrite.commitMetadata() + commitMessage, commitOptions, source, err := pendingWrite.commitMetadata() if err != nil { - return 0, plumbing.ZeroHash, err + return 0, plumbing.ZeroHash, messageResolutionUnsupported, err } hash, err := worktree.Commit(commitMessage, commitOptions) if err != nil { - return 0, plumbing.ZeroHash, fmt.Errorf("failed to create commit: %w", err) + return 0, plumbing.ZeroHash, messageResolutionUnsupported, + fmt.Errorf("failed to create commit: %w", err) } log.FromContext(ctx).Info( @@ -198,7 +261,7 @@ func (w *BranchWorker) executePendingWrite( "message", commitMessage, ) - return 1, hash, nil + return 1, hash, source, nil } func (w *BranchWorker) applyPendingWriteEvents( diff --git a/internal/git/commit_executor_test.go b/internal/git/commit_executor_test.go index c68a22c0..eb8f6c1e 100644 --- a/internal/git/commit_executor_test.go +++ b/internal/git/commit_executor_test.go @@ -109,7 +109,7 @@ func TestExecutor_GroupedSingleEvent_UsesLiveTemplate(t *testing.T) { CommitConfig: config, } - message, options, err := pendingWrite.commitMetadata() + message, options, _, err := pendingWrite.commitMetadata() require.NoError(t, err) assert.Equal(t, "group: alice changed 1", message) assert.Equal(t, "alice", options.Author.Name) @@ -129,7 +129,7 @@ func TestExecutor_GroupedMultiEvent_UsesLiveTemplate(t *testing.T) { CommitConfig: config, } - message, options, err := pendingWrite.commitMetadata() + message, options, _, err := pendingWrite.commitMetadata() require.NoError(t, err) assert.Equal(t, "group: alice 2 team-a", message) assert.Equal(t, "alice", options.Author.Name) @@ -152,7 +152,7 @@ func TestExecutor_AtomicUnit_UsesReconcileMessage(t *testing.T) { GitTargetNamespace: "default", } - message, options, err := pendingWrite.commitMetadata() + message, options, _, err := pendingWrite.commitMetadata() require.NoError(t, err) assert.Equal(t, "reconcile: 2 team-a", message) assert.Equal(t, DefaultCommitterName, options.Author.Name) @@ -167,7 +167,7 @@ func TestExecutor_NoOpUnit_SkipsCommit(t *testing.T) { headBefore, err := repo.Head() require.NoError(t, err) - created, hash, err := worker.executePendingWrite(context.Background(), repo, worktree, PendingWrite{ + created, hash, _, err := worker.executePendingWrite(context.Background(), repo, worktree, PendingWrite{ Kind: PendingWriteCommit, Events: []Event{event}, CommitConfig: ResolveCommitConfig(nil), @@ -210,7 +210,7 @@ func TestExecutor_AppliesEncryptionFromPendingWrite_NotFromWorker(t *testing.T) }, } - created, hash, err := worker.executePendingWrite(context.Background(), repo, worktree, pendingWrite) + created, hash, _, err := worker.executePendingWrite(context.Background(), repo, worktree, pendingWrite) require.NoError(t, err) assert.Equal(t, 1, created) assert.False(t, hash.IsZero(), "a committed write reports its commit hash") @@ -246,7 +246,7 @@ func TestCommitMetadata_ResyncRenderedMessageIsNotHeldToTheLiteralRequestContrac CommitMessage: rendered, } - message, options, err := pendingWrite.commitMetadata() + message, options, _, err := pendingWrite.commitMetadata() require.NoError(t, err) assert.Equal(t, rendered, message) assert.NotNil(t, options) @@ -300,7 +300,7 @@ func TestMessageSource_MatchesTheMessageActuallyRendered(t *testing.T) { assert.Equal(t, tc.want, write.messageSource()) - message, _, err := write.commitMetadata() + message, _, _, err := write.commitMetadata() require.NoError(t, err) if tc.message != "" { assert.Equal(t, tc.message, message) diff --git a/internal/git/commit_metadata_fields_test.go b/internal/git/commit_metadata_fields_test.go index 77baaa31..3d566717 100644 --- a/internal/git/commit_metadata_fields_test.go +++ b/internal/git/commit_metadata_fields_test.go @@ -32,7 +32,7 @@ func labeledDeploymentEvent(name, namespace string, labels map[string]string) Ev } func TestBuildLiveCommitMessageData_CarriesKindAndLabels(t *testing.T) { - data := buildLiveCommitMessageData("someone", "target", []Event{ + data := buildLiveCommitMessageData("someone", "target", "", []Event{ labeledDeploymentEvent("api", "prod", map[string]string{"team": "payments"}), }) @@ -57,7 +57,7 @@ func TestBuildLiveCommitMessageData_ClusterScopedRendersTheSentinel(t *testing.T event := labeledDeploymentEvent("admin", "", nil) event.Identifier = types.NewResourceIdentifier("rbac.authorization.k8s.io", "v1", "clusterroles", "", "admin") - data := buildLiveCommitMessageData("someone", "target", []Event{event}) + data := buildLiveCommitMessageData("someone", "target", "", []Event{event}) if got := data.Resources[0].Namespace; got != types.ClusterScopeSegment { t.Errorf("Namespace = %q, want the %q sentinel, so a template need not guard it", @@ -72,7 +72,7 @@ func TestBuildLiveCommitMessageData_DeleteHasNoObjectMetadata(t *testing.T) { event.Object = nil event.Operation = "DELETE" - ref := buildLiveCommitMessageData("someone", "target", []Event{event}).Resources[0] + ref := buildLiveCommitMessageData("someone", "target", "", []Event{event}).Resources[0] if ref.Kind != "" || ref.Labels != nil { t.Errorf("Kind = %q, Labels = %v, want both empty for a DELETE", ref.Kind, ref.Labels) @@ -84,7 +84,7 @@ func TestBuildLiveCommitMessageData_DeleteHasNoObjectMetadata(t *testing.T) { // A commit is 1:n, so a label is a SET here where placement reads a single value. func TestLiveCommitMessageData_LabelValues(t *testing.T) { - data := buildLiveCommitMessageData("someone", "target", []Event{ + data := buildLiveCommitMessageData("someone", "target", "", []Event{ labeledDeploymentEvent("api", "prod", map[string]string{"team": "payments"}), labeledDeploymentEvent("web", "prod", map[string]string{"team": "storefront"}), labeledDeploymentEvent("cache", "prod", map[string]string{"team": "payments"}), @@ -112,7 +112,7 @@ func TestLiveCommitMessageData_LabelValues(t *testing.T) { // The subject-line case: one shared value names the whole commit. func TestLiveCommitMessageData_LabelValue_AgreedValue(t *testing.T) { - data := buildLiveCommitMessageData("someone", "target", []Event{ + data := buildLiveCommitMessageData("someone", "target", "", []Event{ labeledDeploymentEvent("api", "prod", map[string]string{"team": "payments"}), labeledDeploymentEvent("cache", "prod", map[string]string{"team": "payments"}), }) @@ -137,7 +137,7 @@ func TestLiveCommitMessageData_LabelValue_PartiallyLabeledCommitNamesNoOne(t *te {"a different label", labeledDeploymentEvent("other", "prod", map[string]string{"squad": "payments"})}, } { t.Run(tc.name, func(t *testing.T) { - data := buildLiveCommitMessageData("someone", "target", []Event{labeled, tc.other}) + data := buildLiveCommitMessageData("someone", "target", "", []Event{labeled, tc.other}) if v := data.LabelValue("team"); v != "" { t.Errorf("LabelValue = %q, want empty: %q carries no team, so the commit is not one team's", @@ -154,7 +154,7 @@ func TestLiveCommitMessageData_LabelValue_DeleteLeavesTheCommitUnnamed(t *testin deleted.Object = nil deleted.Operation = "DELETE" - data := buildLiveCommitMessageData("someone", "target", []Event{ + data := buildLiveCommitMessageData("someone", "target", "", []Event{ labeledDeploymentEvent("api", "prod", map[string]string{"team": "payments"}), deleted, }) diff --git a/internal/git/literal_message_test.go b/internal/git/literal_message_test.go index 48924d94..f2c2cd98 100644 --- a/internal/git/literal_message_test.go +++ b/internal/git/literal_message_test.go @@ -43,14 +43,14 @@ func TestCommitMetadata_LiteralPrecedenceAndPreservation(t *testing.T) { CommitConfig: ResolveCommitConfig(nil)} p.CommitConfig.Message.LiveTemplate = "{{ invalid" p.CommitConfig.Message.ReconcileTemplate = "{{ invalid" - actual, options, err := p.commitMetadata() + actual, options, _, err := p.commitMetadata() require.NoError(t, err) assert.Equal(t, message, actual) assert.Equal(t, DefaultCommitterName, options.Committer.Name) } } p := PendingWrite{Kind: PendingWriteCommit, CommitMessage: " \n ", CommitConfig: ResolveCommitConfig(nil)} - _, _, err := p.commitMetadata() + _, _, _, err := p.commitMetadata() require.ErrorContains(t, err, "non-whitespace") } diff --git a/internal/git/open_window.go b/internal/git/open_window.go index 46dc278b..b32813c8 100644 --- a/internal/git/open_window.go +++ b/internal/git/open_window.go @@ -103,7 +103,14 @@ func windowPathKey(e Event, writer eventContentWriter) string { // buildLiveCommitMessageData produces the template context for a grouped // commit unit. Operations are counted by Operation tag; Resources is the // deduplicated list of resource refs in arrival order. -func buildLiveCommitMessageData(author, gitTarget string, events []Event) LiveCommitMessageData { +// +// requestMessage is the attached CommitRequest's message, or empty when no request attached. It is +// carried through unaltered — never parsed, never trimmed — because the request's literal bytes +// reaching the commit is the audit property the whole CommitRequest contract rests on. +func buildLiveCommitMessageData( + author, gitTarget, requestMessage string, + events []Event, +) LiveCommitMessageData { operations := make(map[string]int, groupedCommitOperationKinds) resources := make([]ResourceRef, 0, len(events)) for _, e := range events { @@ -129,10 +136,11 @@ func buildLiveCommitMessageData(author, gitTarget string, events []Event) LiveCo }) } return LiveCommitMessageData{ - Author: author, - GitTarget: gitTarget, - Count: len(events), - Operations: operations, - Resources: resources, + Author: author, + GitTarget: gitTarget, + Count: len(events), + Operations: operations, + Resources: resources, + RequestMessage: requestMessage, } } diff --git a/internal/git/pending_writes.go b/internal/git/pending_writes.go index 7f47a2c3..729673ce 100644 --- a/internal/git/pending_writes.go +++ b/internal/git/pending_writes.go @@ -335,6 +335,18 @@ const ( // verbatim. It counts commits that USED such a message, not CommitRequests: a request // omitting spec.message takes the target's liveTemplate and counts as live. messageSourceCommitRequest = "commit_request" + // messageSourceCommitRequestFramed is a CommitRequest message rendered through the target's + // requestTemplate. Split from commit_request rather than folded into it, even though the text + // originated with the request either way: the difference is not HOW the text was produced (the + // distinction reconcile deliberately collapses, below) but WHETHER the operator's configured + // framing applied at all, which is a question an operator actually asks. + messageSourceCommitRequestFramed = "commit_request_framed" + // messageSourceCommitRequestFallback is a requestTemplate that failed to render, so the + // request's message was committed verbatim instead. It is the ONLY signal for that: the commit + // succeeds, nothing is refused, and no condition moves, so without a counter a template that + // has quietly stopped applying looks exactly like one that was never configured. Alert on its + // rate; see docs/interpreting-metrics.md. + messageSourceCommitRequestFallback = "commit_request_fallback" // messageSourceLive is a live window rendered through the target's liveTemplate. messageSourceLive = "live" // messageSourceReconcile is an atomic snapshot or a resync rendered through diff --git a/internal/git/request_template_test.go b/internal/git/request_template_test.go new file mode 100644 index 00000000..4cf74558 --- /dev/null +++ b/internal/git/request_template_test.go @@ -0,0 +1,190 @@ +// SPDX-License-Identifier: Apache-2.0 + +package git + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/ConfigButler/gitops-reverser/api/v1alpha3" + "github.com/ConfigButler/gitops-reverser/internal/types" +) + +const requestBodyTemplate = "{{.RequestMessage}}\n\n" + + "{{range .Resources -}}\n- [{{.Operation}}] {{.Resource}}/{{.Name}}\n{{end -}}" + +func requestConfig(requestTemplate string) CommitConfig { + return ResolveCommitConfig(nil).WithTargetMessage(&v1alpha3.CommitMessageSpec{ + RequestTemplate: requestTemplate, + }) +} + +func requestWrite(message string, config CommitConfig) PendingWrite { + return PendingWrite{ + Kind: PendingWriteCommit, + CommitMessage: message, + CommitConfig: config, + Events: []Event{{ + Operation: "UPDATE", + Identifier: types.ResourceIdentifier{ + Group: "apps", Version: "v1", Resource: "deployments", + Namespace: "prod", Name: "api", + }, + UserInfo: UserInfo{Username: "alice"}, + GitTargetName: "platform", + GitTargetNamespace: "default", + }}, + Targets: map[pendingTargetKey]ResolvedTargetMetadata{ + {Name: "platform", Namespace: "default"}: {Name: "platform", Namespace: "default"}, + }, + } +} + +// The gap this feature closes: the override used to REPLACE the template, so supplying a save +// message cost you the resource body liveTemplate would have produced. "Why I saved" and "what was +// saved" were mutually exclusive. +func TestFramedRequestMessage_ComposesTheReasonWithWhatWasSaved(t *testing.T) { + config := requestConfig(requestBodyTemplate) + write := requestWrite("fix(api): correct the service port", config) + + message, resolution := write.framedRequestMessage() + + assert.Equal(t, messageResolutionRequestTemplate, resolution) + assert.Equal(t, "fix(api): correct the service port\n\n- [UPDATE] deployments/api\n", message) +} + +// The contract PR 1 of this series must not break: the request's bytes reach the commit unaltered. +// A target with no requestTemplate commits exactly what was supplied — no trimming, no framing, no +// reformatting — which is what makes a CommitRequest auditable against the commit it produced. +func TestCommitMetadata_WithoutARequestTemplateTheLiteralIsCommittedByteForByte(t *testing.T) { + literal := " fix(api): correct the service port \n\n Route traffic to the API container. " + write := requestWrite(literal, ResolveCommitConfig(nil)) + + message, _, resolution, err := write.commitMetadata() + require.NoError(t, err) + + assert.Equal(t, literal, message, "an unframed request message must not be altered at all") + assert.Equal(t, messageResolutionRequest, resolution) + assert.Equal(t, messageSourceCommitRequest, resolution.label()) +} + +// A requestTemplate that fails at render time must not cost the window. The literal already passed +// admission validation, so a correct answer is always in hand; failing would discard the requester's +// save AND every other author's retained events, to punish a formatting mistake. +// +// It is not silent: the commit moves to its own message_source, which is the only signal an +// operator gets for a template that has quietly stopped applying. +func TestFramedRequestMessage_RenderFailureFallsBackToTheLiteralAndSaysSo(t *testing.T) { + // missingkey=error: Labels carries no "team" on this sample, so this renders at admission for a + // labelled resource and dies here for one without. + config := requestConfig("{{.RequestMessage}} {{(index .Resources 0).Labels.team}}") + write := requestWrite("fix(api): correct the service port", config) + + // No error return at all: "the template failed" is not an outcome this function can report, + // which is exactly the guarantee the window depends on. + message, resolution := write.framedRequestMessage() + + assert.Equal(t, "fix(api): correct the service port", message) + assert.Equal(t, messageResolutionRequestFallback, resolution) + assert.Equal(t, messageSourceCommitRequestFallback, resolution.label()) +} + +// The framed and fallback arms must stay distinguishable in commits_total. Folding them together +// would leave "the template works" and "the template silently stopped applying" reading identically. +func TestMessageSource_SeparatesFramedFromFallbackAndPlainRequest(t *testing.T) { + framedConfig := requestConfig(requestBodyTemplate) + + assert.Equal(t, messageSourceCommitRequest, + requestWrite("save", ResolveCommitConfig(nil)).messageSource()) + assert.Equal(t, messageSourceCommitRequestFramed, + requestWrite("save", framedConfig).messageSource()) + + // Stamped at commit time, which is the only place the fallback is known. + stamped := requestWrite("save", framedConfig) + stamped.committedMessageSource = messageResolutionRequestFallback + assert.Equal(t, messageSourceCommitRequestFallback, stamped.messageSource()) +} + +// Precedence: a configured requestTemplate outranks the verbatim arm, and a window with no request +// message is untouched by the field entirely. +func TestResolveMessage_RequestTemplateOutranksTheLiteralArm(t *testing.T) { + framedConfig := requestConfig(requestBodyTemplate) + + assert.Equal(t, messageResolutionRequestTemplate, + requestWrite("save", framedConfig).resolveMessage()) + assert.Equal(t, messageResolutionRequest, + requestWrite("save", ResolveCommitConfig(nil)).resolveMessage()) + assert.Equal(t, messageResolutionLiveTemplate, + requestWrite("", framedConfig).resolveMessage(), + "a window no request attached to renders liveTemplate, template configured or not") +} + +// The rule that makes the audit property hold rather than merely hoped for. Without it a target +// could frame away the requester's reason entirely and the commit would still count as +// request-sourced. +func TestValidateCommitConfig_RejectsARequestTemplateThatDropsTheMessage(t *testing.T) { + err := ValidateCommitConfig(requestConfig("chore: sync {{.Count}} resources")) + + require.Error(t, err) + assert.Contains(t, err.Error(), "requestTemplate must render {{.RequestMessage}}") +} + +// The accept half, and the reason the check probes the RENDERED OUTPUT instead of scanning the +// template source: every one of these is a legitimate way to put the message in the commit, and a +// source scan for the literal "{{.RequestMessage}}" would reject all but the first. +func TestValidateCommitConfig_AcceptsEveryHonestSpellingOfRequestMessage(t *testing.T) { + for name, template := range map[string]string{ + "direct": "{{.RequestMessage}}", + "piped": `{{.RequestMessage | printf "%s"}}`, + "via variable": "{{$m := .RequestMessage}}chore: save\n\n{{$m}}", + "guarded": "{{if .RequestMessage}}{{.RequestMessage}}{{end}}", + "with a body": requestBodyTemplate, + "with a prefix": "save: {{.RequestMessage}}", + } { + t.Run(name, func(t *testing.T) { + assert.NoError(t, ValidateCommitConfig(requestConfig(template))) + }) + } +} + +func TestValidateCommitConfig_RejectsAnUnparseableRequestTemplate(t *testing.T) { + err := ValidateCommitConfig(requestConfig("{{.RequestMessage")) + + require.Error(t, err) + assert.Contains(t, err.Error(), "parse request commit template") +} + +// missingkey=error makes a label a template names but a resource does not carry a render failure. +// Catching it at GitTarget admission is the whole reason validation renders samples: the +// alternative is a commit that falls back months later, for a fault nobody was told about. +// +// It only holds because requestTemplate is validated against the SAME window shapes as +// liveTemplate. A single sample would not catch this: the labelled sample carries "team", so the +// fault appears only on the entries that carry no object at all — which is exactly the shape a +// DELETE has in production, since the object is gone by the time the event is built. +func TestValidateCommitConfig_RejectsARequestTemplateThatReadsAMissingLabel(t *testing.T) { + err := ValidateCommitConfig(requestConfig( + "{{.RequestMessage}}{{range .Resources}} {{.Labels.team}}{{end}}")) + + require.Error(t, err) + assert.Contains(t, err.Error(), "execute request commit template") + + // The accessor form renders empty instead of failing, and must still be accepted — the same + // escape hatch liveTemplate documents. + assert.NoError(t, ValidateCommitConfig(requestConfig( + `{{.RequestMessage}}{{range .Resources}} {{.Label "team"}}{{end}}`))) +} + +// An unset requestTemplate is "do not frame", not "use a standard framing", so it must have no +// built-in default the way the other two templates do. +func TestResolveCommitConfig_RequestTemplateHasNoDefault(t *testing.T) { + assert.Empty(t, ResolveCommitConfig(nil).Message.RequestTemplate) + require.NoError(t, ValidateCommitConfig(ResolveCommitConfig(nil))) + + overlaid := ResolveCommitConfig(nil).WithTargetMessage(&v1alpha3.CommitMessageSpec{ + RequestTemplate: " " + requestBodyTemplate + " ", + }) + assert.Equal(t, requestBodyTemplate, overlaid.Message.RequestTemplate, "overlay trims like the others") +} diff --git a/internal/git/resync_flush.go b/internal/git/resync_flush.go index 579001ae..28002c45 100644 --- a/internal/git/resync_flush.go +++ b/internal/git/resync_flush.go @@ -301,7 +301,7 @@ func (w *BranchWorker) executeResyncPendingWrite( return 0, err } pendingWrite.CommitMessage = rendered - message, options, err := pendingWrite.commitMetadata() + message, options, _, err := pendingWrite.commitMetadata() if err != nil { return 0, err } diff --git a/internal/git/types.go b/internal/git/types.go index 605a1c37..789c4cfb 100644 --- a/internal/git/types.go +++ b/internal/git/types.go @@ -305,6 +305,12 @@ type PendingWrite struct { // resolved Committed (with CommitSHA) once this write is pushed. It rides the write through the // push cooldown and the conflict rebase-replay, so the result follows the data. CommitRequest *commitRequestID + // committedMessageSource is the message source this write actually committed under, stamped by + // executePendingWrites the way CommitSHA is and for the same reason: publishCommitsForPush runs + // after the push, and recomputing the source from the write cannot know that a requestTemplate + // render failed at commit time. Zero means "not stamped", so messageSource() recomputes. + committedMessageSource messageResolution + // CommitSHA is the hash of the commit this write created, captured in // executePendingWrite and refreshed when the write is re-executed on a // rebase-replay (so it is never a stale pre-rebase hash). Zero when the write @@ -588,6 +594,9 @@ type CommitterConfig struct { type CommitMessageConfig struct { LiveTemplate string ReconcileTemplate string + // RequestTemplate frames a CommitRequest's message. Empty — the default — commits that + // message verbatim, which is the behaviour every target had before the field existed. + RequestTemplate string } // ReconcileCommitMessageData is the template context for reconcile commit messages. @@ -690,6 +699,14 @@ type LiveCommitMessageData struct { // Resources is the per-resource list, deduplicated by file path so the // final state is what's being committed. Resources []ResourceRef + // RequestMessage is the message an attached CommitRequest supplied, unaltered. It is empty for + // every window no request attached to, which is the ordinary case and why liveTemplate may + // reference it freely. + // + // It lives on this struct rather than on a parallel request-only context for the reason + // ResourceRef's own comment gives about the placement vocabulary: a reader should not have to + // learn two vocabularies to describe one commit. One sample builder then serves both renders. + RequestMessage string } // LabelValues is the sorted, distinct set of values this commit's resources carry for one @@ -790,5 +807,11 @@ func (c CommitConfig) WithTargetMessage(spec *v1alpha3.CommitMessageSpec) Commit if reconcileTemplate := strings.TrimSpace(spec.ReconcileTemplate); reconcileTemplate != "" { c.Message.ReconcileTemplate = reconcileTemplate } + // No built-in default to fall back to, unlike the two above: an unset requestTemplate is not + // "use the standard framing", it is "do not frame at all", and that has to stay distinguishable + // from a configured one or every existing target would start reformatting its save messages. + if requestTemplate := strings.TrimSpace(spec.RequestTemplate); requestTemplate != "" { + c.Message.RequestTemplate = requestTemplate + } return c } From bc88b0b58c116b624a1b2a21287fab1ead9ed8f0 Mon Sep 17 00:00:00 2001 From: Simon Koudijs Date: Tue, 15 Sep 2026 08:26:18 +0000 Subject: [PATCH 2/3] fix(e2e): retry a seed push that loses to the controller, and tighten framing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- api/v1alpha3/gitprovider_types.go | 27 ++++--- .../crd/bases/configbutler.ai_gittargets.yaml | 19 ++--- docs/configuration.md | 10 ++- internal/git/commit_executor.go | 10 ++- internal/git/request_template_test.go | 27 +++++++ test/e2e/inplace_edit_e2e_test.go | 75 ++++++++++++++++--- 6 files changed, 137 insertions(+), 31 deletions(-) diff --git a/api/v1alpha3/gitprovider_types.go b/api/v1alpha3/gitprovider_types.go index 4c807bc1..b8f77fe4 100644 --- a/api/v1alpha3/gitprovider_types.go +++ b/api/v1alpha3/gitprovider_types.go @@ -184,16 +184,25 @@ type CommitMessageSpec struct { // +optional LiveTemplate string `json:"liveTemplate,omitempty"` - // RequestTemplate optionally frames a CommitRequest's message instead of committing it - // verbatim. It renders only for a window a request's message is attached to, and receives the - // live fields plus RequestMessage, which carries that message unaltered. + // Rationale, kept out of the doc comment (and so out of the CRD schema) by the blank line + // below: a CommitRequest is created by whoever holds create on it in a namespace, so its + // author supplies the message but must not set commit-message policy. Framing the message + // rather than parsing it keeps that split — the operator owns the wording, the requester owns + // the words, and nothing a requester writes is ever executed. Rejecting a template that never + // renders RequestMessage is what keeps the split honest in the other direction: silently + // discarding a requester's stated reason would be worse than having no template at all. + + // RequestTemplate frames a CommitRequest's message instead of committing it verbatim. It + // renders only for a commit window whose attached request supplied a message; every other + // window uses liveTemplate or reconcileTemplate. // - // Omit it and a request's message is committed exactly as supplied, which is the default. - // Set it and the operator owns the wording around the message while the requester still owns - // the message itself: the request is never parsed as a template, so a save-button user cannot - // execute one. A template that never renders RequestMessage is REJECTED — framing the message - // is the whole point, and silently dropping a requester's stated reason is worse than having - // no template at all. + // It receives the same fields as liveTemplate, plus RequestMessage carrying the request's + // message unaltered. That message is never parsed as a template, so template syntax inside it + // stays literal. + // + // Omitted, a request's message is committed exactly as supplied. A template that never renders + // RequestMessage is rejected. One that fails to render at commit time commits the message + // verbatim instead, counted as message_source="commit_request_fallback". // +optional RequestTemplate string `json:"requestTemplate,omitempty"` diff --git a/config/crd/bases/configbutler.ai_gittargets.yaml b/config/crd/bases/configbutler.ai_gittargets.yaml index 42e08e7f..b34584ac 100644 --- a/config/crd/bases/configbutler.ai_gittargets.yaml +++ b/config/crd/bases/configbutler.ai_gittargets.yaml @@ -174,16 +174,17 @@ spec: type: string requestTemplate: description: |- - RequestTemplate optionally frames a CommitRequest's message instead of committing it - verbatim. It renders only for a window a request's message is attached to, and receives the - live fields plus RequestMessage, which carries that message unaltered. + RequestTemplate frames a CommitRequest's message instead of committing it verbatim. It + renders only for a commit window whose attached request supplied a message; every other + window uses liveTemplate or reconcileTemplate. - Omit it and a request's message is committed exactly as supplied, which is the default. - Set it and the operator owns the wording around the message while the requester still owns - the message itself: the request is never parsed as a template, so a save-button user cannot - execute one. A template that never renders RequestMessage is REJECTED — framing the message - is the whole point, and silently dropping a requester's stated reason is worse than having - no template at all. + It receives the same fields as liveTemplate, plus RequestMessage carrying the request's + message unaltered. That message is never parsed as a template, so template syntax inside it + stays literal. + + Omitted, a request's message is committed exactly as supplied. A template that never renders + RequestMessage is rejected. One that fails to render at commit time commits the message + verbatim instead, counted as message_source="commit_request_fallback". type: string type: object x-kubernetes-validations: diff --git a/docs/configuration.md b/docs/configuration.md index de377cf8..4605a68a 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -603,8 +603,8 @@ spec: | Template | Fields | |---|---| -| `liveTemplate` | `Author`, `GitTarget`, `Count`, `Operations`, `Resources`, `RequestMessage`, and the `LabelValues` / `LabelValue` accessors | -| `requestTemplate` | the same fields, with `RequestMessage` carrying the save request's message | +| `liveTemplate` | `Author`, `GitTarget`, `Count`, `Operations`, `Resources`, and the `LabelValues` / `LabelValue` accessors | +| `requestTemplate` | the same fields, plus `RequestMessage` | | Each `Resources` entry | `Operation`, `Group`, `Version`, `Resource`, `Kind`, `Namespace`, `Name`, `APIVersion`, `Labels`, and the `Label` accessor | | `reconcileTemplate` | `Count`, `GitTarget`, `Group`, `Version`, `Resource`, `APIVersion`, `Namespace`, `Revision` | @@ -615,6 +615,12 @@ first-seen order. An entry already matching Git still counts, and several entrie The count can exceed the number of changed resources. A no-op creates no commit, even with a literal message. Printing a resource entry directly keeps its `group/version/resource[/namespace]/name` form. +`RequestMessage` belongs to `requestTemplate`. It exists on the live context too, but is always +empty there: a window carrying a save request's message renders `requestTemplate` when one is +configured and the literal message when one is not, so `liveTemplate` only ever runs for windows +that have no request message. Do not reach for `{{if .RequestMessage}}` inside `liveTemplate`; +it never fires. + `Author` is the raw window username and is empty when no actor is named. It does not use an OIDC display name or the `attribution-unresolved` Git author sentinel. The sentinel appears only in the Git author header when attribution ran without resolving an actor. Messages never change authorship. diff --git a/internal/git/commit_executor.go b/internal/git/commit_executor.go index f464df4e..49c1c157 100644 --- a/internal/git/commit_executor.go +++ b/internal/git/commit_executor.go @@ -156,7 +156,15 @@ func (p PendingWrite) commitMetadata() (string, *gogit.CommitOptions, messageRes message = p.CommitMessage err = ValidateLiteralCommitMessage(message) case messageResolutionRequestTemplate: - message, resolution = p.framedRequestMessage() + // Validated on THIS arm too, not just the verbatim one. The literal reaches Git either + // way — framed on success, and by itself on the fallback below — so framing must not + // become a route that smuggles a message past the check the verbatim path enforces. + // The controller validates earlier (commitrequest_controller.go), which makes this + // belt-and-braces rather than reachable today; it is exactly the kind of invariant that + // stops being true once a second producer of PendingWrite appears. + if err = ValidateLiteralCommitMessage(p.CommitMessage); err == nil { + message, resolution = p.framedRequestMessage() + } case messageResolutionReconcileTemplate: message, err = renderReconcileCommitMessageFromEvents(p.Events, p.Target().Name, p.CommitConfig) case messageResolutionLiveTemplate: diff --git a/internal/git/request_template_test.go b/internal/git/request_template_test.go index 4cf74558..cd0ae840 100644 --- a/internal/git/request_template_test.go +++ b/internal/git/request_template_test.go @@ -188,3 +188,30 @@ func TestResolveCommitConfig_RequestTemplateHasNoDefault(t *testing.T) { }) assert.Equal(t, requestBodyTemplate, overlaid.Message.RequestTemplate, "overlay trims like the others") } + +// 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 — +// it is the invariant that has to survive a second producer of PendingWrite appearing. +func TestCommitMetadata_FramingStillValidatesTheLiteralMessage(t *testing.T) { + config := requestConfig(requestBodyTemplate) + + for name, message := range map[string]string{ + "a control character": "fix(api): correct\tthe service port", + "whitespace only": " \n ", + } { + t.Run(name, func(t *testing.T) { + _, _, _, err := requestWrite(message, config).commitMetadata() + require.Error(t, err, "a framed message must face the same literal check as a verbatim one") + + // Same verdict on both arms, so framing cannot change what is accepted. + _, _, _, unframedErr := requestWrite(message, ResolveCommitConfig(nil)).commitMetadata() + require.Error(t, unframedErr) + }) + } + + // And the valid case still renders, so the check gates nothing it should not. + message, _, resolution, err := requestWrite("fix(api): correct the service port", config).commitMetadata() + require.NoError(t, err) + assert.Equal(t, messageResolutionRequestTemplate, resolution) + assert.Contains(t, message, "fix(api): correct the service port") +} diff --git a/test/e2e/inplace_edit_e2e_test.go b/test/e2e/inplace_edit_e2e_test.go index e2a68f55..9d946d01 100644 --- a/test/e2e/inplace_edit_e2e_test.go +++ b/test/e2e/inplace_edit_e2e_test.go @@ -390,30 +390,85 @@ func renderInPlaceFixtureFolder(fixtureRoot, namespace string) string { return rendered } +// seedRenderedFolderIntoRepo pushes a fixture folder straight into the target branch, the way a +// human editing the repo by hand would. +// +// It RETRIES, because by the time most callers reach it the GitTarget is healthy and actively +// mirroring into this same branch. The seed reads the remote tip, builds a commit on it and pushes +// with the remote's compare-and-swap in force, so any controller commit landing in that window +// rejects the push: +// +// ! [remote rejected] HEAD -> main (incorrect old value provided) +// +// Losing that race is expected and says nothing about the behaviour under test, so each attempt +// rebuilds the commit on the new tip rather than failing the spec. The window is small but real: +// it was reported as an intermittent failure of the unsupported-folder spec, and the helper is +// shared by six spec files, so one unretried push is a flake source for all of them. func seedRenderedFolderIntoRepo(repo *RepoArtifacts, namespace, renderedFolder, gitPath string) { GinkgoHelper() configureRepoOriginWithCredentials(repo, namespace) - mustGit := func(args ...string) { + Eventually(func() error { + return attemptSeedRenderedFolder(repo, renderedFolder, gitPath) + }, seedPushTimeout, seedPushInterval).Should(Succeed(), + "seeding %q kept losing the push race with the controller's own commits", gitPath) +} + +const ( + seedPushTimeout = 60 * time.Second + seedPushInterval = 2 * time.Second +) + +// attemptSeedRenderedFolder is one seed attempt, from the CURRENT remote tip. +// +// It returns errors rather than asserting, because every step of it is retried: an Expect in here +// would fail the spec on the first lost race instead of letting Eventually rebuild on the new tip. +func attemptSeedRenderedFolder(repo *RepoArtifacts, renderedFolder, gitPath string) error { + runGit := func(args ...string) error { out, gitErr := gitRun(repo.CheckoutDir, args...) - Expect(gitErr).NotTo(HaveOccurred(), fmt.Sprintf("git %s: %s", strings.Join(args, " "), out)) + if gitErr != nil { + return fmt.Errorf("git %s: %w: %s", strings.Join(args, " "), gitErr, out) + } + return nil } + // Re-fetch on EVERY attempt: the whole point of retrying is to rebuild on the tip that beat us. if _, err := gitRun(repo.CheckoutDir, "fetch", "origin", "main"); err == nil { - mustGit("checkout", "-B", "main", "origin/main") - mustGit("reset", "--hard", "origin/main") + if err := runGit("checkout", "-B", "main", "origin/main"); err != nil { + return err + } + if err := runGit("reset", "--hard", "origin/main"); err != nil { + return err + } } else { - mustGit("checkout", "--orphan", "main") + if err := runGit("checkout", "--orphan", "main"); err != nil { + return err + } _, _ = gitRun(repo.CheckoutDir, "rm", "-rf", ".") } dest := filepath.Join(repo.CheckoutDir, gitPath) - Expect(os.RemoveAll(dest)).To(Succeed()) - Expect(copyFixtureDir(renderedFolder, dest)).To(Succeed()) + if err := os.RemoveAll(dest); err != nil { + return fmt.Errorf("clear %s: %w", gitPath, err) + } + if err := copyFixtureDir(renderedFolder, dest); err != nil { + return fmt.Errorf("copy fixture into %s: %w", gitPath, err) + } - mustGit("add", gitPath) - mustGit("commit", "-m", "e2e: seed manifest folder fixture") - mustGit("push", "origin", "HEAD:main") + if err := runGit("add", gitPath); err != nil { + return err + } + // A retry that lands on a tip already carrying this exact content stages nothing, and + // `git commit` with no changes is an error. That is success: the fixture is in Git, which is + // all the caller asked for. + if staged, err := gitRun(repo.CheckoutDir, "diff", "--cached", "--quiet"); err == nil { + _ = staged + return nil + } + if err := runGit("commit", "-m", "e2e: seed manifest folder fixture"); err != nil { + return err + } + return runGit("push", "origin", "HEAD:main") } func copyFixtureDir(src, dst string) error { From 21b4a81bfd9aed7399176d135c9a21f4bd9eda0c Mon Sep 17 00:00:00 2001 From: Simon Koudijs Date: Tue, 15 Sep 2026 09:10:07 +0000 Subject: [PATCH 3/3] fix(commit): bind framing to the grouped kind, and mint a fresh probe MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- internal/git/commit.go | 28 ++++++++++++--- internal/git/commit_executor.go | 9 ++++- internal/git/request_template_test.go | 52 +++++++++++++++++++++++++++ 3 files changed, 83 insertions(+), 6 deletions(-) diff --git a/internal/git/commit.go b/internal/git/commit.go index c8c154c3..3d12c3de 100644 --- a/internal/git/commit.go +++ b/internal/git/commit.go @@ -4,6 +4,8 @@ package git import ( "bytes" + "crypto/rand" + "encoding/hex" "errors" "fmt" "regexp" @@ -190,9 +192,21 @@ func liveValidationSamples(sampleEvent Event) [][]Event { return samples } -// requestTemplateProbe is the sentinel that requestTemplate validation renders as the request's -// message. It only has to be something no template could plausibly produce on its own. -const requestTemplateProbe = "" +// newRequestTemplateProbe mints the sentinel that requestTemplate validation renders as the +// request's message. +// +// Fresh per call, and unpredictable, because the check asks "did the message reach the commit?" by +// looking for this string in the rendered output. A FIXED sentinel answers a weaker question: a +// template that emits the constant itself would pass while never referencing .RequestMessage at +// all. Nobody would write that on purpose, but a check that can be satisfied without doing the +// thing it verifies is not a check. +func newRequestTemplateProbe() (string, error) { + var raw [16]byte + if _, err := rand.Read(raw[:]); err != nil { + return "", fmt.Errorf("generate requestTemplate probe: %w", err) + } + return "gitops-reverser-request-probe-" + hex.EncodeToString(raw[:]), nil +} // validateRequestTemplate checks that a configured requestTemplate renders, AND that it actually // puts the request's message in the commit. @@ -216,9 +230,13 @@ func validateRequestTemplate(config CommitConfig, events []Event) error { return nil } + probe, err := newRequestTemplateProbe() + if err != nil { + return err + } rendered, err := renderRequestCommitMessage(PendingWrite{ Kind: PendingWriteCommit, - CommitMessage: requestTemplateProbe, + CommitMessage: probe, Events: events, }, config) if err != nil { @@ -226,7 +244,7 @@ func validateRequestTemplate(config CommitConfig, events []Event) error { } // EVERY sample must carry the message through, not merely one: the contract is that a // requester's reason reaches the commit whatever the window happened to contain. - if !strings.Contains(rendered, requestTemplateProbe) { + if !strings.Contains(rendered, probe) { return errors.New("requestTemplate must render {{.RequestMessage}}: as written it would " + "drop the CommitRequest's message from the commit. Omit requestTemplate to commit that " + "message verbatim") diff --git a/internal/git/commit_executor.go b/internal/git/commit_executor.go index 49c1c157..0fe3d26c 100644 --- a/internal/git/commit_executor.go +++ b/internal/git/commit_executor.go @@ -92,7 +92,14 @@ func (p PendingWrite) resolveMessage() messageResolution { switch { case p.Kind == PendingWriteResync: return messageResolutionPreRendered - case p.CommitMessage != "" && p.CommitConfig.Message.RequestTemplate != "": + // Framing is bound to the GROUPED kind, not merely to "carries a message". A CommitRequest + // attaches its message to an open live window, which finalizes as PendingWriteCommit, so that + // is the only write requestTemplate describes. An atomic snapshot can also carry a message + // (WriteRequest.CommitMessage), and framing one with the live context would silently reroute + // the documented atomic path through a template written for save requests. + case p.Kind == PendingWriteCommit && + p.CommitMessage != "" && + p.CommitConfig.Message.RequestTemplate != "": return messageResolutionRequestTemplate case p.CommitMessage != "": return messageResolutionRequest diff --git a/internal/git/request_template_test.go b/internal/git/request_template_test.go index cd0ae840..5cb6f43b 100644 --- a/internal/git/request_template_test.go +++ b/internal/git/request_template_test.go @@ -215,3 +215,55 @@ func TestCommitMetadata_FramingStillValidatesTheLiteralMessage(t *testing.T) { assert.Equal(t, messageResolutionRequestTemplate, resolution) assert.Contains(t, message, "fix(api): correct the service port") } + +// A fixed sentinel would let a template satisfy the check by emitting the sentinel itself, never +// referencing .RequestMessage. The probe is minted fresh per validation precisely so that the only +// way to pass is to render the message. +func TestValidateCommitConfig_RejectsATemplateThatFakesTheProbe(t *testing.T) { + for name, template := range map[string]string{ + "the old fixed sentinel": "", + "the probe prefix": "gitops-reverser-request-probe-00000000000000000000000000000000", + } { + t.Run(name, func(t *testing.T) { + err := ValidateCommitConfig(requestConfig(template)) + + require.Error(t, err) + assert.Contains(t, err.Error(), "requestTemplate must render {{.RequestMessage}}") + }) + } +} + +// Two validations of the same template must not be able to agree by accident on a constant. +func TestNewRequestTemplateProbe_IsFreshEachTime(t *testing.T) { + first, err := newRequestTemplateProbe() + require.NoError(t, err) + second, err := newRequestTemplateProbe() + require.NoError(t, err) + + assert.NotEqual(t, first, second) + assert.NotEmpty(t, first) +} + +// requestTemplate describes the commit a CommitRequest's message produces, and a CommitRequest +// attaches to a live window — which finalizes as PendingWriteCommit. An atomic snapshot can also +// carry a message, and it must keep the behaviour it had before requestTemplate existed: committed +// verbatim, never framed with a live context it was not written for. +func TestResolveMessage_AtomicWriteIsNeverFramed(t *testing.T) { + config := requestConfig(requestBodyTemplate) + + atomic := requestWrite("chore: snapshot", config) + atomic.Kind = PendingWriteAtomic + assert.Equal(t, messageResolutionRequest, atomic.resolveMessage(), + "an atomic write with a message stays verbatim even when requestTemplate is configured") + assert.Equal(t, messageSourceCommitRequest, atomic.messageSource()) + + // And with no message it still renders reconcileTemplate, as documented. + plainAtomic := requestWrite("", config) + plainAtomic.Kind = PendingWriteAtomic + assert.Equal(t, messageResolutionReconcileTemplate, plainAtomic.resolveMessage()) + + // A resync is untouched either way. + resync := requestWrite("pre-rendered", config) + resync.Kind = PendingWriteResync + assert.Equal(t, messageResolutionPreRendered, resync.resolveMessage()) +}