design the review-notifications initiative

Brief, research, alignment, frame, and task breakdown for initiative
02: per-session review notifications pushed into the agent's session
over the Claude Code channels contract. An agent subscribes to
changes it pushed; a poller turns Gerrit activity into
self-sufficient llmxml payloads, filtered by ownership, account
exclusions, and content regexes; terminal states end subscriptions
automatically. Tasks map to issues #49-#63 under the Notifications
1-4 milestones.

Two ADRs record the load-bearing decisions: delivery rides the
claude/channel experimental capability with a raw-notification
transport seam (the pinned Go SDK has no generic sender), and scope
is explicit per-session subscriptions rather than account-wide
watching, which failed twice in the field. Glossary gains Review
notifications, Subscription, and Channel entries.

Author: xobotyi

design-docs/02-review-notifications.alignment.md

@@ -0,0 +1,84 @@
+# Alignment: Review notifications
+
+- **Date:** 2026-07-13
+- **Brief:** design-docs/02-review-notifications.brief.md
+- **Research:** design-docs/02-review-notifications.research.md
+- **Glossary:** docs/glossary.md (entries added: Review notifications, Subscription, Channel)
+
+## Patterns Adopted
+
+- **behaviorFlag flag/mirror resolution** — every new option (enable, account exclusions, content-pattern
+  exclusions, include-own, poll interval) joins `config.Load` as a `behaviorFlag` with a `GERRIT_MCP_*` mirror.
+  Found in internal/config/config.go:88-117, universal (5/5 existing flags). Errors aggregate via `errors.Join`;
+  invalid exclusion regexes fail startup loudly through the same path.
+- **Disabled = never constructed** — subscribe/unsubscribe tools are registered only when the enable flag is on,
+  mirroring how group-gated tools are simply never registered (cmd/go-gerrit-mcp/main.go:77-81). Gating is by the
+  dedicated flag, not by `registry.Resolve`: this is not a capability group.
+- **llmxml rendering reuse** — notification payloads reuse the comment-thread pipeline and shared helpers
+  (`accountLabel`, `timestamp`; internal/tools/get-change-comments.go, get-change.go) so pushed activity reads
+  exactly like `get_change_comments` output the model already knows.
+- **golib/e sentinels with structured fields** — for the subscription store, poller, and emission path, matching
+  internal/gerritclient conventions.
+- **Model-facing prompts drive behavior** — server instructions and tool descriptions teach the agent when to
+  subscribe (after pushing a reviewable change, when awaiting a review outcome) and what auto-unsubscription means,
+  following the first-contact prompt work.
+
+## Deviations
+
+- **Instructions become composed** — the static `const instructions` (cmd/go-gerrit-mcp/main.go:23) grows a
+  conditional review-notifications section emitted only when the feature is enabled. Zero-config instructions stay
+  byte-identical to today.
+- **First background goroutine** — the poller is the codebase's first concurrent component. It inherits the signal
+  context from `run`, logs to stderr only (stdout belongs to the stdio transport), and terminates with the session.
+  At its boundary it logs-and-retries poll failures instead of returning them — a background loop has no caller;
+  "handle once" is satisfied by logging.
+- **Raw notification emission via wrapping transport** — the pinned Go SDK has no public generic notification
+  sender, so the channel event is written as an ID-less `jsonrpc.Request` through a transport wrapper that captures
+  the `mcp.Connection` (`Write` is documented concurrency-safe). Replace with the SDK's native API when one ships.
+
+## Current State
+
+The server is entirely request-driven: tools fetch from Gerrit on demand, render llmxml, return. No background
+work, no per-session state beyond the shared client, no server-initiated messages. An agent that pushed a change
+for review learns about review activity only by re-reading the change.
+
+## Desired End State
+
+With the feature enabled, the server declares the `claude/channel` capability, registers subscribe/unsubscribe
+tools, and appends channel guidance to its instructions. The agent subscribes to changes it pushed or awaits; a
+poller (60s default interval) batches a query over subscribed changes, cursors per change, and turns new activity —
+human comments and replies, votes (bare votes included), status transitions — into self-sufficient llmxml payloads
+pushed as `notifications/claude/channel` events. Filters drop the caller's own activity (default; flag to include),
+excluded accounts, and messages matching operator-configured regexes. Merged or abandoned changes produce a final
+notification announcing automatic unsubscription and leave the subscription set. Zero configuration keeps the
+server byte-identical to today: no tools, no capability, no instructions, no polling.
+
+## Resolved Questions
+
+- Delivery mechanism → the Claude Code channels contract; self-sufficient payloads; no consumption tool (ADR 2.1).
+- Notification scope → explicit per-session subscriptions; no account-wide mode (ADR 2.2).
+- Feature gating → dedicated flag family with env mirrors; deliberately not a capability group.
+- Bot noise → operator-configured account exclusion list plus content-pattern regexes; no tag-based heuristics.
+  Invalid regex → aggregated startup failure.
+- Own activity → skipped by default, included via flag.
+- Bare human votes → included; often the awaited review outcome.
+- Poll interval → 60s default, flag-configurable.
+- Terminal states → automatic unsubscription with a final announcing notification.
+- Persistence → none; session lifetime; resuming agents re-subscribe.
+
+## Not Yet Specified
+
+- Exact flag, mirror, and tool names — frame-stage, against the glossary.
+- Notification payload element design (tag names, meta attributes, how threads/votes/transitions render) —
+  frame-stage, reusing the comment-render vocabulary.
+- Poller mechanics detail: one batched `change:X OR change:Y` query vs per-change fetches, cursor granularity,
+  dedup keys — frame-stage; research confirms both endpoints and per-vote dates exist.
+- `ServerOptions.Capabilities` override vs inferred tools capability — needs a learning test during implementation
+  (research flags the interaction).
+- Research-preview operational caveats surface (README wording for `--dangerously-load-development-channels`) —
+  frame-stage.
+
+## Recorded ADRs
+
+- [ADR 2.1: Review notifications ride the Claude Code channels contract](./adr/2.1-channels-contract-delivery.md)
+- [ADR 2.2: Session-scoped explicit subscriptions over account-wide watching](./adr/2.2-session-scoped-subscriptions.md)

