feat(gateway): dispatch operator push on approvals, failed runs, and logout

[marcusrbrown]

Wires the operator push dispatcher into the gateway's real event sources, so opted-in operators get a push nudge when an approval is pending or a run fails, and get deactivated on logout. This is the unit that makes push actually fire — it's still disabled-by-default (nothing dispatches unless push config is present) and changes no approval, run, or session behavior.

Targeting: broadcast to all active subscriptions

The dispatcher nudges every opted-in operator, not a specific one. This is deliberate: the operator dashboard is a shared console (approvals are run-scoped, not launcher-scoped), and no dashboard-operator identity is available at the run-failed or approval-pending seams — runs don't carry one and Discord-triggered runs have only a Discord user id. A per-launcher model would leave Discord-triggered runs notifying nobody and would need operator identity plumbed onto run-state that doesn't exist. Broadcasting is safe because the payload is repo-neutral ("something needs attention, open the dashboard") — it carries no run, repo, prompt, or approval content, so a broadcast leaks nothing beyond a timing signal.

What this wires

• Store: listAllActiveRecords — every active subscription across all operators.
• Dispatcher: reshaped from per-operator to broadcast. Per-record ownership re-check is dropped on this path (there's no specific operator to verify against); dedupe is keyed by the event (run/approval + kind), so one event produces at most one nudge per subscription.
• Approval-pending: fires at the coordinator's pending fan-out, after the existing Discord/registry render.
• Failed-run: fires at both terminal-FAILED sites that carry a failure kind, only on a successful transition, with the same kind that was persisted.
• Logout: a session-revoke hook deactivates that operator's subscriptions, registered at session mint so it closes over the verified GitHub identity.

Every dispatch is fire-and-forget and fail-soft — it can't block or break approval registration, run terminalization, Discord, SSE, or logout. When push config is absent the dispatcher is never constructed and all three seams are no-ops.

Accepted v1 behaviors (documented in code)

• Logout deactivates an operator's subscriptions across all their browsers/devices (subscriptions are identity-scoped, not session-scoped; re-subscribing from a still-active session restores push).
• TTL-expired sessions don't fire the revoke hook; their subscriptions are reclaimed by inactive-record pruning rather than immediate deactivation.
• The broadcast sends sequentially, sized for operator-console scale; a bounded concurrent fan-out is the path if subscription counts grow large.

Verification

Gateway type-check, lint, and full suite green (3158 tests). The three seams have wiring tests (additive to the existing flow, fail-soft under a throwing dispatcher, inert when push is disabled), plus a guard so a transient subscription-list failure doesn't consume the dedupe window and drop a retry.

[fro-bot]

Verdict: CONDITIONAL

The feature itself is well-built: the broadcast dispatcher is correct, fail-soft, and the security posture holds. But the PR ships a stray build artifact at the repo root that must be removed before merge.

Blocking issues

• output.json is a stray artifact accidentally committed (output.json, added in db8404f). This is a code-review agent's JSON output ({"reviewer":"maintainability","findings":[...]}) pointing at program.ts:587-648 — it is not part of the feature, sits at the repo root, and is not in .gitignore. Remove it from the PR (and consider adding it to .gitignore so it can't reappear). Everything else here is non-blocking; fixing this one file clears the merge.

Non-blocking concerns

• Lazy-getter temporal coupling in program.ts (operatorPushDispatcher let at ~L593, assigned inside the operator-web block ~L755, read via get operatorPushDispatcher() on runEngineDeps). The WHY is well-documented in comments, and current init order guarantees assignment before any run launches. The trap: a future maintainer who inlines the getter to a plain property would permanently capture undefined and silently disable push — no compiler/lint/runtime signal. (This is exactly the finding the stray output.json records.) Consider a small guard or a runtime assertion rather than relying on prose comments, but not merge-blocking.
• Dedupe shouldSend is best-effort, not atomic (dispatcher.ts:102). Two concurrent dispatchRunFailed/dispatchApprovalPending calls for the same event can both pass shouldSend and both broadcast, producing duplicate nudges. Acceptable for fire-and-forget push (minor UX noise, not a correctness bug) and consistent with the documented contract — flagging only for awareness.
• listAllActiveRecords returns full records WITH secrets across all operators. Correctly scoped: each record is delivered only to its own endpoint/keys, and the JSDoc explicitly warns it is broadcast-dispatch-only. No cross-operator leak. The repo-neutral payload (i18n keys + closed OperatorFailureKind allowlist in payload-builder.ts) leaks nothing beyond a timing signal, matching the PR's stated design.

