mirror of
https://github.com/Twigpine/zero.git
synced 2026-11-10 05:01:28 -10:00
Vasanth T
6ae8396f02
* 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. * 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. * 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. * 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. * 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. * 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. * 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. * 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. * 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.
228 lines
10 KiB
Go
228 lines
10 KiB
Go
package sessions
|
|
|
|
import (
|
|
"encoding/json"
|
|
"fmt"
|
|
"slices"
|
|
"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)
|
|
}
|
|
}
|
|
|
|
// 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)
|
|
|
|
// 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.HasPrefix(line, "- #") && (strings.Contains(line, "tool_call:") || strings.Contains(line, "tool_result:")) {
|
|
toolLines = append(toolLines, line)
|
|
}
|
|
}
|
|
if len(toolLines) != len(want) {
|
|
t.Fatalf("the resume prompt carries %d tool lines, want %d:\n%s", len(toolLines), len(want), out)
|
|
}
|
|
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)
|
|
}
|
|
}
|