Skip to content

fix(cli): stage a rewound turn's quotes into the replacement submit - #5265

Open
ggbdpq wants to merge 11 commits into
apache:mainfrom
ggbdpq:fix/tui-rewind-quote-staging
Open

ggbdpq wants to merge 11 commits into
apache:mainfrom
ggbdpq:fix/tui-rewind-quote-staging

Conversation

@ggbdpq

@ggbdpq ggbdpq commented Sep 13, 2026

Copy link
Copy Markdown
Contributor

Summary

/rewind on a turn that carried quotes used to fail closed with rewind_unsupported_quotes (#5109, behavior C): the TUI could only refill the human-facing text, so the replacement submit would silently drop the turn's structured context.

The runtime-host driver now returns the rewound turn's QuoteRefs verbatim on rewindToTurn, and submitMessage forwards quotes through turn.message.submit — the protocol admission already accepts client-authored quotes (only session-context attachments are Host-owned, and #4804 made a quote alone meaningful content). Attachments and directory references still fail closed, since the TUI cannot re-attach files.

The TUI stages the restored quotes keyed to the branched session:

  • the status line carries a quotes:<n> segment while staging is live (accent salience, dropRank with the other chrome),
  • bare /quotes lists the staged excerpts; /quotes clear is the explicit removal,
  • the first admitted submit consumes the staging; a refusal or failure restages it for the retry, and switching sessions invalidates it without every switch path having to clear it.

The now-dead rewind_unsupported_quotes code and its localized copy are removed together with the branch that emitted them.

Part of #5109 — the remaining scope (editing a selected message that itself carries attachments) still needs the Host to expose the rewritten target-owned refs.

Verification

Check Result
New driver tests (red before the fix: rewind rejected quoted turns; submit content lacked quotes) 2/2 green after
New runner tests (staging visible via notice + status segment; submit carries the QuoteRefs verbatim and consumes them; /quotes clear drops them) 2/2 green after
pi-tui-runner full suite 207/209 — the 2 failures (restores the terminal before exiting on SIGTERM, forces signal exit when outer cleanup never settles) fail identically on pristine upstream/main built in a clean worktree on this machine: pre-existing Windows signal-handling failures
runtime-host-session-driver full suite 25/75 — the 50 failures are all the documented pre-existing Windows class (Session cwd no longer exists: /tmp POSIX fixtures); none of them is a test added or modified here
tui-copy-catalog, tui-primary-guidance, core slash-command-catalog 8/8, 8/8, 1/1
check:asf-headers passed locally (audited 3773 covered in checkout mode)
format:check the repo-wide run segfaults on this machine (biome crash during the full-tree walk, environmental); all 8 changed files pass biome check --formatter-enabled=true --linter-enabled=false individually, and upstream CI runs the repo-wide gate

AI use

Select exactly one:

  • No generative tool made a substantive contribution
  • Generative tooling made a substantive contribution

Tool(s) and scope: GLM-5.3-Flash (ZCode) implemented the driver/TUI changes and tests under human direction and review.

Checklist

  • Tests cover the change and fail without it
  • Lint, format, typecheck and the affected suites pass locally

Does this PR entail a change in behavior?

  • Yes — rewinding a quoted turn now restores its quotes into the replacement submit instead of refusing; /quotes is a new TUI command.
  • No

Rewinding to a turn that carried quotes used to fail closed with
rewind_unsupported_quotes, because the TUI could only refill the
human-facing text and the replacement submit would silently drop the
turn's structured context (apache#5109).

The runtime-host driver now returns the rewound turn's QuoteRefs
verbatim and forwards quotes given to submitMessage through
turn.message.submit, whose admission already accepts them (only
session-context attachments are Host-owned). Attachments and directory
references still fail closed, since the TUI cannot re-attach files.

The TUI stages the restored quotes keyed to the branched session: the
status line carries a quotes:<n> segment while staging is live, bare
/quotes lists the staged excerpts, /quotes clear discards them, and the
first admitted submit consumes the staging while a refusal or failure
restages it for the retry.

Part of apache#5109

Generated-by: GLM-5.3-Flash (ZCode)
@github-actions github-actions Bot added the effort/M Under 500 readable lines label Sep 13, 2026
assert.ok the recorded submit before reading its content, per the
noUnsafeOptionalChaining lint rule.

Part of apache#5109

Generated-by: GLM-5.3-Flash (ZCode)

@Astro-Han Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for addressing the quoted-turn gap in #5109. I reviewed eb5c88fc3fcc56849fddb954349ff9a7074caae6. Returning the existing QuoteRefs and forwarding them through the existing Host admission is the right direction; attachments can continue to fail closed until their target-owned refs can be restored.

I found two reachable issues, detailed inline: P1, a delayed failure can attach one Session's staged quotes to another Session; P2, a quote-only replacement still cannot pass the text-only submit guards. These are draft ownership/content-admission issues within the existing runner, not a reason to introduce a new revision or import framework. Please bind retry restoration to the original draft/session identity and treat staged quotes as meaningful content at the submit entry points.

Validation: the two new runner tests pass in an exact-source bundle with local dependencies. Two additional probes through the real runner/editor submission path reproduce both findings. I also checked the production driver's switch/admission code: session switching does not wait on the submit admission queue, so the delayed-failure interleaving is reachable. I did not run an installed TUI with a live Host or the full repository suite.

Codex-assisted review with local reproduction.

Comment thread packages/cli/src/pi-tui-runner.ts Outdated
if (staged.length > 0) {
stagedRewindQuotes = staged;
stagedQuotesSessionId = input.driver.getSessionId();
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P1] Restore failed-submit quotes only to their original draft

Reachability ②: an admission request can be pending while /session other-session succeeds. Both failure branches tag the old staged payload with driver.getSessionId() at callback time, which is now the other Session. In a real-runner probe, I rewound a quoted turn, held its submit promise, switched Sessions, then rejected the original request; the next unrelated message in the new Session carried the old quote. This silently sends context to the wrong conversation (and potentially a different model connection).

Could you capture the originating Session/draft generation before dispatch and restore only if that same draft still owns the result? Apply that check to both blocked and rejected responses, and prevent stale callbacks from overwriting a newer staged draft or explicit clear. A regression through submit → switch → delayed refusal → submit should carry no old quotes into the new Session.

appendUserPrompt(state, text, messageId, true);
requestRender();
// Quotes staged by a rewind (#5109) ride this message and only this one:
// the staging clears as the message dispatches, and a refusal or failure

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] Let a quote-only replacement reach this submission path

