fix(notify)!: make the Slack silence control legible, public, and durable - #559
Merged
Conversation
The 🔕 control shipped working but unusable: Slack's overflow element takes no label so it renders as a bare `···`, the confirmation is ephemeral so only the clicker sees it, and the card is never touched so scrollback cannot tell a handled finding from an unhandled one. Spec swaps the element for a static_select with a placeholder, and rewrites the card in place on click so the acknowledgement is public and durable in one stroke. The hard invariant it records: replace_original today means "overwrite with plain text", so a naive rewrite would blank the investigation. The rebuild is conditional on usable blocks and falls back to the current ephemeral note. The card is worth more than the marker.
…time Both tests recorded a 24h silence at a hardcoded 2026-08-25 13:00 UTC and then reloaded the ledger. Ledger.New and the checkpoint writer both call pruneLapsedSilencesLocked(time.Now()), so from 2026-08-26 13:00 UTC onward the replay legitimately dropped the silence and the assertions failed — a time bomb that fired 24h after the tests were written, on a clean tree with no code change. TestSilenceSurvivesCompaction in the same file already anchors to time.Now() for exactly this reason; these two just missed the convention. Only the tests that RELOAD are affected: the prune runs at load and at checkpoint, never inline, so the sibling tests that keep one ledger in memory are correct as written.
Slack's overflow element takes no text, so the 🔕 control drew as a bare "···" beside 👍 Accurate / 👎 Off-base. The 🔕 and the word "Silence" appeared only inside the menu — after you had clicked something you could not identify. That was the first thing the first live use of the feature exposed, 2026-08-26. A static_select is the one actions-block element that carries both a visible label and a menu, so the control now renders with a "🔕 Silence…" placeholder. Buttons-per-window would scale to seven controls at five presets, and a button-plus-modal would need views.open, a trigger_id and a submission handler — a new failure surface for a cosmetic win. No migration is needed and none is performed: Slack delivers the chosen option at actions[0].selected_option.value for a static_select exactly as it did for an overflow, so cards already in scrollback stay clickable. That is the load-bearing claim behind not writing dual-shape parsing, so it is now pinned by TestSilenceReadsAStaticSelectPayload rather than merely asserted in review. The 2-option floor stays but stops being a Slack limit: Slack accepts a select with one option, and a one-entry dropdown is simply a button wearing a menu. The card golden moves, so hack/check-screenshots-fresh.sh's acknowledgement is re-pointed at the new digest. This is the one card change of the four so far that a reader of the shipped screenshots would actually notice; retaking them still needs the live workspace whose credentials sit outside this repo.
Both window-count errors explained themselves with Slack's overflow element: "at most 5 options", "requires at least 2", each promising invalid_blocks and an undelivered finding. That was true while the 🔕 control was an overflow. It is a static_select now, and Slack accepts a select with one option and with six — so the errors sent the reader looking for a Slack limit that is no longer there. The bounds themselves stay, because they were always the right numbers for the wrong stated reason: the 🔕 menu is a quick choice on a card someone is reading at 3am, so one option is a button wearing a menu and six is a form. The messages now say that instead. The floor stays scoped to slack.silence_button and the ceiling stays unconditional, unchanged — those scopes never depended on the element type. TestSilenceWindowBoundsDoNotBlameSlack pins the absence, since a stale justification is exactly the kind of thing that survives every future edit unnoticed. The existing table's base config is extracted to silenceTestConfig so both tests build "a minimal valid silence config" from one definition.
The Slack card is never touched when someone silences an investigation, so a handled finding is byte-identical to an unhandled one in scrollback. Stamping the card needs a line, and SilenceAck is the wrong line: it is three sentences aimed at the person who just clicked, explaining what they have switched off. SilenceMarker addresses the other reader — a colleague scanning the channel a day later, who needs only "has anyone already dealt with this?". It stays on one line because it renders in a context block, and it @-prefixes the user so Slack draws a mention. Shared here beside SilenceAck rather than spelled in internal/notify or internal/server for the same reason ShortDuration is: one wording, one place.
silencedCard removes the 🔕 control from the card the click came from and appends a marker context block, so a handled finding stops looking identical to an unhandled one. 👍/👎 are kept: a 👎 re-arms a silenced trigger, and SilenceAck names that as the escape hatch. The refusal is the load-bearing half. The rebuilt card is posted with replace_original: true, which overwrites the Slack message, so a rebuild that guessed would replace the investigation with a lone "🔕 Silenced by …" line and lose the finding it was marking. A naive version was written first to see exactly that: with no blocks in the payload it returned a card consisting only of the marker, and with an actions block holding the control alone it returned `elements: []`, which Slack rejects outright. Both now return false, and the caller falls back to today's ephemeral note. Rewriting is keyed on the block type being "actions" rather than on the presence of an "elements" array, because a context block carries one too. message.blocks is decoded as raw maps: Block Kit has dozens of element types and a typed decode would drop the fields it did not know about — straight out of a card about to be posted back verbatim.
A silence recorded fine but told almost nobody. slackResponseBody marks any non-replacing answer response_type: ephemeral, and silence went down the feedback path, so "🔕 Silenced by @x" reached the clicker alone — a colleague could open the same finding minutes later and start investigating something already handled. The card itself was never touched, so scrollback could not distinguish a handled finding from an unhandled one at all. Both are now fixed by the same move: the card is rebuilt in place through the response_url that was already wired and already SSRF-guarded, so the marker is public by construction and survives in scrollback. chat.update would have needed a new scope; the interaction payload already echoes message.blocks back. Two orderings are load-bearing and both were watched failing first: - The rebuild's refusal is honoured. Wired naively it posted {"replace_original": true, "blocks": null} for a payload carrying no blocks, which tells Slack to overwrite the message with nothing. The finding, its evidence and its next steps, gone, in exchange for a marker about them. - silenced/marker are set only after s.silence.Silence returns nil. Set before, a failing ledger write still stamped "🔕 Silenced by @bob until … · 48h" on the card while storing nothing — the channel told the incident was suppressed while RunLore went on investigating it. updateSlack's validate-and-POST tail becomes postSlackResponse, shared with the new updateSlackBlocks, so the response_url guard has exactly one implementation. The client it uses moves onto the Server: it is worth pooling, and substituting a transport is the only way a test can see what was posted — the guard accepts no host but *.slack.com, so an httptest server is refused before any request.
The Slack page called the control "a 🔕 overflow menu" and attributed the 2-5 window bound to Slack answering a non-conforming block by rejecting the whole message. Neither is true since the control became a labelled static_select: the bound is a UX judgement now, and a reader chasing the stated Slack limit would not find one. configuration.md and the chart comment carried the same claim, and the Matrix page described its reaction as the equivalent of Slack's overflow. The Slack page also gains the paragraph the feature had no documentation for at all: what a click DOES to the card — control removed, a public line naming who silenced it and until when, 👍/👎 kept so the escape hatch survives, and the untouched-card fallback when Slack sends no blocks back.
The rewrite posted replace_original with blocks alone. Slack clears every field the replacing payload omits, so the card's top-level "text" — the one-line summary notify.fallbackText builds for push notifications and block-less clients — was silently dropped. Invisible on screen, which is what makes it worth a test: the card still looked right in the channel while every later notification for that message, and every screen reader, got nothing. The interaction payload echoes the field back, so it is carried over verbatim.
…t exists Five cleanups from a quality pass over the branch, no behaviour change. silencedCard's `len(blocks) == 0` guard was dead: with no blocks the loop body never runs, `out` stays empty, and the `len(out) == 0` check at the bottom already returns the same (nil, false). Two exits for one condition, and a reader had to compare them to find they agreed. Its hand-rolled map copy becomes maps.Clone, which is what internal/source/grafana already uses for the same shallow-copy-then-mutate shape. The test scaffolding had grown three constructors where one belongs — two were one-line pass-throughs — plus a slackCapture struct carrying two values that are compile-time constants at every call site. And three senders, of which sendSilenceCardWithText duplicated sendSilence's sign-and-serve tail byte for byte. The signing boilerplate now has one copy in postSignedInteraction, so a change to how an interaction is authenticated lands in one place. TestSilenceControlIsLabelledNotABareOverflow walked the block shape by hand to find the control, which is exactly what findSilenceControl exists for — in the one file whose stated point is that callers should not care how the control is drawn. Its "no element is an overflow" sweep goes with it: 👍/👎 are buttons, so the control is the only element that could regress, and the static_select assertion three lines down already says so. Last, a comment that asserted something untrue. slackHTTP was justified partly as "frequent enough to be worth a pooled connection", but httpx.SecureClient leaves Transport nil, so every client it ever returned already shared http.DefaultTransport's process-wide idle pool. Hoisting it saves one struct allocation per interaction and nothing else; the test seam is the real reason, and the comment now says only that — plus why the pooling story is wrong, so it does not get re-added.
…tion Two defects in the silence rewrite, both found by review of the branch. The acknowledgement was being built and thrown away. msg = thread.SilenceAck(…) is assembled on the success path, but the card rewrite returned before anything posted it, so the only answer a clicker got was the public marker. The marker says who and until when; it does not say that RunLore will NOT investigate this incident — no model call, no notification, no record — and it does not name the escape hatches this deployment actually has. That last part is the whole of the #556 honesty fix, including its startup warning, and it had become reachable only on the blocks-missing fallback: exactly the feedback_buttons:false operator who most needs telling was the one no longer told. The two are different answers to different readers, and response_url takes five, so the success path now sends both — the rebuilt card, then the ephemeral ack. Only the `return` moved; the refusal path is unchanged and still degrades to the ack alone. The marker's mention was not a mention. It formatted "@%s" with the username, and a bare @bob inside a Block Kit mrkdwn element renders as literal text — link_names is a chat.postMessage option with no effect on blocks. So the marker named the silencer without notifying them or linking their profile, which is most of the reason it names them. <@u9> is the one form Slack linkifies there and p.User.ID was already in scope at the call site. SilenceMarker now takes the ref verbatim rather than a name it prefixes: the transport owns its own mention syntax, the same split as thread.RenderReply taking an escaper. That also stops internal/thread claiming, as its comment did, that a leading @ is enough. Both were watched failing first. TestSilenceStillWarnsTheClicker asserts the card rewrite and the ack are both posted; TestSilenceMarkerMentionsTheUserByID reads the DECODED payload, because encoding/json escapes < and > on the wire and an assertion over a re-marshalled dump would fail a correct mention and pass a broken one. captureSlackResponse records every post instead of the last, since one answer per interaction is no longer the shape. slack.md said the public line was visible to the channel "rather than to the clicker alone", which read as the marker replacing the private ack. It is in addition to it, and the page now says so.
… silence
Everything here descends from one misplacement: rewriting the card put Block Kit
construction into internal/server, which may not import internal/notify. The
renderer's own helpers were on the far side of that line, so the rewrite could
not call them — and three separate defects are what "could not call them" looked
like in production.
internal/slackcard is a leaf package holding what the card's BUILDER and its
REWRITER both need: the action ids, the block_id encoding, EscapeMrkdwn, Date,
UserMention, and the rewrite walk itself. The layering rule is intact — server
still does not import notify — but the duplication it used to cost is gone. The
action ids were bare literals in both packages; the block_id prefix existed twice
behind a test that PARSED the other package's source to compare them. That test
is deleted: the compiler checks it now.
With Date reachable, the marker's expiry is a Slack date token like every other
time on the card, instead of a timestamp in whatever zone the pod runs in —
correct-looking to the author and useless to a colleague in another timezone.
SilenceMarker takes the user ref and the time already rendered, so the transport
owns how a person and an instant are drawn and thread owns only the word order.
That is the same split RenderReply makes by taking an escaper, and it is here
because thread got both halves wrong while it rendered them itself.
The rewrite refuses when it removed nothing. It used to append the marker on the
strength of the card being non-empty, so a click on an already-rewritten card —
a second engineer on a stale view, or a click that raced the first rewrite —
stacked a second marker under the first and reported two different windows for
one finding. It also carried block_id "sil:<key>" onto the block whose control it
had just deleted, breaking the iff-rule the renderer holds and pins.
And a failure to mark the card is now TOLD to the clicker. postSlackResponse
returned nothing, so a refused rewrite or a 500 from Slack was a log line and
otherwise invisible: the silence recorded, the channel none the wiser, and the
one person who could say so walking away certain it had been announced. That is
the "a colleague investigates something already handled" defect the marker exists
to prevent, restored silently. Both ways of failing now append SilenceCardUnmarked
to the acknowledgement.
The client timeout drops from 10s to 1.2s per post. WriteHeader only records the
status — the bytes leave when the handler returns — so these posts sit inside
Slack's 3-SECOND interaction deadline, and a silence now sends two of them. 10s
could blow that budget on one slow post, which Slack answers with an
operation_timeout shown to the clicker and a retry of the whole interaction.
Smaller, from the same review: postSlackResponse tolerates a nil client rather
than panicking inside a handler for a &Server{} built by a struct literal, which
this package's tests already do; the `silenced` bool is gone, since an empty
marker was already the same fact and two variables could only disagree; and the
test fixtures quote through encoding/json instead of splicing, so a fallback
containing a quote stops silently producing a 400 that reads as a rejected card.
Every new guard was mutation-tested: dropping the !removed check, scoping the
block_id deletion to the loop-carried flag, and removing the unmarked disclosure
each fail the tests written for them.
server.go pointed at notify.silenceBlockIDPrefix, which no longer exists, and the implementation plan still described the constant-duplication rule the shared package replaced — naming a guard test that is deleted. The plan keeps its original constraint as the record of what was planned, with a note on what the implementation did instead and why.
…ribes The 2-to-5 preset bound was enforced at two different scopes: the floor only when notify.slack.silence_button was on, the ceiling always. That made sense while the numbers were Slack's — an overflow element rejected the whole message at either breach. It stopped making sense when both were reclassified as a UX judgement about the rendered dropdown, and the reclassification reached the wording without reaching the enforcement. A rule about how many options make a usable menu cannot apply to a deployment that renders no menu. Matrix silences on a bare 🔕 reaction, which carries no duration and reads windows[0] and nothing else, so the second preset is exactly as unreachable there as the sixth. The ceiling's defence — that it costs a Matrix-only deployment nothing it could reach anyway — was equally true of the floor, which is the tell that one rule was pretending to be two. Both ends now sit under silence_button, in one check with one message. The cost, stated rather than discovered: a Matrix-only config may now hold a preset list Slack would refuse, and turning silence_button on later fails at startup instead of silently drawing a bad menu. That is the better failure — loud, at boot, naming the key and the flag — and it is what the error text points at. Scoping only ever ACCEPTS configs that were previously rejected, so no working deployment can break on upgrade. The mirror test of the existing "a single preset is fine for Matrix alone" pins the other end, and both fail if the condition is removed. BREAKING CHANGE: notify.silence.windows may now list more than 5 entries when notify.slack.silence_button is off. Nothing that validated before stops validating — the bound is only relaxed — but a Matrix-only deployment that adopts a longer preset list will fail validation at startup if slack.silence_button is later enabled, naming notify.silence.windows and the flag to turn off.
…le card Three findings from reviewing the previous commits. postSlackResponse closed the response body without reading it. net/http only returns a connection to the idle pool once its body reaches EOF, so every response_url POST was marking its connection dead and redoing DNS, TCP and TLS. That was survivable under a 10s timeout. Under 1.2s it is the difference between a generous bound and a coin flip: a silence makes two sequential posts inside Slack's 3s interaction budget, and a cold handshake on each is exactly how one times out AFTER Slack already applied it — telling the clicker their card went unmarked when it did not. The drain is bounded; the answer is "ok", and a body worth streaming would be a Slack bug, not something to spend the click on. It also makes the slackHTTP comment true. It reasoned that clients from httpx.SecureClient already share http.DefaultTransport's pool — correct, and irrelevant while nothing from this call site ever entered it. The bigger one: the two refusals were reported as the same thing. Silenced returned a bare false whether the card carried NO marker or someone ELSE's, and the handler said "the channel cannot tell this finding is handled" for both. On an already-rewritten card that is plainly false — the channel can see a marker, it is just the earlier window, and the ledger's latest-wins fold means the card and the suppression now disagree. The clicker was steered into writing a redundant "someone might pick this up" note while the real problem went unsaid. The log line was wrong the same way, sending an operator to hunt a payload problem that did not exist. SilenceOutcome names the three cases and the handler answers each: a failed POST and an unusable payload keep SilenceCardUnmarked, an already-rewritten card gets SilenceCardStale, which says the card shows the earlier window rather than claiming nothing is shown. It does not repeat the new window — the ack it is appended to has just stated it. Last, a third stale reference to the guard test deleted two commits ago, sitting directly above the constant it describes and naming the test as the enforcement. The previous commit caught the plan doc and server.go and missed this one.
Smana
changed the title
Review of the previous two commits. No behaviour change except the enum's zero value, which nothing reads today. The doc comments had drifted the moment Silenced stopped returning a bool: one still said "it reports false", another headed a two-item list with "Three things make it refuse", and updateSlackBlocks still pointed at "Silenced's false return". Worse, the note explaining the ordering said stale is "a card that SURVIVED and kept its control" — the exact opposite of the branch it describes, which fires when there was no control to remove. Someone debugging that path would have gone looking for a card still showing the 🔕 menu. The comment above the handler switch described two arms and called both "an unmarked card", written before the third existed. An already-rewritten card IS marked; that is why it gets its own disclosure twelve lines below. The paragraph justifying the switch contradicted the switch. SilenceOutcome gains String(), because six assertions print it with %v and "outcome = 1, want 2" is a poor failure message for a type whose whole purpose is that its two refusals are not interchangeable. SilenceNoUsableCard moves to iota 0: an outcome nobody set should be the one that discloses conservatively, not the one that suppresses every disclosure. Every sibling enum in this repo puts the neutral value first. The switch gains a default. .golangci.yml does not enable exhaustive, so a fourth outcome added later would fall through telling the clicker nothing about their card — the failure the switch exists to prevent, restored by omission. Tests: the ceiling case asserted "notify.silence.windows", a substring every windows error contains, so it would have passed on the wrong error — its mirror had already been updated to "2 to 5". The happy-path test pinned only one of the two cry-wolf strings; the second is now pinned too. The Matrix-only setup was written out three times and is now matrixOnly(). Last, the error's entry/entries branch was pinned by nothing and is gone — reworded so no pluralisation exists rather than tested into place.
The comment claimed a bare Close would make "every response_url POST redo DNS + TCP + TLS", and called that the difference between the 1.2s timeout being a generous bound and a coin flip. Measured against a local server, it is not. body bare Close drained 2 B 1 dial 1 dial 1 KB 1 dial 1 dial 4 KB 2 dials 1 dial 64 KB 2 dials 2 dials net/http has already buffered a small body by the time Close runs, so the connection goes back to the pool either way below roughly 2 KB. Slack answers a response_url POST with "ok". On the path this function actually takes, draining changes nothing — and past the 4 KB cap it changes nothing either, because the copy stops before EOF. The drain stays: it is free on the path that already reached EOF, and it is correct for the middle band where it does help. What was wrong was the justification, and this repo has spent this whole branch removing comments that assert mechanisms the code does not have — including one in this same function that claimed a pooling benefit from hoisting the client. Replacing it with a second false claim in the opposite direction is not an improvement. The numbers are in the comment so the next reader does not have to re-derive them, or trust them.
9 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The 🔕 silence control shipped working but unusable. First live use on 2026-08-26 exposed three separate problems, all in the same click:
···— Slack's overflow element takes no text, so 🔕 and the word "Silence" appeared only inside the menu, after you had clicked something you could not identifystatic_selectrendering a visible🔕 Silence…placeholderslackResponseBodymarks any non-replacing answerresponse_type: ephemeral, so "🔕 Silenced by @x" reached the clicker alone. A colleague could open the same finding minutes later and start investigating something already handledresponse_urlso the marker is public, and the privateSilenceAckstill reaches the clicker👍/👎 are deliberately kept on the silenced card: a 👎 re-arms a silenced trigger, and
SilenceAcknames that as the escape hatch.Linked issue
n/a — found in live use.
The load-bearing half is the refusal
The rebuilt card is posted with
replace_original: true, which overwrites the Slack message. A rebuild that guessed would replace the investigation — the finding, its evidence, its next steps — with a lone "🔕 Silenced by …" line, in exchange for a marker about the thing it just destroyed.A naive version was written first, to watch it happen:
elements: [], which Slack rejects outright.Both now return
falseand the caller falls back to today's ephemeral note. The card is worth more than the marker.Two orderings are load-bearing, both watched failing first:
{"replace_original": true, "blocks": null}— which tells Slack to overwrite the message with nothing.silenced/markerare set only afters.silence.Silencereturns nil. Set before, a failing ledger write still stamped "🔕 Silenced by @bob until … · 48h" on the card while storing nothing — the channel told the incident was suppressed while RunLore went on investigating it.Notable details
actions[0].selected_option.valuefor astatic_selectexactly as it did for an overflow, so cards already in scrollback stay clickable. That is the claim behind not writing dual-shape parsing, so it is pinned byTestSilenceReadsAStaticSelectPayloadrather than merely asserted in review.chat.updatewould have needed a new scope.response_urlwas already wired and already SSRF-guarded; the interaction payload already echoesmessage.blocksback.message.blocksis decoded as raw maps. Block Kit has dozens of element types and a typed decode would drop the fields it did not know about — straight out of a card about to be posted back verbatim.actions, not on the presence of anelementsarray, because a context block carries one too.textis carried through. Slack clears every field the replacing payload omits, so the fallback linenotify.fallbackTextbuilds for push notifications and block-less clients was silently dropped — invisible on screen, which is exactly what makes it worth a test: the card still looked right while every later notification for that message, and every screen reader, got nothing.updateSlack's validate-and-POST tail becomespostSlackResponse, shared with the newupdateSlackBlocks, so theresponse_urlguard has exactly one implementation.Config: the window bounds stop blaming Slack
Both window-count errors explained themselves with Slack's overflow element ("at most 5 options", "requires at least 2"), each promising
invalid_blocksand an undelivered finding. True while the control was an overflow; astatic_selectaccepts a select with one option and with six.The bounds themselves are unchanged — they were always the right numbers for the wrong stated reason. The 🔕 menu is a quick choice on a card someone is reading at 3am: one option is a button wearing a menu, six is a form. The messages now say that.
TestSilenceWindowBoundsDoNotBlameSlackpins the absence, since a stale justification is exactly the kind of thing that survives every future edit unnoticed.No config key changes meaning — no
!needed.Also in here
test(outcome): a time bomb that fired on a clean tree. Two compaction silence tests recorded a 24h silence at a hardcoded2026-08-25 13:00 UTC, then reloaded the ledger.Ledger.Newand the checkpoint writer both callpruneLapsedSilencesLocked(time.Now()), so from 2026-08-26 13:00 UTC onward the replay legitimately dropped the silence and the assertions failed — 24h after the tests were written, with no code change.TestSilenceSurvivesCompactionin the same file already anchors totime.Now(); these two just missed the convention. Only the tests that reload are affected.configuration.md, the chart comment and the Matrix page carried the same claim. The Slack page also gains the paragraph the feature had no documentation for at all: what a click actually does to the card.Review pass on the branch
/code-review+/simplifywere run over the series. Two defects it found are fixed in1e8647ae; the quality cleanups are inddb40926.The acknowledgement was built and thrown away.⚠️ RunLore will NOT investigate this incident … no model call, no notification, no record", and never the escape hatches this deployment has. That last part is the whole of #556's honesty fix, and it had become reachable only on the blocks-missing fallback: the
msg = thread.SilenceAck(…)is assembled on the success path, but the card rewritereturned before anything posted it. The clicker got only the marker — never "feedback_buttons: falseoperator who most needs telling was precisely the one no longer told.response_urltakes five answers; the success path now sends both.The marker's mention was not a mention. It formatted
"@%s"with the username, and a bare@bobinside a Block Kit mrkdwn element renders as literal text —link_namesis achat.postMessageoption with no effect on blocks.<@U9>is the one form Slack linkifies there, andp.User.IDwas already in scope.SilenceMarkernow takes the ref verbatim so the transport owns its own mention syntax — the same split asthread.RenderReplytaking an escaper.Both were watched failing first. The mention test reads the decoded payload, because
encoding/jsonescapes<and>on the wire and an assertion over a re-marshalled dump would fail a correct mention and pass a broken one.The rest of the review, fixed in
4ae089a5Every remaining finding descended from one misplacement: rewriting the card put Block Kit construction into
internal/server, which may not importinternal/notify. The renderer's own helpers were on the far side of that line, and three defects are what "could not call them" looked like.internal/slackcardis a new leaf package holding what the card's builder and its rewriter both need — action ids, theblock_idencoding,EscapeMrkdwn,Date,UserMention, and the rewrite walk. The layering rule survives (server still does not import notify) but stops being paid for in duplication: the action ids were bare literals in both packages, and theblock_idprefix existed twice behind a test that parsed the other package's source to compare them. That test is deleted — the compiler checks it now.With
Datereachable, the marker's expiry is a Slack date token instead of a timestamp in the pod's timezone: correct-looking to whoever wrote it, useless to a colleague in another zone.SilenceMarkernow takes the ref and the time already rendered, so the transport owns how a person and an instant are drawn — the same splitRenderReplymakes by taking an escaper.The rewrite refuses when it removed nothing. It used to append on the strength of the card being non-empty, so a click on an already-rewritten card — a second engineer on a stale view, or one racing the first rewrite — stacked a second marker and reported two different windows for one finding. It also carried
block_id: "sil:<key>"onto the block whose control it had just deleted, breaking the iff-rule the renderer pins.A failed mark is now told to the clicker.
postSlackResponsereturned nothing, so a refused rewrite or a 500 was a log line and otherwise invisible: silence recorded, channel none the wiser, and the one person who could say so certain it had been announced. Both failure modes appendSilenceCardUnmarkedto the ack.The client timeout drops 10s → 1.2s per post.
WriteHeaderonly records the status — the bytes leave when the handler returns — so these posts sit inside Slack's 3-second interaction deadline, and a silence now sends two. 10s could blow that budget on one slow post, which Slack answers with anoperation_timeoutshown to the clicker plus a retry of the whole interaction.Smaller, same review:
postSlackResponsetolerates a nil client rather than panicking inside a handler for a&Server{}built by struct literal (which this package's tests already do); thesilencedbool is gone, since an empty marker was already the same fact; and the test fixtures quote throughencoding/jsoninstead of splicing, so a fallback containing a quote stops producing a 400 that reads as a rejected card.Every new guard was mutation-tested — dropping the
!removedcheck, scoping theblock_iddeletion to the loop-carried flag, and removing the disclosure each fail the tests written for them.The window bounds now sit at one scope
The 2-to-5 preset bound was enforced at two scopes: the floor only under
notify.slack.silence_button, the ceiling always. That fit while the numbers were Slack's — an overflow element rejected the whole message at either breach. It stopped fitting when both were reclassified as a UX judgement about the rendered dropdown, and the reclassification reached the wording without reaching the enforcement.A rule about how many options make a usable menu cannot apply to a deployment that renders no menu. Matrix silences on a bare 🔕 reaction, which reads
windows[0]and nothing else — so the second preset is exactly as unreachable there as the sixth. The ceiling's defence (it costs Matrix nothing it could reach anyway) was equally true of the floor, which is the tell that one rule was pretending to be two. Both ends now sit undersilence_button, in one check with one message.Scoping only ever accepts configs that were previously rejected, so no working deployment breaks on upgrade.
A second review pass, on the new code
Reviewing the commits above found three more, fixed in
11548859:postSlackResponseclosed the response body without reading it.net/httponly returns a connection to the idle pool once its body hits EOF, so everyresponse_urlPOST was killing its connection and redoing DNS + TCP + TLS. Survivable at 10s; at 1.2s it is the difference between a generous bound and a coin flip — a silence makes two sequential posts inside Slack's 3s budget, and a cold handshake on each is exactly how one times out after Slack already applied it, telling the clicker their card went unmarked when it did not. It also makes theslackHTTPcomment true, which reasoned about a shared pool nothing from this call site was entering.Silencedreturned a barefalsewhether the card carried no marker or someone else's, and the handler said "the channel cannot tell this finding is handled" for both. On an already-rewritten card that is plainly false — the channel sees a marker, it is just the earlier window, and the ledger's latest-wins fold means card and suppression now disagree. The clicker was steered into a redundant note while the real problem went unsaid.SilenceOutcomenames the three cases; an already-rewritten card now getsSilenceCardStale.How was it verified?
Full gate, clean:
Plus
go test -raceon every touched package (server,notify,thread,config,outcome) — the silence path posts over HTTP from a pooled client now, so the races are worth ruling out explicitly.The card golden moves, so
hack/check-screenshots-fresh.sh's acknowledgement is re-pointed at the new digest. This is the one card change of the series a reader of the shipped screenshots would actually notice; retaking them still needs the live workspace whose credentials sit outside this repo.Breaking change
Checklist
go build ./...passesgo vet ./...passesgo test ./...passes (-raceon all touched packages)gofmt -l .prints nothinggolangci-lint run ./...reports 0 issues!and the body carries aBREAKING CHANGE:footer — the window-bound scope changed