refactor(notification): replace shoutrrr with native senders#2980
refactor(notification): replace shoutrrr with native senders#2980
Conversation
Remove the containrrr/shoutrrr and go-strip-markdown dependencies by implementing direct HTTP webhook senders for Discord, Mattermost, Teams, Telegram, Ntfy, Pushbullet, and Pushover. SMTP continues to use the existing mail package. A generic webhook fallback handles unknown URL schemes. The send path now routes through connection-typed native senders before falling back to URL-based dispatch, removing the shoutrrr router abstraction entirely. Also adds InitConsumers to event_queue for test-only consumer setup without background listeners, and improves the email e2e test with MIME decoding and race-free event consumption.
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 23 minutes and 49 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (16)
WalkthroughAdds a native notification sender system replacing Shoutrrr, new connection types for Teams and Mattermost, multiple concrete sender implementations (Telegram, Discord, Teams, Mattermost, Ntfy, Pushbullet, Pushover), event consumer initializer Changes
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
notification/suite_test.go (1)
74-74:⚠️ Potential issue | 🟡 MinorStale comment references shoutrrr.
Shoutrrr is being removed in this PR; update the comment to reflect that the webhook now receives payloads from the native webhook sender.
📝 Proposed tweak
- webhookPostdata map[string]string // JSON message sent by shoutrrr to our webhook + webhookPostdata map[string]string // JSON payload received by the test webhook server🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@notification/suite_test.go` at line 74, Update the stale comment on the webhookPostdata variable: replace the reference to "shoutrrr" with "native webhook sender" (or equivalent phrasing) so the comment accurately reads that webhookPostdata map[string]string holds the JSON payload sent by the native webhook sender; update the comment next to webhookPostdata to reflect this new source.notification/shoutrrr_test.go (1)
53-59: 🛠️ Refactor suggestion | 🟠 MajorUse gomega matchers instead of
reflect.DeepEqual+ginkgo.Fail.With the gomega dot-import removed, this spec no longer uses any gomega assertion. Since the rest of the suite uses gomega and the repo mandates it, restore the dot-import and switch to
Expect(...).To(Equal(...))— you get a better diff on failure and consistent style.♻️ Proposed refactor
import ( - "fmt" - "reflect" - "github.com/flanksource/incident-commander/notification" ginkgo "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" ) @@ for _, tt := range tests { ginkgo.It(tt.name, func() { - if got := notification.GetPropsForService(tt.args.service, tt.args.property); !reflect.DeepEqual(got, tt.want) { - ginkgo.Fail(fmt.Sprintf("GetPropsForService() = %v, want %v", tt.args, tt.want)) - } + Expect(notification.GetPropsForService(tt.args.service, tt.args.property)).To(Equal(tt.want)) }) }As per coding guidelines: "Use dot-import for gomega (
. "github.com/onsi/gomega") to write assertions directly asExpect(...)."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@notification/shoutrrr_test.go` around lines 53 - 59, Restore the gomega dot-import (". \"github.com/onsi/gomega\"") in notification/shoutrrr_test.go and replace the reflect.DeepEqual + ginkgo.Fail pattern inside the test loop with a gomega assertion: call Expect(notification.GetPropsForService(tt.args.service, tt.args.property)).To(Equal(tt.want)). Keep the surrounding ginkgo.It and tt iteration unchanged so failures use gomega matchers and produce cleaner diffs.
🧹 Nitpick comments (17)
notification/senders/discord.go (1)
49-58:discordEmbed.Coloris defined but never populated.No caller sets
Color, so the field is dead weight. Either wire it fromdata(e.g., severity → color), or drop it until needed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@notification/senders/discord.go` around lines 49 - 58, discordEmbed.Color is declared but never populated; either remove the field or set it when building embeds (e.g., map notification severity to a Discord color). Update the code that constructs discordEmbed instances (where discordWebhookPayload is created) to assign Color based on the message severity or priority (implement a small helper like severityToDiscordColor and call it when creating each discordEmbed), or remove the Color field from the discordEmbed struct if you choose not to use it.notification/shoutrrr_test.go (1)
1-11: Consider renaming the file now that shoutrrr content is gone.The file now only tests
GetPropsForService, soshoutrrr_test.gois misleading. A rename to something likeproperties_test.go(or merging into an existing suite file) would improve discoverability.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@notification/shoutrrr_test.go` around lines 1 - 11, The test file name shoutrrr_test.go is misleading because it only contains tests for GetPropsForService; rename the file to properties_test.go (or merge its contents into an existing properties/tests file) so the filename reflects the tested symbol GetPropsForService and improves discoverability; update any references or test runners if present (no code changes required inside the file other than the filename).notification/senders/pushbullet.go (1)
3-12: Minor: import ordering.
"context"is a stdlib package but is grouped with the third-partyduty/modelsblock instead of with the other stdlib imports above. Not a correctness issue — goimports/gofmt will usually reorder this — but worth cleaning up since the file is brand new.📝 Proposed tweak
import ( "bytes" + "context" "encoding/json" "fmt" "io" "net/http" - "context" "github.com/flanksource/duty/models" )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@notification/senders/pushbullet.go` around lines 3 - 12, The import block in pushbullet.go has the stdlib package "context" grouped with the third-party package "github.com/flanksource/duty/models"; move "context" into the top stdlib import group (with "bytes", "encoding/json", "fmt", "io", "net/http") so stdlib and third-party imports are separated and ordered correctly, ensuring goimports/gofmt will keep the groups consistent.events/event_queue.go (1)
64-113:InitConsumersandStartConsumersshare a package-levelconsumersslice.Both functions append to the same global. If a test binary ever ends up invoking both (directly or indirectly via a shared bootstrap), you'll register each consumer twice and
ConsumeAllwill process events multiple times. Consider either resettingconsumersat the top ofInitConsumersor guarding with a sentinel, so the test-only path remains idempotent even if reinvoked.Also note the async
Consumerclosure drops theevent+".trace"/event+".debug"property plumbing thatStartConsumershas — if that's intentional for tests, a one-line comment would help future maintainers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@events/event_queue.go` around lines 64 - 113, InitConsumers currently appends to the package-level consumers slice that StartConsumers also appends to, causing double-registration if both are invoked; to fix, make InitConsumers idempotent by either resetting the consumers slice at the start (consumers = nil) or by adding a sentinel boolean (e.g., initConsumersCalled) and returning if already initialized; update the AsyncEventConsumer closure in InitConsumers to preserve the same event+".trace" and event+".debug" property plumbing that StartConsumers sets (or add a one-line comment in InitConsumers explaining why tests intentionally omit that plumbing) so behavior is consistent and future maintainers understand the difference between InitConsumers and StartConsumers.notification/senders/senders_test.go (2)
209-213:rewriteTransportassumes the target URL starts with exactlyhttp://.Line 211 strips a fixed 7-char prefix. Always true for
httptest.NewServer()today, but fragile — if someone ever usesNewTLSServeror a server with an explicit scheme variant, this produces a malformedHost. Parsingt.targetURLonce withurl.Parseand reusingu.Hostis more defensive and clearer.🔧 Proposed fix
-func (t *rewriteTransport) RoundTrip(req *http.Request) (*http.Response, error) { - req.URL.Scheme = "http" - req.URL.Host = t.targetURL[len("http://"):] - return t.base.RoundTrip(req) -} +func (t *rewriteTransport) RoundTrip(req *http.Request) (*http.Response, error) { + u, err := url.Parse(t.targetURL) + if err != nil { + return nil, err + } + req.URL.Scheme = u.Scheme + req.URL.Host = u.Host + return t.base.RoundTrip(req) +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@notification/senders/senders_test.go` around lines 209 - 213, The RoundTrip implementation in rewriteTransport blindly strips a fixed "http://" prefix from t.targetURL which breaks if the URL uses "https://" or other variants; update rewriteTransport.RoundTrip (or initialize rewriteTransport) to parse t.targetURL once with url.Parse and use the parsed URL's Host (u.Host) and Scheme (u.Scheme) to set req.URL.Host and req.URL.Scheme instead of slicing the string, ensuring proper handling of both http and https test servers and any scheme variants.
162-164: Swapping the package-levelhttpClientis not goroutine-safe and relies on serial execution.Both Pushbullet and Pushover tests replace the package-level
httpClientvariable. If Ginkgo is ever run with--procs/-por another spec later adds parallel containers, these tests can race with any other test that happens to invoke a sender concurrently (or with each other). A safer pattern is to makehttpClientoverridable per-sender (e.g., a package-private var that can be injected) or to hoist the URL override into the sender API (e.g.,ForConnectionreturning a sender that reads a configurable base URL).At minimum, a comment explaining the implicit "serial only" requirement would help future maintainers.
Also applies to: 189-191
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@notification/senders/senders_test.go` around lines 162 - 164, The tests are mutating the package-level httpClient (and using rewriteTransport) which is not goroutine-safe; change the sender API so the HTTP client or base URL can be injected per-sender instead of swapping a global: add a package-private option/parameter to Pushbullet and Pushover constructors (or ForConnection) to accept an *http.Client or baseURL and update tests to pass a client configured with rewriteTransport, referencing the httpClient global and rewriteTransport type to locate the existing logic; if you cannot refactor now, at minimum add a clear comment near the httpClient declaration and in these tests stating they require serial execution to avoid races.notification/shoutrrr.go (5)
45-57:firstNonEmptyhas non-deterministic ordering for props and treats props as higher priority than query per-key.Two observations on Lines 45-57:
for p, v := range propsat Line 47 iterates in Go's randomized map order. Ifpropscontains bothToandto(case-insensitive equal), the returned value is non-deterministic. Unlikely in practice, but worth being aware of.- The priority model is "for each key, check all props first, then query". That means
keys[0]in props wins overkeys[0]in query, butkeys[0]in props also wins overkeys[1]in either — which may or may not be intentional compared to the previous shoutrrr semantics.Consider: build a normalized
map[lowercase-key]valuefrom props once, then for each key look upprops[lower(k)]first, elseq.Get(k). This is O(len(keys)) instead of O(len(keys)*len(props)) and avoids the nondeterminism.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@notification/shoutrrr.go` around lines 45 - 57, The function firstNonEmpty currently iterates props with range (non-deterministic map order) and checks props for each key which is O(len(keys)*len(props)); change it to first build a normalized lookup map by lowercasing prop keys (e.g., normalizedProps := map[string]string where keys are strings.ToLower(p) -> v) using the existing props map, then for each key in keys check normalizedProps[strings.ToLower(k)] first and if empty fall back to q.Get(k); this preserves the intended per-key priority (props over query), removes nondeterminism, and reduces complexity to O(len(props)+len(keys)).
150-198: RenameshoutrrrSend/shoutrrrSendRawnow that shoutrrr is gone.Both functions (plus the file name
shoutrrr.go) still carry theshoutrrrbrand, but they no longer route through shoutrrr — they dispatch tosendSMTP/sendGenericWebhook. Keeping the old names is actively misleading for future readers. ConsiderdispatchNotification/dispatchNotificationRaw(orsendByURL/sendByURLRaw) and renaming the file to something likedispatch.go.Also, the two functions share the exact same switch (Lines 160-167 and 190-197) —
shoutrrrSendcould simply build theNotificationTemplateand delegate toshoutrrrSendRaw(or the renamed equivalent) to eliminate the duplication.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@notification/shoutrrr.go` around lines 150 - 198, The functions shoutrrrSendRaw and shoutrrrSend (and file shoutrrr.go) are misnamed and duplicate logic; rename them to dispatchNotificationRaw and dispatchNotification (or sendByURLRaw/sendByURL) and rename the file (e.g., dispatch.go), keep serviceFromURL, sendSMTP and sendGenericWebhook behavior unchanged, and refactor shoutrrrSend (now dispatchNotification) to build the NotificationTemplate (using FormatNotificationMessage for formatting) and delegate to dispatchNotificationRaw (now shoutrrrSendRaw renamed) to remove the duplicated switch; ensure all call sites are updated to the new function names.
116-135:sendGenericWebhookLGTM — one note on idempotency and retries.Straightforward JSON POST with clear error pathing. Two small thoughts:
- The payload merges
data.Propertiesinto amap[string]stringwith reserved keystitleandmessage(Lines 118-124). A user-supplied property namedtitleormessagewill be silently overwritten — consider nesting user properties under apropertieskey or documenting the collision, depending on contract.- Since this is a generic webhook with arbitrary receivers, consider wiring in timeout/retry behavior via
commonshttp.NewClient()builders (e.g.,.Retry(…)) so transient receiver flakiness doesn't drop notifications on the floor. Out of scope for this PR if retry is handled upstream — just flagging.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@notification/shoutrrr.go` around lines 116 - 135, sendGenericWebhook currently flattens data.Properties into payload map[string]string which silently overwrites reserved keys "title" and "message"; update sendGenericWebhook (and related NotificationTemplate usage) to avoid collisions by nesting user-supplied properties under a "properties" key (e.g., payload["properties"]=data.Properties) or explicitly detect/validate collisions and return an error, and optionally wire commonshttp.NewClient() (used via commonshttp.NewClient().R(ctx.Context).Post) with sensible timeouts/retry settings (or delegate to upstream) to guard against transient failures.
156-156: Usectx.Oops()for error wrapping to match the rest of the file and repo conventions.The file already uses
ctx.Oops().Wrapf(err, …)at Lines 87 and 107, but these four sites use barefmt.Errorf:
- Line 156: templating error
- Line 166 / 196: unsupported scheme
- Line 180: format error
Converting them keeps the error envelope (codes, structured context) consistent for observability/categorization and matches the guideline to build errors via
ctx.Oops()with codes fromduty/api. Example for the unsupported-scheme case:return "", ctx.Oops().Code(dutyAPI.EINVALID).Errorf("unsupported notification URL scheme: %s", service)As per coding guidelines: "Use
ctx.Oops()method call (not field access) to build errors with error codes fromgithub.com/flanksource/duty/api" and "Use.Code()with error codes fromgithub.com/flanksource/duty/apifor error categorization."Also applies to: 166-166, 180-180, 196-196
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@notification/shoutrrr.go` at line 156, Replace the bare fmt.Errorf returns with the repo-standard ctx.Oops() error builders: for sites that return an error wrapping another error (the templating and format errors) use ctx.Oops().Wrapf(err, "...") preserving the original message (e.g., replace `fmt.Errorf("error templating notification: %w", err)` with `ctx.Oops().Wrapf(err, "error templating notification")` and similarly for the format error), and for the unsupported URL-scheme branches that currently return a plain fmt.Errorf about the `service` value, return a coded error using ctx.Oops().Code(dutyAPI.EINVALID).Errorf("unsupported notification URL scheme: %s", service); use ctx.Oops(), .Wrapf, and .Code(dutyAPI.EINVALID).Errorf to match the existing usage elsewhere in this file.
59-114:sendSMTPfunction has redundant URL parsing; simplify afterconn.FromURLpopulates credentials.The URL is parsed twice: once manually at line 72 and again inside
conn.FromURL(smtpURL)at line 86. AfterFromURLpopulatesconn, the manual extraction at lines 72–82 and theSetCredentialscall at line 98 can be simplified to useconn.Host,conn.Port,conn.Username.ValueStatic, andconn.Password.ValueStaticdirectly. This eliminates the redundant parse and single source of truth for these values.One caveat:
FromURLapplies a default port of 587 if the parsed port is 0 (lines 155–157 ofapi/v1/connection_smtp.go), whereas the current manual extraction at line 77 does not. Verify thatSetCredentialshandles the defaulted port correctly before refactoring.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@notification/shoutrrr.go` around lines 59 - 114, The sendSMTP function currently parses smtpURL manually and then calls conn.FromURL(smtpURL) leading to duplicated parsing; remove the url.Parse + parsedURL usage and instead rely on conn populated by conn.FromURL: use conn.Host (or conn.Hostname), conn.Port (which FromURL defaulting to 587 will have applied), conn.Username.ValueStatic and conn.Password.ValueStatic when calling SetCredentials and anywhere else, and drop the manual password/port extraction and parsedURL.Query() lookups (use props/query for headers/to/from as before); ensure SetCredentials is invoked with conn.Host and conn.Port so the default port behavior from api/v1/connection_smtp.go is preserved.notification/senders/sender.go (1)
1-16: Consider usingduty/context.Contextfor theSenderinterface.Per the repo coding guidelines,
github.com/flanksource/duty/contextis the primary context type and should be used in function signatures unless there's a specific reason not to. The senders don't currently need DB/logger/properties, but adoptingduty/contexthere would keep the abstraction consistent with the rest of the codebase and would allow senders to log structurally viactx.Loggerlater without a signature change. Since standard librarycontextis also needed forhttp.NewRequestWithContext, alias it asgocontext.As per coding guidelines: "Use
duty/context.Contextfor all function signatures unless there's a specific reason not to" and "When a file needs bothduty/contextand standard librarycontext, alias the standard one asgocontext."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@notification/senders/sender.go` around lines 1 - 16, The Sender interface should use the repo-specific context type: replace the standard context.Context parameter in Sender.Send with duty/context.Context (imported as "github.com/flanksource/duty/context") and when you still need the stdlib context in this file (e.g., for http.NewRequestWithContext elsewhere), import the stdlib context as gocontext; update the import block accordingly and adjust any local code referencing the old ctx type to the new duty/context.Context type so Send(ctx dutycontext.Context, conn *models.Connection, data Data) matches the project convention.notification/senders/telegram.go (1)
24-34: Partial-send failure semantics: first chat failure aborts the rest.If
chatscontains e.g.123,456,789and chat456returns a non-200, chats789never receives the notification, and the caller sees only the first error. Consider accumulating errors (e.g., viaerrors.Join) so transient/individual chat failures don't silently drop downstream recipients. Acceptable as-is for fail-fast semantics, but worth a conscious decision.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@notification/senders/telegram.go` around lines 24 - 34, The current loop over strings.Split(chats, ",") calls telegramSendMessage(token, chatID, data) and returns immediately on the first error, which aborts remaining sends; change this to collect per-chat errors (e.g., using an errors slice and errors.Join) while continuing to send to subsequent chatIDs, trimming blanks as already done, and after the loop return either nil if no errors or the joined error that includes each failing chatID (include context like "telegram chat <chatID>: <err>" for each entry) so callers see the full set of failures instead of only the first.notification/senders/teams.go (1)
27-42: MessageCard format is aligned with a sunsetting Microsoft surface — confirm target webhook type.This payload shape (
@type: MessageCard,@context: http://schema.org/extensions,sections[]withactivityTitle/markdown) is the legacy Office 365 Connector format. Microsoft began retiring the Office 365 Connectors feature from Teams in August 2024 and after the deprecation dates Office 365 Connectors will no longer function, with the deadline to deprecate Office 365 Connectors extended to April 30, 2026. The recommended replacement is Workflows (Power Automate) webhooks. The good news is that by December 2025, Workflows webhooks will support messageCard payloads and posting to shared/private channels, so the payload shape here should continue to work if users pointconn.URLat a new Workflow webhook URL rather than a legacy connector URL.Recommendations:
- Document (in user-facing notification connection docs) that Teams webhook URLs must be Workflows-based, not legacy O365 Connector URLs.
- Consider adding an
httpPost-free Adaptive Card payload option as a future-proof alternative, since MessageCard-formatted payloads will render content but will not support HttpPost action buttons and interactive elements require Adaptive Cards.- The existing hard-coded
ThemeColor: "0076D7"at Line 30 ignores any severity the notification may carry; consider deriving it from notification severity so alerts visually stand out.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@notification/senders/teams.go` around lines 27 - 42, The payload uses the legacy MessageCard shape (constructed via teamsMessageCard -> card and marshalled with json.Marshal) so first add user-facing documentation that Teams webhook URLs must be Workflows (Power Automate) webhooks not legacy Office 365 Connector URLs; second, add a runtime check that validates/flags conn.URL when creating/sending the card (warn or error if the URL pattern/host matches known legacy connector formats) so callers know to update the webhook; third, make ThemeColor dynamic by mapping the notification severity from your input struct (e.g., data.Severity or Notification.Severity) to a color via a small helper (mapSeverityToColor) and use that value for teamsMessageCard.ThemeColor instead of the hard-coded "0076D7"; and finally consider adding a future flag/option to send an Adaptive Card payload instead of MessageCard (document this option) so interactive HttpPost actions are supported later.notification/send.go (3)
380-387: Behavior change: connection properties are no longer visible to user templates.Previously
connection.Propertieswere merged intodata.Propertiesbefore CEL/struct templating, so user-writtenTitle/Template/Propertiesvalues in the CRD could interpolate connection-level properties (e.g.{{.properties.channel_id}}). After this change they're only merged intodata.Propertiesin the shoutrrr fallback (L415) — after templating — so any{{...}}references to connection properties in titles/bodies will now render empty for native senders and for shoutrrr alike.If that's intentional, please call it out in the release notes / migration docs; otherwise consider merging
connectionProperties(connection)intocelEnv["properties"](or into the struct-templater root) before theWalkcall.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@notification/send.go` around lines 380 - 387, The templating step currently runs before connection-level properties are merged, so connection.Properties are not available to user templates; to fix, merge connectionProperties(connection) into celEnv["properties"] (or into the struct templater root) prior to creating/using ctx.NewStructTemplater and before templater.Walk(&data) so that data.Title/Template/Properties can reference connection-level keys (refer to resolveServiceName, ctx.NewStructTemplater, templater, and data for where to apply the change).
399-412:ForConnectionerror is silently swallowed — real config errors fall through to shoutrrr.
if sender, err := senders.ForConnection(connection); err == niltreats every non-nil error as "unsupported type, fall through to shoutrrr". IfForConnectionever returns a real validation/construction error for a supported connection type (e.g. missing required field on a Discord connection), we silently attempt a shoutrrr dispatch againstconnection.URLand surface a misleading error.Consider distinguishing "unsupported scheme" from real errors, e.g. have
ForConnectionreturn(nil, nil)for unsupported types and propagate any real error:♻️ Suggested handling
- if connection != nil { - if sender, err := senders.ForConnection(connection); err == nil { + if connection != nil { + sender, err := senders.ForConnection(connection) + if err != nil { + return "", fmt.Errorf("failed to build sender for %s: %w", connection.Type, err) + } + if sender != nil { senderData := senders.Data{ ... } ... } }Same pattern applies at L502-522 in
SendNotification.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@notification/send.go` around lines 399 - 412, The ForConnection call currently swallows real construction/validation errors by only checking err == nil and otherwise falling through to shoutrrr; update handling so that senders.ForConnection(connection) distinguishes unsupported schemes (returning (nil, nil)) from real errors and then propagate real errors instead of falling back: call senders.ForConnection(connection), if err != nil return fmt.Errorf(...) immediately; if sender == nil treat as unsupported and continue to shoutrrr; then call sender.Send(ctx.Context, connection, senderData) and traceLogSend as before. Apply the same pattern in the other SendNotification block that mirrors this logic.
432-453:resolveServiceNameoverrides upstreamcelEnv["channel"]set inCreateNotificationSendPayloads.
CreateNotificationSendPayloadssetscelEnvMap["channel"] = c.Typeat L696 for custom-notification connections, butSendRawNotificationunconditionally overwrites it at L382 viaresolveServiceName. The two values will usually agree, butresolveServiceNamenormalizesConnectionTypeEmail→"smtp"whereas L696 stores the rawc.Type("email"). Templates that branch onchannel == "email"would have worked insendEventNotificationpath (no override) but not in the raw-template path (overridden to"smtp").Minor, but worth picking one canonical mapping and using it in both places.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@notification/send.go` around lines 432 - 453, resolveServiceName is currently normalizing connection types (e.g., ConnectionTypeEmail → "smtp") while CreateNotificationSendPayloads sets celEnvMap["channel"] = c.Type (e.g., "email"), causing inconsistent channel values; fix by centralizing the mapping: update CreateNotificationSendPayloads to set celEnvMap["channel"] using resolveServiceName(c, shoutrrrURL) (or extract a single normalize helper used by both resolveServiceName and CreateNotificationSendPayloads) so both SendRawNotification and CreateNotificationSendPayloads produce the same canonical channel string (reference: resolveServiceName, CreateNotificationSendPayloads, SendRawNotification, celEnvMap["channel"], ConnectionTypeEmail, ConnectionTypeSlack).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@api/v1/connection_types.go`:
- Around line 66-83: Change the WebhookURL fields in the ConnectionTeams and
ConnectionMattermost structs from plain string to types.EnvVar so webhook URLs
are treated as secrets (update the field types for ConnectionTeams.WebhookURL
and ConnectionMattermost.WebhookURL), keep the existing json tags, regenerate
deepcopy/codegen, and add the downstream plumbing to resolve types.EnvVar to
actual string values before the webhook sender executes so runtime uses the
resolved secret value.
In `@notification/send.go`:
- Around line 502-522: SendNotification and SendRawNotification currently drop
user-specified properties because senders.Data only includes
Title/Message/Attachments; update the native sender path (inside the
senders.ForConnection branch where FormatNotificationMessage, sender.Send, and
traceLogSend are used) to include the rendered properties from the payload into
senders.Data (e.g., add a Properties map/string field) so native senders can
honor common keys like priority/tags/click; if you cannot immediately modify all
native senders to consume them, at minimum log or warn when payload.Properties
is non-empty and a native sender (senders.ForConnection returns a sender) is
chosen so users are aware their properties will be ignored.
In `@notification/senders/discord.go`:
- Around line 36-40: The POST call in notification/senders/discord.go uses
httpClient.Post which does not use the provided ctx; change it to create a
request with http.NewRequestWithContext(ctx, "POST", webhookURL,
bytes.NewReader(body)) and send it with httpClient.Do so cancellation and
timeouts propagate; after Do, handle errors as before and ensure
resp.Body.Close() is deferred only when resp != nil. Update imports if needed
(net/http) and keep the same content-type header on the request.
In `@notification/senders/ntfy.go`:
- Around line 22-41: The code currently sets basic auth unconditionally when
conn.Password is present which causes malformed credentials if conn.Username is
empty; update the logic around req.SetBasicAuth in ntfy.go so basic auth is only
applied when conn.Username != "" (i.e., guard SetBasicAuth(conn.Username,
conn.Password) with a check that conn.Username is non-empty) and add a branch to
set a Bearer Authorization header (req.Header.Set("Authorization", "Bearer
"+conn.Password)) when Username is empty but Password is set; ensure you
reference the existing variables/topic resolution (topic, conn.Properties,
conn.Username, conn.Password) and the request object (req from http.NewRequest)
when implementing the conditional.
- Around line 32-47: The HTTP request in Send (ntfy.go) is created with
http.NewRequest and therefore ignores the provided ctx; change the request
creation to use http.NewRequestWithContext(ctx, http.MethodPost, host+"/"+topic,
strings.NewReader(data.Message)) so the context (cancellation/deadline) is
honored, keep the subsequent header setting (req.Header.Set("Title", ...)),
basic auth (req.SetBasicAuth(conn.Username, conn.Password)), and call to
httpClient.Do(req) unchanged so caller timeouts/shutdowns propagate correctly;
apply the same pattern in other sender implementations that accept ctx.
In `@notification/senders/pushbullet.go`:
- Around line 33-44: The HTTP request in pushbullet.go is created without the
incoming context so cancellations/timeouts don't propagate; replace the call to
http.NewRequest(...) with http.NewRequestWithContext(ctx, http.MethodPost,
"https://api.pushbullet.com/v2/pushes", bytes.NewReader(body)) using the
function's context parameter (add a ctx parameter if the surrounding function —
e.g., the sender method that uses httpClient, token and body — doesn't already
accept one), keep the same header setup (Content-Type, Access-Token) and error
handling, and then call httpClient.Do(req) as before so request
cancellation/deadlines propagate correctly.
- Around line 16-31: Pushbullet.Send currently ignores configured targets
(ConnectionPushbullet.Targets stored in models.Connection.Properties["targets"])
and always sends a single broadcast note; update Send (method Pushbullet.Send)
to read and resolve targets from conn.Properties["targets"] (or equivalent
helper), iterate over each target and include the appropriate target field on
each payload (device_iden, email, or channel_tag) so an individual push is sent
per target, or if you choose not to support targets explicitly add a clear
comment/log and return an error documenting that Targets are unsupported; ensure
payload construction and json.Marshal happen per-target and preserve existing
behavior when no targets are configured.
In `@notification/senders/sender.go`:
- Around line 14-24: The Sender.Send(ctx, …) contract currently ignores ctx
because implementations (ntfy, mattermost, teams, telegram, discord, pushbullet,
pushover) create HTTP requests without attaching the context and/or use
httpClient.Post/PostForm; update each sender to build the request with
http.NewRequestWithContext(ctx, ...) (or call req = req.WithContext(ctx)) and
use httpClient.Do(req) so cancellations/deadlines propagate, keeping the shared
httpClient variable and Data struct unchanged.
In `@notification/senders/telegram.go`:
- Around line 51-55: The code embeds the bot token directly into the request URL
via fmt.Sprintf("https://api.telegram.org/bot%s/sendMessage", token), which can
leak the token if the URL is ever logged or included in transport traces;
replace the direct httpClient.Post call with an explicit http.NewRequest/Do
sequence using a local apiURL variable, and ensure you never log apiURL/raw
req.URL; if you must include the URL in any error or debug message, create a
redacted version (e.g. strings.Replace(apiURL, token, "<redacted>", 1)) and use
that for logging instead of the real URL; update the code paths around the
httpClient.Post call (the call site and any error wrapping) to use the new
request flow and the redacted URL for any messages.
- Around line 36-44: The telegramSendMessage function sets
"parse_mode":"MarkdownV2" but fails to escape data.Message, causing Telegram 400
errors; update telegramSendMessage to call escapeMarkdownV2 on data.Message in
both branches (the no-title branch where payload["text"] = data.Message and the
title branch where the formatted "*%s*\n\n%s" includes the body) so both title
and body are escaped, keep parse_mode as-is, and add a unit test in
senders_test.go that sends a message containing MarkdownV2 special characters
(e.g., "server-01.prod is down!") to assert the payload text is escaped to
prevent regressions.
---
Outside diff comments:
In `@notification/shoutrrr_test.go`:
- Around line 53-59: Restore the gomega dot-import (".
\"github.com/onsi/gomega\"") in notification/shoutrrr_test.go and replace the
reflect.DeepEqual + ginkgo.Fail pattern inside the test loop with a gomega
assertion: call Expect(notification.GetPropsForService(tt.args.service,
tt.args.property)).To(Equal(tt.want)). Keep the surrounding ginkgo.It and tt
iteration unchanged so failures use gomega matchers and produce cleaner diffs.
In `@notification/suite_test.go`:
- Line 74: Update the stale comment on the webhookPostdata variable: replace the
reference to "shoutrrr" with "native webhook sender" (or equivalent phrasing) so
the comment accurately reads that webhookPostdata map[string]string holds the
JSON payload sent by the native webhook sender; update the comment next to
webhookPostdata to reflect this new source.
---
Nitpick comments:
In `@events/event_queue.go`:
- Around line 64-113: InitConsumers currently appends to the package-level
consumers slice that StartConsumers also appends to, causing double-registration
if both are invoked; to fix, make InitConsumers idempotent by either resetting
the consumers slice at the start (consumers = nil) or by adding a sentinel
boolean (e.g., initConsumersCalled) and returning if already initialized; update
the AsyncEventConsumer closure in InitConsumers to preserve the same
event+".trace" and event+".debug" property plumbing that StartConsumers sets (or
add a one-line comment in InitConsumers explaining why tests intentionally omit
that plumbing) so behavior is consistent and future maintainers understand the
difference between InitConsumers and StartConsumers.
In `@notification/send.go`:
- Around line 380-387: The templating step currently runs before
connection-level properties are merged, so connection.Properties are not
available to user templates; to fix, merge connectionProperties(connection) into
celEnv["properties"] (or into the struct templater root) prior to creating/using
ctx.NewStructTemplater and before templater.Walk(&data) so that
data.Title/Template/Properties can reference connection-level keys (refer to
resolveServiceName, ctx.NewStructTemplater, templater, and data for where to
apply the change).
- Around line 399-412: The ForConnection call currently swallows real
construction/validation errors by only checking err == nil and otherwise falling
through to shoutrrr; update handling so that senders.ForConnection(connection)
distinguishes unsupported schemes (returning (nil, nil)) from real errors and
then propagate real errors instead of falling back: call
senders.ForConnection(connection), if err != nil return fmt.Errorf(...)
immediately; if sender == nil treat as unsupported and continue to shoutrrr;
then call sender.Send(ctx.Context, connection, senderData) and traceLogSend as
before. Apply the same pattern in the other SendNotification block that mirrors
this logic.
- Around line 432-453: resolveServiceName is currently normalizing connection
types (e.g., ConnectionTypeEmail → "smtp") while CreateNotificationSendPayloads
sets celEnvMap["channel"] = c.Type (e.g., "email"), causing inconsistent channel
values; fix by centralizing the mapping: update CreateNotificationSendPayloads
to set celEnvMap["channel"] using resolveServiceName(c, shoutrrrURL) (or extract
a single normalize helper used by both resolveServiceName and
CreateNotificationSendPayloads) so both SendRawNotification and
CreateNotificationSendPayloads produce the same canonical channel string
(reference: resolveServiceName, CreateNotificationSendPayloads,
SendRawNotification, celEnvMap["channel"], ConnectionTypeEmail,
ConnectionTypeSlack).
In `@notification/senders/discord.go`:
- Around line 49-58: discordEmbed.Color is declared but never populated; either
remove the field or set it when building embeds (e.g., map notification severity
to a Discord color). Update the code that constructs discordEmbed instances
(where discordWebhookPayload is created) to assign Color based on the message
severity or priority (implement a small helper like severityToDiscordColor and
call it when creating each discordEmbed), or remove the Color field from the
discordEmbed struct if you choose not to use it.
In `@notification/senders/pushbullet.go`:
- Around line 3-12: The import block in pushbullet.go has the stdlib package
"context" grouped with the third-party package
"github.com/flanksource/duty/models"; move "context" into the top stdlib import
group (with "bytes", "encoding/json", "fmt", "io", "net/http") so stdlib and
third-party imports are separated and ordered correctly, ensuring
goimports/gofmt will keep the groups consistent.
In `@notification/senders/sender.go`:
- Around line 1-16: The Sender interface should use the repo-specific context
type: replace the standard context.Context parameter in Sender.Send with
duty/context.Context (imported as "github.com/flanksource/duty/context") and
when you still need the stdlib context in this file (e.g., for
http.NewRequestWithContext elsewhere), import the stdlib context as gocontext;
update the import block accordingly and adjust any local code referencing the
old ctx type to the new duty/context.Context type so Send(ctx
dutycontext.Context, conn *models.Connection, data Data) matches the project
convention.
In `@notification/senders/senders_test.go`:
- Around line 209-213: The RoundTrip implementation in rewriteTransport blindly
strips a fixed "http://" prefix from t.targetURL which breaks if the URL uses
"https://" or other variants; update rewriteTransport.RoundTrip (or initialize
rewriteTransport) to parse t.targetURL once with url.Parse and use the parsed
URL's Host (u.Host) and Scheme (u.Scheme) to set req.URL.Host and req.URL.Scheme
instead of slicing the string, ensuring proper handling of both http and https
test servers and any scheme variants.
- Around line 162-164: The tests are mutating the package-level httpClient (and
using rewriteTransport) which is not goroutine-safe; change the sender API so
the HTTP client or base URL can be injected per-sender instead of swapping a
global: add a package-private option/parameter to Pushbullet and Pushover
constructors (or ForConnection) to accept an *http.Client or baseURL and update
tests to pass a client configured with rewriteTransport, referencing the
httpClient global and rewriteTransport type to locate the existing logic; if you
cannot refactor now, at minimum add a clear comment near the httpClient
declaration and in these tests stating they require serial execution to avoid
races.
In `@notification/senders/teams.go`:
- Around line 27-42: The payload uses the legacy MessageCard shape (constructed
via teamsMessageCard -> card and marshalled with json.Marshal) so first add
user-facing documentation that Teams webhook URLs must be Workflows (Power
Automate) webhooks not legacy Office 365 Connector URLs; second, add a runtime
check that validates/flags conn.URL when creating/sending the card (warn or
error if the URL pattern/host matches known legacy connector formats) so callers
know to update the webhook; third, make ThemeColor dynamic by mapping the
notification severity from your input struct (e.g., data.Severity or
Notification.Severity) to a color via a small helper (mapSeverityToColor) and
use that value for teamsMessageCard.ThemeColor instead of the hard-coded
"0076D7"; and finally consider adding a future flag/option to send an Adaptive
Card payload instead of MessageCard (document this option) so interactive
HttpPost actions are supported later.
In `@notification/senders/telegram.go`:
- Around line 24-34: The current loop over strings.Split(chats, ",") calls
telegramSendMessage(token, chatID, data) and returns immediately on the first
error, which aborts remaining sends; change this to collect per-chat errors
(e.g., using an errors slice and errors.Join) while continuing to send to
subsequent chatIDs, trimming blanks as already done, and after the loop return
either nil if no errors or the joined error that includes each failing chatID
(include context like "telegram chat <chatID>: <err>" for each entry) so callers
see the full set of failures instead of only the first.
In `@notification/shoutrrr_test.go`:
- Around line 1-11: The test file name shoutrrr_test.go is misleading because it
only contains tests for GetPropsForService; rename the file to
properties_test.go (or merge its contents into an existing properties/tests
file) so the filename reflects the tested symbol GetPropsForService and improves
discoverability; update any references or test runners if present (no code
changes required inside the file other than the filename).
In `@notification/shoutrrr.go`:
- Around line 45-57: The function firstNonEmpty currently iterates props with
range (non-deterministic map order) and checks props for each key which is
O(len(keys)*len(props)); change it to first build a normalized lookup map by
lowercasing prop keys (e.g., normalizedProps := map[string]string where keys are
strings.ToLower(p) -> v) using the existing props map, then for each key in keys
check normalizedProps[strings.ToLower(k)] first and if empty fall back to
q.Get(k); this preserves the intended per-key priority (props over query),
removes nondeterminism, and reduces complexity to O(len(props)+len(keys)).
- Around line 150-198: The functions shoutrrrSendRaw and shoutrrrSend (and file
shoutrrr.go) are misnamed and duplicate logic; rename them to
dispatchNotificationRaw and dispatchNotification (or sendByURLRaw/sendByURL) and
rename the file (e.g., dispatch.go), keep serviceFromURL, sendSMTP and
sendGenericWebhook behavior unchanged, and refactor shoutrrrSend (now
dispatchNotification) to build the NotificationTemplate (using
FormatNotificationMessage for formatting) and delegate to
dispatchNotificationRaw (now shoutrrrSendRaw renamed) to remove the duplicated
switch; ensure all call sites are updated to the new function names.
- Around line 116-135: sendGenericWebhook currently flattens data.Properties
into payload map[string]string which silently overwrites reserved keys "title"
and "message"; update sendGenericWebhook (and related NotificationTemplate
usage) to avoid collisions by nesting user-supplied properties under a
"properties" key (e.g., payload["properties"]=data.Properties) or explicitly
detect/validate collisions and return an error, and optionally wire
commonshttp.NewClient() (used via commonshttp.NewClient().R(ctx.Context).Post)
with sensible timeouts/retry settings (or delegate to upstream) to guard against
transient failures.
- Line 156: Replace the bare fmt.Errorf returns with the repo-standard
ctx.Oops() error builders: for sites that return an error wrapping another error
(the templating and format errors) use ctx.Oops().Wrapf(err, "...") preserving
the original message (e.g., replace `fmt.Errorf("error templating notification:
%w", err)` with `ctx.Oops().Wrapf(err, "error templating notification")` and
similarly for the format error), and for the unsupported URL-scheme branches
that currently return a plain fmt.Errorf about the `service` value, return a
coded error using ctx.Oops().Code(dutyAPI.EINVALID).Errorf("unsupported
notification URL scheme: %s", service); use ctx.Oops(), .Wrapf, and
.Code(dutyAPI.EINVALID).Errorf to match the existing usage elsewhere in this
file.
- Around line 59-114: The sendSMTP function currently parses smtpURL manually
and then calls conn.FromURL(smtpURL) leading to duplicated parsing; remove the
url.Parse + parsedURL usage and instead rely on conn populated by conn.FromURL:
use conn.Host (or conn.Hostname), conn.Port (which FromURL defaulting to 587
will have applied), conn.Username.ValueStatic and conn.Password.ValueStatic when
calling SetCredentials and anywhere else, and drop the manual password/port
extraction and parsedURL.Query() lookups (use props/query for headers/to/from as
before); ensure SetCredentials is invoked with conn.Host and conn.Port so the
default port behavior from api/v1/connection_smtp.go is preserved.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5ae8b43e-27c1-482b-8915-bd12b247b5c6
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (19)
api/v1/connection_types.goapi/v1/zz_generated.deepcopy.goevents/event_queue.gogo.modnotification/notification_email_e2e_test.gonotification/send.gonotification/senders/discord.gonotification/senders/mattermost.gonotification/senders/ntfy.gonotification/senders/pushbullet.gonotification/senders/pushover.gonotification/senders/sender.gonotification/senders/senders_test.gonotification/senders/suite_test.gonotification/senders/teams.gonotification/senders/telegram.gonotification/shoutrrr.gonotification/shoutrrr_test.gonotification/suite_test.go
💤 Files with no reviewable changes (1)
- go.mod
| func (p *Pushbullet) Send(ctx context.Context, conn *models.Connection, data Data) error { | ||
| token := conn.Password | ||
| if token == "" { | ||
| return fmt.Errorf("pushbullet connection requires a token (password)") | ||
| } | ||
|
|
||
| payload := map[string]string{ | ||
| "type": "note", | ||
| "title": data.Title, | ||
| "body": data.Message, | ||
| } | ||
|
|
||
| body, err := json.Marshal(payload) | ||
| if err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how/whether Targets is consumed anywhere in the new sender pipeline.
rg -nP --type=go -C3 '\bTargets\b' notification
rg -nP --type=go -C3 'Pushbullet' notificationRepository: flanksource/mission-control
Length of output: 3005
🏁 Script executed:
#!/bin/bash
# 1. Check if api/v1/connection_types.go exists and look for Pushbullet and Targets
find . -name "connection_types.go" -o -name "connection*.go" | head -20Repository: flanksource/mission-control
Length of output: 354
🏁 Script executed:
#!/bin/bash
# 2. Look at the models.Connection struct to understand available fields
fd -t f connection.go -x grep -l "type Connection" {} \;Repository: flanksource/mission-control
Length of output: 53
🏁 Script executed:
#!/bin/bash
# 3. View the complete pushbullet.go implementation
cat -n notification/senders/pushbullet.goRepository: flanksource/mission-control
Length of output: 1490
🏁 Script executed:
#!/bin/bash
# 4. Check how other senders (like Mattermost or Teams) handle targets, if at all
cat -n notification/senders/mattermost.go | head -50Repository: flanksource/mission-control
Length of output: 1458
🏁 Script executed:
#!/bin/bash
# Check the CRD connection_types.go for ConnectionPushbullet definition
cat -n api/v1/connection_types.go | grep -A 30 "Pushbullet"Repository: flanksource/mission-control
Length of output: 2287
🏁 Script executed:
#!/bin/bash
# Also check if there's a Targets field anywhere in connection_types.go
rg -n "Targets" api/v1/connection_types.goRepository: flanksource/mission-control
Length of output: 108
🏁 Script executed:
#!/bin/bash
# Check the models.Connection struct in duty to understand its full structure
rg -A 20 "type Connection struct" --type=goRepository: flanksource/mission-control
Length of output: 1302
🏁 Script executed:
#!/bin/bash
# Check how the CRD is converted to models.Connection (duty's internal model)
cat -n db/connection_crd_convert.go | head -100Repository: flanksource/mission-control
Length of output: 3976
🏁 Script executed:
#!/bin/bash
# Check how other senders are called and what data they receive
rg -A 10 "Pushbullet.*Send" notification --type=goRepository: flanksource/mission-control
Length of output: 1625
🏁 Script executed:
#!/bin/bash
# Look at job.go to understand the notification sending flow
rg -B 5 -A 15 "\.Send\(" notification/job.go | head -80Repository: flanksource/mission-control
Length of output: 53
🏁 Script executed:
#!/bin/bash
# Look for Pushbullet conversion in connection_crd_convert.go
rg -n "Pushbullet" db/connection_crd_convert.go -A 10Repository: flanksource/mission-control
Length of output: 679
🏁 Script executed:
#!/bin/bash
# Search entire file for all notification-related conversions
rg -n "Pushover|Pushbullet|Teams|Mattermost" db/connection_crd_convert.go -B 2 -A 5Repository: flanksource/mission-control
Length of output: 830
🏁 Script executed:
#!/bin/bash
# Get full file to see if Pushbullet is handled at all
wc -l db/connection_crd_convert.goRepository: flanksource/mission-control
Length of output: 103
The Pushbullet sender ignores configured targets, creating a silent regression from the previous shoutrrr-based implementation.
The CRD defines ConnectionPushbullet.Targets (api/v1/connection_types.go:57), which is intentionally converted and stored in models.Connection.Properties["targets"] (db/connection_crd_convert.go:416). However, the Send() method in notification/senders/pushbullet.go never accesses these targets and always sends a single broadcast push with no device_iden, email, or channel_tag set.
Iterate over the resolved targets and send individual pushes per target, or explicitly document that Targets is not supported by this sender.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@notification/senders/pushbullet.go` around lines 16 - 31, Pushbullet.Send
currently ignores configured targets (ConnectionPushbullet.Targets stored in
models.Connection.Properties["targets"]) and always sends a single broadcast
note; update Send (method Pushbullet.Send) to read and resolve targets from
conn.Properties["targets"] (or equivalent helper), iterate over each target and
include the appropriate target field on each payload (device_iden, email, or
channel_tag) so an individual push is sent per target, or if you choose not to
support targets explicitly add a clear comment/log and return an error
documenting that Targets are unsupported; ensure payload construction and
json.Marshal happen per-target and preserve existing behavior when no targets
are configured.
| type Sender interface { | ||
| Send(ctx context.Context, conn *models.Connection, data Data) error | ||
| } | ||
|
|
||
| type Data struct { | ||
| Title string | ||
| Message string | ||
| Attachments []mail.Attachment | ||
| } | ||
|
|
||
| var httpClient = &http.Client{Timeout: 30 * time.Second} |
There was a problem hiding this comment.
Interface contract exposes ctx, but no concrete sender honors it.
The Sender.Send(ctx, …) signature implies cancellation/deadline propagation, yet every implementation (ntfy, mattermost, teams, telegram, discord, pushbullet, pushover) builds its HTTP request via http.NewRequest / httpClient.Post / httpClient.PostForm without attaching ctx. Combined with the shared 30s httpClient timeout at Line 24, a shutdown signal or upstream cancellation cannot abort an in-flight request — the caller has to wait up to 30s per request.
Please update all senders to use http.NewRequestWithContext(ctx, …) (or req.WithContext(ctx)) so the ctx in the interface actually matters.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@notification/senders/sender.go` around lines 14 - 24, The Sender.Send(ctx, …)
contract currently ignores ctx because implementations (ntfy, mattermost, teams,
telegram, discord, pushbullet, pushover) create HTTP requests without attaching
the context and/or use httpClient.Post/PostForm; update each sender to build the
request with http.NewRequestWithContext(ctx, ...) (or call req =
req.WithContext(ctx)) and use httpClient.Do(req) so cancellations/deadlines
propagate, keeping the shared httpClient variable and Data struct unchanged.
- Use types.EnvVar for ConnectionTeams/Mattermost WebhookURL (secret hygiene) - Add Properties field to senders.Data and plumb through both send paths - Use http.NewRequestWithContext in all senders for context propagation - Escape message body in Telegram MarkdownV2 sender (not just title) - Support bearer token auth in Ntfy when username is empty - Fix stale shoutrrr comment in suite_test.go - Add Telegram MarkdownV2 escape test
Gavel resultsGavel exited with code . |
Use $(shell which deps) to find an existing system deps binary, falling back to $(LOCALBIN)/deps. The install-deps target only downloads when the resolved path doesn't exist.
1 participant
Summary by CodeRabbit
New Features
Refactor
Tests
Chores