Repository navigation
fix(mocactl): keep a prompt a 409 held back when leaving the session - #694
Conversation
ingpaolodettori-dev
left a comment
There was a problem hiding this comment.
Summary
Fixes #479: a prompt held back by a 409 (another device's turn) is now written to the transcript as unsent when the user leaves, shown under a warning on resume, and prefilled (never auto-sent). Persistence, the leave-overlay warning, Esc's unchanged withdraw semantics, and back-compat (load skips unknown kinds) all look right and are well tested. One must-fix: the unsent notice and block stay pinned to the bottom forever, so the prompt the user sends from the prefill (and every later turn) renders above the "was not sent" warning. That happens live, with a reprint, and on every later resume.
Author: cwiklik (MEMBER — maintainer)
Areas reviewed: TypeScript (mocactl core/render/views), tests
Agent/IDE config (.claude/.vscode): none
Commits: 1 commit, all signed-off: yes
CI status: passing (all 17 checks)
| */ | ||
| function atBottom(b: Block): boolean { | ||
| if (b.kind === 'user') return b.queued === true || b.unsent === true; | ||
| return b.kind === 'notice' && b.pinned === true; |
There was a problem hiding this comment.
must-fix — An unsent user block (and its pinned notice) stays atBottom forever, so after the resume every later block lands above it. Take the PR's own happy path: resume, the prompt is prefilled, the user presses Enter, and useSession.submit → addUser(s, text, false) → push puts it at tailStart, i.e. above the notice and the unsent copy. Its reply and turn-end go there too. The screen then reads …theirs / turn-end / MINE / reply / turn-end / ⚠ your last prompt was not sent / MINE, with the warning under the answer to that same prompt.
It's worse live: the notice and the unsent block are settled (isSettled → true), so <Static> has already printed them, and splicing a block in before them makes Static print the tail again (a duplicated or misplaced block). It also persists: fromTranscript replays unsent, prompt, frames… through the same push, so every later resume of the session shows the same order.
I reproduced the ordering with a standalone copy of atBottom/tailStart/push. One fix: keep them bottom-pinned only until the turn above settles (e.g. clear unsent/pinned in finalizeOpen/addUser(..., false) or on turn-end), so they become ordinary history. Please also add a test: fromTranscript([turn, frame, done, unsent 'MINE', prompt 'MINE', frame, done]) must put the new prompt below the notice.
There was a problem hiding this comment.
Fixed in f316c91a (also rebased onto main 0695305c).
You were right on both counts, and the e2e made the <Static> half vivid. Run against the old code, pressing Enter on the prefill printed:
their work
your last prompt was not sent — …
› my own question
› my own question
your last prompt was not sent — …
› my own question
The prompt three times, the notice twice — exactly the reprint you predicted.
Rather than clearing in finalizeOpen or on turn-end, the unpinning hangs off sending, which is the event that actually retires the record: addUser now calls unpinBottom first, so unsent/pinned hold the bottom only while the prompt is still the one thing owed to the user. After that they are ordinary history and everything appends after them. Queued prompts keep their flag — they are still waiting, and it is what marks them on screen. The flag is positional only (no renderer reads unsent), so this changes order, not appearance.
Two tests. Yours, verbatim, as fromTranscript([turn, frame, done, unsent 'MINE', prompt 'MINE', frame, done]):
assistant / turn-end / notice / user(unsent) / user(sent) / assistant / turn-end
plus splitStatic(blocks).live === [], so <Static> prints the run exactly once. And the existing e2e now presses Enter on the prefill and asserts indexOf('was not sent') < lastIndexOf('at last') — that's the one that was failing with the duplication above.
Verification on the rebased head: 225/225 across app, blocks, use-session, session-manager, transcripts, turn-reattach-seams, overlay-misc; tsc --noEmit clean; prettier clean. Full package suite 1674 passed, 5 load-induced timeout flakes (a different five than the previous run), all green in isolation — including app.test.tsx and overlay-misc.test.tsx, which this change touches: 218/218.
| const leaveChoice = { | ||
| // Read at render: the overlay opens through a state change, so this is the queue as it stands | ||
| // when the question is asked (#479). | ||
| pendingPrompt: sessionRef.current?.pendingConflictPrompt, |
There was a problem hiding this comment.
nit — This is read from a ref at render, so if the other device's turn ends while the overlay is open (the conflict attach finishes and the resend goes out), the "it will not be sent" line stays until something else re-renders. The cancel path already handles "turn ended while the question was up". A queue event subscription, or re-reading at keypress, would keep the warning honest. Minor; fine as a follow-up.
There was a problem hiding this comment.
Agreed, and taking it as the follow-up you suggested rather than folding it in here.
I did try it test-first. The blocker is the fake harness: play() awaits step.wait before yielding any frames, so there's no way to get "their work" on screen and then hold the other device's turn open while the overlay is up — the hold has to happen after the frames, which the fake can't express. My attempt failed for the wrong reason (an empty frame, the turn never having started), so I took it back out instead of leaving something unsound in the suite.
Doing it properly means giving HarnessStep a post-frames hold, which is worth its own change — it would also make a few other mid-turn scenarios testable. Filing it as a follow-up with the queue-event subscription you described.
0d2dffd to
f316c91
Compare
|
@ingpaolodettori-dev ready for another look — the must-fix is addressed in Short version: sending a prompt now retires the unsent record. Your repro is a test, and the e2e now presses Enter on the prefill — which is what actually caught the duplication (the prompt printed three times, the notice twice). The overlay-staleness nit is deferred to a follow-up, as you suggested — details in the thread, but briefly: the fake harness can't hold a turn open after yielding frames, so testing it needs a new capability in the fake rather than a test against today's seams. 225/225 across the seven affected suites; |
A detachable turn POST that meets another device's turn gets `409 turn_in_progress`, and the client queues a conflict attach plus a resend of the prompt. Quitting or switching sessions at that moment runs `retire()`, whose `clearQueue()` drops every queued prompt -- and for a 409 the queue is the only place that prompt lives, since `runTurn` skips recording it (`if (!conflicted) record()`). It was lost without a word. Note the issue's "Current state" is stale on one point: it says the prompt "has already been written to the local transcript" and so "looks answered". The rossoctl#471 final review changed that. The prompt leaves no trace at all, so the bug is durability, not disambiguation -- the session's auto-title went with it. Now the leave paths record it as a new `unsent` transcript entry before the queue is cleared, the leave overlay says the prompt will not be sent while the user can still choose to stay, and the resume shows it under a warning notice and offers it back in the input, ready on Enter. Never auto-sent: the other device's answer may have changed the question. Esc keeps today's behavior exactly. A withdrawal is the user taking their own prompt back, so it owes a later resume nothing -- hence a dedicated `unsentRecorded` flag rather than clearing `conflict`, which also decides whether a cancel withdraws or cancels remotely. Block placement needed widening for this: an unsent prompt and its notice sit below the turn above them, like a queued prompt, so the replay of the other device's reply keeps filling its own block instead of opening a second one underneath. `TranscriptStore.load` already skips record kinds it does not know, so older clients read these transcripts unchanged. Item 4 of the issue (a notice line before the other device's turn) ships in rossoctl#692 and is deliberately not repeated here. Closes rossoctl#479 Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: cwiklik <cwiklikj@gmail.com>
Review follow-up on rossoctl#479: the `unsent` block and its `pinned` notice were `atBottom` forever, so once the resume handed the prompt back, everything after it was inserted ABOVE the warning about it. The PR's own happy path showed it: resume, the prompt is prefilled, the user presses Enter, and `addUser` put that prompt at `tailStart` — above the notice and the unsent copy, along with its reply and turn-end. The screen read `theirs / turn-end / MINE / reply / turn-end / ⚠ not sent / MINE`, with the warning below the answer to that same prompt. Live it was worse, as the reviewer said: both blocks are settled, so <Static> had already printed them, and splicing in front of them made it reprint the tail. The e2e test, run against the old code, showed the prompt three times and the notice twice. Sending anything now retires the run: `addUser` unpins it first, so the bottom-pinning lasts only while the prompt is still the one thing owed to the user. After that it is ordinary history and later blocks append after it. Queued prompts keep their flag — they are still waiting, and it is what marks them on screen. The flag is positional only (no renderer reads it), so this changes order, not appearance. Two tests: the `fromTranscript` ordering the reviewer asked for, and the e2e extended to press Enter on the prefill, which is what caught the <Static> reprint. The overlay-staleness nit is left as the follow-up the review suggested: the fake harness yields its `wait` before its frames, so holding the other device's turn open past the overlay needs a new capability in the fake rather than a test against today's seams. Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: cwiklik <cwiklikj@gmail.com>
f316c91 to
c164fcc
Compare
|
Rebased onto Three files conflicted, all of them cases where the two PRs add adjacent code to the same spot, so every resolution keeps both sides:
Verification on the rebased head: 236/236 across the seven affected suites, Assisted-By: Claude Code |
ingpaolodettori-dev
left a comment
There was a problem hiding this comment.
Re-targeted at c164fcc: the branch was rebased again onto main 0729604 (the #692 merge); both commit patches are identical to a1be3d1/f316c91, so the finding below still applies unchanged at blocks.ts:170.
Follow-up to our REQUEST_CHANGES at 0d2dffd. The branch was rebased onto main 0695305. git range-diff shows a1be3d1 = 0d2dffd, so the only new code is f316c91: addUser now calls unpinBottom first, which retires the unsent notice and copy when the user sends. That fixes the reported ordering and the reprint for an idle send, and adds the requested fromTranscript test plus an e2e test. The fix has one regression. unpinBottom also runs on a queued send, which happens when the user presses Enter on the prefill while the other device's turn is still streaming (the main #479 scenario). It unpins the notice and copy below the open assistant block, so the rest of the other device's reply lands below the warning. The overlay-staleness nit is deferred as agreed.
Author: cwiklik (MEMBER — maintainer)
Areas reviewed: TypeScript (mocactl render/blocks), tests; incremental diff 0d2dffd→f316c91 only
Agent/IDE config (.claude/.vscode): none
Commits: 2 commits, all signed-off: yes
CI status: passing so far; deploy-scripts, deploy-scripts-bash32, k8s-kind-e2e and one check still pending at draft time
| // was still the one thing owed to the user. Unpinning first is what keeps this prompt -- the | ||
| // prefill the resume offered, usually this very text -- and everything it leads to BELOW the | ||
| // warning about it, instead of spliced in front of blocks <Static> has already printed (#479). | ||
| const base = unpinBottom(s); |
There was a problem hiding this comment.
must-fix — Thanks, the idle path is fixed. But unpinBottom also runs when queued is true, and that breaks the case #479 is about. Resume while the other device's turn is still running: addUnsent puts the notice and the copy below the open assistant block (their ). The user presses Enter on the prefill, so session.busy is true and addUser(s, 'MINE', true) runs. unpinBottom clears pinned/unsent, which leaves only the queued MINE at the bottom. tailStart now points below the notice and the copy. The next text delta can't reach the open block (appendToAssistant looks at tailStart - 1, which is now the unsent copy), so it opens a new assistant block there.
A standalone run of this file (turn, text 'their ', addUnsent, addUser(…, true), text 'work', done, markSent, text 'at last', done) gives:
assistant "their " (final: false, never finalized)
notice ⚠ your last prompt was not sent
user MINE (unsent copy)
assistant "work"
turn-end
user MINE
assistant "at last"
turn-end
The other device's reply is split around the warning. Its first part is never finalized, so splitStatic(...).live holds all 8 blocks and nothing reaches for the rest of the session. (A later resume replays it in the right order, because the transcript records the prompt only when it is sent.)
A queued send hasn't retired anything yet, since the prompt is still waiting, so you can unpin only on a real send:
const base = queued ? s : unpinBottom(s);With that change the same sequence gives their work / turn-end / notice / MINE(unsent) / MINE / at last / turn-end with live: []. Please add this sequence as a blocks.test.ts case. Optionally, also clear the flags in markSent when the queued prompt goes out.
There was a problem hiding this comment.
Fixed in 50f5ebb, with your exact sequence as a test.
You are right, and the reasoning behind my previous fix was wrong in a way worth naming: I hung the unpinning off addUser because sending is what retires the record, but a queued prompt hasn't been sent — it is itself still waiting, behind the very turn the warning is about. So it retires nothing, and your queued ? s : unpinBottom(s) is the right line.
One thing your sketch doesn't mention that I hit on the way: the order inside the non-queued branch has to be unpinBottom(finalizeOpen(s)), not the other way round. Once the pins are cleared, tailStart points at the unsent copy, so a finalizeOpen running after it no longer finds the open reply above them.
I reproduced your sequence as a RED test first and watched it fail for your reason — their reply arriving as two assistant blocks around the warning:
- Expected + Received
"assistant"
- "turn-end"
"notice"
"user"
+ "assistant"
+ "turn-end"
...
It now gives their work / turn-end / notice / MINE(unsent) / MINE / at last / turn-end with live: [], asserting both that their reply is one block ({ text: 'their work', final: true }) and that splitStatic(...).live is empty.
I did not take the optional markSent suggestion. markSent runs on the queued block itself, which push has already placed below the pinned run, so the flags are retired by the time it matters and clearing them there would be a second place to keep in step with no behavior attached. Happy to add it if you would rather have the invariant stated twice.
Verification: 237/237 across the seven affected suites, tsc --noEmit clean, prettier clean. Full package suite 1687 passed with 4 timeout flakes (OnboardingOverlay, LeaveTurnOverlay, trailing-slash onboarding, SBX0 guarded push) that are 208/208 green re-run in isolation — the same load-induced set as before, and two of them are in files this PR touches, which is why I re-ran them.
Assisted-By: Claude Code
The previous fix unpinned the unsent notice and copy on every send, including a queued one. A queued prompt is the rossoctl#479 case itself: the user presses Enter on the resume's prefill while the other device's turn is still streaming, so the prompt waits behind that very turn. Unpinning there drops tailStart below the warning, so the rest of the other device's reply can no longer reach the block it was filling and opens a second one under the warning -- splitting the reply around it and leaving the first half unfinalized, which holds every block in the live area for the rest of the session. Only a prompt that actually goes out retires the record. markSent is what puts a sent prompt below the warning afterwards, and nothing walks back above it, so the unpinning still runs exactly once per record. finalizeOpen now runs before unpinBottom, since once the pins are cleared tailStart no longer points at the open reply. Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: cwiklik <cwiklikj@gmail.com>
ingpaolodettori-dev
left a comment
There was a problem hiding this comment.
Follow-up to our c164fcc review. 50f5ebb closes the must-fix: a queued send no longer unpins the unsent warning, so the other device's still-streaming reply stays one block, and the new blocks.test.ts case pins that sequence. Running finalizeOpen before unpinBottom on a real send is the right order (good catch). Traced the queued and sent paths. No further findings.
Assisted-By: Claude Code
Upstream since v0.10.0: mocactl fixes for unsent prompts and resumed turns (rossoctl#692, rossoctl#694) and the MOCA -> moca rename as a name (rossoctl#695). One conflict, docs/index.md: upstream's renamed table, with our template onboarding row. The rename touches prose only where a deployment depends on it: no Secret, ConfigMap, label, selector, env var, image or config path, and no provenance record or stream name changes; ocp-sbx and ocp-combined render byte-identically to b6b15a6. mocactl's sandbox-service contract test now covers the template lifecycle methods SandboxApiClient added (retire, tags, edit, owner, quota), each checked against docs/api/openapi.yaml. Typecheck, lint, Go, harness 817, control-plane 1184, relay 228, knative-server 443, mocactl 1718 (with --testTimeout=30000), the sbx and onboarding shell suites pass. Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Rong Chang <rong@us.ibm.com>
Closes #479.
The bug
A detachable
POST /v1/turnthat meets another device's turn gets409 turn_in_progress.ActiveSessionthen queues two jobs: a conflict attach, to show the other device's turn, and a resend of the user's prompt for once that turn ends (spec §6.5).Quitting or switching sessions at that moment runs
retire(), whoseclearQueue()filters out every queued prompt. For a 409 that queue is the only place the prompt lives —runTurn'sif (!conflicted) record()deliberately skips recording it. So the prompt vanished, and said nothing.One correction to the issue
The issue's "Current state" says the prompt "has already been written to the local transcript" and therefore "looks answered" on resume. That is no longer true — the #471 final review changed it (
session-manager.ts:594-597,:710). Verified with a characterization test againstmain:So the bug is durability, not disambiguation: the prompt is erased rather than mis-attributed, and the session's auto-title (which that first prompt would have set) goes with it. The fix is framed accordingly. I've noted this on the issue.
The fix
Five layers, each driven by a test watched failing first:
core/transcripts.ts) — a newunsententry/record kind plusappendUnsent(). Deliberately does not title the session or joinprompts: nothing was sent, so it names no session and is not input history.core/session-manager.ts) —pendingConflictPrompt(the prompt a 409 holds back, while its resend is still queued) andrecordUnsentPrompt().queuedcould not answer this: it counts prompts behind this client's own turn too, and only the 409's is lost unseen.app.tsx) —retire()andleaveChoice.cancelcall it before theclearQueue()/cancelRemote()that drops the queue.render/blocks.ts,app.tsx) — a warning notice above the prompt, and the prompt itself back in the input via the existing nonce-keyedprefillseam. Ready on Enter, never auto-sent: the other device's answer may well have changed what the user wants to ask.views/overlays/LeaveTurn.tsx) — "your prompt is waiting for another turn — it will not be sent", while staying is still an option.Esc is untouched
A withdrawal is the user taking their own prompt back, so it owes a later resume nothing. Double-Esc (
app.tsx:617) andwithdraw()are unchanged, and the twoI3seam tests inturn-reattach-seams.test.tsstill pass as written.That is why
recordUnsentPrompt()uses a dedicatedunsentRecordedflag rather than clearingconflictfor idempotency: that marker also decides whether a cancel withdraws or cancels remotely, so clearing it would have silently changed what Esc does — the one behavior the issue says to preserve.Block placement
This needed
blocks.tswidened. An unsent prompt and its notice sit below the turn above them, like a queued prompt (atBottom, generalizing the old queued-onlytailStart). Without that, the resume's attach replaying the rest of the other device's reply opens a second assistant block underneath the unsent prompt instead of filling the one it was already writing. Covered by a test that fails on the old placement.Back-compat
TranscriptStore.load's switch has nodefault, so it already skips record kinds it does not know — older clients read these transcripts unchanged. Now covered explicitly rather than implicitly.Scope
Item 4 of the issue — a notice line before the other device's turn — is deliberately not here. It ships in #692 (
OTHER_DEVICE_TURN_NOTICE) for #483; repeating it would duplicate the constant and conflict in the same file. This branch is cut frommain, not from the #483 branch, so the two PRs stay independent.Verification
transcripts.test.ts21/21,session-manager.test.ts61/61,blocks.test.ts38/38,turn-reattach-seams.test.ts28/28,app.test.tsx60/60,overlay-misc.test.tsx6/6tsc --noEmitclean; prettier cleanTest timed out/waitForflakes that also reproduce on a clean tree (LeaveTurnOverlay,CredentialsOverlay, trailing-slash onboarding,sbx-push); all 5 pass when their files are run in isolation, which I checked specifically because this change editsLeaveTurn.tsx.New end-to-end test covering the whole acceptance path: 409 → leave overlay warns → quit with keep → resume → notice shown, input pre-filled, and
harness.turnsstill length 1 (nothing sent behind the user).Assisted-By: Claude Code