Reachability ①: a Desktop message can contain only QuoteRefs, so rewind now returns an empty prompt with nonempty quotes. The unchanged submitPrompt, steerRunningTurn and Alt+Enter guards still reject empty text before reaching this new quote-forwarding path. A runner probe restoring prompt: '' with one quote shows quotes:1, but Enter produces zero driver submissions; the user must invent extra text to resend the retained content.

Please make the relevant editor/submit guards consider the current draft's quotes as meaningful content, while still rejecting truly empty drafts. Cover a quote-only rewind sent unchanged, and the corresponding empty draft after /quotes clear.

…nly rewinds

Two review findings on apache#5265:

A failed or blocked admission restaged the rewound quotes tagged with the
Session read at callback time, so a Session switch while the admission was
in flight attached the old quotes to the next message of the wrong
conversation. The originating Session and staging generation are now
captured at dispatch, and the restore happens only when neither moved.

A quote-only rewind refills an empty prompt, and the empty-text guards in
submitPrompt, steerRunningTurn and Alt+Enter rejected it before the quote
forwarding path could run. They now treat staged quotes as meaningful
content; a cleared plate stays truly empty and keeps refusing.

Part of apache#5109

Generated-by: GLM-5.3-Flash (ZCode)
@ggbdpq

ggbdpq commented Sep 14, 2026

Copy link
Copy Markdown
Contributor Author

Both findings fixed at 6ca7ff9:

P1 (wrong-Session restore): the originating Session and a staging generation counter are captured at dispatch. The restore now runs only when the Session is unchanged and no newer staging or explicit clear bumped the generation — a switched Session, a newer rewind, or /quotes clear while the admission was in flight all leave the stale callback empty-handed.

P2 (quote-only replacement): submitPrompt, steerRunningTurn and Alt+Enter now treat staged quotes as meaningful content, so an unchanged quote-only rewind reaches the driver; after /quotes clear the empty draft is truly empty and keeps refusing.

Regressions: restores a failed quote submit only to its originating session (rewind → held admission → switch → reject → next submit in the new Session carries no quotes) and lets a quote-only rewind reach the submit path unchanged (empty prompt + staged quotes submits verbatim; after clear, nothing). Full pi-tui-runner suite 209/211 — the 2 failures are the pre-existing Windows SIGTERM pair already characterized on this PR's base.

@github-actions github-actions Bot added effort/L Under 1000 readable lines and removed effort/M Under 500 readable lines labels Sep 15, 2026

@Astro-Han Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the quick turnaround — the quote-only submit path reads correctly. Two findings on the restage guard, reviewed at 6ca7ff99b.

P1 — restageForRetry can never restore. originGeneration is captured before clearStagedQuotes() runs (pi-tui-runner.ts: the capture sits above the stagedGeneration += 1 inside clearStagedQuotes). By the time a blocked result or a failure lands, stagedGeneration !== originGeneration is always true, so the restore never executes. Reachable on the ordinary failure path: /rewind → submit → admission refused or fails → the staged quotes are silently dropped instead of returning for the retry the feature promises. The new tests can't see it: "restores a failed quote submit only to its originating session" asserts the other session stays clean, which also holds when restage never runs; nothing covers the same-session restore. Smallest fix: read stagedGeneration after clearStagedQuotes(), and add the positive case — same-session failed submit restages the quotes.

