From 379e25b1a2f8489338331523fc0a17f6de0dec5d Mon Sep 17 00:00:00 2001 From: mike-diff Date: Wed, 19 Aug 2026 22:12:10 -0700 Subject: [PATCH 1/2] feat(tools): keep both ends of oversized tool output and spill the rest --- harness/continuity.go | 26 ++- harness/continuity_test.go | 11 +- harness/doctor.go | 9 + harness/drive.go | 2 +- harness/e2e_test.go | 89 +++++++++ harness/engines.go | 4 +- harness/harness.go | 7 +- harness/help.go | 9 +- harness/main_test.go | 6 +- harness/mcp.go | 8 +- harness/mcp_test.go | 13 +- harness/repl.go | 2 +- harness/scaffold/tuning.json.example | 12 ++ harness/spill.go | 283 ++++++++++++++++++++++++++ harness/spill_test.go | 287 +++++++++++++++++++++++++++ harness/toolmods.go | 4 +- harness/tools.go | 33 +-- harness/tuning.go | 39 ++++ 18 files changed, 802 insertions(+), 42 deletions(-) create mode 100644 harness/spill.go create mode 100644 harness/spill_test.go diff --git a/harness/continuity.go b/harness/continuity.go index 26ed10c..e8aedbd 100644 --- a/harness/continuity.go +++ b/harness/continuity.go @@ -83,11 +83,7 @@ func renderTranscript(turns []agent.Turn, maxResult int) string { b.WriteByte('\n') case "tool": for _, res := range t.Results { - out := res.Content - if len(out) > maxResult { - out = out[:maxResult] + "..." - } - fmt.Fprintf(&b, "TOOL RESULT: %s\n", strings.ReplaceAll(out, "\n", " · ")) + fmt.Fprintf(&b, "TOOL RESULT: %s\n", strings.ReplaceAll(elideText(res.Content, maxResult), "\n", " · ")) } b.WriteByte('\n') } @@ -100,6 +96,26 @@ func renderTranscript(turns []agent.Turn, maxResult int) string { return s } +// elideText bounds one tool result inside the transcript, keeping its head AND +// its tail around a marker, the same shape diff.go's elide gives a large diff +// and the whole-transcript cut below gives a long transcript. It matters most +// here: the judge rules done on this text, and a head-only cut of a failing +// test run reads as an unbroken wall of PASS lines, which is worse than seeing +// nothing at all. The head/tail split mirrors the transcript cut, so one ratio +// governs both levels. +func elideText(s string, max int) string { + if max <= 0 || len(s) <= max { + return s + } + const marker = " ... " + if max <= len(marker)+2 { // too small to keep both ends meaningfully + return s[:max] + } + budget := max - len(marker) + head := budget / 3 + return s[:head] + marker + s[len(s)-(budget-head):] +} + // imageNote renders a one-line, byte-free summary of a user turn's images for // the transcript: enough for the brief writer to record that images existed and // their shape, never the data itself. diff --git a/harness/continuity_test.go b/harness/continuity_test.go index 60081d6..e9d0686 100644 --- a/harness/continuity_test.go +++ b/harness/continuity_test.go @@ -56,11 +56,20 @@ func TestRenderTranscript(t *testing.T) { {Role: "assistant", Text: "done"}, } got := renderTranscript(h, 100) - for _, want := range []string{"USER: fix the bug", "ASSISTANT ran read", "TOOL RESULT: " + strings.Repeat("z", 100) + "...", "ASSISTANT: done"} { + for _, want := range []string{"USER: fix the bug", "ASSISTANT ran read", "ASSISTANT: done"} { if !strings.Contains(got, want) { t.Fatalf("transcript missing %q:\n%s", want, got) } } + // A tool result is elided from the MIDDLE: the head shows what the output + // was, the tail shows how it ended. Keeping only the head is what let a + // failing command reach the judge looking like a successful one. + if !strings.Contains(got, "TOOL RESULT: "+strings.Repeat("z", 30)) { + t.Fatalf("elided result lost its head:\n%s", got) + } + if !strings.Contains(got, "second line") { + t.Fatalf("elided result lost its tail:\n%s", got) + } if strings.Contains(got, strings.Repeat("z", 101)) { t.Fatal("tool results must be elided to the stub") } diff --git a/harness/doctor.go b/harness/doctor.go index 606a186..191b341 100644 --- a/harness/doctor.go +++ b/harness/doctor.go @@ -218,6 +218,15 @@ func runDoctor() int { pass("%s: writable, %d sessions", sessionsDir(), len(allSessions())) } + // Spilled output is the one mount point that grows on its own, so doctor + // reports what is there and confirms the sweep has something to do. + if n, bytes := outputUsage(); n > 0 { + pass("%s: %d conversation(s), %s (swept after %d idle days)", + outputsDir(), n, humanBytes(bytes), tune.ResultKeepDays) + } else { + pass("%s: empty", outputsDir()) + } + if ok { fmt.Println("\nall checks passed") return 0 diff --git a/harness/drive.go b/harness/drive.go index cd4b6aa..fe5bbfa 100644 --- a/harness/drive.go +++ b/harness/drive.go @@ -215,7 +215,7 @@ func drive(r *repl, cfg driveConfig, firstTurns []agent.Turn) int { // Escape pauses the drive during the judge phase too (not just during a // streamed worker iteration). jctx, jdone := turnCtx() - transcript := renderTranscript(iterTurns, 300) + transcript := renderTranscript(iterTurns, tune.TranscriptResult) var jUsed agent.Usage var jerr error v, jUsed, jerr = judgeGoal(jctx, r.p, cfg.request, transcript) diff --git a/harness/e2e_test.go b/harness/e2e_test.go index d4210d7..e5859c5 100644 --- a/harness/e2e_test.go +++ b/harness/e2e_test.go @@ -667,4 +667,93 @@ func TestE2E(t *testing.T) { t.Fatal("a denied write must not touch disk") } }) + // The defect this feature exists for, end to end through the real binary: a + // failing test run whose output exceeds the window budget must still reach + // the model with its verdict, and the elided middle must be recoverable with + // the read tool at the offset the pointer names. + t.Run("OversizedOutputKeepsVerdictAndSpills", func(t *testing.T) { + m, dir := newRig(t, + []e2eStep{ + eTool("bash", `{"command":"sh gen.sh"}`), + eText("the suite failed"), + }, + []e2eStep{verdictJSON("reported the failure")}) + // A generator whose output brackets the budget: PASS noise around a + // diagnostic in the middle, and the verdict on the final line. + gen := "#!/bin/sh\n" + + "i=0; while [ $i -lt 900 ]; do echo \"=== RUN TestAlpha$i\"; echo \"--- PASS: TestAlpha$i (0.00s)\"; i=$((i+1)); done\n" + + "echo ' a_test.go:403: boom: nil map write at pkg/store.go:88'\n" + + "i=0; while [ $i -lt 900 ]; do echo \"=== RUN TestBeta$i\"; echo \"--- PASS: TestBeta$i (0.00s)\"; i=$((i+1)); done\n" + + "echo 'FAIL\tsigprobe/pkg\t0.013s'\n" + os.WriteFile(filepath.Join(dir, "gen.sh"), []byte(gen), 0o755) + + m.run(t, dir, "run the suite and tell me whether it passed") + + res := m.lastToolResult(t, 2) + // The judge rules done on the transcript, so the verdict must survive the + // per-result elision too: a head-only cut fed it a wall of PASS lines + // from a run that failed. + judged := false + m.mu.Lock() + for _, r := range m.reqs { + if r.Class != "judge" { + continue + } + msgs, _ := r.Body["messages"].([]any) + for _, mm := range msgs { + x, ok := mm.(map[string]any) + if !ok { + continue + } + if c, _ := x["content"].(string); strings.Contains(c, "FAIL\tsigprobe/pkg") { + judged = true + } + } + } + m.mu.Unlock() + if !judged { + t.Error("the judge must see the failing verdict in the transcript") + } + if len(res) > tune.ResultMaxChars { + t.Fatalf("result reached the model unbounded: %d bytes", len(res)) + } + if !strings.Contains(res, "FAIL\tsigprobe/pkg") { + t.Fatalf("the verdict on the last line must survive:\n%s", tailOf(res, 400)) + } + if !strings.Contains(res, "TestAlpha0") { + t.Fatalf("the head must survive:\n%s", headOf(res, 400)) + } + + // The pointer must name a real file, and it must hold what was elided. + i := strings.Index(res, "full output: ") + if i < 0 { + t.Fatalf("no spill pointer in the shaped result:\n%s", res[max(0, len(res)-400):]) + } + path := res[i+len("full output: "):] + path = path[:strings.Index(path, " ")] + body, err := os.ReadFile(path) + if err != nil { + t.Fatalf("spilled output unreadable at the path the model was given: %v", err) + } + if !strings.Contains(string(body), "boom: nil map write") { + t.Fatal("the spilled file must hold the elided diagnostic") + } + if !strings.Contains(string(body), "TestBeta450") { + t.Fatal("the spilled file must hold the full output, not just the shaped ends") + } + }) +} + +func headOf(s string, n int) string { + if len(s) <= n { + return s + } + return s[:n] +} + +func tailOf(s string, n int) string { + if len(s) <= n { + return s + } + return s[len(s)-n:] } diff --git a/harness/engines.go b/harness/engines.go index b733312..8e428a3 100644 --- a/harness/engines.go +++ b/harness/engines.go @@ -20,7 +20,7 @@ func engineTools() ([]agent.Tool, []string, string) { skill, n, ok := skillTool() notes = append(notes, n...) if ok { - tools = append(tools, skill) + tools = append(tools, shaped(skill)) if !tune.SkillNoteOff { sysNote = "\n\n\nSkills are installed. Before starting a task, check the skill tool's manifest: when a line matches the task, load that skill first and follow its instructions.\n" } @@ -28,7 +28,7 @@ func engineTools() ([]agent.Tool, []string, string) { mcp, n, ok := mcpTool() notes = append(notes, n...) if ok { - tools = append(tools, mcp) + tools = append(tools, shaped(mcp)) } return tools, notes, sysNote } diff --git a/harness/harness.go b/harness/harness.go index 467cee2..1c04ef3 100644 --- a/harness/harness.go +++ b/harness/harness.go @@ -18,6 +18,7 @@ import ( "fmt" "os" "os/signal" + "path/filepath" "strings" "sync" "time" @@ -193,7 +194,11 @@ func Main() { sweepDeadProcs(sess.ID) // reap processes a previously-crashed sesh left behind pm := newProcManager(sess.ID) os.Setenv("SESH_SESSION", sess.ID) // tool/gate/statusline mods can find this session's run dir - go gcBlobs() // sweep orphaned image blobs off the hot path; best-effort, never blocks startup + // Spilled tool output is keyed by the chain root, so it survives every + // handoff without rewriting the pointers already in the transcript. + spill = newOutStore(outputDir(sess)) + go gcBlobs() // sweep orphaned image blobs off the hot path; best-effort, never blocks startup + go gcOutput(filepath.Base(spill.dir)) // same for spilled output of conversations long finished tools := builtinTools(*unsafePaths, pm) // The engines (skill, mcp) join only when their user-space content exists: // an empty mount costs zero tokens. They are built-ins, so they claim diff --git a/harness/help.go b/harness/help.go index c08aed1..b8f5a9d 100644 --- a/harness/help.go +++ b/harness/help.go @@ -97,8 +97,10 @@ FILES AND MODS (project .sesh/ overrides global ~/.sesh/) seed_ledger_entries, task_depth, stuck_after, recall_links, diff_lines, proc_promote_secs, max_procs, proc_log_tail, update_check, - input_max_rows, brief_provider, brief_model - (state only what you change) + input_max_rows, brief_provider, brief_model, + result_max_chars, result_head_pct, + result_spill_off, result_keep_days, + transcript_result (state only what you change) tools/ executables that become agent tools (global mount only): --schema describes, args JSON on stdin, stdout is the result; mutating ones @@ -108,6 +110,9 @@ FILES AND MODS (project .sesh/ overrides global ~/.sesh/) line is the reason); broken mod fails closed statusline executable; JSON on stdin, first line shown sessions/, chains/ transcripts and chain ledgers (plain JSON/JSONL) + out/ full text of tool output too large for the + context window, keyed by conversation; the + shaped result points here and read can page it run/ background-process logs and crash records, cleared when a session exits diff --git a/harness/main_test.go b/harness/main_test.go index 9013015..43a7aa5 100644 --- a/harness/main_test.go +++ b/harness/main_test.go @@ -281,6 +281,10 @@ func TestBashOutputCap(t *testing.T) { } } +// TestCappedBuffer: the buffer keeps the TAIL, evicting from the head like the +// process supervisor's ring. A command that outgrows the cap is almost always a +// build or test run, and its verdict is on the last lines. Breaker: keep the +// head instead and "efgh" no longer ends the buffer. func TestCappedBuffer(t *testing.T) { c := &cappedBuffer{max: 5} for _, chunk := range []string{"ab", "cd", "efgh"} { @@ -288,7 +292,7 @@ func TestCappedBuffer(t *testing.T) { t.Fatalf("write must report full consumption: n=%d err=%v", n, err) } } - if string(c.buf) != "abcde" || c.dropped != 3 { + if string(c.buf) != "defgh" || c.dropped != 3 { t.Fatalf("buf=%q dropped=%d", c.buf, c.dropped) } } diff --git a/harness/mcp.go b/harness/mcp.go index 3c5f3cd..8261858 100644 --- a/harness/mcp.go +++ b/harness/mcp.go @@ -622,8 +622,9 @@ func runMCP(ctx context.Context, pool *mcpPool, raw json.RawMessage) (string, bo // renderMCPResult flattens an MCP content array to the one string a sesh // tool returns: text blocks verbatim, other block types as markers, isError -// surfacing as a tool error the model can act on. Oversize output is -// truncated loudly, the same cap as bash. +// surfacing as a tool error the model can act on. Bounding the size is the +// shaper's job at assembly, which keeps both ends and spills the rest, so +// there is no second cut here to lose the tail before it gets there. func renderMCPResult(res json.RawMessage) (string, bool) { var r struct { Content []struct { @@ -647,8 +648,5 @@ func renderMCPResult(res json.RawMessage) (string, bool) { if out == "" { out = "(no output)" } - if len(out) > maxBashOutput { - out = out[:maxBashOutput] + "\n[output truncated at 1MB]" - } return out, r.IsError } diff --git a/harness/mcp_test.go b/harness/mcp_test.go index b45a1e2..7a43cc5 100644 --- a/harness/mcp_test.go +++ b/harness/mcp_test.go @@ -129,6 +129,9 @@ func mcpCall(t *testing.T, tool string, args string) (string, bool) { if !ok { t.Fatal("mcp tool did not activate") } + // shaped() is what engineTools applies at assembly, so bounding the result + // is exercised here the way the model actually meets it. + tl = shaped(tl) raw := json.RawMessage(fmt.Sprintf(`{"server":"gauntlet","tool":%q,"args":%s}`, tool, args)) return tl.Run(context.Background(), raw) } @@ -273,15 +276,15 @@ func TestMCPManifestCacheFeedsNextSession(t *testing.T) { } } -// Breaker: return server output unbounded. +// Breaker: return server output unbounded, or bound it without saying so. func TestMCPBigOutputTruncatedLoudly(t *testing.T) { mcpHome(t, map[string]mcpServerConf{"gauntlet": gauntletConf(nil)}) out, _ := mcpCall(t, "big", `{}`) - if len(out) > maxBashOutput+100 { - t.Fatalf("output not capped: %d bytes", len(out)) + if len(out) > tune.ResultMaxChars { + t.Fatalf("output not bounded: %d bytes", len(out)) } - if !strings.Contains(out, "[output truncated") { - t.Fatal("truncation must be loud, not silent") + if !strings.Contains(out, "elided") { + t.Fatalf("bounding must be loud, not silent:\n%s", out[max(0, len(out)-300):]) } } diff --git a/harness/repl.go b/harness/repl.go index 637c0fe..21e45c0 100644 --- a/harness/repl.go +++ b/harness/repl.go @@ -1061,7 +1061,7 @@ func (r *repl) handoff() bool { } else { emit("%s writing handoff brief...%s\n", dim, reset) } - brief, entry, used, err := writeBrief(context.Background(), bp, renderTranscript(r.history, 300)) + brief, entry, used, err := writeBrief(context.Background(), bp, renderTranscript(r.history, tune.TranscriptResult)) if err != nil { emit("%s handoff brief failed: %v%s\n", red, err, reset) return false diff --git a/harness/scaffold/tuning.json.example b/harness/scaffold/tuning.json.example index 1bce1d9..cbce8f1 100644 --- a/harness/scaffold/tuning.json.example +++ b/harness/scaffold/tuning.json.example @@ -62,6 +62,18 @@ // "proc_log_tail": 200, // default lines a `proc logs` read returns // "proc_spill_off": false, // true keeps logs in memory only (no run/ file) + // --- Tool output too large for the window --- + // An over-budget result keeps its head AND its tail around an elided middle, + // and the full text spills to ~/.sesh/out// so the model can + // read what was elided. Both ends matter: a failing command's diagnostic can + // sit anywhere in the output, but its verdict is always on the last lines. + // "result_max_chars": 28000, // budget for one shaped tool result + // "result_head_pct": 25, // share of that budget given to the head + // "result_spill_off": false, // true keeps the shaped result but writes no file + // "result_keep_days": 7, // how long spilled output outlives its last write + // "transcript_result": 300, // per-result budget in the transcript the judge + // // and the brief writer read (also head + tail) + // --- Other dials (full reference: ~/.sesh/README.md and -help) --- // "task_depth": 3, // subagent nesting cap // "stuck_after": 3, // driven iterations w/o a mutation before "stuck" diff --git a/harness/spill.go b/harness/spill.go new file mode 100644 index 0000000..53d0881 --- /dev/null +++ b/harness/spill.go @@ -0,0 +1,283 @@ +// Shaping tool output for the context window. A tool result that exceeds the +// budget keeps its head AND its tail, with the middle elided and the full text +// spilled to a file the model can read back. +// +// Both ends are load-bearing, which one-ended truncation cannot serve: a failing +// command puts its diagnostic anywhere in the output (measured: 51% in, for a +// verbose test run with one failure) but its verdict on the last line (measured: +// 30 bytes from the end). Cutting either end alone destroys one of the two, and +// keeping only the head is worse than losing information: a truncated run of a +// FAILING suite reads as an unbroken wall of PASS lines, which steers the drive +// judge toward done. +// +// The middle is recoverable rather than trusted, the same bargain the handoff +// makes: the pointer names the elided line range, so read's existing offset +// paging lands directly on it. The shapes here are the ones the harness already +// uses elsewhere (a tail-biased log with its elision reported, in proc; a +// head-plus-tail cut with an elided middle, in renderTranscript). +package harness + +import ( + "context" + "encoding/json" + "fmt" + "os" + "path/filepath" + "strconv" + "strings" + "sync" + "time" + + "github.com/mike-diff/sesh/agent" +) + +func outputsDir() string { return filepath.Join(os.Getenv("HOME"), ".sesh", "out") } + +// outputDir is where one conversation's spilled output lives. It is keyed by +// the CHAIN root, not the session id: a handoff renames nothing here, so a +// pointer written before the boundary still resolves after it. Session.Root is +// empty until the first handoff, and that handoff sets the successor's Root to +// this id, so the key is the same on both sides of every boundary. +func outputDir(s *Session) string { + key := s.Root + if key == "" { + key = s.ID + } + return filepath.Join(outputsDir(), key) +} + +// outStore hands out the spill files for one conversation. Allocation is locked +// because a reply's parallel tool calls (and concurrent task subagents) shape +// their results at the same time. +type outStore struct { + mu sync.Mutex + dir string + seq int +} + +// spill is the live store, resolved once at startup like the tuning dials. Nil +// means shaping still trims but nothing is written, which is what the bench rig +// and doctor run with. +var spill *outStore + +// newOutStore resumes the id sequence from what is already on disk, so a +// resumed session does not hand out ids that overwrite the previous run's +// files. A dir it cannot read yields an empty store rather than an error: +// failing to spill must degrade to plain trimming, never break a tool call. +func newOutStore(dir string) *outStore { + o := &outStore{dir: dir} + entries, err := os.ReadDir(dir) + if err != nil { + return o + } + for _, e := range entries { + name := strings.TrimSuffix(e.Name(), ".log") + n, err := strconv.Atoi(strings.TrimPrefix(name, "out-")) + if err == nil && n > o.seq { + o.seq = n + } + } + return o +} + +// put writes s to a fresh file and returns its path. The write is atomic +// write-then-rename, like the blob store, so a crash mid-spill cannot leave a +// truncated file behind a pointer that claims it is complete. +func (o *outStore) put(s string) (string, error) { + o.mu.Lock() + o.seq++ + path := filepath.Join(o.dir, fmt.Sprintf("out-%d.log", o.seq)) + o.mu.Unlock() + if err := os.MkdirAll(o.dir, 0o700); err != nil { + return "", err + } + tmp := path + ".tmp" + if err := os.WriteFile(tmp, []byte(s), 0o600); err != nil { + return "", err + } + if err := os.Rename(tmp, path); err != nil { + return "", err + } + return path, nil +} + +// headCut returns the largest offset within budget that ends a line, so the +// kept head is whole lines. A single line longer than the budget has no +// boundary to find and is cut mid-line: showing part of it beats showing none. +func headCut(s string, budget int) int { + if budget >= len(s) { + return len(s) + } + if i := strings.LastIndexByte(s[:budget], '\n'); i >= 0 { + return i + 1 + } + return budget +} + +// tailCut returns the smallest offset at or after len(s)-budget that starts a +// line, the mirror of headCut. +func tailCut(s string, budget int) int { + if budget >= len(s) { + return 0 + } + start := len(s) - budget + if i := strings.IndexByte(s[start:], '\n'); i >= 0 { + return start + i + 1 + } + return start +} + +// shape bounds one tool result to the configured budget. Under it, the string +// is returned untouched. Over it, the head and tail are kept around an elided +// middle, and the note between them says how much was dropped and where the +// full text is. +func shape(s string) string { + max := tune.ResultMaxChars + if max <= 0 || len(s) <= max { + return s + } + // The note sits inside the budget, so the shaped result never exceeds it + // however long the spill path turns out to be. + const noteReserve = 240 + body := max - noteReserve + if body < 512 { // a budget too small to say anything useful about: trim only + return s[:max] + } + head := body * tune.ResultHeadPct / 100 + h := headCut(s, head) + t := tailCut(s, body-h) + if t <= h { // nothing actually elided; the cuts met + return s[:max] + } + + total := lineCount(s) + first := strings.Count(s[:h], "\n") + 1 + last := total - lineCount(s[t:]) + + note := fmt.Sprintf("lines %d-%d of %d elided (%d bytes)", first, last, total, t-h) + if path, err := spillFull(s); err == nil { + note += fmt.Sprintf("; full output: %s (read it with offset %d)", path, first) + } + return s[:h] + "\n... [" + note + "]\n" + s[t:] +} + +// spillFull writes the untrimmed result out of line. It reports an error when +// spilling is off or unavailable, in which case the shaped result carries the +// elided range without a path: the model is still told what it is missing. +func spillFull(s string) (string, error) { + if spill == nil || tune.ResultSpillOff { + return "", os.ErrNotExist + } + return spill.put(s) +} + +// shaped wraps a tool so its result is bounded before it reaches the context +// window. Applied at assembly to the tools whose output size the harness does +// not otherwise control: bash, the engines, and tool mods. The tools that +// already shape themselves (read's paging, search's suppression, proc's +// tail-biased logs, edit and write's bounded diff) are deliberately left alone, +// so there is exactly one shaping policy per result. +func shaped(t agent.Tool) agent.Tool { + inner := t.Run + t.Run = func(ctx context.Context, raw json.RawMessage) (string, bool) { + out, isErr := inner(ctx, raw) + return shape(out), isErr + } + return t +} + +// outputGCMinAge is how long a conversation's spilled output outlives its last +// write before gcOutput will collect it. The floor is generous because the cost +// of collecting too early (a pointer in a resumed transcript stops resolving) +// is paid by the model, while the cost of collecting too late is disk. +func outputGCMinAge() time.Duration { + return time.Duration(tune.ResultKeepDays) * 24 * time.Hour +} + +// gcOutput removes spilled output for conversations that have been idle past +// the age floor, keyed by directory rather than by scanning transcripts for +// pointers: a live session writes as it works, so its directory is never stale. +// Conservative like the blob sweep: every error is skipped, the current +// conversation is never touched, and a recent directory is always kept. A +// collected pointer degrades into a read error the model can act on, which is +// why erring toward collection is safe here. +func gcOutput(keep string) { + entries, err := os.ReadDir(outputsDir()) + if err != nil { + return // nothing spilled yet, or unreadable + } + cutoff := time.Now().Add(-outputGCMinAge()) + for _, e := range entries { + if !e.IsDir() || e.Name() == keep { + continue + } + dir := filepath.Join(outputsDir(), e.Name()) + if newest(dir).After(cutoff) { + continue // still in use, or recently was + } + os.RemoveAll(dir) // best-effort; an error just leaves the directory + } +} + +// newest reports the most recent modification time in dir, the directory's own +// included, so a directory whose files were all deleted still ages out. +func newest(dir string) time.Time { + info, err := os.Stat(dir) + if err != nil { + return time.Now() // unreadable: treat as fresh and leave it alone + } + latest := info.ModTime() + entries, err := os.ReadDir(dir) + if err != nil { + return latest + } + for _, e := range entries { + if fi, err := e.Info(); err == nil && fi.ModTime().After(latest) { + latest = fi.ModTime() + } + } + return latest +} + +// outputUsage totals the spilled output on disk: how many conversations have +// any, and how many bytes they hold. Reporting only; every error is skipped, +// because doctor must describe what it can reach rather than fail on a +// directory it cannot. +func outputUsage() (conversations int, bytes int64) { + entries, err := os.ReadDir(outputsDir()) + if err != nil { + return 0, 0 + } + for _, e := range entries { + if !e.IsDir() { + continue + } + files, err := os.ReadDir(filepath.Join(outputsDir(), e.Name())) + if err != nil { + continue + } + var sub int64 + for _, f := range files { + if fi, err := f.Info(); err == nil { + sub += fi.Size() + } + } + if sub > 0 { + conversations++ + bytes += sub + } + } + return conversations, bytes +} + +// humanBytes renders a size the way a person reads it, for doctor's report. +func humanBytes(n int64) string { + switch { + case n >= 1<<20: + return fmt.Sprintf("%.1f MB", float64(n)/(1<<20)) + case n >= 1<<10: + return fmt.Sprintf("%.1f KB", float64(n)/(1<<10)) + default: + return fmt.Sprintf("%d B", n) + } +} diff --git a/harness/spill_test.go b/harness/spill_test.go new file mode 100644 index 0000000..0962752 --- /dev/null +++ b/harness/spill_test.go @@ -0,0 +1,287 @@ +package harness + +import ( + "fmt" + "os" + "path/filepath" + "strconv" + "strings" + "sync" + "testing" + "time" +) + +// failingRun is a stand-in for the output that motivated the shaper: a long +// verbose test run whose diagnostic sits in the middle and whose verdict is the +// last line. A one-ended cut loses exactly one of the two. +func failingRun(t *testing.T) string { + t.Helper() + var b strings.Builder + for i := 1; i <= 400; i++ { + fmt.Fprintf(&b, "=== RUN TestAlpha%d\n--- PASS: TestAlpha%d (0.00s)\n", i, i) + } + b.WriteString(" a_test.go:403: boom: nil map write at pkg/store.go:88\n") + for i := 1; i <= 400; i++ { + fmt.Fprintf(&b, "=== RUN TestBeta%d\n--- PASS: TestBeta%d (0.00s)\n", i, i) + } + b.WriteString("FAIL\nFAIL\tsigprobe/pkg\t0.013s\n") + return b.String() +} + +// withSpill points the store at a real ~/.sesh/out location under a temp HOME, +// so the confinement carve-out is exercised for real rather than bypassed by a +// bare temp dir. +func withSpill(t *testing.T) { + t.Helper() + t.Setenv("HOME", t.TempDir()) + prev := spill + spill = newOutStore(outputDir(&Session{ID: "testchain"})) + t.Cleanup(func() { spill = prev }) +} + +// A head-only cut keeps the PASS wall and drops the verdict, which is how a +// failing run came to read as a passing one. Both ends must survive. +func TestShapeKeepsDiagnosticAndVerdict(t *testing.T) { + withSpill(t) + full := failingRun(t) + if len(full) <= tune.ResultMaxChars { + t.Fatalf("fixture must exceed the budget: %d <= %d", len(full), tune.ResultMaxChars) + } + got := shape(full) + if !strings.Contains(got, "FAIL\tsigprobe/pkg") { + t.Error("shaped result lost the verdict on the final line") + } + if !strings.Contains(got, "boom: nil map write") { + t.Error("shaped result lost the mid-output diagnostic") + } +} + +func TestShapeRespectsBudget(t *testing.T) { + withSpill(t) + for _, n := range []int{tune.ResultMaxChars + 1, 200_000, 1 << 20} { + got := shape(strings.Repeat("x y z\n", n/6)) + if len(got) > tune.ResultMaxChars { + t.Errorf("input %d: shaped to %d, over the %d budget", n, len(got), tune.ResultMaxChars) + } + } +} + +func TestShapeLeavesSmallOutputUntouched(t *testing.T) { + withSpill(t) + s := "ok\tgithub.com/mike-diff/sesh/harness\t8.0s\n" + if got := shape(s); got != s { + t.Errorf("under-budget output was modified:\n%q", got) + } +} + +// The pointer is only useful if the read tool accepts it. Before the carve-out +// every path under ~/.sesh was refused, so the pointer was a dead end. +func TestShapePointerIsReadable(t *testing.T) { + withSpill(t) + path, err := spill.put(failingRun(t)) + if err != nil { + t.Fatalf("spill: %v", err) + } + if refusal := confineRead(path, false); refusal != "" { + t.Fatalf("spilled output must be readable, got refusal: %s", refusal) + } + // Mutation must stay refused: only the observation side is carved out. + if confine(path, false) == "" { + t.Error("spilled output must not be writable") + } +} + +// The pointer promises an offset. Feeding it back through doRead must land on +// the lines the shaper said it elided. +func TestShapePointerOffsetLandsOnElidedLines(t *testing.T) { + withSpill(t) + full := failingRun(t) + got := shape(full) + + path := betweenPointer(t, got, "full output: ", " (read it with offset ") + offset := numberAfter(t, got, " (read it with offset ") + + window, isErr := doRead(path, false, offset, 1) + if isErr { + t.Fatalf("reading the spilled output failed: %s", window) + } + first := strings.SplitN(window, "\n", 2)[0] + // The line at the promised offset must be the first line NOT in the head. + head := got[:strings.Index(got, "\n... [")] + if strings.Contains(head, first) { + t.Errorf("offset %d landed inside the kept head, not the elided middle:\n%q", offset, first) + } + if !strings.Contains(full, first) { + t.Errorf("offset %d returned a line absent from the full output: %q", offset, first) + } +} + +// Spilling off must still tell the model what it is missing. +func TestShapeWithoutSpillStillReportsElision(t *testing.T) { + withSpill(t) + prev := tune.ResultSpillOff + tune.ResultSpillOff = true + defer func() { tune.ResultSpillOff = prev }() + + got := shape(failingRun(t)) + if strings.Contains(got, "full output:") { + t.Error("spill is off; the result must not advertise a path") + } + if !strings.Contains(got, "elided") { + t.Error("the result must still report that output was elided") + } +} + +// A conversation's key must not move when it hands off, or every pointer +// already in the transcript stops resolving. +func TestOutputDirSurvivesHandoff(t *testing.T) { + s := &Session{ID: "gen0", Cwd: t.TempDir()} + want := outputDir(s) + for i := 1; i <= 3; i++ { + s = seedChain(s, "brief", "ledger", "mech", nil) + if got := outputDir(s); got != want { + t.Fatalf("hop %d moved the output dir: %s != %s", i, got, want) + } + } +} + +// A reply's parallel tool calls and concurrent subagents shape at once. +func TestOutStoreAllocatesDistinctIDsConcurrently(t *testing.T) { + o := newOutStore(t.TempDir()) + const n = 8 + paths := make([]string, n) + var wg sync.WaitGroup + for i := range n { + wg.Add(1) + go func(i int) { + defer wg.Done() + p, err := o.put("body " + strconv.Itoa(i)) + if err != nil { + t.Errorf("put: %v", err) + return + } + paths[i] = p + }(i) + } + wg.Wait() + seen := map[string]bool{} + for _, p := range paths { + if p == "" { + continue + } + if seen[p] { + t.Errorf("duplicate spill path handed out: %s", p) + } + seen[p] = true + } + if len(seen) != n { + t.Errorf("got %d distinct paths, want %d", len(seen), n) + } +} + +// A resumed conversation must not hand out ids that overwrite the last run. +func TestOutStoreResumesSequence(t *testing.T) { + dir := t.TempDir() + o := newOutStore(dir) + first, _ := o.put("a") + reopened := newOutStore(dir) + second, _ := reopened.put("b") + if first == second { + t.Fatalf("reopened store reused %s", first) + } + if body, _ := os.ReadFile(first); string(body) != "a" { + t.Errorf("first spill was overwritten: %q", body) + } +} + +func TestGcOutputKeepsFreshAndCurrent(t *testing.T) { + base := t.TempDir() + t.Setenv("HOME", base) + root := filepath.Join(base, ".sesh", "out") + + mk := func(name string, age time.Duration) string { + d := filepath.Join(root, name) + os.MkdirAll(d, 0o700) + f := filepath.Join(d, "out-1.log") + os.WriteFile(f, []byte("body"), 0o600) + when := time.Now().Add(-age) + os.Chtimes(f, when, when) + os.Chtimes(d, when, when) + return d + } + fresh := mk("fresh", time.Hour) + stale := mk("stale", 30*24*time.Hour) + current := mk("current", 30*24*time.Hour) + + gcOutput("current") + + if _, err := os.Stat(fresh); err != nil { + t.Error("a recently written conversation must be kept") + } + if _, err := os.Stat(current); err != nil { + t.Error("the current conversation must never be collected") + } + if _, err := os.Stat(stale); !os.IsNotExist(err) { + t.Error("output past the age floor must be collected") + } +} + +// The judge rules done on this text. A head-only elision fed it a wall of PASS +// lines from a run that failed. +func TestTranscriptElisionKeepsVerdict(t *testing.T) { + full := failingRun(t) + got := elideText(full, tune.TranscriptResult) + if len(got) > tune.TranscriptResult { + t.Errorf("elided to %d, over the %d budget", len(got), tune.TranscriptResult) + } + if !strings.Contains(got, "FAIL") { + t.Errorf("the judge must see the verdict; got:\n%q", got) + } +} + +func TestCappedBufferKeepsTail(t *testing.T) { + c := &cappedBuffer{max: 64} + c.Write([]byte(strings.Repeat("o", 200))) + c.Write([]byte("VERDICT")) + if !strings.HasSuffix(string(c.buf), "VERDICT") { + t.Errorf("cappedBuffer must keep the tail, got %q", c.buf) + } + if len(c.buf) > 64 { + t.Errorf("cappedBuffer over cap: %d", len(c.buf)) + } + if c.dropped == 0 { + t.Error("dropped bytes must be counted") + } +} + +func betweenPointer(t *testing.T, s, after, before string) string { + t.Helper() + i := strings.Index(s, after) + if i < 0 { + t.Fatalf("pointer prefix %q missing from:\n%s", after, s) + } + rest := s[i+len(after):] + j := strings.Index(rest, before) + if j < 0 { + t.Fatalf("pointer suffix %q missing from:\n%s", before, rest) + } + return rest[:j] +} + +func numberAfter(t *testing.T, s, after string) int { + t.Helper() + i := strings.Index(s, after) + if i < 0 { + t.Fatalf("marker %q missing", after) + } + rest := s[i+len(after):] + end := 0 + for end < len(rest) && rest[end] >= '0' && rest[end] <= '9' { + end++ + } + n, err := strconv.Atoi(rest[:end]) + if err != nil { + t.Fatalf("no number after %q: %v", after, err) + } + return n +} diff --git a/harness/toolmods.go b/harness/toolmods.go index c61d5d9..3d3deb5 100644 --- a/harness/toolmods.go +++ b/harness/toolmods.go @@ -84,11 +84,11 @@ func loadToolMods(taken map[string]bool) ([]agent.Tool, []string) { params = p } } - tools = append(tools, agent.Tool{ + tools = append(tools, shaped(agent.Tool{ Def: agent.ToolDef{Name: name, Description: schema.Description, Schema: params}, Run: runToolMod(path), Parallel: parallel, - }) + })) taken[name] = true } return tools, notes diff --git a/harness/tools.go b/harness/tools.go index 595bfc3..5cd24d4 100644 --- a/harness/tools.go +++ b/harness/tools.go @@ -67,14 +67,14 @@ func builtinTools(unsafePaths bool, pm *procManager) []agent.Tool { }), func(_ context.Context, in toolInput) (string, bool) { return doWrite(in.Path, in.Content, unsafePaths) }), hardenedEditTool(unsafePaths), - def("bash", bashDesc(pm), + shaped(def("bash", bashDesc(pm), obj([]string{"command"}, map[string]any{"command": str("The command to run.")}), func(ctx context.Context, in toolInput) (string, bool) { if pm != nil { return pm.doBash(ctx, in.Command) } return boundedBash(ctx, in.Command) - }), + })), } // proc is a built-in (it claims its name ahead of tool mods), but only when // a supervisor is in play: the top-level session, not subagents or the rig. @@ -259,8 +259,11 @@ func confine(path string, unsafe bool) string { } // readableSeshData carves the archive out of the ~/.sesh refusal: -// sessions and chain ledgers are exactly the files context gets offloaded to, -// so the agent (and its subagents) must always be able to find them again. +// sessions, chain ledgers, and spilled tool output are exactly the files +// context gets offloaded to, so the agent (and its subagents) must always be +// able to find them again. Spilled output belongs here for the same reason the +// transcripts do: the shaped result hands the model a path, and a pointer the +// read tool refuses is worse than no pointer at all. // The key and credentials stay refused; mutation stays refused everywhere // under ~/.sesh (write/edit/bash use confine, not confineRead). func readableSeshData(path string) bool { @@ -268,7 +271,7 @@ func readableSeshData(path string) bool { if err != nil { return false } - for _, dir := range []string{sessionsDir(), chainsDir()} { + for _, dir := range []string{sessionsDir(), chainsDir(), outputsDir()} { d, err := filepath.Abs(dir) if err != nil { continue @@ -408,7 +411,10 @@ func doWrite(path, content string, unsafe bool) (string, bool) { const maxBashOutput = 1 << 20 // cappedBuffer keeps at most max bytes and counts what it had to drop. It -// never errors, so the command runs to completion (or timeout) regardless. +// keeps the TAIL, evicting from the head as the ring in the process supervisor +// does: a command that outgrows the cap is almost always a build or test run, +// whose verdict is on its last lines. It never errors, so the command runs to +// completion (or timeout) regardless. type cappedBuffer struct { buf []byte max int @@ -416,16 +422,11 @@ type cappedBuffer struct { } func (c *cappedBuffer) Write(p []byte) (int, error) { - if room := c.max - len(c.buf); room > 0 { - if len(p) <= room { - c.buf = append(c.buf, p...) - return len(p), nil - } - c.buf = append(c.buf, p[:room]...) - c.dropped += len(p) - room - return len(p), nil + c.buf = append(c.buf, p...) + if over := len(c.buf) - c.max; over > 0 { + c.buf = append([]byte(nil), c.buf[over:]...) + c.dropped += over } - c.dropped += len(p) return len(p), nil } @@ -442,7 +443,7 @@ func boundedBash(ctx context.Context, command string) (string, bool) { err := cmd.Run() s := string(out.buf) if out.dropped > 0 { - s += fmt.Sprintf("\n... [output capped: %d more bytes dropped]", out.dropped) + s = fmt.Sprintf("... [output capped: %d earlier bytes dropped]\n", out.dropped) + s } if err != nil { return strings.TrimSpace(s + "\n" + err.Error()), true diff --git a/harness/tuning.go b/harness/tuning.go index fae3005..da44b27 100644 --- a/harness/tuning.go +++ b/harness/tuning.go @@ -109,6 +109,34 @@ type Tuning struct { // InputMaxRows is how many rows the interactive input editor grows to before // it scrolls vertically with the cursor kept in view. Default 6. InputMaxRows int `json:"input_max_rows,omitempty"` + // ResultMaxChars bounds one tool result, head and tail kept around an + // elided middle whose full text spills to a file the model can read. + // Default 28000: under the core's own hard ceiling, so the shaped result + // (which carries the actionable pointer) is what reaches the window rather + // than a generic byte count. Applies to the tools whose output size the + // harness does not otherwise control: bash, the engines, and tool mods. + ResultMaxChars int `json:"result_max_chars,omitempty"` + // ResultHeadPct is how much of that budget goes to the head; the rest is + // the tail. Default 25: a failing command's diagnostic can sit anywhere in + // the output but its verdict is always on the last lines, so the tail earns + // the larger share. + ResultHeadPct int `json:"result_head_pct,omitempty"` + // ResultSpillOff drops the on-disk copy of an over-budget result (the + // shaped head and tail still reach the model, and still report the elided + // line range, just without a path to recover it). Default off, so output is + // recoverable. Inverted so the zero value keeps the default, like every dial. + ResultSpillOff bool `json:"result_spill_off,omitempty"` + // ResultKeepDays is how long a conversation's spilled output outlives its + // last write. Default 7. Collecting early only costs a pointer in a resumed + // transcript the ability to resolve, which the model reads as an ordinary + // missing file. + ResultKeepDays int `json:"result_keep_days,omitempty"` + // TranscriptResult is the per-result budget in the transcript the judge and + // the brief writer read, head and tail kept around an elided middle. + // Default 300. Head-only elision here is what let a failing test run reach + // the judge as a wall of PASS lines, so both ends are kept for the same + // reason the tool result keeps both. + TranscriptResult int `json:"transcript_result,omitempty"` } func defaultTuning() Tuning { @@ -130,6 +158,10 @@ func defaultTuning() Tuning { MaxProcs: 10, ProcLogTail: 200, InputMaxRows: 6, + ResultMaxChars: 28000, + ResultHeadPct: 25, + ResultKeepDays: 7, + TranscriptResult: 300, } } @@ -226,6 +258,10 @@ func overlayTuning(t *Tuning, got Tuning) { set(&t.MaxProcs, got.MaxProcs) set(&t.ProcLogTail, got.ProcLogTail) set(&t.InputMaxRows, got.InputMaxRows) + set(&t.ResultMaxChars, got.ResultMaxChars) + set(&t.ResultHeadPct, got.ResultHeadPct) + set(&t.ResultKeepDays, got.ResultKeepDays) + set(&t.TranscriptResult, got.TranscriptResult) // DiffLines accepts -1 (disable), so its overlay applies on any nonzero. if got.DiffLines != 0 { t.DiffLines = got.DiffLines @@ -237,6 +273,9 @@ func overlayTuning(t *Tuning, got Tuning) { if got.ProcSpillOff { t.ProcSpillOff = true } + if got.ResultSpillOff { + t.ResultSpillOff = true + } if got.UpdateCheck { t.UpdateCheck = true } From bf6f95bed6793651e7c9cdb2ee4a4ed174ed448e Mon Sep 17 00:00:00 2001 From: mike-diff Date: Wed, 19 Aug 2026 22:19:45 -0700 Subject: [PATCH 2/2] ci: run tests under the race detector --- .github/workflows/ci.yml | 5 ++++- AGENTS.md | 5 ++++- README.md | 2 +- 3 files changed, 9 insertions(+), 3 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 9a6560b..1304263 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -12,8 +12,11 @@ jobs: run: test -z "$(gofmt -l .)" || { gofmt -l .; exit 1; } - name: vet run: go vet ./... + # -race, not bare go test: parallel tool calls, concurrent subagents, the + # process supervisor, and the TUI's signal handlers all share state, so a + # data race here is a lost keystroke or a hang, not a theoretical finding. - name: test - run: go test ./... + run: go test -race ./... # The end-to-end rig: the built binary, headless, against a scripted # endpoint. Catches assembled-product regressions unit tests cannot # (wire shape, gates, drive lifecycle, session persistence). diff --git a/AGENTS.md b/AGENTS.md index 55a3a62..ad90022 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -205,7 +205,10 @@ write results carry the diff they applied. ## Working in this repo - Verify before claiming done: `go build ./...`, `go vet ./...`, and - `go test ./...` must all pass. After changing a tool, exercise it with `bash`. + `go test -race ./...` must all pass. The race detector is part of the bar + because concurrency is part of the design: parallel tool calls, concurrent + subagents reporting usage, the process supervisor, and the TUI's signal + handlers all share state. After changing a tool, exercise it with `bash`. - **Tests earn their keep by failing.** Passing is not the bar; agents reliably write tests that pass. Before keeping any test, name the one-line code change that would make it fail. Cannot name one: it proves nothing, diff --git a/README.md b/README.md index 42bb91b..9294fbb 100644 --- a/README.md +++ b/README.md @@ -195,7 +195,7 @@ The core exposes one extension surface (a `Hooks` struct plus tools-as-values) a ## Development ```sh -gofmt -l . && go vet ./... && go test ./... +gofmt -l . && go vet ./... && go test -race ./... ``` CI enforces all three. Keep the core pure: if a change makes `agent/` import a provider, read input, or print, it is in the wrong package.