fix(config): disambiguate PR comment markers across konflate instances

charts/konflate/README.md

@@ -53,6 +53,7 @@ Kubernetes: `>=1.25.0-0`
 | config.closedPrMax | int | `25` | Cap on retained merged PRs (most-recent win); bounds disk + memory. 0 disables the cap (with `closedPrTtl: "0"`, merged diffs are kept forever). |
 | config.closedPrTtl | string | `"336h"` | How long a merged PR is kept (Go duration); 0 disables the age cap. |
 | config.clusterPath | string | `""` | Directory flate renders from; empty = repo root (correct for the standard root-relative layout). |
+| config.commentTag | string | `""` | Disambiguates this instance's hidden comment marker from another konflate deployment's when multiple instances (sharing one bot identity, a folder-per-cluster monorepo's normal shape) render the same PR — otherwise the second to post finds and overwrites the first's comment instead of creating its own. Empty falls back to `statusCheckName` (already commonly set to a distinct per-instance name), then to no tag at all — a single-instance setup with neither set is unaffected. |
 | config.diffTimeout | string | `""` | Hard cap on a single PR render end-to-end (Go duration). Empty = default (10m); "0" disables. Lower on untrusted instances. |
 | config.extraEnv | list | `[]` | Extra raw env vars merged into the container (advanced). |
 | config.fetchTimeout | string | `""` | Advanced: cap on just the git fetch within a render (Go duration); a short bound stops one slow forge fetch from stalling every render. Empty = default (2m); "0" disables. |

charts/konflate/templates/deployment.tpl

@@ -191,6 +191,10 @@ spec:
             - name: KONFLATE_PR_COMMENTS
               value: "true"
             {{- end }}
+            {{- with .Values.config.commentTag }}
+            - name: KONFLATE_COMMENT_TAG
+              value: {{ tpl . $ | quote }}
+            {{- end }}
             {{- if .Values.config.prCommentTemplate }}
             # A custom comment template — mounted from the chart-managed ConfigMap below.
             - name: KONFLATE_PR_COMMENT_TEMPLATE_FILE

charts/konflate/values.schema.json

@@ -46,6 +46,12 @@
           "title": "clusterPath",
           "type": "string"
         },
