Skip to content
Closed
Show file tree
Hide file tree
Changes from 4 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
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 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.
Resolved
Comment thread
yiliang114 marked this conversation as resolved.
Resolved

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');
});
});
16 changes: 16 additions & 0 deletions packages/web-shell/client/components/MessageList.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -5437,6 +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 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.
Resolved
Comment thread
yiliang114 marked this conversation as resolved.
Resolved
!historyPaginationError &&
Comment on lines +5450 to +5452

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 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 @@ -5512,6 +5525,9 @@ export const MessageList = memo(
onInsightReportOpen,
onEditUserMessage,
editableUserTurn,
hasOlderHistory,
historyCapacityReached,
Comment thread
yiliang114 marked this conversation as resolved.
Resolved
historyPaginationError,
Comment thread
yiliang114 marked this conversation as resolved.
Resolved
generateContent,
headerOffset,
visibleItems,
Expand Down
Loading