You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@maka.apache.org by "me2seeks (via GitHub)" <gi...@apache.org> on 2026/08/22 13:15:25 UTC
[PR] fix(cli): let /session detach from a running Turn instead of trapping the client [maka]
me2seeks opened a new pull request, #3498:
URL: https://github.com/apache/maka/pull/3498
Closes #3380
### Problem
A second TUI that switched onto a Session with an in-flight Turn (via `/session` or the picker) was trapped: `/session` was intercepted with "Cannot run /session while a turn is running", and every apparent way out was destructive — following the hint (`Esc`/`Ctrl+C`) or quitting called `driver.stop()`, aborting the shared Turn another client was watching. The only clean escape was `kill -9`.
Multi-client attach is a designed Runtime Host capability: the Turn is Host-owned and each TUI is only its viewport. Desktop already treats session switching as pure view navigation; this fix brings the CLI to the same model.
### Fix: let `/session` detach from a running Turn instead of touching it
- **New `'switch'` mid-turn slash disposition** (alongside `'local'` from #3379). Routed through like `'local'`, but its handler must use busy-aware wrappers: idle it runs under `runControl`'s serial lock as before; mid-turn the switch goes through the new detach path instead of silently no-oping on `runControl`'s busy gate.
- **`switchAwayMidTurn` never calls `driver.stop()`.** After `driver.switchSession` confirms, a monotonic `turnEpoch` fence orphans the in-flight drain:
- late events and synthesized stream failures ("ended without a completion event") from the abandoned Session can no longer reach the adopted transcript;
- the orphaned `runAgentTurn` tail skips all old-session continuations (queue flushes would steer the NEW Session), releases `busy`/activity, and starts the freshly attached Turn exactly once — whichever of the detach path or the orphan tail observes an idle runner first.
- **Interrupt guard during handoff**: `requestTurnInterrupt` is swallowed while a detach is in flight — the driver already points at the next Session, so a stop there would abort whatever that Session has attached.
- **Escape owns the picker**: while the mid-turn session picker is open, Escape closes it instead of arming the double-Escape interrupt for the Turn being left running.
- **Foreign import rows hidden mid-turn**: the Claude Code/Codex import flow starts a new Session and cannot detach.
### Scope notes
- Interrupt semantics are unchanged by design (discussed in #3380): any attached client may stop a Turn.
- Switching away from a *self-started* turn keeps working through the same detach path; host-driven Goal continuation on the abandoned Session is unaffected (the Host keeps running it).
- A failed switch leaves everything fully live — the epoch fence is only armed after the driver confirms the switch (covered by test).
### Testing
- 3 new tests in `pi-tui-runner.test.ts` with a parking-turn driver:
- mid-turn `/session <id>`: switches with zero `stop()` calls, replaces the transcript, fences leaked events, starts the attached Turn after the orphan unwinds, lands follow-ups on the adopted Session;
- `/session` opens the picker mid-turn and Escape closes it without arming an interrupt;
- a failed switch reports the error and leaves the running Turn streaming normally.
- Full CLI suite: 360/360 pass; biome + typecheck clean.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] fix(cli): let /session detach from a running Turn instead of trapping the client [maka]
Posted by "Astro-Han (via GitHub)" <gi...@apache.org>.
Astro-Han commented on PR #3498:
URL: https://github.com/apache/maka/pull/3498#issuecomment-5384113089
Second review pass on `24a3a6711870a427bd82f2b8f03b86b4f52b37c4`, from a different model and a different route than the earlier one on this head. One new [P2], left inline at the epoch fence: [#discussion_r3837660462](https://github.com/apache/maka/pull/3498#discussion_r3837660462).
The mechanism itself holds up, and we checked the thing that usually goes wrong with an escape path — what happens to the thing you escaped from:
- **The Host Turn is not stopped.** `switchAwayMidTurn` never calls `driver.stop()` (verified: zero stop calls in the covering tests), which is the correct asymmetry against the interrupt path. In Runtime Host mode the Turn is Host-owned and this TUI was only its viewport, so detaching the view should not end the work.
- **A failed switch does not orphan the live drain.** `turnEpoch += 1` happens only after `await switchSession` resolves, so a throw leaves the in-flight drain fully live — pinned by a test asserting late deltas still land in the same transcript.
- **The adopted Turn cannot start twice.** `startPendingAttachedTurn` no-ops while `busy || turnRunning`, and the old drain's `finishTurnUi` clears `turnRunning` before releasing; whichever of the two observes an idle runner first starts it.
- **Escape on the picker doesn't interrupt.** Early return while `sessionPickerOverlayOpen`, with a test asserting no stop call.
On structure: one epoch counter, not a second authority over Turn lifetime. That's the right shape for this.
Two things noted without grading, since neither is this PR's job to fix:
- Removing `/transcript` and the `TranscriptViewerOverlay` reference is a behaviour deletion unrelated to detach. The viewer module file itself isn't in this diff, so it will be left behind — worth a follow-up so it doesn't linger as dead code.
- `/session <current id>` still detaches from itself; `goToSession` has no short-circuit. Already reported previously, not re-graded here.
The re-entrant `detaching` [P2] already on this head stands on its own; we looked at it independently and agree, and are deliberately not re-grading it. The finding above is a different failure: not entering the path twice, but callbacks that were already in flight when it was entered once.
On CI: the run on this head was sitting at `action_required` and had never executed — I've approved it, so there should be a real signal shortly. Not approving while the P2 above and the existing one are open.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] fix(cli): let /session detach from a running Turn instead of trapping the client [maka]
Posted by "Astro-Han (via GitHub)" <gi...@apache.org>.
Astro-Han commented on code in PR #3498:
URL: https://github.com/apache/maka/pull/3498#discussion_r3837660462
##########
packages/cli/src/pi-tui-runner.ts:
##########
@@ -1127,6 +1148,11 @@ export async function runMakaPiTui(input: MakaPiTuiInput): Promise<void> {
authoritativeAttachedTurn?: MakaAttachedSessionTurn,
): Promise<GoalTurnOutcome> {
busy = true;
+ const epoch = ++turnEpoch;
+ // A mid-turn /session switch-away (#3380) bumps turnEpoch and orphans this
+ // drain: from that point every callback below must stop touching shared
+ // runner state — the adopted Session owns it now.
+ const superseded = () => epoch !== turnEpoch;
Review Comment:
[P2] This comment says *every* callback below stops touching shared runner state once the epoch is bumped, but two of them do not check.
`superseded()` is consulted at `:1226` (`onEvent`), `:1263` and `:1278`. It is not consulted in:
- **`onPrepared`** (`:1197-1210`) — the non-attached branch ends in `if (turn.summary) adoptSessionMetadata(turn.summary)`, which writes title/cwd/model on the runner
- **`onSkillInvocation`** (`:1211-1222`) — splices `state.entries` and calls `showSkillInvocation`
Both are reachable after a switch-away, because `turnRunning` is set at the top of `runAgentTurn` while `pi-tui-turn.ts:87-88` only invokes these two *after* `await preparePrompt` resolves. So the whole `preparePrompt` window is a period where `/session` can already enter `switchAwayMidTurn`, and neither callback has returned yet.
Concrete sequence:
1. a turn stalls in `preparePrompt` (slow or hung)
2. `/session <other>` — `switchSession` succeeds, `turnEpoch` bumps, `applySwitchResult` replaces the transcript with the adopted session
3. `preparePrompt` finally resolves
4. `onPrepared` runs with the **abandoned** turn's summary and calls `adoptSessionMetadata` on it
The screen now describes the old session while the driver is on the new one, and the next prompt goes to the new one. The skill callback has the same shape: an overlay belonging to the abandoned session appearing over the adopted viewport.
`shouldAbort` doesn't help — it only reads `closed`, not the epoch.
Fix looks like the same one-liner already used in the three fenced callbacks: `if (superseded()) return;` at the top of both, including the `onSkillInvocation` on the `SkillInvocationBlockedError` path. A test that switches while `preparePrompt` is still unresolved, then asserts the metadata still belongs to the adopted session, would pin it.
This is separate from the re-entrant `detaching` P2 already reported inline on this head — that one is about entering the path twice, this one is about callbacks that were already in flight when it was entered once.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] fix(cli): let /session detach from a running Turn instead of trapping the client [maka]
Posted by "me2seeks (via GitHub)" <gi...@apache.org>.
me2seeks commented on PR #3498:
URL: https://github.com/apache/maka/pull/3498#issuecomment-5380606786
Working on this PR — happy to iterate on review feedback. Local verification: CLI suite 360/360, biome + typecheck clean.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] fix(cli): let /session detach from a running Turn instead of trapping the client [maka]
Posted by "Astro-Han (via GitHub)" <gi...@apache.org>.
Astro-Han merged PR #3498:
URL: https://github.com/apache/maka/pull/3498
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] fix(cli): let /session detach from a running Turn instead of trapping the client [maka]
Posted by "Astro-Han (via GitHub)" <gi...@apache.org>.
Astro-Han commented on code in PR #3498:
URL: https://github.com/apache/maka/pull/3498#discussion_r3836284034
##########
packages/cli/src/pi-tui-runner.ts:
##########
@@ -1499,6 +1549,76 @@ export async function runMakaPiTui(input: MakaPiTuiInput): Promise<void> {
requestRender();
};
+ // Mid-turn `/session` switch-away (#3380): adopt another Session while a
+ // Turn is still running on the current one. In Runtime Host mode the Turn is
+ // Host-owned — this TUI was only its viewport — so detaching the view must
+ // not stop it (unlike the interrupt path, driver.stop() is never called
+ // here). Bumping turnEpoch orphans the in-flight drain; its runAgentTurn
+ // tail unwinds through the superseded branch and releases busy/activity,
+ // then either that tail or the startPendingAttachedTurn below starts the
+ // freshly attached Turn, whichever observes an idle runner first.
+ const switchAwayMidTurn = async (sessionId: string) => {
Review Comment:
**[P2] `switchAwayMidTurn` is re-entrant, and a second detach clears `detaching` while the first is still in flight.**
Mid-turn, both `/session` entry points deliberately skip `runControl`'s serial lock — that is the point of the change, since the lock is held by the running Turn. Both are also invoked fire-and-forget: `void goToSession(sessionId)` and `void openSessionPicker()`. So nothing serializes `switchAwayMidTurn` against itself, and it has an await boundary in the middle of it (`await input.driver.switchSession(sessionId)`).
Sequence: the user types `/session s2`. While that request is in flight they type `/session s3` — normal impatience, and the window is as long as the round trip. Now two `switchAwayMidTurn` calls are live.
The consequence we care about is the `finally` block. `detaching` is a boolean, so when the first call settles it runs `detaching = false` while the second is still awaiting its switch. That reopens `requestTurnInterrupt`, whose new guard exists precisely for this window — your own comment there says *"a stop here would abort whatever that Session has attached."* A Ctrl+C landing in that reopened window calls `driver.stop()` against the freshly adopted session, which is the exact outcome this PR promises cannot happen. The invariant is stated correctly in the code and then broken by re-entry.
Two smaller effects ride along: `applySwitchResult` runs twice in whatever order the responses return, so the user can end up on `s2` after asking for `s3`; and two `Detached from the running Turn` notices land in the adopted transcript.
The mechanism to fix it already exists — `detaching` is shaped like a re-entrancy flag, it just is not read at the entry points. Rejecting the second detach is enough:
```ts
const goToSession = async (sessionId: string): Promise<void> => {
if (!turnRunning) { await runControl(() => switchSession(sessionId)); return; }
if (detaching) return; // a detach is already handing this view over
await switchAwayMidTurn(sessionId).catch(reportError);
};
```
with the same guard on `openSessionPicker`. Turning `detaching` into a counter would also work but is more machinery than the situation needs — a second detach during a handoff has no meaningful semantics, so dropping it is the honest behaviour.
A regression test would issue a second `/session` before the first `switchSession` promise resolves and assert that `driver.stop()` is still never called and that exactly one `Detached` notice appears. The existing `failNextSwitch` test already shows the harness can control that promise's timing.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] fix(cli): let /session detach from a running Turn instead of trapping the client [maka]
Posted by "me2seeks (via GitHub)" <gi...@apache.org>.
me2seeks commented on code in PR #3498:
URL: https://github.com/apache/maka/pull/3498#discussion_r3837787411
##########
packages/cli/src/pi-tui-runner.ts:
##########
@@ -1499,6 +1549,76 @@ export async function runMakaPiTui(input: MakaPiTuiInput): Promise<void> {
requestRender();
};
+ // Mid-turn `/session` switch-away (#3380): adopt another Session while a
+ // Turn is still running on the current one. In Runtime Host mode the Turn is
+ // Host-owned — this TUI was only its viewport — so detaching the view must
+ // not stop it (unlike the interrupt path, driver.stop() is never called
+ // here). Bumping turnEpoch orphans the in-flight drain; its runAgentTurn
+ // tail unwinds through the superseded branch and releases busy/activity,
+ // then either that tail or the startPendingAttachedTurn below starts the
+ // freshly attached Turn, whichever observes an idle runner first.
+ const switchAwayMidTurn = async (sessionId: string) => {
Review Comment:
Confirmed both mechanisms locally before fixing (reproduced the double `switchSession` and the early `detaching` clear with a parked first switch), fixed at `332404759`:
- `goToSession` now rejects while `detaching` is held. The picker path needed no separate guard — its selection routes through `goToSession` (:2179), so one guard covers both entry points.
- Pinned by a test: second mid-turn `/session` during a parked switch yields exactly one detach notice, no extra `stop()`, single adoption.
One nuance worth flagging: after the blocked re-entry, the queued command text can replay through the idle path once the adopted turn settles, producing a benign second `switchSession` to the same id — that is pre-existing queue behavior outside this window, not a reopen of it.
##########
packages/cli/src/pi-tui-runner.ts:
##########
@@ -1127,6 +1148,11 @@ export async function runMakaPiTui(input: MakaPiTuiInput): Promise<void> {
authoritativeAttachedTurn?: MakaAttachedSessionTurn,
): Promise<GoalTurnOutcome> {
busy = true;
+ const epoch = ++turnEpoch;
+ // A mid-turn /session switch-away (#3380) bumps turnEpoch and orphans this
+ // drain: from that point every callback below must stop touching shared
+ // runner state — the adopted Session owns it now.
+ const superseded = () => epoch !== turnEpoch;
Review Comment:
Confirmed before fixing: reproduced an abandoned Turn adopting its summary over the adopted session when `preparePrompt` resolved after a completed switch-away. Fixed at `332404759` — `onPrepared` and `onSkillInvocation` now open with the same `superseded()` fence as the other callbacks, which also covers the `SkillInvocationBlockedError` invocation since both call sites share the callback. New test parks `preparePrompt`, detaches mid-park, then resolves: the abandoned title/cwd never reach the runner metadata and the skill card cannot land on the adopted viewport. CLI suite 362/362, biome clean.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org