design-docs/02-review-notifications.brief.md

@@ -0,0 +1,82 @@
+# Brief: Review notifications
+
+- **Date:** 2026-07-13
+- **Initiative:** 02-review-notifications
+
+## Motivation
+
+An agent that pushes a change for review has no way to learn about review activity except manually re-reading the
+change, and an earlier account-wide poller (outside this project) proved the naive approach wrong twice: it notified
+about every change the account owned — so concurrent agent sessions woke each other up — and it had no activity
+filtering, so CI votes produced the same wake-ups as human review. The server should track review activity for
+exactly the changes a session subscribed to and push it to that session alone.
+
+## Goals
+
+- Two tools — subscribe and unsubscribe by change identifier — registered only when the review-notifications flag is
+  enabled; model-facing prompts (server instructions, tool descriptions) teach the agent to subscribe after pushing a
+  reviewable change or whenever it awaits a review outcome.
+- Self-sufficient notifications: after subscribing, the agent starts receiving review activity for that change as
+  pushed notifications whose payload carries the new activity inline (rendered llmxml — comment threads, review
+  messages, votes, status transitions). No consumption tool; nothing to fetch afterwards.
+- Per-instance, in-memory subscription set and cursor. Session scoping falls out of the stdio process model: one
+  server per agent session, so concurrent sessions never see each other's subscriptions. No persistence; a restarted
+  session re-subscribes.
+- Polling-based detection: the server polls Gerrit for subscribed changes on an interval; an empty subscription set
+  polls nothing.
+- Event filtering:
+  - Events authored by the authenticated account are skipped by default; a flag includes them.
+  - A configurable account exclusion list (usernames/IDs) silences unwanted accounts, e.g. noisy bots. No tag-based
+    bot heuristics: whether a bot is noise or signal (a CI verdict) is the operator's call, and per-user defaults
+    belong in the operator's shared client configuration, not in server heuristics.
+  - A configurable content-pattern exclusion list drops individual messages and comments by their text, regardless
+    of author. Account exclusion is too coarse for accounts that mix signal and noise: an AI reviewer's verdict
+    matters while its "review started" announcement does not, and a human comment that merely summons a bot is
+    noise from an account that otherwise matters.
+  - Human votes without comment text are included — a bare vote is often the awaited review outcome.
+- Automatic unsubscription on terminal states: when a subscribed change reaches merged or abandoned, the review is
+  almost certainly over — the final notification carries the transition, tells the model the subscription ended
+  automatically and no further notifications will arrive, and the poller drops the change.
+- Dedicated flags with environment mirrors, following the existing flag/mirror pattern: enable flag, account
+  exclusion list, content-pattern exclusion list, include-own toggle, poll interval. This is deliberately not a
+  capability group.
+
+## Non-goals
+
+- Notification channel configuration (multiple channels, per-channel settings) — dedicated follow-up work.
+- Persistence of subscriptions or cursors across sessions.
+- Attention-set integration, SSH stream-events, webhooks — alternative sources deliberately rejected for now.
+- Notifying about changes the account owns but the session never subscribed to.
+
+## Constraints
+
+- Zero configuration means the feature does not exist: no tools registered, no instructions emitted (ADR 1.2 safety
+  posture: defaults are contractual).
+- Subscribing is trail-free — pure local state, nothing visible on the Gerrit instance.
+- Project scoping continues to confine every operation, subscriptions included.
+- Notification payloads are llmxml (ADR 1.3).
+
+## Key risks
+
+- Gerrit's `updated` timestamp moves on any mutation, so detail fetches may be triggered by events the filters then
+  discard — a cost concern, not a correctness one.
+- A dead session cannot unsubscribe; the poller must terminate subscriptions meaningfully when a change reaches a
+  terminal state (merged/abandoned) instead of polling forever.
+- Push delivery depends on what MCP clients actually surface to the model mid-session; payload design must not
+  assume more than the protocol guarantees.
+
+## Flagged term ambiguities
+
+- "Review notification channel" — the user-facing name of the feature and its flag family; needs a canonical
+  glossary entry distinguishing it from capability groups.
+- "Subscription" — the per-session tracked set; needs a definition that pins its session lifetime and trail-free
+  nature.
+
+## Questions for research
+
+- How MCP server-to-client push works in the Go SDK (notification types available to a stdio server) and what
+  mainstream clients surface to the model mid-session.
+- How to detect new votes and distinguish vote-only updates: `ChangeMessageInfo` tags/epochs vs detailed labels.
+- Whether comment deltas are derivable from change messages alone or require comment-list diffing per poll.
+- Batched change query behavior and limits for `change:X OR change:Y` polling.
+- What exactly moves a change's `updated` field, and its timestamp granularity.

