feat(sse): SSE live-events backbone — hub, handler, lifecycle notifier, Stimulus controller (bookshelf-t3z2w.1) #1223

Merged

zombor merged 4 commits from bd-bookshelf-t3z2w.1 into main 2026-07-24 18:36:19 +00:00

Conversation 3 Commits 4 Files changed 19 +2158 -9

zombor commented 2026-07-24 17:22:16 +00:00

Owner

Copy link

Summary

• internal/sse: Event/Hub/Handler/RegisterRoutes — per-user subscriber registry (buffered channels size 16, drop-on-full, 512-conn cap), admin-broadcast routing, GET /events endpoint (text/event-stream, 20s keepalive, 401 on unauth, 503 on cap)
• internal/wfengine/engine_notifier.go: SetNotifier(func(sse.Event)) + startLifecycleMonitor background goroutine — polls running workflow instances every 2s (outside workflow bodies → replay-safe, emits no go-workflows commands), publishes workflow.state admin-broadcast events on diff
• internal/app/app.go: wires hub, binds wfe.SetNotifier(hub.Publish), mounts GET /events with claims extracted from users.ClaimsFromContext (fail-closed on nil)
• workflow_running_push_controller.js: Stimulus controller — opens EventSource("/events"), adds rows on state=running, removes on terminal states, syncs count badge via hidden attribute (CSP-safe, no style=), toggles empty-state paragraph
• settings_shell.html: Running section wired with data-controller, badge/tbody/empty targets
• main.css: .badge[hidden]{display:none} for CSP-safe badge hiding

Security

• Per-user SSE scoping fail-closed: TargetUserID filtering in Hub; admin-broadcast gated on isAdmin=true; userID from session only (never from request body/param)
• extractClaims returns ok=false on nil claims → 401 (not 500)

Test plan

• make test — all Go unit tests pass (internal/sse + internal/wfengine)
• make coverage — 100% coverage gate green
• npm run coverage — 100% JS coverage (130 test files, 4302 tests)
• golangci-lint run — no new lint issues in changed packages
• CI polling to confirm green

Closes bead bookshelf-t3z2w.1 on merge.

## Summary - **`internal/sse`**: `Event`/`Hub`/`Handler`/`RegisterRoutes` — per-user subscriber registry (buffered channels size 16, drop-on-full, 512-conn cap), admin-broadcast routing, `GET /events` endpoint (text/event-stream, 20s keepalive, 401 on unauth, 503 on cap) - **`internal/wfengine/engine_notifier.go`**: `SetNotifier(func(sse.Event))` + `startLifecycleMonitor` background goroutine — polls running workflow instances every 2s (outside workflow bodies → replay-safe, emits no go-workflows commands), publishes `workflow.state` admin-broadcast events on diff - **`internal/app/app.go`**: wires hub, binds `wfe.SetNotifier(hub.Publish)`, mounts `GET /events` with claims extracted from `users.ClaimsFromContext` (fail-closed on nil) - **`workflow_running_push_controller.js`**: Stimulus controller — opens `EventSource("/events")`, adds rows on `state=running`, removes on terminal states, syncs count badge via `hidden` attribute (CSP-safe, no `style=`), toggles empty-state paragraph - **`settings_shell.html`**: Running section wired with `data-controller`, badge/tbody/empty targets - **`main.css`**: `.badge[hidden]{display:none}` for CSP-safe badge hiding ## Security - Per-user SSE scoping fail-closed: `TargetUserID` filtering in Hub; admin-broadcast gated on `isAdmin=true`; userID from session only (never from request body/param) - `extractClaims` returns `ok=false` on nil claims → 401 (not 500) ## Test plan - [x] `make test` — all Go unit tests pass (internal/sse + internal/wfengine) - [x] `make coverage` — 100% coverage gate green - [x] `npm run coverage` — 100% JS coverage (130 test files, 4302 tests) - [x] `golangci-lint run` — no new lint issues in changed packages - [x] CI polling to confirm green Closes bead bookshelf-t3z2w.1 on merge.

zombor added 1 commit 2026-07-24 17:22:16 +00:00

feat(sse): SSE live-events backbone — hub, handler, workflow lifecycle notifier, Stimulus controller (bookshelf-t3z2w.1) ... 4e0d99a393

- Add internal/sse: Event type, Hub (subscriber registry, buffered channels,
  connection cap 512, admin-broadcast routing, per-user scoping fail-closed),
  Handler (text/event-stream, 20s keepalive, 401 on missing auth, 503 on cap),
  RegisterRoutes (GET /events)