P2 — a newer rewind does not invalidate an in-flight restage. The rewind path assigns stagedRewindQuotes/stagedQuotesSessionId directly (~pi-tui-runner.ts:2193) without bumping stagedGeneration, so after the capture order is fixed, a re-rewind landing while an admission is in flight would still be overwritten by the older failure's restage — the exact case the comment lists. Bump the generation on that assignment too (or route both writes through one setter).

Everything else in the delta looks right — session-id capture and the empty-text-with-quotes gates in submitPrompt/steerRunningTurn/handleSubmit cover the paths we discussed.

@me2seeks you know the TUI submit path best — could you sanity-check the restage semantics above?

中文版

在 6ca7ff99b 复核。P1:originGeneration 在 clearStagedQuotes()(内部 stagedGeneration += 1)之前捕获,失败或 blocked 落地时守卫恒真,restageForRetry 永不执行——带 staged quotes 的提交失败后 quotes 静默丢失,不回到草稿供重试。新测试只断言另一会话不继承,restage 不执行时也成立,所以没兜住;最小修法是把 generation 读取挪到 dispatch-clear 之后,并补同会话失败恢复的正例。P2:rewind 重新 stage 的赋值(~pi-tui-runner.ts:2193)不 bump generation,修好顺序后「in-flight 期间新 rewind」仍会被旧失败回调覆写,需在赋值处一并 bump。其余 delta(session 捕获、空文本+quotes 放行)没问题。

AI assistance: I used Devin to re-check the delta against the earlier findings; the assessment is mine.

The restage guard captured stagedGeneration before the dispatch's own
clearStagedQuotes(), which bumps the generation, so the guard compared
against a pre-clear value and a blocked or failed admission never
restored the staged quotes to the draft. Read the generation after the
clear instead, and route every staged-quote write through one setter
that bumps the generation, so a re-rewind landing while an admission is
in flight is never overwritten by the older failure's restage (apache#5109
review).

Generated-by: GLM-5.3-Flash (ZCode)
@ggbdpq

ggbdpq commented Sep 16, 2026

Copy link
Copy Markdown
Contributor Author

Both findings fixed at 399f15e:

P1 (restage unreachable): originGeneration is now read after the dispatch's own clearStagedQuotes(), so the guard compares against the staging state the submit actually left behind — an ordinary failure passes and restages, while a Session switch, a newer rewind, or an explicit clear landing in flight still vetoes. Added the positive case: a same-session failed submit restages the quotes onto the next submit (fails on the old order — restage never ran).

P2 (stale restage overwrites newer staging): all staged-quote writes now route through one setStagedQuotes setter that bumps stagedGeneration (the rewind assignment included), so the write-site invariant no longer depends on the clear happening to sit next to it. Added the displacement case: a re-rewind landing while the first admission is in flight wins — the stale failure's restage is vetoed and the next submit carries the newer rewind's quotes (asserted with per-rewind distinct quote texts).

Verification: full pi-tui-runner suite 211/213 — the only 2 failures are the pre-existing SIGTERM pair documented in the local baseline (fail identically on the unmodified base); biome clean on both touched files.

@Astro-Han Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for pushing this through three review rounds — I re-reviewed the current head (399f15e0) with fresh eyes, focusing on the staging semantics rather than the earlier findings.

What I verified

  • Both earlier findings are genuinely fixed. originGeneration is now read after the dispatch's own clearStagedQuotes() (pi-tui-runner.ts:1310-1324), and every write routes through setStagedQuotes, which bumps the generation (:623-628, used by the rewind path at :2199-2205). So an ordinary failure restages, while a Session switch, a newer rewind, or /quotes clear landing in flight vetoes the stale callback. All four new runner tests (__tests__/pi-tui-runner.test.ts:6780-7082) exercise exactly those vetoes plus the positive restage case.
  • Quote-only submits are admissible end to end. hasMeaningfulMessageContent (packages/core/src/events.ts:186) is the single predicate shared by protocol admission (packages/runtime-host/src/protocol/turn.ts:472-489), storage and compaction, so empty text + quotes is legal at every boundary. I also confirmed pi-tui's editor does call onSubmit('') for Enter on an empty buffer (editor.js:submitValue), so the new gates at :1227, :1367, :1385 are actually reachable — and that addToHistory drops empty input, so no empty history entry is created.
  • No duplication when the admission outcome is lost. RuntimeHostMakaSessionDriverImpl#submitMessage swallows outcome_unknown / interrupted-after-dispatch and resolves undefined, i.e. success, so the staging stays consumed rather than being re-sent on a retry. That's the right call for this feature.
  • Quotes are the right refs, verbatim, in order. rewindToTurn returns promptMessage.quotes from the rewound turn (runtime-host-session-driver.ts:901), the revision copy retains the copied turn ids, and the driver forwards a copy unchanged (:546). A newer rewind replaces (not appends to) the staging, so no cross-turn accumulation. No dedup is needed — the refs come from the Host, which already applied TURN_MESSAGE_QUOTE_MAX_COUNT/size limits when they were admitted.
  • Attachments and directory refs still fail closed before any branch exists — the driver test asserts no session.revision.create for those carriers (__tests__/runtime-host-session-driver.test.ts:2125-2213), so a quoted and attached turn can't half-rewind.
  • Repo-wide grep finds no leftover references to the removed rewind_unsupported_quotes code or copy outside the two changed files; the new /quotes entry is wired through both catalogs with all three locales filled in.

