Skip to content

Move a session between windows, and publish its window/workspace ownership - #484

Open
olomix wants to merge 16 commits into
umputun:masterfrom
olomix:cross-window-session-move
Open

olomix wants to merge 16 commits into
umputun:masterfrom
olomix:cross-window-session-move

Conversation

@olomix

@olomix olomix commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

A session is pinned to the window it was created in, so splitting work across windows means closing it and starting the process over. This adds moving a session to another open window with its live shell intact.

The move is a transfer of the same Session instance between two AppStores, so the surface and its shell survive untouched — dismantleNSView was already a no-op and viewDidMoveToWindow already re-pushes scale and size, so nothing in the AppKit or libghostty layer needed changing. WindowLibrary owns the operation because it spans two stores, and it evicts the session from the source window's zoom and dashboard registries before detaching, so that window can't keep pointing at a surface now hosted elsewhere. On the wire it is session.move --to-window, taking an optional destination workspace inside that window and --select to make it active there; the destination must already be open, since a closed window has no mounted deck to host the surface. The sidebar row menu and the command palette get matching entries. Cross-window drag stays out of scope, and the strict one-bundle-one-window model is unchanged.

Moving a session invalidates the AGTERM_WINDOW_ID and AGTERM_WORKSPACE_ID its shell was spawned with, and nothing can rewrite a live process's environment — the workspace id already went stale on any ordinary workspace move, with no way to re-read it. The tree now publishes windowId and workspaceId on every session node, and tree --all-windows projects every open window in one response, so a shell can resolve its own current owners in a single call. The skill docs demote both variables to spawn-time hints and point at that query instead.

olomix added 15 commits August 26, 2026 09:04
A cross-window move keeps the Session instance and TerminalView reuses the cached
surface, so no factory re-runs in the destination and every callback kept resolving
the session against the source store: shell exit, overlay teardown and exit status,
unseen/status clears, search and font size all silently no-oped, and the source store
leaked. Extract the per-surface wiring and re-apply it through a new
WindowLibrary.rebindAdoptedSession hook.

Also route adoptSession(select:) through selectSession so an adopted session arrives
with its unseen badge and completed flash cleared, and reject --select without
--to-window in both the CLI and the dispatcher instead of dropping it.
@olomix
olomix requested a review from umputun as a code owner August 26, 2026 16:58
…n-move

# Conflicts:
#	.claude/rules/control-api.md
#	agtermCore/Tests/agtermCoreTests/WindowLibraryTests.swift
@umputun

umputun commented Aug 27, 2026

Copy link
Copy Markdown
Owner

I want the capability, so this is about the form rather than the idea. Three design questions I cannot resolve from the current form, then a list of review findings which are secondary to them.

1. Ownership addressing

this is the first question I cannot resolve. AGTERM_WINDOW_ID is an addressing argument, and the bundled skill ships it that way. plugins/agterm/skills/agterm/examples.md uses --window "$AGTERM_WINDOW_ID" at lines 531, 799, 873, 877, 879 and 880: sidebar collapse, dashboard, pick --no-block, tree --json, pick result, pick cancel.

after a move the window selector still resolves, but to the window the session left. sidebar collapse silently mutates that window. pick opens its picker there, while tree, pick result and pick cancel keep reading or mutating that source-window picker. dashboard opens there when its targets resolve there, and errors when they do not. None of them reaches the session's current window. A stale but syntactically valid UUID is worse than an absent one, because the caller cannot tell.

the updated SKILL.md shows the shape of this. Lines 58-59 add a warning that a stale --window "$AGTERM_WINDOW_ID" "turns a working command into no such session", which is the session-scoped case where you do get an error. The six examples above are window-scoped and left as they are, so the same file covers the detectable failure and demonstrates the silent wrong-window case.

I do not see a safe fix confined to the move itself. Is there one that preserves existing scripts? Nothing can rewrite a live process's environment, so any cross-window move leaves a wrong value in a shell agterm itself spawned. The mitigation here is to demote both variables to spawn-time hints and tell every caller to read tree --all-windows instead, and that falls on every session forever, including the great majority that never move, to support one command. A session that never moves could keep trusting the env, but an agent has no way to know the user did not move its session from the GUI a moment ago, so anything that has to stay correct must stop trusting it globally.

what I would want first is ownership addressing that cannot go stale. AGTERM_SESSION_ID is the one identity in that env that survives everything, so something like a dynamic "the window owning this session" selector, or resolving the current owner from the session id, rather than a documentation demotion carried inside the move. That is its own change with its own migration for existing scripts.