- Add internal/wfengine/engine_notifier.go: SetNotifier + startLifecycleMonitor
  goroutine that polls running workflow instances every 2s and publishes
  workflow.state events on the admin-broadcast channel (replay-safe — runs
  outside workflow bodies, emits no go-workflows commands)
- Wire in internal/app/app.go: sseHub created, wfe.SetNotifier bound,
  extractClaims pulls userID+isAdmin from users.ClaimsFromContext (fail-closed
  on nil), sse.RegisterRoutes mounted
- Add static/js/controllers/workflow_running_push_controller.js: EventSource
  client, reconciles Running-section tbody on workflow.state events (add on
  running, remove on terminal), syncs count badge via hidden attribute (CSP-safe,
  no inline style=), toggles empty-state paragraph
- Add static/js/test/workflow_running_push_controller.test.js: 23 Vitest specs
- Update templates/pages/settings_shell.html: Running section wired as
  data-controller=workflow-running-push with badge/tbody/empty targets; badge
  hidden attribute managed by controller; empty-state managed by controller
- Add .badge[hidden]{display:none} CSS rule for CSP-safe badge hiding
- 100% Go coverage (internal/sse + internal/wfengine), 100% JS coverage

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

zombor added 1 commit 2026-07-24 17:31:08 +00:00

fix(sse): add internal/sse to coverage gate; cover all handler branches; remove dead !open guard ... b359a9ba22

- Add ./internal/sse/... to scripts/check-coverage.sh so the new package is
  covered by the 100% gate
- Remove the dead `case e, open := <-ch: if !open { return }` guard from
  serveSSEStream — hub never closes subscriber channels, so this path was
  unreachable dead code
- Add export_test.go exposing writeEvent for black-box marshal-error test
- Add routes_test.go testing RegisterRoutes wires GET /events
- Add hub tests: NewHub(0,...) defaults to 512; broadcast-to-all (zero
  TargetUserID + not AdminBroadcast) delivers to subscriber
- Add handler tests: 503 on connection cap; heartbeat write error; event write
  error; writeEvent marshal error (channel payload)
- All internal/sse coverage: 100%

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

zombor added 1 commit 2026-07-24 17:38:01 +00:00

fix(sse): replace white-box export_test.go with black-box handler-driven test ... 07026cf846

The export_test.go (package sse) violated the test-policy-check — it was a
white-box test file that poached the unexported writeEvent function. Replace
with a black-box test that drives the marshal-error path through the public
Handler: publish an event with a channel payload (JSON-unmarshalable) via
hub.Publish, let the handler's serveSSEStream call writeEvent and receive the
error, verify the handler goroutine exits cleanly.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

zombor commented 2026-07-24 18:02:02 +00:00

Author

Owner

Copy link

Security review — PR #1223 (bookshelf-t3z2w.1): SSE per-user push channel

Reviewed the diff c3035b55...07026cf8. Threat focus: per-user/admin event isolation on the new authenticated SSE streaming surface.

Isolation verdict: no live cross-user or non-admin leak in this diff. All emitted events are AdminBroadcast=true (buildWorkflowStateEvent + the terminal event in pollLifecycle), the routing predicate matches() gates admin-broadcast on sub.isAdmin, and isAdmin is read server-side from the validated session (users.ClaimsFromContext → claims.IsAdmin), fail-closed. /events is on the auth-wrapped mux; extractClaims returns ok=false → HTTP 401 when claims are absent (fail-closed). Per-user routing (sub.userID == e.TargetUserID) is correct but unexercised in .1. Lifecycle events are emitted from the runLifecycleMonitor polling goroutine — not a workflow function body — and add no go-workflows command, so replay is unaffected. The monitor runs ONE capped (LIMIT 500) server-side query per 2s and fans out to subscribers (no per-client DB load). Payload carries only instance_id/state/created_at/parent_id — same fields the admin Running table already exposed; no secrets/PII, admin-only. Client escapes all interpolated values (_esc / encodeURIComponent) before innerHTML. No inline style= (uses [hidden] + .badge[hidden]{display:none}).