+        "commentTag": {
+          "default": "",
+          "description": "Disambiguates this instance's hidden comment marker from another konflate deployment's when multiple instances (sharing one bot identity, a folder-per-cluster monorepo's normal shape) render the same PR — otherwise the second to post finds and overwrites the first's comment instead of creating its own. Empty falls back to `statusCheckName` (already commonly set to a distinct per-instance name), then to no tag at all — a single-instance setup with neither set is unaffected.",
+          "title": "commentTag",
+          "type": "string"
+        },
         "diffTimeout": {
           "default": "",
           "description": "Hard cap on a single PR render end-to-end (Go duration). Empty = default (10m); \"0\" disables. Lower on untrusted instances.",

charts/konflate/values.yaml

@@ -181,6 +181,8 @@ config:
   statusCheckName: ""
   # -- Opt-in: post (and update in place) a PR comment with the rendered summary on each successful render. Needs a write credential and stays off until both are set; independent of `statusChecks`.
   prComments: false
+  # -- Disambiguates this instance's hidden comment marker from another konflate deployment's when multiple instances (sharing one bot identity, a folder-per-cluster monorepo's normal shape) render the same PR — otherwise the second to post finds and overwrites the first's comment instead of creating its own. Empty falls back to `statusCheckName` (already commonly set to a distinct per-instance name), then to no tag at all — a single-instance setup with neither set is unaffected.
+  commentTag: ""
   # -- Optional Go text/template for the PR-comment body, replacing the built-in summary. When set, the chart mounts it (a ConfigMap) and points `KONFLATE_PR_COMMENT_TEMPLATE_FILE` at it. The konflate marker is injected automatically. Context: `.PR`, `.Diff`, `.ReviewURL`, `.Summary` (the default body). Passed verbatim (NOT tpl'd) — it's konflate's own template. See the README.
   prCommentTemplate: ""
   # -- konflate's externally-reachable base URL (e.g. https://konflate.example.com); a posted status/comment links back to it. Empty = posted without a link. `basePath` is appended automatically when it is set and not already present.

internal/config/config.go

@@ -88,6 +88,15 @@ type Config struct {
 	// write credential with StatusChecks; the two are independent toggles.
 	PRComments bool `env:"KONFLATE_PR_COMMENTS" envDefault:"false"`
 
+	// CommentTag disambiguates this konflate instance's PR comment (and its hidden
+	// marker) from another konflate deployment's on the same PR — a folder-per-cluster
+	// monorepo's normal shape, where multiple instances sharing one bot identity can
+	// render the same PR. Empty falls back to StatusCheckName (already commonly set to
+	// a distinct per-instance name for branch protection), then to no tag at all —
+	// unset with StatusCheckName also unset reproduces every prior release's marker.
+	// See CommentMarkerTag.
+	CommentTag string `env:"KONFLATE_COMMENT_TAG"`
+
 	// PRCommentTemplateFile is an optional path to a Go text/template that renders
 	// the PR-comment body, replacing the built-in summary. It's parsed once at
 	// startup; the konflate marker is injected automatically, so the template need
@@ -464,6 +473,25 @@ func (c *Config) StatusChecksEnabled() bool { return c.StatusChecks && c.WriteEn
 // the rendered summary: the toggle is on and a write credential is configured.
 func (c *Config) PRCommentsEnabled() bool { return c.PRComments && c.WriteEnabled() }
 
+// CommentMarkerTag returns the string embedded in konflate's PR-comment marker to
+// disambiguate this instance's comment from another konflate deployment's on the
+// same PR (see CommentTag). Falls back to StatusCheckName, but only when it was
+// set to something other than DefaultStatusCheckName: Load fills a blank
+// StatusCheckName in with that constant before this ever runs, so falling back to
+// it unconditionally would tag every single-instance install identically and
+// change its marker on upgrade — the exact untagged-by-default behavior this is
+// supposed to preserve. "" (neither set, or StatusCheckName still the default)
+// reproduces the untagged marker every prior release used.
+func (c *Config) CommentMarkerTag() string {
+	if c.CommentTag != "" {
+		return c.CommentTag
+	}
+	if c.StatusCheckName != "" && c.StatusCheckName != DefaultStatusCheckName {
+		return c.StatusCheckName
+	}
+	return ""
+}
+
 // AppConfigured reports whether GitHub App write credentials are set (a client id
 // and a private key). The installation is auto-resolved from the repo, so it's not
 // part of the credential. A partial config (one without the other) is not

internal/config/config_test.go

@@ -205,6 +205,67 @@ func TestLoad_ForceGenericProvider(t *testing.T) {
 	}
 }
 
+// TestCommentMarkerTag covers the fallback chain: an explicit CommentTag wins,
+// otherwise a genuinely custom StatusCheckName is reused, otherwise "" —
+// reproducing the untagged marker every prior release used. StatusCheckName
+// still holding DefaultStatusCheckName does NOT count as "custom" — Load fills
+// a blank one in with that constant (see TestLoad_CommentMarkerTag), so treating
+// it as a real tag would identically tag every single-instance install.
+func TestCommentMarkerTag(t *testing.T) {
+	t.Parallel()
+	tests := []struct {
+		name            string
+		commentTag      string
+		statusCheckName string
+		want            string
+	}{
+		{"neither set: no tag", "", "", ""},
+		{"StatusCheckName still the default: no tag", "", DefaultStatusCheckName, ""},
+		{"falls back to a custom StatusCheckName", "", "Konflate (dev-app)", "Konflate (dev-app)"},
+		{"explicit CommentTag wins over StatusCheckName", "dev-app", "Konflate (prod-app)", "dev-app"},
+		{"explicit CommentTag alone", "dev-app", "", "dev-app"},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			t.Parallel()
+			c := &Config{CommentTag: tt.commentTag, StatusCheckName: tt.statusCheckName}
+			if got := c.CommentMarkerTag(); got != tt.want {
+				t.Errorf("CommentMarkerTag() = %q, want %q", got, tt.want)
+			}
+		})
+	}
+}
+
+// TestLoad_CommentMarkerTag is the regression Greptile caught: Load fills a
+// blank StatusCheckName in with DefaultStatusCheckName before any caller sees
+// it, so a naive "fall back to StatusCheckName" would tag a bare single-instance
+// install's marker on every render — the compatibility break this whole feature
+// is supposed to avoid. Constructing Config directly (as TestCommentMarkerTag
+// does) can't catch this; only the real Load path can.
+func TestLoad_CommentMarkerTag(t *testing.T) {
+	t.Setenv("KONFLATE_REPO", "github://owner/repo")
+
+	cfg, err := Load()
+	if err != nil {
+		t.Fatalf("Load(): %v", err)
+	}
+	if cfg.StatusCheckName != DefaultStatusCheckName {
+		t.Fatalf("StatusCheckName = %q, want Load to fill it with %q", cfg.StatusCheckName, DefaultStatusCheckName)
+	}
+	if got := cfg.CommentMarkerTag(); got != "" {
+		t.Errorf("CommentMarkerTag() = %q, want \"\" for a bare install (Load-filled default StatusCheckName must not count as a tag)", got)
+	}
+
+	t.Setenv("KONFLATE_STATUS_CHECK_NAME", "Konflate (dev-app)")
+	cfg, err = Load()
+	if err != nil {
+		t.Fatalf("Load() with a custom status check name: %v", err)
+	}
+	if got := cfg.CommentMarkerTag(); got != "Konflate (dev-app)" {
+		t.Errorf("CommentMarkerTag() = %q, want the explicitly configured status check name", got)
+	}
+}
+
 // TestAuthenticatedSources verifies forge read auth is recognized from either a
 // read token or a complete GitHub App (whose installation token authenticates
 // reads). A write-only PAT or a partial App config does not count.