Findings

1. A quoted replacement submit renders as nothing in the TUI transcript. This is the one thing I'd like a decision on. The TUI never renders QuoteRefs: the durable user projection emits message.displayText ?? message.text only (pi-transcript.ts:1086-1096), renderUserBlock returns [] for blank text (:2229-2230), and the pending bar prints Steering: with an empty preview for a queued one (:1972). So for the quote-only flow this PR explicitly enables (:6987), the user sends a message and sees no row for it — just an assistant reply to an invisible prompt, with the quotes:<n> segment gone and /quotes now reporting "No restored quotes are staged", i.e. no remaining evidence of what went out. The desktop already solves both halves: quote chips plus a structured-only branch that avoids an empty bubble (packages/ui/src/chat-turn.tsx:250-275). Rendering the quotes (or at least a · N restored quote(s) hint) in the durable user entry would close the loop; happy for it to be a follow-up, but it's user-visible as-is.

2. nit — the staging is hidden on a Session switch, not invalidated. effectiveStagedQuotes() compares stagedQuotesSessionId to getSessionId() (:615-619), so leaving the branched session drops quotes:<n>, but coming back later (/session, the side-conversation toggle, /resume) silently re-arms the old quotes and the next submit carries them with no notice — potentially many turns later. The comment at :609-611 says switch paths "invalidate" the staging, which isn't quite what the code does. If resurrection isn't intended, bump stagedGeneration (or clear) when the view leaves that session; if it is, the comment and the copy could say so.

3. nit — /quotes clear always reports success. The handler clears and pushes quotesCleared unconditionally (:4296-4302), so it claims "Restored quotes discarded" when nothing was staged, and also when the quotes are currently riding an in-flight submit (which will still carry them). Bare /quotes already distinguishes the empty case with quotesNone; clear could reuse that check.

4. nit — a few cheap coverage gaps.

  • restageForRetry() is called on the disposition === 'blocked' branch (:1337) but no test covers it — only the rejected-promise path does. The existing HostSkillDriver (__tests__/pi-tui-runner.test.ts:11545) can refuse, so a quoted rewind plus a refused skill would cover it directly.
  • Every staging test uses a single quote, so ordering, the quotes:<n> count, and the /quotes listing order for >1 excerpt are unverified. A two-quote rewind result would pin all three.
  • /quotes is declared midTurn: 'local' but no test runs it mid-turn, which is the disposition most likely to regress silently.

5. nit — a second rewind can dangle the quotes' provenance. Nothing validates sourceTurnId against the branched session, and a revision copy slices before the target turn (packages/runtime-host/src/server/session-revision-coordinator.ts:371-380). Rewinding twice in a row can therefore stage refs whose source turn no longer exists in the new branch. The inline text is intact so nothing is dropped from the model input; only a chip click-through in the desktop may fail to resolve. Not worth changing here — just noting the staging deliberately carries refs it doesn't re-validate.

Net: I found no correctness bug in the staging/restage logic itself — the generation + session-id guards hold up under the interleavings I traced. Item 1 is the one I'd want an explicit answer on; the rest are nits.

@Astro-Han Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for pushing this through three review rounds — I re-reviewed the current head (399f15e0) with fresh eyes, focusing on the staging semantics rather than the earlier findings.