design-docs/02-review-notifications.frame.md

@@ -0,0 +1,161 @@
+# Frame: Review notifications
+
+- **Date:** 2026-07-13
+- **Alignment:** design-docs/02-review-notifications.alignment.md
+
+Naming fixed by this frame (per glossary): tools `subscribe_change` / `unsubscribe_change`; flag family
+`--review-notifications` (enable), `--review-notifications-poll-interval`, `--review-notifications-include-own`,
+`--review-notifications-exclude-accounts`, `--review-notifications-exclude-patterns`, each with the matching
+`GERRIT_MCP_REVIEW_NOTIFICATIONS*` mirror. The `--notify-*` prefix is deliberately avoided — `notify` already means
+email-notify levels on review posting. New package: `internal/notifications` (subscription store, poller, filters,
+payload rendering); transport wrapper lives beside the server assembly.
+
+## Phase 1: Tracer Bullet — subscribe, poll, push
+
+The thinnest wire through every layer: one flag, one tool, a poller that detects only `updated` movement, a minimal
+payload, and a channel notification that reaches a real client.
+
+**Components:**
+
+- `internal/config` — `--review-notifications` behaviorFlag (+ mirror); interval flag with 60s fallback.
+- `internal/notifications` — subscription store (mutex-guarded map keyed by change number, per-change cursor =
+  last-seen `updated`); poller goroutine: batched `change:A OR change:B` query per tick, emits a bare event when
+  `updated` moved.
+- `internal/tools` — `subscribe_change` tool (validates the change exists and is in project scope via
+  `GetChange`, stores it, returns llmxml ack naming current status and patch set).
+- `cmd/go-gerrit-mcp` — capability declaration (`Experimental["claude/channel"]`) and conditional instructions
+  sentence when enabled; wrapping transport capturing the `mcp.Connection`; poller lifecycle bound to the signal
+  context.
+- Emission — ID-less `jsonrpc.Request`, method `notifications/claude/channel`, params `{content, meta}`; content is
+  a minimal `<review_activity change="..." status="...">` element for now.
+
+**Testing strategy:**
+
+- Learning tests first (see Learning Tests) — SDK capability override and raw-write behavior are assumptions until
+  executed.
+- Unit: store add/duplicate/remove; poller tick against `httptest` Gerrit stub (updated moved / not moved / query
+  error → logged and retried).
+- Manual probe: JSON-RPC pipe with a subscription, mutate the change live, observe the notification line on stdout.
+
+**Verification gate:**
+
+- Live end-to-end against Claude Code: `claude --dangerously-load-development-channels server:gerrit`, subscribe to
+  a sandbox change, post a comment on it from the UI, the `<channel source="gerrit">` block appears in the session.
+  This gate validates the entire contract stack before any depth is built.
+
+**Acceptance criteria:**
+
+- [ ] Zero-config server is byte-identical to today: no tool, no capability, no instructions change, no goroutine.
+- [ ] With the flag on, subscribing then mutating the change produces exactly one notification per poll tick with
+      movement; quiet ticks produce nothing; empty subscription set skips the query entirely.
+
+## Phase 2: Real activity deltas and terminal states
+
+**Components:**
+
+- `internal/notifications` — cursor grows per-kind high-water marks; event extraction from `GetChange` detail +
+  `ListChangeComments`: new change messages (author, date, tag, revision), new votes (`ApprovalInfo.Date` newer
+  than cursor, value, label), new/updated comment threads (reuse the thread pipeline), status transitions.
+- `internal/tools` — `unsubscribe_change` tool; payload rendering: `<review_activity>` composing existing
+  vocabulary — `<message>`, `<vote>`, thread/`<comment>` elements exactly as `get_change_comments` renders them;
+  `meta` carries `change`, `kind`, `project`.
+- Terminal handling — merged/abandoned detected from status: final notification carries the transition plus
+  `subscription="ended"` semantics in prose, change leaves the store.
+
+**Testing strategy:**
+
+- Golden tests for payload rendering (existing golden harness in `internal/tools`).
+- Unit: cursor semantics — replay the same poll twice, second emits nothing; vote-only update emits a vote event;
+  comment burst groups into one payload.
+- Live probe against the production incident change for thread-render parity.
+
+**Verification gate:**
+
+- A subscribed change taken through comment, reply, vote, and submit on a sandbox produces the expected
+  notification sequence ending in the auto-unsubscribe notice, and the store is empty afterwards.
+
+**Acceptance criteria:**
+
+- [ ] Every activity kind (message, vote, thread, transition) renders and pushes; nothing repeats across ticks.
+- [ ] Terminal states always end the subscription with the announcing notification, including when the terminal
+      transition and other activity arrive in the same tick.
+
+## Phase 3: Filters and model-facing prompts
+
+**Components:**
+
+- `internal/config` — `include-own`, `exclude-accounts`, `exclude-patterns` flags; regex compilation at load with
+  aggregated fail-loud errors.
+- `internal/notifications` — filter chain applied per extracted event: self-authorship (default drop, flag keeps),
+  excluded accounts (username or numeric ID), content regexes against message/comment text. Filtered-to-empty
+  ticks emit nothing.
+- `internal/tools` / `cmd` — full instructions section: when to subscribe, what arrives, auto-unsubscribe meaning,
+  re-subscribe-after-restart guidance; tool descriptions final.
+
+**Testing strategy:**
+
+- Unit: table tests per filter and their composition; config tests for invalid regex (startup fails naming the
+  pattern) and mirror resolution.
+- Golden: instructions output with the feature on/off.
+
+**Verification gate:**
+
+- Live: a bot-authored comment matching an exclusion pattern produces no notification while a human reply in the
+  same tick does.
+
+**Acceptance criteria:**
+
+- [ ] Own activity silent by default, delivered with the include flag.
+- [ ] Account and pattern exclusions drop matching events only; invalid regex aborts startup with an aggregated
+      error naming the pattern.
+
+## Phase 4: Hardening and operator docs
+
+**Components:**
+
+- `internal/notifications` — poll failure policy (log, keep subscription, retry next tick; repeated failures do not
+  kill the poller), change-became-inaccessible handling (scope loss / deletion → end subscription with notice),
+  clean shutdown ordering on context cancellation.
+- `README.md` — feature section: flags table rows, channel enablement (`--channels` / research-preview
+  `--dangerously-load-development-channels server:<name>`), org-policy caveat, per-project configuration synergy.
+- `docs/` — glossary already updated; verify vocabulary consistency across tool descriptions and README.
+
+**Testing strategy:**
+
+- Unit: error-path tests (Gerrit 5xx, 404 on subscribed change, ctx cancellation mid-tick).
+- `go test -race` across the package (first concurrent component in the codebase).
+
+**Verification gate:**
+
+- Kill/restart and failure-injection probes leave no goroutine leaks (`-race` clean, poller exits with the session).
+
+**Acceptance criteria:**
+
+- [ ] Poller survives transient Gerrit failures and shuts down cleanly with the session.
+- [ ] README documents the feature end-to-end including research-preview caveats.
+
+## Learning Tests
+
+- **SDK capabilities override** — setting `ServerOptions.Capabilities` with `Experimental["claude/channel"]`
+  preserves the inferred tools capability (research flags the interaction; verify before Phase 1 builds on it).
+- **Raw notification write** — an ID-less `jsonrpc.Request` written via the wrapped `mcp.Connection` concurrently
+  with SDK traffic is framed correctly and received by a client (probe with a stub client before trusting it).
+- **Channel injection** — Claude Code with the development flag renders our notification as a `<channel>` block
+  (Phase 1 gate doubles as this test).
+- **Gerrit vote messages** — a bare vote on the production instance produces a `ChangeMessageInfo` and a dated
+  `ApprovalInfo`; default query results include `updated` without `o=` options.
+
+## Phase Sequence
+
+Phase 1 (tracer bullet, no deps)
+    ↓
+Phase 2 (depends on Phase 1)
+Phase 3 (config/filter work can start against Phase 1; filter-chain integration depends on Phase 2's event model)
+    ↓
+Phase 4 (depends on Phases 2–3)
+
+## Scope Boundaries
+
+**In scope:** subscription tools, poller, filters, payload rendering, channel emission, operator docs.
+**Out of scope:** multiple notification channels and per-channel configuration; subscription persistence;
+attention-set/stream-events/webhook sources; permission relay; official channel-plugin packaging.