Repository navigation
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.
…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.
hqhq1025
left a comment
There was a problem hiding this comment.
Reviewed exact head bd25e82fc2044fee586c8a16660129fc88417605. The previous parent-text leak on side open and the post-retraction text overwrite on side close are addressed by this revision. I found one additional P2 in a later close window (inline). The two P2 findings already published in review 5362482066, concerning lost retracted quotes on session changes and stale drafts in Ctrl+/, remain and are not duplicated here. This head is not ready to merge.
Validation: Node 24 clean npm ci, build:test, focused TUI switch/retraction/close tests 8/8, git diff --check, and a clean merge-tree against current main ed38ccbb. Exact-head hosted test passed. The new finding follows the async close control flow and the editor input contract; I did not run a dedicated live-TUI close-gate reproduction. Native Windows/macOS and a real separated Runtime Host were not tested.
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.
…m dying in the switch paths Ninth-round review (apache#5265 review) flagged three P2s and three P3s in what happens to a drained retraction's payload after the drain: - Quotes lost across sessions (P2): every switch path cleared the staging a drained retraction had just staged, and the Host already removed the queue entries the quotes came from - the TUI copy was the only one, so side open, /session, a quote-only side close, and a rewind all lost them. Retraction-restored quotes now ride the recovered draft text, wherever it goes: applySwitchResult still clears turn-staged quotes outright (the apache#5109 rule stands) but spares the draft-riding lane; the side conversation's draft slots carry quotes alongside their text (parked on toggle/open, staged back for the owning view on return); a rewind that branches without its own quotes keeps them; a rewind that carries its own quotes still replaces them (apache#5109 semantics, now visible and pinned by a test with distinct quotes per side). - Ctrl+/ lost the recovered text (P2): the toggle captured the draft before waiting a pending retraction out, then wrote the stale draft back over what the drain had restored - side editor, parent editor, and Host queue all empty with no notice. The capture moved after the switch's drain, like the side open. - The close window overwrote live typing (P2): a close admitted against an empty editor left input unlocked while its driver call was in flight, then setText(parentDraft) discarded whatever was typed during the wait. The live editor content now outranks the parked parent draft (kept, parent draft appended below when both exist). - Quote-only close (P3): the abort guard only looked at editor text, so a retraction returning quotes with no text still closed and dropped the staging; staged quotes now count as a keep-open reason. - Notice wording (P3): the kept-open notice no longer claims a retracted message for what may be the user's own typing - two neutral literals replace the old one (check:tui-copy registry updated). - Rewind test (P3): the drain-before-rewind test minted the same quote on both sides, so the designed replacement could not be distinguished from survival; each rewind now carries a distinct quote and the resubmit asserts the rewound turn's quote won. Every finding verified red under the reviewer's reproduction on the pre-fix runner and green after; new coverage asserts what the TUI actually submitted (driver.submittedQuotes) across the side round trip, the /session switch, and the quote-less rewind. Full pi-tui-runner suite: 252 pass / 2 known pre-existing SIGTERM failures. check:tui-copy, check:locale-hygiene (vs upstream/main), check:asf-headers, and biome on the touched files pass; merge-tree against upstream/main is clean. Generated-by: GLM-5.3-Flash (ZCode)
|
{"body":"Addressing the three P3s from #5265 (review) on |
hqhq1025
left a comment
There was a problem hiding this comment.
Reviewed exact head 5e538681b1f84439796923b06f977f32cf40ea5a. The direct session-switch, side-toggle, and side-close paths now preserve the retracted payload or live input as intended. One retry path still loses restored quotes after a failed admission followed by a switch (inline finding), so I would not treat this head as ready to merge.
Node 24 build:test, eight focused TUI cases, TUI copy validation, diff-check, the exact-head hosted test, and a static merge with current main 5ac266b1 pass. I did not run an interactive TUI against a separate real Host or native Windows/macOS. The finding is based on the staging control flow; I did not add a dedicated dynamic failure-plus-switch probe.
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.
The last open P2 from the ninth-round review (apache#5265 review) is an ordering bug in the submit path's staging bookkeeping: - A submit captured the staged quotes' draft-following provenance AFTER its own clear: clearStagedQuotes() resets the flag, so for every quote-carrying submit the capture always read false. When the admission then failed (or resolved without a receipt), restageForRetry() rebuilt the staging as ordinary turn-staged quotes, and the next /session or side-view switch cleared them in applySwitchResult() - stranding the recovered text without its context, the exact loss the draft-following lane exists to prevent. The read now happens before the dispatch's clear, so a failed restage re-arms the quotes in the lane they were staged in and a later switch carries them with the draft. The generation guard keeps its post-clear read: the apache#5109 restage-fence semantics are untouched. New coverage pins the combination path no prior test reached: a retraction-restored quote submit whose admission fails, then a /session switch - the resubmit after the switch must still carry the quotes. Red under the reviewer's ordering on the pre-fix runner, green with the moved read, and red again with only the read moved back (ablation). Focused quote/retraction/switch/side pattern 47/47; full pi-tui-runner suite 253/255 with the two failures being the known pre-existing SIGTERM tests; biome on the touched files, check:tui-copy, and check:asf-headers pass. Generated-by: GLM-5.3-Flash (ZCode)
hqhq1025
left a comment
There was a problem hiding this comment.
Reviewed exact head eb0f8e4edaafa54241aa06f1c96dd4a6a6e8eed1. The follow-up carries retraction-restored quotes with recovered drafts across session/side transitions, preserves that provenance after a failed submit, and retains text typed during side close. Those paths are covered by focused tests, but I found one P2 switch-serialization gap (inline). I would not treat this head as ready to merge until that race is addressed or disproved.
Node 24 clean install and core/storage/runtime/runtime-host/MCP/eval/CLI builds passed. Ten focused retraction/switch tests and the TUI copy guard passed. Current-head hosted test is successful; fresh main d7dffca9 merge-tree and diff-check are clean. I did not run a real separated Runtime Host, native Windows/macOS, or the full CLI suite. The concurrency finding is from the reachable control flow, not a live Host reproduction.
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.
The mid-turn `/session` detach set `detaching` only after awaiting the retraction drain, while every other navigation guard (`goToSession`, the side toggle, a mid-turn side open) reads `detaching` to refuse concurrent switches. With a retraction held in flight, two `/session` commands (or a command and a toggle) could both pass their guard while waiting on the same drain and then call `driver.switchSession` concurrently: the Runtime Host driver does not serialize those calls, so the later navigation can be lost to the channel-generation fence, and concurrent side toggles can apply the draft handoff twice. Reserve the detach before the drain so the existing guard holds through adoption, and hoist the `goToSession` guard above the idle branch: the running Turn can end while the drain is pending, which makes the idle path reachable mid-drain. Both new regressions drive two navigations across one held retraction and assert a single `driver.switchSession`; both fail on the previous head with two concurrent re-keys. Generated-by: GLM-5.3-Flash (ZCode)
|
@hqhq1025 gentle ping for a re-review when you have a moment. Your 09-30 P2 ("Reserve the mid-turn detach before awaiting the retraction drain") was confirmed against the code at Details are in the thread. (Re-requesting you through the review UI isn't available to this account, hence the mention.) |
Astro-Han
left a comment
There was a problem hiding this comment.
Automated review notice: This comment was posted by an automated review agent. It is not an independent human review and does not replace one.
Incremental review of eb0f8e4e → d8bef927. The prior P2 is fixed. detaching is now reserved before the retraction drain, goToSession's guard covers the idle branch, and two regressions cover both paths.
P2 — CI test fails because of main #5910. #5910 rebound retract from Alt+Up to Shift+Left, but the PR's tests still send \x1b[1;3A, so 18 retraction tests time out on the merge ref. Please merge main, switch the sends to \x1b[1;2D, and update the "Alt+Up" comments. One P3 is inline.
|
@hqhq1025 Re-review requested at head
Also on this head: local |
…rain window for /side and /new Main apache#5910 removed the Alt+Up retract chord and rebound retraction to Shift+Left, so on the merge ref a `\x1b[1;3A` send no longer retracts: this PR's retraction tests sat waiting for a refill that never came and the CI `test` job timed out. Point all 22 staged sends at `\x1b[1;2D` and rename the Alt+Up comments and test titles to Shift+Left, matching the binding apache#5910 left behind. Merging main also completes the drain-window picture the previous commit shored up for `/session`: with the Turn ended while a mid-turn detach is still parked inside its retraction drain, an idle `/side` — and `/new`, which bypasses the busy gate by design (apache#3210 review) — could pass unguarded and re-key the driver concurrently with the in-flight switch's own re-key. Return early on `detaching` at the top of `openSideConversation` and `newSession`. Both new regressions drive the second navigation across one held retraction and assert it never reaches the driver; both fail on the previous head with the concurrent re-key. Generated-by: GLM-5.3-Flash (ZCode)
Astro-Han
left a comment
There was a problem hiding this comment.
Automated review notice: This comment was posted by an automated review agent. It is not an independent human review and does not replace one.
Incremental review, d8bef92 -> 737f470. The delta is a merge of upstream main (aa2e162, merge-base now 49e01b1). The merge was clean: git show --remerge-diff is empty, so no conflict resolution needed checking. After it comes one author commit, 737f470. With main's changes removed, the author's change is pi-tui-runner.ts (+18/-6: six comment lines reworded to Shift+Left plus two detaching guards) and pi-tui-runner.test.ts (22 key sends retargeted plus two new tests).
Prior findings
- P2 (previously P1 in our notes): CI red because PR tests sent Alt+Up (
\x1b[1;3A), while main #5910 rebound retract to Shift+Left. FIXED. All 22 sends now use\x1b[1;2D, and the Alt+Up comments and titles are renamed to Shift+Left. No1;3AorAlt+Upreferences remain underpackages/cli/src. CItestpasses on 737f470 (run 37808080958). - P3:
/sideand/newcould re-key the driver during a mid-turn detach's retraction drain once the Turn ended. FIXED.openSideConversation(pi-tui-runner.ts:2341) andnewSession(:3871) now return early ondetaching, matching the/sessionguard at :2311. The only caller ofnewSession(the/newcommand's runControl body) ignores the boolean, soreturn falsehas no side effect. The two new regressions ("refuses an idle /side ..." and "refuses /new ... while a mid-turn detach is still draining") hold one retraction across the drain and assert the second navigation never reaches the driver.
New findings
None. Like the existing /session guard, the refusal is silent (no notice). That is consistent with current behavior and is not raised as a finding.
Protocol / epoch: No files under packages/runtime-host/src/protocol are touched. Not applicable.
Status: CI test passes. GitHub reports MERGEABLE but BLOCKED by an earlier CHANGES_REQUESTED review that is not ours. Main is 1 commit ahead (#5709, usage/storage only), with no overlap with the PR's files.
|
@me2seeks your changes-requested review is attached to head
Since then: 14 incremental review rounds by @hqhq1025 across successive heads (final one: "no remaining substantiated P0-P3"), @Astro-Han's incremental review of the current head Could you either re-review the current head or dismiss the outdated review so the merge gate reflects reality? Happy to walk through any remaining concern. |
|
Merged Conflict resolution, file by file:
Local verification on the merge commit: the 4 drain-window regression tests pass; full pi-tui-runner suite 278/278 (main's #3582 added tests, so the count grew from 260); pi-transcript 114 pass / 3 skipped (pre-existing Windows-path guards); runtime-host-session-driver 86/86; One note on CI: the |
Astro-Han
left a comment
There was a problem hiding this comment.
Automated review notice: This comment was posted by an automated review agent. It is not an independent human review and does not replace one.
I found no new P0–P3 issue in the update from 737f470716b752ae8a657879b9610178c2e648de to 4d7764420e2b9975c47e1c72ef7b6c6799493083. The requested 59cc0ef2 merge is included; the subsequent commit has an unchanged tree and retriggers CI.
The merge resolution preserves the retraction/drain fencing while adopting main's boolean session-switch result. A mid-turn detach is reserved before its retraction wait, and /session, /side, and /new retain their guards. The tests use Shift+Left, matching main's current binding.
The two issues in me2seeks' older CHANGES_REQUESTED review are addressed in this head:
- A submit resolving without a receipt now restages its quotes under the session/generation fence and displays an outcome-unknown notice. The code explicitly documents the possible-duplicate tradeoff; it does not silently treat uncertainty as success or automatically replay a submit (
pi-tui-runner.ts:1519–1559). - Durable messages use the neutral
N quote(s)label, including ordinary quoted messages and quote-only messages (pi-transcript.ts:1584–1590), rather than claiming every quote was restored by rewind.
The Core and CLI builds passed. The three complete TUI-runner, Runtime Host-driver, and transcript test files passed 478 tests, with three Windows-only skips. Disabling the generated outcome-unknown handling made its regression fail; restoring the old “restored quote” label made the durable-transcript regression fail. Both passed after restoring the generated code. Copy-policy, formatting, and diff checks passed.
This exact head's CI is green and the merge with current main is conflict-free. The older CHANGES_REQUESTED review is still present; these findings do not dismiss or replace that review. I did not exercise an interactive TUI against a separate live Runtime Host, native Windows/macOS, or the full repository suite.
|
@liugddx — escalation for the merge gate on this one, since it has been stuck for three weeks on a stale review. The only remaining blocker is me2seeks's CHANGES_REQUESTED from 2026-09-20, filed against
Could you either dismiss the stale review or review the current head? The branch has not moved since the thread clear-out, CI is green, and it is mergeable. This is one of eight PRs we are trying to land this week. |
liugddx
left a comment
There was a problem hiding this comment.
Reviewed exact head 4d7764420. The quote plumbing is the right shape: the driver hands QuoteRefs back verbatim and forwards them through the existing turn.message.submit admission (protocol/turn.ts:422-436), so no protocol or epoch change is needed. me2seeks' P1 and P2 are fixed. The outcome-unknown undefined now restages under the session/generation fence (pi-tui-runner.ts:1549-1559), and disabling that branch turns its regression red. The transcript label is now neutral (pi-transcript.ts:1590).
P2, scope: about half of the runner change, and roughly 1,276 of the 2,630 test lines, are retraction/switch fencing for pre-existing plain-text races (settleRetractions, holdSwitchWindow, the detaching guards, side-close abort). None of those tests involve quotes. Please split that fencing into its own PR so each half can be reviewed and reverted on its own.
P3: /new (:4071) is the only re-keying path outside the switch window. A Shift+Left retraction still in flight across /new is discarded after the Host has already removed the entries; please wrap it the way /session is wrapped.
P3: the live transient row (:1498) carries no quote count, so a quote-only resubmit renders as an empty prompt until reconnect. Pass staged.length through.
P3: no driver test covers retractQueued with non-empty quotes. The four "drains a retraction before X" tests and the three "blocks Shift+Left while X" tests could each be one table-driven test.
Verification on this head: I built core through cli locally (Windows). pi-transcript passed 115 (2 skipped), runtime-host-session-driver 86/86, tui-copy-catalog 8/8, check-tui-copy ok. pi-tui-runner passed 276/278; the 2 failures are the SIGTERM tests, which this PR doesn't touch. git merge-tree against current main is clean.
Review on apache#5265 (P2): about half of the runner change and ~1,276 test lines fenced retraction/switch races in plain-text handling that predates this branch — settleRetractions, holdSwitchWindow, the detaching guards, the side-close abort for recovered drafts, the retraction's session fence, and the Alt+Up/Shift+Left window blocks. None of that involves quotes. That fencing half moves to its own PR (branch fix/tui-switch-fencing), so each half can be reviewed and reverted on its own. This branch keeps only the quote-staging feature: the QuoteRef pipeline, rewind quote stage/restage, quote-only rewind, restaging a retracted message's quotes, the transcript quote trace, and every test that asserts quote content. Three quote tests were rewritten to observe a completed retraction instead of a held one (the held retraction was the fencing drain's scenario), and one test keeps the replacement contract a removed fencing test used to carry: a rewind branching with its own quotes replaces the quotes a retraction restored. The quote-only side-close abort stays: it has no meaning without the quote staging, and a quote-only retraction would otherwise lose its only copy on a side close.
Review on apache#5265 (P3): the live transient row carried no quote count, so a quote-only resubmit (empty text plus staged quotes) rendered as an empty prompt until the reconnect replay replaced it with the durable entry — which does carry the trace. Pass the staged count through appendUserPrompt so the live row shows the same `· N quote(s)` line from the moment the submit dispatches. The steering_message acknowledgment replaces the same row with the Host's echo, so it forwards the echoed content's quote count too — otherwise a quote-only steer would lose the trace again one hop later.
Review on apache#5265 (P3): no driver test covered retractQueued with quotes on the queued entry. The fake queue.retract now echoes the recorded submit's full MessageContent the way the Host does, and the new test pins the contract the runner's restage depends on: a queued submit's QuoteRefs come back with the retraction instead of vanishing with the queue row.
|
All four findings from your review of
Two judgment calls to flag:
Test matrix after the split (both heads): pi-tui-runner 261/261 here / 257/257 on #6070, pi-transcript 117 (114 pass / 3 skip), runtime-host-session-driver 87/87 here / 85/85 on #6070, tui-copy-catalog 8/8, Ready for re-review. |
Astro-Han
left a comment
There was a problem hiding this comment.
Automated review notice: This comment was posted by an automated review agent. It is not an independent human review and does not replace one.
I reviewed the update from 4d7764420e2b9975c47e1c72ef7b6c6799493083 to 27a82ea71ec00d7d1ab72a2cd31d5704d2ca6deb. I found no new verified P0–P2 issue in the retained quote-restoration changes.
The requested split is implemented: general retraction/session-switch fencing and its race tests have moved to #6070. This PR retains the quote plumbing, retry/session-generation guards, quote-only submissions, draft-associated quote restoration and the quote-only side-close guard. The previous outcome-unknown restoration and neutral durable N quote(s) label remain intact.
The two new coverage fixes are present:
pi-tui-runner.ts:1460reads the staged quotes before adding the transient row.pi-transcript.ts:1008preserves the count when asteering_messageacknowledgement replaces that row. My additional probe verified the live row, acknowledgement, plain acknowledgement without quotes, single-row identity and canonical reconstruction; omitting the acknowledgement count makes the probe fail.runtime-host-session-driver.test.ts:2141exercises non-empty quotes in the actual driver'squeue.retractresponse rather than only empty fixtures. The driver still returns the quote objects without changing their order.
With freshly installed dependencies, Core through CLI built successfully. Six complete CLI/Core test files passed 475 tests, with three Windows-only skips and no failures. CLI type checking, changed-file Biome, copy-policy and diff checks passed. The normal root test build did not fully pass: it reaches the unchanged Desktop workhub-dock.test.ts:99 fixture and fails on its obsolete enabled prop. Desktop, UI and the dependency lock are identical to this branch's base 031bca51, so this is an inherited build limitation, not a new quote-restoration finding.
CI 38148236748 passed on this exact head. The merge with main 6b9457f5b is conflict-free and has no protocol change at epoch 217. The old CHANGES_REQUESTED review has not been dismissed by this comment. After either this PR or #6070 lands, please rebase and verify the combined side-close/toggle paths: the quote-only and text-draft protections are intentionally split between the two branches. I did not test their combined tree, a separate live Host/TUI, native Windows/macOS, or the full repository suite.
|
Thanks @ggbdpq, the split is exactly what I asked for, and I re-checked it at
Both of your judgment calls are fine with me. The quote-only side-close guard belongs here. Carrying the count through the I'm not approving this exact head yet. It conflicts with #6070 in @me2seeks, both findings from your CHANGES_REQUESTED review on |
|
@liugddx @me2seeks All findings from both rounds are addressed at |
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.