Skip to content

notify: wire the webhook sink behind an opt-in env var - #215

Closed
gnanam1990 wants to merge 1 commit into
mainfrom
feat/notify-webhook
Closed

notify: wire the webhook sink behind an opt-in env var#215
gnanam1990 wants to merge 1 commit into
mainfrom
feat/notify-webhook

Conversation

@gnanam1990

@gnanam1990 gnanam1990 commented Jun 15, 2026

Copy link
Copy Markdown
Collaborator

Summary

The WebhookSink (best-effort Slack / generic JSON POST when a turn completes or awaits input) was fully built, redaction-safe, and unit-tested — but never attached to a Notifier. deadcode therefore flagged its whole machinery as unreachable, and the v16 audit listed it as "wire or remove notify.WebhookSink". This wires it.

What changed

  • New helper notify.MaybeAddWebhookSink(n, env, logf):
    • reads ZERO_NOTIFY_WEBHOOK_URL (and optional ZERO_NOTIFY_WEBHOOK_SUMMARY)
    • no-op when the URL is unset/blank → safe to call unconditionally
    • the URL (which typically embeds a secret token) comes from the environment, so it never has to be written into an on-disk config file
  • Attached at both notifier construction sites:
    • headless exec — failed deliveries log to stderr (never stdout, which may carry stream-json); the sink redacts before logging
    • TUIlogf=nil, because the TUI owns the alt-screen and a stderr write would corrupt the display

Behavior / safety

  • Opt-in & fail-closed: with ZERO_NOTIFY_WEBHOOK_URL unset, nothing is attached and behavior is identical to before.
  • The sink remains gated by the existing Mode/focus policy (TestNotifierOffSuppressesSinks proves Mode==off suppresses sinks), so a webhook only delivers when notifications are enabled (e.g. --notify both). No change to default notification behavior.
  • Secret hygiene unchanged: the sink already redacts URL + message + links before any POST/log.

Verification

  • deadcode: 100 → 93 unreachable funcs — the 7 WebhookSink entries (NewWebhookSink, Emit, text, redactLinks, log, eventType, readSnippet) become reachable; no new dead code.
  • New tests: attaches + delivers (httptest, asserts summary propagation), no-op on blank/whitespace URL, nil-guard (nil notifier / nil env).
  • gofmt · vet · build (host + linux + windows) · full go test ./... (-race on notify) · staticcheck (no new) · govulncheck (0) all green.

Summary by CodeRabbit

Release Notes

  • New Features
    • Run notifications can now be delivered to an external webhook via the ZERO_NOTIFY_WEBHOOK_URL environment variable for integration with external systems
    • Optional ZERO_NOTIFY_WEBHOOK_SUMMARY environment variable allows customizing the run summary in webhook payloads
    • Webhook delivery errors are logged without affecting application output

The WebhookSink (Slack / generic JSON POST on turn completion) was fully
implemented, redaction-safe, and tested — but never attached to a Notifier, so
deadcode flagged its entire machinery (NewWebhookSink, Emit, text, redactLinks,
log, eventType, readSnippet) as unreachable.

Wire it via a small, testable helper, MaybeAddWebhookSink(n, env, logf):
  - reads ZERO_NOTIFY_WEBHOOK_URL (+ optional ZERO_NOTIFY_WEBHOOK_SUMMARY)
  - no-op when the URL is unset/blank, so it is safe to call unconditionally
  - sourcing the URL from the environment keeps the secret token out of any
    on-disk config file

Attached at both notifier construction sites:
  - headless exec: failed deliveries log to stderr (never stdout); the sink
    redacts before logging
  - TUI: logf=nil so a delivery failure can't corrupt the alt-screen

The sink stays gated by the existing Mode/focus policy (verified by
TestNotifierOffSuppressesSinks), so a webhook only fires when notifications are
enabled (e.g. --notify both) — no change to default behavior. Opt-in and
fail-closed: with the env var unset nothing is attached.

deadcode: 100 -> 93 unreachable funcs (the 7 WebhookSink entries become
reachable); no new dead code. gofmt/vet/build(host+linux+windows)/test -race/
staticcheck(no new)/govulncheck(0) all pass.
@gnanam1990

Copy link
Copy Markdown
Collaborator Author

Consolidating the audit-backlog work into a single PR with one commit per change (per request). This change is preserved verbatim as a commit there.

@gnanam1990 gnanam1990 closed this Jun 15, 2026
@github-actions

Copy link
Copy Markdown
Contributor

Zero automated PR review

Verdict: No blockers found

Blockers

  • None found.

Validation

  • [pass] Diff hygiene: git diff --check
  • [pass] Tests: go test ./...
  • [pass] Build: go run ./cmd/zero-release build
  • [pass] Smoke build: go run ./cmd/zero-release smoke

Scope

Head: 1519ada0f28d
Changed files (4): internal/cli/exec.go, internal/notify/webhook_wire.go, internal/notify/webhook_wire_test.go, internal/tui/model.go

This deterministic review checks validation status and basic diff hygiene. A human reviewer still owns product judgment and design quality.

@coderabbitai

coderabbitai Bot commented Jun 15, 2026

Copy link
Copy Markdown

Review Change Stack

Caution

Review failed

Pull request was closed or merged during review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: f0f5b7cd-886a-4314-98c6-fb7709ba2c41

📥 Commits

Reviewing files that changed from the base of the PR and between f857c4a and 1519ada.

📒 Files selected for processing (4)
  • internal/cli/exec.go
  • internal/notify/webhook_wire.go
  • internal/notify/webhook_wire_test.go
  • internal/tui/model.go

Walkthrough

Adds opt-in webhook fan-out for run notifications. A new MaybeAddWebhookSink helper reads ZERO_NOTIFY_WEBHOOK_URL and ZERO_NOTIFY_WEBHOOK_SUMMARY from the environment, performs nil/empty guards, and attaches a WebhookSink to the provided notifier. This helper is called in both the CLI exec path and the TUI model during notifier initialization.

Changes

Webhook Notification Fan-out

Layer / File(s) Summary
MaybeAddWebhookSink helper and tests
internal/notify/webhook_wire.go, internal/notify/webhook_wire_test.go
Defines EnvWebhookURL/EnvWebhookSummary constants and MaybeAddWebhookSink with nil-notifier, nil-env, and blank-URL guards; tests cover end-to-end delivery via an httptest server, blank-URL noop, and nil-input safety.
CLI and TUI integration callsites
internal/cli/exec.go, internal/tui/model.go
Both callsites invoke MaybeAddWebhookSink immediately after constructing the per-run notifier; CLI routes delivery failures to stderr, TUI silences them to avoid corrupting the alt-screen display.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

  • Gitlawb/zero#182: Adds the underlying WebhookSink/WebhookConfig and sink fan-out that MaybeAddWebhookSink depends on to register the sink at the callsites introduced in this PR.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 28.57% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main change: wiring the webhook sink behind an opt-in environment variable, which is the primary purpose of this changeset.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/notify-webhook

Comment @coderabbitai help to get the list of available commands and usage tips.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant