Skip to content

Commit 8220ed9

Browse files
committed
fix: enforce user message ordering before agent responses in turns
1 parent 8b10489 commit 8220ed9

File tree

5 files changed

+298
-6
lines changed

5 files changed

+298
-6
lines changed

docs/app-server-events.md

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -55,6 +55,33 @@ OpenCode has no native turn lifecycle notifications. The backend emits:
5555
- `turn/completed` after a successful prompt response
5656
- `error` for failed prompt paths (instead of emitting `turn/completed`)
5757

58+
## Message Ordering Invariant
59+
60+
Within a conversation thread, items MUST be displayed in semantic order:
61+
62+
1. **Turn Structure**: A turn consists of a user message followed by zero or more
63+
agent responses (messages, tool calls, reasoning, etc.)
64+
65+
2. **Ordering Rule**: Within each turn, the user message MUST appear before any
66+
agent responses, regardless of the item ID sequence numbers.
67+
68+
3. **Cross-Turn Order**: Turns are ordered by the user message's sequence number.
69+
70+
### Why This Matters
71+
72+
The backend assigns item IDs (`item_1`, `item_2`, ...) in event processing order,
73+
not semantic order. Race conditions can cause tool events to be processed before
74+
user message events, resulting in tool IDs < user message IDs.
75+
76+
### Enforcement (Defense in Depth)
77+
78+
- **Backend**: Pre-allocates user message ID at turn start to ensure it's always
79+
lowest in the turn (`event_translator.rs:start_turn`)
80+
- **Frontend**: Enforces semantic ordering in `sortItemsBySyntheticOrder()`
81+
regardless of ID sequence (`threadItems.ts`)
82+
83+
Both layers enforce the invariant so a regression in one doesn't break ordering.
84+
5885
## Notes
5986

6087
- All protocol translation stays in Rust by design.

src-tauri/src/backend/event_translator.rs

Lines changed: 79 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -119,9 +119,12 @@ impl SessionTranslationState {
119119
}
120120

121121
/// Start a new turn for a specific session.
122+
///
123+
/// INVARIANT: User message ID is always lowest in a turn. If not already set,
124+
/// we pre-allocate it here so any subsequent tool IDs are guaranteed higher.
122125
pub(crate) fn start_turn(&mut self, session_id: String, turn_id: String) {
123126
self.session_id = session_id.clone();
124-
let turn_state = self.session_turns.entry(session_id).or_default();
127+
let turn_state = self.session_turns.entry(session_id.clone()).or_default();
125128
// User prompt parts can arrive before `session.status=active` (common for
126129
// subagent sessions). Preserve the in-progress user message so later chunks
127130
// keep merging into the same frontend item instead of creating a duplicate.
@@ -135,8 +138,19 @@ impl SessionTranslationState {
135138
turn_state.reasoning_item_id = None;
136139
turn_state.reasoning_part_id = None;
137140
turn_state.reasoning_text_len = 0;
138-
turn_state.user_message_item_id = preserved_user_message_item_id;
139-
turn_state.user_message_text = preserved_user_message_text;
141+
142+
// INVARIANT: Pre-allocate user message ID if not already set.
143+
// This ensures user message ID is always lower than any tool IDs in this turn.
144+
if let Some(id) = preserved_user_message_item_id {
145+
let turn_state = self.session_turns.get_mut(&session_id).unwrap();
146+
turn_state.user_message_item_id = Some(id);
147+
turn_state.user_message_text = preserved_user_message_text;
148+
} else {
149+
let pre_allocated_id = self.next_item_id();
150+
let turn_state = self.session_turns.get_mut(&session_id).unwrap();
151+
turn_state.user_message_item_id = Some(pre_allocated_id);
152+
turn_state.user_message_text = String::new();
153+
}
140154
}
141155

142156
/// Prepare translation state for replaying historical messages.
@@ -2942,6 +2956,68 @@ mod tests {
29422956
);
29432957
}
29442958