What I verified

  • Both earlier findings are genuinely fixed. originGeneration is now read after the dispatch's own clearStagedQuotes() (pi-tui-runner.ts:1310-1324), and every write routes through setStagedQuotes, which bumps the generation (:623-628, used by the rewind path at :2199-2205). So an ordinary failure restages, while a Session switch, a newer rewind, or /quotes clear landing in flight vetoes the stale callback. All four new runner tests (__tests__/pi-tui-runner.test.ts:6780-7082) exercise exactly those vetoes plus the positive restage case.
  • Quote-only submits are admissible end to end. hasMeaningfulMessageContent (packages/core/src/events.ts:186) is the single predicate shared by protocol admission (packages/runtime-host/src/protocol/turn.ts:472-489), storage and compaction, so empty text + quotes is legal at every boundary. I also confirmed pi-tui's editor does call onSubmit('') for Enter on an empty buffer (editor.js:submitValue), so the new gates at :1227, :1367, :1385 are actually reachable — and that addToHistory drops empty input, so no empty history entry is created.
  • No duplication when the admission outcome is lost. RuntimeHostMakaSessionDriverImpl#submitMessage swallows outcome_unknown / interrupted-after-dispatch and resolves undefined, i.e. success, so the staging stays consumed rather than being re-sent on a retry. That's the right call for this feature.
  • Quotes are the right refs, verbatim, in order. rewindToTurn returns promptMessage.quotes from the rewound turn (runtime-host-session-driver.ts:901), the revision copy retains the copied turn ids, and the driver forwards a copy unchanged (:546). A newer rewind replaces (not appends to) the staging, so no cross-turn accumulation. No dedup is needed — the refs come from the Host, which already applied TURN_MESSAGE_QUOTE_MAX_COUNT/size limits when they were admitted.
  • Attachments and directory refs still fail closed before any branch exists — the driver test asserts no session.revision.create for those carriers (__tests__/runtime-host-session-driver.test.ts:2125-2213), so a quoted and attached turn can't half-rewind.
  • Repo-wide grep finds no leftover references to the removed rewind_unsupported_quotes code or copy outside the two changed files; the new /quotes entry is wired through both catalogs with all three locales filled in.

Findings

1. A quoted replacement submit renders as nothing in the TUI transcript. This is the one thing I'd like a decision on. The TUI never renders QuoteRefs: the durable user projection emits message.displayText ?? message.text only (pi-transcript.ts:1086-1096), renderUserBlock returns [] for blank text (:2229-2230), and the pending bar prints Steering: with an empty preview for a queued one (:1972). So for the quote-only flow this PR explicitly enables (:6987), the user sends a message and sees no row for it — just an assistant reply to an invisible prompt, with the quotes:<n> segment gone and /quotes now reporting "No restored quotes are staged", i.e. no remaining evidence of what went out. The desktop already solves both halves: quote chips plus a structured-only branch that avoids an empty bubble (packages/ui/src/chat-turn.tsx:250-275). Rendering the quotes (or at least a · N restored quote(s) hint) in the durable user entry would close the loop; happy for it to be a follow-up, but it's user-visible as-is.

2. nit — the staging is hidden on a Session switch, not invalidated. effectiveStagedQuotes() compares stagedQuotesSessionId to getSessionId() (:615-619), so leaving the branched session drops quotes:<n>, but coming back later (/session, the side-conversation toggle, /resume) silently re-arms the old quotes and the next submit carries them with no notice — potentially many turns later. The comment at :609-611 says switch paths "invalidate" the staging, which isn't quite what the code does. If resurrection isn't intended, bump stagedGeneration (or clear) when the view leaves that session; if it is, the comment and the copy could say so.

3. nit — /quotes clear always reports success. The handler clears and pushes quotesCleared unconditionally (:4296-4302), so it claims "Restored quotes discarded" when nothing was staged, and also when the quotes are currently riding an in-flight submit (which will still carry them). Bare /quotes already distinguishes the empty case with quotesNone; clear could reuse that check.

4. nit — a few cheap coverage gaps.

  • restageForRetry() is called on the disposition === 'blocked' branch (:1337) but no test covers it — only the rejected-promise path does. The existing HostSkillDriver (__tests__/pi-tui-runner.test.ts:11545) can refuse, so a quoted rewind plus a refused skill would cover it directly.
  • Every staging test uses a single quote, so ordering, the quotes:<n> count, and the /quotes listing order for >1 excerpt are unverified. A two-quote rewind result would pin all three.
  • /quotes is declared midTurn: 'local' but no test runs it mid-turn, which is the disposition most likely to regress silently.

5. nit — a second rewind can dangle the quotes' provenance. Nothing validates sourceTurnId against the branched session, and a revision copy slices before the target turn (packages/runtime-host/src/server/session-revision-coordinator.ts:371-380). Rewinding twice in a row can therefore stage refs whose source turn no longer exists in the new branch. The inline text is intact so nothing is dropped from the model input; only a chip click-through in the desktop may fail to resolve. Not worth changing here — just noting the staging deliberately carries refs it doesn't re-validate.

Net: I found no correctness bug in the staging/restage logic itself — the generation + session-id guards hold up under the interleavings I traced. Item 1 is the one I'd want an explicit answer on; the rest are nits.

