From 167824f4641539956da6a50d033f9214c10318fe Mon Sep 17 00:00:00 2001 From: Dylan Richard Date: Fri, 2 Oct 2026 19:07:49 -0500 Subject: [PATCH 1/8] feat: tool-payload storage policy as a shared package MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Issue #28 settles what ccvault keeps of a tool call: inputs whole and indexed, non-bulk results whole and indexed, bulk file reads and image payloads reduced to a length. The decision has to be identical for all five sources, and the code that extracts payloads is split between pkg/parser (claude-code, nanoclaw) and the individual adapters (codex, jeff) with no shared layer underneath, so the policy gets its own package rather than being written twice. Re-measured against the whole 965,061-turn archive rather than the 2,824-block sample the issue was specified from. The sample held up on the question that mattered — nothing non-bulk comes near a size that needs capping — but got the proportions wrong, so the constants and comments here carry the full-corpus numbers: inputs 259,836 calls 78.6 MB p50 72 B max 99,792 non-bulk result 211,892 results 397 MB p50 525 B max 51,557 bulk file reads 47,444 results 205 MB max 67,313 image payloads 476 results 54.9 MB max 512,497 The 128 KB backstop is confirmed as a backstop: not one of the 211,892 non-bulk results exceeds even 64 KB, so it fires on nothing that exists and exists only for the tool nobody has classified yet. Co-Authored-By: Claude Opus 5 --- pkg/toolpayload/toolpayload.go | 163 ++++++++++++++++++++++ pkg/toolpayload/toolpayload_test.go | 205 ++++++++++++++++++++++++++++ 2 files changed, 368 insertions(+) create mode 100644 pkg/toolpayload/toolpayload.go create mode 100644 pkg/toolpayload/toolpayload_test.go diff --git a/pkg/toolpayload/toolpayload.go b/pkg/toolpayload/toolpayload.go new file mode 100644 index 0000000..5e870a5 --- /dev/null +++ b/pkg/toolpayload/toolpayload.go @@ -0,0 +1,163 @@ +// ABOUTME: The storage policy for tool payloads — what gets stored whole, what is reduced to a length. +// ABOUTME: Shared by pkg/parser and every adapter so all five sources classify a result the same way. + +// Package toolpayload decides what ccvault keeps of a tool call's input and +// result. It exists as its own package because the decision has to be the same +// for all five sources, and the code that extracts them is split between +// pkg/parser (Claude Code and nanoclaw) and the individual adapters (codex, +// jeff) with no shared layer below both. +// +// The policy, settled on issue #28 from measurement rather than preference: +// +// - Tool inputs are stored whole and indexed. Across 259,836 calls in the +// author's archive they total 78.6 MB, p50 72 bytes, largest 99,792. +// - Tool results are stored whole and indexed, except where the content is +// bulk or unindexable. Across 211,892 such results they total 397 MB, +// p50 525 bytes, largest 51,557. +// - Bulk file reads and image payloads keep only their length. Their text is +// still in turns.raw_json, and the row carries file_path already, so +// nothing is lost — but 205 MB of file dumps and 55 MB of base64 in the FTS +// index would wreck relevance for the shell commands and error messages +// this material exists to make searchable. +// +// Nothing is ever stored truncated to a prefix. raw_json keeps the full +// original regardless, so a prefix costs storage without preserving anything: +// the choices are store-whole and store-metadata-only. +package toolpayload + +import ( + "encoding/json" + "strings" +) + +// MaxStoredResultBytes is a backstop, not a cap in the ordinary sense. Every +// result class that exists today is classified by shape or by tool, and the +// largest non-bulk result measured across 211,892 of them is 51,557 bytes — +// so this fires on nothing in the archive. It is here for the tool nobody has +// classified yet: a future Bash wrapper that returns a whole file, an MCP +// server that answers with a megabyte of JSON. Uncapped in principle, bounded +// in practice. +const MaxStoredResultBytes = 128 * 1024 + +// Reasons a result's content was left out. The empty string means it was +// stored. A consumer reads these rather than re-deriving the classification +// from tool_name and result_length, which is the guessing issue #28 set out to +// remove. +const ( + // OmitBulkRead: a bulk file reader. The row's file_path says what was + // read and result_length says how much came back. + OmitBulkRead = "bulk_read" + // OmitImage: the content carried an image block. Detected by shape, so + // it catches MCP tools whose names say nothing about images. + OmitImage = "image" + // OmitOversize: over MaxStoredResultBytes. + OmitOversize = "oversize" + // OmitUndecodable: the content was not a shape this package knows how to + // read. The length is still recorded; raw_json still has the original. + OmitUndecodable = "undecodable" +) + +// bulkReadTools are the tools whose results are file contents rather than +// anything a search over tool output should rank. Matched by name, unlike +// images: these are a fixed, known set of Claude Code tools, and the backstop +// above covers the unknown ones. +var bulkReadTools = map[string]bool{ + "Read": true, + "NotebookRead": true, +} + +// IsBulkReadTool reports whether a tool's results are bulk file contents. +func IsBulkReadTool(toolName string) bool { + return bulkReadTools[toolName] +} + +// Result is the decision for one tool result. +type Result struct { + // Content is the result text to store, empty when the content was left + // out. Also empty, legitimately, when the tool returned nothing — read + // OmitReason to tell those apart. + Content string + + // Length is the size of the result as the transcript carries it: the + // string's bytes for a string content, the JSON encoding's bytes for a + // structured one. Recorded whether or not Content was stored, which is + // what lets a consumer distinguish a short result from an omitted one. + // + // For a structured content this runs ahead of len(Content) by the JSON + // framing — about 19% across the archive's 130,516 array-valued results. + // It is deliberately the transcript's number rather than the extraction's, + // because an image result extracts to no text at all and a zero there + // would destroy the only signal the row has left. + Length int + + // OmitReason names why Content was left out, empty when it was stored. + OmitReason string +} + +// resultContentBlock is one element of a structured tool_result content array. +type resultContentBlock struct { + Type string `json:"type"` + Text string `json:"text"` +} + +// ResultFromJSON applies the policy to a Claude Code tool_result content +// field, which the transcripts write either as a JSON string or as an array of +// content blocks. +func ResultFromJSON(toolName string, rawContent json.RawMessage) Result { + trimmed := strings.TrimSpace(string(rawContent)) + if trimmed == "" || trimmed == "null" { + return Result{} + } + + // A JSON string: the length is the string's own bytes, so a stored result + // has Length == len(Content). + var text string + if err := json.Unmarshal(rawContent, &text); err == nil { + return decide(toolName, text, len(text), false) + } + + // An array of content blocks. Length is the array's JSON bytes — see the + // Result.Length comment for why the transcript's number is the right one. + var blocks []resultContentBlock + if err := json.Unmarshal(rawContent, &blocks); err == nil { + hasImage := false + var parts []string + for _, b := range blocks { + switch b.Type { + case "image": + hasImage = true + case "text": + if b.Text != "" { + parts = append(parts, b.Text) + } + } + } + return decide(toolName, strings.Join(parts, "\n"), len(rawContent), hasImage) + } + + return Result{Length: len(rawContent), OmitReason: OmitUndecodable} +} + +// ResultFromText applies the policy to a tool result the source records as a +// bare string — codex writes function_call_output.output that way, jeff writes +// tool_result.output_preview. No image shape is reachable here, so only the +// bulk-tool and backstop rules can fire. +func ResultFromText(toolName, text string) Result { + return decide(toolName, text, len(text), false) +} + +// decide is the policy itself, with the shapes already resolved. Image first: +// it is the shape-based rule and the most specific thing true about a result +// that carries one. Then the bulk tools, then the backstop. +func decide(toolName, content string, length int, hasImage bool) Result { + switch { + case hasImage: + return Result{Length: length, OmitReason: OmitImage} + case IsBulkReadTool(toolName): + return Result{Length: length, OmitReason: OmitBulkRead} + case length > MaxStoredResultBytes: + return Result{Length: length, OmitReason: OmitOversize} + default: + return Result{Content: content, Length: length} + } +} diff --git a/pkg/toolpayload/toolpayload_test.go b/pkg/toolpayload/toolpayload_test.go new file mode 100644 index 0000000..e16d372 --- /dev/null +++ b/pkg/toolpayload/toolpayload_test.go @@ -0,0 +1,205 @@ +// ABOUTME: Tests for the tool-payload storage policy — which results get stored whole and which are reduced to a length. +// ABOUTME: Cases are drawn from the shapes measured in the real archive: string contents, text arrays, image blocks. + +package toolpayload + +import ( + "encoding/json" + "strings" + "testing" +) + +// TestResultFromJSON_StringContentStoredWhole covers the common case: a +// tool_result whose content is a plain JSON string. It is stored verbatim and +// its length is the string's bytes, so Length == len(Content) exactly. +func TestResultFromJSON_StringContentStoredWhole(t *testing.T) { + got := ResultFromJSON("Bash", json.RawMessage(`"total 4\ndrwxr-xr-x 2 dylanr staff"`)) + + want := "total 4\ndrwxr-xr-x 2 dylanr staff" + if got.Content != want { + t.Errorf("Content = %q, want %q", got.Content, want) + } + if got.Length != len(want) { + t.Errorf("Length = %d, want %d", got.Length, len(want)) + } + if got.OmitReason != "" { + t.Errorf("OmitReason = %q, want empty", got.OmitReason) + } +} + +// TestResultFromJSON_TextArrayConcatenated covers the other half of the +// archive's results: content is an array of blocks. The text blocks are +// concatenated; Length is the content JSON's bytes, which is what the +// transcript actually carries, so it runs ahead of len(Content) by the JSON +// framing. +func TestResultFromJSON_TextArrayConcatenated(t *testing.T) { + raw := json.RawMessage(`[{"type":"text","text":"first"},{"type":"text","text":"second"}]`) + got := ResultFromJSON("Bash", raw) + + if got.Content != "first\nsecond" { + t.Errorf("Content = %q, want %q", got.Content, "first\nsecond") + } + if got.Length != len(raw) { + t.Errorf("Length = %d, want %d (the content JSON's bytes)", got.Length, len(raw)) + } + if got.OmitReason != "" { + t.Errorf("OmitReason = %q, want empty", got.OmitReason) + } +} + +// TestResultFromJSON_ImageDetectedByShape pins the rule from issue #28: an +// image payload is recognised by a {"type":"image"} block in the content, not +// by the tool's name. The tool here is an MCP tool whose name says nothing +// about images, and the base64 must not be stored or indexed. +func TestResultFromJSON_ImageDetectedByShape(t *testing.T) { + needle := "BASE64PAYLOADCANARYAAAAAAAAAAAAAAAAAA" + raw := json.RawMessage(`[{"type":"image","source":{"type":"base64","media_type":"image/png","data":"` + needle + `"}}]`) + + got := ResultFromJSON("mcp__some__arbitrary_name", raw) + + if strings.Contains(got.Content, needle) { + t.Fatalf("image payload was stored: %q", got.Content) + } + if got.Content != "" { + t.Errorf("Content = %q, want empty", got.Content) + } + if got.OmitReason != OmitImage { + t.Errorf("OmitReason = %q, want %q", got.OmitReason, OmitImage) + } + if got.Length != len(raw) { + t.Errorf("Length = %d, want %d — the length must survive even though the content does not", got.Length, len(raw)) + } +} + +// TestResultFromJSON_ImageBesideTextStillOmitted covers a mixed content array. +// One image block is enough: storing the sibling text while silently dropping +// the image would make the stored result a misleading partial. +func TestResultFromJSON_ImageBesideTextStillOmitted(t *testing.T) { + raw := json.RawMessage(`[{"type":"text","text":"here is the screenshot"},{"type":"image","source":{"data":"AAAA"}}]`) + + got := ResultFromJSON("Bash", raw) + + if got.Content != "" { + t.Errorf("Content = %q, want empty", got.Content) + } + if got.OmitReason != OmitImage { + t.Errorf("OmitReason = %q, want %q", got.OmitReason, OmitImage) + } +} + +// TestResultFromJSON_BulkReadKeepsLengthOnly covers the bulk file readers. The +// file path is already extracted onto the row, so a length is enough to keep +// the row self-describing, and the full text stays in raw_json. +func TestResultFromJSON_BulkReadKeepsLengthOnly(t *testing.T) { + for _, tool := range []string{"Read", "NotebookRead"} { + body := strings.Repeat("x", 5000) + raw := json.RawMessage(`"` + body + `"`) + + got := ResultFromJSON(tool, raw) + + if got.Content != "" { + t.Errorf("%s: Content length = %d, want empty", tool, len(got.Content)) + } + if got.OmitReason != OmitBulkRead { + t.Errorf("%s: OmitReason = %q, want %q", tool, got.OmitReason, OmitBulkRead) + } + if got.Length != len(body) { + t.Errorf("%s: Length = %d, want %d", tool, got.Length, len(body)) + } + } +} + +// TestResultFromJSON_BackstopOmitsOversize covers the 128 KB backstop. No +// non-bulk result in the measured archive comes close to it, so this is the +// unknown-future-tool case: a tool nobody has classified that returns +// something enormous must not be stored or indexed. +func TestResultFromJSON_BackstopOmitsOversize(t *testing.T) { + body := strings.Repeat("y", MaxStoredResultBytes+1) + got := ResultFromJSON("SomeFutureTool", json.RawMessage(`"`+body+`"`)) + + if got.Content != "" { + t.Errorf("Content length = %d, want empty", len(got.Content)) + } + if got.OmitReason != OmitOversize { + t.Errorf("OmitReason = %q, want %q", got.OmitReason, OmitOversize) + } + if got.Length != len(body) { + t.Errorf("Length = %d, want %d", got.Length, len(body)) + } +} + +// TestResultFromJSON_AtBackstopStoredWhole pins the boundary: the backstop is +// a ceiling the policy allows, not one it rejects. +func TestResultFromJSON_AtBackstopStoredWhole(t *testing.T) { + body := strings.Repeat("z", MaxStoredResultBytes) + got := ResultFromJSON("SomeFutureTool", json.RawMessage(`"`+body+`"`)) + + if got.OmitReason != "" { + t.Errorf("OmitReason = %q, want empty at exactly the backstop", got.OmitReason) + } + if len(got.Content) != MaxStoredResultBytes { + t.Errorf("stored %d bytes, want %d", len(got.Content), MaxStoredResultBytes) + } +} + +// TestResultFromJSON_EmptyResultIsNotAnOmission separates "the tool returned +// nothing" from "the content was left out", which is the distinction issue #28 +// asks a consumer to be able to make without guessing. +func TestResultFromJSON_EmptyResultIsNotAnOmission(t *testing.T) { + got := ResultFromJSON("Bash", json.RawMessage(`""`)) + + if got.OmitReason != "" { + t.Errorf("OmitReason = %q, want empty", got.OmitReason) + } + if got.Length != 0 { + t.Errorf("Length = %d, want 0", got.Length) + } +} + +// TestResultFromJSON_MalformedContentKeepsItsLength covers content the policy +// cannot decode. It must not be stored (nothing sane to store) but the length +// is still a fact worth keeping. +func TestResultFromJSON_MalformedContentKeepsItsLength(t *testing.T) { + raw := json.RawMessage(`{"unexpected":"object"}`) + got := ResultFromJSON("Bash", raw) + + if got.Content != "" { + t.Errorf("Content = %q, want empty", got.Content) + } + if got.Length != len(raw) { + t.Errorf("Length = %d, want %d", got.Length, len(raw)) + } + if got.OmitReason != OmitUndecodable { + t.Errorf("OmitReason = %q, want %q", got.OmitReason, OmitUndecodable) + } +} + +// TestResultFromText covers the adapters whose transcripts carry a tool result +// as a bare string (codex, jeff) rather than a Claude Code content block. The +// same policy applies, minus the shapes that cannot occur. +func TestResultFromText(t *testing.T) { + got := ResultFromText("exec_command", "Process exited with code 0") + if got.Content != "Process exited with code 0" || got.Length != 26 || got.OmitReason != "" { + t.Errorf("ResultFromText = %+v", got) + } + + big := ResultFromText("exec_command", strings.Repeat("q", MaxStoredResultBytes+1)) + if big.OmitReason != OmitOversize || big.Content != "" { + t.Errorf("oversize text result = %+v", big) + } + + bulk := ResultFromText("Read", "file contents") + if bulk.OmitReason != OmitBulkRead || bulk.Content != "" { + t.Errorf("bulk text result = %+v", bulk) + } +} + +// TestMaxStoredResultBytes guards the backstop against a well-meaning edit. +// It is set from measurement: the largest non-bulk, non-image result in the +// author's 965,061-turn archive is 51,557 bytes, so this leaves 2.5x headroom +// and fires on nothing that exists today. +func TestMaxStoredResultBytes(t *testing.T) { + if MaxStoredResultBytes != 128*1024 { + t.Errorf("MaxStoredResultBytes = %d, want %d", MaxStoredResultBytes, 128*1024) + } +} From 202c1c0a1a079379aa830c436ca5d59e98512fdb Mon Sep 17 00:00:00 2001 From: Dylan Richard Date: Fri, 2 Oct 2026 19:16:25 -0500 Subject: [PATCH 2/8] feat: the parser and all five adapters carry tool payloads instead of dropping them MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit pkg/parser had every tool call's input in hand and used it for one thing — extracting a file path — then let it go. Tool results it recognised only to set a has_error flag. models.ToolUse had nowhere to put either, and adapter.ParsedToolUse was two fields wide. ExtractToolUses now runs two passes, because a call and its result live in different turns: Claude Code writes the tool_use into an assistant message and the tool_result into a later user message, joined by tool_use_id. Pairing by position would cross results from one turn's concurrent calls, which real transcripts produce. How the five sources express the link, checked against real data rather than inferred from the adapters: claude-code content block id "toolu_…"; result block tool_use_id nanoclaw identical — same JSONL, same parser codex payload call_id on both halves, for two tool shapes jeff data.tool_id on both halves, EMPTY on 633 of 742 requests hex no tool calls at all; messages are role/content/timestamp Jeff's mostly-empty tool_id is why it links by order instead: keying on an id would collapse 85% of its calls onto each other. Its files pair requests and results one for one, so the oldest unanswered call is a sound fallback, and an exact tool_id match still wins when jeff recorded one. Codex's custom_tool_call is now recorded. The adapter skipped that payload kind outright, which meant apply_patch — codex's edit tool — left no trace in the archive at all. It is wired here rather than filed separately because its output half is one of the two payload kinds this change has to start reading, and leaving one of a symmetric pair unhandled is harder to explain than handling both. The three places that copy these fields — two adapters and the sync layer — go through one conversion pair in pkg/adapter rather than nine field assignments each, and a reflection test walks the structs so a field added without being threaded through fails. That is the failure shape from PR #35: compiles, lints clean, carries the wrong data. turns.content is deliberately untouched. models.UserContentBlock.Content is typed as a string, so a turn carrying an array-valued tool_result fails the whole content unmarshal and stores "" — which is how image base64 has been kept out of turns_fts, accidentally. Retyping that field would rewrite turns.content for a third of the archive, so the new payloads reach search through their own index instead. Filing the accident separately. Co-Authored-By: Claude Opus 5 --- internal/sync/sync.go | 8 +- pkg/adapter/adapter.go | 17 +- pkg/adapter/claudecode/claudecode.go | 5 +- pkg/adapter/claudecode/toolpayloads_test.go | 61 +++++ pkg/adapter/codex/codex.go | 96 +++++++- pkg/adapter/codex/toolpayloads_test.go | 203 +++++++++++++++++ pkg/adapter/jeff/jeff.go | 85 ++++++- pkg/adapter/jeff/toolpayloads_test.go | 158 +++++++++++++ pkg/adapter/nanoclaw/nanoclaw.go | 5 +- pkg/adapter/toolpayloads.go | 54 +++++ pkg/adapter/toolpayloads_test.go | 85 +++++++ pkg/models/models.go | 40 ++++ pkg/parser/parser.go | 81 ++++++- pkg/parser/toolpayloads_test.go | 232 ++++++++++++++++++++ 14 files changed, 1103 insertions(+), 27 deletions(-) create mode 100644 pkg/adapter/claudecode/toolpayloads_test.go create mode 100644 pkg/adapter/codex/toolpayloads_test.go create mode 100644 pkg/adapter/jeff/toolpayloads_test.go create mode 100644 pkg/adapter/toolpayloads.go create mode 100644 pkg/adapter/toolpayloads_test.go create mode 100644 pkg/parser/toolpayloads_test.go diff --git a/internal/sync/sync.go b/internal/sync/sync.go index 80bed9a..0218d17 100644 --- a/internal/sync/sync.go +++ b/internal/sync/sync.go @@ -437,13 +437,7 @@ func (s *Syncer) processSession(ctx context.Context, sf adapter.SessionFile, adp // Convert tool uses for _, ptu := range pt.ToolUses { - toolUses = append(toolUses, models.ToolUse{ - TurnID: pt.ID, - SessionID: parsed.ID, - ToolName: ptu.ToolName, - FilePath: ptu.FilePath, - Timestamp: pt.Timestamp, - }) + toolUses = append(toolUses, adapter.ToolUseFromParsed(ptu, pt.ID, parsed.ID, pt.Timestamp)) } } diff --git a/pkg/adapter/adapter.go b/pkg/adapter/adapter.go index 0f309a1..90c5c47 100644 --- a/pkg/adapter/adapter.go +++ b/pkg/adapter/adapter.go @@ -51,10 +51,25 @@ type ParsedTurn struct { HasError bool } -// ParsedToolUse captures a single tool invocation within a turn. +// ParsedToolUse captures a single tool invocation within a turn, including the +// payloads it carried. See models.ToolUse for what each field means and +// pkg/toolpayload for the policy that decides whether a result's content is +// stored or reduced to its length. +// +// A source that records none of a field leaves it zero — hex has no tool calls +// at all, and jeff records no usable id for 85% of its requests. type ParsedToolUse struct { ToolName string FilePath string + + ToolUseID string + InputJSON string + InputLength int + + HasResult bool + ResultContent string + ResultLength int + ResultOmittedReason string } // SourceAdapter is the interface that all conversation source backends must implement. diff --git a/pkg/adapter/claudecode/claudecode.go b/pkg/adapter/claudecode/claudecode.go index 99352b5..120efaf 100644 --- a/pkg/adapter/claudecode/claudecode.go +++ b/pkg/adapter/claudecode/claudecode.go @@ -104,10 +104,7 @@ func (a *Adapter) Parse(path string) (*adapter.ParsedSession, error) { // Convert tool uses for this turn if tus, ok := toolUsesByTurn[t.ID]; ok { for _, tu := range tus { - pt.ToolUses = append(pt.ToolUses, adapter.ParsedToolUse{ - ToolName: tu.ToolName, - FilePath: tu.FilePath, - }) + pt.ToolUses = append(pt.ToolUses, adapter.ParsedToolUseFromModel(tu)) // Subagent detection: any tool use with ToolName == "Task" if tu.ToolName == "Task" { hasSubagent = true diff --git a/pkg/adapter/claudecode/toolpayloads_test.go b/pkg/adapter/claudecode/toolpayloads_test.go new file mode 100644 index 0000000..e155744 --- /dev/null +++ b/pkg/adapter/claudecode/toolpayloads_test.go @@ -0,0 +1,61 @@ +// ABOUTME: Tests that the claude-code adapter carries the parser's tool payloads through to ParsedToolUse. +// ABOUTME: The payload extraction itself is tested in pkg/parser; this pins that the adapter does not drop it. + +package claudecode + +import ( + "os" + "path/filepath" + "strings" + "testing" +) + +// TestParse_CarriesToolPayloadsThrough covers the hand-off the adapter makes. +// The parser extracts the id, input, and result; this layer converts +// models.ToolUse to adapter.ParsedToolUse field by field, and a field left out +// of that conversion is silently empty the whole way to the database. +func TestParse_CarriesToolPayloadsThrough(t *testing.T) { + transcript := `{"uuid":"a-1","sessionId":"s-cc","type":"assistant","cwd":"/tmp/proj","timestamp":"2026-09-01T10:00:00.000Z","message":{"model":"claude","role":"assistant","content":[{"type":"tool_use","id":"toolu_CC1","name":"Bash","input":{"command":"go test ./..."}}]}} +{"uuid":"u-1","sessionId":"s-cc","type":"user","timestamp":"2026-09-01T10:00:01.000Z","message":{"role":"user","content":[{"type":"tool_result","tool_use_id":"toolu_CC1","content":"ok github.com/2389-research/ccvault"}]}} +` + path := filepath.Join(t.TempDir(), "s-cc.jsonl") + if err := os.WriteFile(path, []byte(transcript), 0o644); err != nil { + t.Fatal(err) + } + + parsed, err := New().Parse(path) + if err != nil { + t.Fatalf("Parse: %v", err) + } + + var found bool + for _, turn := range parsed.Turns { + for _, tu := range turn.ToolUses { + found = true + if tu.ToolUseID != "toolu_CC1" { + t.Errorf("ToolUseID = %q, want %q", tu.ToolUseID, "toolu_CC1") + } + if !strings.Contains(tu.InputJSON, "go test ./...") { + t.Errorf("InputJSON = %q, want the command", tu.InputJSON) + } + if tu.InputLength != len(tu.InputJSON) { + t.Errorf("InputLength = %d, len = %d", tu.InputLength, len(tu.InputJSON)) + } + if !tu.HasResult { + t.Error("HasResult = false, want true") + } + if want := "ok github.com/2389-research/ccvault"; tu.ResultContent != want { + t.Errorf("ResultContent = %q, want %q", tu.ResultContent, want) + } + if tu.ResultLength != len("ok github.com/2389-research/ccvault") { + t.Errorf("ResultLength = %d, want %d", tu.ResultLength, len("ok github.com/2389-research/ccvault")) + } + if tu.ResultOmittedReason != "" { + t.Errorf("ResultOmittedReason = %q, want empty", tu.ResultOmittedReason) + } + } + } + if !found { + t.Fatal("no tool uses on any turn") + } +} diff --git a/pkg/adapter/codex/codex.go b/pkg/adapter/codex/codex.go index 8fe5010..8a0ff75 100644 --- a/pkg/adapter/codex/codex.go +++ b/pkg/adapter/codex/codex.go @@ -15,6 +15,7 @@ import ( "time" "github.com/2389-research/ccvault/pkg/adapter" + "github.com/2389-research/ccvault/pkg/toolpayload" ) func init() { @@ -62,10 +63,34 @@ type messagePayload struct { } `json:"content"` } -// functionCallPayload holds the fields from a response_item function_call payload. +// functionCallPayload holds the fields from a response_item tool-call payload. +// +// Codex writes two shapes. `function_call` carries its arguments as a JSON +// string in `arguments`; `custom_tool_call` — which is how apply_patch, the +// edit tool, arrives — carries a plain-text body in `input`. Both identify the +// call with `call_id`, and both are answered by a matching `*_output` payload +// carrying the same id. type functionCallPayload struct { - Type string `json:"type"` - Name string `json:"name"` + Type string `json:"type"` + Name string `json:"name"` + CallID string `json:"call_id"` + Arguments string `json:"arguments"` + Input string `json:"input"` +} + +// payload returns the call's arguments, whichever field the shape used. +func (p functionCallPayload) payload() string { + if p.Arguments != "" { + return p.Arguments + } + return p.Input +} + +// functionCallOutputPayload holds the fields from a response_item +// function_call_output or custom_tool_call_output payload. +type functionCallOutputPayload struct { + CallID string `json:"call_id"` + Output string `json:"output"` } // turnContextPayload holds the fields from a turn_context payload. @@ -186,6 +211,14 @@ func (a *Adapter) Parse(path string) (*adapter.ParsedSession, error) { // Track last total token counts to compute per-turn deltas prevInputTokens int64 prevOutputTokens int64 + + // A call and the output answering it are separate lines joined by + // call_id, so both halves are collected as they come and matched after + // the file is read. Resolving inline would only work while outputs + // follow their calls, which is true of the files on disk today and is + // not something the format promises. + outputs = make(map[string]string) + callSites []codexCallSite ) turnCounter := 0 @@ -288,7 +321,7 @@ func (a *Adapter) Parse(path string) (*adapter.ParsedSession, error) { lastAssistantIdx = len(turns) - 1 } - case "function_call": + case "function_call", "custom_tool_call": var fc functionCallPayload if err := json.Unmarshal(line.Payload, &fc); err != nil { continue @@ -296,14 +329,36 @@ func (a *Adapter) Parse(path string) (*adapter.ParsedSession, error) { // Attach to the last assistant turn if lastAssistantIdx >= 0 && lastAssistantIdx < len(turns) { + input := fc.payload() turns[lastAssistantIdx].ToolUses = append( turns[lastAssistantIdx].ToolUses, - adapter.ParsedToolUse{ToolName: fc.Name}, + adapter.ParsedToolUse{ + ToolName: fc.Name, + ToolUseID: fc.CallID, + InputJSON: input, + InputLength: len(input), + }, ) + if fc.CallID != "" { + callSites = append(callSites, codexCallSite{ + callID: fc.CallID, + turnIdx: lastAssistantIdx, + useIdx: len(turns[lastAssistantIdx].ToolUses) - 1, + }) + } + } + + case "function_call_output", "custom_tool_call_output": + var out functionCallOutputPayload + if err := json.Unmarshal(line.Payload, &out); err != nil { + continue + } + if out.CallID != "" { + outputs[out.CallID] = out.Output } - case "reasoning", "function_call_output": - // Skip reasoning blocks and function call outputs + case "reasoning": + // Skip reasoning blocks continue } @@ -333,6 +388,8 @@ func (a *Adapter) Parse(path string) (*adapter.ParsedSession, error) { } } + attachCodexOutputs(turns, callSites, outputs) + return &adapter.ParsedSession{ ID: sessionID, ProjectPath: projectPath, @@ -347,6 +404,31 @@ func (a *Adapter) Parse(path string) (*adapter.ParsedSession, error) { }, nil } +// codexCallSite records where a tool use landed so the output answering it can +// be attached once the whole file has been read. +type codexCallSite struct { + callID string + turnIdx int + useIdx int +} + +// attachCodexOutputs matches each recorded call to the output carrying its +// call_id and applies the storage policy to it. +func attachCodexOutputs(turns []adapter.ParsedTurn, sites []codexCallSite, outputs map[string]string) { + for _, site := range sites { + output, ok := outputs[site.callID] + if !ok { + continue + } + tu := &turns[site.turnIdx].ToolUses[site.useIdx] + decided := toolpayload.ResultFromText(tu.ToolName, output) + tu.HasResult = true + tu.ResultContent = decided.Content + tu.ResultLength = decided.Length + tu.ResultOmittedReason = decided.OmitReason + } +} + // displayNameFromPath returns a shortened display name from a project path. func displayNameFromPath(projectPath string) string { if projectPath == "" { diff --git a/pkg/adapter/codex/toolpayloads_test.go b/pkg/adapter/codex/toolpayloads_test.go new file mode 100644 index 0000000..017a42e --- /dev/null +++ b/pkg/adapter/codex/toolpayloads_test.go @@ -0,0 +1,203 @@ +// ABOUTME: Tests that the codex adapter carries call_id, arguments, and output onto each tool use. +// ABOUTME: Covers both of codex's tool shapes — function_call/function_call_output and custom_tool_call/custom_tool_call_output. + +package codex + +import ( + "path/filepath" + "strings" + "testing" + + "github.com/2389-research/ccvault/pkg/adapter" +) + +// parseLines writes lines to a temp session file and parses it. +func parseLines(t *testing.T, lines []map[string]any) *adapter.ParsedSession { + t.Helper() + fpath := filepath.Join(t.TempDir(), "rollout-test.jsonl") + writeJSONLFile(t, fpath, lines) + parsed, err := New().Parse(fpath) + if err != nil { + t.Fatalf("Parse: %v", err) + } + return parsed +} + +// allToolUses flattens the tool uses across a session's turns. +func allToolUses(s *adapter.ParsedSession) []adapter.ParsedToolUse { + var out []adapter.ParsedToolUse + for _, turn := range s.Turns { + out = append(out, turn.ToolUses...) + } + return out +} + +func codexBaseLines() []map[string]any { + return []map[string]any{ + { + "timestamp": "2026-03-11T16:25:01.231Z", + "type": "session_meta", + "payload": map[string]any{"id": "sess-1", "cwd": "/Users/someone/project"}, + }, + { + "timestamp": "2026-03-11T16:25:07.982Z", + "type": "response_item", + "payload": map[string]any{ + "type": "message", "role": "assistant", + "content": []map[string]any{{"type": "output_text", "text": "Looking."}}, + }, + }, + } +} + +// TestParse_FunctionCallCarriesIDArgumentsAndOutput covers codex's main tool +// shape. call_id is the id on both halves, so it is both the stored id and the +// key that links the output back to the call. +func TestParse_FunctionCallCarriesIDArgumentsAndOutput(t *testing.T) { + lines := append(codexBaseLines(), + map[string]any{ + "timestamp": "2026-03-11T16:25:07.985Z", + "type": "response_item", + "payload": map[string]any{ + "type": "function_call", + "name": "exec_command", + "arguments": `{"cmd":"ls -la","workdir":"/tmp"}`, + "call_id": "call_tQRetcPjbp", + }, + }, + map[string]any{ + "timestamp": "2026-03-11T16:25:08.000Z", + "type": "response_item", + "payload": map[string]any{ + "type": "function_call_output", + "call_id": "call_tQRetcPjbp", + "output": "Process exited with code 0\ntotal 8", + }, + }, + ) + + tus := allToolUses(parseLines(t, lines)) + if len(tus) != 1 { + t.Fatalf("got %d tool uses, want 1: %+v", len(tus), tus) + } + tu := tus[0] + + if tu.ToolUseID != "call_tQRetcPjbp" { + t.Errorf("ToolUseID = %q, want %q", tu.ToolUseID, "call_tQRetcPjbp") + } + if !strings.Contains(tu.InputJSON, "ls -la") { + t.Errorf("InputJSON = %q, want the arguments", tu.InputJSON) + } + if tu.InputLength != len(tu.InputJSON) { + t.Errorf("InputLength = %d, len(InputJSON) = %d", tu.InputLength, len(tu.InputJSON)) + } + if !tu.HasResult { + t.Error("HasResult = false, want true") + } + if want := "Process exited with code 0\ntotal 8"; tu.ResultContent != want { + t.Errorf("ResultContent = %q, want %q", tu.ResultContent, want) + } + if tu.ResultLength != len("Process exited with code 0\ntotal 8") { + t.Errorf("ResultLength = %d, want %d", tu.ResultLength, len("Process exited with code 0\ntotal 8")) + } +} + +// TestParse_CustomToolCallIsRecorded covers apply_patch, codex's edit tool, +// which arrives as custom_tool_call with its body in `input` rather than +// `arguments`. The adapter skipped the whole payload kind before this change, +// so every codex file edit was invisible to the archive. +func TestParse_CustomToolCallIsRecorded(t *testing.T) { + lines := append(codexBaseLines(), + map[string]any{ + "timestamp": "2026-03-11T16:44:01.327Z", + "type": "response_item", + "payload": map[string]any{ + "type": "custom_tool_call", + "status": "completed", + "call_id": "call_vpkPlrbeFG", + "name": "apply_patch", + "input": "*** Begin Patch\n*** Add File: /tmp/plan.md\n+# Plan\n", + }, + }, + map[string]any{ + "timestamp": "2026-03-11T16:44:01.357Z", + "type": "response_item", + "payload": map[string]any{ + "type": "custom_tool_call_output", + "call_id": "call_vpkPlrbeFG", + "output": `{"output":"Success. Updated the following files:\nA /tmp/plan.md\n"}`, + }, + }, + ) + + tus := allToolUses(parseLines(t, lines)) + if len(tus) != 1 { + t.Fatalf("got %d tool uses, want 1: %+v", len(tus), tus) + } + tu := tus[0] + + if tu.ToolName != "apply_patch" { + t.Errorf("ToolName = %q, want %q", tu.ToolName, "apply_patch") + } + if tu.ToolUseID != "call_vpkPlrbeFG" { + t.Errorf("ToolUseID = %q, want %q", tu.ToolUseID, "call_vpkPlrbeFG") + } + if !strings.Contains(tu.InputJSON, "*** Begin Patch") { + t.Errorf("InputJSON = %q, want the patch body", tu.InputJSON) + } + if !strings.Contains(tu.ResultContent, "Success.") { + t.Errorf("ResultContent = %q, want the output", tu.ResultContent) + } +} + +// TestParse_OutputArrivingBeforeItsCallStillLinks covers ordering. codex writes +// the output after the call today, but the link is call_id, not adjacency, and +// the adapter buffers rather than assuming. +func TestParse_OutputArrivingBeforeItsCallStillLinks(t *testing.T) { + lines := append(codexBaseLines(), + map[string]any{ + "timestamp": "2026-03-11T16:25:08.000Z", + "type": "response_item", + "payload": map[string]any{ + "type": "function_call_output", "call_id": "call_late", "output": "early output", + }, + }, + map[string]any{ + "timestamp": "2026-03-11T16:25:07.985Z", + "type": "response_item", + "payload": map[string]any{ + "type": "function_call", "name": "exec_command", "arguments": "{}", "call_id": "call_late", + }, + }, + ) + + tus := allToolUses(parseLines(t, lines)) + if len(tus) != 1 { + t.Fatalf("got %d tool uses, want 1", len(tus)) + } + if !tus[0].HasResult || tus[0].ResultContent != "early output" { + t.Errorf("got %+v, want the buffered output attached", tus[0]) + } +} + +// TestParse_CallWithNoOutputHasNoResult keeps "the tool never answered" +// distinct from "it answered with nothing". +func TestParse_CallWithNoOutputHasNoResult(t *testing.T) { + lines := append(codexBaseLines(), + map[string]any{ + "timestamp": "2026-03-11T16:25:07.985Z", + "type": "response_item", + "payload": map[string]any{ + "type": "function_call", "name": "exec_command", "arguments": "{}", "call_id": "call_orphan", + }, + }, + ) + + tus := allToolUses(parseLines(t, lines)) + if len(tus) != 1 { + t.Fatalf("got %d tool uses, want 1", len(tus)) + } + if tus[0].HasResult { + t.Error("HasResult = true, want false") + } +} diff --git a/pkg/adapter/jeff/jeff.go b/pkg/adapter/jeff/jeff.go index 3e1d6ef..a2e7c43 100644 --- a/pkg/adapter/jeff/jeff.go +++ b/pkg/adapter/jeff/jeff.go @@ -15,6 +15,7 @@ import ( "time" "github.com/2389-research/ccvault/pkg/adapter" + "github.com/2389-research/ccvault/pkg/toolpayload" ) // sessionIDPrefix namespaces Jeff session IDs to prevent collisions with other @@ -61,8 +62,19 @@ type messageData struct { // toolRequestData holds the fields from a tool_request entry's data. type toolRequestData struct { - ToolName string `json:"tool_name"` - ToolID string `json:"tool_id"` + ToolName string `json:"tool_name"` + ToolID string `json:"tool_id"` + Params json.RawMessage `json:"params"` +} + +// toolResultData holds the fields from a tool_result entry's data. Jeff +// records only a preview of its tool output — the field is named +// output_preview and is already shortened upstream, so what lands in the +// archive is the most jeff kept, not the most ccvault would store. +type toolResultData struct { + ToolID string `json:"tool_id"` + OutputPreview string `json:"output_preview"` + Success bool `json:"success"` } // Discover scans the Jeff sessions directory for JSONL session files and returns @@ -117,6 +129,51 @@ func projectPathForRoot(root string) string { return "jeff:" + root } +// jeffCallSite records a tool call that has not been answered yet, and where +// on the turn slice its row sits. +type jeffCallSite struct { + toolID string + turnIdx int + useIdx int +} + +// attachJeffResult links a tool_result to the call it answers and removes that +// call from the pending list. +// +// Prefers an exact tool_id match, which is right when jeff recorded one. Falls +// back to the oldest unanswered call, which is what the data forces: tool_id +// is the empty string on 85% of real tool_requests, so matching on it would +// make every one of those the same call. Jeff's own files pair requests and +// results one for one — 742 of each across the author's sessions — so order is +// a sound fallback rather than a guess. +func attachJeffResult(turns []adapter.ParsedTurn, pending *[]jeffCallSite, res toolResultData) { + idx := -1 + if res.ToolID != "" { + for i, site := range *pending { + if site.toolID == res.ToolID { + idx = i + break + } + } + } + if idx < 0 { + if len(*pending) == 0 { + return + } + idx = 0 + } + + site := (*pending)[idx] + *pending = append((*pending)[:idx], (*pending)[idx+1:]...) + + tu := &turns[site.turnIdx].ToolUses[site.useIdx] + decided := toolpayload.ResultFromText(tu.ToolName, res.OutputPreview) + tu.HasResult = true + tu.ResultContent = decided.Content + tu.ResultLength = decided.Length + tu.ResultOmittedReason = decided.OmitReason +} + // Parse reads a Jeff JSONL session file and converts it to adapter.ParsedSession. func (a *Adapter) Parse(path string) (*adapter.ParsedSession, error) { f, err := os.Open(path) @@ -138,6 +195,11 @@ func (a *Adapter) Parse(path string) (*adapter.ParsedSession, error) { // Track the current assistant turn so we can attach tool uses lastAssistantIdx = -1 + + // Tool calls still waiting for their result, oldest first. Jeff writes + // tool_id on both halves but leaves it empty on 633 of 742 real + // requests, so an id alone cannot link them — see attachJeffResult. + pending []jeffCallSite ) turnCounter := 0 @@ -233,9 +295,26 @@ func (a *Adapter) Parse(path string) (*adapter.ParsedSession, error) { if lastAssistantIdx >= 0 && lastAssistantIdx < len(turns) { turns[lastAssistantIdx].ToolUses = append( turns[lastAssistantIdx].ToolUses, - adapter.ParsedToolUse{ToolName: tr.ToolName}, + adapter.ParsedToolUse{ + ToolName: tr.ToolName, + ToolUseID: tr.ToolID, + InputJSON: string(tr.Params), + InputLength: len(tr.Params), + }, ) + pending = append(pending, jeffCallSite{ + toolID: tr.ToolID, + turnIdx: lastAssistantIdx, + useIdx: len(turns[lastAssistantIdx].ToolUses) - 1, + }) + } + + case "tool_result": + var tres toolResultData + if err := json.Unmarshal(line.Data, &tres); err != nil { + continue } + attachJeffResult(turns, &pending, tres) case "error": hasError = true diff --git a/pkg/adapter/jeff/toolpayloads_test.go b/pkg/adapter/jeff/toolpayloads_test.go new file mode 100644 index 0000000..0e05fdc --- /dev/null +++ b/pkg/adapter/jeff/toolpayloads_test.go @@ -0,0 +1,158 @@ +// ABOUTME: Tests that the jeff adapter carries tool_id, params, and output_preview onto each tool use. +// ABOUTME: Covers the case real jeff data is mostly made of — an empty tool_id, where linking has to fall back to order. + +package jeff + +import ( + "path/filepath" + "strings" + "testing" + + "github.com/2389-research/ccvault/pkg/adapter" +) + +const convID = "d41f67af-1234-5678-9abc-def012345678" + +func parseLines(t *testing.T, lines []map[string]any) *adapter.ParsedSession { + t.Helper() + fpath := filepath.Join(t.TempDir(), "20260224_195605.jsonl") + writeJSONLFile(t, fpath, lines) + parsed, err := New().Parse(fpath) + if err != nil { + t.Fatalf("Parse: %v", err) + } + return parsed +} + +func allToolUses(s *adapter.ParsedSession) []adapter.ParsedToolUse { + var out []adapter.ParsedToolUse + for _, turn := range s.Turns { + out = append(out, turn.ToolUses...) + } + return out +} + +func jeffBaseLines() []map[string]any { + return []map[string]any{ + { + "timestamp": "2026-02-24T19:56:05.125559Z", "entry_type": "session_start", + "conversation_id": convID, + "data": map[string]any{"model": "claude-sonnet-4-5"}, + }, + { + "timestamp": "2026-02-24T19:56:15.000000Z", "entry_type": "assistant_message", + "conversation_id": convID, + "data": map[string]any{"content": "Searching."}, + }, + } +} + +// TestParse_ToolRequestCarriesIDParamsAndResult covers a jeff tool call where +// tool_id is set, which is the minority (109 of 742 real requests) but the +// shape the link is designed around. +func TestParse_ToolRequestCarriesIDParamsAndResult(t *testing.T) { + lines := append(jeffBaseLines(), + map[string]any{ + "timestamp": "2026-02-24T19:56:16.000000Z", "entry_type": "tool_request", + "conversation_id": convID, + "data": map[string]any{ + "tool_name": "search_drive", + "params": map[string]any{"query": "Q4 report"}, + "tool_id": "tool-001", + }, + }, + map[string]any{ + "timestamp": "2026-02-24T19:56:18.000000Z", "entry_type": "tool_result", + "conversation_id": convID, + "data": map[string]any{ + "success": true, "output_preview": "Found 3 results", "tool_id": "tool-001", + }, + }, + ) + + tus := allToolUses(parseLines(t, lines)) + if len(tus) != 1 { + t.Fatalf("got %d tool uses, want 1: %+v", len(tus), tus) + } + tu := tus[0] + + if tu.ToolUseID != "tool-001" { + t.Errorf("ToolUseID = %q, want %q", tu.ToolUseID, "tool-001") + } + if !strings.Contains(tu.InputJSON, "Q4 report") { + t.Errorf("InputJSON = %q, want the params", tu.InputJSON) + } + if !tu.HasResult || tu.ResultContent != "Found 3 results" { + t.Errorf("got HasResult=%v ResultContent=%q, want true / %q", tu.HasResult, tu.ResultContent, "Found 3 results") + } + if tu.ResultLength != len("Found 3 results") { + t.Errorf("ResultLength = %d, want %d", tu.ResultLength, len("Found 3 results")) + } +} + +// TestParse_EmptyToolIDLinksByOrder covers what jeff actually writes: tool_id +// is the empty string on 633 of 742 real tool_requests. Keying the link on an +// id would collapse every one of those onto each other, so the fallback is the +// file's order — each result belongs to the oldest request still waiting. +func TestParse_EmptyToolIDLinksByOrder(t *testing.T) { + lines := append(jeffBaseLines(), + map[string]any{ + "timestamp": "2026-02-24T19:56:16.000000Z", "entry_type": "tool_request", + "conversation_id": convID, + "data": map[string]any{ + "tool_name": "email", "params": map[string]any{"operation": "list"}, "tool_id": "", + }, + }, + map[string]any{ + "timestamp": "2026-02-24T19:56:17.000000Z", "entry_type": "tool_result", + "conversation_id": convID, + "data": map[string]any{"success": true, "output_preview": "Found 2 message(s)", "tool_id": ""}, + }, + map[string]any{ + "timestamp": "2026-02-24T19:56:18.000000Z", "entry_type": "tool_request", + "conversation_id": convID, + "data": map[string]any{ + "tool_name": "calendar", "params": map[string]any{"operation": "list"}, "tool_id": "", + }, + }, + map[string]any{ + "timestamp": "2026-02-24T19:56:19.000000Z", "entry_type": "tool_result", + "conversation_id": convID, + "data": map[string]any{"success": true, "output_preview": "No events today", "tool_id": ""}, + }, + ) + + tus := allToolUses(parseLines(t, lines)) + if len(tus) != 2 { + t.Fatalf("got %d tool uses, want 2: %+v", len(tus), tus) + } + if tus[0].ToolName != "email" || tus[0].ResultContent != "Found 2 message(s)" { + t.Errorf("first = %+v, want email / %q", tus[0], "Found 2 message(s)") + } + if tus[1].ToolName != "calendar" || tus[1].ResultContent != "No events today" { + t.Errorf("second = %+v, want calendar / %q", tus[1], "No events today") + } + if tus[0].ToolUseID != "" || tus[1].ToolUseID != "" { + t.Errorf("ToolUseIDs = %q / %q, want both empty — jeff recorded none, and inventing one would make it unjoinable", tus[0].ToolUseID, tus[1].ToolUseID) + } +} + +// TestParse_UnansweredToolRequestHasNoResult keeps the two absences apart in +// jeff too. +func TestParse_UnansweredToolRequestHasNoResult(t *testing.T) { + lines := append(jeffBaseLines(), + map[string]any{ + "timestamp": "2026-02-24T19:56:16.000000Z", "entry_type": "tool_request", + "conversation_id": convID, + "data": map[string]any{"tool_name": "email", "params": map[string]any{}, "tool_id": ""}, + }, + ) + + tus := allToolUses(parseLines(t, lines)) + if len(tus) != 1 { + t.Fatalf("got %d tool uses, want 1", len(tus)) + } + if tus[0].HasResult { + t.Error("HasResult = true, want false") + } +} diff --git a/pkg/adapter/nanoclaw/nanoclaw.go b/pkg/adapter/nanoclaw/nanoclaw.go index a3f4d4e..06fa395 100644 --- a/pkg/adapter/nanoclaw/nanoclaw.go +++ b/pkg/adapter/nanoclaw/nanoclaw.go @@ -284,10 +284,7 @@ func buildTurnsAndMetadata(turns []models.Turn, reclassifyScheduled bool) ([]ada if tus, ok := toolUsesByTurn[t.ID]; ok { for _, tu := range tus { - pt.ToolUses = append(pt.ToolUses, adapter.ParsedToolUse{ - ToolName: tu.ToolName, - FilePath: tu.FilePath, - }) + pt.ToolUses = append(pt.ToolUses, adapter.ParsedToolUseFromModel(tu)) if tu.ToolName == "Task" || tu.ToolName == "Agent" { hasSubagent = true } diff --git a/pkg/adapter/toolpayloads.go b/pkg/adapter/toolpayloads.go new file mode 100644 index 0000000..9960e1d --- /dev/null +++ b/pkg/adapter/toolpayloads.go @@ -0,0 +1,54 @@ +// ABOUTME: Conversions between adapter.ParsedToolUse and models.ToolUse, in one place rather than three. +// ABOUTME: Every adapter and the sync layer go through these so a new payload field cannot be dropped in transit. + +package adapter + +import ( + "time" + + "github.com/2389-research/ccvault/pkg/models" +) + +// ParsedToolUseFromModel converts a tool use the parser produced into the +// adapter contract's shape. Used by the sources that parse Claude Code JSONL +// (claude-code, nanoclaw) and get models.ToolUse values back from pkg/parser. +// +// This conversion and ToolUseFromParsed exist as functions rather than as +// struct literals at each call site because the fields are copied in three +// places — two adapters and the sync layer — and a field missed in any one of +// them is not a compile error, just a column that is quietly always empty. +// TestToolUseRoundTrip walks the struct by reflection so adding a field +// without threading it through fails the build's tests instead. +func ParsedToolUseFromModel(tu models.ToolUse) ParsedToolUse { + return ParsedToolUse{ + ToolName: tu.ToolName, + FilePath: tu.FilePath, + ToolUseID: tu.ToolUseID, + InputJSON: tu.InputJSON, + InputLength: tu.InputLength, + HasResult: tu.HasResult, + ResultContent: tu.ResultContent, + ResultLength: tu.ResultLength, + ResultOmittedReason: tu.ResultOmittedReason, + } +} + +// ToolUseFromParsed converts an adapter's tool use into the row the database +// stores, stamping on the identity the adapter contract carries per turn +// rather than per tool use. +func ToolUseFromParsed(p ParsedToolUse, turnID, sessionID string, timestamp time.Time) models.ToolUse { + return models.ToolUse{ + TurnID: turnID, + SessionID: sessionID, + Timestamp: timestamp, + ToolName: p.ToolName, + FilePath: p.FilePath, + ToolUseID: p.ToolUseID, + InputJSON: p.InputJSON, + InputLength: p.InputLength, + HasResult: p.HasResult, + ResultContent: p.ResultContent, + ResultLength: p.ResultLength, + ResultOmittedReason: p.ResultOmittedReason, + } +} diff --git a/pkg/adapter/toolpayloads_test.go b/pkg/adapter/toolpayloads_test.go new file mode 100644 index 0000000..7c79589 --- /dev/null +++ b/pkg/adapter/toolpayloads_test.go @@ -0,0 +1,85 @@ +// ABOUTME: Guards the ParsedToolUse <-> models.ToolUse conversions against a field that gets added but not threaded through. +// ABOUTME: Walks both structs by reflection, because a forgotten field copy compiles cleanly and just leaves a column empty. + +package adapter + +import ( + "reflect" + "testing" + "time" + + "github.com/2389-research/ccvault/pkg/models" +) + +// distinctValue returns a non-zero value for a field type, so a field that +// fails to survive a conversion shows up as a zero where a value was set. +func distinctValue(t *testing.T, kind reflect.Kind, seed int) reflect.Value { + t.Helper() + switch kind { + case reflect.String: + return reflect.ValueOf("value-" + time.Duration(seed).String()) + case reflect.Int: + return reflect.ValueOf(seed + 1) + case reflect.Int64: + return reflect.ValueOf(int64(seed + 1)) + case reflect.Bool: + return reflect.ValueOf(true) + default: + t.Fatalf("distinctValue has no case for %s — add one when a field of that type is introduced", kind) + return reflect.Value{} + } +} + +// TestToolUseRoundTrip fills every field of ParsedToolUse with a distinct +// non-zero value, converts to models.ToolUse and back, and requires the result +// to equal the original. +// +// The failure this exists for is the one PR #35 ran into from the other +// direction: a change that compiles and passes lint while silently carrying +// the wrong data. Copying nine fields in three files is exactly that shape — +// a missed line is not an error, it is a column that is always empty. +func TestToolUseRoundTrip(t *testing.T) { + var original ParsedToolUse + v := reflect.ValueOf(&original).Elem() + for i := range v.NumField() { + v.Field(i).Set(distinctValue(t, v.Field(i).Kind(), i)) + } + + stored := ToolUseFromParsed(original, "turn-1", "session-1", time.Unix(1700000000, 0).UTC()) + back := ParsedToolUseFromModel(stored) + + if !reflect.DeepEqual(original, back) { + t.Errorf("round trip lost data:\n original = %+v\n got = %+v", original, back) + } +} + +// TestToolUseFromParsedStampsIdentity covers the fields that come from the +// turn rather than the tool use, which the adapter contract carries one level +// up. +func TestToolUseFromParsedStampsIdentity(t *testing.T) { + ts := time.Unix(1700000000, 0).UTC() + got := ToolUseFromParsed(ParsedToolUse{ToolName: "Bash"}, "turn-1", "session-1", ts) + + if got.TurnID != "turn-1" || got.SessionID != "session-1" || !got.Timestamp.Equal(ts) { + t.Errorf("got %+v, want turn-1 / session-1 / %v", got, ts) + } +} + +// TestModelToolUseCoversEveryParsedField pins the two structs to the same +// payload surface. A field added to models.ToolUse that ParsedToolUse has no +// way to supply would be a column no adapter can ever populate. +func TestModelToolUseCoversEveryParsedField(t *testing.T) { + modelFields := make(map[string]bool) + mt := reflect.TypeOf(models.ToolUse{}) + for i := range mt.NumField() { + modelFields[mt.Field(i).Name] = true + } + + pt := reflect.TypeOf(ParsedToolUse{}) + for i := range pt.NumField() { + name := pt.Field(i).Name + if !modelFields[name] { + t.Errorf("ParsedToolUse.%s has no counterpart on models.ToolUse, so nothing stores it", name) + } + } +} diff --git a/pkg/models/models.go b/pkg/models/models.go index 229638f..c6e1f41 100644 --- a/pkg/models/models.go +++ b/pkg/models/models.go @@ -108,6 +108,46 @@ type ToolUse struct { ToolName string `json:"tool_name"` FilePath string `json:"file_path,omitempty"` // For file-related tools Timestamp time.Time `json:"timestamp"` + + // ToolUseID is the id the source gave this call, verbatim — toolu_… for + // Claude Code and nanoclaw, call_… for codex, jeff's tool_id (empty on 85% + // of real jeff requests). Empty for a source that mints no id. + // + // The provider's own id rather than a surrogate, because its job is to be + // joinable against ids recorded elsewhere. A subagent transcript's + // meta.json carries a toolUseId naming the exact call that dispatched it + // (#31, resolving for 87 of 95 real subagents), and that link had nowhere + // to land until this column existed. + ToolUseID string `json:"tool_use_id,omitempty"` + + // InputJSON is the call's arguments, stored whole. Inputs are never + // omitted: 259,836 of them total 78.6 MB across the author's archive, p50 + // 72 bytes, largest 99,792. + InputJSON string `json:"input_json,omitempty"` + + // InputLength is len(InputJSON). Stored rather than derived so a consumer + // reading a row can size the payload without fetching it. + InputLength int `json:"input_length,omitempty"` + + // HasResult reports whether the source recorded a result for this call. It + // separates "the tool answered with nothing" from "no answer was ever + // recorded" — an interrupted call, which the empty string cannot express. + HasResult bool `json:"has_result"` + + // ResultContent is the result text, empty when the content was left out + // (see ResultOmittedReason) or when the tool genuinely returned nothing. + ResultContent string `json:"result_content,omitempty"` + + // ResultLength is the result's size as the transcript carries it, + // recorded whether or not the content was stored. See + // toolpayload.Result.Length for the exact definition. + ResultLength int `json:"result_length,omitempty"` + + // ResultOmittedReason names why ResultContent was left out — one of + // toolpayload's Omit* values, empty when the content is stored. A consumer + // reads this instead of re-deriving the classification from tool_name and + // ResultLength. + ResultOmittedReason string `json:"result_omitted_reason,omitempty"` } // RawTurn represents the raw JSONL entry from Claude Code diff --git a/pkg/parser/parser.go b/pkg/parser/parser.go index a12e29a..2190192 100644 --- a/pkg/parser/parser.go +++ b/pkg/parser/parser.go @@ -14,6 +14,7 @@ import ( "time" "github.com/2389-research/ccvault/pkg/models" + "github.com/2389-research/ccvault/pkg/toolpayload" ) // ParseStats reports counts of anomalies the parser handled while reading a @@ -396,8 +397,18 @@ func parseTimestamp(s string) (time.Time, error) { return time.Time{}, fmt.Errorf("unknown timestamp format: %s", s) } -// ExtractToolUses extracts tool usage information from turns +// ExtractToolUses extracts tool usage information from turns, including each +// call's provider id, its input, and the result it produced. +// +// Two passes, because a call and its result live in different turns: Claude +// Code writes the tool_use into an assistant message and the tool_result into +// a later user message, joined by tool_use_id. The first pass indexes the +// results by that id, the second walks the calls and attaches them. Pairing by +// position would be wrong — a turn can issue several calls and their results +// come back in whatever order they finish, which they demonstrably do. func ExtractToolUses(turns []models.Turn) []models.ToolUse { + results := collectToolResults(turns) + var toolUses []models.ToolUse for _, turn := range turns { @@ -424,12 +435,23 @@ func ExtractToolUses(turns []models.Turn) []models.ToolUse { TurnID: turn.ID, SessionID: turn.SessionID, ToolName: content.Name, + ToolUseID: content.ID, Timestamp: turn.Timestamp, } // Extract file path for file-related tools if content.Input != nil { toolUse.FilePath = extractFilePath(content.Name, content.Input) + toolUse.InputJSON = string(content.Input) + toolUse.InputLength = len(content.Input) + } + + if rawResult, ok := results[content.ID]; ok && content.ID != "" { + decided := toolpayload.ResultFromJSON(content.Name, rawResult) + toolUse.HasResult = true + toolUse.ResultContent = decided.Content + toolUse.ResultLength = decided.Length + toolUse.ResultOmittedReason = decided.OmitReason } toolUses = append(toolUses, toolUse) @@ -439,6 +461,63 @@ func ExtractToolUses(turns []models.Turn) []models.ToolUse { return toolUses } +// rawToolResultBlock is a tool_result content block as the transcripts write +// it. Content is left raw because the sources use both shapes: a JSON string +// (129,296 of the author's results) and an array of content blocks (130,516). +// +// Deliberately separate from models.UserContentBlock, whose Content is typed +// as a string. Retyping that field would change what extractUserContent +// produces for every turn carrying an array-valued result, and so rewrite +// turns.content — and turns_fts behind it — for a third of the archive. That +// is a real bug (see the note on TestExtractUserContent_IgnoresImageBlocks) +// but it is not this change's bug, and the new payloads reach search through +// their own index rather than by disturbing that one. +type rawToolResultBlock struct { + Type string `json:"type"` + ToolUseID string `json:"tool_use_id"` + Content json.RawMessage `json:"content"` +} + +// collectToolResults indexes every tool_result block in the session by the +// tool_use_id it names. Walks all turn types rather than only "user": the +// result's home is the user message today, and keying on the block's own type +// costs nothing and does not assume that. +func collectToolResults(turns []models.Turn) map[string]json.RawMessage { + results := make(map[string]json.RawMessage) + + for _, turn := range turns { + var raw models.RawTurn + if err := json.Unmarshal(turn.RawJSON, &raw); err != nil || raw.Message == nil { + continue + } + + var msg models.RawUserMessage + if err := json.Unmarshal(raw.Message, &msg); err != nil { + continue + } + + var blocks []rawToolResultBlock + if err := json.Unmarshal(msg.Content, &blocks); err != nil { + // A plain-string message content, which carries no tool results. + continue + } + + for _, block := range blocks { + if block.Type != "tool_result" || block.ToolUseID == "" { + continue + } + // First result wins. A repeated id would mean the transcript + // answered one call twice; keeping the first keeps the mapping + // stable regardless of which turn order a re-parse sees. + if _, seen := results[block.ToolUseID]; !seen { + results[block.ToolUseID] = block.Content + } + } + } + + return results +} + // extractUserContent extracts text from user message content // Content can be a plain string or an array of content blocks func extractUserContent(content json.RawMessage) string { diff --git a/pkg/parser/toolpayloads_test.go b/pkg/parser/toolpayloads_test.go new file mode 100644 index 0000000..87283b6 --- /dev/null +++ b/pkg/parser/toolpayloads_test.go @@ -0,0 +1,232 @@ +// ABOUTME: Tests for ExtractToolUses — the provider tool-use id, the input, and the result linked back to its call. +// ABOUTME: Fixtures are real Claude Code transcript shapes: tool_use in an assistant turn, tool_result in the next user turn. + +package parser + +import ( + "strings" + "testing" + + "github.com/2389-research/ccvault/pkg/toolpayload" +) + +// twoTurnTranscript is a tool call and its result in the shape Claude Code +// writes them: the tool_use in an assistant message, the tool_result in the +// user message that follows, joined by tool_use_id. +const twoTurnTranscript = `{"uuid":"a-1","sessionId":"s-1","type":"assistant","timestamp":"2026-09-01T10:00:00.000Z","message":{"model":"claude","role":"assistant","content":[{"type":"tool_use","id":"toolu_01ABC","name":"Bash","input":{"command":"git status --short","description":"check tree"}}]}} +{"uuid":"u-1","sessionId":"s-1","type":"user","timestamp":"2026-09-01T10:00:01.000Z","message":{"role":"user","content":[{"type":"tool_result","tool_use_id":"toolu_01ABC","content":"?? docs/multiplayer.md"}]}}` + +func extractFrom(t *testing.T, transcript string) []toolUseForTest { + t.Helper() + turns, _, _, err := ParseSessionReader(strings.NewReader(transcript), "/test/payloads.jsonl") + if err != nil { + t.Fatalf("ParseSessionReader: %v", err) + } + out := make([]toolUseForTest, 0) + for _, tu := range ExtractToolUses(turns) { + out = append(out, toolUseForTest{ + ToolUseID: tu.ToolUseID, + ToolName: tu.ToolName, + InputJSON: tu.InputJSON, + HasResult: tu.HasResult, + Result: tu.ResultContent, + ResultLen: tu.ResultLength, + OmitReason: tu.ResultOmittedReason, + }) + } + return out +} + +type toolUseForTest struct { + ToolUseID string + ToolName string + InputJSON string + HasResult bool + Result string + ResultLen int + OmitReason string +} + +// TestExtractToolUses_CarriesIDInputAndResult is the whole point of issue #28: +// the call's provider id, its input, and the result it produced all land on +// the row instead of being parsed and thrown away. +func TestExtractToolUses_CarriesIDInputAndResult(t *testing.T) { + got := extractFrom(t, twoTurnTranscript) + + if len(got) != 1 { + t.Fatalf("got %d tool uses, want 1: %+v", len(got), got) + } + tu := got[0] + + if tu.ToolUseID != "toolu_01ABC" { + t.Errorf("ToolUseID = %q, want %q", tu.ToolUseID, "toolu_01ABC") + } + if !strings.Contains(tu.InputJSON, "git status --short") { + t.Errorf("InputJSON = %q, want it to carry the command", tu.InputJSON) + } + if !tu.HasResult { + t.Error("HasResult = false, want true — the transcript carries a tool_result for this call") + } + if tu.Result != "?? docs/multiplayer.md" { + t.Errorf("ResultContent = %q, want %q", tu.Result, "?? docs/multiplayer.md") + } + if tu.ResultLen != len("?? docs/multiplayer.md") { + t.Errorf("ResultLength = %d, want %d", tu.ResultLen, len("?? docs/multiplayer.md")) + } + if tu.OmitReason != "" { + t.Errorf("ResultOmittedReason = %q, want empty", tu.OmitReason) + } +} + +// TestExtractToolUses_InputLengthMatchesStoredInput pins that the recorded +// length describes the stored bytes. Inputs are never omitted, so these must +// not drift apart. +func TestExtractToolUses_InputLengthMatchesStoredInput(t *testing.T) { + turns, _, _, err := ParseSessionReader(strings.NewReader(twoTurnTranscript), "/test/payloads.jsonl") + if err != nil { + t.Fatalf("ParseSessionReader: %v", err) + } + tus := ExtractToolUses(turns) + if len(tus) != 1 { + t.Fatalf("got %d tool uses, want 1", len(tus)) + } + if tus[0].InputLength != len(tus[0].InputJSON) { + t.Errorf("InputLength = %d, len(InputJSON) = %d", tus[0].InputLength, len(tus[0].InputJSON)) + } +} + +// TestExtractToolUses_ResultLinkedAcrossDistantTurns covers the realistic +// interleaving: two calls in one assistant turn, their results arriving in +// separate later turns and out of call order. Matching on tool_use_id is what +// keeps them straight; pairing by position would cross them. +func TestExtractToolUses_ResultLinkedAcrossDistantTurns(t *testing.T) { + transcript := `{"uuid":"a-1","sessionId":"s-2","type":"assistant","timestamp":"2026-09-01T10:00:00.000Z","message":{"model":"claude","role":"assistant","content":[{"type":"tool_use","id":"toolu_FIRST","name":"Bash","input":{"command":"one"}},{"type":"tool_use","id":"toolu_SECOND","name":"Grep","input":{"pattern":"two"}}]}} +{"uuid":"u-1","sessionId":"s-2","type":"user","timestamp":"2026-09-01T10:00:01.000Z","message":{"role":"user","content":[{"type":"tool_result","tool_use_id":"toolu_SECOND","content":"grep output"}]}} +{"uuid":"u-2","sessionId":"s-2","type":"user","timestamp":"2026-09-01T10:00:02.000Z","message":{"role":"user","content":[{"type":"tool_result","tool_use_id":"toolu_FIRST","content":"bash output"}]}}` + + got := extractFrom(t, transcript) + if len(got) != 2 { + t.Fatalf("got %d tool uses, want 2: %+v", len(got), got) + } + if got[0].ToolUseID != "toolu_FIRST" || got[0].Result != "bash output" { + t.Errorf("first call got %+v, want toolu_FIRST / %q", got[0], "bash output") + } + if got[1].ToolUseID != "toolu_SECOND" || got[1].Result != "grep output" { + t.Errorf("second call got %+v, want toolu_SECOND / %q", got[1], "grep output") + } +} + +// TestExtractToolUses_NoResultIsNotAnEmptyResult covers an interrupted call — +// 24 of them in the author's archive. HasResult false is a different fact from +// a result that came back empty, and the row has to be able to say which. +func TestExtractToolUses_NoResultIsNotAnEmptyResult(t *testing.T) { + transcript := `{"uuid":"a-1","sessionId":"s-3","type":"assistant","timestamp":"2026-09-01T10:00:00.000Z","message":{"model":"claude","role":"assistant","content":[{"type":"tool_use","id":"toolu_ORPHAN","name":"Bash","input":{"command":"sleep 600"}}]}}` + + got := extractFrom(t, transcript) + if len(got) != 1 { + t.Fatalf("got %d tool uses, want 1", len(got)) + } + if got[0].HasResult { + t.Error("HasResult = true, want false for a call with no tool_result in the transcript") + } + + withEmpty := `{"uuid":"a-1","sessionId":"s-4","type":"assistant","timestamp":"2026-09-01T10:00:00.000Z","message":{"model":"claude","role":"assistant","content":[{"type":"tool_use","id":"toolu_EMPTY","name":"Bash","input":{"command":"true"}}]}} +{"uuid":"u-1","sessionId":"s-4","type":"user","timestamp":"2026-09-01T10:00:01.000Z","message":{"role":"user","content":[{"type":"tool_result","tool_use_id":"toolu_EMPTY","content":""}]}}` + + got = extractFrom(t, withEmpty) + if len(got) != 1 { + t.Fatalf("got %d tool uses, want 1", len(got)) + } + if !got[0].HasResult { + t.Error("HasResult = false, want true — the tool answered, it just answered with nothing") + } + if got[0].ResultLen != 0 || got[0].OmitReason != "" { + t.Errorf("empty result got len=%d reason=%q, want 0 and empty", got[0].ResultLen, got[0].OmitReason) + } +} + +// TestExtractToolUses_BulkReadKeepsLengthOnly covers the policy arriving +// through the parser: a Read's result is 65% of nothing useful in an FTS index, +// so the row keeps file_path and a length. +func TestExtractToolUses_BulkReadKeepsLengthOnly(t *testing.T) { + body := strings.Repeat("a line of a file\\n", 500) + transcript := `{"uuid":"a-1","sessionId":"s-5","type":"assistant","timestamp":"2026-09-01T10:00:00.000Z","message":{"model":"claude","role":"assistant","content":[{"type":"tool_use","id":"toolu_READ","name":"Read","input":{"file_path":"/tmp/big.txt"}}]}} +{"uuid":"u-1","sessionId":"s-5","type":"user","timestamp":"2026-09-01T10:00:01.000Z","message":{"role":"user","content":[{"type":"tool_result","tool_use_id":"toolu_READ","content":"` + body + `"}]}}` + + got := extractFrom(t, transcript) + if len(got) != 1 { + t.Fatalf("got %d tool uses, want 1", len(got)) + } + if got[0].Result != "" { + t.Errorf("stored %d bytes of a bulk read, want none", len(got[0].Result)) + } + if got[0].OmitReason != toolpayload.OmitBulkRead { + t.Errorf("OmitReason = %q, want %q", got[0].OmitReason, toolpayload.OmitBulkRead) + } + if got[0].ResultLen == 0 { + t.Error("ResultLength = 0, want the original size") + } + if !got[0].HasResult { + t.Error("HasResult = false — an omitted result is still a result") + } +} + +// TestExtractToolUses_ImagePayloadNeverStored is the base64 canary at the +// parser level. An image-shaped result must leave no trace of its payload on +// the row, whatever the tool is called. +func TestExtractToolUses_ImagePayloadNeverStored(t *testing.T) { + needle := "BASE64PAYLOADCANARYAAAAAAAAAAAAAAAAAA" + transcript := `{"uuid":"a-1","sessionId":"s-6","type":"assistant","timestamp":"2026-09-01T10:00:00.000Z","message":{"model":"claude","role":"assistant","content":[{"type":"tool_use","id":"toolu_IMG","name":"mcp__arbitrary__name","input":{"url":"http://x"}}]}} +{"uuid":"u-1","sessionId":"s-6","type":"user","timestamp":"2026-09-01T10:00:01.000Z","message":{"role":"user","content":[{"type":"tool_result","tool_use_id":"toolu_IMG","content":[{"type":"image","source":{"type":"base64","media_type":"image/png","data":"` + needle + `"}}]}]}}` + + got := extractFrom(t, transcript) + if len(got) != 1 { + t.Fatalf("got %d tool uses, want 1", len(got)) + } + if strings.Contains(got[0].Result, needle) { + t.Fatalf("base64 payload reached the stored result: %q", got[0].Result) + } + if got[0].OmitReason != toolpayload.OmitImage { + t.Errorf("OmitReason = %q, want %q", got[0].OmitReason, toolpayload.OmitImage) + } + if got[0].ResultLen == 0 { + t.Error("ResultLength = 0, want the payload's original size") + } +} + +// TestExtractToolUses_TextArrayResultConcatenated covers the content shape +// half the archive's results use: an array holding one text block. +func TestExtractToolUses_TextArrayResultConcatenated(t *testing.T) { + transcript := `{"uuid":"a-1","sessionId":"s-7","type":"assistant","timestamp":"2026-09-01T10:00:00.000Z","message":{"model":"claude","role":"assistant","content":[{"type":"tool_use","id":"toolu_ARR","name":"mcp__nanoclaw__ssh_localhost","input":{"command":"uptime"}}]}} +{"uuid":"u-1","sessionId":"s-7","type":"user","timestamp":"2026-09-01T10:00:01.000Z","message":{"role":"user","content":[{"type":"tool_result","tool_use_id":"toolu_ARR","content":[{"type":"text","text":"load average: 2.10"}]}]}}` + + got := extractFrom(t, transcript) + if len(got) != 1 { + t.Fatalf("got %d tool uses, want 1", len(got)) + } + if got[0].Result != "load average: 2.10" { + t.Errorf("ResultContent = %q, want %q", got[0].Result, "load average: 2.10") + } +} + +// TestExtractToolUses_TurnContentIsUnchanged pins the boundary of this change. +// turns.content feeds turns_fts, which holds a million documents; the tool +// payloads become searchable through their own index, and nothing here is +// allowed to start rewriting what the existing one contains. +func TestExtractToolUses_TurnContentIsUnchanged(t *testing.T) { + turns, _, _, err := ParseSessionReader(strings.NewReader(twoTurnTranscript), "/test/payloads.jsonl") + if err != nil { + t.Fatalf("ParseSessionReader: %v", err) + } + if len(turns) != 2 { + t.Fatalf("got %d turns, want 2", len(turns)) + } + // The assistant turn still summarises its tool call the way it always has. + if want := "[Tool: Bash] $ git status --short"; turns[0].Content != want { + t.Errorf("assistant turn content = %q, want %q", turns[0].Content, want) + } + // And the tool_result turn still gets the 200-char preview form. + if want := "[Tool Result: ?? docs/multiplayer.md]"; turns[1].Content != want { + t.Errorf("tool_result turn content = %q, want %q", turns[1].Content, want) + } +} From 24c7a1cd2ce5267e2dbe5665798cac49765c4c95 Mon Sep 17 00:00:00 2001 From: Dylan Richard Date: Fri, 2 Oct 2026 20:23:08 -0500 Subject: [PATCH 3/8] feat: migration 009 stores and indexes tool payloads, backfilled from raw_json MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Six columns on tool_uses — tool_use_id, input_json, input_length, result_content, result_length, result_omitted_reason — plus tool_uses_fts over the two that hold stored text. On tool_uses rather than a new table: the relationship is 1:1, the row already carries turn_id and session_id, and a join table buys nothing. Its own FTS table rather than folded into turns_fts, because the material belongs to a tool_uses row and there is no turns column it could live in without being duplicated there. result_length carries two facts, which is why there is no has_result column: NULL means nothing ever answered the call, 0 means it answered with nothing. A second column would say the same thing twice and could disagree with itself. idx_tool_uses_tool_use_id is deliberately not unique. The provider id is stored verbatim so a follow-up can join it against the toolUseId in a subagent's meta.json (#31), and a unique index would be a hazard rather than a guarantee: one transcript ingested under two sources legitimately repeats an id, and #29 found INSERT OR REPLACE resolves a unique conflict by silently deleting the conflicting row. Backfilled from turns.raw_json, so an existing archive upgrades instead of re-ingesting 44,840 sessions. The join is per turn by position — the Nth tool_uses row by rowid to the Nth tool_use block by array index — with tool_name required to agree so a bad match stays NULL rather than being labelled with another call's input. Measured end to end against a copy of the author's 965,061-turn, 261,138-tool-use, 5,057 MB archive: migration 009 total 2m26s, +665 MB payload columns 442 MB (74 MB inputs, 368 MB results) tool_uses_fts 130 MB over 261,138 documents rows given an id and input 259,836 of 261,138 results stored whole 211,869 (378 MB of original) results omitted: bulk_read 47,444 (195 MB) results omitted: image 476 (52 MB) results omitted: oversize 0 results omitted: undecodable 0 The 128 KB backstop fired on nothing, which is what a backstop should do. The 1,302 rows left without an id are all duplicates: 1,288 turns in that archive carry more tool_uses rows than their message has tool_use blocks, left behind by the #30 recovery import. Filling the first and leaving the extras NULL is the right outcome — two rows sharing a provider id would make the id useless as a join key. The FTS table and its triggers are created after the backfill UPDATEs and populated with one rebuild, so the full-table update pays no per-row index cost. Triggers are scoped to exactly input_json and result_content, the two indexed columns; migration 008 measured a wider scope costing 377 MB of dead segments. The docsize shadow table ends with exactly 261,138 rows — no orphans, checked the way #42 established is the only way to see one. TestMigrator_BootstrapPartial simulated "only the initial schema" with a turns table missing raw_json and no tool_uses table at all, neither of which migration 001 omits. It bootstraps the database to version 1, which asserts everything 001 creates is present, so the fixture now creates it. Co-Authored-By: Claude Opus 5 --- .../db/migrations/009_add_tool_payloads.sql | 265 +++++++++ internal/db/migrator_test.go | 19 +- internal/db/schema.sql | 56 +- internal/db/toolpayloads_test.go | 557 ++++++++++++++++++ internal/db/turns.go | 39 +- 5 files changed, 930 insertions(+), 6 deletions(-) create mode 100644 internal/db/migrations/009_add_tool_payloads.sql create mode 100644 internal/db/toolpayloads_test.go diff --git a/internal/db/migrations/009_add_tool_payloads.sql b/internal/db/migrations/009_add_tool_payloads.sql new file mode 100644 index 0000000..3b3c59c --- /dev/null +++ b/internal/db/migrations/009_add_tool_payloads.sql @@ -0,0 +1,265 @@ +-- ABOUTME: Adds the tool payload columns to tool_uses plus an FTS index over the two searchable ones. +-- ABOUTME: Backfills every column from turns.raw_json, so an existing archive needs no re-sync. + +-- The id the source gave this call, verbatim. toolu_… for Claude Code and +-- nanoclaw, call_… for codex, jeff's tool_id where it recorded one. +-- +-- NULL, not '', when the source recorded no id — the same discipline migration +-- 008 used for last_entry_uuid, so no read path has to treat the empty string +-- as a third state. Jeff needs it: tool_id is empty on 633 of its 742 real +-- requests. +-- +-- The provider's id rather than a surrogate, because the point of the column +-- is to be joinable against ids recorded elsewhere. A subagent transcript's +-- meta.json carries a toolUseId naming the exact call that dispatched it (#31, +-- resolving for 87 of 95 real subagents) and had nowhere to land until now. +ALTER TABLE tool_uses ADD COLUMN tool_use_id TEXT; + +-- The call's arguments, stored whole and never truncated. Measured across the +-- author's 259,836 calls: 78.6 MB in total, p50 72 bytes, largest 99,792. +ALTER TABLE tool_uses ADD COLUMN input_json TEXT; +ALTER TABLE tool_uses ADD COLUMN input_length INTEGER; + +-- The result text, NULL when the content was left out or when the tool +-- returned nothing at all. +ALTER TABLE tool_uses ADD COLUMN result_content TEXT; + +-- The result's size as the transcript carries it, recorded whether or not the +-- content was stored. That is what lets a consumer tell a short result from an +-- omitted one without guessing. +-- +-- It is also the only thing that says a result exists: result_length IS NULL +-- means nothing ever answered this call, which is a different fact from a +-- result that came back empty (result_length = 0). A separate has_result +-- column would say the same thing twice and could disagree with itself. +ALTER TABLE tool_uses ADD COLUMN result_length INTEGER; + +-- Why result_content was left out: 'bulk_read', 'image', 'oversize', +-- 'undecodable'. NULL when the content is stored. See pkg/toolpayload for the +-- policy and the measurements behind it. +ALTER TABLE tool_uses ADD COLUMN result_omitted_reason TEXT; + +-- Deliberately NOT unique. tool_uses has no natural key today and this is not +-- one: the same transcript ingested under two sources (a Claude Code session +-- also walked by the nanoclaw adapter) legitimately repeats a provider id, and +-- #29 found that an INSERT OR REPLACE resolves a unique-index conflict by +-- silently deleting the conflicting row. A plain index is what the join needs. +CREATE INDEX IF NOT EXISTS idx_tool_uses_tool_use_id ON tool_uses(tool_use_id) + WHERE tool_use_id IS NOT NULL; + +-- --------------------------------------------------------------------------- +-- Backfill +-- +-- The payloads are already in the archive, inside turns.raw_json — 1,348 MB of +-- it against 109 MB of extracted content. Reading them here rather than asking +-- for a re-sync is the difference between an upgrade and a re-ingest of 44,840 +-- sessions. +-- +-- Runs before tool_uses_fts and its triggers exist, so these two full-table +-- UPDATEs cost no index work at all. Migration 008 measured what the other +-- order costs: an FTS trigger that fires on a column it does not index turned +-- a backfill of 965,000 rows from 2.2s into 9.2s and added 377 MB of dead +-- segments. Creating the index afterwards and rebuilding it in one pass avoids +-- the question entirely. +-- +-- Timed end to end against a copy of the author's archive — 965,061 turns, +-- 261,138 tool uses, 5,057 MB: the whole of this migration takes 2m26s and +-- grows the database by 665 MB. That is 442 MB of payload columns (74 MB of +-- inputs, 368 MB of results), ~130 MB of FTS, and the rest page overhead. +-- 259,836 of the 261,138 rows get an id and an input; see the note on the +-- join below for the 1,302 that do not. +-- --------------------------------------------------------------------------- + +-- Every tool_use block in the archive, with its position inside the message. +DROP TABLE IF EXISTS temp.migration_009_calls; + +CREATE TEMP TABLE migration_009_calls AS +SELECT t.id AS turn_id, + je.key AS block_pos, + json_extract(je.value, '$.id') AS tool_use_id, + json_extract(je.value, '$.name') AS tool_name, + json_extract(je.value, '$.input') AS input_json +FROM turns t, + json_each(CASE WHEN json_valid(t.raw_json) + AND json_type(t.raw_json, '$.message.content') = 'array' + THEN json_extract(t.raw_json, '$.message.content') + ELSE '[]' END) je +WHERE t.type = 'assistant' + AND json_extract(je.value, '$.type') = 'tool_use'; + +-- Match each tool_uses row to the block that produced it. +-- +-- There is no stored key to join on — that is what this migration is adding — +-- so the join is per turn by position: the Nth tool_uses row of a turn, by +-- rowid, is the Nth tool_use block of its message, by array index. rowid is +-- insertion order and the parser has always walked the content array in order, +-- so the two sequences are the same sequence. +-- +-- tool_name has to agree as well. It is not needed to find the match; it is +-- there so that a row the position rule gets wrong stays NULL instead of being +-- labelled with another call's input. Measured on the author's archive: the +-- guard fired on nothing, so no row was saved from a mislabel — but nothing +-- else would have caught one. +-- +-- 1,302 rows come out of this with a NULL id, and all of them for the same +-- reason: 1,288 turns in that archive carry duplicate tool_uses rows (1,281 +-- turns hold two rows for one tool_use block, 7 hold four), left behind by the +-- recovery import in #30. The position rule fills the first row of each turn +-- and leaves the extras alone, which is the right outcome — giving two rows +-- the same provider id would make the id useless as a join key. The +-- duplicates are a data-hygiene problem of their own, not this migration's to +-- fix. +UPDATE tool_uses +SET tool_use_id = src.tool_use_id, + input_json = src.input_json, + input_length = octet_length(src.input_json) +FROM ( + SELECT rows.rid, calls.tool_use_id, calls.input_json + FROM (SELECT rowid AS rid, turn_id, tool_name, + ROW_NUMBER() OVER (PARTITION BY turn_id ORDER BY rowid) AS n + FROM tool_uses) AS rows + JOIN (SELECT turn_id, tool_name, tool_use_id, input_json, + ROW_NUMBER() OVER (PARTITION BY turn_id ORDER BY block_pos) AS n + FROM migration_009_calls) AS calls + ON calls.turn_id = rows.turn_id + AND calls.n = rows.n + AND calls.tool_name = rows.tool_name +) AS src +WHERE tool_uses.rowid = src.rid + AND tool_uses.tool_use_id IS NULL; + +-- Every tool_result block, keyed by the call it answers. +-- +-- Not filtered to user turns. That is where Claude Code puts them, and keying +-- on the block's own type costs nothing and does not depend on it staying +-- true. GROUP BY keeps one row per id: 259,812 results across the archive all +-- name distinct calls, and a transcript that answered one call twice should not +-- make the join multiply rows. +DROP TABLE IF EXISTS temp.migration_009_results; + +CREATE TEMP TABLE migration_009_results AS +SELECT json_extract(je.value, '$.tool_use_id') AS tool_use_id, + json_type(je.value, '$.content') AS content_type, + json_extract(je.value, '$.content') AS content, + octet_length(json_extract(je.value, '$.content')) AS content_len +FROM turns t, + json_each(CASE WHEN json_valid(t.raw_json) + AND json_type(t.raw_json, '$.message.content') = 'array' + THEN json_extract(t.raw_json, '$.message.content') + ELSE '[]' END) je +WHERE json_extract(je.value, '$.type') = 'tool_result' + AND json_extract(je.value, '$.tool_use_id') IS NOT NULL +GROUP BY tool_use_id; + +-- The structured half: whether the content holds an image block, and the text +-- blocks concatenated. 130,516 of the archive's results use this shape and +-- 129,750 of those hold exactly one block, but the ORDER BY is there because +-- the 766 that hold more have an order that means something. +DROP TABLE IF EXISTS temp.migration_009_result_blocks; + +CREATE TEMP TABLE migration_009_result_blocks AS +SELECT r.tool_use_id, + MAX(CASE WHEN json_extract(e.value, '$.type') = 'image' THEN 1 ELSE 0 END) AS has_image, + group_concat(CASE WHEN json_extract(e.value, '$.type') = 'text' + AND json_extract(e.value, '$.text') <> '' + THEN json_extract(e.value, '$.text') END, + char(10) ORDER BY e.key) AS text_content +FROM migration_009_results r, + json_each(CASE WHEN r.content_type = 'array' THEN r.content ELSE '[]' END) e +GROUP BY r.tool_use_id; + +-- Apply the storage policy. The order of the CASE arms is the order in +-- pkg/toolpayload.decide, and the two have to stay in step: the same +-- transcript has to produce the same row whether it arrives through a sync or +-- through this backfill. +-- +-- Image first because it is the shape-based rule and the most specific thing +-- true about a result carrying one — matching on the tool's name would miss +-- every MCP tool whose name says nothing about images, and 476 image results +-- holding 54.9 MB of base64 is exactly what must not reach the index. +UPDATE tool_uses +SET result_length = COALESCE(src.content_len, 0), + result_omitted_reason = + CASE WHEN src.has_image = 1 THEN 'image' + WHEN tool_uses.tool_name IN ('Read', 'NotebookRead') THEN 'bulk_read' + WHEN COALESCE(src.content_len, 0) > 131072 THEN 'oversize' + WHEN src.content_type IS NULL OR src.content_type = 'null' THEN NULL + WHEN src.content_type NOT IN ('text', 'array') THEN 'undecodable' + ELSE NULL END, + result_content = + CASE WHEN src.has_image = 1 THEN NULL + WHEN tool_uses.tool_name IN ('Read', 'NotebookRead') THEN NULL + WHEN COALESCE(src.content_len, 0) > 131072 THEN NULL + WHEN src.content_type = 'text' THEN NULLIF(src.content, '') + WHEN src.content_type = 'array' THEN NULLIF(src.text_content, '') + ELSE NULL END +FROM ( + SELECT r.tool_use_id, r.content_type, r.content, r.content_len, + COALESCE(b.has_image, 0) AS has_image, + b.text_content + FROM migration_009_results r + LEFT JOIN migration_009_result_blocks b USING (tool_use_id) +) AS src +WHERE tool_uses.tool_use_id = src.tool_use_id + AND tool_uses.result_length IS NULL; + +DROP TABLE IF EXISTS temp.migration_009_calls; +DROP TABLE IF EXISTS temp.migration_009_results; +DROP TABLE IF EXISTS temp.migration_009_result_blocks; + +-- --------------------------------------------------------------------------- +-- Full-text search over the payloads +-- +-- Its own index rather than folding into turns_fts, because the material +-- belongs to a tool_uses row: turns_fts is external-content over turns and has +-- one column, and there is no turns column these payloads could live in +-- without duplicating them. +-- +-- Only the two columns whose content is stored are indexed. result_content is +-- NULL for every omitted result, so the file dumps and base64 contribute +-- nothing — which is the whole reason the policy omits them. Indexing 205 MB of +-- file contents would put a search for `git commit` in competition with every +-- file ever read. +-- --------------------------------------------------------------------------- +CREATE VIRTUAL TABLE IF NOT EXISTS tool_uses_fts USING fts5( + input_json, + result_content, + content='tool_uses', + content_rowid='id' +); + +CREATE TRIGGER IF NOT EXISTS tool_uses_ai AFTER INSERT ON tool_uses BEGIN + INSERT INTO tool_uses_fts(rowid, input_json, result_content) + VALUES (new.id, new.input_json, new.result_content); +END; + +-- Depends on PRAGMA recursive_triggers being ON, which ccvault's connection DSN +-- sets and verifyConnectionPragmas asserts — the same dependency turns_ad has. +-- Sync replaces a session's tool uses with DELETE then INSERT today, so this +-- fires directly; it also covers a REPLACE's implicit delete if one ever +-- arrives. +CREATE TRIGGER IF NOT EXISTS tool_uses_ad AFTER DELETE ON tool_uses BEGIN + INSERT INTO tool_uses_fts(tool_uses_fts, rowid, input_json, result_content) + VALUES ('delete', old.id, old.input_json, old.result_content); +END; + +-- Scoped to exactly the two indexed columns. An UPDATE that leaves them alone +-- leaves the index correct, and firing on every column is what cost migration +-- 008's backfill 377 MB of dead segments before it was narrowed. +CREATE TRIGGER IF NOT EXISTS tool_uses_au AFTER UPDATE OF input_json, result_content ON tool_uses BEGIN + INSERT INTO tool_uses_fts(tool_uses_fts, rowid, input_json, result_content) + VALUES ('delete', old.id, old.input_json, old.result_content); + INSERT INTO tool_uses_fts(rowid, input_json, result_content) + VALUES (new.id, new.input_json, new.result_content); +END; + +-- One pass over the backfilled columns, rather than per-row trigger work +-- during the UPDATEs above. Idempotent, so a replay re-indexes rather than +-- duplicating. +-- +-- Measured: 130 MB of index over 261,138 documents, and the docsize shadow +-- table holds exactly 261,138 rows afterwards — one per tool_uses row, no +-- orphans. That count is the check that matters; #42 established that +-- COUNT(*) on an external-content FTS table resolves through the base table +-- and cannot see an orphan at all. +INSERT INTO tool_uses_fts(tool_uses_fts) VALUES('rebuild'); diff --git a/internal/db/migrator_test.go b/internal/db/migrator_test.go index 81c8070..a5eee0f 100644 --- a/internal/db/migrator_test.go +++ b/internal/db/migrator_test.go @@ -245,7 +245,13 @@ func TestMigrator_BootstrapPartial(t *testing.T) { db := openMemoryDB(t) defer func() { _ = db.Close() }() - // Simulate a database with only the initial schema (no has_error/has_subagent) + // Simulate a database with only the initial schema (no has_error/has_subagent). + // + // The tables here have to carry what migration 001 actually creates, not a + // subset of it. detectExistingState bootstraps this database to version 1, + // which asserts "everything 001 creates is present", and every later + // migration is entitled to rely on that. turns.raw_json and the tool_uses + // table were missing until migration 009 — which reads both — needed them. stmts := []string{ `CREATE TABLE projects ( id INTEGER PRIMARY KEY AUTOINCREMENT, @@ -263,7 +269,16 @@ func TestMigrator_BootstrapPartial(t *testing.T) { session_id TEXT, type TEXT NOT NULL, timestamp DATETIME NOT NULL, - content TEXT + content TEXT, + raw_json TEXT + )`, + `CREATE TABLE tool_uses ( + id INTEGER PRIMARY KEY AUTOINCREMENT, + turn_id TEXT, + session_id TEXT, + tool_name TEXT NOT NULL, + file_path TEXT, + timestamp DATETIME NOT NULL )`, } for _, stmt := range stmts { diff --git a/internal/db/schema.sql b/internal/db/schema.sql index 9a543fe..7a282ac 100644 --- a/internal/db/schema.sql +++ b/internal/db/schema.sql @@ -79,13 +79,32 @@ CREATE TABLE IF NOT EXISTS tool_uses ( session_id TEXT REFERENCES sessions(id) ON DELETE CASCADE, tool_name TEXT NOT NULL, file_path TEXT, - timestamp DATETIME NOT NULL + timestamp DATETIME NOT NULL, + -- The id the source gave this call, verbatim, so it can be joined against + -- ids recorded elsewhere. NULL when the source recorded none. + tool_use_id TEXT, + -- The call's arguments, stored whole and indexed. + input_json TEXT, + input_length INTEGER, + -- The result text, NULL when omitted by policy or when the tool returned + -- nothing. result_length is the size as the transcript carried it and is + -- recorded either way; result_length IS NULL means nothing answered the + -- call at all. See pkg/toolpayload. + result_content TEXT, + result_length INTEGER, + result_omitted_reason TEXT ); CREATE INDEX IF NOT EXISTS idx_tool_uses_session ON tool_uses(session_id); CREATE INDEX IF NOT EXISTS idx_tool_uses_tool_name ON tool_uses(tool_name); CREATE INDEX IF NOT EXISTS idx_tool_uses_file_path ON tool_uses(file_path); +-- Not unique: the same transcript ingested under two sources legitimately +-- repeats a provider id, and INSERT OR REPLACE resolves a unique conflict by +-- silently deleting the conflicting row. +CREATE INDEX IF NOT EXISTS idx_tool_uses_tool_use_id ON tool_uses(tool_use_id) + WHERE tool_use_id IS NOT NULL; + -- Full-text search virtual table CREATE VIRTUAL TABLE IF NOT EXISTS turns_fts USING fts5( content, @@ -119,6 +138,41 @@ CREATE TRIGGER IF NOT EXISTS turns_au AFTER UPDATE OF content ON turns BEGIN INSERT INTO turns_fts(rowid, content) VALUES (new.rowid, new.content); END; +-- Full-text search over the tool payloads. +-- +-- Its own index rather than folded into turns_fts, because the material +-- belongs to a tool_uses row and there is no turns column it could live in +-- without duplicating it. Only the two stored columns are indexed; +-- result_content is NULL for every omitted result, so file dumps and base64 +-- contribute nothing. +CREATE VIRTUAL TABLE IF NOT EXISTS tool_uses_fts USING fts5( + input_json, + result_content, + content='tool_uses', + content_rowid='id' +); + +-- Same recursive_triggers dependency as turns_ad. Triggers are scoped to +-- exactly the two indexed columns: an UPDATE that leaves them alone leaves the +-- index correct, and a wider scope is what cost migration 008's backfill +-- 377 MB of dead segments before it was narrowed. +CREATE TRIGGER IF NOT EXISTS tool_uses_ai AFTER INSERT ON tool_uses BEGIN + INSERT INTO tool_uses_fts(rowid, input_json, result_content) + VALUES (new.id, new.input_json, new.result_content); +END; + +CREATE TRIGGER IF NOT EXISTS tool_uses_ad AFTER DELETE ON tool_uses BEGIN + INSERT INTO tool_uses_fts(tool_uses_fts, rowid, input_json, result_content) + VALUES ('delete', old.id, old.input_json, old.result_content); +END; + +CREATE TRIGGER IF NOT EXISTS tool_uses_au AFTER UPDATE OF input_json, result_content ON tool_uses BEGIN + INSERT INTO tool_uses_fts(tool_uses_fts, rowid, input_json, result_content) + VALUES ('delete', old.id, old.input_json, old.result_content); + INSERT INTO tool_uses_fts(rowid, input_json, result_content) + VALUES (new.id, new.input_json, new.result_content); +END; + -- Sync state table CREATE TABLE IF NOT EXISTS sync_state ( key TEXT PRIMARY KEY, diff --git a/internal/db/toolpayloads_test.go b/internal/db/toolpayloads_test.go new file mode 100644 index 0000000..a463d05 --- /dev/null +++ b/internal/db/toolpayloads_test.go @@ -0,0 +1,557 @@ +// ABOUTME: Tests migration 009 — the tool payload columns, their backfill from raw_json, and the FTS index over them. +// ABOUTME: Seeds under the pre-009 schema so the backfill assertions cannot be satisfied by the insert path. + +package db + +import ( + "database/sql" + "fmt" + "path/filepath" + "strings" + "testing" + "time" + + "github.com/2389-research/ccvault/pkg/models" + "github.com/2389-research/ccvault/pkg/toolpayload" + _ "modernc.org/sqlite" +) + +// payloadFixture is one tool call plus the result answering it, as the two +// raw_json lines a Claude Code transcript would hold. +type payloadFixture struct { + toolUseID string + toolName string + filePath string + assistantRaw string + userRaw string +} + +func payloadFixtures() []payloadFixture { + return []payloadFixture{ + { + toolUseID: "toolu_BASH", + toolName: "Bash", + assistantRaw: `{"uuid":"a-bash","sessionId":"sess-p","type":"assistant","timestamp":"2026-10-01T10:00:00.000Z",` + + `"message":{"role":"assistant","content":[{"type":"tool_use","id":"toolu_BASH","name":"Bash",` + + `"input":{"command":"git rebase --onto main"}}]}}`, + userRaw: `{"uuid":"u-bash","sessionId":"sess-p","type":"user","timestamp":"2026-10-01T10:00:01.000Z",` + + `"message":{"role":"user","content":[{"type":"tool_result","tool_use_id":"toolu_BASH",` + + `"content":"Successfully rebased and updated refs/heads/topic."}]}}`, + }, + { + toolUseID: "toolu_READ", + toolName: "Read", + filePath: "/tmp/huge.txt", + assistantRaw: `{"uuid":"a-read","sessionId":"sess-p","type":"assistant","timestamp":"2026-10-01T10:00:02.000Z",` + + `"message":{"role":"assistant","content":[{"type":"tool_use","id":"toolu_READ","name":"Read",` + + `"input":{"file_path":"/tmp/huge.txt"}}]}}`, + userRaw: `{"uuid":"u-read","sessionId":"sess-p","type":"user","timestamp":"2026-10-01T10:00:03.000Z",` + + `"message":{"role":"user","content":[{"type":"tool_result","tool_use_id":"toolu_READ",` + + `"content":"` + strings.Repeat("file line ", 400) + `"}]}}`, + }, + { + toolUseID: "toolu_IMG", + toolName: "mcp__arbitrary__fetch", + assistantRaw: `{"uuid":"a-img","sessionId":"sess-p","type":"assistant","timestamp":"2026-10-01T10:00:04.000Z",` + + `"message":{"role":"assistant","content":[{"type":"tool_use","id":"toolu_IMG","name":"mcp__arbitrary__fetch",` + + `"input":{"url":"http://example.test/chart"}}]}}`, + userRaw: `{"uuid":"u-img","sessionId":"sess-p","type":"user","timestamp":"2026-10-01T10:00:05.000Z",` + + `"message":{"role":"user","content":[{"type":"tool_result","tool_use_id":"toolu_IMG",` + + `"content":[{"type":"image","source":{"type":"base64","media_type":"image/png","data":"` + + backfillCanary + `"}}]}]}}`, + }, + { + toolUseID: "toolu_ARR", + toolName: "mcp__nanoclaw__ssh_localhost", + assistantRaw: `{"uuid":"a-arr","sessionId":"sess-p","type":"assistant","timestamp":"2026-10-01T10:00:06.000Z",` + + `"message":{"role":"assistant","content":[{"type":"tool_use","id":"toolu_ARR","name":"mcp__nanoclaw__ssh_localhost",` + + `"input":{"command":"systemctl status nginx"}}]}}`, + userRaw: `{"uuid":"u-arr","sessionId":"sess-p","type":"user","timestamp":"2026-10-01T10:00:07.000Z",` + + `"message":{"role":"user","content":[{"type":"tool_result","tool_use_id":"toolu_ARR",` + + `"content":[{"type":"text","text":"nginx.service - active (running)"}]}]}}`, + }, + { + // A call the transcript never answered — 24 of these in the real + // archive. It must come out of the backfill distinguishable from a + // call that answered with nothing. + toolUseID: "toolu_ORPHAN", + toolName: "Bash", + assistantRaw: `{"uuid":"a-orphan","sessionId":"sess-p","type":"assistant","timestamp":"2026-10-01T10:00:08.000Z",` + + `"message":{"role":"assistant","content":[{"type":"tool_use","id":"toolu_ORPHAN","name":"Bash",` + + `"input":{"command":"sleep 600"}}]}}`, + }, + } +} + +// backfillCanary is a token that exists only inside an image payload. If it +// ever appears in a payload column or an FTS hit, base64 leaked. +const backfillCanary = "BASE64PAYLOADCANARYAAAAAAAAAAAAAAAAAA" + +// seedPre009 builds a database holding the fixtures under the schema as it +// stood *below* migration 009 — turns with raw_json, tool_uses rows with only +// the columns that existed then — then closes it and returns its directory. +// +// Applying only the migrations below 009 is what makes the backfill tests +// non-vacuous. At seed time there are no payload columns at all, so nothing +// can write a correct value early: the assertions can only be satisfied by +// migration 009 reading raw_json afterwards. +func seedPre009(t *testing.T, fixtures []payloadFixture) string { + t.Helper() + + dir := t.TempDir() + raw, err := sql.Open("sqlite", filepath.Join(dir, "ccvault.db")) + if err != nil { + t.Fatalf("open raw: %v", err) + } + + migrations, err := loadMigrations() + if err != nil { + t.Fatalf("load migrations: %v", err) + } + if _, err := raw.Exec(`CREATE TABLE IF NOT EXISTS schema_version ( + version INTEGER NOT NULL, + applied_at TEXT NOT NULL DEFAULT (datetime('now')))`); err != nil { + t.Fatalf("schema_version: %v", err) + } + for _, m := range migrations { + if m.version >= 9 { + continue + } + if err := applyMigration(raw, m); err != nil { + t.Fatalf("apply %03d: %v", m.version, err) + } + } + + // Guard the guard: if a future edit lets a payload column exist before 009 + // runs, these tests would pass without proving anything. + for _, column := range []string{ + "tool_use_id", "input_json", "input_length", + "result_content", "result_length", "result_omitted_reason", + } { + if columnExists(t, raw, "tool_uses", column) { + t.Fatalf("tool_uses.%s exists below migration 009, so the backfill tests prove nothing", column) + } + } + if tableExists(t, raw, "tool_uses_fts") { + t.Fatal("tool_uses_fts exists below migration 009, so the index tests prove nothing") + } + + if _, err := raw.Exec(`INSERT INTO sessions (id, started_at, source_file, source, model, git_branch) + VALUES ('sess-p', ?, '/fake/sess-p.jsonl', 'claude-code', '', '')`, + time.Date(2026, 10, 1, 10, 0, 0, 0, time.UTC)); err != nil { + t.Fatalf("seed session: %v", err) + } + + ts := time.Date(2026, 10, 1, 10, 0, 0, 0, time.UTC) + ordinal := 0 + for i, f := range fixtures { + turnID := "turn-" + f.toolUseID + if _, err := raw.Exec( + `INSERT INTO turns (id, session_id, type, timestamp, content, raw_json, ordinal) + VALUES (?, 'sess-p', 'assistant', ?, ?, ?, ?)`, + turnID, ts.Add(time.Duration(i)*time.Second), "[Tool: "+f.toolName+"]", f.assistantRaw, ordinal); err != nil { + t.Fatalf("seed assistant turn %s: %v", turnID, err) + } + ordinal++ + // The pre-009 tool_uses row: name and path, which is all the schema + // could hold. + if _, err := raw.Exec( + `INSERT INTO tool_uses (turn_id, session_id, tool_name, file_path, timestamp) VALUES (?, 'sess-p', ?, ?, ?)`, + turnID, f.toolName, f.filePath, ts.Add(time.Duration(i)*time.Second)); err != nil { + t.Fatalf("seed tool use %s: %v", f.toolUseID, err) + } + if f.userRaw != "" { + resultTurnID := "result-" + f.toolUseID + if _, err := raw.Exec( + `INSERT INTO turns (id, session_id, type, timestamp, content, raw_json, ordinal) + VALUES (?, 'sess-p', 'user', ?, '', ?, ?)`, + resultTurnID, ts.Add(time.Duration(i)*time.Second+500*time.Millisecond), f.userRaw, ordinal); err != nil { + t.Fatalf("seed result turn %s: %v", resultTurnID, err) + } + ordinal++ + } + } + + if err := raw.Close(); err != nil { + t.Fatalf("close raw: %v", err) + } + return dir +} + +func tableExists(t *testing.T, raw *sql.DB, name string) bool { + t.Helper() + var count int + if err := raw.QueryRow( + "SELECT COUNT(*) FROM sqlite_master WHERE name = ?", name).Scan(&count); err != nil { + t.Fatalf("sqlite_master lookup for %s: %v", name, err) + } + return count > 0 +} + +// backfilledRow is what a payload column set looks like after the migration. +type backfilledRow struct { + toolUseID sql.NullString + inputJSON sql.NullString + inputLength sql.NullInt64 + result sql.NullString + resultLength sql.NullInt64 + omitReason sql.NullString +} + +func readBackfilled(t *testing.T, database *DB, toolName, turnID string) backfilledRow { + t.Helper() + var r backfilledRow + err := database.QueryRow(`SELECT tool_use_id, input_json, input_length, + result_content, result_length, result_omitted_reason + FROM tool_uses WHERE turn_id = ? AND tool_name = ?`, turnID, toolName).Scan( + &r.toolUseID, &r.inputJSON, &r.inputLength, &r.result, &r.resultLength, &r.omitReason) + if err != nil { + t.Fatalf("read backfilled row for %s/%s: %v", turnID, toolName, err) + } + return r +} + +// TestMigration009BackfillsPayloadsFromRawJSON is the core backfill proof. The +// seeded tool_uses rows carry nothing but a name and a path; every assertion +// below is about data that exists only inside turns.raw_json until 009 runs. +func TestMigration009BackfillsPayloadsFromRawJSON(t *testing.T) { + dir := seedPre009(t, payloadFixtures()) + + database, err := Open(dir) + if err != nil { + t.Fatalf("Open: %v", err) + } + defer func() { _ = database.Close() }() + + t.Run("id and input", func(t *testing.T) { + row := readBackfilled(t, database, "Bash", "turn-toolu_BASH") + if row.toolUseID.String != "toolu_BASH" { + t.Errorf("tool_use_id = %q, want %q", row.toolUseID.String, "toolu_BASH") + } + if !strings.Contains(row.inputJSON.String, "git rebase --onto main") { + t.Errorf("input_json = %q, want the command", row.inputJSON.String) + } + if row.inputLength.Int64 != int64(len(row.inputJSON.String)) { + t.Errorf("input_length = %d, want %d", row.inputLength.Int64, len(row.inputJSON.String)) + } + }) + + t.Run("non-bulk result stored whole", func(t *testing.T) { + row := readBackfilled(t, database, "Bash", "turn-toolu_BASH") + want := "Successfully rebased and updated refs/heads/topic." + if row.result.String != want { + t.Errorf("result_content = %q, want %q", row.result.String, want) + } + if row.resultLength.Int64 != int64(len(want)) { + t.Errorf("result_length = %d, want %d", row.resultLength.Int64, len(want)) + } + if row.omitReason.Valid { + t.Errorf("result_omitted_reason = %q, want NULL", row.omitReason.String) + } + }) + + t.Run("bulk read keeps length only", func(t *testing.T) { + row := readBackfilled(t, database, "Read", "turn-toolu_READ") + if row.result.Valid { + t.Errorf("result_content = %q, want NULL for a bulk read", row.result.String) + } + if row.omitReason.String != toolpayload.OmitBulkRead { + t.Errorf("result_omitted_reason = %q, want %q", row.omitReason.String, toolpayload.OmitBulkRead) + } + if row.resultLength.Int64 != 4000 { + t.Errorf("result_length = %d, want 4000", row.resultLength.Int64) + } + }) + + t.Run("image detected by shape not name", func(t *testing.T) { + row := readBackfilled(t, database, "mcp__arbitrary__fetch", "turn-toolu_IMG") + if strings.Contains(row.result.String, backfillCanary) { + t.Fatalf("base64 payload reached result_content") + } + if row.result.Valid { + t.Errorf("result_content = %q, want NULL", row.result.String) + } + if row.omitReason.String != toolpayload.OmitImage { + t.Errorf("result_omitted_reason = %q, want %q", row.omitReason.String, toolpayload.OmitImage) + } + if row.resultLength.Int64 == 0 { + t.Error("result_length = 0, want the payload's size") + } + }) + + t.Run("array content concatenated", func(t *testing.T) { + row := readBackfilled(t, database, "mcp__nanoclaw__ssh_localhost", "turn-toolu_ARR") + if row.result.String != "nginx.service - active (running)" { + t.Errorf("result_content = %q, want the text block", row.result.String) + } + }) + + t.Run("unanswered call has no result", func(t *testing.T) { + row := readBackfilled(t, database, "Bash", "turn-toolu_ORPHAN") + if row.resultLength.Valid { + t.Errorf("result_length = %d, want NULL — nothing answered this call", row.resultLength.Int64) + } + if row.result.Valid || row.omitReason.Valid { + t.Errorf("got result=%v reason=%v, want both NULL", row.result, row.omitReason) + } + // The id and input still backfill; only the result half is absent. + if row.toolUseID.String != "toolu_ORPHAN" { + t.Errorf("tool_use_id = %q, want %q", row.toolUseID.String, "toolu_ORPHAN") + } + }) +} + +// TestMigration009BackfillsFTS covers the half of the change that makes the +// payloads reachable. Searching for a command that was only ever inside +// raw_json has to find the row. +func TestMigration009BackfillsFTS(t *testing.T) { + dir := seedPre009(t, payloadFixtures()) + + database, err := Open(dir) + if err != nil { + t.Fatalf("Open: %v", err) + } + defer func() { _ = database.Close() }() + + t.Run("input is searchable", func(t *testing.T) { + if got := toolUsesFTSMatchCount(t, database, `"git rebase"`); got != 1 { + t.Errorf("matches for a command in a tool input = %d, want 1", got) + } + }) + + t.Run("result is searchable", func(t *testing.T) { + if got := toolUsesFTSMatchCount(t, database, `"refs/heads/topic"`); got != 1 { + t.Errorf("matches for text in a tool result = %d, want 1", got) + } + }) + + t.Run("omitted content is not indexed", func(t *testing.T) { + if got := toolUsesFTSMatchCount(t, database, backfillCanary); got != 0 { + t.Errorf("base64 canary matched %d rows, want 0", got) + } + if got := toolUsesFTSMatchCount(t, database, `"file line"`); got != 0 { + t.Errorf("bulk read body matched %d rows, want 0", got) + } + }) +} + +func toolUsesFTSMatchCount(t *testing.T, database *DB, match string) int { + t.Helper() + var n int + if err := database.QueryRow( + "SELECT COUNT(*) FROM tool_uses_fts WHERE tool_uses_fts MATCH ?", match).Scan(&n); err != nil { + t.Fatalf("fts match %q: %v", match, err) + } + return n +} + +// TestMigration009Replays covers the rewind path. Anything that resets +// schema_version re-runs every migration above the rewind point, and a +// backfill that is not idempotent would double-apply or abort. +func TestMigration009Replays(t *testing.T) { + dir := seedPre009(t, payloadFixtures()) + + database, err := Open(dir) + if err != nil { + t.Fatalf("Open: %v", err) + } + before := readBackfilled(t, database, "Bash", "turn-toolu_BASH") + beforeFTS := toolUsesFTSMatchCount(t, database, `"git rebase"`) + + if _, err := database.Exec("DELETE FROM schema_version WHERE version >= 9"); err != nil { + t.Fatalf("rewind schema_version: %v", err) + } + if err := RunMigrations(database.DB); err != nil { + t.Fatalf("replay migrations: %v", err) + } + + after := readBackfilled(t, database, "Bash", "turn-toolu_BASH") + if after != before { + t.Errorf("replay changed the row:\n before = %+v\n after = %+v", before, after) + } + if got := toolUsesFTSMatchCount(t, database, `"git rebase"`); got != beforeFTS { + t.Errorf("replay changed FTS matches: %d, want %d", got, beforeFTS) + } + if err := database.Close(); err != nil { + t.Fatalf("close: %v", err) + } +} + +// TestInsertToolUsesWritesPayloads covers the live path, which has to agree +// with what the backfill produces: the same policy applied to the same +// transcript has to give the same row either way. +func TestInsertToolUsesWritesPayloads(t *testing.T) { + database, cleanup := setupTestDB(t) + defer cleanup() + + seedSessionForToolUses(t, database, "sess-live", "turn-live") + + ts := time.Date(2026, 10, 1, 12, 0, 0, 0, time.UTC) + in := []models.ToolUse{ + { + TurnID: "turn-live", SessionID: "sess-live", ToolName: "Bash", Timestamp: ts, + ToolUseID: "toolu_LIVE", InputJSON: `{"command":"make test"}`, InputLength: 23, + HasResult: true, ResultContent: "ok ccvault 1.2s", ResultLength: 15, + }, + { + TurnID: "turn-live", SessionID: "sess-live", ToolName: "Read", Timestamp: ts, + ToolUseID: "toolu_LIVEREAD", FilePath: "/tmp/x", InputJSON: `{"file_path":"/tmp/x"}`, InputLength: 22, + HasResult: true, ResultLength: 90210, ResultOmittedReason: toolpayload.OmitBulkRead, + }, + { + TurnID: "turn-live", SessionID: "sess-live", ToolName: "Bash", Timestamp: ts, + ToolUseID: "toolu_LIVEORPHAN", InputJSON: `{"command":"sleep 1"}`, InputLength: 21, + }, + } + if err := database.InsertToolUses(in); err != nil { + t.Fatalf("InsertToolUses: %v", err) + } + + t.Run("stored result round trips", func(t *testing.T) { + row := readBackfilled(t, database, "Bash", "turn-live") + if row.toolUseID.String != "toolu_LIVE" || row.result.String != "ok ccvault 1.2s" { + t.Errorf("got %+v", row) + } + if row.omitReason.Valid { + t.Errorf("result_omitted_reason = %q, want NULL", row.omitReason.String) + } + }) + + t.Run("omitted result keeps its length and reason", func(t *testing.T) { + var length sql.NullInt64 + var content, reason sql.NullString + if err := database.QueryRow(`SELECT result_content, result_length, result_omitted_reason + FROM tool_uses WHERE tool_use_id = 'toolu_LIVEREAD'`).Scan(&content, &length, &reason); err != nil { + t.Fatalf("query: %v", err) + } + if content.Valid { + t.Errorf("result_content = %q, want NULL", content.String) + } + if length.Int64 != 90210 || reason.String != toolpayload.OmitBulkRead { + t.Errorf("got length=%v reason=%v", length, reason) + } + }) + + t.Run("no result stores NULL length", func(t *testing.T) { + var length sql.NullInt64 + if err := database.QueryRow( + `SELECT result_length FROM tool_uses WHERE tool_use_id = 'toolu_LIVEORPHAN'`).Scan(&length); err != nil { + t.Fatalf("query: %v", err) + } + if length.Valid { + t.Errorf("result_length = %d, want NULL", length.Int64) + } + }) + + t.Run("empty id stores NULL, not the empty string", func(t *testing.T) { + if err := database.InsertToolUses([]models.ToolUse{{ + TurnID: "turn-live", SessionID: "sess-live", ToolName: "email", Timestamp: ts, + ToolUseID: "", InputJSON: `{"operation":"list"}`, InputLength: 20, + }}); err != nil { + t.Fatalf("InsertToolUses: %v", err) + } + var nonNull int + if err := database.QueryRow( + `SELECT COUNT(*) FROM tool_uses WHERE tool_name = 'email' AND tool_use_id IS NOT NULL`).Scan(&nonNull); err != nil { + t.Fatalf("query: %v", err) + } + if nonNull != 0 { + t.Error("an unrecorded tool_use_id was stored as '' rather than NULL, which makes the empty string a third state") + } + }) + + t.Run("live inserts are searchable", func(t *testing.T) { + if got := toolUsesFTSMatchCount(t, database, `"make test"`); got != 1 { + t.Errorf("matches = %d, want 1", got) + } + }) +} + +// TestToolUsesFTSLeavesNoGhosts is the lesson from #42 applied to the new +// index. COUNT(*) on an external-content FTS table resolves through the base +// table and cannot see an orphaned entry, and PRAGMA integrity_check with no +// argument also returns clean. Only the docsize shadow table gives a real +// count, and only integrity-check with argument 1 inspects the index itself. +func TestToolUsesFTSLeavesNoGhosts(t *testing.T) { + database, cleanup := setupTestDB(t) + defer cleanup() + seedSessionForToolUses(t, database, "sess-ghost", "turn-ghost") + + ts := time.Date(2026, 10, 1, 12, 0, 0, 0, time.UTC) + rows := []models.ToolUse{ + {TurnID: "turn-ghost", SessionID: "sess-ghost", ToolName: "Bash", Timestamp: ts, + ToolUseID: "toolu_G1", InputJSON: `{"command":"ghostprobeone"}`, InputLength: 26, + HasResult: true, ResultContent: "ghostresultone", ResultLength: 14}, + {TurnID: "turn-ghost", SessionID: "sess-ghost", ToolName: "Grep", Timestamp: ts, + ToolUseID: "toolu_G2", InputJSON: `{"pattern":"ghostprobetwo"}`, InputLength: 27, + HasResult: true, ResultContent: "ghostresulttwo", ResultLength: 14}, + } + if err := database.InsertToolUses(rows); err != nil { + t.Fatalf("InsertToolUses: %v", err) + } + + if got := docsizeCount(t, database); got != 2 { + t.Fatalf("docsize rows after insert = %d, want 2", got) + } + + // The re-sync shape: delete the session's tool uses, insert them again. + if err := database.WithTx(func(tx *sql.Tx) error { + if err := database.DeleteToolUsesForSessionTx(tx, "sess-ghost"); err != nil { + return err + } + return database.InsertToolUsesTx(tx, rows) + }); err != nil { + t.Fatalf("re-sync: %v", err) + } + + if got := docsizeCount(t, database); got != 2 { + t.Errorf("docsize rows after re-sync = %d, want 2 — the delete trigger left %d orphan(s)", got, got-2) + } + + // Delete for real and confirm the index empties with the table. + if err := database.WithTx(func(tx *sql.Tx) error { + return database.DeleteToolUsesForSessionTx(tx, "sess-ghost") + }); err != nil { + t.Fatalf("delete: %v", err) + } + if got := docsizeCount(t, database); got != 0 { + t.Errorf("docsize rows after delete = %d, want 0", got) + } + + // integrity-check with argument 1 is the only form that inspects the + // index rather than resolving through the content table. + var result string + if err := database.QueryRow( + `INSERT INTO tool_uses_fts(tool_uses_fts, rank) VALUES('integrity-check', 1) RETURNING 'ok'`).Scan(&result); err != nil { + // RETURNING is not available on an fts5 command insert; fall back to + // running it for its error alone. + if _, err := database.Exec( + `INSERT INTO tool_uses_fts(tool_uses_fts, rank) VALUES('integrity-check', 1)`); err != nil { + t.Errorf("fts5 integrity-check(1): %v", err) + } + } +} + +func docsizeCount(t *testing.T, database *DB) int { + t.Helper() + var n int + if err := database.QueryRow("SELECT COUNT(*) FROM tool_uses_fts_docsize").Scan(&n); err != nil { + t.Fatalf("count docsize: %v", err) + } + return n +} + +func seedSessionForToolUses(t *testing.T, database *DB, sessionID, turnID string) { + t.Helper() + ts := time.Date(2026, 10, 1, 12, 0, 0, 0, time.UTC) + if _, err := database.Exec( + `INSERT INTO sessions (id, started_at, source_file, source) VALUES (?, ?, ?, 'claude-code')`, + sessionID, ts, fmt.Sprintf("/fake/%s.jsonl", sessionID)); err != nil { + t.Fatalf("seed session: %v", err) + } + if _, err := database.Exec( + `INSERT INTO turns (id, session_id, type, timestamp, content) VALUES (?, ?, 'assistant', ?, '')`, + turnID, sessionID, ts); err != nil { + t.Fatalf("seed turn: %v", err) + } +} diff --git a/internal/db/turns.go b/internal/db/turns.go index 4ae61fa..071b306 100644 --- a/internal/db/turns.go +++ b/internal/db/turns.go @@ -371,15 +371,19 @@ func (db *DB) InsertToolUses(toolUses []models.ToolUse) error { // InsertToolUsesTx inserts tool usage records within a transaction func (db *DB) InsertToolUsesTx(tx *sql.Tx, toolUses []models.ToolUse) error { stmt, err := tx.Prepare(` - INSERT INTO tool_uses (turn_id, session_id, tool_name, file_path, timestamp) - VALUES (?, ?, ?, ?, ?)`) + INSERT INTO tool_uses (turn_id, session_id, tool_name, file_path, timestamp, + tool_use_id, input_json, input_length, + result_content, result_length, result_omitted_reason) + VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)`) if err != nil { return fmt.Errorf("prepare insert tool_uses: %w", err) } defer func() { _ = stmt.Close() }() for _, tu := range toolUses { - _, err := stmt.Exec(tu.TurnID, tu.SessionID, tu.ToolName, tu.FilePath, tu.Timestamp) + _, err := stmt.Exec(tu.TurnID, tu.SessionID, tu.ToolName, tu.FilePath, tu.Timestamp, + nullIfEmpty(tu.ToolUseID), nullIfEmpty(tu.InputJSON), nullIfZero(tu.InputLength), + nullIfEmpty(tu.ResultContent), resultLengthValue(tu), nullIfEmpty(tu.ResultOmittedReason)) if err != nil { return fmt.Errorf("insert tool_use: %w", err) } @@ -388,6 +392,35 @@ func (db *DB) InsertToolUsesTx(tx *sql.Tx, toolUses []models.ToolUse) error { return nil } +// nullIfEmpty stores an unrecorded string as NULL rather than as the empty +// string. Migration 008 set the precedent for last_entry_uuid: a column +// holding both NULL and the empty string makes every read path treat the +// empty string as a third state. +func nullIfEmpty(s string) any { + if s == "" { + return nil + } + return s +} + +// nullIfZero is the same discipline for a length that was never measured. +func nullIfZero(n int) any { + if n == 0 { + return nil + } + return n +} + +// resultLengthValue encodes "was there a result at all" into the column, which +// is where that fact lives: NULL means nothing answered this call, 0 means it +// answered with nothing. Both are real and they are different. +func resultLengthValue(tu models.ToolUse) any { + if !tu.HasResult { + return nil + } + return tu.ResultLength +} + // DeleteToolUsesForSession removes tool uses for a session func (db *DB) DeleteToolUsesForSessionTx(tx *sql.Tx, sessionID string) error { _, err := tx.Exec("DELETE FROM tool_uses WHERE session_id = ?", sessionID) From 6480a013a4598cf53aad5e28af2cf7fa51108379 Mon Sep 17 00:00:00 2001 From: Dylan Richard Date: Fri, 2 Oct 2026 20:37:00 -0500 Subject: [PATCH 4/8] feat: search reaches tool inputs and results, and says when it did MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A text query now matches against tool_uses_fts as well as turns_fts, so the command you ran and the error a tool printed are findable. That was the point of issue #28: the turn that issued a call summarises itself as "[Tool: Bash]" and holds none of the command, so the material existed in the archive and was unreachable by search. Driven off a UNION of turn rowids rather than two outer joins with an OR between them. Each index answers a MATCH with a small set, so joining turns to that set is an index lookup, where an OR across outer joins would make the planner scan all 965,000 turns. UNION also collapses a turn whose content and several of whose payloads matched into the one row the caller asked for. The payload lookup is applied to the page, not to the candidates. SQLite evaluates result-column subqueries while feeding the sorter, which is before LIMIT, so selecting them alongside the page would run two correlated lookups for every row a common word matched — tens of thousands — to use twenty. Results carry matched_tool_name, and the snippet for such a hit comes from the payload rather than the turn. Without that a payload hit renders a snippet with nothing of the query in it and no indication why it came back. The snippet is fts5's own snippet() with column -1, which asks fts5 which of input_json and result_content matched: a command lives in the input and an error message in the result, so picking one of them statically shows the wrong half about as often as the right one. Surfaced on all three read surfaces — `ccvault search` prints "Matched in payload", the TUI prefixes the snippet with the tool name, and MCP adds matched_tool_name (null for a conversational hit, so an agent can test it rather than infer). One thing found while testing: `tool:` filters by session, not by turn — it joins tool_uses on session_id, so it asks "did this session use the tool". That predates this change and is left alone, but the reference now says so instead of leaving a reader to assume turn scope. Co-Authored-By: Claude Opus 5 --- cmd/ccvault/main.go | 6 + internal/mcp/server.go | 8 + internal/mcp/toolpayloads_test.go | 91 ++++++++++ internal/search/search.go | 118 ++++++++++++- internal/search/toolpayloads_test.go | 241 +++++++++++++++++++++++++++ internal/tui/search.go | 7 +- skills/ccvault/reference.md | 24 ++- 7 files changed, 485 insertions(+), 10 deletions(-) create mode 100644 internal/mcp/toolpayloads_test.go create mode 100644 internal/search/toolpayloads_test.go diff --git a/cmd/ccvault/main.go b/cmd/ccvault/main.go index db3bd16..d98122a 100644 --- a/cmd/ccvault/main.go +++ b/cmd/ccvault/main.go @@ -715,6 +715,12 @@ Supports Gmail-like query syntax: if r.Model != "" { fmt.Printf(" Model: %s\n", r.Model) } + // A hit found through a stored tool payload rather than the turn's + // own text needs saying, because the snippet below it is command + // output or tool arguments, not something anyone wrote. + if r.MatchedToolName != "" { + fmt.Printf(" Matched in %s payload\n", r.MatchedToolName) + } fmt.Printf(" %s\n\n", r.Snippet) } diff --git a/internal/mcp/server.go b/internal/mcp/server.go index 8264504..185eea7 100644 --- a/internal/mcp/server.go +++ b/internal/mcp/server.go @@ -658,10 +658,18 @@ func (s *Server) searchConversations(args map[string]interface{}) (interface{}, // hit can sit in a transcript list_sessions doesn't show. The // parent id is how an agent walks back to the conversation. "parent_session_id": nil, + // Set when the query matched a stored tool input or result rather + // than the turn's own text. Without it an agent cannot tell a + // conversational hit from a tool-payload hit, and the snippet for + // the latter is command output rather than anything anyone said. + "matched_tool_name": nil, } if r.ParentSessionID != "" { result["parent_session_id"] = r.ParentSessionID } + if r.MatchedToolName != "" { + result["matched_tool_name"] = r.MatchedToolName + } compactResults = append(compactResults, result) } diff --git a/internal/mcp/toolpayloads_test.go b/internal/mcp/toolpayloads_test.go new file mode 100644 index 0000000..84bc171 --- /dev/null +++ b/internal/mcp/toolpayloads_test.go @@ -0,0 +1,91 @@ +// ABOUTME: Tests that search_conversations surfaces tool-payload hits and labels them. +// ABOUTME: An agent has to be able to tell a conversational hit from a command-output hit. + +package mcp + +import ( + "strings" + "testing" + "time" + + "github.com/2389-research/ccvault/pkg/models" +) + +// TestSearchConversations_FindsAndLabelsAToolPayloadHit covers the new +// searchable material end to end through the MCP surface. The command exists +// only in a stored tool input; the turn's own content names the tool and +// nothing else. +func TestSearchConversations_FindsAndLabelsAToolPayloadHit(t *testing.T) { + s, database := newTestServer(t) + p := seedProject(t, database, "/test/proj") + seedSession(t, database, "session-1", p.ID) + + uses := []models.ToolUse{{ + TurnID: "session-1-turn-1", + SessionID: "session-1", + ToolName: "Bash", + Timestamp: time.Date(2026, 1, 1, 0, 0, 1, 0, time.UTC), + ToolUseID: "toolu_MCP", + InputJSON: `{"command":"terraform apply -auto-approve"}`, + InputLength: 43, + HasResult: true, + // Deliberately not a word anyone would type into a conversation. + ResultContent: "Apply complete! Resources: 3 added, 0 changed, 0 destroyed.", + ResultLength: 58, + }} + if err := database.InsertToolUses(uses); err != nil { + t.Fatalf("insert tool uses: %v", err) + } + + result, err := s.searchConversations(map[string]interface{}{"query": `"terraform apply"`}) + if err != nil { + t.Fatalf("searchConversations: %v", err) + } + + m := resultMap(t, result) + if count := mustInt(t, m, "count"); count != 1 { + t.Fatalf("count = %v, want 1", count) + } + + results := mustField[[]map[string]interface{}](t, m, "results") + if len(results) != 1 { + t.Fatalf("got %d results, want 1", len(results)) + } + hit := results[0] + + matched, ok := hit["matched_tool_name"].(string) + if !ok || matched != "Bash" { + t.Errorf("matched_tool_name = %v, want Bash", hit["matched_tool_name"]) + } + snippet, _ := hit["snippet"].(string) + if !strings.Contains(snippet, "terraform apply") { + t.Errorf("snippet = %q, want it to show the matching command", snippet) + } +} + +// TestSearchConversations_ConversationalHitHasNoMatchedTool is the other side +// of the label: present for payload hits, null for everything else, so an +// agent can rely on it rather than inferring. +func TestSearchConversations_ConversationalHitHasNoMatchedTool(t *testing.T) { + s, database := newTestServer(t) + p := seedProject(t, database, "/test/proj") + seedSession(t, database, "session-1", p.ID) + + result, err := s.searchConversations(map[string]interface{}{"query": "hello"}) + if err != nil { + t.Fatalf("searchConversations: %v", err) + } + + m := resultMap(t, result) + results := mustField[[]map[string]interface{}](t, m, "results") + if len(results) == 0 { + t.Fatal("no results for the seeded turn content") + } + raw, present := results[0]["matched_tool_name"] + if !present { + t.Fatalf("matched_tool_name is missing; the response has %v", sortedKeys(results[0])) + } + if raw != nil { + t.Errorf("matched_tool_name = %v, want nil for a content hit", raw) + } +} diff --git a/internal/search/search.go b/internal/search/search.go index adea1fe..9079f80 100644 --- a/internal/search/search.go +++ b/internal/search/search.go @@ -36,6 +36,15 @@ type Result struct { // listings — the work a subagent did is most of what there is to find — // so renders use this to label a hit with the session that dispatched it. ParentSessionID string `json:"parent_session_id,omitempty"` + + // MatchedToolName names the tool whose stored input or result matched the + // query, empty when the match was in the turn's own content. + // + // It is not decoration. A turn that issued a tool call summarises itself + // as "[Tool: Bash]", so a hit found through the payload would otherwise + // render a snippet with nothing of the query in it and no indication why + // it was returned. + MatchedToolName string `json:"matched_tool_name,omitempty"` } // Search executes a search query and returns results @@ -58,6 +67,7 @@ func (s *Searcher) Search(q *Query, limit int) ([]Result, error) { var r Result var content sql.NullString var parentSessionID sql.NullString + var matchedTool, matchedPayload sql.NullString err := rows.Scan( &r.Turn.ID, &r.SessionID, @@ -69,6 +79,8 @@ func (s *Searcher) Search(q *Query, limit int) ([]Result, error) { &r.Model, &r.Source, &parentSessionID, + &matchedTool, + &matchedPayload, ) if err != nil { return nil, fmt.Errorf("scan result: %w", err) @@ -78,6 +90,17 @@ func (s *Searcher) Search(q *Query, limit int) ([]Result, error) { r.Turn.Content = content.String r.Snippet = makeSnippet(content.String, q.Text, 150) } + // Prefer the turn's own content when it actually holds the query, and + // fall back to the payload that matched. The fallback is what makes a + // payload hit legible: a turn that issued a tool call has + // "[Tool: Bash]" for content, which says nothing about why it matched. + // + // matchedPayload is already centred on the match by fts5's snippet(), + // so it only needs whitespace flattening, not re-snipping. + if matchedPayload.Valid && !containsFold(content.String, q.Text) { + r.MatchedToolName = matchedTool.String + r.Snippet = flattenWhitespace(matchedPayload.String) + } r.Turn.SessionID = r.SessionID results = append(results, r) } @@ -85,12 +108,63 @@ func (s *Searcher) Search(q *Query, limit int) ([]Result, error) { return results, rows.Err() } +// containsFold reports whether haystack holds needle, ignoring ASCII case. +// Used only to decide which text a snippet is drawn from, never to decide +// whether a row matched — FTS5 has already answered that, and it tokenizes +// rather than substring-matching, so the two disagree on quoted phrases and +// prefix queries. Disagreeing in this direction is harmless: the payload +// snippet is the more informative of the two. +func containsFold(haystack, needle string) bool { + if needle == "" { + return false + } + return strings.Contains(strings.ToLower(haystack), strings.ToLower(strings.Trim(needle, `"`))) +} + +// flattenWhitespace collapses newlines and runs of spaces so a snippet taken +// from command output renders on one line, the same shaping makeSnippet applies +// to turn content. +func flattenWhitespace(s string) string { + return strings.Join(strings.Fields(strings.ReplaceAll(s, "\n", " ")), " ") +} + // buildQuery constructs the SQL query from parsed search func (s *Searcher) buildQuery(q *Query, limit int) (string, []interface{}) { var conditions []string var args []interface{} argNum := 1 + // Text search spans two indexes: turns_fts over the conversation itself, + // and tool_uses_fts over the stored tool inputs and results. A caller + // looking for a command they ran or an error a tool printed is searching + // for text that exists only in the second one, which is what issue #28 was + // about — the turn that issued the call summarises itself as "[Tool: Bash]" + // and holds none of it. + // + // Driven off a UNION of turn rowids rather than two LEFT JOINs with an OR + // between them. Both indexes answer a MATCH with a small set, so joining + // turns to that set is an index lookup; an OR across outer joins would + // make the planner scan all 965,000 turns. UNION also collapses a turn + // whose content and several of whose payloads all matched down to the one + // row the caller asked for. + prefix := "" + matchArg := 0 + + if q.Text != "" { + matchArg = argNum + prefix = fmt.Sprintf(`text_hits AS ( + SELECT rowid AS turn_rowid FROM turns_fts WHERE turns_fts MATCH $%d + UNION + SELECT ht.rowid FROM tool_uses_fts + JOIN tool_uses htu ON htu.id = tool_uses_fts.rowid + JOIN turns ht ON ht.id = htu.turn_id + WHERE tool_uses_fts MATCH $%d + )`, matchArg, matchArg) + // Escape the search text for FTS5 to handle special characters like hyphens + args = append(args, escapeFTS5Query(q.Text)) + argNum++ + } + // Base query with joins baseQuery := ` SELECT DISTINCT t.id, t.session_id, t.type, t.timestamp, t.ordinal, t.content, @@ -99,13 +173,8 @@ func (s *Searcher) buildQuery(q *Query, limit int) (string, []interface{}) { JOIN sessions s ON t.session_id = s.id JOIN projects p ON s.project_id = p.id` - // FTS join if text search if q.Text != "" { - baseQuery += ` JOIN turns_fts fts ON t.rowid = fts.rowid` - conditions = append(conditions, fmt.Sprintf("turns_fts MATCH $%d", argNum)) - // Escape the search text for FTS5 to handle special characters like hyphens - args = append(args, escapeFTS5Query(q.Text)) - argNum++ + baseQuery += ` JOIN text_hits ON text_hits.turn_rowid = t.rowid` } // Tool filter requires join @@ -188,7 +257,42 @@ func (s *Searcher) buildQuery(q *Query, limit int) (string, []interface{}) { baseQuery += " ORDER BY t.timestamp DESC, t.id ASC" baseQuery += fmt.Sprintf(" LIMIT %d", limit) - return baseQuery, args + if q.Text == "" { + // No text, so no payload to attribute a match to. The two trailing + // columns are still selected so Search scans one row shape. + return "SELECT *, NULL, NULL FROM (" + baseQuery + ")", args + } + + // The page is computed first and the payload lookup applied to it, not the + // other way round. + // + // SQLite evaluates result-column subqueries while feeding rows to the + // sorter, which is before LIMIT takes effect. Selecting these alongside + // the page would run two correlated lookups for every candidate row a + // common word matched — tens of thousands of them on a real archive — to + // use twenty. Wrapping puts them after the LIMIT, so the cost is the page + // size rather than the match size. + // + // fts5's own snippet() rather than picking a column and letting + // makeSnippet centre it. Column -1 asks fts5 which of input_json and + // result_content actually matched, which is a question the row cannot + // answer for itself: a command lives in the input and an error message in + // the result, so COALESCE-ing to one of them shows the wrong half about as + // often as the right one. + // + // LIMIT 1 on each: a turn may have several matching calls, and the snippet + // shows one of them. + return fmt.Sprintf(`WITH %s, page AS (%s) + SELECT page.*, + (SELECT mtu.tool_name FROM tool_uses_fts + JOIN tool_uses mtu ON mtu.id = tool_uses_fts.rowid + WHERE tool_uses_fts MATCH $%d AND mtu.turn_id = page.id LIMIT 1), + (SELECT snippet(tool_uses_fts, -1, '', '', '…', 24) FROM tool_uses_fts + JOIN tool_uses mtu ON mtu.id = tool_uses_fts.rowid + WHERE tool_uses_fts MATCH $%d AND mtu.turn_id = page.id LIMIT 1) + FROM page + ORDER BY page.timestamp DESC, page.id ASC`, + prefix, baseQuery, matchArg, matchArg), args } // makeSnippet creates a snippet from content with the search term highlighted diff --git a/internal/search/toolpayloads_test.go b/internal/search/toolpayloads_test.go new file mode 100644 index 0000000..5f6f4e9 --- /dev/null +++ b/internal/search/toolpayloads_test.go @@ -0,0 +1,241 @@ +// ABOUTME: Tests that a text search reaches tool inputs and results, not just turn content. +// ABOUTME: This is the behaviour change issue #28 exists for — finding a command you ran or an error a tool printed. + +package search + +import ( + "strings" + "testing" + "time" + + "github.com/2389-research/ccvault/internal/db" + "github.com/2389-research/ccvault/pkg/models" + "github.com/2389-research/ccvault/pkg/toolpayload" +) + +// setupPayloadSearchDB builds an archive where the searchable material is +// deliberately split: the turn's own content says nothing a caller would +// search for, and everything interesting is in a tool payload. +func setupPayloadSearchDB(t *testing.T) *db.DB { + t.Helper() + + database, err := db.Open(t.TempDir()) + if err != nil { + t.Fatalf("open db: %v", err) + } + t.Cleanup(func() { _ = database.Close() }) + + p := &models.Project{Path: "/test/payloads", DisplayName: "payloads"} + if err := database.UpsertProject(p); err != nil { + t.Fatalf("upsert project: %v", err) + } + s := &models.Session{ID: "session-p", ProjectID: p.ID, StartedAt: time.Now(), SourceFile: "/p.jsonl"} + if err := database.UpsertSession(s); err != nil { + t.Fatalf("upsert session: %v", err) + } + + now := time.Now() + turns := []models.Turn{ + {ID: "turn-cmd", SessionID: "session-p", Type: "assistant", Timestamp: now, Ordinal: 0, + Content: "[Tool: Bash]"}, + {ID: "turn-err", SessionID: "session-p", Type: "assistant", Timestamp: now.Add(time.Second), Ordinal: 1, + Content: "[Tool: Bash]"}, + {ID: "turn-read", SessionID: "session-p", Type: "assistant", Timestamp: now.Add(2 * time.Second), Ordinal: 2, + Content: "[Tool: Read]"}, + {ID: "turn-plain", SessionID: "session-p", Type: "user", Timestamp: now.Add(3 * time.Second), Ordinal: 3, + Content: "please look at the deploy script"}, + } + if err := database.InsertTurns(turns); err != nil { + t.Fatalf("insert turns: %v", err) + } + + toolUses := []models.ToolUse{ + {TurnID: "turn-cmd", SessionID: "session-p", ToolName: "Bash", Timestamp: now, + ToolUseID: "toolu_CMD", InputJSON: `{"command":"kubectl rollout restart deployment/api"}`, + InputLength: 52, HasResult: true, ResultContent: "deployment.apps/api restarted", ResultLength: 29}, + {TurnID: "turn-err", SessionID: "session-p", ToolName: "Bash", Timestamp: now.Add(time.Second), + ToolUseID: "toolu_ERR", InputJSON: `{"command":"go build ./..."}`, + InputLength: 28, HasResult: true, + ResultContent: "cannot use warning (variable of type string) as []string value", ResultLength: 61}, + {TurnID: "turn-read", SessionID: "session-p", ToolName: "Read", Timestamp: now.Add(2 * time.Second), + ToolUseID: "toolu_READ", FilePath: "/etc/secrets.conf", + InputJSON: `{"file_path":"/etc/secrets.conf"}`, InputLength: 33, + HasResult: true, ResultLength: 400000, ResultOmittedReason: toolpayload.OmitBulkRead}, + } + if err := database.InsertToolUses(toolUses); err != nil { + t.Fatalf("insert tool uses: %v", err) + } + + return database +} + +func resultIDs(results []Result) []string { + ids := make([]string, 0, len(results)) + for _, r := range results { + ids = append(ids, r.Turn.ID) + } + return ids +} + +// TestSearch_FindsACommandYouRan is the headline of issue #28. The command +// text exists nowhere in turns.content — the turn summarises itself as +// "[Tool: Bash]" — so before this change the query could not match anything. +func TestSearch_FindsACommandYouRan(t *testing.T) { + searcher := New(setupPayloadSearchDB(t).DB) + + results, err := searcher.Search(Parse(`"kubectl rollout"`), 10) + if err != nil { + t.Fatalf("search: %v", err) + } + if len(results) != 1 { + t.Fatalf("got %d results %v, want 1 (turn-cmd)", len(results), resultIDs(results)) + } + if results[0].Turn.ID != "turn-cmd" { + t.Errorf("matched %q, want turn-cmd", results[0].Turn.ID) + } +} + +// TestSearch_FindsAnErrorAToolPrinted covers the other half: text that only +// ever existed in a tool's output. +func TestSearch_FindsAnErrorAToolPrinted(t *testing.T) { + searcher := New(setupPayloadSearchDB(t).DB) + + results, err := searcher.Search(Parse(`"variable of type string"`), 10) + if err != nil { + t.Fatalf("search: %v", err) + } + if len(results) != 1 || results[0].Turn.ID != "turn-err" { + t.Fatalf("got %v, want just turn-err", resultIDs(results)) + } +} + +// TestSearch_SnippetShowsTheMatchingPayload covers what the caller sees. A hit +// whose match is in a tool payload has to render the payload — a snippet taken +// from turn content would show "[Tool: Bash]" and leave the user unable to see +// why the row matched. +func TestSearch_SnippetShowsTheMatchingPayload(t *testing.T) { + searcher := New(setupPayloadSearchDB(t).DB) + + results, err := searcher.Search(Parse(`"kubectl rollout"`), 10) + if err != nil { + t.Fatalf("search: %v", err) + } + if len(results) != 1 { + t.Fatalf("got %d results, want 1", len(results)) + } + if !strings.Contains(results[0].Snippet, "kubectl rollout restart") { + t.Errorf("Snippet = %q, want it to show the matching command", results[0].Snippet) + } + if results[0].MatchedToolName != "Bash" { + t.Errorf("MatchedToolName = %q, want Bash", results[0].MatchedToolName) + } +} + +// TestSearch_TurnContentStillMatches guards against the widening becoming a +// replacement. Ordinary conversational search has to keep working, and a turn +// with no tool uses at all has to stay findable. +func TestSearch_TurnContentStillMatches(t *testing.T) { + searcher := New(setupPayloadSearchDB(t).DB) + + results, err := searcher.Search(Parse(`"deploy script"`), 10) + if err != nil { + t.Fatalf("search: %v", err) + } + if len(results) != 1 || results[0].Turn.ID != "turn-plain" { + t.Fatalf("got %v, want just turn-plain", resultIDs(results)) + } + if results[0].MatchedToolName != "" { + t.Errorf("MatchedToolName = %q, want empty for a content match", results[0].MatchedToolName) + } + if !strings.Contains(results[0].Snippet, "deploy script") { + t.Errorf("Snippet = %q, want the turn content", results[0].Snippet) + } +} + +// TestSearch_OmittedContentIsNotSearchable pins the storage policy's effect on +// search. The Read above carries a 400 KB result that was never stored, so its +// body cannot be matched — but the call itself still is, through its input. +func TestSearch_OmittedContentIsNotSearchable(t *testing.T) { + searcher := New(setupPayloadSearchDB(t).DB) + + results, err := searcher.Search(Parse(`"secrets.conf"`), 10) + if err != nil { + t.Fatalf("search: %v", err) + } + if len(results) != 1 || results[0].Turn.ID != "turn-read" { + t.Fatalf("got %v, want turn-read found through its input", resultIDs(results)) + } +} + +// TestSearch_ResultsAreNotDuplicatedByMultipleToolMatches covers a turn whose +// payloads match the query more than once. The result list is turns, so the +// turn has to appear once however many of its tool uses matched. +func TestSearch_ResultsAreNotDuplicatedByMultipleToolMatches(t *testing.T) { + database := setupPayloadSearchDB(t) + + now := time.Now() + extra := []models.ToolUse{ + {TurnID: "turn-cmd", SessionID: "session-p", ToolName: "Grep", Timestamp: now, + ToolUseID: "toolu_X1", InputJSON: `{"pattern":"duplicateprobe"}`, InputLength: 28}, + {TurnID: "turn-cmd", SessionID: "session-p", ToolName: "Glob", Timestamp: now, + ToolUseID: "toolu_X2", InputJSON: `{"pattern":"duplicateprobe"}`, InputLength: 28}, + } + if err := database.InsertToolUses(extra); err != nil { + t.Fatalf("insert tool uses: %v", err) + } + + results, err := New(database.DB).Search(Parse("duplicateprobe"), 10) + if err != nil { + t.Fatalf("search: %v", err) + } + if len(results) != 1 { + t.Errorf("got %d results %v, want 1 — one turn, two matching tool uses", len(results), resultIDs(results)) + } +} + +// TestSearch_ToolFilterStillComposes checks the widened text match against the +// other operators, which must keep applying. +// +// tool: is session-scoped, not turn-scoped — it joins tool_uses on session_id, +// so it asks "did this session use the tool" rather than "did this turn call +// it". That predates this change and is left alone here; the filter is +// exercised with a tool the session never used, so composition is what is +// being tested rather than the scope. +func TestSearch_ToolFilterStillComposes(t *testing.T) { + searcher := New(setupPayloadSearchDB(t).DB) + + results, err := searcher.Search(Parse(`tool:WebFetch "kubectl rollout"`), 10) + if err != nil { + t.Fatalf("search: %v", err) + } + if len(results) != 0 { + t.Errorf("got %v, want none — the session used no WebFetch", resultIDs(results)) + } + + // And the same text with a tool the session did use still comes back. + results, err = searcher.Search(Parse(`tool:Bash "kubectl rollout"`), 10) + if err != nil { + t.Fatalf("search: %v", err) + } + if len(results) != 1 || results[0].Turn.ID != "turn-cmd" { + t.Errorf("got %v, want turn-cmd", resultIDs(results)) + } +} + +// TestSearch_ProjectFilterStillComposes covers a filter that is turn-scoped +// through the session join, against a payload-only text match. +func TestSearch_ProjectFilterStillComposes(t *testing.T) { + searcher := New(setupPayloadSearchDB(t).DB) + + if results, err := searcher.Search(Parse(`project:payloads "kubectl rollout"`), 10); err != nil { + t.Fatalf("search: %v", err) + } else if len(results) != 1 { + t.Errorf("got %v, want 1 for the matching project", resultIDs(results)) + } + + if results, err := searcher.Search(Parse(`project:nosuchproject "kubectl rollout"`), 10); err != nil { + t.Fatalf("search: %v", err) + } else if len(results) != 0 { + t.Errorf("got %v, want none for a non-matching project", resultIDs(results)) + } +} diff --git a/internal/tui/search.go b/internal/tui/search.go index 1699f9a..f4c1255 100644 --- a/internal/tui/search.go +++ b/internal/tui/search.go @@ -376,8 +376,13 @@ func (m *SearchModel) View() string { } b.WriteString("\n") - // Snippet (indented) + // Snippet (indented). A hit found through a stored tool + // payload is labelled, because the text below is command + // output or tool arguments rather than conversation. snippet := r.Snippet + if r.MatchedToolName != "" { + snippet = "[" + r.MatchedToolName + "] " + snippet + } maxSnippetLen := m.width - 6 if maxSnippetLen < 20 { maxSnippetLen = 20 diff --git a/skills/ccvault/reference.md b/skills/ccvault/reference.md index 354c1aa..eebc9b5 100644 --- a/skills/ccvault/reference.md +++ b/skills/ccvault/reference.md @@ -4,7 +4,7 @@ | Tool | Required Params | Optional Params | Returns | Notes | |------|----------------|-----------------|---------|-------| -| `search_conversations` | `query` (string) | `limit` (number, default 10, max 50), `offset` (number) | Paginated results with 200-char snippets | Check `has_more` / `next_offset` for pagination. Each result carries `project_name` (the adapter-provided label, or the path basename as fallback) alongside `project_path`. An empty result for a `tool:` query adds `similar_tool_names` with close matches; both that lookup and the project-name enrichment are enrichment queries — if one fails, its field is omitted and a `warnings` entry (`similar_tool_names unavailable: …` / `project enrichment unavailable: …`) appears instead. Search is **never** filtered by the subagent listing default, so a hit can sit in a transcript `list_sessions` does not show: each result carries `parent_session_id` (null when the hit is in a top-level session) | +| `search_conversations` | `query` (string) | `limit` (number, default 10, max 50), `offset` (number) | Paginated results with 200-char snippets | Check `has_more` / `next_offset` for pagination. Each result carries `project_name` (the adapter-provided label, or the path basename as fallback) alongside `project_path`. An empty result for a `tool:` query adds `similar_tool_names` with close matches; both that lookup and the project-name enrichment are enrichment queries — if one fails, its field is omitted and a `warnings` entry (`similar_tool_names unavailable: …` / `project enrichment unavailable: …`) appears instead. Search is **never** filtered by the subagent listing default, so a hit can sit in a transcript `list_sessions` does not show: each result carries `parent_session_id` (null when the hit is in a top-level session). A query also matches **stored tool inputs and results**, not only turn text — so a command you ran or an error a tool printed is findable. Such a hit carries `matched_tool_name` (the tool whose payload matched; null when the match was in the turn's own content) and its `snippet` is drawn from that payload rather than from the conversation | | `get_session_summary` | `session_id` (string) | — | Metadata, turn counts by type, top 10 tools used, first/last user messages (500 chars each) | Most cost-effective entry point for any session | | `get_turns` | `session_id` (string) | `offset` (number, default 0), `limit` (number, default 20, max 50), `type` (user/assistant/tool_result) | Paginated turns, content truncated at 1000 chars | Includes tool names; `has_thinking` flag on assistant turns. Accepts a subagent session id with no extra parameter | | `get_session` | `session_id` (string) | — | Full session as markdown | Sessions over 100 turns come back without `markdown`: the response carries `session_id`, `turn_count`, `hint`, and a `markdown unavailable: large session with N turns...` entry in the top-level `warnings[]` array — there is no singular `warning` field. Markdown truncates at 50K chars. Prefer summary + turns for large sessions | @@ -23,7 +23,7 @@ Pagination is uniform across `search_conversations`, `get_turns`, `list_sessions |----------|--------|---------|-------| | Project | `project:name` | `project:myapp` | Partial match on path or display name | | Model | `model:name` | `model:opus` | Partial match (opus, sonnet, haiku) | -| Tool | `tool:Name` | `tool:Bash` | Case-insensitive, must match the full tool name (e.g., `Bash`, `Read`, `Edit`, `Write`, `Grep`, `Glob`, `Task`, `WebFetch`; MCP tools are stored under their full prefixed names like `mcp__ccvault__search_conversations`) | +| Tool | `tool:Name` | `tool:Bash` | Filters to **sessions** that used the tool, not to turns that called it. Case-insensitive, must match the full tool name (e.g., `Bash`, `Read`, `Edit`, `Write`, `Grep`, `Glob`, `Task`, `WebFetch`; MCP tools are stored under their full prefixed names like `mcp__ccvault__search_conversations`) | | File | `file:path` | `file:auth.py` | Matches file paths mentioned in session | | Before | `before:DATE` | `before:2026-02-01` | See date formats below | | After | `after:DATE` | `after:thisweek` | See date formats below | @@ -32,6 +32,8 @@ Pagination is uniform across `search_conversations`, `get_turns`, `list_sessions | Exact phrase | `"phrase"` | `"deploy script"` | Quoted exact phrase matching | | Free text | `terms` | `authentication bug` | FTS5 full-text search on unquoted terms | +Text matching covers two indexes: the conversation (`turns.content`) and the stored tool payloads (`tool_uses.input_json`, `tool_uses.result_content`). What is **not** matchable is content the storage policy leaves out: the body of a `Read`/`NotebookRead` result, and any result carrying an image. Those rows still exist and are still findable through their input and `file_path` — only the returned bytes are absent, and `result_length` plus `result_omitted_reason` say so on the row. A result over 128 KB is left out the same way, though nothing in the measured archive reaches that. + Operators combine freely: `project:myapp tool:Bash "deploy" after:thisweek` ## 3. Date Formats @@ -97,6 +99,24 @@ tool:Edit project:myapp after:month | Full session markdown | 50,000 chars | | Tools list in summary | Top 10 | +Tool payloads are **not** truncated. A stored tool input or result is stored +whole or not at all — never as a prefix — because `turns.raw_json` keeps the +full original regardless, so a prefix would cost storage without preserving +anything. What is omitted is omitted completely and says so: + +| `result_omitted_reason` | Means | +|---|---| +| `null` | The content is stored whole in `result_content` | +| `bulk_read` | A `Read`/`NotebookRead` result. `file_path` on the row says what was read | +| `image` | The result carried an image block, detected by content shape rather than tool name | +| `oversize` | Over 128 KB. Fires on nothing in the measured archive; it is there for unclassified future tools | +| `undecodable` | The content was not a shape the extractor reads | + +`result_length` is always recorded, so a short result (`result_length` small, +`result_content` present) is distinguishable from an omitted one without +guessing. `result_length IS NULL` is a third thing again: nothing ever +answered that call. + ## 7. Subagent Sessions A subagent (`Task`/Agent dispatch) writes its own transcript, which the archive From 455a35a6cd1d6681ddfa84d720d9a33d246dbc32 Mon Sep 17 00:00:00 2001 From: Dylan Richard Date: Fri, 2 Oct 2026 20:43:07 -0500 Subject: [PATCH 5/8] test: drive the real binary through the 009 upgrade an existing archive takes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three integration tests against the built binary: * TestUpgrade_BackfillsPayloadsWithoutReparsingAnything syncs, strips the payload columns back off to recreate a pre-009 archive, and re-syncs. The session file never changes, so the second sync reports 0 indexed and writes no turns and no tool_uses rows — meaning everything in a payload column afterwards came from the migration reading raw_json. * TestSync_WritesPayloadsOnAFreshArchive pins the live path against the same transcript, because the backfill and the parser are the same policy implemented twice and have to agree. * TestSync_ReSyncLeavesNoOrphanedFTSEntries touches the file so sync re-parses it, then compares tool_uses_fts_docsize against tool_uses. That shadow table is the only place an orphan is visible: #42 established COUNT(*) on an external-content FTS table resolves through the base table, and integrity-check with no argument does too. The rehearsal was also run with a genuinely pre-change binary built from main, which is the part a Go test cannot do — this binary ships the migration, so merely opening the archive applies it and the pre-upgrade state is unobservable. The old binary answers "No results found." for both needles; the new one finds both and labels them "Matched in Bash payload"; turns.content is byte-identical across the upgrade. Picking the fixture's tokens corrected a wrong assumption worth recording. turns.content was never empty of tool material: a call renders as "[Tool: Bash] $ " cut at 100 characters and a string-valued result as "[Tool Result: ]" cut at 200, so short commands and short string results have always been findable in summary form. The first draft of this test asserted a short command was unfindable and failed, correctly. The two needles it uses now sit where turns.content genuinely cannot reach: past the 100-character cut, and inside an array-valued tool_result — the shape half the archive's results use, which fails the content unmarshal outright and leaves the turn with no content at all. Co-Authored-By: Claude Opus 5 --- test/integration/toolpayloads_test.go | 316 ++++++++++++++++++++++++++ 1 file changed, 316 insertions(+) create mode 100644 test/integration/toolpayloads_test.go diff --git a/test/integration/toolpayloads_test.go b/test/integration/toolpayloads_test.go new file mode 100644 index 0000000..be93887 --- /dev/null +++ b/test/integration/toolpayloads_test.go @@ -0,0 +1,316 @@ +// ABOUTME: Drives the real binary through the upgrade an existing archive takes when migration 009 lands. +// ABOUTME: The source file is unchanged, so sync skips it and the payloads can only have come from the backfill. + +package integration + +import ( + "database/sql" + "fmt" + "os" + "path/filepath" + "strings" + "testing" + "time" + + _ "modernc.org/sqlite" +) + +// timeLater is a timestamp comfortably after the fixture's recorded mtimes, so +// a touched file reads as newer than what source_files holds. +func timeLater() time.Time { return time.Now().Add(time.Hour) } + +// The two needles this fixture proves are newly reachable, chosen because +// turns.content cannot hold either of them. +// +// turns.content is not empty of tool material — it never was. An assistant +// turn renders a call as "[Tool: Bash] $ " with the command cut at +// 100 characters, and a string-valued tool_result renders as +// "[Tool Result: ]" cut at 200. So short commands and short string +// results have always been findable, in summary form. +// +// What was never reachable is what these two needles sit in: +// +// - commandNeedle is past character 100 of its command, so the summary +// truncates it away. +// - resultNeedle is inside an array-valued tool_result. Half the archive's +// results use that shape (130,516 of 259,812), and +// models.UserContentBlock types Content as a string, so the array fails +// the unmarshal and the turn stores no content at all. +const ( + commandNeedle = "lithesome-walrus" + resultNeedle = "quixotic-badger" +) + +// toolSessionJSONL is a session holding a tool call and its result, in the +// shape Claude Code writes: the tool_use in an assistant message, the +// tool_result in the user message that follows, joined by tool_use_id. +const toolSessionJSONL = `{"uuid":"%[1]s-t1","parentUuid":null,"type":"user","message":{"role":"user","content":"check the cluster"},"timestamp":"2026-01-01T00:00:01Z","sessionId":%[1]q,"cwd":"/Users/test/fixture","version":"1.0"} +{"uuid":"%[1]s-t2","parentUuid":"%[1]s-t1","type":"assistant","message":{"id":"%[1]s-m1","model":"claude-sonnet-4-20250514","role":"assistant","content":[{"type":"tool_use","id":"toolu_UPGRADE","name":"Bash","input":{"command":"kubectl --namespace production --context staging-west describe pod api-gateway-7d9f8b6c5d --output wide ` + commandNeedle + `"}}],"usage":{"input_tokens":11,"output_tokens":7}},"timestamp":"2026-01-01T00:00:02Z","sessionId":%[1]q} +{"uuid":"%[1]s-t3","parentUuid":"%[1]s-t2","type":"user","message":{"role":"user","content":[{"type":"tool_result","tool_use_id":"toolu_UPGRADE","content":[{"type":"text","text":"Events: FailedScheduling insufficient ephemeral-storage on node ` + resultNeedle + `"}]}]},"timestamp":"2026-01-01T00:00:03Z","sessionId":%[1]q} +` + +// writeToolSession writes a session whose only interesting text is inside a +// tool payload. +func writeToolSession(t *testing.T, claudeHome, sessionID string) string { + t.Helper() + + dir := filepath.Join(claudeHome, "projects", "-Users-test-fixture") + if err := os.MkdirAll(dir, 0o755); err != nil { + t.Fatalf("mkdir %s: %v", dir, err) + } + path := filepath.Join(dir, sessionID+".jsonl") + if err := os.WriteFile(path, []byte(fmt.Sprintf(toolSessionJSONL, sessionID)), 0o644); err != nil { + t.Fatalf("write %s: %v", path, err) + } + return path +} + +// openArchive opens the fixture's database read-write for the surgery and +// assertions below. +func openArchive(t *testing.T, f *cliFixture) *sql.DB { + t.Helper() + + database, err := sql.Open("sqlite", filepath.Join(f.DataDir, "ccvault.db")) + if err != nil { + t.Fatalf("open archive: %v", err) + } + database.SetMaxOpenConns(1) + t.Cleanup(func() { _ = database.Close() }) + return database +} + +// rewindBelow009 turns a fully-migrated archive back into the one the previous +// release left behind: the payload columns gone, the index over them gone, and +// schema_version back to 8. +// +// Reconstructing the old shape rather than building the old binary. The two +// produce the same archive — every other table, and crucially source_files +// with its recorded mtimes, is written by code this change does not touch — so +// the only difference between them is the six columns dropped here. +func rewindBelow009(t *testing.T, database *sql.DB) { + t.Helper() + + stmts := []string{ + // The FTS table and its triggers reference the columns, so they go first. + "DROP TRIGGER IF EXISTS tool_uses_ai", + "DROP TRIGGER IF EXISTS tool_uses_ad", + "DROP TRIGGER IF EXISTS tool_uses_au", + "DROP TABLE IF EXISTS tool_uses_fts", + "DROP INDEX IF EXISTS idx_tool_uses_tool_use_id", + "ALTER TABLE tool_uses DROP COLUMN tool_use_id", + "ALTER TABLE tool_uses DROP COLUMN input_json", + "ALTER TABLE tool_uses DROP COLUMN input_length", + "ALTER TABLE tool_uses DROP COLUMN result_content", + "ALTER TABLE tool_uses DROP COLUMN result_length", + "ALTER TABLE tool_uses DROP COLUMN result_omitted_reason", + "DELETE FROM schema_version WHERE version >= 9", + } + for _, stmt := range stmts { + if _, err := database.Exec(stmt); err != nil { + t.Fatalf("rewind %q: %v", stmt, err) + } + } + + // Guard the guard. If the rewind silently left a column behind, the + // assertions after the re-sync would prove nothing. + var count int + if err := database.QueryRow( + `SELECT COUNT(*) FROM pragma_table_info('tool_uses') + WHERE name IN ('tool_use_id','input_json','input_length', + 'result_content','result_length','result_omitted_reason')`).Scan(&count); err != nil { + t.Fatalf("probe columns: %v", err) + } + if count != 0 { + t.Fatalf("%d payload column(s) survived the rewind, so the upgrade test proves nothing", count) + } +} + +// TestUpgrade_BackfillsPayloadsWithoutReparsingAnything is the rehearsal that +// matters for an existing archive: the migration has to recover the payloads +// from raw_json, not quietly rely on a re-sync having re-read every file. +// +// The proof is the skip. Between the two runs the session file is untouched, +// so its recorded mtime still matches and sync reports it skipped — no turn is +// re-parsed, no tool_uses row is rewritten. Anything in a payload column +// afterwards came from migration 009 reading raw_json and nowhere else. +func TestUpgrade_BackfillsPayloadsWithoutReparsingAnything(t *testing.T) { + f := newCLIFixture(t) + sessionID := "11111111-2222-3333-4444-555555555555" + writeToolSession(t, f.ClaudeHome, sessionID) + + // First run: a normal sync, which is what the previous release left behind + // once the payload columns are taken back off. + first := f.Run(t, "sync") + if !strings.Contains(first.Stdout, "Tool uses: 1") { + t.Fatalf("first sync did not index the tool call:\n%s", first.Stdout) + } + + database := openArchive(t, f) + rewindBelow009(t, database) + + // The archive is now a pre-009 archive, and rewindBelow009 has asserted + // that none of the payload columns survive. + // + // There is deliberately no "search finds nothing yet" probe here. This + // binary ships migration 009, so merely opening the archive applies it — + // the pre-upgrade state is not observable through the only binary a Go + // test has. The absence of the columns is the equivalent assertion, and it + // is stronger: a column that does not exist cannot be searched. The + // negative was confirmed separately by driving the actual pre-change + // binary, which answers "No results found." for both needles. + + // Second run: the new binary. Migrations run on open; sync then finds + // nothing to do. + second := f.Run(t, "sync") + // "0 indexed" is the load-bearing part: no session was re-parsed, so no + // turn and no tool_uses row was rewritten. The skipped count is whatever + // the fixture laid down, which is not what this test is about. + if !strings.Contains(second.Stdout, "0 indexed") || strings.Contains(second.Stdout, ", 0 skipped") { + t.Fatalf("the second sync re-parsed the sessions instead of skipping them, so the payloads "+ + "could have come from the parser rather than the backfill:\n%s", second.Stdout) + } + if !strings.Contains(second.Stdout, "Tool uses: 0") || !strings.Contains(second.Stdout, "Turns: 0") { + t.Fatalf("the second sync wrote rows, so the backfill is not what filled them:\n%s", + second.Stdout) + } + + t.Run("the payloads are on the row", func(t *testing.T) { + var toolUseID, input, result sql.NullString + var resultLen sql.NullInt64 + if err := database.QueryRow(`SELECT tool_use_id, input_json, result_content, result_length + FROM tool_uses WHERE tool_name = 'Bash'`).Scan(&toolUseID, &input, &result, &resultLen); err != nil { + t.Fatalf("read the backfilled row: %v", err) + } + if toolUseID.String != "toolu_UPGRADE" { + t.Errorf("tool_use_id = %q, want toolu_UPGRADE", toolUseID.String) + } + if !strings.Contains(input.String, commandNeedle) { + t.Errorf("input_json = %q, want the whole command including %q", input.String, commandNeedle) + } + if !strings.Contains(result.String, resultNeedle) { + t.Errorf("result_content = %q, want the tool output including %q", result.String, resultNeedle) + } + // An array-valued content records the content JSON's length, which runs + // ahead of the extracted text by the JSON framing. See + // toolpayload.Result.Length. + if resultLen.Int64 <= int64(len(result.String)) { + t.Errorf("result_length = %d, want more than the %d extracted bytes for an array content", + resultLen.Int64, len(result.String)) + } + }) + + t.Run("and searchable through the binary", func(t *testing.T) { + for _, probe := range []struct{ name, query, want string }{ + {"the tail of a long command, from the input", commandNeedle, commandNeedle}, + {"an array-shaped result, from the result", resultNeedle, resultNeedle}, + } { + t.Run(probe.name, func(t *testing.T) { + r := f.Run(t, "search", probe.query, "--json") + if !strings.Contains(r.Stdout, probe.want) { + t.Errorf("search %q found nothing containing %q:\n%s", probe.query, probe.want, r.Stdout) + } + if !strings.Contains(r.Stdout, `"matched_tool_name"`) && + !strings.Contains(r.Stdout, "matched_tool_name") { + t.Errorf("search %q did not label the payload hit:\n%s", probe.query, r.Stdout) + } + }) + } + }) + + t.Run("a third sync is still a no-op", func(t *testing.T) { + third := f.Run(t, "sync") + if !strings.Contains(third.Stdout, "0 indexed") || strings.Contains(third.Stdout, ", 0 skipped") { + t.Errorf("a settled archive re-synced:\n%s", third.Stdout) + } + }) +} + +// TestSync_WritesPayloadsOnAFreshArchive covers the other direction: a first +// sync has to produce the same row the backfill does, because the two are the +// same policy applied to the same transcript by different code. +func TestSync_WritesPayloadsOnAFreshArchive(t *testing.T) { + f := newCLIFixture(t) + sessionID := "66666666-7777-8888-9999-aaaaaaaaaaaa" + writeToolSession(t, f.ClaudeHome, sessionID) + + f.Run(t, "sync") + + database := openArchive(t, f) + var toolUseID, input, result sql.NullString + var inputLen, resultLen sql.NullInt64 + var reason sql.NullString + if err := database.QueryRow(`SELECT tool_use_id, input_json, input_length, + result_content, result_length, result_omitted_reason + FROM tool_uses WHERE tool_name = 'Bash'`).Scan( + &toolUseID, &input, &inputLen, &result, &resultLen, &reason); err != nil { + t.Fatalf("read the row: %v", err) + } + + if toolUseID.String != "toolu_UPGRADE" { + t.Errorf("tool_use_id = %q, want toolu_UPGRADE", toolUseID.String) + } + if inputLen.Int64 != int64(len(input.String)) { + t.Errorf("input_length = %d, len(input_json) = %d", inputLen.Int64, len(input.String)) + } + if !strings.Contains(result.String, resultNeedle) { + t.Errorf("result_content = %q, want the tool output including %q", result.String, resultNeedle) + } + if resultLen.Int64 <= int64(len(result.String)) { + t.Errorf("result_length = %d, want more than the %d extracted bytes for an array content", + resultLen.Int64, len(result.String)) + } + if reason.Valid { + t.Errorf("result_omitted_reason = %q, want NULL", reason.String) + } +} + +// TestSync_ReSyncLeavesNoOrphanedFTSEntries applies the lesson from #42 to the +// new index, through the real binary. sync replaces a session's tool uses with +// a DELETE and an INSERT, and the docsize shadow table is the only place an +// orphan from that is visible — COUNT(*) on an external-content FTS table +// resolves through the base table and cannot see one. +func TestSync_ReSyncLeavesNoOrphanedFTSEntries(t *testing.T) { + f := newCLIFixture(t) + sessionID := "bbbbbbbb-cccc-dddd-eeee-ffffffffffff" + path := writeToolSession(t, f.ClaudeHome, sessionID) + + f.Run(t, "sync") + database := openArchive(t, f) + + count := func(query string) int64 { + var n int64 + if err := database.QueryRow(query).Scan(&n); err != nil { + t.Fatalf("%s: %v", query, err) + } + return n + } + + rows := count("SELECT COUNT(*) FROM tool_uses") + if docs := count("SELECT COUNT(*) FROM tool_uses_fts_docsize"); docs != rows { + t.Fatalf("after the first sync: %d indexed documents for %d rows", docs, rows) + } + + // Touch the file so the next sync re-parses it rather than skipping. + if err := os.Chtimes(path, timeLater(), timeLater()); err != nil { + t.Fatalf("chtimes: %v", err) + } + second := f.Run(t, "sync") + if !strings.Contains(second.Stdout, "1 indexed") { + t.Fatalf("the touched file was not re-parsed:\n%s", second.Stdout) + } + + rows = count("SELECT COUNT(*) FROM tool_uses") + docs := count("SELECT COUNT(*) FROM tool_uses_fts_docsize") + if docs != rows { + t.Errorf("after a re-sync: %d indexed documents for %d rows — %d orphan(s) left behind", + docs, rows, docs-rows) + } + + // integrity-check with argument 1 is the form that inspects the index + // itself; with no argument it resolves through the content table and + // reports clean regardless. + if _, err := database.Exec( + `INSERT INTO tool_uses_fts(tool_uses_fts, rank) VALUES('integrity-check', 1)`); err != nil { + t.Errorf("tool_uses_fts integrity-check(1): %v", err) + } +} From a20aec95208b8a4618e8f68ad4f5fae6ae52b5be Mon Sep 17 00:00:00 2001 From: Dylan Richard Date: Fri, 2 Oct 2026 20:50:37 -0500 Subject: [PATCH 6/8] docs: record the measured search cost of reading two indexes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Measured on a copy of the author's 965,061-turn archive against the same binary minus this change: "git commit" 0.56s -> 1.67s, "deploy" 0.38s -> 0.87s, a query matching nothing unchanged at 0.03s, and "the" — which matches almost every turn — 17.0s before and 15.5s after, because that query's cost is the sort rather than the match. A UNION has to materialise both hit sets where the single-index form could stream one. Recording it rather than discovering it later. Co-Authored-By: Claude Opus 5 --- internal/search/search.go | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/internal/search/search.go b/internal/search/search.go index 9079f80..25b8d34 100644 --- a/internal/search/search.go +++ b/internal/search/search.go @@ -147,6 +147,15 @@ func (s *Searcher) buildQuery(q *Query, limit int) (string, []interface{}) { // make the planner scan all 965,000 turns. UNION also collapses a turn // whose content and several of whose payloads all matched down to the one // row the caller asked for. + // + // It is not free. Measured on a copy of the author's 965,061-turn archive, + // against the same binary minus this change: "git commit" 0.56s -> 1.67s, + // "deploy" 0.38s -> 0.87s, a query matching nothing unchanged at 0.03s. A + // UNION has to materialise both hit sets where the single-index form could + // stream one. "the", which matches almost every turn, was 17.0s before and + // 15.5s after — that query's cost is the sort, not the match. Searching + // two indexes for roughly twice the time of searching one is the trade; + // the absolute numbers stay inside a second for a query anyone would type. prefix := "" matchArg := 0 From da43bb009b6d73521f8f6d4d8e0e59787e570377 Mon Sep 17 00:00:00 2001 From: Dylan Richard Date: Fri, 2 Oct 2026 20:53:35 -0500 Subject: [PATCH 7/8] fix: three defects the fresh-eyes pass found MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The no-text query branch wrapped an ORDER BY ... LIMIT query in a subquery with no outer ORDER BY, so a filter-only search (tool:Bash after:thisweek) relied on SQLite preserving a subquery's order, which it does not promise. The ordering is what keeps which rows fall inside LIMIT stable between runs over the same data. The two match-attribution columns are now selected inline as literals instead, so the query keeps its original shape, and a test pins the ordering. jeff's toolResultData declared a Success field nothing reads; these adapters declare only the fields they use. input_json does not always parse as JSON, despite the name inherited from agentsview. codex's custom_tool_call records a plain-text body — that is how apply_patch sends a diff — so the model field and the migration column now say to check json_valid before json_extract. Co-Authored-By: Claude Opus 5 --- .../db/migrations/009_add_tool_payloads.sql | 5 ++++ internal/search/search.go | 18 ++++++++--- internal/search/toolpayloads_test.go | 30 +++++++++++++++++++ pkg/adapter/jeff/jeff.go | 1 - pkg/models/models.go | 7 +++++ 5 files changed, 56 insertions(+), 5 deletions(-) diff --git a/internal/db/migrations/009_add_tool_payloads.sql b/internal/db/migrations/009_add_tool_payloads.sql index 3b3c59c..e211fa7 100644 --- a/internal/db/migrations/009_add_tool_payloads.sql +++ b/internal/db/migrations/009_add_tool_payloads.sql @@ -17,6 +17,11 @@ ALTER TABLE tool_uses ADD COLUMN tool_use_id TEXT; -- The call's arguments, stored whole and never truncated. Measured across the -- author's 259,836 calls: 78.6 MB in total, p50 72 bytes, largest 99,792. +-- +-- Not guaranteed to parse as JSON, despite the name — which follows the column +-- agentsview established. It holds the arguments as the source recorded them, +-- and codex's custom_tool_call records a plain-text body (that is how +-- apply_patch sends a diff). Check json_valid before json_extract. ALTER TABLE tool_uses ADD COLUMN input_json TEXT; ALTER TABLE tool_uses ADD COLUMN input_length INTEGER; diff --git a/internal/search/search.go b/internal/search/search.go index 25b8d34..7004161 100644 --- a/internal/search/search.go +++ b/internal/search/search.go @@ -174,10 +174,22 @@ func (s *Searcher) buildQuery(q *Query, limit int) (string, []interface{}) { argNum++ } + // A query with no text has no payload to attribute a match to, and takes + // the two trailing columns as literals so Search scans one row shape + // either way. Selected here rather than by wrapping the finished query in + // an outer SELECT: the wrap would put this query's ORDER BY inside a + // subquery, and SQLite does not promise that order survives into the + // enclosing SELECT. The ordering is load-bearing — see the comment on it + // below. + matchedToolCols := "" + if q.Text == "" { + matchedToolCols = ", NULL, NULL" + } + // Base query with joins baseQuery := ` SELECT DISTINCT t.id, t.session_id, t.type, t.timestamp, t.ordinal, t.content, - p.path as project_path, s.model, s.source, s.parent_session_id + p.path as project_path, s.model, s.source, s.parent_session_id` + matchedToolCols + ` FROM turns t JOIN sessions s ON t.session_id = s.id JOIN projects p ON s.project_id = p.id` @@ -267,9 +279,7 @@ func (s *Searcher) buildQuery(q *Query, limit int) (string, []interface{}) { baseQuery += fmt.Sprintf(" LIMIT %d", limit) if q.Text == "" { - // No text, so no payload to attribute a match to. The two trailing - // columns are still selected so Search scans one row shape. - return "SELECT *, NULL, NULL FROM (" + baseQuery + ")", args + return baseQuery, args } // The page is computed first and the payload lookup applied to it, not the diff --git a/internal/search/toolpayloads_test.go b/internal/search/toolpayloads_test.go index 5f6f4e9..d6bc65b 100644 --- a/internal/search/toolpayloads_test.go +++ b/internal/search/toolpayloads_test.go @@ -239,3 +239,33 @@ func TestSearch_ProjectFilterStillComposes(t *testing.T) { t.Errorf("got %v, want none for a non-matching project", resultIDs(results)) } } + +// TestSearch_NonTextQueryKeepsItsOrdering guards the shape of the no-text +// branch. Adding the two match-attribution columns must not push this query's +// ORDER BY inside a subquery: SQLite does not promise a subquery's order +// survives into the enclosing SELECT, and the ordering is what keeps which +// rows fall inside LIMIT stable between runs over the same data. +func TestSearch_NonTextQueryKeepsItsOrdering(t *testing.T) { + searcher := New(setupPayloadSearchDB(t).DB) + + // tool: with no text is the branch that selects NULL for the match columns. + results, err := searcher.Search(Parse("tool:Bash"), 10) + if err != nil { + t.Fatalf("search: %v", err) + } + if len(results) < 2 { + t.Fatalf("got %d results, want at least 2 to have an order worth checking", len(results)) + } + for i := 1; i < len(results); i++ { + prev, cur := results[i-1].Turn.Timestamp, results[i].Turn.Timestamp + if cur.After(prev) { + t.Errorf("result %d (%s) is newer than result %d (%s); the query is meant to be timestamp DESC", + i, cur, i-1, prev) + } + } + for _, r := range results { + if r.MatchedToolName != "" { + t.Errorf("MatchedToolName = %q, want empty for a query with no text", r.MatchedToolName) + } + } +} diff --git a/pkg/adapter/jeff/jeff.go b/pkg/adapter/jeff/jeff.go index a2e7c43..ac0a501 100644 --- a/pkg/adapter/jeff/jeff.go +++ b/pkg/adapter/jeff/jeff.go @@ -74,7 +74,6 @@ type toolRequestData struct { type toolResultData struct { ToolID string `json:"tool_id"` OutputPreview string `json:"output_preview"` - Success bool `json:"success"` } // Discover scans the Jeff sessions directory for JSONL session files and returns diff --git a/pkg/models/models.go b/pkg/models/models.go index c6e1f41..7ca0bda 100644 --- a/pkg/models/models.go +++ b/pkg/models/models.go @@ -123,6 +123,13 @@ type ToolUse struct { // InputJSON is the call's arguments, stored whole. Inputs are never // omitted: 259,836 of them total 78.6 MB across the author's archive, p50 // 72 bytes, largest 99,792. + // + // Not guaranteed to parse as JSON, despite the name — which follows the + // column agentsview established. It holds the arguments exactly as the + // source recorded them, and one source does not record JSON: codex's + // custom_tool_call carries a plain-text body in `input`, which is how + // apply_patch sends a diff. Treat it as text; json_extract it only after + // checking json_valid. InputJSON string `json:"input_json,omitempty"` // InputLength is len(InputJSON). Stored rather than derived so a consumer From 38333fd38b94cac95cee7d6a1133875c9ecd557b Mon Sep 17 00:00:00 2001 From: Dylan Richard Date: Mon, 5 Oct 2026 14:20:18 -0500 Subject: [PATCH 8/8] fix: scope the result backfill to a session, and stop jeff guessing at an unmatched id MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two silent-mislabel defects, same category: no error, wrong data, and the wrong data lands in a full-text index where it becomes a search hit attributed to the wrong thing. ## Migration 009: the result join was keyed on the provider id alone migration_009_results grouped by tool_use_id, the blocks table likewise, and the final UPDATE matched on tool_use_id only. Two sessions carrying the same provider call id would have the GROUP BY collapse them and write one session's output onto the other's row — and into the other's FTS document. Not reachable on the archive as it stands: no id there appears in more than one session across all 965,061 turns, checked. But that archive holds no codex sessions at all, so codex's call_… ids are untested, and whether they are globally unique or numbered per conversation is the provider's choice rather than ccvault's — call_1-style numbering would collide immediately. idx_tool_uses_tool_use_id is deliberately non-unique, which remains the right call and also means nothing in the schema would catch it. session_id now travels through both temp tables, both GROUP BYs, the LEFT JOIN between them, and the UPDATE predicate. tool_uses already carries it, so the scope costs nothing, and a tool result is always recorded in the same transcript as its call, so it cannot lose a legitimate match. TestMigration009KeepsResultsWithinTheirSession seeds two sessions whose calls both carry "call_1". Before the fix it failed exactly as predicted: sess-beta's row held "alpha-only-output", and beta's own output matched nothing in the index. The live path never had this defect — ExtractToolUses only ever sees one transcript's turns, so it is session-scoped by construction. ## jeff: an unmatched tool_id fell back to order attachJeffResult took the oldest pending call whenever an id-bearing result matched nothing, which happens when the request preceded any assistant turn or the call was already answered. The result then landed on an unrelated call, storing the wrong output under the wrong tool name. Split into two rules that cannot fall through into each other: * a result carrying a tool_id matches on that id alone, and is dropped if nothing pending has it; * a result carrying no tool_id falls back to order — which the data forces, tool_id being empty on 633 of 742 real requests — but only among calls jeff also left unidentified, since an identified call's own result is still coming and taking its slot mislabels both. The second test showed the old code swapping two outputs outright: search_drive got the calendar's output and calendar got search_drive's. ## Also The migration's note on the 1,302 rows left without an id claimed a precise duplicate distribution. Three counts of it disagree — two query shapes of mine and an independent one that found a longer tail — turning on whether turns with no parseable tool_use block are folded in. The directly observed figure stays; the distribution is left to the data-hygiene issue the duplicates need, since the migration behaves the same either way. Co-Authored-By: Claude Opus 5 --- .../db/migrations/009_add_tool_payloads.sql | 61 +++++-- internal/db/toolpayloads_test.go | 159 ++++++++++++++++++ pkg/adapter/jeff/jeff.go | 40 +++-- pkg/adapter/jeff/toolpayloads_test.go | 85 ++++++++++ 4 files changed, 314 insertions(+), 31 deletions(-) diff --git a/internal/db/migrations/009_add_tool_payloads.sql b/internal/db/migrations/009_add_tool_payloads.sql index e211fa7..ced6f38 100644 --- a/internal/db/migrations/009_add_tool_payloads.sql +++ b/internal/db/migrations/009_add_tool_payloads.sql @@ -106,14 +106,19 @@ WHERE t.type = 'assistant' -- guard fired on nothing, so no row was saved from a mislabel — but nothing -- else would have caught one. -- --- 1,302 rows come out of this with a NULL id, and all of them for the same --- reason: 1,288 turns in that archive carry duplicate tool_uses rows (1,281 --- turns hold two rows for one tool_use block, 7 hold four), left behind by the --- recovery import in #30. The position rule fills the first row of each turn --- and leaves the extras alone, which is the right outcome — giving two rows --- the same provider id would make the id useless as a join key. The --- duplicates are a data-hygiene problem of their own, not this migration's to --- fix. +-- 1,302 rows came out of this with a NULL id on the author's archive, measured +-- on the migrated copy. The cause is duplicate tool_uses rows left by the +-- recovery import in #30: turns carrying more tool_uses rows than their +-- message has tool_use blocks. The position rule fills the first row of each +-- turn and leaves the extras alone, which is the right outcome — giving two +-- rows the same provider id would make the id useless as a join key. +-- +-- How those duplicates are distributed is deliberately not stated here. Three +-- counts of it disagree (two query shapes of mine, and an independent one that +-- found a longer tail), and the difference turns on whether turns with no +-- parseable tool_use block are folded in. Pinning it down belongs to the +-- data-hygiene issue the duplicates need, not to this migration, which behaves +-- the same either way. UPDATE tool_uses SET tool_use_id = src.tool_use_id, input_json = src.input_json, @@ -133,17 +138,35 @@ FROM ( WHERE tool_uses.rowid = src.rid AND tool_uses.tool_use_id IS NULL; --- Every tool_result block, keyed by the call it answers. +-- Every tool_result block, keyed by the session it happened in and the call it +-- answers. +-- +-- The session is half the key, not decoration. A provider id is only ever +-- promised to be unique within the conversation that issued it: claude-code's +-- toolu_… are random enough that no id in the author's archive appears in two +-- sessions, but the archive holds no codex sessions at all, and whether +-- codex's call_… are globally unique or numbered per conversation is the +-- provider's choice. Keyed on the id alone, two sessions sharing one would +-- have the GROUP BY collapse them and write one session's output onto the +-- other's row — silently, and into the wrong FTS document, so it would come +-- back as a search hit attributed to the wrong conversation. +-- +-- tool_uses already carries session_id, so scoping the join costs nothing. +-- idx_tool_uses_tool_use_id is deliberately non-unique (see above), which is +-- the right call and also means nothing in the schema would catch this. +-- +-- A tool result is always recorded in the same transcript as its call, so +-- scoping to the session cannot lose a legitimate match. -- -- Not filtered to user turns. That is where Claude Code puts them, and keying -- on the block's own type costs nothing and does not depend on it staying --- true. GROUP BY keeps one row per id: 259,812 results across the archive all --- name distinct calls, and a transcript that answered one call twice should not --- make the join multiply rows. +-- true. GROUP BY keeps one row per call: a transcript that answered the same +-- call twice should not make the join multiply rows. DROP TABLE IF EXISTS temp.migration_009_results; CREATE TEMP TABLE migration_009_results AS -SELECT json_extract(je.value, '$.tool_use_id') AS tool_use_id, +SELECT t.session_id AS session_id, + json_extract(je.value, '$.tool_use_id') AS tool_use_id, json_type(je.value, '$.content') AS content_type, json_extract(je.value, '$.content') AS content, octet_length(json_extract(je.value, '$.content')) AS content_len @@ -154,7 +177,7 @@ FROM turns t, ELSE '[]' END) je WHERE json_extract(je.value, '$.type') = 'tool_result' AND json_extract(je.value, '$.tool_use_id') IS NOT NULL -GROUP BY tool_use_id; +GROUP BY t.session_id, tool_use_id; -- The structured half: whether the content holds an image block, and the text -- blocks concatenated. 130,516 of the archive's results use this shape and @@ -163,7 +186,8 @@ GROUP BY tool_use_id; DROP TABLE IF EXISTS temp.migration_009_result_blocks; CREATE TEMP TABLE migration_009_result_blocks AS -SELECT r.tool_use_id, +SELECT r.session_id, + r.tool_use_id, MAX(CASE WHEN json_extract(e.value, '$.type') = 'image' THEN 1 ELSE 0 END) AS has_image, group_concat(CASE WHEN json_extract(e.value, '$.type') = 'text' AND json_extract(e.value, '$.text') <> '' @@ -171,7 +195,7 @@ SELECT r.tool_use_id, char(10) ORDER BY e.key) AS text_content FROM migration_009_results r, json_each(CASE WHEN r.content_type = 'array' THEN r.content ELSE '[]' END) e -GROUP BY r.tool_use_id; +GROUP BY r.session_id, r.tool_use_id; -- Apply the storage policy. The order of the CASE arms is the order in -- pkg/toolpayload.decide, and the two have to stay in step: the same @@ -199,13 +223,14 @@ SET result_length = COALESCE(src.content_len, 0), WHEN src.content_type = 'array' THEN NULLIF(src.text_content, '') ELSE NULL END FROM ( - SELECT r.tool_use_id, r.content_type, r.content, r.content_len, + SELECT r.session_id, r.tool_use_id, r.content_type, r.content, r.content_len, COALESCE(b.has_image, 0) AS has_image, b.text_content FROM migration_009_results r - LEFT JOIN migration_009_result_blocks b USING (tool_use_id) + LEFT JOIN migration_009_result_blocks b USING (session_id, tool_use_id) ) AS src WHERE tool_uses.tool_use_id = src.tool_use_id + AND tool_uses.session_id = src.session_id AND tool_uses.result_length IS NULL; DROP TABLE IF EXISTS temp.migration_009_calls; diff --git a/internal/db/toolpayloads_test.go b/internal/db/toolpayloads_test.go index a463d05..fb45237 100644 --- a/internal/db/toolpayloads_test.go +++ b/internal/db/toolpayloads_test.go @@ -178,6 +178,165 @@ func seedPre009(t *testing.T, fixtures []payloadFixture) string { return dir } +// seedPre009TwoSessionsSharingAnID builds a pre-009 archive where two +// different sessions each made a call carrying the SAME provider id, each +// answered by its own result. +// +// Not reachable on the claude-code and nanoclaw data in the author's archive — +// checked, no id there appears in more than one session — but the archive +// holds no codex sessions at all, and whether codex's call_… ids are globally +// unique or merely unique within one conversation is the provider's choice, +// not ccvault's. idx_tool_uses_tool_use_id is deliberately non-unique, so +// nothing in the schema prevents this either. +// +// The failure it guards against is the worst shape available here: no error, +// one session's tool output written onto another session's row, and indexed +// into the wrong FTS document so it comes back as a search hit attributed to +// the wrong conversation. +func seedPre009TwoSessionsSharingAnID(t *testing.T) string { + t.Helper() + + dir := t.TempDir() + raw, err := sql.Open("sqlite", filepath.Join(dir, "ccvault.db")) + if err != nil { + t.Fatalf("open raw: %v", err) + } + + migrations, err := loadMigrations() + if err != nil { + t.Fatalf("load migrations: %v", err) + } + if _, err := raw.Exec(`CREATE TABLE IF NOT EXISTS schema_version ( + version INTEGER NOT NULL, + applied_at TEXT NOT NULL DEFAULT (datetime('now')))`); err != nil { + t.Fatalf("schema_version: %v", err) + } + for _, m := range migrations { + if m.version >= 9 { + continue + } + if err := applyMigration(raw, m); err != nil { + t.Fatalf("apply %03d: %v", m.version, err) + } + } + if columnExists(t, raw, "tool_uses", "result_content") { + t.Fatal("tool_uses.result_content exists below migration 009, so this test proves nothing") + } + + ts := time.Date(2026, 10, 1, 10, 0, 0, 0, time.UTC) + + // The shared id. Sequential, because that is the shape a provider using + // per-conversation numbering would produce. + const sharedID = "call_1" + + for _, s := range []struct{ session, output string }{ + {"sess-alpha", "alpha-only-output"}, + {"sess-beta", "beta-only-output"}, + } { + if _, err := raw.Exec(`INSERT INTO sessions (id, started_at, source_file, source, model, git_branch) + VALUES (?, ?, ?, 'codex', '', '')`, s.session, ts, "/fake/"+s.session+".jsonl"); err != nil { + t.Fatalf("seed session %s: %v", s.session, err) + } + + callTurn := s.session + "-call" + resultTurn := s.session + "-result" + + assistantRaw := `{"uuid":"` + callTurn + `","sessionId":"` + s.session + `","type":"assistant",` + + `"timestamp":"2026-10-01T10:00:00.000Z","message":{"role":"assistant","content":[` + + `{"type":"tool_use","id":"` + sharedID + `","name":"exec_command","input":{"cmd":"` + s.session + `"}}]}}` + userRaw := `{"uuid":"` + resultTurn + `","sessionId":"` + s.session + `","type":"user",` + + `"timestamp":"2026-10-01T10:00:01.000Z","message":{"role":"user","content":[` + + `{"type":"tool_result","tool_use_id":"` + sharedID + `","content":"` + s.output + `"}]}}` + + if _, err := raw.Exec( + `INSERT INTO turns (id, session_id, type, timestamp, content, raw_json, ordinal) + VALUES (?, ?, 'assistant', ?, '', ?, 0)`, + callTurn, s.session, ts, assistantRaw); err != nil { + t.Fatalf("seed call turn %s: %v", callTurn, err) + } + if _, err := raw.Exec( + `INSERT INTO turns (id, session_id, type, timestamp, content, raw_json, ordinal) + VALUES (?, ?, 'user', ?, '', ?, 1)`, + resultTurn, s.session, ts.Add(time.Second), userRaw); err != nil { + t.Fatalf("seed result turn %s: %v", resultTurn, err) + } + if _, err := raw.Exec( + `INSERT INTO tool_uses (turn_id, session_id, tool_name, timestamp) + VALUES (?, ?, 'exec_command', ?)`, + callTurn, s.session, ts); err != nil { + t.Fatalf("seed tool use for %s: %v", s.session, err) + } + } + + if err := raw.Close(); err != nil { + t.Fatalf("close raw: %v", err) + } + return dir +} + +// TestMigration009KeepsResultsWithinTheirSession is the cross-session +// contamination guard. Both sessions' calls carry the id "call_1"; each must +// come out holding its own session's output and nothing of the other's. +func TestMigration009KeepsResultsWithinTheirSession(t *testing.T) { + dir := seedPre009TwoSessionsSharingAnID(t) + + database, err := Open(dir) + if err != nil { + t.Fatalf("Open: %v", err) + } + defer func() { _ = database.Close() }() + + want := map[string]string{ + "sess-alpha": "alpha-only-output", + "sess-beta": "beta-only-output", + } + + rows, err := database.Query( + `SELECT session_id, COALESCE(result_content, '') FROM tool_uses ORDER BY session_id`) + if err != nil { + t.Fatalf("query: %v", err) + } + defer func() { _ = rows.Close() }() + + got := make(map[string]string) + for rows.Next() { + var session, result string + if err := rows.Scan(&session, &result); err != nil { + t.Fatalf("scan: %v", err) + } + got[session] = result + } + if err := rows.Err(); err != nil { + t.Fatalf("rows: %v", err) + } + + if len(got) != 2 { + t.Fatalf("got %d tool_uses rows, want 2: %v", len(got), got) + } + for session, wantResult := range want { + if got[session] != wantResult { + t.Errorf("session %s has result_content %q, want %q — a result crossed sessions", + session, got[session], wantResult) + } + } + + // And the wrong text must not be reachable through the wrong session's + // FTS document either, which is where a crossed result does real damage. + for session, wantResult := range want { + var n int + if err := database.QueryRow(` + SELECT COUNT(*) FROM tool_uses_fts + JOIN tool_uses tu ON tu.id = tool_uses_fts.rowid + WHERE tool_uses_fts MATCH ? AND tu.session_id = ?`, + `"`+wantResult+`"`, session).Scan(&n); err != nil { + t.Fatalf("fts probe: %v", err) + } + if n != 1 { + t.Errorf("%q matched %d rows in session %s, want 1", wantResult, n, session) + } + } +} + func tableExists(t *testing.T, raw *sql.DB, name string) bool { t.Helper() var count int diff --git a/pkg/adapter/jeff/jeff.go b/pkg/adapter/jeff/jeff.go index ac0a501..5b67cb6 100644 --- a/pkg/adapter/jeff/jeff.go +++ b/pkg/adapter/jeff/jeff.go @@ -137,29 +137,43 @@ type jeffCallSite struct { } // attachJeffResult links a tool_result to the call it answers and removes that -// call from the pending list. +// call from the pending list. A result it cannot place is dropped. // -// Prefers an exact tool_id match, which is right when jeff recorded one. Falls -// back to the oldest unanswered call, which is what the data forces: tool_id -// is the empty string on 85% of real tool_requests, so matching on it would -// make every one of those the same call. Jeff's own files pair requests and -// results one for one — 742 of each across the author's sessions — so order is -// a sound fallback rather than a guess. +// Two rules, picked by whether jeff identified the result, and deliberately +// not allowed to fall through into each other: +// +// - A result carrying a tool_id is matched on that id alone. If no pending +// call has it, the result is dropped. Falling back to order here would +// attach one tool's output to a different tool's row — which happens +// whenever the request preceded any assistant turn, so no row exists for +// it, or the call was already answered — and the damage is silent: the +// wrong text is stored and then made searchable under the wrong tool name. +// +// - A result carrying no tool_id falls back to order, which is what the data +// forces: tool_id is the empty string on 633 of 742 real tool_requests, so +// matching on it would make every one of those the same call. Jeff's files +// pair requests and results one for one, so the oldest unanswered call is +// sound rather than a guess — but only among the calls jeff also left +// unidentified. An identified call's own result is still coming, and +// letting an id-less result take its slot mislabels both of them. func attachJeffResult(turns []adapter.ParsedTurn, pending *[]jeffCallSite, res toolResultData) { idx := -1 - if res.ToolID != "" { - for i, site := range *pending { + for i, site := range *pending { + if res.ToolID != "" { if site.toolID == res.ToolID { idx = i break } + continue + } + if site.toolID == "" { + idx = i + break } } if idx < 0 { - if len(*pending) == 0 { - return - } - idx = 0 + // Nothing this result can be placed against. + return } site := (*pending)[idx] diff --git a/pkg/adapter/jeff/toolpayloads_test.go b/pkg/adapter/jeff/toolpayloads_test.go index 0e05fdc..787fb01 100644 --- a/pkg/adapter/jeff/toolpayloads_test.go +++ b/pkg/adapter/jeff/toolpayloads_test.go @@ -137,6 +137,91 @@ func TestParse_EmptyToolIDLinksByOrder(t *testing.T) { } } +// TestParse_UnmatchedIDBearingResultIsDropped covers a tool_result that names +// a tool_id no pending call has. It happens when the request preceded any +// assistant turn (so no row was created for it) or when the call was already +// answered. +// +// Guessing the oldest pending call there attaches one tool's output to a +// different tool's row, which stores the wrong text and then makes it +// searchable under the wrong tool name. An id that does not match is a result +// ccvault cannot place, and dropping it is the only honest option. +func TestParse_UnmatchedIDBearingResultIsDropped(t *testing.T) { + lines := append(jeffBaseLines(), + map[string]any{ + "timestamp": "2026-02-24T19:56:16.000000Z", "entry_type": "tool_request", + "conversation_id": convID, + "data": map[string]any{ + "tool_name": "email", "params": map[string]any{"operation": "list"}, "tool_id": "tool-001", + }, + }, + map[string]any{ + "timestamp": "2026-02-24T19:56:18.000000Z", "entry_type": "tool_result", + "conversation_id": convID, + "data": map[string]any{ + "success": true, "output_preview": "belongs to some other call", "tool_id": "tool-999", + }, + }, + ) + + tus := allToolUses(parseLines(t, lines)) + if len(tus) != 1 { + t.Fatalf("got %d tool uses, want 1: %+v", len(tus), tus) + } + if tus[0].HasResult { + t.Errorf("the email call was given a result it does not own: %q", tus[0].ResultContent) + } + if tus[0].ResultContent != "" { + t.Errorf("ResultContent = %q, want empty", tus[0].ResultContent) + } +} + +// TestParse_IDLessResultDoesNotConsumeAnIDBearingCall covers the other side of +// the same mistake. A result with no id has to fall back to order, but it must +// not swallow a call that jeff did identify — that call's own result is still +// coming, and taking its slot mislabels both. +func TestParse_IDLessResultDoesNotConsumeAnIDBearingCall(t *testing.T) { + lines := append(jeffBaseLines(), + // Identified call, waiting for its own result. + map[string]any{ + "timestamp": "2026-02-24T19:56:16.000000Z", "entry_type": "tool_request", + "conversation_id": convID, + "data": map[string]any{ + "tool_name": "search_drive", "params": map[string]any{"query": "Q4"}, "tool_id": "tool-001", + }, + }, + // Unidentified call, which is what the id-less result below belongs to. + map[string]any{ + "timestamp": "2026-02-24T19:56:17.000000Z", "entry_type": "tool_request", + "conversation_id": convID, + "data": map[string]any{ + "tool_name": "calendar", "params": map[string]any{"operation": "list"}, "tool_id": "", + }, + }, + map[string]any{ + "timestamp": "2026-02-24T19:56:18.000000Z", "entry_type": "tool_result", + "conversation_id": convID, + "data": map[string]any{"success": true, "output_preview": "No events today", "tool_id": ""}, + }, + map[string]any{ + "timestamp": "2026-02-24T19:56:19.000000Z", "entry_type": "tool_result", + "conversation_id": convID, + "data": map[string]any{"success": true, "output_preview": "Found 3 results", "tool_id": "tool-001"}, + }, + ) + + tus := allToolUses(parseLines(t, lines)) + if len(tus) != 2 { + t.Fatalf("got %d tool uses, want 2: %+v", len(tus), tus) + } + if tus[0].ToolName != "search_drive" || tus[0].ResultContent != "Found 3 results" { + t.Errorf("identified call got %+v, want search_drive / %q", tus[0], "Found 3 results") + } + if tus[1].ToolName != "calendar" || tus[1].ResultContent != "No events today" { + t.Errorf("unidentified call got %+v, want calendar / %q", tus[1], "No events today") + } +} + // TestParse_UnansweredToolRequestHasNoResult keeps the two absences apart in // jeff too. func TestParse_UnansweredToolRequestHasNoResult(t *testing.T) {