worth knowing, since it narrows the damage: no cookbook recipe is exposed. project-launcher and claude-conversation-picker use AGT_WINDOW_ID, the keymap token resolved when the chord fires, close-tab-when-done walks window list, status-announcer takes the window from the event payload. All live. The exposure is the bundled skill and whatever people wrote themselves.

2. Parity with cross-workspace move

moving a session between workspaces has three entry points today: drag in WorkspaceSidebar+DragDrop.swift, batch, where a selected-row drag carries all of sidebarSelectionIDs in visual order; the sidebar "Move to" submenu, also batch; and a single-session palette row. This ships the last two and not drag.

that is the one I would not leave out, because the drag transport already crosses windows. draggedSessionIDs reads the ids off the system pasteboard, so a drag started in window A is already readable by window B's outline view. The drop is refused only because resolveSessionMove maps each id through the local store's sessionLocation, a foreign id comes back nil, and sources.count stops matching. So the established gesture reaches the boundary and is turned away while a new menu path crosses it.

the batch half is the real work there rather than the plumbing: a multi-row drop has to preserve the block's order at the hovered index, and WindowLibrary.moveSession moves one session at a time with adoptSession appending. adoptSession(at:) already exposes indexed insertion but has no production caller, and a single-session index does not give you the atomic batch transfer a multi-row drop needs.

3. Scope

this is a 48-file, +3465/-887 change covering the move, the ownership read-back, UI, tests and docs. Two separate things make me want to step back from it. Whole-workspace transfer has the same window-addressing problem across every session in the workspace, so it multiplies the exposure even if it reuses the transfer engine. And this change omits the adjacent drag interaction, which is where most of the remaining work sits.

I would rather design one transfer that covers sessions and workspaces, with the addressing sorted first, than take the session-only form now and grow it. I am not asking you to make this smaller by trimming tests or docs, and a safe design may well start out bigger. What I am weighing is the permanent cost.

on that: transfer duties are spread across surface rebinding, controller eviction, notification move records and store-membership observers. The major below is one missed observer. Store membership carries no removal reason, so transfer and close look identical to every observer, and every future thing holding a session-to-window relationship inherits that.

Review findings, secondary to the above

fixing these alone would not answer the design questions above.

major. Moving a session whose overlay is the keymap or ghostty.conf editor consumes its completion token. WindowContentView.openOverlaySessionIDs is built from the source store, so detaching drops the id and the .onChange at line 172 reports it closed. handleClosedEditorOverlays (249-261) then clears the app-global token on AppActions (keymapEditOverlaySession :354, ghosttyEditOverlaySession :378) and reloads while the user is still editing. When the editor really exits in the destination that handler finds nil, so the finished edit is saved on disk but not applied until a manual reload or restart. The ghostty.conf path also consumes ghosttyEditOverlaySnapshot, so its changed-or-not test runs at the wrong moment. moveSession (449-455) guards only a pending pick, so nothing stops the move while an overlay is up.

minor. Three surfaces say the move changes neither window's selection, and the source does reselect: detachSession captures wasActive and calls closeReselectionTarget. plugins/agterm/skills/agterm/reference.md:418-419, site/commands.html:1012-1013, and the doc comment at WindowLibrary.swift:441-442, which contradicts detachSession's own comment. An agent moving the session it is sitting in then finds --target active --window A resolving to something else.

minor. The cross-window batch arm at ControlServer+SessionActions.swift:553-579 loops ids in argument order with a detach and adopt each, while every other arm goes through AppStore.moveSessions and its tree-order remove-first block insert. Destination order follows argument order, affected counts ids resolved rather than moves made, and --to-window active can reorder sessions already in the target. The docs this PR updates state the opposite.

minor. AdoptedSurfaceRebindTests.swift:45-61 is the only test for rebindSurfaces and asserts two dashboardFontOverride nils. It passes one store as both source and destination, so delete all five wirePane/wireOverlay/wireScratch calls and it stays green.

minor. adoptSession's at index: has no production caller. The rollback at WindowLibrary.swift:467 appends, so a session that fails to adopt comes back to the end of its source workspace rather than where it was, and source.location(ofSession:) before the detach would give the index.

minor. session move --to-window W WORKSPACE against an older still-running app degrades silently: the old dispatcher ignores the unknown toWindow, falls through to the workspace positional and relocates in-window answering ok. Without the positional it fails closed.

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.

2 participants