2959+
#[test]
2960+
fn user_message_id_always_lower_than_tool_ids_in_turn() {
2961+
let mut state = SessionTranslationState::new("ses_test".into());
2962+
2963+
// Simulate turn starting with session.status=active
2964+
state.start_turn("ses_test".into(), "turn_1".into());
2965+
2966+
// Get user message ID (should be pre-allocated by start_turn)
2967+
let user_id = state.user_message_item("ses_test");
2968+
2969+
// Simulate tool calls getting IDs
2970+
let tool_id_1 = state.next_item_id();
2971+
let tool_id_2 = state.next_item_id();
2972+
2973+
// Parse sequence numbers
2974+
let user_seq: u64 = user_id.strip_prefix("item_").unwrap().parse().unwrap();
2975+
let tool_seq_1: u64 = tool_id_1.strip_prefix("item_").unwrap().parse().unwrap();
2976+
let tool_seq_2: u64 = tool_id_2.strip_prefix("item_").unwrap().parse().unwrap();
2977+
2978+
assert!(
2979+
user_seq < tool_seq_1,
2980+
"User message ID ({user_id}) must be lower than first tool ID ({tool_id_1})"
2981+
);
2982+
assert!(
2983+
user_seq < tool_seq_2,
2984+
"User message ID ({user_id}) must be lower than second tool ID ({tool_id_2})"
2985+
);
2986+
}
2987+
2988+
#[test]
2989+
fn start_turn_preserves_existing_user_message_id() {
2990+
let mut state = SessionTranslationState::new("ses_test".into());
2991+
2992+
// Simulate user message arriving BEFORE session.status=active
2993+
let early_user_id = state.user_message_item("ses_test");
2994+
2995+
// Now turn starts - should preserve the existing user message ID
2996+
state.start_turn("ses_test".into(), "turn_1".into());
2997+
2998+
// Get user message ID again - should be the same
2999+
let after_start_id = state.user_message_item("ses_test");
3000+
3001+
assert_eq!(
3002+
early_user_id, after_start_id,
3003+
"start_turn must preserve existing user message ID"
3004+
);
3005+
3006+
// Tool IDs should still be higher
3007+
let tool_id = state.next_item_id();
3008+
let user_seq: u64 = early_user_id
3009+
.strip_prefix("item_")
3010+
.unwrap()
3011+
.parse()
3012+
.unwrap();
3013+
let tool_seq: u64 = tool_id.strip_prefix("item_").unwrap().parse().unwrap();
3014+
3015+
assert!(
3016+
user_seq < tool_seq,
3017+
"User message ID ({early_user_id}) must be lower than tool ID ({tool_id})"
3018+
);
3019+
}
3020+
29453021
#[test]
29463022
fn part_delta_routes_reasoning_by_part_id() {
29473023
let mut state = make_state();

src-tauri/src/shared/codex_core.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2135,6 +2135,7 @@ mod tests {
21352135
"hello".to_string(),
21362136
Some(vec!["http://localhost/image.png".to_string()]),
21372137
None,
2138+
None,
21382139
)
21392140
.await;
21402141

src/utils/threadItems.test.ts

Lines changed: 118 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -145,6 +145,124 @@ describe("threadItems", () => {
145145
]);
146146
});
147147