… transcript

Third-round review items on the rewind quote staging:

- A session change now clears the staged quotes outright instead of only
  hiding them while the user is elsewhere: keying alone let a silent
  resurrection re-arm the quotes on return, potentially many turns
  later. The rewind re-stages its own quotes after the switch settles.
- /quotes clear distinguishes the nothing-staged case (including quotes
  that already left on an in-flight submit) instead of always claiming
  a discard.
- A quote-only submit stored no text, so the replacement message left
  no trace in the transcript — an answer to an invisible prompt. The
  durable user entry now carries the restored-quote count and renders a
  trace line for it.
- Coverage: the blocked-disposition restage, two-quote ordering across
  the status line, /quotes listing, and the submit, and /quotes routing
  mid-turn.

Generated-by: GLM-5.3-Flash (ZCode)
@ggbdpq

ggbdpq commented Sep 17, 2026

Copy link
Copy Markdown
Contributor Author

Addressed everything actionable at 8822a0911 — thank you for tracing the editor's empty-buffer Enter and the driver's outcome-unknown swallow; both were load-bearing for the original findings.

Item 1 (quote-only submit renders as nothing) — implemented now, not deferred. The durable user entry carries the restored-quote count and renders a trace line: blank text renders · N restored quote(s) as its row, and a quoted message with text appends the hint under it. Locale-free structural text, matching the quotes:<n> status segment's convention; localized chips stay desktop-side as you noted. Pinned in pi-transcript.test.ts through replaceTranscriptWithStoredMessages + renderMakaPiTranscript (blank+quotes, text+quotes, and no-quotes cases).

Item 2 (hidden vs invalidated) — the staging is now invalidated. applySwitchResult — the single choke point every session change routes through — clears the staging outright; the rewind re-stages its own quotes after the switch settles. The stale comment now describes what the code does. Resurrection is no longer possible from any path.

Item 3 (clear always reports success) — fixed. /quotes clear with nothing staged reports quotesNone instead of claiming a discard; this also covers the in-flight case, where the honest answer is that there is nothing left to clear. The quote-only test that had pinned the old lying copy now asserts the truthful one.

Item 4 (coverage) — all three gaps closed:

  • blocked-disposition restage: a quoted rewind whose replacement the Host refuses (blocked + failed skill invocation) restages; the retry carries the refs again.
  • two-quote rewind: quotes:2, the /quotes listing order, and the replacement submit's ref order are all pinned (two distinct excerpts).
  • /quotes mid-turn: with a running Turn claiming busy, the listing still routes through the local disposition.

Item 5 (dangling provenance after a second rewind) — agreed, no change here: the refs are carried deliberately un-revalidated, and the desktop chip click-through is the place that resolves them.

Verification: full pi-tui-runner + pi-transcript suites 328/332 — the only failures are the two pre-existing SIGTERM cases on the local baseline (fail identically without this branch's changes); biome clean on all four touched files.

# Conflicts:
#	packages/cli/src/session-driver.ts
@github-actions github-actions Bot added effort/XL Under 2500 readable lines and removed effort/L Under 1000 readable lines labels Sep 18, 2026
The runtime-host PTY close-wait timeout fired on a merge head whose
runtime-host tree is identical to green upstream; the PR's delta is
 confined to packages/cli. Local pi-tui + transcript suites pass on the
merge head.
@ggbdpq

ggbdpq commented Sep 20, 2026

Copy link
Copy Markdown
Contributor Author

Gentle ping — everything actionable from your 09-17 review landed at 8822a0911 (the transcript trace line for quote-only submits, switch-path invalidation via applySwitchResult, the truthful /quotes clear copy, and the three coverage gaps). The head is now a127b13e: same tree plus a merge of main and a CI retrigger. Checks are green and the branch merges cleanly — re-review whenever you have a moment.

@me2seeks me2seeks left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review(对抗式复核,head a127b13ed)

方向认可:把 rewind 掉的 turn 的 QuoteRef 原样返回并转发进替换 submit,而不是 fail-closed(rewind_unsupported_quotes),同时删掉死代码和对应文案,这是干净的做法。前几轮 review 的 P1(generation 读取顺序)和 switch 失效化确实已修好,我复核确认。

不过还有 1 个 P1 和 1 个 P2 需要处理。


P1 — outcome_unknown 路径下 staged quotes 会被静默吞掉,且永不 restage

restageForRetry() 只在两个分支被调用(packages/cli/src/pi-tui-runner.ts):

  • .then 里 result?.disposition === 'blocked'
  • .catch 里

但真实 driver 在「投递结果未知」时不抛异常,而是 resolve undefined(packages/cli/src/runtime-host-session-driver.ts:551-557):

} catch (error) {
  if (
    (error instanceof RuntimeHostOperationError && error.code === 'outcome_unknown') ||
    (error instanceof RuntimeHostRequestInterruptedError && error.dispatch === 'dispatched')
  ) {
    return undefined;   // 注意:resolve,不是 reject
  }
  throw error;
}