internal/server/comment.go

@@ -77,7 +77,8 @@ func newCommentTemplate(cfg *config.Config, log *slog.Logger) *template.Template
 func (s *Server) commentBody(env api.DiffEnvelope) string {
 	reviewURL := s.reviewURL(env.PR.Number)
 	admonitions := s.cfg.Forge.Kind == config.ForgeGitHub
-	defaultBody := func() string { return summaryMarkdown(env, reviewURL, admonitions, s.Version) }
+	tag := s.cfg.CommentMarkerTag()
+	defaultBody := func() string { return summaryMarkdown(env, reviewURL, admonitions, s.Version, tag) }
 	if s.commentTmpl == nil {
 		return defaultBody()
 	}
@@ -116,16 +117,19 @@ func (s *Server) commentBody(env api.DiffEnvelope) string {
 			"pr", env.PR.Number, "error", err)
 		return defaultBody()
 	}
-	return ensureMarker(env.PR.Number, b.String())
+	return ensureMarker(env.PR.Number, tag, b.String())
 }
 
 // ensureMarker guarantees the konflate marker is in body so comment write-back can
 // find and edit the comment; a custom template needn't include it. The marker is a
-// hidden HTML comment, so prepending it is invisible in the rendered comment.
-func ensureMarker(number int, body string) string {
-	marker := konflateMarker(number)
+// hidden HTML comment, so prepending it is invisible in the rendered comment. Any
+// other konflate marker already in body (a custom template that embeds one
+// verbatim) is stripped first — see stripAnyMarker — so exactly one, correct
+// marker survives.
+func ensureMarker(number int, tag, body string) string {
+	marker := konflateMarker(number, tag)
 	if strings.Contains(body, marker) {
 		return body
 	}
-	return marker + "\n" + body
+	return marker + "\n" + stripAnyMarker(number, body)
 }

internal/server/comment_test.go

@@ -41,7 +41,7 @@ func newCommentServer(t *testing.T, src string) *Server {
 func TestCommentBody_DefaultSummary(t *testing.T) {
 	t.Parallel()
 	body := newCommentServer(t, "").commentBody(readyEnvelope())
-	if !strings.HasPrefix(body, konflateMarker(7)) {
+	if !strings.HasPrefix(body, konflateMarker(7, "")) {
 		t.Errorf("default body should start with the marker: %q", body)
 	}
 	if !strings.Contains(body, "<sub>konflate · rendered") {
@@ -53,14 +53,14 @@ func TestCommentBody_CustomTemplate(t *testing.T) {
 	t.Parallel()
 	body := newCommentServer(t, "## {{ .PR.Title }} (#{{ .PR.Number }})\n{{ .Summary }}").commentBody(readyEnvelope())
 	// The marker is injected even though the template never mentions it.
-	if !strings.HasPrefix(body, konflateMarker(7)) {
+	if !strings.HasPrefix(body, konflateMarker(7, "")) {
 		t.Errorf("custom body should be prefixed with the marker: %q", body)
 	}
 	if !strings.Contains(body, "## bump nginx (#7)") {
 		t.Errorf("custom body missing the rendered header: %q", body)
 	}
 	// {{ .Summary }} embeds the marker-less default, so exactly one marker total.
-	if n := strings.Count(body, konflateMarker(7)); n != 1 {
+	if n := strings.Count(body, konflateMarker(7, "")); n != 1 {
 		t.Errorf("expected exactly one marker, got %d: %q", n, body)
 	}
 	if !strings.Contains(body, "<sub>konflate · rendered") {
@@ -109,7 +109,7 @@ func TestCommentBody_TemplateMdFuncEscapesRawDiffText(t *testing.T) {
 func TestCommentBody_CustomTemplateWithoutSummary(t *testing.T) {
 	t.Parallel()
 	body := newCommentServer(t, "a custom note for #{{ .PR.Number }}").commentBody(readyEnvelope())
-	if !strings.HasPrefix(body, konflateMarker(7)+"\n") {
+	if !strings.HasPrefix(body, konflateMarker(7, "")+"\n") {
 		t.Errorf("marker should be prepended: %q", body)
 	}
 	if !strings.Contains(body, "a custom note for #7") {
@@ -123,7 +123,7 @@ func TestCommentBody_SectionsPlacedIndividually(t *testing.T) {
 	body := newCommentServer(t, "## Cautions\n{{ .Sections.Cautions }}\n\n## Images\n{{ .Sections.Images }}").
 		commentBody(sampleSummaryEnv())
 
-	if !strings.HasPrefix(body, konflateMarker(142)) {
+	if !strings.HasPrefix(body, konflateMarker(142, "")) {
 		t.Errorf("marker should be injected: %q", body)
 	}
 	if !strings.Contains(body, "[!WARNING]") || !strings.Contains(body, "Deployment web/api") {
@@ -148,23 +148,45 @@ func TestCommentBody_ExecuteErrorFallsBackToDefault(t *testing.T) {
 	if !strings.Contains(body, "<sub>konflate · rendered") {
 		t.Errorf("a failing template should fall back to the default summary: %q", body)
 	}
-	if !strings.Contains(body, konflateMarker(7)) {
+	if !strings.Contains(body, konflateMarker(7, "")) {
 		t.Errorf("fallback body missing the marker: %q", body)
 	}
 }
 
 func TestEnsureMarker(t *testing.T) {
 	t.Parallel()
-	m := konflateMarker(7)
-	if got := ensureMarker(7, "hello"); got != m+"\nhello" {
+	m := konflateMarker(7, "")
+	if got := ensureMarker(7, "", "hello"); got != m+"\nhello" {
 		t.Errorf("ensureMarker should prepend the marker: %q", got)
 	}
 	already := "lead\n" + m + "\nbody"
-	if got := ensureMarker(7, already); got != already {
+	if got := ensureMarker(7, "", already); got != already {
 		t.Errorf("ensureMarker should not duplicate an existing marker: %q", got)
 	}
 }
 
+// TestEnsureMarker_StripsStaleMarkerFromCustomTemplate covers a mixed-rollout
+// gap: a custom PR-comment template written before this feature existed could
+// easily embed the old literal "<!-- konflate:pr-{{ .PR.Number }} -->" form
+// verbatim (the contract says a template needn't include the marker, not that
+// it mustn't). Once this instance has a tag, that stale untagged marker must
+// not survive alongside the new tagged one — left in place, an older or
+// differently-tagged instance could still Contains-match and overwrite this
+// comment.
+func TestEnsureMarker_StripsStaleMarkerFromCustomTemplate(t *testing.T) {
+	t.Parallel()
+	stale := konflateMarker(7, "") // the pre-feature literal a hand-written template might embed
+	body := stale + "\ncustom template body"
+	got := ensureMarker(7, "dev-app", body)
+	want := konflateMarker(7, "dev-app")
+	if !strings.Contains(got, want) {
+		t.Fatalf("result missing the current tagged marker: %q", got)
+	}
+	if strings.Contains(got, stale) {
+		t.Errorf("stale untagged marker survived alongside the tagged one, still matchable by an older instance: %q", got)
+	}
+}
+
 func TestNewCommentTemplate(t *testing.T) {
 	t.Parallel()
 	t.Run("nil when unset", func(t *testing.T) {

internal/server/handlers.go

@@ -231,7 +231,7 @@ func (s *Server) handleSummary(w http.ResponseWriter, r *http.Request) {
 		} else {
 			w.WriteHeader(http.StatusOK)
 		}
-		_, _ = io.WriteString(w, summaryMarkdown(env, env.ReviewURL, flavor == "github", s.Version))
+		_, _ = io.WriteString(w, summaryMarkdown(env, env.ReviewURL, flavor == "github", s.Version, s.cfg.CommentMarkerTag()))
 		return
 	}
 	code := http.StatusOK

internal/server/markdown.go

@@ -1,6 +1,8 @@
 package server
 
 import (
+	"crypto/sha256"
+	"encoding/hex"
 	"fmt"
 	"net/http"
 	"regexp"
@@ -9,19 +11,52 @@ import (
 	"github.com/home-operations/konflate/internal/api"
 )
 
-// konflateMarker is the hidden HTML comment tagging konflate's own PR comment,
-// so comment write-back can find and edit it in place instead of posting a new
-// one on each render. summaryMarkdown embeds it at the top; the forge Writer
-// matches a comment body against it.
-func konflateMarker(number int) string {
-	return fmt.Sprintf("<!-- konflate:pr-%d -->", number)
+// commentTagHashLen is the number of hex characters of the tag's sha256 kept in
+// the marker — short enough to stay unobtrusive in a hidden HTML comment, long
+// enough (48 bits) that two distinct operator-chosen tags colliding is not a
+// realistic concern at any real fleet size.
+const commentTagHashLen = 12
+
+// konflateMarker is the hidden HTML comment tagging konflate's own PR comment, so
+// comment write-back can find and edit it in place instead of posting a new one on
+// each render. tag disambiguates this konflate instance's comment from another
+// instance's on the same PR (see config.CommentMarkerTag); empty reproduces the
+// untagged marker every prior release used.
+//
+// tag is hashed rather than embedded verbatim: it's operator config (a
+// StatusCheckName or CommentTag), not PR content, but two visually distinct tags
+// (e.g. "dev/app" and "dev app") could otherwise sanitize down to the identical
+// safe-charset string and silently reintroduce the exact cross-instance collision
+// this exists to prevent. A hash sidesteps both that and the HTML-comment-breakout
+// concern (an embedded "-->") in one step — the marker is hidden, so it never
+// needs to be human-readable.
+func konflateMarker(number int, tag string) string {
+	if tag == "" {
+		return fmt.Sprintf("<!-- konflate:pr-%d -->", number)
+	}
+	sum := sha256.Sum256([]byte(tag))
+	return fmt.Sprintf("<!-- konflate:pr-%d:%s -->", number, hex.EncodeToString(sum[:])[:commentTagHashLen])
+}
+
+// stripAnyMarker removes any konflate marker for number from body — tagged or
+// not, and regardless of which tag. ensureMarker calls this before prepending
+// the current marker so a custom template that already embeds a marker
+// verbatim (the documented contract says a template needn't include one, not
+// that it mustn't — an operator could easily have copied the old
+// "<!-- konflate:pr-{{ .PR.Number }} -->" form from before this feature
+// existed) ends up with exactly one marker: the current, correct one. Left
+// in place, a stale marker would let an older or differently-tagged instance
+// still Contains-match and overwrite this comment during a mixed rollout.
+func stripAnyMarker(number int, body string) string {
+	re := regexp.MustCompile(fmt.Sprintf(`<!-- konflate:pr-%d(?::[0-9a-f]+)? -->\n?`, number))
+	return re.ReplaceAllString(body, "")
 }
 
 // summaryMarkdown renders a PR's diff summary as a paste-ready Markdown block for
 // posting back onto the pull request, prefixed with the konflate marker (a hidden
 // HTML comment) so a poster can find and edit its own comment in place.
-func summaryMarkdown(env api.DiffEnvelope, reviewURL string, admonitions bool, version string) string {
-	return konflateMarker(env.PR.Number) + "\n" + summaryMarkdownBody(env, reviewURL, admonitions, version)
+func summaryMarkdown(env api.DiffEnvelope, reviewURL string, admonitions bool, version, tag string) string {
+	return konflateMarker(env.PR.Number, tag) + "\n" + summaryMarkdownBody(env, reviewURL, admonitions, version)
 }
 
 // summaryMarkdownBody is the marker-less summary body. It carries no heading —
@@ -495,8 +530,8 @@ func shortVer(v string) string {
 	if i < 0 {
 		return v
 	}
-	if hex := v[i+1:]; len(hex) > 6 && isHex(hex) {
-		return v[:i+1] + hex[:6] + "…"
+	if digest := v[i+1:]; len(digest) > 6 && isHex(digest) {
+		return v[:i+1] + digest[:6] + "…"
 	}
 	return v
 }