[29/36] Add OC-140 WebSocket lifecycle UX - #61
Conversation
eb4afe7 to
bc22156
Compare
eaec0cb to
b3c82bb
Compare
|
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:
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. |
|
Re-review (whole-stack pass, per REVIEW_GUIDE.md) The persistent-session claim from the #39 thread ( The Major secret leak is still open and I traced it line by line:
New:
Session lifecycle otherwise checks out: panel close disconnects, deactivation runs 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. |
|
Addressed in
Validation:
The malformed base/head placeholders in the PR description are also corrected. |
7ed4772 to
b3c82bb
Compare
|
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:
|
|
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:
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
|
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:
I validated the exact final 37-PR composition with npm run build, npm run compile, all 539 tests, and all 47 demo validations passing. |
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