From b45bb2ece8c7c68b0ccb80b8daa7e43c5a4545b0 Mon Sep 17 00:00:00 2001 From: Vasanthdev2004 Date: Wed, 9 Sep 2026 22:39:52 +0530 Subject: [PATCH 1/2] fix(sessions): render resume context fields in key order extractText walked a map[string]any directly and Go randomizes map iteration, so the same session put the same fields in a different order in every process: one run rendered a tool call as c1 read_file {"path":...}, the next as {"path":...} c1 read_file. That block is part of the user prompt, so the prompt itself changed run to run, which leaves a provider nothing stable to prefix-cache for it and makes two resumes of one session impossible to diff while debugging. Only the ordering is decided here. The summary short-circuit is untouched, a list keeps the order it was written in, and which fields survive at all stays the caller's projection. Pinned as exact rendered strings rather than as two renders agreeing: a same-process repeat passes whenever the random order happens to repeat, which for a small map is often. The raw-JSON case proved it while being written, matching on the first five attempts before diverging on the sixth. Closes #1020 --- internal/sessions/exec_prompt_order_test.go | 124 ++++++++++++++++++++ internal/sessions/exec_session.go | 22 +++- 2 files changed, 143 insertions(+), 3 deletions(-) create mode 100644 internal/sessions/exec_prompt_order_test.go diff --git a/internal/sessions/exec_prompt_order_test.go b/internal/sessions/exec_prompt_order_test.go new file mode 100644 index 000000000..927eea505 --- /dev/null +++ b/internal/sessions/exec_prompt_order_test.go @@ -0,0 +1,124 @@ +package sessions + +import ( + "encoding/json" + "strings" + "testing" +) + +// ONE SESSION HAS TO RENDER ONE PROMPT. +// +// extractText walked a map[string]any directly, and Go randomizes map +// iteration, so the resume context put the same fields in a different order in +// every process: `c1 read_file {"path":...}` one run, `{"path":...} c1 +// read_file` the next. That block is part of the user prompt, so the prompt +// itself changed run to run, which leaves a provider nothing stable to +// prefix-cache and makes two resumes of one session impossible to diff. +// +// PINNED AS AN EXACT STRING, NOT AS TWO RUNS AGREEING. A test that renders the +// same payload twice in one process and compares the results passes whenever +// the randomized order happens to repeat, which for a small map is often. The +// only assertion that cannot pass by luck names the order it expects. +func TestSummarizePayloadRendersFieldsInKeyOrder(t *testing.T) { + payload := map[string]any{ + "id": "c1", + "name": "read_file", + "arguments": `{"path":"deploy/prod.env"}`, + "status": "ok", + } + + // Keys sorted: arguments, id, name, status. + const want = `{"path":"deploy/prod.env"} c1 read_file ok` + for attempt := range 50 { + if got := summarizePayload(payload); got != want { + t.Fatalf("attempt %d rendered %q, want %q", attempt, got, want) + } + } +} + +// The same for a payload that arrives as raw JSON, which is the shape the store +// hands back, since that decodes into a map and takes the same path. +func TestSummarizePayloadRendersRawJSONInKeyOrder(t *testing.T) { + raw, err := json.Marshal(map[string]any{ + "zulu": "last", + "alpha": "first", + "mike": "middle", + }) + if err != nil { + t.Fatal(err) + } + + const want = "first middle last" + for attempt := range 50 { + if got := summarizePayload(json.RawMessage(raw)); got != want { + t.Fatalf("attempt %d rendered %q, want %q", attempt, got, want) + } + } +} + +// The summary short-circuit is untouched: a payload carrying one still answers +// with it alone, rather than with every field in key order. +func TestSummarizePayloadStillPrefersASummary(t *testing.T) { + payload := map[string]any{ + "alpha": "not this", + "summary": "this one", + "zulu": "nor this", + } + if got := summarizePayload(payload); got != "this one" { + t.Fatalf("rendered %q, want the summary alone", got) + } +} + +// And a list keeps the order it was written in, which is the caller's and not +// this function's to sort. +func TestSummarizePayloadKeepsListOrder(t *testing.T) { + payload := map[string]any{"items": []any{"third", "first", "second"}} + if got := summarizePayload(payload); got != "third first second" { + t.Fatalf("rendered %q, want the list in its written order", got) + } +} + +// THE WHOLE BLOCK IS STABLE, not just one payload. Rendered through the +// prompt-context path a resume actually uses, so a future renderer that walks a +// map of its own is caught here too. +func TestResumeContextBlockIsIdenticalAcrossRenders(t *testing.T) { + events := []Event{ + {Sequence: 1, Type: EventMessage, Payload: mustOrderPayload(t, map[string]any{"role": "user", "content": "rotate the deploy key"})}, + {Sequence: 2, Type: EventToolCall, Payload: mustOrderPayload(t, map[string]any{"id": "c1", "name": "read_file", "arguments": `{"path":"deploy/prod.env"}`})}, + {Sequence: 3, Type: EventToolResult, Payload: mustOrderPayload(t, map[string]any{"name": "read_file", "status": "ok", "output": "contents"})}, + } + + first := renderOrderContext(t, events) + if strings.TrimSpace(first) == "" { + t.Fatal("SETUP INVALID: the context block rendered empty, so identical renders prove nothing") + } + for attempt := 1; attempt < 50; attempt++ { + if got := renderOrderContext(t, events); got != first { + t.Fatalf("attempt %d rendered a different context block:\n first: %s\n then: %s", attempt, first, got) + } + } +} + +func renderOrderContext(t *testing.T, events []Event) string { + t.Helper() + lines := []string{} + for _, event := range promptContextEvents(events) { + var decoded any + if len(event.Payload) > 0 { + if err := json.Unmarshal(event.Payload, &decoded); err != nil { + t.Fatal(err) + } + } + lines = append(lines, string(event.Type)+": "+summarizePayload(decoded)) + } + return strings.Join(lines, "\n") +} + +func mustOrderPayload(t *testing.T, payload map[string]any) json.RawMessage { + t.Helper() + raw, err := json.Marshal(payload) + if err != nil { + t.Fatal(err) + } + return raw +} diff --git a/internal/sessions/exec_session.go b/internal/sessions/exec_session.go index b86b364b5..2858b3787 100644 --- a/internal/sessions/exec_session.go +++ b/internal/sessions/exec_session.go @@ -4,6 +4,7 @@ import ( "encoding/json" "fmt" "log" + "sort" "strings" "unicode/utf8" ) @@ -267,9 +268,24 @@ func extractText(value any) string { if summary, ok := typed["summary"].(string); ok && strings.TrimSpace(summary) != "" { return summary } - parts := []string{} - for _, item := range typed { - if text := extractText(item); text != "" { + // BY KEY, BECAUSE THIS TEXT BECOMES PART OF A PROMPT. Go randomizes map + // iteration, so the same session rendered its resume context in a + // different field order in every process: one run said + // `c1 read_file {"path":...}` and the next `{"path":...} c1 read_file`. + // The block goes into the user prompt, so the prompt itself changed run + // to run, which gives a provider nothing stable to prefix-cache and + // leaves two resumes of one session impossible to diff while debugging. + // + // Only the ordering is decided here. Which fields survive is the + // caller's projection, and a slice keeps the order it was written in. + keys := make([]string, 0, len(typed)) + for key := range typed { + keys = append(keys, key) + } + sort.Strings(keys) + parts := make([]string, 0, len(keys)) + for _, key := range keys { + if text := extractText(typed[key]); text != "" { parts = append(parts, text) } } From 6d19afa3c46f75994202eaa42f573007d63e64db Mon Sep 17 00:00:00 2001 From: Vasanthdev2004 Date: Fri, 11 Sep 2026 10:20:14 +0530 Subject: [PATCH 2/2] test(sessions): name the expected context block instead of the first render Comparing later renders against the first only asks whether the renderer is stable, and a renderer that is stably wrong passes it: any fixed field order satisfies it, including the unsorted one this change replaces. It also hid something. The events were tool calls, which promptContextEvents filters out of the resume context on this branch, so the block being compared was a single message line and nothing said so. The test now names the block it expects, on events that survive the filter, and both failures are visible. --- internal/sessions/exec_prompt_order_test.go | 30 ++++++++++++++------- 1 file changed, 21 insertions(+), 9 deletions(-) diff --git a/internal/sessions/exec_prompt_order_test.go b/internal/sessions/exec_prompt_order_test.go index 927eea505..51d169c51 100644 --- a/internal/sessions/exec_prompt_order_test.go +++ b/internal/sessions/exec_prompt_order_test.go @@ -81,20 +81,32 @@ func TestSummarizePayloadKeepsListOrder(t *testing.T) { // THE WHOLE BLOCK IS STABLE, not just one payload. Rendered through the // prompt-context path a resume actually uses, so a future renderer that walks a // map of its own is caught here too. +// +// THE EXPECTED BLOCK IS WRITTEN OUT, NOT TAKEN FROM THE FIRST RENDER. Comparing +// later renders against the first only asks whether the renderer is stable, and +// a renderer that is stably wrong passes: any fixed field order satisfies it, +// including the one this change replaces. It also hid something. The first +// version of this test used tool events, which promptContextEvents filters out +// of the resume context on this branch, so the block being compared was a +// single line and nobody could tell. Naming the expected block makes both +// failures visible. func TestResumeContextBlockIsIdenticalAcrossRenders(t *testing.T) { events := []Event{ {Sequence: 1, Type: EventMessage, Payload: mustOrderPayload(t, map[string]any{"role": "user", "content": "rotate the deploy key"})}, - {Sequence: 2, Type: EventToolCall, Payload: mustOrderPayload(t, map[string]any{"id": "c1", "name": "read_file", "arguments": `{"path":"deploy/prod.env"}`})}, - {Sequence: 3, Type: EventToolResult, Payload: mustOrderPayload(t, map[string]any{"name": "read_file", "status": "ok", "output": "contents"})}, + {Sequence: 2, Type: EventMessage, Payload: mustOrderPayload(t, map[string]any{"role": "assistant", "content": "reading the env file"})}, + {Sequence: 3, Type: EventMessage, Payload: mustOrderPayload(t, map[string]any{"role": "user", "content": "and the hooks"})}, } - first := renderOrderContext(t, events) - if strings.TrimSpace(first) == "" { - t.Fatal("SETUP INVALID: the context block rendered empty, so identical renders prove nothing") - } - for attempt := 1; attempt < 50; attempt++ { - if got := renderOrderContext(t, events); got != first { - t.Fatalf("attempt %d rendered a different context block:\n first: %s\n then: %s", attempt, first, got) + // Keys sorted, so content comes before role in every line. + want := strings.Join([]string{ + "message: rotate the deploy key user", + "message: reading the env file assistant", + "message: and the hooks user", + }, "\n") + + for attempt := range 50 { + if got := renderOrderContext(t, events); got != want { + t.Fatalf("attempt %d rendered:\n%s\nwant:\n%s", attempt, got, want) } } }