Missing tests

None. Coverage is thorough and matches the reshaped behavior:

• Broadcast across multiple operators, dedupe suppression, stale-key skip, dead-subscription markDead with the record's own operatorId, retryable/error no-op, one-record-failure isolation, list-failure not consuming the dedupe slot, and no-sensitive-logging.
• listAllActiveRecords store tests: cross-operator, excludes inactive/tombstoned, empty.
• onSessionRevoke wiring: fires with the captured githubUserId on revoke, and a throwing hook does not break sessionStore.delete.
• Removed verifyStillOwned/per-operator tests are correctly dropped with the collapsed code path. Full suite green locally (3158 passed).

Risk assessment (LOW/MED/HIGH): LOW

Disabled-by-default (dispatcher never constructed without operatorPush config; every call site is optional-chained and fire-and-forget), so blast radius on existing deployments is zero. The removal of verifyStillOwned is sound under the broadcast model — each subscriber only ever receives their own notification, and 410 handling still reaps dead subscriptions. No public-API breaks (all changed signatures are internal gateway deps). The one genuine defect is the committed artifact, which is inert but should not ship.

---

Run Summary

Field	Value
Event	pull_request
Repository	fro-bot/agent
Run ID	29126999182
Cache	hit
Session	ses_0b1e89388ffeIWa31N8QdkDfZu

[fro-bot]

Verdict: PASS

Re-review after 44bc9d6. The single blocking issue from my prior review is resolved: output.json is removed from the tree and /output.json is now in .gitignore so it can't reappear. The feature commits are unchanged from the prior review and remain sound; gateway suite is green (3158 passed).

Blocking issues

None. The stray output.json artifact has been removed and gitignored (.gitignore +3). No feature code changed since the previous review.

Non-blocking concerns

Carried forward from the prior review, both acceptable and non-blocking:

• Lazy-getter temporal coupling in program.ts (operatorPushDispatcher let declared before runEngineDeps, assigned in the operator-web block, read via get operatorPushDispatcher()). Well-documented; current init order guarantees assignment before any run launches. The only trap is a future maintainer inlining the getter to a plain property, which would silently capture undefined — worth a runtime guard someday, but not merge-blocking.
• Dedupe shouldSend is best-effort, not atomic (dispatcher.ts). Concurrent same-event dispatches could both pass and double-send. Acceptable for fire-and-forget push and consistent with the documented contract.

Missing tests

None. Coverage matches the reshaped behavior: broadcast across operators, dedupe suppression, stale-key skip, dead-subscription markDead with the record's own operatorId, retryable/error no-op, one-record-failure isolation, list-failure not consuming the dedupe slot, no-sensitive-logging, listAllActiveRecords store cases (cross-operator / excludes inactive+tombstoned / empty), and onSessionRevoke wiring (fires with captured githubUserId; throwing hook doesn't break sessionStore.delete). Full suite green locally (3158 passed).

Risk assessment (LOW/MED/HIGH): LOW

Disabled-by-default — the dispatcher is never constructed without operatorPush config, and every call site is optional-chained and fire-and-forget, so blast radius on existing deployments is zero. The verifyStillOwned removal is sound under the broadcast model (each subscriber only ever receives their own notification; 410 still reaps dead subs). Repo-neutral payloads leak nothing beyond a timing signal. No public-API breaks. Ship it.

---

Run Summary

Field	Value
Event	pull_request
Repository	fro-bot/agent
Run ID	29127371466
Cache	hit
Session	ses_0b1e89388ffeIWa31N8QdkDfZu