#admit(:1271)把这个 undefined 原样透传。于是对于 outcome_unknown / interrupted-after-dispatch 的 submit:

  • .then 里 result?.disposition 是 undefined、if (result) 为 false → 不 restage;
  • .catch 不触发 → 不 restage。

而 staging 在 dispatch 时已经被 clearStagedQuotes() 清掉,所以 quotes 被静默消费、永久丢失。

这里需要纠正上一轮 review 的一个判断。上一轮把这条路径读成了「resolve undefined,i.e. success,所以 staging 保持消费是正确的」。但按 RuntimeHostRequestInterruptedError 自己的文案(packages/runtime-host/src/client/connection.ts:328-330),dispatch === 'dispatched' 的含义是:

'the operation outcome is unknown; do not retry it automatically'

message-coordinator.ts:1236 的 outcome_unknown 同样是「Message disposition cannot be proven in this Host Epoch」——结果无法证明,而不是「已确认接纳」。所以这不是一个「有意保持消费」的设计,而是一个未被识别的缺口:恰恰在「消息可能没被接纳」时,把上下文静默丢掉了。

需要说明的是,这里存在一个真实的两难:如果消息其实已经被接纳,restage 会导致重试时重复发送 quotes。但当前代码既没有注释说明这是有意为之,也没有测试覆盖这条路径。建议明确决策并写清理由(例如「outcome_unknown 视为已接纳,故不 restage,避免重复」),或者按语义选择 restage 并说明重复风险。

测试盲区:所有新测试用的 HeldSubmitQuotedDriver.submitMessage 都是 reject(packages/cli/src/__tests__/pi-tui-runner.test.ts 的 new Promise((_, reject) => ...)),没有任何用例走 resolve-undefined 这条真实路径。


P2 — 任意带 quotes 的用户消息都会被标成「restored quote」,不只是 rewind 恢复的

packages/cli/src/pi-transcript.ts(本 PR 新增):

const restoredQuotes = message.quotes?.length;
entries.push({
  kind: 'user',
  messageId: message.id,
  text: message.displayText ?? message.text,
  ...(restoredQuotes ? { quotes: restoredQuotes } : {}),
});

渲染时无条件加标签:

const hint = `· ${entry.quotes} restored quote${entry.quotes === 1 ? '' : 's'}`;

