Conversation
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)
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
left a comment
There was a problem hiding this comment.
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.
| if (staged.length > 0) { | ||
| stagedRewindQuotes = staged; | ||
| stagedQuotesSessionId = input.driver.getSessionId(); | ||
| } |
There was a problem hiding this comment.
[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 |
There was a problem hiding this comment.
[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)
|
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 P2 (quote-only replacement): Regressions: |
Astro-Han
left a comment
There was a problem hiding this comment.
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)
|
Both findings fixed at 399f15e: P1 (restage unreachable): P2 (stale restage overwrites newer staging): all staged-quote writes now route through one Verification: full |
Astro-Han
left a comment
There was a problem hiding this comment.
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.
originGenerationis now read after the dispatch's ownclearStagedQuotes()(pi-tui-runner.ts:1310-1324), and every write routes throughsetStagedQuotes, 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 clearlanding 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 emptytext+ quotes is legal at every boundary. I also confirmed pi-tui's editor does callonSubmit('')for Enter on an empty buffer (editor.js:submitValue), so the new gates at:1227,:1367,:1385are actually reachable — and thataddToHistorydrops empty input, so no empty history entry is created. - No duplication when the admission outcome is lost.
RuntimeHostMakaSessionDriverImpl#submitMessageswallowsoutcome_unknown/ interrupted-after-dispatch and resolvesundefined, 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.
rewindToTurnreturnspromptMessage.quotesfrom 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 appliedTURN_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.createfor 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_quotescode or copy outside the two changed files; the new/quotesentry 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 thedisposition === 'blocked'branch (:1337) but no test covers it — only the rejected-promise path does. The existingHostSkillDriver(__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/quoteslisting order for >1 excerpt are unverified. A two-quote rewind result would pin all three. /quotesis declaredmidTurn: '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
left a comment
There was a problem hiding this comment.
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.
originGenerationis now read after the dispatch's ownclearStagedQuotes()(pi-tui-runner.ts:1310-1324), and every write routes throughsetStagedQuotes, 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 clearlanding 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 emptytext+ quotes is legal at every boundary. I also confirmed pi-tui's editor does callonSubmit('')for Enter on an empty buffer (editor.js:submitValue), so the new gates at:1227,:1367,:1385are actually reachable — and thataddToHistorydrops empty input, so no empty history entry is created. - No duplication when the admission outcome is lost.
RuntimeHostMakaSessionDriverImpl#submitMessageswallowsoutcome_unknown/ interrupted-after-dispatch and resolvesundefined, 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.
rewindToTurnreturnspromptMessage.quotesfrom 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 appliedTURN_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.createfor 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_quotescode or copy outside the two changed files; the new/quotesentry 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 thedisposition === 'blocked'branch (:1337) but no test covers it — only the rejected-promise path does. The existingHostSkillDriver(__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/quoteslisting order for >1 excerpt are unverified. A two-quote rewind result would pin all three. /quotesis declaredmidTurn: '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)
|
Addressed everything actionable at 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 Item 2 (hidden vs invalidated) — the staging is now invalidated. Item 3 (clear always reports success) — fixed. Item 4 (coverage) — all three gaps closed:
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 |
# Conflicts: # packages/cli/src/session-driver.ts
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.
|
Gentle ping — everything actionable from your 09-17 review landed at |
me2seeks
left a comment
There was a problem hiding this comment.
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)
|
Thanks for the adversarial re-read — both findings confirmed and fixed on P1 — outcome_unknown restage. Confirmed: the real driver resolves P2 — neutral trace. Also confirmed: Tests: new |
…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)
|
Follow-up on |
Generated-by: GLM-5.3-Flash (ZCode)
|
Gentle ping — the head is |
hqhq1025
left a comment
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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.
Summary
/rewindon a turn that carried quotes used to fail closed withrewind_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 onrewindToTurn, andsubmitMessageforwards quotes throughturn.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:
quotes:<n>segment while staging is live (accent salience, dropRank with the other chrome),/quoteslists the staged excerpts;/quotes clearis the explicit removal,The now-dead
rewind_unsupported_quotescode 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
/quotes cleardrops them)pi-tui-runnerfull suiterestores the terminal before exiting on SIGTERM,forces signal exit when outer cleanup never settles) fail identically on pristineupstream/mainbuilt in a clean worktree on this machine: pre-existing Windows signal-handling failuresruntime-host-session-driverfull suiteSession cwd no longer exists: /tmpPOSIX fixtures); none of them is a test added or modified heretui-copy-catalog,tui-primary-guidance, coreslash-command-catalogcheck:asf-headersformat:checkbiome check --formatter-enabled=true --linter-enabled=falseindividually, and upstream CI runs the repo-wide gateAI use
Select exactly one:
Tool(s) and scope: GLM-5.3-Flash (ZCode) implemented the driver/TUI changes and tests under human direction and review.
Checklist
Does this PR entail a change in behavior?
/quotesis a new TUI command.