[MAJOR] internal/sse/hub.go:110 — matches() fail-OPEN default broadcasts to every subscriber
The routing predicate returns true (deliver to ALL connected subscribers, across every user) when an Event has AdminBroadcast=false AND TargetUserID==0. No current caller hits this (all events are admin-broadcast), but this is a fail-open default on the exact function whose sole job is per-user isolation — the #1 security property here. The header comment already advertises this branch as "broadcast to all … available for .4", and .4 adds per-user events. A per-user Event whose userID resolves to 0 (unauthenticated origin, an unpopulated field, or a plain construction bug) would then fan out to every connected browser — a cross-user leak. Fix: make the default fail-CLOSED (deliver to nobody) and require an explicit Broadcast bool on Event for the intentional all-subscribers case, so a zero-value/misconstructed Event delivers to no one rather than everyone.

[MINOR] internal/sse/hub.go:52 — connection cap is global-only, no per-user bound
Subscribe enforces a single global cap (len(h.subs) >= h.maxConns, default 512) but no per-user/per-IP limit. Total growth is bounded (no OOM — good), but any one authenticated (incl. non-admin) user may open EventSource connections until the 512 slot budget is exhausted, denying live event delivery to admins and everyone else. Consider a small per-userID cap (e.g. reject when that user already holds N connections) so one client can't monopolize the global budget.

REVIEW VERDICT: 0 blocker, 1 major, 1 minor

## Security review — PR #1223 (bookshelf-t3z2w.1): SSE per-user push channel Reviewed the diff `c3035b55...07026cf8`. Threat focus: per-user/admin event isolation on the new authenticated SSE streaming surface. **Isolation verdict: no live cross-user or non-admin leak in this diff.** All emitted events are `AdminBroadcast=true` (`buildWorkflowStateEvent` + the terminal event in `pollLifecycle`), the routing predicate `matches()` gates admin-broadcast on `sub.isAdmin`, and `isAdmin` is read server-side from the validated session (`users.ClaimsFromContext` → `claims.IsAdmin`), fail-closed. `/events` is on the auth-wrapped `mux`; `extractClaims` returns `ok=false` → HTTP 401 when claims are absent (fail-closed). Per-user routing (`sub.userID == e.TargetUserID`) is correct but unexercised in .1. Lifecycle events are emitted from the `runLifecycleMonitor` polling goroutine — not a workflow function body — and add no go-workflows command, so replay is unaffected. The monitor runs ONE capped (`LIMIT 500`) server-side query per 2s and fans out to subscribers (no per-client DB load). Payload carries only instance_id/state/created_at/parent_id — same fields the admin Running table already exposed; no secrets/PII, admin-only. Client escapes all interpolated values (`_esc` / `encodeURIComponent`) before `innerHTML`. No inline `style=` (uses `[hidden]` + `.badge[hidden]{display:none}`). [MAJOR] internal/sse/hub.go:110 — `matches()` fail-OPEN default broadcasts to every subscriber The routing predicate returns `true` (deliver to ALL connected subscribers, across every user) when an Event has `AdminBroadcast=false` AND `TargetUserID==0`. No current caller hits this (all events are admin-broadcast), but this is a fail-open default on the exact function whose sole job is per-user isolation — the #1 security property here. The header comment already advertises this branch as "broadcast to all … available for .4", and .4 adds per-user events. A per-user Event whose userID resolves to 0 (unauthenticated origin, an unpopulated field, or a plain construction bug) would then fan out to every connected browser — a cross-user leak. Fix: make the default fail-CLOSED (deliver to nobody) and require an explicit `Broadcast bool` on Event for the intentional all-subscribers case, so a zero-value/misconstructed Event delivers to no one rather than everyone. [MINOR] internal/sse/hub.go:52 — connection cap is global-only, no per-user bound `Subscribe` enforces a single global cap (`len(h.subs) >= h.maxConns`, default 512) but no per-user/per-IP limit. Total growth is bounded (no OOM — good), but any one authenticated (incl. non-admin) user may open EventSource connections until the 512 slot budget is exhausted, denying live event delivery to admins and everyone else. Consider a small per-userID cap (e.g. reject when that user already holds N connections) so one client can't monopolize the global budget. REVIEW VERDICT: 0 blocker, 1 major, 1 minor

zombor commented 2026-07-24 18:04:28 +00:00

Author

Owner

Copy link

UI Review — PR #1223 (bookshelf-t3z2w.1)

Markup + CSS review (no rendered screenshot available; live-update behavior is jsdom-tested and the static table shape is unchanged).

Assessment

