Skip to content

Commit 0164568

Browse files
amtiYoclaude
andcommitted
Close the last two delivery-path gaps (v1.0.0-rc.1)
Second-pass review found two lost-notification bugs in the delivery path: - Body length was budgeted by raw Markdown runes, but rendering can expand the visible text (a Markdown image becomes an 'Image:' prefix), so an image-heavy release/PR body could exceed Telegram's 4096-char limit and be rejected, silently losing the notification. Bodies are now re-rendered to fit by visible length and titles are bounded. - Subscriptions matched webhooks by repo full name, so a GitHub repo rename silently dropped all notifications. Match by stable github_repo_id instead and reconcile the cached name. Also: recognize /start@botname in groups, drain a buffered fatal error past a racing shutdown signal, log lease-expiry terminal failures. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
1 parent 28a3036 commit 0164568

9 files changed

Lines changed: 188 additions & 24 deletions

File tree

‎CHANGELOG.md‎

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,33 @@ GitHub Releases.
1010

1111
Use this section for changes that are merged but not released yet.
1212

13+
## v1.0.0-rc.1 - 2026-06-25
14+
15+
Release candidate. A second independent review pass closed the last two
16+
lost-notification gaps in the delivery path; everything else was clean.
17+
18+
### Reliability
19+
20+
- Release and pull-request notification bodies are now budgeted by their
21+
**visible** rendered length, not the raw Markdown slice. Rendering can expand
22+
the visible text (a Markdown image becomes an `Image:` prefix), so an
23+
image-heavy body could previously exceed Telegram's 4096-character limit and
24+
be rejected outright — silently losing the notification with no retry. The
25+
body is re-rendered to fit, and titles are bounded.
26+
- Subscriptions are matched to incoming webhooks by the stable
27+
`github_repo_id` instead of the repository's full name. A GitHub repo
28+
**rename** no longer silently drops every notification for that repo (the hook
29+
keeps firing under the same id with a new name); the cached name is refreshed.
30+
31+
### Fixed
32+
33+
- `/start@botname` — the form Telegram delivers in groups — is now recognized,
34+
so the "open me in a DM" prompt fires there.
35+
- A fatal startup error (e.g. the HTTP port already in use) is no longer masked
36+
as a clean exit when a shutdown signal arrives at the same instant.
37+
- A notification job that exhausts its attempts purely through worker
38+
lease-expiry is now logged instead of being a silent terminal drop.
39+
1340
## v1.0.0-alpha.4 - 2026-06-25
1441

1542
A multi-dimension review pass: two delivery-path correctness blockers closed

‎cmd/branchy/main.go‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -164,6 +164,15 @@ func run() error {
164164
case <-ctx.Done():
165165
case runErr = <-errCh:
166166
}
167+
// If a shutdown signal won the race against a fatal goroutine error (e.g. a
168+
// bind failure buffered the same instant a SIGTERM arrived), drain it so the
169+
// real failure is still reported instead of a misleading clean exit.
170+
if runErr == nil {
171+
select {
172+
case runErr = <-errCh:
173+
default:
174+
}
175+
}
167176

168177
// Cancel the signal context so the polling and worker loops observe it,
169178
// shut the HTTP server, then wait for every goroutine to return before the

‎internal/db/db.go‎