148+
it("places user message before agent responses even when user has higher ID", () => {
149+
// BUG CASE: backend assigned tool ID before user message ID
150+
const items: ConversationItem[] = [
151+
{
152+
id: "item_1",
153+
kind: "tool",
154+
toolType: "fileChange",
155+
title: "Edit: App.tsx",
156+
detail: "",
157+
status: "completed",
158+
},
159+
{
160+
id: "item_2",
161+
kind: "message",
162+
role: "user",
163+
text: "Find where skills is passed to Composer",
164+
},
165+
{
166+
id: "item_3",
167+
kind: "message",
168+
role: "assistant",
169+
text: "Let me search for that...",
170+
},
171+
];
172+
173+
const prepared = prepareThreadItems(items);
174+
175+
// INVARIANT: User message must appear before agent responses
176+
expect(prepared[0].kind).toBe("message");
177+
expect((prepared[0] as { role: string }).role).toBe("user");
178+
expect(prepared.map((item) => item.id)).toEqual(["item_2", "item_1", "item_3"]);
179+
});
180+
181+
it("handles multiple turns with out-of-order IDs", () => {
182+
const items: ConversationItem[] = [
183+
// Turn 1: tool arrived before user message
184+
{
185+
id: "item_1",
186+
kind: "tool",
187+
toolType: "read",
188+
title: "Read foo.ts",
189+
detail: "",
190+
status: "completed",
191+
},
192+
{
193+
id: "item_2",
194+
kind: "message",
195+
role: "user",
196+
text: "First prompt",
197+
},
198+
{
199+
id: "item_3",
200+
kind: "message",
201+
role: "assistant",
202+
text: "First response",
203+
},
204+
// Turn 2: correct order
205+
{
206+
id: "item_4",
207+
kind: "message",
208+
role: "user",
209+
text: "Second prompt",
210+
},
211+
{
212+
id: "item_5",
213+
kind: "tool",
214+
toolType: "write",
215+
title: "Write bar.ts",
216+
detail: "",
217+
status: "completed",
218+
},
219+
];
220+
221+
const prepared = prepareThreadItems(items);
222+
223+
// Turn 1: user message should be first
224+
expect(prepared[0].id).toBe("item_2"); // user
225+
expect(prepared[1].id).toBe("item_1"); // tool (moved after user)
226+
expect(prepared[2].id).toBe("item_3"); // assistant
227+
// Turn 2: already correct
228+
expect(prepared[3].id).toBe("item_4"); // user
229+
expect(prepared[4].id).toBe("item_5"); // tool
230+
});
231+
232+
it("handles user message with multiple preceding tool items", () => {
233+
const items: ConversationItem[] = [
234+
{
235+
id: "item_1",
236+
kind: "tool",
237+
toolType: "read",
238+
title: "Read file1.ts",
239+
detail: "",
240+
status: "completed",
241+
},
242+
{
243+
id: "item_2",
244+
kind: "tool",
245+
toolType: "read",
246+
title: "Read file2.ts",
247+
detail: "",
248+
status: "completed",
249+
},
250+
{
251+
id: "item_3",
252+
kind: "message",
253+
role: "user",
254+
text: "User prompt",
255+
},
256+
];
257+
258+
const prepared = prepareThreadItems(items);
259+
260+
// User message should be first, followed by both tools
261+
expect(prepared[0].id).toBe("item_3"); // user
262+
expect(prepared[1].id).toBe("item_1"); // tool
263+
expect(prepared[2].id).toBe("item_2"); // tool
264+
});
265+
148266
it("summarizes explored reads and hides raw commands", () => {
149267
const items: ConversationItem[] = [
150268
{

src/utils/threadItems.ts

Lines changed: 73 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -456,22 +456,92 @@ function getSyntheticItemOrderKey(id: string): SyntheticItemOrderKey | null {
456456
};
457457
}
458458

459+
type IndexedItem = {
460+
item: ConversationItem;
461+
index: number;
462+
key: SyntheticItemOrderKey | null;
463+
isUserMessage: boolean;
464+
isAssistantMessage: boolean;
465+
isToolItem: boolean;
466+
};
467+
468+
/**
469+
* INVARIANT: User messages appear before agent responses within the same turn.
470+
*
471+
* This function enforces semantic ordering regardless of ID assignment order.
472+
* The backend may assign IDs in processing order (tool gets ID before user message),
473+
* but the UI must display user message first.
474+
*/
459475
function sortItemsBySyntheticOrder(items: ConversationItem[]) {
460476
if (items.length < 2) {
461477
return items;
462478
}
463-
const withIndex = items.map((item, index) => ({
479+
480+
const indexed: IndexedItem[] = items.map((item, index) => ({
464481
item,
465482
index,
466483
key: getSyntheticItemOrderKey(item.id),
484+
isUserMessage: item.kind === "message" && item.role === "user",
485+
isAssistantMessage: item.kind === "message" && item.role === "assistant",
486+
isToolItem: item.kind === "tool" || item.kind === "explore",
467487
}));
468-
withIndex.sort((a, b) => {
488+
489+
// First pass: sort by ID sequence within same family
490+
indexed.sort((a, b) => {
469491
if (a.key && b.key && a.key.family === b.key.family && a.key.seq !== b.key.seq) {
470492
return a.key.seq - b.key.seq;
471493
}
472494
return a.index - b.index;
473495
});
474-
return withIndex.map((entry) => entry.item);
496+
497+
// Second pass: enforce user-message-first invariant
498+
// For each user message, move it before any UNOWNED tool items that precede it.
499+
// A tool is "unowned" if there's no user message between it and the current user.
500+
// Stop at any message (user or assistant) - they mark turn boundaries.
501+
const result: IndexedItem[] = [];
502+
const maxTurnWindow = 20;
503+
504+
for (let i = 0; i < indexed.length; i++) {
505+
const current = indexed[i];
506+
507+
if (current.isUserMessage && current.key) {
508+
const userSeq = current.key.seq;
509+
const family = current.key.family;
510+
511+
// Find insertion point: before preceding unowned tool items
512+
let insertAt = result.length;
513+
let foundOwnedTools = false;
514+
515+
for (let j = result.length - 1; j >= 0; j--) {
516+
const prev = result[j];
517+
if (!prev.key || prev.key.family !== family) break;
518+
// Any message marks turn boundary - tools before it belong to that turn
519+
if (prev.isUserMessage || prev.isAssistantMessage) {
520+
foundOwnedTools = true;
521+
break;
522+
}
523+
// Only move tool/explore items, not reasoning/diff/review/todo
524+
if (!prev.isToolItem) break;
525+
// Tool item with lower seq - check if same turn
526+
if (prev.key.seq < userSeq && userSeq - prev.key.seq <= maxTurnWindow) {
527+
insertAt = j;
528+
} else {
529+
break;
530+
}
531+
}
532+
533+
// Only move if the tools are unowned (no prior message claimed them)
534+
if (foundOwnedTools) {
535+
insertAt = result.length;
536+
}
537+
538+
result.splice(insertAt, 0, current);
539+
} else {
540+
result.push(current);
541+
}
542+
}
543+
544+
return result.map((entry) => entry.item);
475545
}
476546

477547
export function prepareThreadItems(items: ConversationItem[]) {

0 commit comments

Comments
 (0)