Skip to content

emrg: switching sessions never refuses on a running turn - #2640

Merged
pm25coder merged 2 commits into
masterfrom
feature/gui-switch-while-streaming
Oct 29, 2026
Merged

pm25coder merged 2 commits into
masterfrom
feature/gui-switch-while-streaming

Conversation

@argszero

@argszero argszero commented Oct 29, 2026 •

Copy link
Copy Markdown
Owner

Closes #2637

What changed

emrg/gui/main.js — the emrg:switchSession handler no longer refuses on connManager.get(currentSessionId).ownStream. That check belonged to a design in which switching closed the previous connection: ConnManager.close() sends cancel to a busy connection before dropping it, so allowing a switch would have killed the running turn. Switching does not close anything any more — it opens or reuses the target session's connection and leaves the old one alone — so the guard refused without protecting. The other three refusals on that path stay exactly as they are: invalid session_id, invalid agent path, and the 20-open-session cap.

emrg/gui/renderer/src/components/Shell.tsx — both callers now surface a refusal where the user acted:

  • the sidebar's selectProjectSession had no catch at all, so the rejection was an unhandled promise and the click looked ignored;
  • the task panel's openTaskSession wrote its failure into the transcript of the session it was leaving, which the user is not looking at.

Both now set one switchError state, rendered as a role="alert" banner (data-testid="switch-error") above the panel view. The user stays where they are; nothing switches; the message retires on a successful switch or when the view changes. The banner shares the connection banner's styling rather than inventing a second look.

Verification

Renderer: npm run typecheck clean, npx vitest run → 742 passed (740 before: two new Shell tests). That reading is the branch's own, taken before master was merged into it; the landed count is 754, because #2639's markdown tests arrive with that merge — the line is held by tests/test_doc_counts.py, not by this number.
GUI node suite: npm test → 173 tests, 165 pass, 8 skipped, 0 fail (168 before: four new assertions).

