refactor(cli): drop dead fallback retry and followup-takeover paths - #3650
refactor(cli): drop dead fallback retry and followup-takeover paths#3650rbalachandar wants to merge 3 commits into
Conversation
The Runtime Host is now the complete queue authority (per-entry queue ops, protocol epoch 40, apache#3544), which leaves two CLI code paths with no production consumer. Covers sections 1 and 2 of apache#3556; section 3 (runtime-kernel embedded queue API) is intentionally left for a separate PR pending maintainer confirmation. Section 1 — fallback retry machinery: the only production driver (RuntimeHostMakaSessionDriverImpl) returns `fallback` solely when no sessionId exists, while every fallback producer (steer during a running turn, alt+enter queue) requires a live turn and therefore a session. Removes the retry timer loop, deferred-fallback state, turn-boundary flush, pending-bar merge, and their tests. trackEnqueue / settlePendingEnqueues stay: the interrupt path still needs in-flight submit ordering. Section 2 — takePendingFollowup: the production stub is always null (the Host starts queued follow-ups atomically; returning text would make the TUI double-submit), so the runner's re-queue/nextPrompt fold was unreachable outside test doubles. Removes the interface method, stub, consumer block, and the doubles' implementations. Verified as still needed and kept: Host op queue.retract (CLI interrupt / alt+up), sessions:steer/enqueue IPC, session_busy fallback in sessions:send, and the QueueUpdateEvent steering/followup mirrors the pending bar renders.
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Verdict: NO-GO at exact head ad2d5a69cbd79e450688d7c40fa46b22da77c329. Simplify audit: removing the production always-null takePendingFollowup stub and its consumer is demand-free and sound, but the fallback producer is still reachable during first-session admission. A production-shaped delayed-prepare regression probe failed with actual prompts ['start'] versus expected ['start', 'must survive']; the current focused exact-head suites pass 249/249 because the fallback-specific coverage was deleted. Hosted exact-head test is green but does not exercise this window. One P1 inline; no other P0–P3 findings.
| return; | ||
| } | ||
| // Queued: the runtime's `queue_update` event refreshes the mirror. | ||
| .then(() => { |
There was a problem hiding this comment.
[P1] Preserve first-session input until Session admission completes
runAgentTurn() sets turnRunning = true before preparePrompt() awaits #ensureSession(). On a new TUI Session, #createSession() has not yet assigned #sessionId, so Enter here (and Alt+Enter in the matching hunk below) calls the production driver's #enqueue(), gets { kind: 'fallback' }, and this callback now ignores that outcome after the editor was cleared. A production-shaped delayed-prepare probe observed this and retained only ['start'] instead of ['start', 'must survive']. Either expose queue availability only after Session identity exists or retain a durable fallback handoff, and keep a delayed session.create regression.
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Review at ad2d5a69. One [P1], filed inline on pi-tui-runner.ts. Not approving on this head.
The premise of the change is right — the Host is the queue authority now — but the two paths being removed are not equally dead. takePendingFollowup is genuinely dead and can go. The pendingFallback retry path still has a live producer: the driver's #enqueue early-returns { kind: 'fallback' } whenever there is no session id yet, and that window is reachable by an ordinary user during the first Session of a run.
test is terminal green on this exact head, but the suite lost its fallback coverage in the same commit, so green here does not speak to the window in question.
中文
在 ad2d5a69 上审。一条 [P1]，已作为行内评论提在 pi-tui-runner.ts；本 head 不 approve。
PR 的前提是对的——queue 权威确实已经在 Host 侧。但被删的两条路径并不同样是死的：takePendingFollowup 确实死了，可以删；pendingFallback 这条仍有活的生产者——driver 的 #enqueue 在还没有 session id 时会早返回 { kind: 'fallback' }，而这个窗口在一次运行的首个 Session 里普通用户就能撞到。
exact head 上 test 是终态绿，但覆盖 fallback 的测试是和代码在同一个 commit 里一起删掉的，所以这个绿并不能说明这个窗口的问题。
| .then(() => { | ||
| // The runtime's `queue_update` event refreshes the mirror. | ||
| requestRender(); | ||
| }) |
There was a problem hiding this comment.
[P1] The first-session window still produces a fallback outcome, and this handler drops it.
runAgentTurn sets turnRunning = true (pi-tui-runner.ts:1077) before preparePrompt awaits #ensureSession(). In the driver, #sessionId is assigned only after session.create returns (runtime-host-session-driver.ts:781 then :798). A second Enter or Alt+Enter inside that window therefore reaches the production #enqueue, which still returns { kind: 'fallback' } when there is no session id (:848) — that early return is not removed by this PR.
fallback is a fulfilled promise, not a rejection, so the new .then(() => requestRender()) ignores it and only .catch calls refillEditorFromQueues. The editor was already cleared by editor.setText(''), so the user's text is silently lost. Alt+Enter has the same shape at :934-939.
The rest of the removal looks right: takePendingFollowup really is dead — the production stub always returns null because the Host opens follow-ups atomically, and returning text there would double-submit. That part can stay gone. The pendingFallback retry loop is what still has a live producer.
Suggested direction: either stop treating steer/queue as successful before a session identity exists, or keep a minimal durable handoff for that window (the full retry loop is not needed). Worth a regression test with a delayed session.create — the suite is green here because the fallback tests were removed alongside the code, and the hosted test lane does not cover this window.
中文
首个 session 的入场窗口里 fallback 仍然是活的。turnRunning 在 preparePrompt 之前就置位，而 #sessionId 要等 session.create 返回后才赋值；这段窗口内再按 Enter/Alt+Enter，driver 仍会返回 { kind: 'fallback' }（该早返回本 PR 没删）。fallback 是 fulfilled 而非 reject，新的 .then 直接忽略它，只有 .catch 才会回填编辑器——而编辑器已经被清空，用户的文本就这么丢了。
takePendingFollowup 确实是死代码（生产 stub 恒为 null，Host 原子开 follow-up，返回文本会双提交），删掉没问题。有活生产者的是 pendingFallback 这条。建议：要么在 session 身份出现前不把 steer/queue 当成功，要么为这个窗口保留一个最小的持久交接（不必整套 retry loop），并补一个 session.create 延迟返回的回归测试。
Review follow-up on the fallback-path removal. The production driver's
#enqueue still returns { kind: 'fallback' } while session.create has not
yet assigned a session id, and runAgentTurn sets turnRunning before
preparePrompt awaits #ensureSession() — so Enter and Alt+Enter inside
that first-session window produced a fallback that the simplified
handlers ignored after the editor was cleared, silently dropping the
text.
Retain a minimal durable handoff for exactly that window: fallback
outcomes while a turn is running are held in a CLI-owned pendingAdmission
list (rendered in the pending bar), re-enqueued once at the turn
boundary, and returned to the editor as an editable draft if still
undelivered or the turn aborted. No retry loop — the window is bounded by
the first turn. Interrupt exit and alt+up refills merge the held texts,
so no path drops them.
Held steer-kind text is delivered at the boundary, so it opens the
follow-up turn rather than injecting mid-turn into the first turn; the
retry loop that could land it mid-turn is deliberately not restored.
Adds a delayed-session.create regression test covering the window for
both Enter (steer) and Alt+Enter (queue); it fails on the previous head
where the fallback outcome was ignored.
|
Addressed the P1 in 9bc2587 — thanks for the precise window diagnosis. Fix: retained a minimal durable handoff for exactly the first-session admission window, per your suggested direction (no retry loop). A Regression: added a delayed- One semantic nuance to flag: held steer-kind text delivers at the turn boundary, so it opens the follow-up turn rather than injecting mid-turn into the first turn — the retry loop that could land it mid-turn is deliberately not restored. If you'd rather have mid-turn delivery once the session id appears, that would need a small retry/notify hook and I'm happy to add it.
|
2 participants
Summary
Removes two CLI code paths whose last real consumer is gone now that the Runtime Host is the complete queue authority (per-entry queue ops, protocol epoch 40, #3544). Covers sections 1 and 2 of the audit; section 3 (runtime-kernel embedded queue API) is left for a separate PR pending the embedded-composition decision.
RuntimeHostMakaSessionDriverImpl) returnsfallbacksolely when nosessionIdexists, while every fallback producer (steer during a running turn, alt+enter queue) requires a live turn and therefore a session. Removes the retry timer loop, deferred-fallback state, turn-boundary flush, and the pending-bar merge, plus their tests.takePendingFollowup(~30 lines): the production stub is alwaysnull(the Host starts queued follow-ups atomically; returning text would double-submit), so the runner's re-queue/nextPromptfold was unreachable outside test doubles. Removes the interface method, stub, consumer block, and doubles.Kept as still needed (per the audit):
trackEnqueue/settlePendingEnqueues(interrupt-path in-flight submit ordering), Host opqueue.retract,sessions:steer/sessions:enqueueIPC, thesession_busyfallback insessions:send, and theQueueUpdateEventsteering/followup mirrors the pending bar renders.Refs #3556
Verification
npm run build— passnpm run lint(biome, 2652 files) — passnpm run format:check— passnpm run typecheck— passpackages/cliworkspace suite: 412/412 passpendingFallback,takePendingFollowup, or the fallback retry helpersnpm testAI use
Select exactly one:
Tool(s) and scope: Claude Code — reviewed the removal against the issue inventory, verified kept/removed boundaries, ran verification; human contributor of record reviewed and owns the change.
Checklist
Does this PR entail a change in behavior?