1. Canonical class reuse — PASS
The Running section table retains class="table workflows-table", consistent with the sibling History table. All other structural classes — .tasks-section, .tasks-section-title, .badge, .empty-state — are pre-existing canonical classes. No bespoke parallel class system introduced. The new data-workflow-running-push-target="*" attributes are Stimulus target annotations, not styling classes.

2. #1217 bug lessons correctly applied — PASS

• templates/pages/settings_shell.html:1758 — Badge uses the hidden attribute, not style="display:none". CSP-safe.
• static/css/main.css:3665 — The +6 CSS lines are exactly .badge[hidden] { display: none; } with an explanatory comment — the documented canonical fix pattern.
• templates/pages/settings_shell.html:1793 — Empty-state <p> is always in DOM with hidden managed by Go conditional on initial render, then toggled by the JS controller on SSE events — correct inverse logic (hidden when WorkflowRunning is non-empty).

3. No inline style= — PASS
Zero style="..." CSS attribute occurrences in the diff. Confirmed via full-file grep.

4. CSS additions minimal, existing tokens only — PASS
Six new CSS lines. No bespoke class names, no hardcoded hex/pixel values, no new design tokens. .badge[hidden] composes the existing .badge selector with the HTML hidden attribute — minimal and correct.

5. Controller _sync() count correctness — PASS
Server-rendered running-workflow rows carry data-instance-id="{{.InstanceID}}" (in the unchanged {{range .WorkflowRunning}} block). The querySelectorAll("tr[data-instance-id]") in _sync() therefore counts both server-rendered and JS-inserted rows correctly; initial badge text matches the DOM row count.

---

REVIEW VERDICT: 0 blocker, 0 major, 0 minor

## UI Review — PR #1223 (bookshelf-t3z2w.1) **Markup + CSS review** (no rendered screenshot available; live-update behavior is jsdom-tested and the static table shape is unchanged). ### Assessment **1. Canonical class reuse — PASS** The Running section table retains `class="table workflows-table"`, consistent with the sibling History table. All other structural classes — `.tasks-section`, `.tasks-section-title`, `.badge`, `.empty-state` — are pre-existing canonical classes. No bespoke parallel class system introduced. The new `data-workflow-running-push-target="*"` attributes are Stimulus target annotations, not styling classes. **2. #1217 bug lessons correctly applied — PASS** - `templates/pages/settings_shell.html:1758` — Badge uses the `hidden` attribute, not `style="display:none"`. CSP-safe. - `static/css/main.css:3665` — The +6 CSS lines are exactly `.badge[hidden] { display: none; }` with an explanatory comment — the documented canonical fix pattern. - `templates/pages/settings_shell.html:1793` — Empty-state `<p>` is always in DOM with `hidden` managed by Go conditional on initial render, then toggled by the JS controller on SSE events — correct inverse logic (hidden when WorkflowRunning is non-empty). **3. No inline `style=` — PASS** Zero `style="..."` CSS attribute occurrences in the diff. Confirmed via full-file grep. **4. CSS additions minimal, existing tokens only — PASS** Six new CSS lines. No bespoke class names, no hardcoded hex/pixel values, no new design tokens. `.badge[hidden]` composes the existing `.badge` selector with the HTML `hidden` attribute — minimal and correct. **5. Controller `_sync()` count correctness — PASS** Server-rendered running-workflow rows carry `data-instance-id="{{.InstanceID}}"` (in the unchanged `{{range .WorkflowRunning}}` block). The `querySelectorAll("tr[data-instance-id]")` in `_sync()` therefore counts both server-rendered and JS-inserted rows correctly; initial badge text matches the DOM row count. --- REVIEW VERDICT: 0 blocker, 0 major, 0 minor

zombor added 1 commit 2026-07-24 18:14:13 +00:00

fix(sse): default-deny event routing + per-user connection cap (bookshelf-t3z2w.1) ... 7c1dea86e1

- matches() is now DEFAULT-DENY: AdminBroadcast, TargetUserID, or explicit
  Broadcast=true must be set; an all-zero/unpopulated Event reaches nobody.
  Eliminates the fail-open where AdminBroadcast=false + TargetUserID=0
  would fan out to ALL subscribers (cross-user leak vector for slice .4).
- Added Event.Broadcast bool for intentional site-wide broadcasts (explicit
  opt-in replaces the implicit zero-value broadcast).
- Hub gains a per-user connection cap (maxConnsPerUser, default 16) in
  addition to the existing global cap (default 512). Subscribe returns
  (nil, nil) when either cap is reached; other users/admins are unaffected.