Lines changed: 26 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import (
66
"encoding/json"
77
"errors"
88
"fmt"
9+
"log/slog"
910
"math/rand"
1011
"sort"
1112
"strings"
@@ -265,25 +266,41 @@ func (s *Store) GetSubscriptionForUser(ctx context.Context, telegramUserID int64
265266
return sub, err
266267
}
267268

268-
func (s *Store) ListActiveSubscriptionsForRepoEvent(ctx context.Context, repoFullName, event string) ([]Subscription, error) {
269+
// ListActiveSubscriptionsForRepoEvent matches on the stable github_repo_id, not
270+
// the repo's full name, so a GitHub repo rename does not silently break delivery
271+
// (the webhook payload keeps firing under the same id with a new name).
272+
func (s *Store) ListActiveSubscriptionsForRepoEvent(ctx context.Context, repoID int64, event string) ([]Subscription, error) {
269273
rows, err := s.pool.Query(ctx, `
270274
SELECT s.id::text, s.telegram_user_id, s.destination_type, s.destination_chat_id,
271275
s.github_repo_id, s.repo_full_name, s.events, s.branch_mode, s.branch_name,
272276
s.branch_names, s.pull_request_actions, s.release_mode,
273277
s.status, s.pause_reason, r.default_branch, r.html_url
274278
FROM subscriptions s
275279
JOIN repositories r ON r.github_repo_id = s.github_repo_id
276-
WHERE s.repo_full_name = $1
280+
WHERE s.github_repo_id = $1
277281
AND s.status = 'active'
278282
AND $2 = ANY(s.events)
279-
`, repoFullName, event)
283+
`, repoID, event)
280284
if err != nil {
281285
return nil, err
282286
}
283287
defer rows.Close()
284288
return scanSubscriptions(rows)
285289
}
286290

291+
// ReconcileSubscriptionRepoName refreshes the cached repo_full_name for every
292+
// subscription on a repo (matched by stable github_repo_id) when GitHub reports
293+
// a new name — e.g. after a rename — so the bot UI and any name-based paths do
294+
// not drift. A no-op when nothing changed.
295+
func (s *Store) ReconcileSubscriptionRepoName(ctx context.Context, repoID int64, fullName string) error {
296+
_, err := s.pool.Exec(ctx, `
297+
UPDATE subscriptions
298+
SET repo_full_name = $2, updated_at = now()
299+
WHERE github_repo_id = $1 AND repo_full_name <> $2
300+
`, repoID, fullName)
301+
return err
302+
}
303+
287304
func (s *Store) ListActiveEventsForRepo(ctx context.Context, repoFullName string) ([]string, error) {
288305
rows, err := s.pool.Query(ctx, `
289306
SELECT DISTINCT unnest(events)
@@ -524,6 +541,12 @@ func (s *Store) ClaimPendingNotificationJobs(ctx context.Context, limit int, lea
524541
`, row.job.ID, nextAttempts); err != nil {
525542
return nil, err
526543
}
544+
// This terminal transition happens inside the claim tx, so it
545+
// never flows through the worker's recordOutcome/logOutcome. Log
546+
// it here so an exhausted-by-lease-expiry failure is not a silent
547+
// drop.
548+
slog.Warn("notification job failed after repeated lease expiry without a result",
549+
"job_id", row.job.ID, "attempts", nextAttempts, "subscription_id", row.job.SubscriptionID)
527550
continue
528551
}
529552
if _, err := tx.Exec(ctx, `

‎internal/notify/format.go‎

Lines changed: 20 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ import (
1010

1111
type Event struct {
1212
Type string
13+
RepoID int64
1314
RepoFullName string
1415
DefaultBranch string
1516
Actor string
@@ -144,7 +145,7 @@ func pullRequestEvent(event Event) string {
144145
header := fmt.Sprintf("%s <b>%s</b>\nPull request %s", iconPR, esc(event.RepoFullName), esc(action))
145146

146147
var meta []string
147-
if title := strings.TrimSpace(event.Title); title != "" {
148+
if title := truncateText(event.Title, maxTitleRunes); title != "" {
148149
label := title
149150
if event.Number > 0 {
150151
label = fmt.Sprintf("#%d %s", event.Number, title)
@@ -197,20 +198,23 @@ func showsPRBody(action string) bool {
197198
}
198199

199200
// maxReleaseBodyRunes caps how much of a release's notes we render. Telegram
200-
// allows 4096 visible characters per message; the body is sliced from the raw
201-
// Markdown (visible text after rendering is never longer) and wrapped in an
202-
// expandable blockquote, so a generous cap stays well under the limit even with
203-
// the header, footer, and "Full release notes" link, while no longer cutting
204-
// typical release notes off mid-sentence.
201+
// allows 4096 visible characters per message; bodyBlock holds the rendered body
202+
// to this many *visible* characters and the header/footer/title are bounded, so
203+
// the whole message stays under the limit while no longer cutting typical
204+
// release notes off mid-sentence.
205205
const maxReleaseBodyRunes = 3500
206206

207+
// maxTitleRunes bounds an attacker-influenced release/PR title in the header so
208+
// it cannot, together with the body, push the message past Telegram's limit.
209+
const maxTitleRunes = 200
210+
207211
func releaseEvent(event Event) string {
208212
label := "Release"
209213
if event.Prerelease {
210214
label = "Pre-release"
211215
}
212216
versionLine := fmt.Sprintf("<b>%s</b>", label)
213-
if version := firstNonEmpty(event.Title, event.TagName); version != "" {
217+
if version := truncateText(firstNonEmpty(event.Title, event.TagName), maxTitleRunes); version != "" {
214218
if link := safeURL(event.URL); link != "" {
215219
versionLine += fmt.Sprintf(` · <a href="%s">%s</a>`, escAttr(link), esc(version))
216220
} else {
@@ -282,6 +286,15 @@ const (
282286
// contain them are shown as-is.
283287
func bodyBlock(raw string, maxRunes int) (string, bool) {
284288
rendered, truncated := renderGitHubMarkdown(raw, maxRunes)
289+
// Rendering can make the visible text LONGER than the raw slice — a Markdown
290+
// image becomes an "Image: " prefix — so a raw-rune cap alone can overshoot
291+
// Telegram's 4096 visible-char limit and get the whole message rejected.
292+
// Shrink the raw cap and re-render until the visible body fits its budget.
293+
for rawCap := maxRunes; rawCap > 64 && visibleRuneCount(rendered) > maxRunes; {
294+
rawCap = rawCap * 3 / 4
295+
rendered, _ = renderGitHubMarkdown(raw, rawCap)
296+
truncated = true
297+
}
285298
if rendered == "" {
286299
return "", false
287300
}

‎internal/notify/format_test.go‎

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -132,6 +132,28 @@ func TestGitHubEventTrimsOverlongCommitListToRemainder(t *testing.T) {
132132
}
133133
}
134134

135+
func TestGitHubEventBodyStaysUnderTelegramLimit(t *testing.T) {
136+
// Markdown-image bomb: rendering expands each "![](x)" into an "Image: "
137+
// prefix, so a raw-rune cap alone would overshoot Telegram's 4096 visible
138+
// limit and get the whole message rejected. With an overlong title too, the
139+
// full message must still fit.
140+
body := strings.Repeat("![](x) ", 800)
141+
text := GitHubEvent(Event{
142+
Type: "release",
143+
RepoFullName: "acme/repo",
144+
Title: strings.Repeat("T", 500),
145+
TagName: "v1",
146+
URL: "https://github.com/acme/repo/releases/tag/v1",
147+
Body: body,
148+
})
149+
if got := visibleRuneCount(text); got > 4096 {
150+
t.Fatalf("message visible length = %d, exceeds Telegram's 4096-char limit", got)
151+
}
152+
if !strings.Contains(text, "Full release notes") {
153+
t.Fatalf("a body trimmed to fit should still link to the full notes:\n%s", text)
154+
}
155+
}
156+
135157
func TestGitHubEventCollapsesOnlyLongBodies(t *testing.T) {
136158
mk := func(body string) string {
137159
return GitHubEvent(Event{Type: "release", RepoFullName: "acme/repo", TagName: "v1", URL: "https://github.com/acme/repo/releases/tag/v1", Body: body})

‎internal/telegram/bot.go‎

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -197,11 +197,21 @@ func (b *Bot) handleUpdate(ctx context.Context, update Update) error {
197197
return nil
198198
}
199199

200+
// isStartCommand reports whether text is the /start command, tolerating the
201+
// /start@botname form Telegram delivers in groups and any trailing arguments.
202+
func isStartCommand(text string) bool {
203+
text = strings.TrimSpace(text)
204+
if i := strings.IndexAny(text, " @"); i >= 0 {
205+
text = text[:i]
206+
}
207+
return text == "/start"
208+
}
209+
200210
func (b *Bot) handleMessage(ctx context.Context, msg Message) error {
201211
if err := b.upsertUser(ctx, msg.From); err != nil {
202212
return err
203213
}
204-
if msg.Text == "/start" {
214+
if isStartCommand(msg.Text) {
205215
if msg.Chat.Type != "private" {
206216
var markup *InlineKeyboardMarkup
207217
if username := b.botUsername(ctx); username != "" {

‎internal/webhooks/events.go‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -107,6 +107,7 @@ func parsePush(body []byte) (notify.Event, bool, error) {
107107
Compare string `json:"compare"`
108108
Deleted bool `json:"deleted"`
109109
Repository struct {
110+
ID int64 `json:"id"`
110111
FullName string `json:"full_name"`
111112
DefaultBranch string `json:"default_branch"`
112113
HTMLURL string `json:"html_url"`
@@ -154,6 +155,7 @@ func parsePush(body []byte) (notify.Event, bool, error) {
154155
}
155156
return notify.Event{
156157
Type: "push",
158+
RepoID: payload.Repository.ID,
157159
RepoFullName: payload.Repository.FullName,
158160
DefaultBranch: payload.Repository.DefaultBranch,
159161
Actor: actor,
@@ -188,6 +190,7 @@ func parsePullRequest(body []byte) (notify.Event, bool, error) {
188190
}
189191
return notify.Event{
190192
Type: "pull_request",
193+
RepoID: payload.Repository.ID,
191194
RepoFullName: payload.Repository.FullName,
192195
DefaultBranch: payload.Repository.DefaultBranch,
193196
Actor: payload.Sender.Login,
@@ -225,6 +228,7 @@ func parseRelease(body []byte) (notify.Event, bool, error) {
225228
title := firstNonEmpty(payload.Release.Name, payload.Release.TagName)
226229
return notify.Event{
227230
Type: "release",
231+
RepoID: payload.Repository.ID,
228232
RepoFullName: payload.Repository.FullName,
229233
DefaultBranch: payload.Repository.DefaultBranch,
230234
Actor: payload.Sender.Login,
@@ -240,6 +244,7 @@ func parseRelease(body []byte) (notify.Event, bool, error) {
240244
}
241245

242246
type repo struct {
247+
ID int64 `json:"id"`
243248
FullName string `json:"full_name"`
244249
DefaultBranch string `json:"default_branch"`
245250
HTMLURL string `json:"html_url"`

‎internal/webhooks/handler.go‎

Lines changed: 16 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,10 @@ type Store interface {
2323
// transaction, returning (enqueued, duplicate, error). A true duplicate means
2424
// the delivery was already fully processed.
2525
EnqueueNotificationJobs(ctx context.Context, deliveryID, event, repoFullName string, jobs []db.NotificationJobInsert, maxAttempts int) (int, bool, error)
26-
ListActiveSubscriptionsForRepoEvent(ctx context.Context, repoFullName, event string) ([]db.Subscription, error)
26+
// ListActiveSubscriptionsForRepoEvent matches by stable github_repo_id so a
27+
// repo rename does not break delivery.
28+
ListActiveSubscriptionsForRepoEvent(ctx context.Context, repoID int64, event string) ([]db.Subscription, error)
29+
ReconcileSubscriptionRepoName(ctx context.Context, repoID int64, fullName string) error
2730
}
2831

2932
type Handler struct {
@@ -109,12 +112,23 @@ func (h *Handler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
109112
return
110113
}
111114

112-
subs, err := h.store.ListActiveSubscriptionsForRepoEvent(r.Context(), event.RepoFullName, event.Type)
115+
subs, err := h.store.ListActiveSubscriptionsForRepoEvent(r.Context(), event.RepoID, event.Type)
113116
if err != nil {
114117
slog.Error("webhook list subscriptions failed", "delivery_id", deliveryID, "repo", event.RepoFullName, "error", err)
115118
http.Error(w, "list subscriptions", http.StatusInternalServerError)
116119
return
117120
}
121+
// If GitHub renamed the repo, the payload carries the new name while our
122+
// cached one is stale; refresh it. Delivery itself already matched by the
123+
// stable repo id, so this only keeps the displayed name current.
124+
for _, sub := range subs {
125+
if sub.RepoFullName != event.RepoFullName {
126+
if err := h.store.ReconcileSubscriptionRepoName(r.Context(), event.RepoID, event.RepoFullName); err != nil {
127+
slog.Warn("reconcile repo name failed", "repo_id", event.RepoID, "error", err)
128+
}
129+
break
130+
}
131+
}
118132

119133
text := notify.GitHubEvent(event)
120134
var jobs []db.NotificationJobInsert

‎internal/webhooks/handler_test.go‎

Lines changed: 52 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -206,23 +206,64 @@ func TestHandlerEnqueuesMatchingSubscription(t *testing.T) {
206206
}
207207
}
208208

209+
func TestHandlerReconcilesRenamedRepo(t *testing.T) {
210+
// The repo was renamed: the webhook payload carries the new name (acme/repo,
211+
// id 12345) while our cached subscription still has the old name. Delivery
212+
// must still match (by stable id) and the cached name must be reconciled.
213+
body := pushFixture()
214+
store := &fakeStore{subs: []db.Subscription{{
215+
ID: "sub-1",
216+
DestinationChatID: 123,
217+
BranchMode: "default",
218+
DefaultBranch: "main",
219+
RepoFullName: "acme/old-name",
220+
}}}
221+
handler := NewHandler("secret", store, 5, Limits{})
222+
223+
req := httptest.NewRequest(http.MethodPost, "/webhooks/github", strings.NewReader(string(body)))
224+
req.Header.Set("X-Hub-Signature-256", SignForTest("secret", body))
225+
req.Header.Set("X-GitHub-Event", "push")
226+
req.Header.Set("X-GitHub-Delivery", "delivery-1")
227+
rec := httptest.NewRecorder()
228+
229+
handler.ServeHTTP(rec, req)
230+
231+
if rec.Code != http.StatusOK {
232+
t.Fatalf("status = %d, want %d", rec.Code, http.StatusOK)
233+
}
234+
if len(store.jobs) != 1 {
235+
t.Fatalf("jobs = %d, want 1 (a rename must not drop delivery)", len(store.jobs))
236+
}
237+
if store.reconcileCalls != 1 || store.reconciledName != "acme/repo" {
238+
t.Fatalf("reconcile = %dx name=%q; want 1, acme/repo", store.reconcileCalls, store.reconciledName)
239+
}
240+
}
241+
209242
type fakeStore struct {
210-
duplicate bool
211-
enqueueCalls int
212-
listCalls int
213-
deliveryID string
214-
event string
215-
repoFullName string
216-
maxAttempts int
217-
jobs []db.NotificationJobInsert
218-
subs []db.Subscription
243+
duplicate bool
244+
enqueueCalls int
245+
listCalls int
246+
reconcileCalls int
247+
reconciledName string
248+
deliveryID string
249+
event string
250+
repoFullName string
251+
maxAttempts int
252+
jobs []db.NotificationJobInsert
253+
subs []db.Subscription
219254
}
220255

221-
func (f *fakeStore) ListActiveSubscriptionsForRepoEvent(context.Context, string, string) ([]db.Subscription, error) {
256+
func (f *fakeStore) ListActiveSubscriptionsForRepoEvent(context.Context, int64, string) ([]db.Subscription, error) {
222257
f.listCalls++
223258
return f.subs, nil
224259
}
225260

261+
func (f *fakeStore) ReconcileSubscriptionRepoName(_ context.Context, _ int64, fullName string) error {
262+
f.reconcileCalls++
263+
f.reconciledName = fullName
264+
return nil
265+
}
266+
226267
func (f *fakeStore) EnqueueNotificationJobs(_ context.Context, deliveryID, event, repoFullName string, jobs []db.NotificationJobInsert, maxAttempts int) (int, bool, error) {
227268
f.enqueueCalls++
228269
f.deliveryID = deliveryID
@@ -240,7 +281,7 @@ func pushFixture() []byte {
240281
return []byte(`{
241282
"ref":"refs/heads/main",
242283
"compare":"https://github.com/acme/repo/compare/a...b",
243-
"repository":{"full_name":"acme/repo","default_branch":"main","html_url":"https://github.com/acme/repo"},
284+
"repository":{"id":12345,"full_name":"acme/repo","default_branch":"main","html_url":"https://github.com/acme/repo"},
244285
"pusher":{"name":"octocat"},
245286
"sender":{"login":"octocat"},
246287
"head_commit":{"message":"ship it","url":"https://github.com/acme/repo/commit/1"},

0 commit comments

Comments
 (0)