Repository navigation
emrg: switching sessions never refuses on a running turn - #2640
Conversation
argszero
left a comment
There was a problem hiding this comment.
✅ 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.
|
#2639 landed as Before this one can land: merge 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
left a comment
There was a problem hiding this comment.
✅ 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, base7d4db75d): 6 paths —main.js(+4 −5),Shell.tsx(+48 −27),Shell.test.tsx(+31),shell.css(+2 −1), newtest/switch-session.test.js(+81),Agent.md(+3 −3).check-merge-freshness.pyreads FRESH (merge base is master's tip) anddiff(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-626states that switching no longer closes the previous connection (browser-tab semantics: switching back finds the turn still generating), and the switch path only callsconnManager?.get(prevSid)?.clearGroups()— a cache, not the connection. A guard whose premise has been removed refuses without protecting, and theownStreamtest 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(prevSidabsent); and it proves its own extractor slices by comparing two handlers. The three mutation arms in the body redden exactly one axis each, andConnManager.close()'s cancel-before-close is deliberately not re-asserted here — it is behaviour, already held against real frames byconn-manager.test.js.
Two notes that are not defects:
- The
Agent.mdpackaging 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.mdmoves to 742 renderer, because master was3988a2b3when 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
left a comment
There was a problem hiding this comment.
✅ 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
left a comment
There was a problem hiding this comment.
✅ 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.pyon 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'sios_app_prompt.mdandtest_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).ownStreamrefusal back makesswitch-session.test.jsred (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:
emrg/gui/test/switch-session.test.js:45-49asserts!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.Shell.tsxgains acatchon the sidebar session-open path and no test drives it: deleting thatcatchleavesShell.test.tsxat 52 passed. Both new Shell tests start a switch from the task panel (panel-taskstocomposer), never throughonSelectAgentSession. 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.
Closes #2637
What changed
emrg/gui/main.js— theemrg:switchSessionhandler no longer refuses onconnManager.get(currentSessionId).ownStream. That check belonged to a design in which switching closed the previous connection:ConnManager.close()sendscancelto 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: invalidsession_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:selectProjectSessionhad nocatchat all, so the rejection was an unhandled promise and the click looked ignored;openTaskSessionwrote its failure into the transcript of the session it was leaving, which the user is not looking at.Both now set one
switchErrorstate, rendered as arole="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 typecheckclean,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 bytests/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.jsreads the handler's own region — bounded at the nextipcMain.handle(, not at EOF, so a hit outside it cannot be read as a hit inside it — and holds four properties:ownStream;close(prevSidabsent) — the fact the removal depends on, since a close there would sendcancelto the running turn;ConnManager.close()'s cancel-before-close is not re-asserted here: it is behaviour, already held byconn-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:
ownStreamaxis goes red — that one test onlysetSwitchErrorin the task-panel catchUnable to find an element by: [data-testid="switch-error"]connManager?.close(prevSid)added on the switch pathPython side:
uv run python -c "from emrg.client.app import run_client"anduv run python -m emrg --helpclean;tests/test_doc_counts.py(87 passed with the cap guard) green.Agent.mdmoves to 754 renderer / 172 GUI on the current master, andtests/test_doc_counts.pyplustest_agent_md_prompt_cap.pypass 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-resolvesAgent.mdagainst it: master's renderer breakdown (which carries #2639'smarkdown 21,Shell 50,vendorMarkdown 8) plus this PR's two deltas — GUI168 → 172with+ 4 switch-session, andShell 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.mdis 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) ismain.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.