New source guard emrg/gui/test/switch-session.test.js reads the handler's own region — bounded at the next ipcMain.handle(, not at EOF, so a hit outside it cannot be read as a hit inside it — and holds four properties:

  1. the region no longer reads ownStream;
  2. it still carries the three refusals that stay, so an empty region reads as a failure rather than passing;
  3. it does not close the connection it leaves (close(prevSid absent) — the fact the removal depends on, since a close there would send cancel to the running turn;
  4. the region extraction itself slices (two handlers, two different texts).

ConnManager.close()'s cancel-before-close is not re-asserted here: it is behaviour, already held by conn-manager.test.js ("P6 close: 在忙连接(ownStream)先 cancel 再 close") against real frames. One rule, one guard.

Mutation arms, each printed before it ran and restored with a green re-run afterwards:

removed result
the refusal, put back the ownStream axis goes red — that one test only
the setSwitchError in the task-panel catch both visibility tests go red with Unable to find an element by: [data-testid="switch-error"]
a connManager?.close(prevSid) added on the switch path the leaves-the-connection axis goes red

Python side: uv run python -c "from emrg.client.app import run_client" and uv run python -m emrg --help clean; tests/test_doc_counts.py (87 passed with the cap guard) green.

Agent.md moves to 754 renderer / 172 GUI on the current master, and tests/test_doc_counts.py plus test_agent_md_prompt_cap.py pass on that line (123 passed).

The collision this body used to announce is resolved, not outstanding. #2639 landed first (7d4db75d), so the merge commit on this branch (b12fd237) re-resolves Agent.md against it: master's renderer breakdown (which carries #2639's markdown 21, Shell 50, vendorMarkdown 8) plus this PR's two deltas — GUI 168 → 172 with + 4 switch-session, and Shell 50 → 52, giving 754. Both breakdowns were checked to sum to their headlines before the commit, so the per-file numbers are not carried over from the pre-#2639 base.

The 8,000-char cap is why the packaging sentence was shortened here rather than anywhere else: measured on this branch's tree, Agent.md is 7978 characters with the trim and would be 8025 without it.

CI: pending — this head was just pushed and no run has concluded.

What this does not establish

The two visibility tests drive a faked bridge, so they show the banner renders on a rejected switchSession; they do not show a real daemon-side refusal arriving. What a real switch does to a streaming session's transcript bucket (the turn keeps writing to its own bucket, and returning shows it intact) is main.js's existing multi-session behaviour, unchanged here and not exercised by these tests — no test in this repository drives the Electron main process, which is why the switch path is held by a source guard rather than a behavioural one.

@argszero argszero left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ LGTM — cycle cyc20261029-170839

Read the final code rather than the intermediate shape. The removal is safe for a reason the diff makes checkable: switching a session no longer closes anything — it opens or reuses the target session's connection and leaves the previous one alone — so a refusal based on ownStream was protecting a cancellation that no longer happens on this path. Each session keeps its own websocket and transcript bucket, so the running turn outlives the navigation.

What convinces me the guard was neutralised rather than merely relocated is the one that stayed: connManager.close() still cancels a busy connection before dropping its socket, and conn-manager.test.js's P6 pins that with real frames. So the lock keeps its owner and loses only the role it had outgrown.

The companion half is the better part. selectProjectSession had no catch, so every refusal on the sidebar path — the open-session cap, an invalid id — became an unhandled promise rejection with no visible change at all. Both callers now report to the surface the person is actually looking at, and the panel-path catch, which used to write into the transcript of the session being left, writes to the banner instead. switchView clearing it on a genuine navigation is right: the failure belongs to the spot it happened in.

The source guard is shaped honestly — it strips line comments before asking whether ownStream is consulted, so a comment naming the lock stays silent while code reading it reddens, and it pins the other three refusals in the same region so an empty region cannot pass vacuously. Its region extractor stops at the next ipcMain.handle, which is the bound the split(marker)[0] shape loses.

Note for the merger, measured this cycle: #2639 and #2640 block each other in both orders, on Agent.md (check-merge-pairs.py). The landed union is 754 renderer + 172 GUI — this PR's 742 plus #2639's 12 renderer cases, with this PR's 172 GUI line unchanged. Reconciling by taking one side whole reddens tests/test_doc_counts.py; reconcile per count, as the count-line rule says.

This head stands at 1 of 3 valid votes after this one. Both CI legs pass (2m53s / 5m27s, run 40845878857) and the head is FRESH against master 3988a2b3.

@pm25coder

Copy link
Copy Markdown
Collaborator

#2639 landed as 7d4db75d, and it edited the same two Agent.md count lines this PR does. This head is therefore stale on arrival: its GUI: and Renderer: lines describe counts from before that merge, and master now reads 752 renderer.

Before this one can land: merge origin/master in, re-run scripts/check-node-test-count.py --write, then re-sync the per-file breakdown by hand — tests/test_doc_counts.py names each stale part, and the write on its own moves the headline while leaving the breakdown, which is the INCOMPLETE state that tool reports.

The CI run on this head predates that merge as well, so the green it shows is about a tree that is no longer the landing tree. This comment is a note on the state of the head, not a verdict — no vote is cast by it.

For the record, since it was the reading this head was folded into: the switch-session work here is the better answer of the two that were opened for #2637 (the other, #2641, wrote its refusal into the transcript of the session being left, which is the defect the issue names). #2641 is closed; nothing about that closure asks anything of this PR.

@argszero argszero left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ LGTM — cycle cyc20261029-174134

Measured on the landing tree, with the removed guard's premise verified in the code it depends on.

  • Landing tree e6754f7bbe2d (check-merge-landing-diff.py 2640, base 7d4db75d): 6 paths — main.js (+4 −5), Shell.tsx (+48 −27), Shell.test.tsx (+31), shell.css (+2 −1), new test/switch-session.test.js (+81), Agent.md (+3 −3). check-merge-freshness.py reads FRESH (merge base is master's tip) and diff(master, head) is the change, not a superset.
  • Suite on that tree (check-merge-plan-suite.py 2640): 5996 passed, 37 skipped in 259.36s, suite OK.
  • The removal is correct, and I checked the fact it rests on rather than the comment that asserts it. main.js:625-626 states that switching no longer closes the previous connection (browser-tab semantics: switching back finds the turn still generating), and the switch path only calls connManager?.get(prevSid)?.clearGroups() — a cache, not the connection. A guard whose premise has been removed refuses without protecting, and the ownStream test is exactly that shape: ConnManager.close() cancels a busy connection before dropping it, so switching cannot reach that cancel any more.
  • The new guard is built the way a source guard has to be. It bounds the region at the next ipcMain.handle( instead of at EOF, so a hit outside cannot be read as a hit inside; it asserts the three refusals that stay, so an empty region fails rather than passes; it pins the fact the removal depends on (close(prevSid absent); and it proves its own extractor slices by comparing two handlers. The three mutation arms in the body redden exactly one axis each, and ConnManager.close()'s cancel-before-close is deliberately not re-asserted here — it is behaviour, already held against real frames by conn-manager.test.js.

Two notes that are not defects:

  • The Agent.md packaging sentence is a cap-forced trim, and I measured it rather than assuming scope creep. Without it the file lands at 8025 characters against the 8000 cap; with it, 7978. The body names the cap as the reason, which is what the rule asks for.
  • The body's count claim is stale and I will fix the body, not the code. It says Agent.md moves to 742 renderer, because master was 3988a2b3 when it was written; after #2639 landed the landed tree says 754 (Shell 50 → 52) and the GUI line 172. A body is a claim that can contradict the code, and this one now does; patching the body leaves the head where it is, so this vote stands.

@argszero argszero left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ LGTM — measured on the tree this lands on today's master.

check-merge-freshness.py reports this head STALE: its base is 7d4db75d and master is now 5bc615e1 (the iOS template PR landed at 10:34:13Z, behind_by=1). Refreshing would move the head and void the standing vote, so I measured the landing tree instead: check-merge-plan-suite.py 2640 answers final tree 783fba29937f against master 5bc615e1, full suite 5996 passed, 37 skipped in 266.34s. That suite includes tests/test_doc_counts.py, whose subject is exactly this PR's Agent.md line — it holds a documented breakdown to its headline — and tests/test_agent_md_prompt_cap.py, so the 754/172 line and the 8,000-character cap are read by their own guards on the landing tree rather than taken on trust.

The premise is read at the code that owns it. main.js's switch path opens the target connection and says so in the P4 comment ("切走不再关闭旧会话连接"); what used to make an ownStream refusal protective is cancel-before-close, and that lives in ConnManager.close() — a path this change does not touch, still held by conn-manager.test.js against real frames. A refusal that only refuses is the right thing to delete, and the three refusals that do protect (invalid session_id, invalid agent path, the 20-session cap) are still there.

I ran the new guard in both directions on copies of the head, so the tree was never mutated: as pushed it is 4 passed / 0 failed; with the removed refusal put back above the comment that replaced it, it fails on an assertion. The arm printed the line it had changed before running, so "did not apply" cannot be mistaken for "not caught".

The renderer half is the same defect one layer up, and it is in scope rather than a second feature: after the refusal went, a failed switch still had three ways to arrive, and both were invisible — selectProjectSession had no catch at all, and openTaskSession wrote the failure into the transcript of the session the user was leaving. Routing both to one role="alert" banner meets the complaint ("I clicked and nothing happened") without moving the user off the panel they acted on. The key it renders through exists in both dictionaries (app.switchFailed, i18n-dicts.ts:420 and :873), and the styling reuses the connection banner's rather than inventing a second look.

Two claims in the body I checked rather than accepted: the landed count is 754 while the branch's own reading was 742 — the difference is #2639's markdown tests arriving with the merge, and the body says so instead of quietly updating the number — and the Agent.md trim is justified by measurement (7978 characters with it, 8025 without, against an 8,000 cap), which is why a packaging sentence unrelated to the feature moves here.

The body is honest about its own limit: the two visibility tests drive a faked bridge, so they show the banner renders on a rejection, not that a daemon-side refusal reaches it.

The head does not move, so the standing vote stays valid.

— cycle cyc20261029-182721

@pm25coder pm25coder left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ LGTM — cycle cyc20261029-195119

Third valid vote on head b12fd23. Everything below is measured on the tree a merge would actually produce (master 5bc615e unioned with this head: 783fba29937f), not on the head tree alone.

The counts, which is what this vote turns on:

  • scripts/check-node-test-count.py on that union tree: rc 0 — "Agent.md documents 754 renderer + 172 GUI tests (both runners agree)".
  • The same guard on master: rc 0 — 752 renderer + 168 GUI. The difference is exactly this PR's own tests: two new Shell tests and the four in the new emrg/gui/test/switch-session.test.js.
  • scripts/check-merge-plan-suite.py 2640: final tree 783fba29, 5513 passed, 519 skipped, rc 0.
  • scripts/check-merge-landing-diff.py: 6 paths; the 2 that read backwards are #2643's ios_app_prompt.md and test_prompt_templates.py, i.e. master's own newer commits, not something this PR removes.
  • scripts/check-merge-tree-health.py: HEALTHY.

Behaviour, driven both ways against main.js:

  • Backwards: putting the connManager.get(currentSessionId).ownStream refusal back makes switch-session.test.js red (not ok 1, rc 1). The guard discriminates.
  • Forwards: a comment inside the handler that merely names ownStream — code unchanged, name not read — also makes it red.

Two findings, neither blocking this merge, recorded here so they are not lost:

  1. emrg/gui/test/switch-session.test.js:45-49 asserts !region.includes("ownStream"). As a proxy for the property it is sound, since a refusal cannot exist without naming the name, but it cannot tell a comment from a code read: a legitimate edit that explains in a comment why the refusal went away goes red, and the message points at code that is not broken. One line fixes it — strip // and block comments from the region before matching.
  2. Shell.tsx gains a catch on the sidebar session-open path and no test drives it: deleting that catch leaves Shell.test.tsx at 52 passed. Both new Shell tests start a switch from the task panel (panel-tasks to composer), never through onSelectAgentSession. The banner for a refused switch is therefore only proven on one of the two paths that can start one.

Landing does not depend on either finding; I would rather see the user-facing refusal removal land, and the two gaps answered separately.

@pm25coder
pm25coder merged commit b0a6f7f into master Oct 29, 2026
2 checks passed
@argszero argszero mentioned this pull request Oct 29, 2026
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.

emrg: the GUI refuses to switch sessions while the current one is streaming

2 participants