From 4bc2bb68182178c8375c0cbe13521210c1f1d768 Mon Sep 17 00:00:00 2001 From: Smaine Kahlouch Date: Mon, 24 Aug 2026 18:34:59 +0200 Subject: [PATCH] fix(notify): stop cutting "what changed" mid-word MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The field was capped at 200 runes with the generic truncate helper, so a live card ended "…consistent with Coder provisioning workspaces outside Argo/Fl…". That reads as a broken renderer rather than an intentional elision, and 200 was tighter than what models actually put in change_ref: some cards carry a revision range, others a sentence explaining the change, and the explanatory form is the more useful one. truncateWords backs the cut off to the last space, and the field rises to 600 runes. The cap stays enforced — a tail with no space in its final half is one long token (a sha, a URL) and still gets the hard cut rather than losing half its value. The scan starts at the FIRST DROPPED rune, not the last kept one, which is what makes the word-boundary case ordinary rather than special: when the cap lands exactly on a space the prefix already ends on a boundary, and starting there finds it instead of scanning past a whole word that fitted. The live fixture is that case — at 600 it cuts right after "the", and the first attempt threw that word away. The value is capped BEFORE escaping, reversing this file's other two truncate sites deliberately. Those cap at 2900, a last-resort guard where it does not matter that "&" costs 5 of the budget. This cap fires routinely, so measuring escaped runes would hand a change_ref describing "1.14.2 -> 1.15.0 && " a materially smaller allowance than one without meta characters — and capping the source also means truncateWords only ever sees real prose, so no cut can sever an entity. n <= 1 is guarded: no caller passes it today, but the doc invites reuse and a caller deriving n from a remaining budget reaches 0 by subtraction, where r[:n-1] panicked. A panic in the notifier takes down the card for an incident already in progress. The guard is measured, not asserted: TestTruncateWords is the unit table covering the hard cut, the exact boundary and the degenerate caps, and the card-level test now measures the rendered FIELD. An earlier version asserted len(blocksText(...)) > 260, which the header, verdict and footer satisfy on their own — it passed with the cap set to 60, so the half of the guard meant to pin the raised cap pinned nothing. Mutation-tested against caps of 60 and 200 and against reverting to plain truncate. internal/thread.truncateWords is the byte-budget twin of this function; both doc comments now say so. --- internal/notify/slack.go | 60 +++++++++++++++++- internal/notify/slack_test.go | 111 ++++++++++++++++++++++++++++++++++ 2 files changed, 170 insertions(+), 1 deletion(-) diff --git a/internal/notify/slack.go b/internal/notify/slack.go index 14fbdd65..ae6ad00a 100644 --- a/internal/notify/slack.go +++ b/internal/notify/slack.go @@ -15,6 +15,7 @@ import ( "os" "strings" "time" + "unicode" "github.com/Smana/runlore/internal/config" "github.com/Smana/runlore/internal/httpx" @@ -820,7 +821,14 @@ func metadataFields(inv providers.Investigation) []map[string]any { // deploy, which is the first thing they should check. if len(inv.RootCauses) > 0 { if ch := inv.RootCauses[0].ChangeRef; ch != "" { - add("What changed", truncate(escapeMrkdwn(ch), 200)) + // Capped BEFORE escaping, which is the reverse of this file's other two + // truncate sites and deliberate. Those cap at 2900 — a last-resort guard on + // a pathological payload — where it does not matter that "&" costs 5 of the + // budget. This cap fires routinely, so measuring escaped runes would hand a + // change_ref describing "1.14.2 -> 1.15.0 && " a materially smaller + // allowance than one without meta characters. Capping the source also means + // truncateWords only ever sees real prose, so no cut can sever an entity. + add("What changed", escapeMrkdwn(truncateWords(ch, slackWhatChangedRunes))) } } if inv.Occurrences > 1 && inv.Prior == nil { @@ -1058,6 +1066,56 @@ func truncate(s string, n int) string { return string(r[:n-1]) + "…" } +// slackWhatChangedRunes caps the "What changed" metadata field. 200 was tighter +// than what models actually put in change_ref: it carries a revision range on some +// cards and a sentence explaining the change on others, and the explanatory form is +// the more useful of the two. 600 holds it while still leaving the two-column +// metadata grid readable, and stays well inside the 2000-rune per-field cap add() +// applies. +const slackWhatChangedRunes = 600 + +// truncateWords caps a string to n runes like truncate, but backs the cut off to +// the last space so the ellipsis never lands inside a word. Use it for prose the +// model wrote; use truncate for values that are one token (a ref, a URL, a sha) +// or where the cap is a hard protocol limit. +// +// A mid-word cut reads as a rendering fault rather than an intentional elision — +// live, a change_ref ended "…outside Argo/Fl…", which looks like the card broke. +// The cap itself is still enforced: a tail with no space in its final half is one +// long token, and that gets the hard cut rather than losing half the value. +// +// Call it on RAW text and escape the result, not the other way round. It counts +// runes of content, and a cut through escaped text can sever an "&" into a +// visible "&am" — the same rendering fault, in the one branch the back-off cannot +// reach, since a long token has no space to back off to. +// +// internal/thread.truncateWords is the byte-budget twin of this function, applying +// the same rule to the KB validator's byte limits (and trimming dangling +// punctuation, which Slack does not need). Keep the two in step. +func truncateWords(s string, n int) string { + r := []rune(s) + if len(r) <= n { + return s + } + if n < 2 { + return "" // no room for content and a mark both + } + // The scan starts at the FIRST DROPPED rune, not the last kept one. That is what + // makes the word-boundary case ordinary rather than special: when r[n-1] is + // itself a space the prefix already ends on a boundary, and starting there finds + // it instead of scanning past a whole word that fitted. Bounded at half the + // allowance — a tail with no space in it is one long token (a sha, a URL), and + // backing off to a distant space would lose more than the cut does. + cut := r[:n-1] + for i := n - 1; i > n/2; i-- { + if unicode.IsSpace(r[i]) { + cut = r[:i] + break + } + } + return strings.TrimRightFunc(string(cut), unicode.IsSpace) + "…" +} + // Multi delivers to several notifiers, best-effort: a failing notifier is logged, // not propagated, so one bad sink doesn't block the others. type Multi struct { diff --git a/internal/notify/slack_test.go b/internal/notify/slack_test.go index d150182b..a75521ef 100644 --- a/internal/notify/slack_test.go +++ b/internal/notify/slack_test.go @@ -1838,3 +1838,114 @@ func TestFormatCarriesTheNoRemedyNotice(t *testing.T) { t.Errorf("Format printed the notice over a supplied remedy:\n%s", out) } } + +// TestTruncateWords is the unit table for the helper itself. The card-level test +// below drives one realistic fixture through summaryBlocks; this covers the branches +// that fixture can never reach — the hard cut, the exact-boundary case, a severed +// entity, and the degenerate caps. +func TestTruncateWords(t *testing.T) { + cases := []struct { + name, in string + n int + want string + }{ + {"short enough is returned whole", "hello world", 20, "hello world"}, + {"exactly n is not truncated", "hello", 5, "hello"}, + {"backs off to the word boundary", "hello world foo", 12, "hello world…"}, + // The cut lands on a space, so the prefix is ALREADY word-aligned: backing off + // further would drop "bb", a whole word that fitted. + {"a cut on the boundary keeps the last whole word", "aaaa bb cc", 8, "aaaa bb…"}, + // No space in the final half — one long token (a sha, a URL). The cap wins; + // losing half the value to reach a distant space would be worse. + {"one long token still gets the hard cut", "abcdefghijklmnopqrstuvwxyz", 10, "abcdefghi…"}, + {"trailing space is trimmed before the ellipsis", "hello world xx", 13, "hello world…"}, + {"a cap with no room for content and a mark", "hello", 1, ""}, + {"n=0 does not panic", "hello", 0, ""}, + {"n negative does not panic", "hello", -3, ""}, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + if got := truncateWords(c.in, c.n); got != c.want { + t.Errorf("truncateWords(%q, %d) = %q, want %q", c.in, c.n, got, c.want) + } + }) + } +} + +// whatChangedField returns the rendered "What changed" metadata value, without its +// label. Asserting on the field rather than the whole encoded blob is what keeps the +// cap assertions honest. +func whatChangedField(t *testing.T, inv providers.Investigation) string { + t.Helper() + for _, f := range metadataFields(inv) { + txt, _ := f["text"].(string) + if after, ok := strings.CutPrefix(txt, "*What changed:*\n"); ok { + return after + } + } + t.Fatalf("no What changed field rendered") + return "" +} + +// TestSlackWhatChangedTruncatesOnWordBoundary: a long change_ref is cut at a word +// boundary, never mid-word. +// +// Live card, 2026-08-24: the field ended "…consistent with Coder provisioning +// workspaces outside Argo/Fl…". Two things were wrong at once — the 200-rune cap +// was tighter than the explanatory text models actually put in change_ref, and the +// cut landed inside a word, which reads as a rendering fault rather than an +// intentional elision. The cap is a limit and stays enforced; where it falls is +// what changed. +func TestSlackWhatChangedTruncatesOnWordBoundary(t *testing.T) { + long := "argocd Application coder-shared sync to bc20dc3a at 09:17:46Z, which recreated the whole " + + "shared-0 instance directory and restarted the singleton coder Deployment with a newly mounted " + + "kubeconfig ConfigMap plus new environment variables, consistent with Coder provisioning " + + "workspaces outside Argo CD rather than through the tracked GitOps path, and additionally " + + "retargeted the provisioner daemon concurrency, rewrote the access URL, replaced the instance " + + "kustomization, and rolled every workspace template that referenced the previous access URL " + + "so that the migrated instance would come up against the new control plane endpoint" + inv := providers.Investigation{ + Title: "t", + RootCauses: []providers.Hypothesis{{Summary: "s", ChangeRef: long}}, + } + got := whatChangedField(t, inv) + + // The cap must be materially larger than the 200 it replaced. Measured on the + // FIELD, so lowering the cap fails this — asserting on the whole encoded blocks + // blob did not: the header, verdict and footer satisfied it on their own, and it + // passed with the cap set to 60. + if n := len([]rune(strings.TrimSuffix(got, "…"))); n <= 200 { + t.Errorf("the cap is too tight to hold an explanatory change_ref: %d runes\n%s", n, got) + } + // The cap is still a cap. + if n := len([]rune(got)); n > 600 { + t.Errorf("the cap is not enforced: %d runes", n) + } + + // This fixture reaches the cap exactly ON a space — the case the first revision + // got wrong, scanning back past the boundary and dropping "the", a whole word + // that fitted. The exact suffix pins the cut point, the word boundary and the + // absence of a stray space before the ellipsis in one assertion. + if !strings.HasSuffix(got, "against the…") { + t.Errorf("a cut landing on a word boundary must keep the last whole word, got tail %q", + got[max(0, len(got)-24):]) + } + + // Shifted so the cap lands INSIDE a word, which is what distinguishes truncateWords + // from the plain truncate this field used to call. Without the shift both helpers + // return the same string and nothing pins the wiring. + inv.RootCauses[0].ChangeRef = "sync: " + long + shifted := whatChangedField(t, inv) + if !strings.HasSuffix(shifted, "come up…") { + t.Errorf("expected a back-off to the word boundary, got tail %q, want a mid-word hard cut to be avoided", + shifted[max(0, len(shifted)-24):]) + } + + // The raised cap must not start truncating the revision-range form, which is the + // other shape change_ref carries — and it must still be escaped. + const short = "flux: HelmRelease harbor 1.14.2 -> 1.15.0 " + inv.RootCauses[0].ChangeRef = short + if got := whatChangedField(t, inv); got != escapeMrkdwn(short) { + t.Errorf("a short change_ref must render verbatim and escaped:\ngot %q\nwant %q", got, escapeMrkdwn(short)) + } +}