From 139cffa6c353fa8c57a4a7ebb70e4b59180cb0b7 Mon Sep 17 00:00:00 2001 From: Vasanthdev2004 Date: Mon, 7 Sep 2026 14:51:46 +0530 Subject: [PATCH 1/9] fix(sessions): carry an interrupted turn's work into the resume prompt A turn that read six files and then died on a provider error left this behind for the next turn: - #1 message: add retries to the http client - #6 error: provider error: upstream timeout Nothing named the files, so the next turn re-read them from scratch (#913). The work was never lost from the session: tool calls and results are recorded as they happen, and the error path carries them out with everything else. promptContextEvents then dropped both types on the way into the prompt, so the record existed and nothing read it. Filtering them was deliberate (#460) and is right for work an answer already describes: forty read_file results add length and no information next to the assistant message explaining them. It is wrong only for work with no answer after it, and that is exactly what an interruption leaves. A turn that ends normally was survivable for that reason, which is why this went unnoticed: the answer was doing the remembering. So the events after the last assistant message come along and the rest stays filtered. The tail gets its own allowance rather than sharing the conversation budget, so a tool-heavy interrupted turn still cannot push an earlier message out of the context. That is the property #460 added the filter for and it is asserted directly. This changes a tested contract. TestFormatExecPromptKeepsConversationMessagesWhenNoisyEventsFollow asserted that tool results are omitted, and its fixture is an interrupted turn: 43 results after an assistant answer with nothing speaking for them. That is the #913 shape, so no rule satisfies both and the contract had to move. The test keeps the guarantee it was written for, that conversation survives a noisy turn, and a counterpart now pins the other half: work an answer already covers is still filtered. Not fixed here, and worth saying plainly: this carries what was DONE, not what was READ. Zero has no conversation-history parameter at all, agent.Run seeds from the system prompt and the user prompt every time, so file contents cannot survive a turn boundary today. The next turn learns it already read a path and can decide, rather than starting blind. --- .../sessions/exec_prompt_tool_context_test.go | 144 ++++++++++++++++++ internal/sessions/exec_session.go | 92 ++++++++++- internal/sessions/store_test.go | 45 +++++- 3 files changed, 271 insertions(+), 10 deletions(-) create mode 100644 internal/sessions/exec_prompt_tool_context_test.go diff --git a/internal/sessions/exec_prompt_tool_context_test.go b/internal/sessions/exec_prompt_tool_context_test.go new file mode 100644 index 000000000..57c828387 --- /dev/null +++ b/internal/sessions/exec_prompt_tool_context_test.go @@ -0,0 +1,144 @@ +package sessions + +import ( + "encoding/json" + "fmt" + "strings" + "testing" +) + +func toolContextPayload(t *testing.T, value map[string]any) json.RawMessage { + t.Helper() + raw, err := json.Marshal(value) + if err != nil { + t.Fatal(err) + } + return json.RawMessage(raw) +} + +// interruptedTurnEvents is a turn that read two files and then died on a +// provider error, which is the shape #913 reports: no assistant answer was ever +// produced, so nothing in the conversation describes the work. +func interruptedTurnEvents(t *testing.T) []Event { + t.Helper() + return []Event{ + {Sequence: 1, Type: EventMessage, Payload: toolContextPayload(t, map[string]any{"role": "user", "content": "add retries to the http client"})}, + {Sequence: 2, Type: EventToolCall, Payload: toolContextPayload(t, map[string]any{"id": "c1", "name": "read_file", "arguments": `{"path":"internal/http/client.go"}`})}, + {Sequence: 3, Type: EventToolResult, Payload: toolContextPayload(t, map[string]any{"name": "read_file", "content": "package http"})}, + {Sequence: 4, Type: EventToolCall, Payload: toolContextPayload(t, map[string]any{"id": "c2", "name": "read_file", "arguments": `{"path":"internal/http/retry.go"}`})}, + {Sequence: 5, Type: EventToolResult, Payload: toolContextPayload(t, map[string]any{"name": "read_file", "content": "var defaultBackoff = 250ms"})}, + {Sequence: 6, Type: EventError, Payload: toolContextPayload(t, map[string]any{"message": "provider error: upstream timeout"})}, + } +} + +func resumePrompt(t *testing.T, events []Event) string { + t.Helper() + return FormatExecPrompt("continue", PreparedExec{ + Mode: ModeResume, + Session: Metadata{SessionID: "s1"}, + ContextEvents: events, + }) +} + +// AN INTERRUPTED TURN HAS TO SAY WHAT IT DID. +// +// A turn that ends normally describes its own work in the assistant's answer, +// so the next turn inherits a prose record. A turn killed by a provider error +// produces no answer, and the tool events that were the only record of the work +// were filtered out of the resume context. The next turn was told a request had +// been made and an error had happened, and nothing else, so it re-read from +// scratch (#913). +func TestResumePromptCarriesInterruptedToolWork(t *testing.T) { + out := resumePrompt(t, interruptedTurnEvents(t)) + + for _, want := range []string{"internal/http/client.go", "internal/http/retry.go"} { + if !strings.Contains(out, want) { + t.Errorf("the resume prompt does not name %s, so the next turn cannot know it was already read:\n%s", want, out) + } + } + // The conversation spine is still there. + for _, want := range []string{"add retries to the http client", "upstream timeout"} { + if !strings.Contains(out, want) { + t.Errorf("the resume prompt lost conversation context %q:\n%s", want, out) + } + } +} + +// AND THE ORDER STAYS THE ORDER IT HAPPENED IN. +// +// The sequence number is rendered into each line, so a list that jumps +// backwards reads as a corrupted history rather than a merge artifact. +func TestResumePromptKeepsEventsInSequenceOrder(t *testing.T) { + out := resumePrompt(t, interruptedTurnEvents(t)) + + last := -1 + for _, line := range strings.Split(out, "\n") { + if !strings.HasPrefix(line, "- #") { + continue + } + var seq int + if _, err := fmt.Sscanf(line, "- #%d", &seq); err != nil { + t.Fatalf("unparsable context line %q", line) + } + if seq <= last { + t.Fatalf("context is out of order at %q (previous #%d):\n%s", line, last, out) + } + last = seq + } + if last < 0 { + t.Fatal("SETUP INVALID: the prompt rendered no context lines") + } +} + +// TOOL EVENTS MUST NOT EVICT CONVERSATION. +// +// This is what the original filter was added for (#460): tool events vastly +// outnumber messages, so admitting them into the same trailing budget would let +// one tool-heavy turn push every earlier message out of the context. A separate +// allowance is what keeps both properties at once. +func TestToolEventsNeverEvictConversation(t *testing.T) { + var events []Event + seq := 0 + next := func() int { seq++; return seq } + // An early message that must survive, then a flood of tool work. + events = append(events, Event{Sequence: next(), Type: EventMessage, Payload: toolContextPayload(t, map[string]any{"role": "user", "content": "EARLIEST-REQUEST-MARKER"})}) + for i := 0; i < 500; i++ { + arguments := fmt.Sprintf(`{"path":"noise/%03d.go"}`, i) + events = append(events, Event{Sequence: next(), Type: EventToolCall, Payload: toolContextPayload(t, map[string]any{"name": "read_file", "arguments": arguments})}) + events = append(events, Event{Sequence: next(), Type: EventToolResult, Payload: toolContextPayload(t, map[string]any{"name": "read_file", "content": "noise"})}) + } + events = append(events, Event{Sequence: next(), Type: EventMessage, Payload: toolContextPayload(t, map[string]any{"role": "user", "content": "LATEST-REQUEST-MARKER"})}) + + out := resumePrompt(t, events) + for _, want := range []string{"EARLIEST-REQUEST-MARKER", "LATEST-REQUEST-MARKER"} { + if !strings.Contains(out, want) { + t.Errorf("a tool-heavy turn evicted conversation event %q, which is the regression the filter exists to prevent", want) + } + } + + selected := promptContextEvents(events) + tools := 0 + for _, event := range selected { + if event.Type == EventToolCall || event.Type == EventToolResult { + tools++ + } + } + if tools == 0 { + t.Error("no tool work survived at all, so an interrupted tool-heavy turn still says nothing about what it did") + } + if len(selected) > 80 { + t.Errorf("selected %d events, over the 80 the prompt budget allows", len(selected)) + } +} + +// A session with no tool work renders exactly as it did before. +func TestResumePromptUnchangedWithoutToolEvents(t *testing.T) { + events := []Event{ + {Sequence: 1, Type: EventMessage, Payload: toolContextPayload(t, map[string]any{"role": "user", "content": "hello"})}, + {Sequence: 2, Type: EventMessage, Payload: toolContextPayload(t, map[string]any{"role": "assistant", "content": "hi"})}, + } + out := resumePrompt(t, events) + if strings.Count(out, "- #") != 2 { + t.Fatalf("a tool-free session rendered %d context lines, want 2:\n%s", strings.Count(out, "- #"), out) + } +} diff --git a/internal/sessions/exec_session.go b/internal/sessions/exec_session.go index b86b364b5..009ff28a9 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" ) @@ -188,23 +189,98 @@ func FormatExecPrompt(prompt string, prepared PreparedExec) string { }, "\n") } +// promptContextEvents chooses what a resumed turn is told about the session so +// far. +// +// A RESUMED TURN NEEDS TO KNOW WHAT THE PREVIOUS ONE DID, NOT ONLY WHAT IT SAID. +// +// Conversation events alone were selected here, so a turn that read six files +// and then died before answering left this behind: +// +// - #1 message: add retries to the http client +// - #6 error: provider error: upstream timeout +// +// Nothing names the files, so the next turn re-reads them from scratch (#913). +// On a turn that ENDS NORMALLY the assistant's answer describes the work, which +// is why this was survivable; an interrupted turn produces no such answer, and +// the record of the work goes with it. +// +// ONLY THE UNSUMMARIZED TAIL, NOT EVERY TOOL EVENT. Filtering tool events out +// was deliberate (#460) and is right for work the assistant has already +// described: forty read_file results add length and no information next to the +// answer that explains them. It is wrong only for work with no answer after it, +// which is precisely what an interrupted turn leaves behind. So the events after +// the last assistant message come along, and everything the assistant already +// spoke for stays filtered. +// +// The tail also gets its own allowance rather than sharing the conversation +// budget, so a tool-heavy interrupted turn can never push an earlier message out +// of the context. That is the property #460 added this filter for and it still +// holds. func promptContextEvents(events []Event) []Event { const maxPromptContextEvents = 80 + // Enough to describe an interrupted turn's work, small enough that the + // conversation still dominates the prompt. + const maxPromptContextTailEvents = 24 - filtered := make([]Event, 0, len(events)) - for _, event := range events { + lastSpoken := -1 + for index, event := range events { + if event.Type == EventMessage && payloadRole(event.Payload) == "assistant" { + lastSpoken = index + } + } + + conversation := make([]Event, 0, len(events)) + tail := make([]Event, 0, maxPromptContextTailEvents) + for index, event := range events { switch event.Type { case EventMessage, EventCompaction, EventSessionFork, EventSessionChild, EventSpecialistStart, EventSpecialistStop, EventError: - filtered = append(filtered, event) + conversation = append(conversation, event) + case EventToolCall, EventToolResult: + if index > lastSpoken { + tail = append(tail, event) + } } } - if len(filtered) == 0 { - filtered = append(filtered, events...) + if len(conversation) == 0 && len(tail) == 0 { + // An unrecognized event stream still says more than nothing. + conversation = append(conversation, events...) + } + if len(conversation) > maxPromptContextEvents { + conversation = conversation[len(conversation)-maxPromptContextEvents:] + } + tailBudget := min(maxPromptContextTailEvents, maxPromptContextEvents-len(conversation)) + if tailBudget < 0 { + tailBudget = 0 + } + if len(tail) > tailBudget { + tail = tail[len(tail)-tailBudget:] } - if len(filtered) > maxPromptContextEvents { - filtered = filtered[len(filtered)-maxPromptContextEvents:] + if len(tail) == 0 { + return conversation + } + // Merged back into execution order: the sequence numbers are rendered, so a + // list that jumps backwards would read as a corrupted history. + merged := make([]Event, 0, len(conversation)+len(tail)) + merged = append(merged, conversation...) + merged = append(merged, tail...) + sort.SliceStable(merged, func(i, j int) bool { return merged[i].Sequence < merged[j].Sequence }) + return merged +} + +// payloadRole reads the "role" field of a message payload, returning "" when the +// payload is not a decodable object or carries no role. +func payloadRole(payload json.RawMessage) string { + if len(payload) == 0 { + return "" + } + var decoded struct { + Role string `json:"role"` + } + if err := json.Unmarshal(payload, &decoded); err != nil { + return "" } - return filtered + return strings.TrimSpace(decoded.Role) } func forkTitle(title string) string { diff --git a/internal/sessions/store_test.go b/internal/sessions/store_test.go index 685cb52dc..07f467759 100644 --- a/internal/sessions/store_test.go +++ b/internal/sessions/store_test.go @@ -610,8 +610,49 @@ func TestFormatExecPromptKeepsConversationMessagesWhenNoisyEventsFollow(t *testi t.Fatalf("expected prompt to contain %q, got %q", want, prompt) } } - if strings.Contains(prompt, "noisy tool result") { - t.Fatalf("expected prompt to omit noisy non-conversation events, got %q", prompt) + // The tool results here follow the last assistant answer, so they are work + // nothing has spoken for yet. They are carried on purpose: that is the whole + // of what an interrupted turn leaves behind (#913). The property this test + // was written for (#460) is that CONVERSATION survives a noisy turn, and the + // assertions above still hold it. + if !strings.Contains(prompt, "noisy tool result") { + t.Fatalf("expected unanswered tool work to be carried, got %q", prompt) + } + if count := strings.Count(prompt, "noisy tool result"); count > 24 { + t.Fatalf("carried %d tool events, over the tail allowance of 24: %q", count, prompt) + } +} + +// AND WORK THE ASSISTANT ALREADY SPOKE FOR STAYS FILTERED. +// +// The counterpart to the case above, and what keeps that one from being +// satisfied by carrying every tool event ever recorded. Once an answer describes +// the work, repeating the raw results adds length and no information. +func TestFormatExecPromptOmitsToolWorkAnAnswerAlreadyCovers(t *testing.T) { + events := []Event{ + {Sequence: 1, Type: EventMessage, Payload: json.RawMessage(`{"role":"user","content":"first user request"}`)}, + } + for sequence := 2; sequence <= 20; sequence++ { + events = append(events, Event{Sequence: sequence, Type: EventToolResult, Payload: json.RawMessage(`{"name":"read_file","output":"already summarized tool result"}`)}) + } + events = append(events, + Event{Sequence: 21, Type: EventMessage, Payload: json.RawMessage(`{"role":"assistant","content":"I read the files and here is the summary"}`)}, + Event{Sequence: 22, Type: EventMessage, Payload: json.RawMessage(`{"role":"user","content":"latest user request"}`)}, + ) + + prompt := FormatExecPrompt("continue", PreparedExec{ + Mode: ModeResume, + Session: Metadata{SessionID: "session-already-answered"}, + ContextEvents: events, + }) + + if strings.Contains(prompt, "already summarized tool result") { + t.Fatalf("tool work the assistant already described was repeated verbatim, got %q", prompt) + } + for _, want := range []string{"first user request", "here is the summary", "latest user request"} { + if !strings.Contains(prompt, want) { + t.Fatalf("expected prompt to contain %q, got %q", want, prompt) + } } } From 1e52a3625dc6dcba064988c50dead3161b32f0c4 Mon Sep 17 00:00:00 2001 From: Vasanthdev2004 Date: Mon, 7 Sep 2026 19:08:29 +0530 Subject: [PATCH 2/9] fix(sessions): carry the tool outcome, not the tool output The tail was carrying whole tool-result events, so up to 500 bytes of raw tool output rode into a later turn prompt. Nothing redacts on the way in, and the prompt renderer truncates rather than filters, so the only thing standing between a file read and a later prompt was a length limit. It bought almost nothing. The tail exists so a resumed turn knows what the interrupted one did, and the CALL already says that: the tool name and its arguments, which is the path for a read. The result body is the one part of the pair that can carry file contents. The status is kept rather than dropping the result entirely. A bare call reads as work that succeeded, so a failed read would come back as a file the next turn believes it already has. Raised by CodeRabbit on the PR, and right. --- .../sessions/exec_prompt_tool_context_test.go | 29 +++++++++++++ internal/sessions/exec_session.go | 42 ++++++++++++++++++- internal/sessions/store_test.go | 17 ++++++-- 3 files changed, 84 insertions(+), 4 deletions(-) diff --git a/internal/sessions/exec_prompt_tool_context_test.go b/internal/sessions/exec_prompt_tool_context_test.go index 57c828387..efe08776b 100644 --- a/internal/sessions/exec_prompt_tool_context_test.go +++ b/internal/sessions/exec_prompt_tool_context_test.go @@ -142,3 +142,32 @@ func TestResumePromptUnchangedWithoutToolEvents(t *testing.T) { t.Fatalf("a tool-free session rendered %d context lines, want 2:\n%s", strings.Count(out, "- #"), out) } } + +// TOOL OUTPUT MUST NOT RIDE ALONG INTO A LATER PROMPT. +// +// The tail is there to say what the interrupted turn did, and the call already +// says that. Carrying the result body would put up to 500 bytes of raw tool +// output into a prompt on a later turn, and nothing redacts on the way in. +func TestResumePromptCarriesToolOutcomeWithoutOutput(t *testing.T) { + secret := "AKIAIOSFODNN7EXAMPLE-and-more-file-contents" + events := []Event{ + {Sequence: 1, Type: EventMessage, Payload: toolContextPayload(t, map[string]any{"role": "user", "content": "look at the config"})}, + {Sequence: 2, Type: EventToolCall, Payload: toolContextPayload(t, map[string]any{"id": "c1", "name": "read_file", "arguments": `{"path":"deploy/prod.env"}`})}, + {Sequence: 3, Type: EventToolResult, Payload: toolContextPayload(t, map[string]any{"name": "read_file", "status": "ok", "output": secret})}, + {Sequence: 4, Type: EventToolCall, Payload: toolContextPayload(t, map[string]any{"id": "c2", "name": "read_file", "arguments": `{"path":"deploy/missing.env"}`})}, + {Sequence: 5, Type: EventToolResult, Payload: toolContextPayload(t, map[string]any{"name": "read_file", "status": "error", "output": "no such file"})}, + {Sequence: 6, Type: EventError, Payload: toolContextPayload(t, map[string]any{"message": "provider error: upstream timeout"})}, + } + out := resumePrompt(t, events) + + if strings.Contains(out, secret) { + t.Errorf("tool output reached the resume prompt:\n%s", out) + } + // What the turn DID still survives: the paths from the calls, and how each + // one ended. + for _, want := range []string{"deploy/prod.env", "deploy/missing.env", "error"} { + if !strings.Contains(out, want) { + t.Errorf("the resume prompt lost %q, so the next turn cannot tell what happened:\n%s", want, out) + } + } +} diff --git a/internal/sessions/exec_session.go b/internal/sessions/exec_session.go index 009ff28a9..361af4fc1 100644 --- a/internal/sessions/exec_session.go +++ b/internal/sessions/exec_session.go @@ -236,10 +236,14 @@ func promptContextEvents(events []Event) []Event { switch event.Type { case EventMessage, EventCompaction, EventSessionFork, EventSessionChild, EventSpecialistStart, EventSpecialistStop, EventError: conversation = append(conversation, event) - case EventToolCall, EventToolResult: + case EventToolCall: if index > lastSpoken { tail = append(tail, event) } + case EventToolResult: + if index > lastSpoken { + tail = append(tail, toolResultOutcome(event)) + } } } if len(conversation) == 0 && len(tail) == 0 { @@ -268,6 +272,42 @@ func promptContextEvents(events []Event) []Event { return merged } +// toolResultOutcome strips a tool result down to WHICH tool ran and HOW IT +// ENDED, dropping the output body. +// +// The tail exists so a resumed turn knows what the interrupted one did, and the +// CALL already carries that: the tool name and its arguments, which is the path +// for a read. The result adds only a 500-byte prefix of the output, which is +// worth little beside the call and is the one part of an event that can carry +// file contents. Nothing redacts on the way into a prompt, so keeping it would +// re-emit raw tool output into a later turn on the strength of a truncation +// limit alone. +// +// The status stays, because dropping it would be worse than dropping the whole +// result: a bare call reads as work that succeeded, so a failed read would come +// back as a file the next turn believes it already has. +func toolResultOutcome(event Event) Event { + var decoded struct { + Name string `json:"name"` + Status string `json:"status"` + } + if err := json.Unmarshal(event.Payload, &decoded); err != nil { + // Undecodable: drop the payload rather than pass an unknown shape through. + event.Payload = json.RawMessage(`{}`) + return event + } + trimmed, err := json.Marshal(map[string]string{ + "name": decoded.Name, + "status": decoded.Status, + }) + if err != nil { + event.Payload = json.RawMessage(`{}`) + return event + } + event.Payload = json.RawMessage(trimmed) + return event +} + // payloadRole reads the "role" field of a message payload, returning "" when the // payload is not a decodable object or carries no role. func payloadRole(payload json.RawMessage) string { diff --git a/internal/sessions/store_test.go b/internal/sessions/store_test.go index 07f467759..ee83f57e6 100644 --- a/internal/sessions/store_test.go +++ b/internal/sessions/store_test.go @@ -615,10 +615,18 @@ func TestFormatExecPromptKeepsConversationMessagesWhenNoisyEventsFollow(t *testi // of what an interrupted turn leaves behind (#913). The property this test // was written for (#460) is that CONVERSATION survives a noisy turn, and the // assertions above still hold it. - if !strings.Contains(prompt, "noisy tool result") { - t.Fatalf("expected unanswered tool work to be carried, got %q", prompt) + // The tool results here follow the last assistant answer, so they are work + // nothing has spoken for yet, and an interrupted turn leaves exactly this + // behind (#913). Their IDENTITY is carried; their OUTPUT is not, because the + // call beside them already names what was touched and the body is the part + // that can carry file contents into a later prompt. + if !strings.Contains(prompt, "tool_result: read_file") { + t.Fatalf("expected unanswered tool work to be named, got %q", prompt) + } + if strings.Contains(prompt, "noisy tool result") { + t.Fatalf("tool output body reached the prompt, got %q", prompt) } - if count := strings.Count(prompt, "noisy tool result"); count > 24 { + if count := strings.Count(prompt, "tool_result:"); count > 24 { t.Fatalf("carried %d tool events, over the tail allowance of 24: %q", count, prompt) } } @@ -649,6 +657,9 @@ func TestFormatExecPromptOmitsToolWorkAnAnswerAlreadyCovers(t *testing.T) { if strings.Contains(prompt, "already summarized tool result") { t.Fatalf("tool work the assistant already described was repeated verbatim, got %q", prompt) } + if strings.Contains(prompt, "tool_result:") { + t.Fatalf("an answered turn still listed its tool events, got %q", prompt) + } for _, want := range []string{"first user request", "here is the summary", "latest user request"} { if !strings.Contains(prompt, want) { t.Fatalf("expected prompt to contain %q, got %q", want, prompt) From 23b1e5f92fc55a100a76131774b9c525e9746ae3 Mon Sep 17 00:00:00 2001 From: Vasanthdev2004 Date: Mon, 7 Sep 2026 19:10:14 +0530 Subject: [PATCH 3/9] test(sessions): scope the outcome assertion to the tool_result lines The status check searched the whole prompt for "error", which the provider error message in the same fixture also contains. Blanking the status out left the test passing, so it was asserting the presence of an unrelated line. Scoped to the tool_result lines, it fails on that mutation naming the outcome it lost. --- .../sessions/exec_prompt_tool_context_test.go | 22 ++++++++++++++++--- 1 file changed, 19 insertions(+), 3 deletions(-) diff --git a/internal/sessions/exec_prompt_tool_context_test.go b/internal/sessions/exec_prompt_tool_context_test.go index efe08776b..e2626a22b 100644 --- a/internal/sessions/exec_prompt_tool_context_test.go +++ b/internal/sessions/exec_prompt_tool_context_test.go @@ -163,11 +163,27 @@ func TestResumePromptCarriesToolOutcomeWithoutOutput(t *testing.T) { if strings.Contains(out, secret) { t.Errorf("tool output reached the resume prompt:\n%s", out) } - // What the turn DID still survives: the paths from the calls, and how each - // one ended. - for _, want := range []string{"deploy/prod.env", "deploy/missing.env", "error"} { + // What the turn DID still survives: the paths, from the calls. + for _, want := range []string{"deploy/prod.env", "deploy/missing.env"} { if !strings.Contains(out, want) { t.Errorf("the resume prompt lost %q, so the next turn cannot tell what happened:\n%s", want, out) } } + // And HOW EACH ONE ENDED, asserted only against the tool_result lines. The + // prompt also carries a provider error message, so an unscoped search for + // "error" passes whether or not the status survived. It did pass with the + // status blanked out, which is why this is scoped. + var resultLines []string + for _, line := range strings.Split(out, "\n") { + if strings.Contains(line, "tool_result:") { + resultLines = append(resultLines, line) + } + } + if len(resultLines) != 2 { + t.Fatalf("SETUP INVALID: %d tool_result lines, want 2:\n%s", len(resultLines), out) + } + joined := strings.Join(resultLines, "\n") + if !strings.Contains(joined, "ok") || !strings.Contains(joined, "error") { + t.Errorf("the tool_result lines lost their outcome, so a failed read reads as a file the next turn already has:\n%s", joined) + } } From dab4691b21f92d74d4526a1cabc006d1062fa858 Mon Sep 17 00:00:00 2001 From: Vasanthdev2004 Date: Mon, 7 Sep 2026 20:23:05 +0530 Subject: [PATCH 4/9] test(sessions): pin the whole of every tool line, not a search for one string Two assertions here were weaker than what they claimed. Searching the prompt for the fixture's whole secret only rejects that exact string, so a change that carried a truncated or partly redacted prefix of the output into the prompt would have passed. And asking whether "ok" and "error" each appear somewhere among the result lines passes just as well with the two statuses swapped, or both hung off the wrong call. Both tool calls and both results are now pinned outright, which says what the trim actually promises: nothing but the tool name and the outcome survives a result, and the line order pairs each outcome with its own call. Compared as a set of fields per line rather than as a string, because the renderer walks the payload map and Go randomizes that order, so the same events come out in a different field order on every run. Reported by CodeRabbit on this PR. --- .../sessions/exec_prompt_tool_context_test.go | 80 ++++++++++++++----- 1 file changed, 59 insertions(+), 21 deletions(-) diff --git a/internal/sessions/exec_prompt_tool_context_test.go b/internal/sessions/exec_prompt_tool_context_test.go index e2626a22b..ad9e5a401 100644 --- a/internal/sessions/exec_prompt_tool_context_test.go +++ b/internal/sessions/exec_prompt_tool_context_test.go @@ -3,6 +3,7 @@ package sessions import ( "encoding/json" "fmt" + "slices" "strings" "testing" ) @@ -160,30 +161,67 @@ func TestResumePromptCarriesToolOutcomeWithoutOutput(t *testing.T) { } out := resumePrompt(t, events) - if strings.Contains(out, secret) { - t.Errorf("tool output reached the resume prompt:\n%s", out) - } - // What the turn DID still survives: the paths, from the calls. - for _, want := range []string{"deploy/prod.env", "deploy/missing.env"} { - if !strings.Contains(out, want) { - t.Errorf("the resume prompt lost %q, so the next turn cannot tell what happened:\n%s", want, out) - } - } - // And HOW EACH ONE ENDED, asserted only against the tool_result lines. The - // prompt also carries a provider error message, so an unscoped search for - // "error" passes whether or not the status survived. It did pass with the - // status blanked out, which is why this is scoped. - var resultLines []string + // THE TOOL LINES ARE PINNED WHOLE, NOT SEARCHED. + // + // Two reasons the obvious assertions are too weak to say what the trim + // promises. Searching the prompt for the fixture's full secret only rejects + // that exact string, so a change that carried a truncated or partly + // redacted prefix of the output would pass it. And asking whether "ok" and + // "error" each appear somewhere among the result lines passes just as well + // when the two statuses are swapped, or both attached to the wrong call. + // + // Pinning the whole of every tool line covers both: nothing but the tool + // name and the outcome survives a result, each outcome sits on its own + // line, and the line order pairs each one with the call above it. + // + // Compared as a set of fields per line rather than as a string, because the + // renderer walks the payload map and Go randomizes that order, so the same + // events render their fields in a different order on every run. + want := []struct { + sequence int + kind string + fields []string + }{ + {2, "tool_call", []string{"c1", "read_file", `{"path":"deploy/prod.env"}`}}, + {3, "tool_result", []string{"read_file", "ok"}}, + {4, "tool_call", []string{"c2", "read_file", `{"path":"deploy/missing.env"}`}}, + {5, "tool_result", []string{"read_file", "error"}}, + } + var toolLines []string for _, line := range strings.Split(out, "\n") { - if strings.Contains(line, "tool_result:") { - resultLines = append(resultLines, line) + if strings.HasPrefix(line, "- #") && (strings.Contains(line, "tool_call:") || strings.Contains(line, "tool_result:")) { + toolLines = append(toolLines, line) } } - if len(resultLines) != 2 { - t.Fatalf("SETUP INVALID: %d tool_result lines, want 2:\n%s", len(resultLines), out) + if len(toolLines) != len(want) { + t.Fatalf("the resume prompt carries %d tool lines, want %d:\n%s", len(toolLines), len(want), out) } - joined := strings.Join(resultLines, "\n") - if !strings.Contains(joined, "ok") || !strings.Contains(joined, "error") { - t.Errorf("the tool_result lines lost their outcome, so a failed read reads as a file the next turn already has:\n%s", joined) + for index, line := range toolLines { + var sequence int + var kind string + rest, found := strings.CutPrefix(line, "- #") + if !found { + t.Fatalf("unparsable context line %q", line) + } + if _, err := fmt.Sscanf(rest, "%d %s", &sequence, &kind); err != nil { + t.Fatalf("unparsable context line %q: %v", line, err) + } + kind = strings.TrimSuffix(kind, ":") + _, payload, _ := strings.Cut(rest, ": ") + fields := strings.Fields(payload) + slices.Sort(fields) + expected := slices.Clone(want[index].fields) + slices.Sort(expected) + if sequence != want[index].sequence || kind != want[index].kind || !slices.Equal(fields, expected) { + t.Errorf("tool line %d is not what the trim promises.\ngot: #%d %s %v\nwant: #%d %s %v\nfull prompt:\n%s", + index, sequence, kind, fields, want[index].sequence, want[index].kind, expected, out) + } + } + // Named on its own so a leak reports as a leak rather than as the + // formatting drift the comparison above would also catch. A PREFIX, because + // the danger is output reaching a later prompt at all, not this exact + // fixture string reaching it. + if strings.Contains(out, secret[:16]) { + t.Errorf("tool output reached the resume prompt:\n%s", out) } } From c35412bd018662938a88a425736a1c8637eb161c Mon Sep 17 00:00:00 2001 From: Vasanthdev2004 Date: Tue, 8 Sep 2026 08:48:27 +0530 Subject: [PATCH 5/9] fix(sessions): keep a tool call's identity and drop its payload in resume context toolResultOutcome strips a result down to name and status, and calls were left verbatim, so an interrupted write_file put its content into the next turn's prompt while the matching result body did not. Same secret, one door left open. Admitting calls is the #913 fix and has to stay: a resumed turn knowing which file was read is the whole point. What had to change is the symmetry. toolCallIdentity is the call-side half of toolResultOutcome. It decodes the arguments, keeps the fields that say what a call was about, and drops the ones that carry what it was about to write, run or search for. Paths, directories, urls, names, patterns and a read window survive; write_file's content, edit_file's old and new strings, apply_patch's hunks and a shell command line do not. An allow-list rather than a deny-list, so an argument this file has never seen is dropped rather than replayed. A new tool with a new body field then shows up as a missing path in a resume prompt, which is visible, instead of a new leak, which is not. Keyed by argument name rather than tool name, so an MCP tool whose argument is a url keeps it without being enumerated. Arguments that do not decode as an object are removed outright: text that could not be read is text that cannot be checked. The tail budget gets a comment rather than a change. Tail slots exist only when the conversation uses fewer than the 80-event cap, so a session already at the cap carries no interrupted tool work, which is what every session did before tool events were admitted and what the #460 cap is there to hold. Reserving a minimum tail by trimming conversation further is a product decision, and it is not this one. Reported by jatmn. --- .../exec_prompt_tool_call_identity_test.go | 153 ++++++++++++++++++ internal/sessions/exec_session.go | 84 +++++++++- 2 files changed, 236 insertions(+), 1 deletion(-) create mode 100644 internal/sessions/exec_prompt_tool_call_identity_test.go diff --git a/internal/sessions/exec_prompt_tool_call_identity_test.go b/internal/sessions/exec_prompt_tool_call_identity_test.go new file mode 100644 index 000000000..0862ac8b9 --- /dev/null +++ b/internal/sessions/exec_prompt_tool_call_identity_test.go @@ -0,0 +1,153 @@ +package sessions + +import ( + "fmt" + "strings" + "testing" +) + +// A CALL'S PAYLOAD MUST NOT RIDE INTO A LATER PROMPT ANY MORE THAN A RESULT'S. +// +// toolResultOutcome drops result bodies, and calls were left verbatim, so an +// interrupted write_file put its content into the next turn's prompt while the +// matching result body did not. Same secret, one door left open. The identity +// still has to survive, since a resumed turn knowing WHICH file was written is +// the whole point of admitting calls at all. +// +// Pinned as whole lines, compared as fields, for the same reason the result test +// is: a substring search for one fixture secret passes when a truncated or +// reworded copy leaks, and the renderer walks a map so field order varies. +func TestResumePromptCarriesToolCallIdentityWithoutPayload(t *testing.T) { + const secret = "AWS_SECRET_ACCESS_KEY=wJalrXUtnFEMI/K7MDENG" + const token = "Bearer sk-live-4eC39HqLyjWDarjtT1zdp7dc" + events := []Event{ + {Sequence: 1, Type: EventMessage, Payload: toolContextPayload(t, map[string]any{"role": "user", "content": "rotate the deploy key"})}, + {Sequence: 2, Type: EventToolCall, Payload: toolContextPayload(t, map[string]any{"id": "c1", "name": "write_file", "arguments": `{"path":"deploy/prod.env","content":"` + secret + `"}`})}, + {Sequence: 3, Type: EventToolResult, Payload: toolContextPayload(t, map[string]any{"name": "write_file", "status": "ok", "output": "written"})}, + {Sequence: 4, Type: EventToolCall, Payload: toolContextPayload(t, map[string]any{"id": "c2", "name": "edit_file", "arguments": `{"path":"deploy/prod.env","old_string":"` + secret + `","new_string":"rotated"}`})}, + {Sequence: 5, Type: EventToolResult, Payload: toolContextPayload(t, map[string]any{"name": "edit_file", "status": "ok"})}, + {Sequence: 6, Type: EventToolCall, Payload: toolContextPayload(t, map[string]any{"id": "c3", "name": "exec_command", "arguments": `{"cmd":"curl -H '` + token + `' https://api.example/rotate","workdir":"deploy"}`})}, + {Sequence: 7, Type: EventToolResult, Payload: toolContextPayload(t, map[string]any{"name": "exec_command", "status": "ok"})}, + {Sequence: 8, Type: EventToolCall, Payload: toolContextPayload(t, map[string]any{"id": "c4", "name": "apply_patch", "arguments": `{"patch":"*** Begin Patch\n*** Update File: deploy/prod.env\n-` + secret + `\n+rotated\n*** End Patch"}`})}, + {Sequence: 9, Type: EventToolResult, Payload: toolContextPayload(t, map[string]any{"name": "apply_patch", "status": "ok"})}, + {Sequence: 10, Type: EventToolCall, Payload: toolContextPayload(t, map[string]any{"id": "c5", "name": "read_file", "arguments": `{"path":"deploy/prod.env","offset":1,"limit":40}`})}, + {Sequence: 11, Type: EventToolResult, Payload: toolContextPayload(t, map[string]any{"name": "read_file", "status": "ok", "output": secret})}, + {Sequence: 12, Type: EventError, Payload: toolContextPayload(t, map[string]any{"message": "provider error: upstream timeout"})}, + } + out := resumePrompt(t, events) + + // The payloads: none of them, in any form. + for _, leaked := range []string{secret, secret[:16], token, token[:14], "rotated", "Begin Patch", "curl -H"} { + if strings.Contains(out, leaked) { + t.Errorf("tool call payload %q reached the resume prompt:\n%s", leaked, out) + } + } + + // The identities: every one of them, from the call side. + want := map[int]struct { + kind string + fields []string + }{ + 2: {"tool_call", []string{"c1", "write_file", `{"path":"deploy/prod.env"}`}}, + 4: {"tool_call", []string{"c2", "edit_file", `{"path":"deploy/prod.env"}`}}, + 6: {"tool_call", []string{"c3", "exec_command", `{"workdir":"deploy"}`}}, + 8: {"tool_call", []string{"c4", "apply_patch"}}, + 10: {"tool_call", []string{"c5", "read_file"}}, + } + seen := map[int]bool{} + for _, line := range strings.Split(out, "\n") { + if !strings.HasPrefix(line, "- #") || !strings.Contains(line, "tool_call:") { + continue + } + var sequence int + var kind string + rest, _ := strings.CutPrefix(line, "- #") + if _, err := fmt.Sscanf(rest, "%d %s", &sequence, &kind); err != nil { + t.Fatalf("unparsable context line %q: %v", line, err) + } + expected, wanted := want[sequence] + if !wanted { + t.Errorf("unexpected tool_call line %q", line) + continue + } + seen[sequence] = true + _, payload, _ := strings.Cut(rest, ": ") + fields := strings.Fields(payload) + for _, field := range expected.fields { + if !containsField(fields, field) { + t.Errorf("line #%d lost identity field %q: %q", sequence, field, line) + } + } + } + for sequence := range want { + if !seen[sequence] { + t.Errorf("tool_call #%d did not reach the resume prompt at all, so the interrupted work is invisible again", sequence) + } + } + // read_file keeps its window too, or a resumed turn re-reads a part it had. + if !strings.Contains(out, `"offset":1`) || !strings.Contains(out, `"limit":40`) { + t.Errorf("read_file lost its offset/limit window:\n%s", out) + } +} + +func containsField(fields []string, want string) bool { + for _, field := range fields { + if field == want { + return true + } + } + // The reduced arguments object may carry more than one identity key, in map + // order, so a single-key expectation is also satisfied by an object that + // contains that key. + if strings.HasPrefix(want, "{") { + key := strings.TrimSuffix(strings.TrimPrefix(want, "{"), "}") + for _, field := range fields { + if strings.HasPrefix(field, "{") && strings.Contains(field, key) { + return true + } + } + } + return false +} + +// ARGUMENTS THAT CANNOT BE READ ARE NOT PASSED THROUGH AS TEXT. +// +// An allow-list only protects what it can see. Arguments that do not decode as +// an object are dropped rather than rendered, since text this file could not +// inspect is text it cannot vouch for. +func TestResumePromptDropsUnreadableToolCallArguments(t *testing.T) { + const secret = "sk-live-unparseable-9f8e7d6c" + events := []Event{ + {Sequence: 1, Type: EventMessage, Payload: toolContextPayload(t, map[string]any{"role": "user", "content": "go"})}, + {Sequence: 2, Type: EventToolCall, Payload: toolContextPayload(t, map[string]any{"id": "c1", "name": "bash", "arguments": `not json at all ` + secret})}, + {Sequence: 3, Type: EventError, Payload: toolContextPayload(t, map[string]any{"message": "provider error"})}, + } + out := resumePrompt(t, events) + if strings.Contains(out, secret) { + t.Errorf("unparseable arguments were rendered as text:\n%s", out) + } + if !strings.Contains(out, "bash") { + t.Errorf("the call's identity was lost along with its unreadable arguments:\n%s", out) + } +} + +// AND A TOOL THIS FILE HAS NEVER HEARD OF STILL KEEPS ITS PATH. +// +// The allow-list is by key, not by tool name, so an MCP tool or a future core +// tool whose argument is a path or a url carries it into the resume prompt +// without being enumerated here, while any body field it has is dropped. +func TestResumePromptKeepsIdentityForUnknownTools(t *testing.T) { + const body = "large opaque payload that must not be replayed" + events := []Event{ + {Sequence: 1, Type: EventMessage, Payload: toolContextPayload(t, map[string]any{"role": "user", "content": "go"})}, + {Sequence: 2, Type: EventToolCall, Payload: toolContextPayload(t, map[string]any{"id": "c1", "name": "mcp_uploader", "arguments": `{"url":"https://files.example/x","blob":"` + body + `"}`})}, + {Sequence: 3, Type: EventError, Payload: toolContextPayload(t, map[string]any{"message": "provider error"})}, + } + out := resumePrompt(t, events) + if strings.Contains(out, body) { + t.Errorf("an unlisted body field was replayed:\n%s", out) + } + if !strings.Contains(out, "https://files.example/x") { + t.Errorf("an unlisted tool lost its url identity:\n%s", out) + } +} diff --git a/internal/sessions/exec_session.go b/internal/sessions/exec_session.go index 361af4fc1..c2e111cc5 100644 --- a/internal/sessions/exec_session.go +++ b/internal/sessions/exec_session.go @@ -238,7 +238,7 @@ func promptContextEvents(events []Event) []Event { conversation = append(conversation, event) case EventToolCall: if index > lastSpoken { - tail = append(tail, event) + tail = append(tail, toolCallIdentity(event)) } case EventToolResult: if index > lastSpoken { @@ -253,6 +253,11 @@ func promptContextEvents(events []Event) []Event { if len(conversation) > maxPromptContextEvents { conversation = conversation[len(conversation)-maxPromptContextEvents:] } + // Tail slots exist only when the conversation uses fewer than the 80-event + // cap. A session whose conversation already fills it carries no interrupted + // tool work, which is what every session did before tool events were admitted + // at all, and what the #460 cap is there to hold. Reserving a minimum tail by + // trimming conversation further is a product decision, not this change. tailBudget := min(maxPromptContextTailEvents, maxPromptContextEvents-len(conversation)) if tailBudget < 0 { tailBudget = 0 @@ -272,6 +277,83 @@ func promptContextEvents(events []Event) []Event { return merged } +// toolCallIdentityKeys are the argument fields that say WHAT a call was about +// without carrying what it was about to write, run, or search for. +// +// Path, directory, URL, name and pattern fields identify the work: the file that +// was read, the tree that was listed, the expression that was searched. Body +// fields carry payload: write_file's content, edit_file's old and new strings, +// apply_patch's hunks, a shell command with whatever credential was on its +// line. The result side already drops payload through toolResultOutcome, and +// admitting calls into resume context without the same care put an interrupted +// write_file's content into the next turn's prompt while its result body did +// not. Same fact, one door left open. +// +// AN ALLOW-LIST, NOT A DENY-LIST, so an argument this file has never heard of is +// dropped rather than replayed. A new tool with a new body field is then a +// missing path in a resume prompt, which is visible, instead of a new leak, which +// is not. The keys cover every alias the tools accept for the identity fields. +var toolCallIdentityKeys = map[string]bool{ + // files and directories + "path": true, "file": true, "file_path": true, "filepath": true, "filename": true, + "dir": true, "directory": true, "cwd": true, "workdir": true, + // what a search was for; these are what the interrupted turn was looking at + "pattern": true, "glob": true, "query": true, "regex": true, "expression": true, + // fetches and named resources + "url": true, "name": true, + // read windows, so a resumed turn knows which part it already had + "offset": true, "limit": true, +} + +// toolCallIdentity keeps a tool call's identity and drops its payload, the +// symmetric half of toolResultOutcome. +// +// The arguments travel as a JSON string inside the payload. They are decoded, +// reduced to the identity keys, and re-encoded; anything that does not decode as +// an object is removed outright rather than passed through as text, since text +// that could not be read is text that cannot be checked. +func toolCallIdentity(event Event) Event { + var decoded map[string]json.RawMessage + if err := json.Unmarshal(event.Payload, &decoded); err != nil { + event.Payload = json.RawMessage(`{}`) + return event + } + raw, present := decoded["arguments"] + if !present { + return event + } + kept := map[string]any{} + var argumentsText string + if err := json.Unmarshal(raw, &argumentsText); err == nil { + var arguments map[string]any + if err := json.Unmarshal([]byte(argumentsText), &arguments); err == nil { + for key, value := range arguments { + if toolCallIdentityKeys[strings.ToLower(key)] { + kept[key] = value + } + } + } + } + if len(kept) == 0 { + delete(decoded, "arguments") + } else { + reduced, err := json.Marshal(kept) + if err != nil { + delete(decoded, "arguments") + } else { + quoted, _ := json.Marshal(string(reduced)) + decoded["arguments"] = quoted + } + } + rebuilt, err := json.Marshal(decoded) + if err != nil { + event.Payload = json.RawMessage(`{}`) + return event + } + event.Payload = json.RawMessage(rebuilt) + return event +} + // toolResultOutcome strips a tool result down to WHICH tool ran and HOW IT // ENDED, dropping the output body. // From e27174f6f471f7e4d6cca7a54b2ee27d4062934b Mon Sep 17 00:00:00 2001 From: Vasanthdev2004 Date: Wed, 9 Sep 2026 11:17:15 +0530 Subject: [PATCH 6/9] fix(sessions): scrub credentials out of retained tool-call values An allow-listed key is not a safe value. web_fetch accepts a credential in the query string and redacts the URL it reports back, so an interrupted fetch had its token dropped from the result and kept verbatim in the call, and this projection then carried it into the next resume or fork prompt. Retained string values now go through the same redaction web_fetch uses on its own returned URL: host, path and ordinary query survive, the token does not, and non-string values are untouched. --- .../exec_prompt_tool_call_identity_test.go | 52 +++++++++++++++++++ internal/sessions/exec_session.go | 23 +++++++- 2 files changed, 74 insertions(+), 1 deletion(-) diff --git a/internal/sessions/exec_prompt_tool_call_identity_test.go b/internal/sessions/exec_prompt_tool_call_identity_test.go index 0862ac8b9..5ed435b46 100644 --- a/internal/sessions/exec_prompt_tool_call_identity_test.go +++ b/internal/sessions/exec_prompt_tool_call_identity_test.go @@ -151,3 +151,55 @@ func TestResumePromptKeepsIdentityForUnknownTools(t *testing.T) { t.Errorf("an unlisted tool lost its url identity:\n%s", out) } } + +// AND AN ALLOW-LISTED KEY IS NOT A SAFE VALUE. +// +// web_fetch accepts a credential in the query string and redacts the URL it +// reports back, so an interrupted fetch had its token dropped from the result +// and kept verbatim in the call. This projection is what admits the call into a +// later turn's prompt, so the token rode into the next resume or fork. The +// identity the resumed turn actually needs is the host and path, and those +// survive. +func TestResumePromptRedactsCredentialsInsideRetainedValues(t *testing.T) { + const token = "secret-value-9f8e7d6c5b4a" + events := []Event{ + {Sequence: 1, Type: EventMessage, Payload: toolContextPayload(t, map[string]any{"role": "user", "content": "check the feed"})}, + {Sequence: 2, Type: EventToolCall, Payload: toolContextPayload(t, map[string]any{"id": "c1", "name": "web_fetch", "arguments": `{"url":"https://api.example/data?access_token=` + token + `&page=2"}`})}, + {Sequence: 3, Type: EventToolCall, Payload: toolContextPayload(t, map[string]any{"id": "c2", "name": "web_fetch", "arguments": `{"url":"https://reader:hunter2@api.example/feed"}`})}, + {Sequence: 4, Type: EventError, Payload: toolContextPayload(t, map[string]any{"message": "provider error: upstream timeout"})}, + } + out := resumePrompt(t, events) + + for _, leaked := range []string{token, token[:12], "hunter2"} { + if strings.Contains(out, leaked) { + t.Errorf("credential %q reached the resume prompt:\n%s", leaked, out) + } + } + // The non-secret identity is the whole reason calls are admitted at all. + for _, kept := range []string{"api.example", "/data", "page=2", "/feed"} { + if !strings.Contains(out, kept) { + t.Errorf("URL identity %q was lost to the scrub:\n%s", kept, out) + } + } +} + +// The scrub must not eat ordinary identities. A path, a glob and a query are +// what the resumed turn navigates by, and none of them is a credential. +func TestResumePromptKeepsOrdinaryIdentitiesThroughTheScrub(t *testing.T) { + events := []Event{ + {Sequence: 1, Type: EventMessage, Payload: toolContextPayload(t, map[string]any{"role": "user", "content": "go"})}, + {Sequence: 2, Type: EventToolCall, Payload: toolContextPayload(t, map[string]any{"id": "c1", "name": "read_file", "arguments": `{"path":"internal/sessions/exec_session.go","offset":1,"limit":40}`})}, + {Sequence: 3, Type: EventToolCall, Payload: toolContextPayload(t, map[string]any{"id": "c2", "name": "grep", "arguments": `{"pattern":"func toolCallIdentity","glob":"src/**/*.tsx"}`})}, + {Sequence: 4, Type: EventError, Payload: toolContextPayload(t, map[string]any{"message": "provider error"})}, + } + out := resumePrompt(t, events) + for _, kept := range []string{"internal/sessions/exec_session.go", "func toolCallIdentity", "src/**/*.tsx"} { + if !strings.Contains(out, kept) { + t.Errorf("identity %q was lost to the scrub:\n%s", kept, out) + } + } + // Numbers are not strings and are nothing to scrub; they must survive whole. + if !strings.Contains(out, "40") { + t.Errorf("a numeric read window did not survive the scrub:\n%s", out) + } +} diff --git a/internal/sessions/exec_session.go b/internal/sessions/exec_session.go index c2e111cc5..8bdf59559 100644 --- a/internal/sessions/exec_session.go +++ b/internal/sessions/exec_session.go @@ -7,6 +7,8 @@ import ( "sort" "strings" "unicode/utf8" + + "github.com/Gitlawb/zero/internal/redaction" ) type ExecMode string @@ -305,6 +307,25 @@ var toolCallIdentityKeys = map[string]bool{ "offset": true, "limit": true, } +// redactedIdentityValue scrubs credentials out of a value the projection keeps. +// +// AN ALLOW-LISTED KEY IS NOT A SAFE VALUE. A url is a valid place for a +// credential to appear: web_fetch accepts a query token and redacts the URL it +// reports back, so an interrupted fetch of +// https://api.example/data?access_token=... had its token dropped from the +// result and kept verbatim in the call. This projection is what admits call +// arguments into a later turn's prompt, so the same scrub belongs here or the +// credential is replayed on resume. Host and path survive it, which is the +// identity the resumed turn needs; non-string values (an offset, a limit) are +// nothing to scrub. +func redactedIdentityValue(value any) any { + text, ok := value.(string) + if !ok { + return value + } + return redaction.RedactString(text, redaction.Options{}) +} + // toolCallIdentity keeps a tool call's identity and drops its payload, the // symmetric half of toolResultOutcome. // @@ -329,7 +350,7 @@ func toolCallIdentity(event Event) Event { if err := json.Unmarshal([]byte(argumentsText), &arguments); err == nil { for key, value := range arguments { if toolCallIdentityKeys[strings.ToLower(key)] { - kept[key] = value + kept[key] = redactedIdentityValue(value) } } } From a67ea8735ac5bf69923862cbdd6a03bec2746c98 Mon Sep 17 00:00:00 2001 From: Vasanthdev2004 Date: Wed, 9 Sep 2026 11:50:04 +0530 Subject: [PATCH 7/9] fix(sessions): project a tool call's top-level fields instead of patching one toolResultOutcome builds its output from the fields it keeps, so a field it has never heard of cannot reach a prompt through it. The call side edited arguments in place and returned every sibling key, and returned the payload untouched when there were no arguments at all, so a producer recording one more top-level field would put it into the next turn. Both halves now name what survives and drop the rest. --- .../exec_prompt_tool_call_identity_test.go | 37 +++++++++++++ internal/sessions/exec_session.go | 54 ++++++++++++------- 2 files changed, 71 insertions(+), 20 deletions(-) diff --git a/internal/sessions/exec_prompt_tool_call_identity_test.go b/internal/sessions/exec_prompt_tool_call_identity_test.go index 5ed435b46..eb4bac555 100644 --- a/internal/sessions/exec_prompt_tool_call_identity_test.go +++ b/internal/sessions/exec_prompt_tool_call_identity_test.go @@ -203,3 +203,40 @@ func TestResumePromptKeepsOrdinaryIdentitiesThroughTheScrub(t *testing.T) { t.Errorf("a numeric read window did not survive the scrub:\n%s", out) } } + +// AND A FIELD THIS FILE HAS NEVER HEARD OF DOES NOT RIDE ALONG. +// +// toolResultOutcome builds its output from the fields it keeps, so an unknown +// one cannot reach a prompt through it. The call side edited arguments in place +// and returned every sibling key, which is the opposite contract: a producer +// recording one more top-level field, or the same payload with no arguments at +// all, put that field straight into the next turn. The projection names what +// survives, so both halves now drop what they do not name. +func TestResumePromptProjectsTopLevelToolCallFields(t *testing.T) { + const body = "opaque top-level payload that must not be replayed" + events := []Event{ + {Sequence: 1, Type: EventMessage, Payload: toolContextPayload(t, map[string]any{"role": "user", "content": "go"})}, + {Sequence: 2, Type: EventToolCall, Payload: toolContextPayload(t, map[string]any{ + "id": "c1", "name": "write_file", "arguments": `{"path":"deploy/prod.env"}`, "rawInput": body, + })}, + // The same shape with no arguments at all, which used to return the whole + // payload untouched. + {Sequence: 3, Type: EventToolCall, Payload: toolContextPayload(t, map[string]any{ + "id": "c2", "name": "read_file", "stashedContent": body, + })}, + {Sequence: 4, Type: EventError, Payload: toolContextPayload(t, map[string]any{"message": "provider error"})}, + } + out := resumePrompt(t, events) + + for _, leaked := range []string{body, "rawInput", "stashedContent"} { + if strings.Contains(out, leaked) { + t.Errorf("an unnamed top-level field %q reached the resume prompt:\n%s", leaked, out) + } + } + // The identity itself still survives, from both calls. + for _, kept := range []string{"c1", "write_file", "deploy/prod.env", "c2", "read_file"} { + if !strings.Contains(out, kept) { + t.Errorf("call identity %q was lost to the projection:\n%s", kept, out) + } + } +} diff --git a/internal/sessions/exec_session.go b/internal/sessions/exec_session.go index 8bdf59559..bf7fc0ec9 100644 --- a/internal/sessions/exec_session.go +++ b/internal/sessions/exec_session.go @@ -326,6 +326,12 @@ func redactedIdentityValue(value any) any { return redaction.RedactString(text, redaction.Options{}) } +// toolCallIdentityFields are the top-level payload fields a projected call +// keeps: which call it was, and which tool. Everything else on the payload is +// dropped, including a field added by a future producer that this file has +// never seen. +var toolCallIdentityFields = []string{"id", "name"} + // toolCallIdentity keeps a tool call's identity and drops its payload, the // symmetric half of toolResultOutcome. // @@ -333,40 +339,48 @@ func redactedIdentityValue(value any) any { // reduced to the identity keys, and re-encoded; anything that does not decode as // an object is removed outright rather than passed through as text, since text // that could not be read is text that cannot be checked. +// +// REBUILT, NOT PATCHED, WHICH IS WHAT MAKES IT THE SYMMETRIC HALF. +// toolResultOutcome constructs its output from the two fields it keeps, so a +// field it has never heard of cannot reach a prompt through it. Editing +// arguments in place and returning the rest of the payload is the opposite +// contract: it drops what it recognises as a body and forwards every sibling +// key, so a producer recording one more top-level field would put that field +// into the next turn. The projection names what survives. func toolCallIdentity(event Event) Event { var decoded map[string]json.RawMessage if err := json.Unmarshal(event.Payload, &decoded); err != nil { event.Payload = json.RawMessage(`{}`) return event } - raw, present := decoded["arguments"] - if !present { - return event + projected := map[string]json.RawMessage{} + for _, field := range toolCallIdentityFields { + if value, present := decoded[field]; present { + projected[field] = value + } } kept := map[string]any{} - var argumentsText string - if err := json.Unmarshal(raw, &argumentsText); err == nil { - var arguments map[string]any - if err := json.Unmarshal([]byte(argumentsText), &arguments); err == nil { - for key, value := range arguments { - if toolCallIdentityKeys[strings.ToLower(key)] { - kept[key] = redactedIdentityValue(value) + if raw, present := decoded["arguments"]; present { + var argumentsText string + if err := json.Unmarshal(raw, &argumentsText); err == nil { + var arguments map[string]any + if err := json.Unmarshal([]byte(argumentsText), &arguments); err == nil { + for key, value := range arguments { + if toolCallIdentityKeys[strings.ToLower(key)] { + kept[key] = redactedIdentityValue(value) + } } } } } - if len(kept) == 0 { - delete(decoded, "arguments") - } else { - reduced, err := json.Marshal(kept) - if err != nil { - delete(decoded, "arguments") - } else { - quoted, _ := json.Marshal(string(reduced)) - decoded["arguments"] = quoted + if len(kept) > 0 { + if reduced, err := json.Marshal(kept); err == nil { + if quoted, err := json.Marshal(string(reduced)); err == nil { + projected["arguments"] = quoted + } } } - rebuilt, err := json.Marshal(decoded) + rebuilt, err := json.Marshal(projected) if err != nil { event.Payload = json.RawMessage(`{}`) return event From 789bd389aa5692e9d5478daa0e97f43b5e924c83 Mon Sep 17 00:00:00 2001 From: Vasanthdev2004 Date: Fri, 11 Sep 2026 10:10:54 +0530 Subject: [PATCH 8/9] fix(sessions): name the value shapes a projected argument may keep, and scope the ambiguous aliases Two halves of the same gap: the projection decided which KEYS survive and said nothing about the values under them or about which tool gave them meaning. Values. Strings were scrubbed and everything else passed through, which reads as "a number is nothing to scrub" and is true of a number and not of an object. MCP schemas allow object and array properties, so {"query":{"api_key":"...","content":"private body"}} arrived under a permitted key and carried a whole payload into the next prompt. Redacting a serialized object would not have helped, since ordinary private text has no credential shape to match. The retained shapes are named now: a scrubbed string, and the scalars a read window is made of. A container is dropped and the call keeps its name and id, so a resumed turn still knows which tool ran. Meaning. grep accepts "search" for its pattern and edit_file accepts the same word for the text being replaced, so admitting it globally would have replayed file contents. It is scoped to grep, and the rule is kept as a test: an argument name any mutating tool accepts for body content belongs in the per-tool table or nowhere. The unambiguous supported forms are added to the shared table: glob's match, and read_file's start_line, end_line, max_lines, byte_offset and byte_limit. Driven through the store on both resume and fork, including a call the tool would have rejected, since the agent records OnToolCall before execution. The persisted events are asserted unchanged: this is a view for the prompt, not an edit to the record. --- .../exec_prompt_tool_call_identity_test.go | 214 ++++++++++++++++++ internal/sessions/exec_session.go | 78 ++++++- 2 files changed, 280 insertions(+), 12 deletions(-) diff --git a/internal/sessions/exec_prompt_tool_call_identity_test.go b/internal/sessions/exec_prompt_tool_call_identity_test.go index eb4bac555..56f913f8d 100644 --- a/internal/sessions/exec_prompt_tool_call_identity_test.go +++ b/internal/sessions/exec_prompt_tool_call_identity_test.go @@ -240,3 +240,217 @@ func TestResumePromptProjectsTopLevelToolCallFields(t *testing.T) { } } } + +// AND A PERMITTED KEY IS NOT PERMISSION FOR WHATEVER HANGS UNDER IT. +// +// The projection scrubbed strings and passed everything else through, which +// reads as "a number is nothing to scrub" and is true of a number and not of an +// object. MCP schemas allow object and array properties, so a whole payload can +// arrive under a permitted key. Redacting a serialized object would not help: +// ordinary private text has no credential shape to match. +func TestResumePromptDropsNestedArgumentContainers(t *testing.T) { + const body = "private body text with no credential shape at all" + const token = "secret-value-9f8e7d6c5b4a" + events := []Event{ + {Sequence: 1, Type: EventMessage, Payload: toolContextPayload(t, map[string]any{"role": "user", "content": "go"})}, + {Sequence: 2, Type: EventToolCall, Payload: toolContextPayload(t, map[string]any{ + "id": "c1", "name": "mcp_search", + "arguments": `{"query":{"api_key":"` + token + `","content":"` + body + `"}}`, + })}, + {Sequence: 3, Type: EventToolCall, Payload: toolContextPayload(t, map[string]any{ + "id": "c2", "name": "web_fetch", + "arguments": `{"url":["https://api.example/data?access_token=` + token + `"]}`, + })}, + {Sequence: 4, Type: EventError, Payload: toolContextPayload(t, map[string]any{"message": "provider error"})}, + } + out := resumePrompt(t, events) + + for _, leaked := range []string{body, body[:20], token, token[:12], "api_key"} { + if strings.Contains(out, leaked) { + t.Errorf("nested argument content %q reached the resume prompt:\n%s", leaked, out) + } + } + // The call still says which tool ran, so a resumed turn can ask again. + for _, kept := range []string{"c1", "mcp_search", "c2", "web_fetch"} { + if !strings.Contains(out, kept) { + t.Errorf("call identity %q was lost along with the container:\n%s", kept, out) + } + } +} + +// THE SEARCH ALIASES ARE TOOL-SPECIFIC, AND ONE OF THEM IS A TRAP. +// +// grep accepts `search` for its pattern and edit_file accepts the same word for +// the text being replaced. Admitting it globally for the sake of the first would +// replay file contents through the second. +func TestResumePromptKeepsGrepSearchAndDropsEditSearch(t *testing.T) { + const oldText = "OLD FILE CONTENTS THAT MUST NOT BE REPLAYED" + events := []Event{ + {Sequence: 1, Type: EventMessage, Payload: toolContextPayload(t, map[string]any{"role": "user", "content": "go"})}, + {Sequence: 2, Type: EventToolCall, Payload: toolContextPayload(t, map[string]any{ + "id": "c1", "name": "grep", "arguments": `{"search":"func toolCallIdentity","path":"internal/sessions"}`, + })}, + {Sequence: 3, Type: EventToolCall, Payload: toolContextPayload(t, map[string]any{ + "id": "c2", "name": "edit_file", "arguments": `{"path":"a.go","search":"` + oldText + `","replace":"new"}`, + })}, + {Sequence: 4, Type: EventError, Payload: toolContextPayload(t, map[string]any{"message": "provider error"})}, + } + out := resumePrompt(t, events) + + if !strings.Contains(out, "func toolCallIdentity") { + t.Errorf("grep lost the pattern it was searching for:\n%s", out) + } + if strings.Contains(out, oldText) || strings.Contains(out, oldText[:20]) { + t.Errorf("edit_file replayed the text it was replacing:\n%s", out) + } + // Both calls keep the path, which is identity for either tool. + for _, kept := range []string{"internal/sessions", "a.go"} { + if !strings.Contains(out, kept) { + t.Errorf("path identity %q was lost:\n%s", kept, out) + } + } +} + +// AND THE SUPPORTED SEARCH AND WINDOW FORMS SURVIVE, not only the canonical +// ones. A resumed turn that knows the path but not which slice was asked for +// has half the identity and will read again. +func TestResumePromptKeepsSupportedAliasesAndWindows(t *testing.T) { + events := []Event{ + {Sequence: 1, Type: EventMessage, Payload: toolContextPayload(t, map[string]any{"role": "user", "content": "go"})}, + {Sequence: 2, Type: EventToolCall, Payload: toolContextPayload(t, map[string]any{ + "id": "c1", "name": "glob", "arguments": `{"match":"src/**/*.go"}`, + })}, + {Sequence: 3, Type: EventToolCall, Payload: toolContextPayload(t, map[string]any{ + "id": "c2", "name": "read_file", "arguments": `{"path":"large.go","byte_offset":4096,"byte_limit":512}`, + })}, + {Sequence: 4, Type: EventToolCall, Payload: toolContextPayload(t, map[string]any{ + "id": "c3", "name": "read_file", "arguments": `{"path":"big.go","start_line":40,"end_line":80,"max_lines":41}`, + })}, + {Sequence: 5, Type: EventError, Payload: toolContextPayload(t, map[string]any{"message": "provider error"})}, + } + out := resumePrompt(t, events) + + for _, kept := range []string{"src/**/*.go", "4096", "512", "40", "80", "41"} { + if !strings.Contains(out, kept) { + t.Errorf("supported identity or window %q was lost:\n%s", kept, out) + } + } +} + +// THE SHARED TABLE MUST NEVER NAME A MUTATING TOOL'S BODY ARGUMENT. +// +// This is the rule the grep/edit_file collision taught, kept as a test so the +// next alias added for a search tool cannot quietly open the same door. The +// aliases below are the ones the mutating tools accept for the content they +// write; they belong in the per-tool table or nowhere. +func TestIdentityKeysNeverNameAMutatingToolBodyArgument(t *testing.T) { + bodyAliases := map[string][]string{ + "edit_file": {"old_string", "old", "search", "find", "old_str", "new_string", "new", "replace", "replacement", "new_str"}, + "write_file": {"content", "contents", "text", "body", "data", "file_content"}, + "apply_patch": {"patch"}, + } + for tool, aliases := range bodyAliases { + for _, alias := range aliases { + if toolCallIdentityKeys[alias] { + t.Errorf("%s accepts %q for content it writes, and the shared identity table admits it: a resumed prompt would replay the body", tool, alias) + } + } + } + // The per-tool table is where an ambiguous name belongs, and grep's use of + // `search` is the case that proves the mechanism is wired. + if !toolCallIdentityKeysByTool["grep"]["search"] { + t.Error("grep lost its search alias, so an interrupted grep resumes without the pattern") + } +} + +// THROUGH THE STORE, NOT JUST THE RENDERER. +// +// Every test above hands FormatExecPrompt an events slice it built in memory, +// so none of them shows the projection still applies to events that were +// written, reloaded and prepared by the real resume and fork paths. That is +// where it has to hold: the leak this guards against is a later turn reading a +// recorded call, and the recording is what makes it later. +// +// Includes a call the tool would have REJECTED. The agent records OnToolCall +// before execution, so a read_file whose path is an object is persisted even +// though the tool refuses it, and a projection that only considered +// well-formed calls would carry it through. +func TestPersistedToolCallsAreProjectedOnResumeAndFork(t *testing.T) { + const body = "private body text with no credential shape at all" + const token = "secret-value-9f8e7d6c5b4a" + const oldText = "OLD FILE CONTENTS THAT MUST NOT BE REPLAYED" + + store := NewStore(StoreOptions{RootDir: t.TempDir()}) + session, err := store.Create(CreateInput{SessionID: "projection_persisted"}) + if err != nil { + t.Fatal(err) + } + appends := []AppendEventInput{ + {Type: EventMessage, Payload: map[string]any{"role": "user", "content": "go"}}, + {Type: EventToolCall, Payload: map[string]any{ + "id": "c1", "name": "mcp_search", + "arguments": `{"query":{"api_key":"` + token + `","content":"` + body + `"}}`, + }}, + {Type: EventToolCall, Payload: map[string]any{ + "id": "c2", "name": "edit_file", + "arguments": `{"path":"a.go","search":"` + oldText + `","replace":"new"}`, + }}, + // Rejected by read_file, recorded anyway. + {Type: EventToolCall, Payload: map[string]any{ + "id": "c3", "name": "read_file", + "arguments": `{"path":{"nested":"` + body + `"},"start_line":40}`, + }}, + {Type: EventToolCall, Payload: map[string]any{ + "id": "c4", "name": "grep", + "arguments": `{"search":"func toolCallIdentity","path":"internal/sessions"}`, + }}, + {Type: EventError, Payload: map[string]any{"message": "provider error: upstream timeout"}}, + } + if _, err := store.AppendEvents(session.SessionID, appends); err != nil { + t.Fatal(err) + } + + for _, mode := range []struct { + name string + options PrepareExecOptions + }{ + {"resume", PrepareExecOptions{Store: store, Resume: session.SessionID}}, + {"fork", PrepareExecOptions{Store: store, Fork: session.SessionID, SessionID: "projection_persisted_fork"}}, + } { + t.Run(mode.name, func(t *testing.T) { + prepared, err := PrepareExec(mode.options) + if err != nil { + t.Fatalf("PrepareExec: %v", err) + } + prompt := FormatExecPrompt("continue", prepared) + + for _, leaked := range []string{body, body[:20], token, token[:12], oldText, oldText[:20], "api_key"} { + if strings.Contains(prompt, leaked) { + t.Errorf("%q survived persistence into the %s prompt:\n%s", leaked, mode.name, prompt) + } + } + // The identities that make the tail worth carrying at all. + for _, kept := range []string{"mcp_search", "edit_file", "read_file", "grep", "func toolCallIdentity", "internal/sessions", "a.go"} { + if !strings.Contains(prompt, kept) { + t.Errorf("identity %q was lost from the %s prompt:\n%s", kept, mode.name, prompt) + } + } + }) + } + + // AND THE STORE STILL HOLDS THE ORIGINALS. The projection is a view for the + // prompt, not an edit to the record: /rewind and an audit read the events. + events, err := store.ReadEvents(session.SessionID) + if err != nil { + t.Fatal(err) + } + stored := "" + for _, event := range events { + stored += string(event.Payload) + } + for _, want := range []string{body, token, oldText} { + if !strings.Contains(stored, want) { + t.Errorf("the projection modified the persisted events: %q is no longer in the store", want) + } + } +} diff --git a/internal/sessions/exec_session.go b/internal/sessions/exec_session.go index bf7fc0ec9..774f9a296 100644 --- a/internal/sessions/exec_session.go +++ b/internal/sessions/exec_session.go @@ -301,13 +301,32 @@ var toolCallIdentityKeys = map[string]bool{ "dir": true, "directory": true, "cwd": true, "workdir": true, // what a search was for; these are what the interrupted turn was looking at "pattern": true, "glob": true, "query": true, "regex": true, "expression": true, + "match": true, // fetches and named resources "url": true, "name": true, - // read windows, so a resumed turn knows which part it already had + // read windows, so a resumed turn knows which part it already had. Every + // form read_file accepts, because a resumed turn that knows the path but not + // which slice was asked for has half the identity and will read again. "offset": true, "limit": true, + "start_line": true, "end_line": true, "max_lines": true, + "byte_offset": true, "byte_limit": true, } -// redactedIdentityValue scrubs credentials out of a value the projection keeps. +// toolCallIdentityKeysByTool holds the argument names whose meaning depends on +// which tool was called, so they cannot live in the table above. +// +// grep accepts `search` for its pattern, and edit_file accepts the same word +// for the text being replaced. Admitting it globally for the sake of the first +// would replay file contents through the second, which is the whole thing this +// projection exists to stop. The rule this encodes: an argument name that any +// MUTATING tool accepts for body content is scoped to the tools that mean +// something else by it, never added to the shared table. +var toolCallIdentityKeysByTool = map[string]map[string]bool{ + "grep": {"search": true}, +} + +// retainedIdentityValue decides whether a value under a permitted key may be +// kept, and what it looks like if so. // // AN ALLOW-LISTED KEY IS NOT A SAFE VALUE. A url is a valid place for a // credential to appear: web_fetch accepts a query token and redacts the URL it @@ -316,14 +335,40 @@ var toolCallIdentityKeys = map[string]bool{ // result and kept verbatim in the call. This projection is what admits call // arguments into a later turn's prompt, so the same scrub belongs here or the // credential is replayed on resume. Host and path survive it, which is the -// identity the resumed turn needs; non-string values (an offset, a limit) are -// nothing to scrub. -func redactedIdentityValue(value any) any { - text, ok := value.(string) - if !ok { - return value - } - return redaction.RedactString(text, redaction.Options{}) +// identity the resumed turn needs. +// +// AND A KEY IS NOT PERMISSION FOR WHATEVER HANGS UNDER IT. Scrubbing strings +// and passing everything else through read as "numbers are nothing to scrub", +// which is true of a number and not of an object: MCP schemas allow object and +// array properties, so {"query":{"api_key":"...","content":"private body"}} +// arrives under a permitted key and carries a whole payload with it. Redacting +// a serialized object would not help either, since ordinary private text has no +// credential shape to match. +// +// So the retained shapes are named: a string, scrubbed, and the scalars a read +// window is made of. A container is dropped. The call keeps its name and id, so +// a resumed turn still knows which tool ran and can ask again; what it does not +// get is arbitrary nested content it was never meant to see. +func retainedIdentityValue(value any) (any, bool) { + switch typed := value.(type) { + case string: + return redaction.RedactString(typed, redaction.Options{}), true + case float64, int, int64, bool, json.Number: + return typed, true + default: + // Objects, arrays, null, and anything a future decoder invents. + return nil, false + } +} + +// toolCallIdentityKeyAllowed reports whether key is an identity field for the +// tool that was called. +func toolCallIdentityKeyAllowed(toolName, key string) bool { + lowered := strings.ToLower(key) + if toolCallIdentityKeys[lowered] { + return true + } + return toolCallIdentityKeysByTool[strings.ToLower(strings.TrimSpace(toolName))][lowered] } // toolCallIdentityFields are the top-level payload fields a projected call @@ -359,6 +404,12 @@ func toolCallIdentity(event Event) Event { projected[field] = value } } + // The tool decides what some argument names mean, so the projection has to + // know which tool this was before it can say which keys are identity. + toolName := "" + if raw, present := decoded["name"]; present { + _ = json.Unmarshal(raw, &toolName) + } kept := map[string]any{} if raw, present := decoded["arguments"]; present { var argumentsText string @@ -366,8 +417,11 @@ func toolCallIdentity(event Event) Event { var arguments map[string]any if err := json.Unmarshal([]byte(argumentsText), &arguments); err == nil { for key, value := range arguments { - if toolCallIdentityKeys[strings.ToLower(key)] { - kept[key] = redactedIdentityValue(value) + if !toolCallIdentityKeyAllowed(toolName, key) { + continue + } + if retained, ok := retainedIdentityValue(value); ok { + kept[key] = retained } } } From 29a5253013d1c4735053ca5482c498ee0cd58337 Mon Sep 17 00:00:00 2001 From: Vasanthdev2004 Date: Fri, 11 Sep 2026 13:17:19 +0530 Subject: [PATCH 9/9] test(sessions): assert the window key with its value, not the number alone strings.Contains(out, "40") finds it inside "4096", so the start_line assertion passed whether or not start_line survived the projection, which is the opposite of what it claimed. The rendered arguments carry the pairs, so the assertions name them. --- .../sessions/exec_prompt_tool_call_identity_test.go | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/internal/sessions/exec_prompt_tool_call_identity_test.go b/internal/sessions/exec_prompt_tool_call_identity_test.go index 56f913f8d..ad55e34b2 100644 --- a/internal/sessions/exec_prompt_tool_call_identity_test.go +++ b/internal/sessions/exec_prompt_tool_call_identity_test.go @@ -330,9 +330,16 @@ func TestResumePromptKeepsSupportedAliasesAndWindows(t *testing.T) { } out := resumePrompt(t, events) - for _, kept := range []string{"src/**/*.go", "4096", "512", "40", "80", "41"} { + // KEY AND VALUE TOGETHER, NOT THE NUMBER ALONE. Searching for "40" finds + // it inside "4096", so the start_line assertion passed whether or not + // start_line survived at all, which is the opposite of what it claims. + for _, kept := range []string{ + "src/**/*.go", + `"byte_offset":4096`, `"byte_limit":512`, + `"start_line":40`, `"end_line":80`, `"max_lines":41`, + } { if !strings.Contains(out, kept) { - t.Errorf("supported identity or window %q was lost:\n%s", kept, out) + t.Errorf("supported identity or window %s was lost:\n%s", kept, out) } } }