Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 7 additions & 4 deletions packages/vscode-ide-companion/src/webview/EmbeddedApp.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -323,10 +323,13 @@ export function EmbeddedApp() {
const [switchingSessionId, setSwitchingSessionId] = useState<string>();
const [creatingSession, setCreatingSession] = useState(false);
const [editingMessage, setEditingMessage] = useState<EditingMessage>();
const latestSubmittedPromptRef = useRef<{
sessionId: string;
prompt: string;
} | undefined>(undefined);
const latestSubmittedPromptRef = useRef<
| {
sessionId: string;
prompt: string;
}
| undefined
>(undefined);
const sessionSwitchStartedAtRef = useRef(0);
const sessionSwitchTimerRef = useRef<
ReturnType<typeof setTimeout> | undefined
Expand Down
148 changes: 148 additions & 0 deletions packages/web-shell/client/components/MessageList.dom.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ vi.mock('./MessageItem', async () => {
assistantTurnFooterInfo,
sendFailed,
onRetrySend,
onEditUserMessage,
}: {
message: Message;
showAssistantActions?: boolean;
Expand All @@ -63,6 +64,7 @@ vi.mock('./MessageItem', async () => {
assistantTurnFooterInfo?: WebShellAssistantTurnFooterRenderInfo;
sendFailed?: boolean;
onRetrySend?: () => void;
onEditUserMessage?: () => void;
}) => {
if (message.role === 'tool_group') {
messageItemTestState.toolArrays.push(message.tools);
Expand Down Expand Up @@ -107,6 +109,17 @@ vi.mock('./MessageItem', async () => {
'data-testid': `disclosure-${message.id}`,
})
: null,
onEditUserMessage
? React.createElement(
'button',
{
'data-testid': `edit-${message.id}`,
onClick: onEditUserMessage,
type: 'button',
},
'edit',
)
: null,
showAssistantBranch
? React.createElement('button', {
'data-testid': `branch-${message.id}`,
Expand Down Expand Up @@ -353,6 +366,7 @@ function mount(
pendingApproval?: PermissionRequest | null;
failedPromptMessageId?: string;
onRetryFailedPrompt?: () => void;
onEditUserMessage?: (targetTurnIndex: number, content: string) => void;
} = {},
): HTMLElement {
const container = document.createElement('div');
Expand Down Expand Up @@ -392,6 +406,7 @@ function mount(
onCanScrollToBottomChange={opts.onCanScrollToBottomChange}
failedPromptMessageId={opts.failedPromptMessageId}
onRetryFailedPrompt={opts.onRetryFailedPrompt}
onEditUserMessage={opts.onEditUserMessage}
/>
</TranscriptRenderModeProvider>
</CompactModeContext.Provider>
Expand All @@ -416,8 +431,11 @@ function rerenderMessages(
catchingUp?: boolean;
isResponding?: boolean;
hasOlderHistory?: boolean;
historyCapacityReached?: boolean;
historyPaginationError?: boolean;
onLoadOlderHistory?: (options?: { force?: boolean }) => Promise<void>;
sessionKey?: string;
onEditUserMessage?: (targetTurnIndex: number, content: string) => void;
} = {},
): void {
const entry = mounted.find((item) => item.container === container);
Expand All @@ -435,8 +453,11 @@ function rerenderMessages(
catchingUp={opts.catchingUp}
isResponding={opts.isResponding}
hasOlderHistory={opts.hasOlderHistory}
historyCapacityReached={opts.historyCapacityReached}
historyPaginationError={opts.historyPaginationError}
onLoadOlderHistory={opts.onLoadOlderHistory}
sessionKey={opts.sessionKey}
onEditUserMessage={opts.onEditUserMessage}
/>
</TranscriptRenderModeProvider>
</CompactModeContext.Provider>
Expand Down Expand Up @@ -6505,3 +6526,130 @@ describe('MessageList — turn collapse (DOM)', () => {
expect(parallelAgentsSummary(c)).toBeNull();
});
});

describe('user message edit affordance (issue #10385)', () => {
function clickEdit(container: HTMLElement, messageId: string): void {
const button = container.querySelector(`[data-testid="edit-${messageId}"]`);
expect(button).not.toBeNull();
act(() => {
(button as HTMLButtonElement).dispatchEvent(
new MouseEvent('click', { bubbles: true }),
);
});
}

it('does not offer editing while older history is unloaded', () => {
const onEditUserMessage = vi.fn();
// Session-global history has more user turns than the loaded window:
// only the last two user messages are rendered. The window-local ordinal
// of the last message (1) is not its session-global turn index, so the
// edit affordance must not be offered while the window is incomplete.
const c = mount([userMsg('u4'), asstMsg('a4'), userMsg('u5')], undefined, {
hasOlderHistory: true,
onLoadOlderHistory: async () => {},
onEditUserMessage,
});
expect(c.querySelector('[data-testid="edit-u5"]')).toBeNull();
expect(c.querySelector('[data-testid="edit-u4"]')).toBeNull();
expect(onEditUserMessage).not.toHaveBeenCalled();
});

it('does not offer editing when older history was dropped for window capacity', () => {
const onEditUserMessage = vi.fn();
const c = mount([userMsg('u4'), asstMsg('a4'), userMsg('u5')], undefined, {
historyCapacityReached: true,
onEditUserMessage,
});
expect(c.querySelector('[data-testid="edit-u5"]')).toBeNull();
expect(onEditUserMessage).not.toHaveBeenCalled();
});

it('does not offer editing while an older-history page terminally failed to load', () => {
const onEditUserMessage = vi.fn();
// A non-retryable load-older failure latches the provider at
// hasMore=false, capacityReached=false, paginationError=true while the
// window is still missing older turns, so the window-local ordinal of
// the last message (1) is not its session-global turn index either.
const c = mount([userMsg('u4'), asstMsg('a4'), userMsg('u5')], undefined, {
historyPaginationError: true,
onEditUserMessage,
});
expect(c.querySelector('[data-testid="edit-u5"]')).toBeNull();
expect(onEditUserMessage).not.toHaveBeenCalled();
});

it('offers editing only for the last user message when the window is complete', () => {
const onEditUserMessage = vi.fn();
const c = mount([userMsg('u1'), asstMsg('a1'), userMsg('u2')], undefined, {
onEditUserMessage,
});
expect(c.querySelector('[data-testid="edit-u1"]')).toBeNull();
clickEdit(c, 'u2');
expect(onEditUserMessage).toHaveBeenCalledTimes(1);
expect(onEditUserMessage).toHaveBeenCalledWith(1, 'q');
});

it('re-offers editing when hasOlderHistory flips without a message identity change', () => {
const onEditUserMessage = vi.fn();
const messages = [userMsg('u4'), asstMsg('a4'), userMsg('u5')];
const c = mount(messages, undefined, {
hasOlderHistory: true,
onEditUserMessage,
});
expect(c.querySelector('[data-testid="edit-u5"]')).toBeNull();
// A load-older request resolving with zero additional turns flips the
// flag while the messages array keeps its identity; the render callback
// must not hold the stale flag value.
rerenderMessages(c, messages, {
hasOlderHistory: false,
onEditUserMessage,
});
expect(c.querySelector('[data-testid="edit-u5"]')).not.toBeNull();
clickEdit(c, 'u5');
expect(onEditUserMessage).toHaveBeenCalledWith(1, 'q');
});

it('re-offers editing when historyCapacityReached flips without a message identity change', () => {
const onEditUserMessage = vi.fn();
const messages = [userMsg('u4'), asstMsg('a4'), userMsg('u5')];
const c = mount(messages, undefined, {
historyCapacityReached: true,
onEditUserMessage,
});
expect(c.querySelector('[data-testid="edit-u5"]')).toBeNull();
rerenderMessages(c, messages, {
historyCapacityReached: false,
onEditUserMessage,
});
expect(c.querySelector('[data-testid="edit-u5"]')).not.toBeNull();
});

it('re-offers editing when historyPaginationError flips without a message identity change', () => {
const onEditUserMessage = vi.fn();
const messages = [userMsg('u4'), asstMsg('a4'), userMsg('u5')];
const c = mount(messages, undefined, {
historyPaginationError: true,
onEditUserMessage,
});
expect(c.querySelector('[data-testid="edit-u5"]')).toBeNull();
// A successful retry of the failed load-older page clears the flag
// while the messages array keeps its identity; the render callback
// must not hold the stale flag value.
rerenderMessages(c, messages, {
historyPaginationError: false,
onEditUserMessage,
});
expect(c.querySelector('[data-testid="edit-u5"]')).not.toBeNull();
});

it('does not count user_shell echoes when numbering user turns', () => {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[Critical] R1-2: This test pins only half of the echo-skew question the PR description settles. editableUserTurn numbers every rendered role === 'user' message, but rewind snapshots are indexed by admitted daemon turns only (Session.ts makeSnapshot deliberately skips locally handled commands). Locally echoed slash commands — /stats, /status, /about, /context; none are hidden by VSCODE_HIDDEN_SLASH_COMMANDS — append kind: 'user' blocks with no meta via echoLocalCommandIfIdle (App.tsx:10498) → appendLocalUserTranscriptMessage, and the adapter renders them role: 'user', so they ARE counted while every gate flag correctly stays false on a complete window. The affordance then lands on the echo row itself, or passes an inflated ordinal for the next real prompt; editUserMessage finds no snapshot at that index → rewind.empty after the composer was replaced (standalone), or composer.editExpired at re-submit with subsequent sends kept cancelling until the editing state is cleared (the shipped VS Code flow, EmbeddedApp.tsx:1371-1414). One routine /stats breaks message editing for the rest of the live session view; the drift clears only on reload. A second entrance of the same root: a definitely-rejected send's optimistic block persists (only attachments are removed) and is counted too, with no snapshot counterpart. The user_shell case pinned here covers !-style shell echoes only — the description's "no numbering distortion at the message level" holds for those, not for slash-command echoes.

Witness (probe in the PR's own harness at the reviewed commit; it flips):

[..., echo('/stats')]                 → edit button rendered ON the echo row; onEditUserMessage(2, '/stats')
[..., echo, ..., userMsg('u2')]       → clickEdit('u2') passes (2, 'q') against daemon snapshot indices {0,1}
same array minus the echo             → clickEdit('u2') passes (1, 'q')  ← matches the daemon index

Suggested fix: tag local echoes at creation (e.g. meta: { source: 'local_command' } in appendLocalUserTranscriptMessage — the adapter already lifts meta.source) and skip such blocks in the editableUserTurn producer (both the numbering and the lastId candidate); applying the same marker to never-dispatched optimistic blocks also closes the rejected-send entrance. Failing closed whenever such a block is present is the minimal in-PR alternative.

Fix acceptance criterion: a DOM test mounting [userMsg('u1'), asstMsg('a1'), localEchoMsg('e1'), userMsg('u2')] with the local marker — assert no edit-e1 button and clickEdit(c, 'u2') calling onEditUserMessage with (1, 'q'); removing the producer skip must turn it red.

中文说明

这个测试只钉住了 PR 描述所下的“回显序号偏差”结论的一半。editableUserTurn 对每一个渲染出的 role === 'user' 消息编号,但回退快照只按守护进程真正受理的轮次索引(Session.ts 的 makeSnapshot 有意跳过本地处理的命令)。本地回显的 slash 命令 —— /stats/status/about/context,均未被 VSCODE_HIDDEN_SLASH_COMMANDS 隐藏 —— 经 echoLocalCommandIfIdle(App.tsx:10498)→ appendLocalUserTranscriptMessage 追加不带 meta 的 kind: 'user' 块,适配器把它们渲染为 role: 'user',因此它们会被计入编号,而窗口完整时所有门控标志都正确地保持为 false。于是编辑入口要么落在回显行本身上,要么为下一条真实消息传入偏大的序号;editUserMessage 在该索引上找不到快照 → 独立端在 composer 已被替换后抛 rewind.empty;发货的 VS Code 流程(EmbeddedApp.tsx:1371-1414)则在重新提交时抛 composer.editExpired,并在编辑状态被清除前持续取消后续发送。一次再平常不过的 /stats 就会让当前会话视图里的消息编辑在整个会话期间失效(只有重载才恢复)。同一根因的第二个入口:被彻底拒绝的发送留下的乐观块仍然存在(只移除了附件)且同样被计入编号,却没有对应快照。这里钉住的 user_shell 用例只覆盖 ! 形式的 shell 回显 —— 描述中“消息层不存在序号偏差”的结论对 shell 回显成立,对 slash 命令回显不成立。

验证探针(在被审提交上用 PR 自带的测试框架运行,可翻转):含回显时编辑按钮渲染在回显行上、传入 (2, '/stats');回显之后的真实消息点击编辑传入 (2, 'q'),而守护进程快照索引只有 {0,1};去掉回显后传入 (1, 'q'),与守护进程索引一致。

建议修复:在创建时给本地回显打标记(如在 appendLocalUserTranscriptMessage 中加 meta: { source: 'local_command' } —— 适配器已提升 meta.source),并在 editableUserTurn 生产者中跳过这类块(编号与 lastId 候选都跳过);同一标记用于从未分发的乐观块即可同时关闭被拒发送的入口。退一步的最小方案:只要存在这类块就失败关闭。

验收标准:新增 DOM 测试,以 [userMsg('u1'), asstMsg('a1'), localEchoMsg('e1'), userMsg('u2')](回显带本地标记)挂载,断言没有 edit-e1 按钮、clickEdit(c, 'u2')(1, 'q') 调用 onEditUserMessage;移除生产者中的跳过逻辑必须让该测试变红。

— qwen3.8-max via Qwen Code /review (v0.22.2)

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Confirmed real at the reviewed head, but escalating instead of fixing in this pass — the correct fix exceeds the 3-file budget for this worker and needs a design decision.

Verified evidence:

  1. Snapshots skip local commands: Session.ts (~5232) places makeSnapshot after the slash-command/hook early returns, with a comment stating locally handled commands must not create phantom snapshots that desync the snapshot index.
  2. Echo path: echoLocalCommandIfIdle (App.tsx:7356; call sites 10498/10514 for /stats, /status, /about) → store.appendLocalUserMessage(text) with no meta → adapter renders role: 'user' (transcriptToMessages.ts:470-483 lifts meta.source when present) → counted by editableUserTurn (MessageList.tsx:2892, counts every role === 'user').
  3. /stats//status//about//context are absent from VSCODE_HIDDEN_SLASH_COMMANDS (EmbeddedApp.tsx:83-103).
  4. Rejected-send entrance confirmed: on a definite admission rejection the optimistic block persists with attachments stripped (actions.ts ~929-941) and no user.text.delta ever merges into it.

Design questions blocking a worker-pass fix:

  • Tagging surface spans ≥4 files: App.tsx echo wrapper, ChatPane.tsx direct echoes (/goal ~780, /context ~1054 — the latter is a toolbar button, not a rare path), the editableUserTurn producer in MessageList.tsx, plus tests.
  • Marker mechanism matters: appendTextDelta merges event meta into the existing block meta (existing.meta = { ...existing.meta, ...event.meta }), so tagging optimistic prompt blocks would survive daemon admission and wrongly skip admitted turns from numbering. Only never-dispatched blocks may be tagged, which argues for per-call-site tagging or an SDK-level local-origin flag (API change).
  • Semantics decision: skip-and-renumber vs fail-closed-while-present for the echo case, and how the rejected-send entrance should interact with the retry affordance.

Leaving unresolved for maintainer decision.

Comment thread
yiliang114 marked this conversation as resolved.
Comment thread
yiliang114 marked this conversation as resolved.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[Critical] R1-2: [fails-closed] This round's re-check rules this blocker still stands at 452ee22 — the base merge moved the echo path into sdk-typescript but changed nothing about the mechanism. editableUserTurn numbers every rendered role === 'user' message, but rewind snapshots are indexed by admitted daemon turns only. Locally echoed slash commands (/stats, /status, /about, /context) append kind:'user' blocks with no meta via echoLocalCommandIfIdle (App.tsx:7407) → store.appendLocalUserMessageappendLocalUserTranscriptMessage (sdk-typescript/src/daemon/ui/transcript.ts:145), and the adapter renders meta-less blocks as role:'user' (adapters/transcriptToMessages.ts:419), so they ARE counted while every gate flag correctly stays false on a complete window. The affordance then lands on the echo row itself, or passes an inflated ordinal for the next real prompt; editUserMessage (App.tsx:9491) finds no snapshot at that index and throws rewind.empty after the composer was replaced. One routine /stats breaks message editing for the rest of the live session view; the drift clears only on reload. A second entrance of the same root: a definitely-rejected send's optimistic block persists and is counted too, with no snapshot counterpart. The user_shell case pinned here covers !-style shell echoes only.

Witness (fresh DOM probe at 452ee22 in this review's scratch tree; it flips):

[..., echo('/stats')]        -> edit button rendered ON the echo row; onEditUserMessage(1, '/stats') vs snapshots {0}
[..., echo, a1, u2]          -> clickEdit('u2') passes (2, 'q') against snapshot indices {0,1}
same array minus the echo    -> clickEdit('u2') passes (1, 'q')  <- matches the daemon index
echo as role 'user_shell'    -> not counted, no button on the echo row

Suggested fix: tag local echoes at creation (e.g. meta: { source: 'local_command' } in the appendLocalUserMessage call chain) and skip such blocks in the editableUserTurn producer (both the numbering and the lastId candidate); applying the same marker to never-dispatched optimistic blocks also closes the rejected-send entrance. Failing closed whenever such a block is present is the minimal in-PR alternative.

Fix acceptance criterion: a DOM test mounting [userMsg('u1'), asstMsg('a1'), localEchoMsg('e1'), userMsg('u2')] with the local marker — assert no edit-e1 button and clickEdit(c, 'u2') calling onEditUserMessage with (1, 'q'); removing the producer skip must turn it red.

中文说明

本轮复查裁定该阻断在 452ee22 上依然成立 —— base 合并把回显路径挪进了 sdk-typescript,但机制本身没有任何变化。editableUserTurn 对每一个渲染出的 role === 'user' 消息编号,但回退快照只按守护进程真正受理的轮次索引。本地回显的 slash 命令(/stats/status/about/context)经 echoLocalCommandIfIdle(App.tsx:7407)→ store.appendLocalUserMessageappendLocalUserTranscriptMessage(sdk-typescript/src/daemon/ui/transcript.ts:145)追加不带 meta 的 kind:'user' 块,适配器把不带 meta 的块渲染为 role:'user'(adapters/transcriptToMessages.ts:419),因此它们会被计入编号,而窗口完整时所有门控标志都正确地保持为 false。于是编辑入口要么落在回显行本身上,要么为下一条真实消息传入偏大的序号;editUserMessage(App.tsx:9491)在该索引上找不到快照,在 composer 已被替换后抛出 rewind.empty。一次再平常不过的 /stats 就会让当前会话视图里的消息编辑在整个会话期间失效(只有重载才恢复)。同一根因的第二个入口:被彻底拒绝的发送留下的乐观块仍然存在且同样被计入编号,却没有对应快照。这里钉住的 user_shell 用例只覆盖 ! 形式的 shell 回显。

验证探针(本轮在 452ee22 上的独立临时树中运行的全新 DOM 探针,可翻转):含回显时编辑按钮渲染在回显行上、相对快照集 {0} 传入 (1, '/stats');回显之后的真实消息点击编辑传入 (2, 'q'),而快照索引只有 {0,1};去掉回显后传入 (1, 'q'),与守护进程索引一致;回显以 user_shell 角色渲染时不被计入、回显行上无按钮。

建议修复:在创建时给本地回显打标记(如在 appendLocalUserMessage 调用链中加 meta: { source: 'local_command' }),并在 editableUserTurn 生产者中跳过这类块(编号与 lastId 候选都跳过);同一标记用于从未分发的乐观块即可同时关闭被拒发送的入口。退一步的最小方案:只要存在这类块就失败关闭。

验收标准:新增 DOM 测试,以 [userMsg('u1'), asstMsg('a1'), localEchoMsg('e1'), userMsg('u2')](回显带本地标记)挂载,断言没有 edit-e1 按钮、clickEdit(c, 'u2')(1, 'q') 调用 onEditUserMessage;移除生产者中的跳过逻辑必须让该测试变红。

— qwen3.8-max via Qwen Code /review (v0.22.3)

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Re-assertion of the open R1-2 thread (PRRT_kwDOPB-92c6dRhRj) — same file, same echo-skew finding, unchanged at this head. The author already responded there (3884091068) and the fix is deliberately human-gated: per-call-site tagging vs. an SDK-level flag spans >3 files and needs a design call before code lands. Tracking continues in the original thread; this duplicate stays open but unactioned until that decision is made.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Same blocker as the original R1-2 thread (PRRT_kwDOPB-92c6dRhRj), where the author has already responded. The fix remains human-gated: tagging local-echo/rejected-send blocks and skipping them in the editableUserTurn producer is a >3-file design decision spanning the sdk-typescript transcript store (appendLocalUserTranscriptMessage), the client adapter, and the test suite. The branch is being actively revised by the PR team (head 14cfa47 merged today); keeping this open until that work lands. No code changes from our side this round.

const onEditUserMessage = vi.fn();
const c = mount(
[userMsg('u1'), userShellMsg('s1'), asstMsg('a1'), userMsg('u2')],
undefined,
{ onEditUserMessage },
);
clickEdit(c, 'u2');
expect(onEditUserMessage).toHaveBeenCalledWith(1, 'q');
});
});
12 changes: 12 additions & 0 deletions packages/web-shell/client/components/MessageList.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -5437,8 +5437,19 @@ export const MessageList = memo(
onEditUserMessage={
onEditUserMessage &&
!isResponding &&
// Rewind snapshots are indexed session-globally, but the
// rendered transcript is a capped/paginated window. While
// older history is still unloaded (hasOlderHistory), was
// dropped for window capacity (historyCapacityReached), or
// the load-older page failed terminally
// (historyPaginationError — the provider then latches
// hasMore=false and merges nothing), the window-local
// user-turn ordinal is not the session-global turn index,
// so offering edit here would rewind to the wrong snapshot
// (or none at all). Fail closed (#10385).

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[Critical] R4-1: [fails-closed] The gate's certified invariant — window-local user-turn ordinal equals the session-global rewind-snapshot index — breaks even on a COMPLETE window once a session exceeds 100 admitted user turns. FileHistoryService caps snapshots at MAX_SNAPSHOTS = 100 (packages/core/src/services/fileHistoryService.ts:103) and evicts the oldest (816-820), while the rewind_snapshots RPC reports turnIndex: idx — the position in the evicted array (packages/cli/src/acp-integration/acpAgent.ts:8543). getRewindableUserTurnCount is uncapped (Session.ts:3981-3994), so above the cap the reported set stays 0..99 while the transcript ordinal keeps counting. A session with 101 admitted user turns fits entirely in the window (DEFAULT_MAX_BLOCKS = 1000), all three flags are false, and this gate offers edit — but the last message's ordinal (100) never matches the reported 0..99 set, so every click throws rewind.empty after the composer was replaced: the affordance is offered yet deterministically dead in exactly the state this comment certifies as safe. The same idx < rewindableTurnCount filter also drops the newest snapshots when context compression shrinks the API-history user-turn count, so compressed sessions fail identically below 100 turns (verified in mechanism; over-rewind is structurally excluded — the probe confirmed every surviving entry still maps to its correct turn).

Witness (probe running the real FileHistoryService at 452ee22):

ARM A (N=100): reportedTurnIndexes 0..99, lastOrdinal 99 -> lookup resolves, edit works
ARM B (N=101): firstPromptId rebased to turn 2, lastOrdinal 100 -> lookup 'rewind.empty', edit dead
ARM C (50 snapshots, rewindableTurnCount=30): only idx 0..29 reported -> 'rewind.empty', overRewindPossible: false

Deterministic split at exactly MAX_SNAPSHOTS + 1.

Suggested fix: stop deriving the rewind target from a positional index — resolve it by promptId/record id (the daemon already resolves promptId → snapshot, acpAgent.ts:11366-11382), or have the RPC report eviction-aware global ordinals; if the fix must stay inside this PR's client-side gate, fail closed when the window's user-turn count exceeds the resolvable snapshot count (≤ MAX_SNAPSHOTS). The fix must respect MAX_SNAPSHOTS = 100 (fileHistoryService.ts:103) and the positional turnIndex: idx contract (acpAgent.ts:8543) that the rewind handler's promptId → array-position resolution (acpAgent.ts:11382) relies on. Fix acceptance criterion: a case in this describe mounting a complete window of 101 user messages and asserting the edit button is NOT rendered (or that the handler receives a snapshot-resolvable id, if the promptId fix is chosen); removing the guard must turn it red.

中文说明

门控所认证的不变量 —— 窗口内用户轮序号等于会话全局回退快照索引 —— 在会话超过 100 个受理轮次后,即使窗口完整也会失效。FileHistoryService 把快照上限设为 MAX_SNAPSHOTS = 100(packages/core/src/services/fileHistoryService.ts:103)并驱逐最旧的(816-820),而 rewind_snapshots RPC 报告的是 turnIndex: idx —— 被驱逐后数组中的位置(packages/cli/src/acp-integration/acpAgent.ts:8543)。getRewindableUserTurnCount 不设上限(Session.ts:3981-3994),因此超过上限后上报集合仍是 0..99,而转录序号继续增长。101 个受理轮的会话完全放得进窗口(DEFAULT_MAX_BLOCKS = 1000),三个标志全为 false,门控放行编辑 —— 但最后一条消息的序号(100)永远匹配不上 0..99 的上报集合,每次点击都在 composer 已被替换后抛出 rewind.empty:入口被提供、在这段注释认证为安全的状态里却确定性地失效。同一 idx < rewindableTurnCount 过滤器也会在上下文压缩减少 API 历史用户轮数时丢掉最新的快照,因此压缩后的会话在 100 轮以下同样失效(机制已验证;过度回退在结构上被排除 —— 探针确认所有幸存条目仍映射到正确的轮次)。

验证探针(在 452ee22 上运行真实 FileHistoryService):ARM A(N=100)上报 0..99、最后序号 99 → 查找到,编辑可用;ARM B(N=101)firstPromptId 重定基到第 2 轮、最后序号 100 → 'rewind.empty',编辑失效;ARM C(50 个快照、rewindableTurnCount=30)只上报 0..29 → 'rewind.empty',overRewindPossible: false。分裂点恰在 MAX_SNAPSHOTS + 1

建议修复:不要再从位置索引推导回退目标 —— 按 promptId/记录 id 解析(守护进程已能把 promptId 解析到快照,acpAgent.ts:11366-11382),或让 RPC 上报感知驱逐的全局序号;若修复必须留在本 PR 的客户端门控内,则在窗口用户轮数超过可解析快照数(≤ MAX_SNAPSHOTS)时失败关闭。修复必须尊重 MAX_SNAPSHOTS = 100(fileHistoryService.ts:103)与位置式 turnIndex: idx 契约(acpAgent.ts:8543)—— 回退处理器的 promptId → 数组位置解析(acpAgent.ts:11382)依赖它。验收标准:在本 describe 中新增以 101 条用户消息的完整窗口挂载的用例,断言编辑按钮不渲染(若选择 promptId 修复,则断言处理器收到可解析的快照 id);移除该守卫必须让测试变红。

— qwen3.8-max via Qwen Code /review (v0.22.3)

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Verified as a real, distinct angle at 452ee22 — checked the anchors at this head:

  • MAX_SNAPSHOTS = 100 with oldest-first eviction — packages/core/src/services/fileHistoryService.ts:103, 816-820
  • rewind_snapshots reports the positional turnIndex: idx filtered by idx < rewindableTurnCount — packages/cli/src/acp-integration/acpAgent.ts:8528-8543
  • getRewindableUserTurnCount() is uncapped and counts API-history user turns — packages/cli/src/acp-integration/session/Session.ts:3981-3994 (note: lives in cli/acp-integration/session, not core)

So the gate's certified invariant (window-local ordinal == session-global snapshot index) breaks on a COMPLETE window past 100 admitted turns, and the rewindableTurnCount filter reproduces it below 100 under context compression. Distinct from R2-1: the window is complete and all three flags are correctly false here, so R2-1's positive completeness signal would not close this entrance. Leaving open — the durable fix (promptId-keyed snapshot resolution, which the daemon already supports for rewind execution, or eviction-aware ordinals) crosses core/cli and needs a design call rather than a client-side gate tweak in this PR.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Verified against head 14cfa47 — the mechanism holds at every cited point:

  • MAX_SNAPSHOTS = 100 with oldest-eviction: packages/core/src/services/fileHistoryService.ts:103,816-820
  • rewind_snapshots reports positional turnIndex: idx over the post-eviction array, filtered by idx < rewindableTurnCount: packages/cli/src/acp-integration/acpAgent.ts:8528,8538-8543
  • getRewindableUserTurnCount is uncapped and derived from API history (so compression shrinks it): packages/cli/src/acp-integration/session/Session.ts:3990
  • the gate reads only the three window flags, no snapshot-cap signal: packages/web-shell/client/components/MessageList.tsx:5450-5452
  • editUserMessage matches entry.turnIndex === turnIndex and throws rewind.empty after the composer is replaced: packages/web-shell/client/App.tsx:9545-9548
  • a 101-turn session fits one complete window: DEFAULT_MAX_BLOCKS = 1_000, packages/sdk-typescript/src/daemon/ui/transcript.ts:31

Classification: this is the same invariant class as R1-2/R2-1 — the gate's certified invariant (window-local user-turn ordinal == session-global rewind-snapshot index) is broken by server-side state (snapshot-cap eviction, compression-shrunk turn count) that none of the gate's client-side flags can express. No <=3-file fix exists: id-based resolution or eviction-aware ordinals changes the rewind-snapshot RPC contract across acpAgent.ts / Session.ts / fileHistoryService.ts plus the client gate, and a client-only fail-closed is impossible because the client cannot know the resolvable snapshot count without a provider signal. Folding into the same human-gated provider-contract design decision tracked in the original R2-1 thread (PRRT_kwDOPB-92c6dUzxW). No code changes this round; the branch is being actively revised by the PR team.

!hasOlderHistory &&
!historyCapacityReached &&
Comment thread
yiliang114 marked this conversation as resolved.
Comment thread
yiliang114 marked this conversation as resolved.
!historyPaginationError &&
Comment on lines +5450 to +5452

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[Critical] R2-1: The three flags negated here are treated as a complete signal that the window-local user-turn ordinal equals the session-global rewind-snapshot index, but the history provider has incomplete-window states that none of the three express. Proven by running the real provider: recordIds are stamped only during replay of persisted transcripts, never on the live event stream (bridge.ts ~6660), so when a client-side retention trim evicts the oldest blocks of a session lived from creation, onTruncation fires with no retained block carrying a recordId; the provider's fail-closed branch (DaemonSessionProvider.tsx ~858-877) then drops hasMore=false while preserving the other two flags at false — nothing records the eviction. The edit affordance is then offered on the last user message of a window that is missing persisted older turns; clicking it passes the window-local ordinal to editUserMessage (App.tsx:9439), which matches it against session-global snapshots and silently rewinds to an earlier turn, discarding everything after it — exactly the #10385 defect this gate exists to close.

Witness (real DaemonSessionProvider run, jsdom harness, maxBlocks: 2, live-only session — 3 user turns streamed, then count-trimmed):

PROBE-B observed: {"hasMore":false,"capacityReached":false,"paginationError":false,"loading":false,"windowKinds":["user","assistant"],"userBlocksInWindow":1,"userTurnsStreamed":3}

The window holds 1 of 3 user turns while all three gate flags read false. Two sibling states share the same root: the re-open byte gate (~836-856) only re-arms hasMore when postTrimRetainedBytes < byteCap — a single oversized block keeps it false after the trim; and the initial replay load (~1827-1834) computes historyHasMore false whenever the daemon lacks session_transcript_pagination (older external daemons) or no anchor recordId resolves, even when the replay itself was truncated. Default caps are large (50k blocks / 128MiB), so production reach needs a long pure-live session or a host-configured smaller window — but once reached, the state is deterministic.

Suggested fix: express window-completeness positively instead of as the complement of the three load flags — on an anchor-less eviction trim (and the truncated-replay initial-load states), have the provider latch a signal the gate can read (e.g. set capacityReached=true there, or add a fourth historyWindowIncomplete flag), and add it to this gate and to the render-callback dependency array below.

Fix acceptance criterion: a DaemonSessionProvider test asserting that after an anchor-less eviction trim the exposed history state still signals the window is incomplete, plus a case in this suite mounting with that signal set and asserting [data-testid="edit-u5"] is null — removing either the provider latch or the gate check must turn them red.

中文说明

这里取反的三个标志被当作"窗口内局部用户轮序号等于会话全局回退快照索引"的完备信号,但历史提供器存在这三者都无法表达的不完整窗口状态。通过运行真实提供器证实:recordId 只在重放持久化转录时打标,实时事件流上从不打标(bridge.ts ~6660),因此当一个从头活跃的会话被客户端保留策略裁剪掉最旧的块时,onTruncation 触发而留存块中没有任何一个携带 recordId;提供器的失败关闭分支(DaemonSessionProvider.tsx ~858-877)随即把 hasMore 置为 false,另外两个标志保持 false —— 驱逐发生得无声无息。此时窗口明明缺少已持久化的更早轮次,编辑入口却仍会出现在最后一条用户消息上;点击后把窗口内局部序号传给 editUserMessage(App.tsx:9439),后者按会话全局快照匹配,静默回退到更早的轮次,丢弃其后所有内容 —— 正是本门控要关闭的 #10385 缺陷。

验证证据(真实 DaemonSessionProvider 运行,jsdom 框架,maxBlocks: 2,纯实时会话 —— 流式输入 3 个用户轮后按数量裁剪):窗口只剩 3 个用户轮中的 1 个,而三个门控标志全部为 false(见上方 PROBE-B 输出)。

同根因还有两个兄弟状态:重开字节门(~836-856)仅在 postTrimRetainedBytes < byteCap 时才重新置位 hasMore —— 单个超大块就能让裁剪后仍不满足;初始重放加载(~1827-1834)在守护进程不支持 session_transcript_pagination(较老的外部守护进程)或无法解析锚点 recordId 时,即使重放本身被截断,也会把 historyHasMore 算成 false。默认上限很大(5 万块 / 128MiB),生产上需要一个长时间的纯实时会话或宿主配置的更小窗口才会触发 —— 但一旦触发,状态是确定性的。

建议修复:用正向信号表达窗口完整性,而不是三个加载标志的取反 —— 在无锚点驱逐裁剪(以及截断重放的初始加载状态)时,让提供器置位一个门控可读的信号(如在那里置 capacityReached=true,或新增第四个 historyWindowIncomplete 标志),并把它加入本门控和下方的渲染回调依赖数组。

验收标准:DaemonSessionProvider 测试断言无锚点驱逐裁剪后暴露的历史状态仍标记窗口不完整;并在本套件中新增一个以该信号挂载的用例,断言 [data-testid="edit-u5"] 为 null —— 移除提供器置位或门控检查都必须让测试变红。

— qwen3.8-max via Qwen Code /review (v0.22.2)

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Confirmed real at this head by code read — escalating instead of fixing in this pass, the correct fix is a provider-contract change beyond this worker's budget.

Verified evidence in DaemonSessionProvider.tsx:

  1. Anchor-less live-trim branch (~856-874): the state update is gated on if (history.hasMore). A live-only session loads with hasMore=false (initial replay load ~1827 computes historyHasMore=false with no persisted history), so when a retention trim evicts oldest blocks and no retained block carries a recordId, nothing records the eviction — all three gate flags stay false on a truncated window. Matches the reported PROBE-B shape exactly.
  2. Re-anchor branch byte gate (~836-856): hasMore is only re-armed when olderHistoryReachable (pagination feature && postTrimRetainedBytes < byteCap); a single oversized block leaves every flag false after the trim.
  3. capacityReached is latched only on the replay-rebuild trim (~2184) and rejected-page (~3761) paths — never on live streaming trims.
  4. recordIds flow in only via evidenceCursor on replayed persisted records (mappers.ts ~602); live-streamed blocks carry none, which is what makes live trims anchor-less.
  5. maxBlocks / maxRetainedBytes are host-configurable provider options (types.ts), so the state is reachable deterministically once a small window is configured or defaults are outlived.

Design questions blocking a worker-pass fix:

  • No existing flag can express all three states honestly: latching capacityReached=true misstates the truncated-replay initial-load state (no capacity event occurred) and feeds the provider's own re-open-on-eviction logic (~884) and rejected-page footprint accounting; paginationError is not an error; hasMore=true would offer a load-older affordance the exclusive-before anchor contract can never satisfy — exactly what the fail-closed branch comment refuses.
  • The honest fix is a positive window-completeness signal (e.g. a fourth historyWindowIncomplete flag), which is a provider API change: types.ts + DaemonSessionProvider.tsx (three latch sites) + both wiring call sites (App.tsx 13247-13252, ChatPane.tsx 1318-1321) + the MessageList gate/deps + tests — over the 3-file budget, with flag naming/semantics and the older-daemon truncated-replay case to decide.

Leaving unresolved for maintainer decision.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[Critical] R2-1: [certifies-falsely] This round's re-check rules this blocker still stands at 452ee22 — the gate still reads exactly the three flags, and the provider still has incomplete-window states none of them express. (a) Anchor-less live retention trim: DaemonSessionProvider.tsx:839-876 re-opens hasMore only when olderHistoryReachable (pagination feature advertised AND postTrimRetainedBytes < byteCap); when it is not, and in the uncomputable-anchor branch, nothing is latched while older turns were evicted. (b) Truncated replay on a daemon that does not advertise session_transcript_pagination (or has no firstPersistedRecordId): historyHasMore computes false (DaemonSessionProvider.tsx:1830-1837) and the injection (1855-1864) publishes hasMore:false, capacityReached:false, paginationError:false over a truncated window. In either state the gate opens, the affordance is offered on the last user message, and clicking it passes the window-local ordinal to editUserMessage, which matches it against session-global snapshots and silently rewinds to an earlier turn, discarding everything after it — exactly the #10385 defect this gate exists to close. This round's four finder agents independently rediscovered these entrances; they fold into this re-post under the original id.

Witness (probe driving the real DaemonSessionProvider at 452ee22):

[no-pagination-feature]  -> {"hasMore":false,"capacityReached":false,"paginationError":false}, window holds 1 of the session's user turns, truncation marker visible
[feature-but-no-anchor]  -> {"hasMore":false,"capacityReached":false,"paginationError":false}, same shape
[feature-and-anchor]     -> {"hasMore":true,...}  <- flips, proving the probe discriminates

Suggested fix: express window-completeness positively instead of as the complement of the three load flags — on an anchor-less eviction trim and the truncated-replay injection states have the provider latch a signal the gate can read (e.g. set capacityReached=true there, or add a fourth historyWindowIncomplete flag), and add it to this gate and the render-callback dependency array. The fix must not re-open the load-older affordance anchor-less: DaemonSessionProvider.tsx:895-898if (!paginationSupported || !anchored) { return; } — and the fail-closed eviction branch (862-876) deliberately drops both anchors. Fix acceptance criterion: a DaemonSessionProvider test asserting that after an anchor-less eviction trim or a truncated replay the exposed history state still signals the window is incomplete, plus a case in this suite mounting with that signal set and asserting [data-testid=edit-u5] is null; removing either the provider latch or the gate check must turn them red.

中文说明

本轮复查裁定该阻断在 452ee22 上依然成立 —— 门控仍然只读取这三个标志,而 provider 仍存在这三个标志都无法表达的不完整窗口状态。(a) 无锚点的在线保留裁剪:DaemonSessionProvider.tsx:839-876 仅在 olderHistoryReachable(通告了分页功能且 postTrimRetainedBytes < byteCap)时才重新打开 hasMore;不满足时、以及重锚点不可计算分支中,较早轮次已被驱逐却没有置位任何标志。(b) 未通告 session_transcript_pagination(或没有 firstPersistedRecordId)的守护进程上的截断重放:historyHasMore 计算为 false(DaemonSessionProvider.tsx:1830-1837),注入(1855-1864)在截断的窗口上发布 hasMore:false, capacityReached:false, paginationError:false。在这两种状态下门控都会打开,编辑入口出现在最后一条用户消息上,点击后把窗口内局部序号传给 editUserMessage,与全局快照匹配后静默回退到更早的轮次,丢弃其后的一切 —— 正是本门控要关闭的 #10385 缺陷。本轮四个探查代理独立重新发现了这些入口;现以原编号并入本条重发。

验证探针(在 452ee22 上驱动真实 DaemonSessionProvider):[无分页功能] 与 [有功能但无锚点] 两个分支均在窗口只含会话 1 个用户轮、截断标记可见的情况下发布 {"hasMore":false,"capacityReached":false,"paginationError":false};[有功能且有锚点] 分支翻转为 {"hasMore":true,...},证明探针具有区分力。

建议修复:把窗口完整性表达为正向信号,而不是三个加载标志的补集 —— 在无锚点驱逐裁剪与截断重放注入状态中,让 provider 置位一个门控可读的信号(如在那里设 capacityReached=true,或新增第四个 historyWindowIncomplete 标志),并把它加入本门控与渲染回调依赖数组。修复不得在无锚点时重新打开加载更早内容的入口:DaemonSessionProvider.tsx:895-898 —— if (!paginationSupported || !anchored) { return; } —— 失败关闭的驱逐分支(862-876)有意丢弃两个锚点。验收标准:DaemonSessionProvider 测试断言无锚点驱逐裁剪或截断重放后暴露的历史状态仍发出窗口不完整信号,并在本套件中以该信号置位挂载、断言 [data-testid=edit-u5] 为 null;移除 provider 置位或门控检查任一项都必须让测试变红。

— qwen3.8-max via Qwen Code /review (v0.22.3)

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Re-assertion of the open R2-1 thread (PRRT_kwDOPB-92c6dUzxW) — same line (MessageList.tsx:5452), same finding: the gate reads the three negated flags as a complete window-completeness signal while the provider has incomplete-window states none of them express. The author already responded there (3885451398); the fix is human-gated on the provider-contract decision (positive window-completeness signal vs. a fourth flag, ~5 files). Tracking continues in the original thread; this duplicate stays open until that decision is made.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Same blocker as the original R2-1 thread (PRRT_kwDOPB-92c6dUzxW), where the author has already responded. The fix remains human-gated: expressing window-completeness positively (a provider latch on anchor-less eviction / truncated replay, or a fourth flag) is a provider-contract design decision in DaemonSessionProvider that the PR team owns. The branch is being actively revised by the PR team (head 14cfa47 merged today); keeping this open until that decision lands. No code changes from our side this round.

displayItem.message.role === 'user' &&
editableUserContent !== undefined &&
displayItem.message.id === editableUserTurn.lastId
Expand Down Expand Up @@ -5516,6 +5527,7 @@ export const MessageList = memo(
editableUserTurn,
hasOlderHistory,
historyCapacityReached,
Comment thread
yiliang114 marked this conversation as resolved.
historyPaginationError,
Comment thread
yiliang114 marked this conversation as resolved.
generateContent,
headerOffset,
visibleItems,
Expand Down
Loading