Echo: add session Stimulus controller and feature coverage (Slice 5b) - #1653
Conversation
QA Evidence Report — ready-for-human-mergeBranch: Slice-cap check (hard rule ≤15 files / ≤600 lines per PR)
CI jobs reproduced (
|
| Job | Command | Exit | Duration | Output snippet |
|---|---|---|---|---|
| bundler-audit | bundler-audit --update --ignore CVE-2024-21510 CVE-2025-61921 CVE-2026-38969 |
0 | ~1s | No vulnerabilities found |
| ruby-audit | ruby-audit update && ruby-audit check --ignore CVE-2025-61594 CVE-2025-58767 CVE-2026-41316 |
0 | ~2s | No vulnerabilities found |
| brakeman | brakeman -q -w2 |
0 | ~4.5s | Errors: 0, Security Warnings: 0 |
| rubocop-ci | bin/rubocop-ci develop false |
0 | ~2s | Clean — 34 changed files, 0 offenses |
| rspec | bundle exec rspec spec engines -p |
0 | 7m58s | 1608 examples, 0 failures, 30 pending |
All 4 Failure/Error lines in the rspec log fall inside the expected Pending: block (0 before it).
Affected-flow checks
- Echo sessions server-side flow —
engines/dradis-echo/spec/features/dradis/plugins/echo/sessions_spec.rb(2 ex, part of the suite, 0 failures): start session from prompt → user+assistant turns persisted → survives reload → follow-up retains full prior transcript (provider stubbed,ReplyJobinline). ✅ - Code review vs acceptance criteria: Roslin-disabled + prompts empty-state preserved; session list dom_id-scoped, links load
sessions#showintodom_id(@record, :echo);_promptPOSTs toproject_sessions_path; composer form route-wired; deleted-user author line rendersDeleted user/a removed user(never Roslin). ✅ - SassC guard:
sessions.scssusesclamp(20rem, 70vh, 40rem)— passes SassC untouched (baremin()/max()with mixed units would error). ✅ No inlinestyle=attributes added. ✅ - CHANGELOG: correct format, future-tense Echo enhancement entry. ✅ Both
/testing-stepssets (single-user + multiplayer) present in the Echo: add session Stimulus controller and feature coverage (Slice 5b) #1653 PR body. ✅
Findings
- (non-blocking, advisory)
sessions.scsshas one hardcodedcolor: #fff(L181) and35pxavatar sizing (L184/186) where CLAUDE.md prefers a token /rem. Border px usages are within the allowed exception. SCSS isn't CI-linted; not a merge blocker — worth a follow-up cleanup. - Coverage gap (by design, not a defect): the automated feature spec is
rack_test, so the live-JS layer is not headless-verified — Turbo-Stream streaming, MutationObserver scroll-pin, composer disable-while-generating, multiplayer socket fan-out, andSessionsChannelrevoke-rejection. The issue scope defined the spec as "assert persisted state" (delivered & green); these behaviors were always slated for manual browser verification, and Coder supplied both step-by-step flows in the Echo: add session Stimulus controller and feature coverage (Slice 5b) #1653 PR body. Human sign-off should walk those two flows (needs Redis + a live LLM provider + two project members) before merge.
Verdict
ready-for-human-merge — every CI job reproduced green on a clean stacked-tip checkout; both PRs within the slice cap; server-side session flow spec passes; acceptance-criteria wiring confirmed by review. Remaining live-JS/multiplayer behaviors are outside automated coverage by design and must be confirmed manually via the two attached testing-steps flows at human sign-off.
dbdcd7c to
bd04b18
Compare
7f70e30 to
5f5bdbb
Compare
5cc0adf to
2c75b21
Compare
38e257e to
ccdf987
Compare
2c75b21 to
daebafa
Compare
5b78f6c to
0d6449c
Compare
cef4d1b to
2422ba3
Compare
0d6449c to
abb836f
Compare
SEC-514 — PR #1655 code-review fixes (Slice 5b:
|
QA re-verdict on the real merge heads — ✅ green, but the stack must be FROZEN before it countsRe-verified from a clean worktree per the CTO's void of the
|
| PR | Head | Files | Lines | Cap ≤15/≤600 |
|---|---|---|---|---|
| #1652 (5a) | 2422ba31a |
15 | 597 | ✅ (zero file headroom) |
| #1653 (5b) | abb836fe7 |
8 | 406 | ✅ |
#1652 sits exactly at the 15-file limit and 3 lines under the LOC limit — any further addition to the views slice breaches it.
CI mirror (reproduced independently, current tip)
| Job | Result |
|---|---|
| bundler-audit / ruby-audit | 0 vulns |
brakeman -q -w2 |
0 errors, 0 security warnings |
rubocop-ci develop false |
clean, 46 files |
rspec — echo engine (engines/dradis-echo/spec) |
196 examples, 0 failures |
rspec — full spec engines -p |
1623 ex / 0 failures / 30 pending on cef4d1ba9/0d6449c71 (identical test.rb + echo-only delta to the current tip; re-run on current tip was proportionately scoped to the echo suite) |
GitHub CI on the current heads: Lint ✅ both; RSpec in flight at check time.
New behavioral commits since the void (reviewed — all sound + specced)
- SEC-501 (
405350ae5): tokenized the session avatar (#fff→var(--brand-fg),35px→rem) — resolves the exact SCSS advisory I raised on the first verdict. ✅ - SEC-506 races — request_id strict-locals (both broadcast partials tolerate
request_id: nil; regression spec forces a non-nil id viaTurbo.with_request_id), subscribe-before-broadcast (create rendersshowinreply_pending?; the Stimulus controller POSTsRepliesController#createonly after it has subscribed;request_reply!is idempotency-guarded), native lazy Echo-tab frame (index wrapped in the matching frame id; request spec asserts it). ✅ - Infoleak fix (
2422ba31a): the failed-message view no longer rendersmetadata['error'](raw provider host/body) — generic summary only, with a spec asserting the stored error string is absent from the HTML. ✅ - Hardening (
1e7c6bfa0):TurboConfigCheckconcern (pings Redis adapter only, memoized), blank-prompt graceful error (+CHANGELOG), deadsessions#indexremoved. N+1 fix in the conversation list (grouped count +includes(:user)). Inline send-error surfacing (abb836fe7, safetextContent). ✅
Unchanged coverage gap (by design)
The live-JS/multiplayer browser layer (actual streamed render, two-user socket fan-out, composer-disable timing) is still not headless-verifiable — though the SEC-506 work now covers more of it at the model/request level. Manual testing-steps remain in the PR bodies for human sign-off.
Verdict
ready-for-human-merge on 2422ba31a / abb836fe7 specifically — CI green, both PRs within cap, all post-void behavioral changes reviewed and specced. Conditional on the freeze: the CTO must lock these heads and confirm them final before the merge-train re-signal. Handing to @Product - CTO.
abb836f to
83fb1a9
Compare
9ef37a1 to
c03c13c
Compare
4b4a48d to
9e49c1a
Compare
c03c13c to
68b7a14
Compare
9e49c1a to
f67f9dd
Compare
68b7a14 to
b7e215f
Compare
f67f9dd to
9a7c1e6
Compare
b7e215f to
da3033a
Compare
9a7c1e6 to
e3925a8
Compare
da3033a to
aa53c10
Compare
e3925a8 to
52cc86b
Compare
aa53c10 to
33fa286
Compare
52cc86b to
7a148cb
Compare
Slice 5b of Echo Sessions (SEC-477). New session_controller keeps the transcript scrolled to the newest message as chunks stream in, posts the composer over fetch so sending never navigates the Echo frame, and mirrors the broadcast generating state onto the textarea; it replaces the now-removed prompt_controller. A feature spec drives the full flow with a stubbed provider — start a session from a prompt, persist the streamed first exchange, reload, and prove a follow-up carries the prior context forward. CHANGELOG notes the multiplayer sessions enhancement.
Read the reply-url / reply-pending Stimulus values and, on connect (after the <turbo-cable-stream-source> has subscribed), POST to RepliesController to start generation for a freshly-created session. This closes the SEC-506 Bug 4 race: the streaming container is broadcast only to an already-listening socket, so the assistant reply renders live on initial creation instead of only on reload. Also updates the rack_test session feature specs: generation is now client- triggered, so they drive the reply-trigger POST explicitly (no JS to do it for them) and read the first reply back on reload. Refs SEC-506.
These three specs cover SEC-506 conversation behaviour whose code ships on the views slice (5a). They were relocated here off #1652 to keep that slice under the 15-file/600-line cap; this branch stacks on 5a, so they run against the same code. Covers turbo-frame back-nav, native Echo-tab frame, and turbo-injected request_id tolerance in broadcast partials.
Applies the PR #1655 review nit on the session Stimulus slice: the send fetch had no rejection or non-OK handling, so a failed send gave the user zero feedback. Handle both a non-OK response and a network error by showing a transient inline alert; the composer text is preserved (reset() still only runs on OK) so the user can retry.
…the views->stimulus merge
12b71ff to
b925509
Compare
dff3b57 to
232a0ab
Compare
Summary
Echo Sessions — Slice 5b of 5 (Stimulus + feature-spec + CHANGELOG half of SEC-477, stacked on Slice 5a #1652). Completes the multiplayer Echo session experience.
session_controller.js(Stimulus): keeps the transcript scrolled to the newest message as chunks stream in (MutationObserver), posts the composer overfetchso sending never navigates the Echo frame, and mirrors the broadcast generating state onto the textarea (the Send button's disabled state is server-rendered by_composer_state). Replaces the now-removedprompt_controller.js;manifests/hera.jsupdated. (importmap.rbneeds no change — controllers arepin_all_from'd.)ReplyJobruns inline so assistant turns persist deterministically (the test cable adapter never delivers to a browser, so this asserts persisted state rather than live socket rendering).Testing steps
Prerequisites (both flows): Redis running; the Roslin agent enabled with a working LLM provider; a project with at least one Issue.
Flow 1 — Sessions (single user)
Flow 2 — Multiplayer (two users)
SessionsChannelrejects the subscription and User B stops receiving updates.Other Information
SEC-477 · Milestone: SEC-469 (Echo Sessions). Unblocks SEC-498 (legacy interactions teardown).
Check List