问题是 StoredMessage.quotes 并不是「rewind 恢复」专属字段——桌面端普通引用消息也写入同一个字段,并且会进入 TUI 渲染的同一份 durable transcript:

  • apps/desktop/src/renderer/features/workbar/tools/side-chat/use-quote-companion.ts:1238(普通引用提交)
  • apps/desktop/src/main/session-local-service.ts:198(把 content.quotes 投影回本地消息)
  • 桌面端 edit & resend 的 restage 路径(#5274)同样会写入该字段

而 TUI 在打开/切换会话时会走 applySwitchResult → replaceTranscript(messages) → storedMessagesToTranscriptEntries(pi-tui-runner.ts:1878、pi-transcript.ts:422),所以任何带引用的用户消息都会显示「· N restored quotes」,事实错误。

本 PR 自己的测试也把这个错误固化了:pi-transcript.test.ts 里 message-2 是 text: 'with words' + 一个 quote,断言输出 · 1 restored quote。而 packages/ui 对同一字段的中性投影是「Inline quoted excerpts」(packages/ui/src/materialize.ts:62),两边语义已经分叉。

建议:把标签改成中性(例如「· N quote(s)」),或者给 entry 增加一个真正的「restored」标记(由 rewind 路径显式设置),而不是用「有没有 quotes」来推断。


已确认没问题的点

  • 删除的代码没有残留引用:全仓 grep 无 rewind_unsupported_quotes / unsupportedQuotes 残留。
  • quote-only submit 链路通:hasMeaningfulMessageContent 是 protocol admission / 存储 / compaction 共用的单一谓词,空 text + quotes 在各边界都合法;/quotes 三个 locale(en / zh-CN / zh-TW)文案齐全。
  • /quotes 解析:parts = trimmed.split(/\s+/),/quotes、/quotes clear、其它形式走 usage 提示,与文件内其它命令(如 /host)一致;midTurn: 'local' 是合法取值。
  • /new 路径:newSession 不调用 clearStagedQuotes(),但 startNewSession 会把 #sessionId 置 null,effectiveStagedQuotes() 因 session 不匹配返回 [],且之后任何切回都会经 applySwitchResult 清空——不会 resurrect,属于注释措辞不够精确,不是 active bug。

小结:P1 建议在合并前明确决策并补测试;P2 建议改中性标签。其余为已澄清项。

…the quote trace

Two findings from the adversarial re-review at a127b13:

- An outcome_unknown submit (or an interruption after the dispatch went
  out) resolves without a receipt, and the dispatch had already consumed
  the quote staging, so the quotes vanished silently with no restage and
  no test coverage. Restage them when the receipt is absent and surface
  a notice naming the uncertainty: admission cannot be proven either
  way, and losing the user's explicit context to an unproven outcome is
  worse than a visible duplicate ride (status line shows the restore;
  /quotes clear discards it).
- The durable user-entry trace said "restored quote(s)" for every
  message carrying quotes, but StoredMessage.quotes also carries plain
  desktop quotes (including edit-restaged ones), so any quoted message
  resurfaced in the TUI mislabeled itself as rewind-restored. Word the
  trace neutrally ("· N quote(s)").

New UnknownOutcomeSubmitDriver pins the resolve-undefined path the
previous tests only exercised through rejection.

Generated-by: GLM-5.3-Flash (ZCode)
@ggbdpq

ggbdpq commented Sep 20, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for the adversarial re-read — both findings confirmed and fixed on cb6d10882 (merged latest main first, no conflicts).

P1 — outcome_unknown restage. Confirmed: the real driver resolves undefined for outcome_unknown and for an interruption after dispatch, and the .then arm only handled disposition === 'blocked', so the staging was silently consumed exactly when admission was least proven. Decision: restage on the unknown path and surface a notice ("Submit outcome unknown; staged quotes restored for retry."). An unproven outcome is not an admission; losing the user's explicitly staged context is the irreversible branch, while the duplicate-ride risk if the message did land stays visible (status line quotes:N) and discardable (/quotes clear). The restage still routes through the same session/generation guards, so a Session switch, newer rewind, or explicit clear landing in flight keeps winning.

P2 — neutral trace. Also confirmed: StoredMessage.quotes carries every quoted user message (desktop plain quotes and edit-restaged ones included), not just rewind restages, so the durable trace mislabeled them. The trace line is now the neutral · N quote(s), matching @maka/ui's neutral projection of the same field.

Tests: new UnknownOutcomeSubmitDriver pins the resolve-undefined path (all prior held-submit tests rejected — the reviewer-noted blind spot), and the transcript test now asserts the neutral wording instead of enshrining the old label. Both fail against the pre-fix source and pass on cb6d10882; the rest of the two suites is unchanged (only the two known SIGTERM flakes, pre-existing and unrelated).

…talog

The notice introduced for the unknown submit outcome was a visible
literal in pi-tui-runner, which check:tui-copy correctly rejects — TUI
copy must go through the localized catalog. Add quotesRestoredUnknown to
the rewind catalog (en / zh-CN / zh-TW) and reference it.

Generated-by: GLM-5.3-Flash (ZCode)
@ggbdpq

ggbdpq commented Sep 20, 2026

Copy link
Copy Markdown
Contributor Author

Follow-up on c07c577a7: the first push's CI failure was my mistake, not the fix itself — the unknown-outcome notice was a visible literal in pi-tui-runner, which check:tui-copy correctly rejects. The notice now goes through the rewind copy catalog (quotesRestoredUnknown, en / zh-CN / zh-TW), guard green, test re-run green.

@ggbdpq

ggbdpq commented Sep 24, 2026

Copy link
Copy Markdown
Contributor Author

Gentle ping — the head is dcb9c22a4 (CI green, merges cleanly into the current main): the fixes for your 09-20 review plus one style cleanup commit. Everything actionable from that review is in — the outcome_unknown restage with the catalog notice and the neutral N quote(s) label — so a re-review whenever you have a moment would be much appreciated.

@hqhq1025 hqhq1025 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change now carries rewound QuoteRefs through the CLI driver and into the replacement submit, accepts quote-only submits, and uses neutral wording in the durable transcript. I found one remaining failure-ordering issue below. The current-head test check passed, but I did not run local TUI tests or a cross-surface smoke test. The earlier CHANGES_REQUESTED review is attached to an older commit, not this head.

Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.

const restageForRetry = (): boolean => {
if (!staged.length) return false;
if (input.driver.getSessionId() !== originSessionId) return false;
if (stagedGeneration !== originGeneration) return false;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: The generation only changes when staging is written. After a quoted submit clears staging, a second ordinary submit in the same session does not advance it; /quotes clear also returns early while staging is empty (lines 4695-4705). If the first admission then fails or resolves without a receipt, this callback restages its old QuoteRefs, so a later unrelated message silently carries the earlier quote despite the intervening user intent. Invalidate the pending restoration when an ordinary submit or explicit clear supersedes it, and test both orderings with a held admission.

Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/XL Under 2500 readable lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants