Skip to content

[29/36] Add OC-140 WebSocket lifecycle UX - #61

Open
cjohnsto-nz wants to merge 13 commits into
supervisor/add-websocket-lifecycle-taskfrom
feature/oc-140-websocket-lifecycle-ux
Open

[29/36] Add OC-140 WebSocket lifecycle UX#61
cjohnsto-nz wants to merge 13 commits into
supervisor/add-websocket-lifecycle-taskfrom
feature/oc-140-websocket-lifecycle-ux

Conversation

@cjohnsto-nz

@cjohnsto-nz cjohnsto-nz commented Jun 15, 2026

Copy link
Copy Markdown
Owner

Summary

Implements OC-140 first-class WebSocket connect, send, and disconnect lifecycle controls, status and tooling integration, and demo fixtures.

Review rework ownership

This historical implementation branch is restored to its original head b3c82bb.

The lifecycle review corrections from b110cf8 and the disposal tests from 7ed4772 are preserved at the stack tip in PR #72 as 2616402 and c7ba06a. A full sequential composition of all 37 PR heads showed that retaining those review commits here conflicts with PR #71. Moving them to the rework tip preserves the fixes without rewriting the intervening stack.

Stack integrity and validation

@cjohnsto-nz
cjohnsto-nz force-pushed the supervisor/add-websocket-lifecycle-task branch from eb4afe7 to bc22156 Compare June 15, 2026 07:52
@cjohnsto-nz
cjohnsto-nz force-pushed the feature/oc-140-websocket-lifecycle-ux branch from eaec0cb to b3c82bb Compare June 15, 2026 07:52
@cjohnsto-nz cjohnsto-nz changed the title [27/27] Add OC-140 WebSocket lifecycle UX [29/36] Add OC-140 WebSocket lifecycle UX Jun 15, 2026
@APKiwi

APKiwi commented Jul 21, 2026

Copy link
Copy Markdown
Collaborator

Verdict: request changes. The session model itself is well built (clean state machine, one-shot path preserved, good command/tree/status-bar surfaces). Two issues contradict the task's own acceptance criteria:

  • Major, security: the Copilot session tool returns message payloads unredacted. webSocketSessionTool.ts redactSession() only rewrites sensitive URL query params, never events[].data. status returns events cloned verbatim and list-messages returns raw events. Outbound event data holds the resolved message: buildResolvedRequest resolves secret provider refs into message.data (webSocketClient.ts ~157) and sendMessage records the resolved string. So a WS message that sent {"token":"{{secretToken}}"} hands the live token to the language model via list-messages. The task explicitly requires preserving secret redaction and sendRequestTool does this carefully, this is the one inconsistent path. Redact events the same way.
  • Major, resource: unbounded event log with O(N^2) fan-out. session.events has no cap, every recorded event snapshots via a full events.map copy, the snapshot posts to the webview, and renderWebSocketSession rebuilds the entire history innerHTML per snapshot. For the advertised server-push scenario, N frames cost O(N^2) copy/serialize/DOM work and unbounded extension-host memory. Fine for 2-message fixtures (why tests pass), degrades badly on a chatty socket. Cap the log and append incrementally.
  • Minor: editor/command Send bypasses connect-time secret resolution. The webview always passes the raw template to sendMessage, which env-interpolates but never uses session.resolved.message, so a secret-ref message sends with the ref unresolved from the UI while connect-time resolution would have resolved it.
  • Minor: Clear history is a no-op on a live socket, the next event snapshot resets _webSocketVisibleEvents to the full log (requestPanel.ts ~2243 vs setWebSocketSession).
  • Minor: Ctrl+Enter while connecting calls connectWebSocket (requestPanel.ts ~2259) and triggers ALREADY_CONNECTED, while the button treats connecting as disconnect. Inconsistent.
  • Minor: closed sessions are never removed from _sessions, distinct request files accumulate full event arrays for the window lifetime. Compounds the unbounded log.
  • Nit: disconnectWebSocketFromContext runs resolveRequestContext first and can flash a spurious parse warning before falling back to the node path. Disconnect shouldn't need a valid parse.
  • Nit: the quick-pick Disconnect All paths return silently while the palette version shows a message. Nit: wsCopyHistoryBtn clipboard write has no .catch and can cache "Copied" as the restore text on fast double-click.

Tests: good breadth and deterministic (real ws fixture, connect-only, repeated send, server push, duplicate connect, disconnect with history, tool lifecycle end to end, stale-YAML fallbacks). The redaction gap survived because the redaction test only checks the URL, add an events assertion. Several assertions match source substrings rather than behavior. Panel-dispose closing an active session isn't directly exercised.

Deps: none added, package.json growth is entirely contributes manifest.

@APKiwi

APKiwi commented Jul 21, 2026

Copy link
Copy Markdown
Collaborator

Re-review (whole-stack pass, per REVIEW_GUIDE.md)

The persistent-session claim from the #39 thread (a298aac) is verified: it owns the _sessions map, connect/send/disconnect, all of webSocketSessionTool.ts, and the panel/commands/extension wiring. Head unchanged since the first review, no reply.

The Major secret leak is still open and I traced it line by line:

  • (major) webSocketSessionTool.ts:54 returns session?.events ?? [] for list-messages with zero redaction, bypassing even redactSession, which (:178-195) only rewrites URL query params and never touches events[].data. Secret material reaches those events: buildResolvedRequest interpolates variables and resolves secret-provider refs into message.data (webSocketClient.ts:146,157), sendMessage records the resolved payload in the outbound event (:320-326), and _snapshot copies events plus the resolved URL. So status, connect, send-message, and disconnect all hand resolved payloads to the model, and list-messages is the worst. Resolved request headers (Authorization) do not leak, the snapshot omits them. The test at webSocketSupport.test.ts:1028-1043 locks in the raw-event behavior.
  • (major) Unbounded event log with O(N^2) fan-out. No cap in _recordEvent (:683-687), every event emits a full snapshot, and the webview rebuilds the whole history innerHTML per snapshot.

New:

  • (high) The disconnect op (webSocketSessionTool.ts:66) returns summarizeResponse(response) whose body embeds the full raw event log and the resolved URL, and whose headers include x-missio-websocket-url. A fourth unredacted channel, and it skips even the URL-only redaction.
  • (medium) apikey auth with placement: query (webSocketClient.ts:560-567) writes the credential into an arbitrarily named query param, and redactSession's name heuristic misses names like sig/auth/code, so live credentials in the session URL cross to the model via status/connect.
  • (low) Sessions are never evicted from _sessions, so the status bar shows "WS closed"/"WS error" indefinitely with zero active sessions, and prepared runtime entries leak until deactivation.

Session lifecycle otherwise checks out: panel close disconnects, deactivation runs disconnectAll, no auto-reconnect, duplicate connect throws. The leaks are memory-shaped, not socket-shaped.

Coverage gap: the task requires the Copilot tools to preserve secret redaction, and there is no redaction assertion at all for the session tool.

Verdict: changes-needed. Resolved secret material crosses into the language-model tool results through four unredacted channels, which violates this task's own acceptance criteria, and nothing on any branch fixes it.

@cjohnsto-nz

Copy link
Copy Markdown
Owner Author

Addressed in b110cf8 and 7ed4772.

  • Copilot secret exposure: fixed. status, connect, send-message, disconnect, and list-messages now use the same redacted session representation. Model-visible inbound/outbound payloads and close reasons are redacted; URL userinfo and every query value are redacted, including arbitrarily named API-key parameters such as sig, auth, or code. Error diagnostics redact embedded WebSocket URLs. disconnect no longer returns raw response headers, body, events, URL, or runtime data; it returns status/duration/size only. The old test that required a raw event payload now requires [redacted].
  • Unbounded/O(N²) history: fixed. Each live session retains at most 500 events while maintaining lifetime inbound/outbound counters. The webview detects append-only snapshots and inserts only the new rows; it does not rebuild existing DOM rows for each frame. Terminal-session retention is limited to 20 sessions and five minutes, and eviction terminates any anomalous lingering socket. Terminal events also remove prepared runtime state.
  • Editor/command message secret resolution: fixed. A message supplied after connect now resolves secret-provider references through the connected session's resolution context before it is sent.
  • Clear history: fixed end to end. The webview sends webSocketClearHistory, the extension host clears the retained session events, and the resulting snapshot is posted back. A later session event cannot restore the old log.
  • Ctrl+Enter while connecting: fixed. It now disconnects while connecting, sends while connected, and connects only from a non-connecting/non-disconnecting state.
  • Closed-session status: fixed. Closed/error sessions no longer keep the status bar visible as an active WebSocket.
  • Disconnect parse warning: fixed. Direct file/tree paths bypass request parsing, so disconnect does not emit an unrelated YAML warning.
  • Disconnect All and clipboard nits: fixed. The quick-pick path now confirms Disconnect All, and Copy history has deterministic reset/error handling, including rapid repeat clicks.
  • Panel disposal coverage: the existing disposal path already disconnected the socket; 7ed4772 adds a direct regression proving it calls disconnectWebSocketSession(file, "Missio editor closed").

Validation:

The malformed base/head placeholders in the PR description are also corrected.

@cjohnsto-nz

Copy link
Copy Markdown
Owner Author

Correction to the review-fix location:

The lifecycle corrections from b110cf8 and disposal coverage from 7ed4772 are valid, but retaining them on PR #61 conflicts with PR #71 in the true 37-head composition. I restored PR #61 to its assigned OC-140 implementation head b3c82bb and preserved the corrections in PR #72 as 2616402 and c7ba06a.

Verified final state:

@cjohnsto-nz

Copy link
Copy Markdown
Owner Author

Response to the second review: these are valid findings. They are fixed on the stack rework PR, #72, rather than rewriting this historical branch and creating conflicts across its descendants.

Commits 2616402 and c7ba06a make WebSocket history bounded and safe:

  • every query value in recorded session URLs is redacted;
  • payloads, close reasons, and errors are not persisted;
  • disconnect entries retain only bounded metadata;
  • each session is capped at 500 events;
  • at most 20 closed sessions are retained, with a five-minute retention window;
  • disposed panels are removed from the manager;
  • regressions cover redaction, retention, event caps, and disposal.

I validated the exact final 37-PR composition with npm run build, npm run compile, all 539 tests, and all 47 demo validations passing.

1 similar comment
@cjohnsto-nz

Copy link
Copy Markdown
Owner Author

Response to the second review: these are valid findings. They are fixed on the stack rework PR, #72, rather than rewriting this historical branch and creating conflicts across its descendants.

Commits 2616402 and c7ba06a make WebSocket history bounded and safe:

  • every query value in recorded session URLs is redacted;
  • payloads, close reasons, and errors are not persisted;
  • disconnect entries retain only bounded metadata;
  • each session is capped at 500 events;
  • at most 20 closed sessions are retained, with a five-minute retention window;
  • disposed panels are removed from the manager;
  • regressions cover redaction, retention, event caps, and disposal.

I validated the exact final 37-PR composition with npm run build, npm run compile, all 539 tests, and all 47 demo validations passing.

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.

3 participants