- NewHub signature extended: NewHub(maxConns, maxConnsPerUser int, logger).
- 100% coverage maintained; 31 specs, all green.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

zombor commented 2026-07-24 18:23:06 +00:00

Author

Owner

Copy link

Security re-review — PR #1223 fix delta (07026cf8..7c1dea86)

Focused on the two prior findings. Both genuinely closed; no new issues.

1. MAJOR (fail-open in matches()) — CLOSED

internal/sse/hub.go matches() is now default-deny. Traced every return path:

• AdminBroadcast true → return sub.isAdmin
• TargetUserID != 0 → return sub.userID == e.TargetUserID
• Broadcast true → return true
• fallthrough → return false

An all-zero Event (AdminBroadcast=false, TargetUserID=0, Broadcast=false) hits the
final return false — reaches nobody. No residual return true default. New
Event.Broadcast bool is the ONLY site-wide path and defaults to its zero value
(false = no delivery). Regression test present:
Describe("default-deny: all-zero Event reaches nobody") asserts
Consistently(ch,"50ms").ShouldNot(Receive()). Existing delivery preserved:
both wfengine/engine_notifier.go Event constructions set AdminBroadcast:true,
so admin workflow-state delivery still routes.

2. MINOR (per-user connection cap) — CLOSED

Subscribe now enforces h.userConns[userID] >= h.maxConnsPerUser AFTER the
global cap, both under h.mu.Lock(). Increment (userConns[userID]++) and
decrement in unsubscribe (userConns[sub.userID]--, map entry deleted at 0) are
under the same lock — no race past the check. Default 16 when <=0. Tests confirm:
a user at cap is rejected (nil channel) while a different user and a different
admin still connect.

3. No new issue

NewHub(512, 16, logger) in app.go; signature change threaded to every caller
(all *_test.go NewHub calls updated). Logs carry only user_id/cap/current/
is_admin — no secrets/PII.

REVIEW VERDICT: 0 blocker, 0 major, 0 minor

## Security re-review — PR #1223 fix delta (07026cf8..7c1dea86) Focused on the two prior findings. Both genuinely closed; no new issues. ### 1. MAJOR (fail-open in matches()) — CLOSED `internal/sse/hub.go` `matches()` is now default-deny. Traced every return path: - `AdminBroadcast` true → `return sub.isAdmin` - `TargetUserID != 0` → `return sub.userID == e.TargetUserID` - `Broadcast` true → `return true` - fallthrough → `return false` An all-zero Event (AdminBroadcast=false, TargetUserID=0, Broadcast=false) hits the final `return false` — reaches nobody. No residual `return true` default. New `Event.Broadcast` bool is the ONLY site-wide path and defaults to its zero value (false = no delivery). Regression test present: `Describe("default-deny: all-zero Event reaches nobody")` asserts `Consistently(ch,"50ms").ShouldNot(Receive())`. Existing delivery preserved: both `wfengine/engine_notifier.go` Event constructions set `AdminBroadcast:true`, so admin workflow-state delivery still routes. ### 2. MINOR (per-user connection cap) — CLOSED `Subscribe` now enforces `h.userConns[userID] >= h.maxConnsPerUser` AFTER the global cap, both under `h.mu.Lock()`. Increment (`userConns[userID]++`) and decrement in unsubscribe (`userConns[sub.userID]--`, map entry deleted at 0) are under the same lock — no race past the check. Default 16 when <=0. Tests confirm: a user at cap is rejected (nil channel) while a different user and a different admin still connect. ### 3. No new issue `NewHub(512, 16, logger)` in `app.go`; signature change threaded to every caller (all `*_test.go` NewHub calls updated). Logs carry only `user_id`/`cap`/`current`/ `is_admin` — no secrets/PII. REVIEW VERDICT: 0 blocker, 0 major, 0 minor

zombor force-pushed bd-bookshelf-t3z2w.1 from 7c1dea86e1 to 40bd9d873f 2026-07-24 18:29:10 +00:00 Compare

zombor merged commit 0f155eb98c into main 2026-07-24 18:36:19 +00:00

zombor referenced this pull request from a commit 2026-07-24 18:36:21 +00:00

Merge pull request 'feat(sse): SSE live-events backbone — hub, handler, lifecycle notifier, Stimulus controller (bookshelf-t3z2w.1)' (#1223) from bd-bookshelf-t3z2w.1 into main

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels

No items

No labels

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

1 participant

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

zombor/pergamum!1223

No description provided.