From 3aa1e97dc38890bb21058e797ef67fb58979f8e5 Mon Sep 17 00:00:00 2001 From: kite Date: Thu, 10 Sep 2026 16:17:38 +0800 Subject: [PATCH] perf(grouping): return file indices instead of paths The grouping LLM call asked the model to echo full file paths back in its JSON response, making the output size proportional to the sum of all path lengths. On large change sets this overflowed the completion token limit; the truncated JSON then failed to parse and the whole change set degraded to per-file dispatch, multiplying downstream review calls. Switch the grouping contract to integer indices: - buildFileList prefixes each file with a zero-based index, e.g. "[0] MODIFIED path (+12/-3)". - groupingResponse.Files is now []int; the model returns those indices. - parseGroupingResponse maps indices back to diffs by position, skipping out-of-range indices (the index equivalent of the previous unknown-path skip) and duplicates. A parse failure still returns an error and the caller falls back to per-file dispatch, exactly as before. - Prompts updated to ask for integer indices. The response is now an order of magnitude smaller, so truncation on large change sets becomes rare instead of common. Because the grouping response is now indices, the session viewer resolves them back to paths for display: - buildGroupingIndex scans the request's numbered file list (user message only) to build an index->path map. - parseGroupingGroups unmarshals the response into indices; a legacy path-string response reports not-ok so the viewer keeps showing the raw text (already readable for those older sessions). - groupingView maps indices to paths and falls back to the raw response when nothing resolves (format drift), avoiding a wall of "#idx". - The grouping card renders label + resolved paths with a collapsible raw response for audit. No on-disk format changes; existing sessions render retroactively. --- internal/agent/grouping.go | 51 ++++--- internal/agent/grouping_test.go | 48 +++--- internal/agent/manifest_integration_test.go | 2 +- .../template/prompts/grouping_task_system.md | 5 +- .../template/prompts/grouping_task_user.md | 4 +- internal/viewer/handler_test.go | 53 +++++++ internal/viewer/server.go | 2 + internal/viewer/static/style.css | 32 ++++ internal/viewer/store.go | 139 ++++++++++++++++++ internal/viewer/store_test.go | 137 +++++++++++++++++ internal/viewer/templates/session.html | 43 +++++- 11 files changed, 470 insertions(+), 46 deletions(-) diff --git a/internal/agent/grouping.go b/internal/agent/grouping.go index 1639ede25..336202eb5 100644 --- a/internal/agent/grouping.go +++ b/internal/agent/grouping.go @@ -38,9 +38,14 @@ type FileGroupInfo struct { Files []string `json:"files"` } +// groupingResponse is one group as returned by the LLM. Files holds the integer +// indices printed beside each file in buildFileList, not paths: an index costs a +// few output tokens where a path costs its full length, so the response stays +// well inside the completion limit even for large change sets and no longer +// truncates into a whole-set per-file fallback. type groupingResponse struct { - Label string `json:"label"` - Files []string `json:"files"` + Label string `json:"label"` + Files []int `json:"files"` } // groupDiffsResult holds the grouping output and any LLM usage to record. @@ -241,11 +246,16 @@ func callGroupingLLM(ctx context.Context, diffs []model.Diff, client llm.LLMClie return groups, usage, err } +// buildFileList renders the change set for the grouping prompt, one file per +// line, each prefixed with its zero-based index. The index is what the model +// groups by (see groupingResponse): it maps back to diffs[i] in +// parseGroupingResponse, so the two must agree on ordering — both walk diffs in +// slice order. formatDiffEntry is left untouched because it is shared with the +// other-changed-files block, which has no index to show. func buildFileList(diffs []model.Diff) string { var sb strings.Builder - for _, d := range diffs { - sb.WriteString(formatDiffEntry(d)) - sb.WriteString("\n") + for i, d := range diffs { + fmt.Fprintf(&sb, "[%d] %s\n", i, formatDiffEntry(d)) } return sb.String() } @@ -266,31 +276,30 @@ func parseGroupingResponse(content string, diffs []model.Diff) ([]FileGroup, err var resp []groupingResponse if err := json.Unmarshal([]byte(content), &resp); err != nil { + // A parse failure (including a response truncated by the completion limit) + // returns an error; the caller falls back to per-file dispatch. Indices + // keep this output an order of magnitude smaller than paths did, so a + // truncation that reaches this point is far rarer than before. return nil, fmt.Errorf("parse grouping JSON: %w", err) } - diffByPath := make(map[string]model.Diff, len(diffs)) - for _, d := range diffs { - diffByPath[d.NewPath] = d - } - - seen := make(map[string]bool, len(diffs)) + seen := make([]bool, len(diffs)) var groups []FileGroup for _, g := range resp { var gDiffs []model.Diff - for _, f := range g.Files { - if seen[f] { - // Skip duplicate — file already assigned to an earlier group + for _, idx := range g.Files { + if idx < 0 || idx >= len(diffs) { + // Skip an index that names no file — the index equivalent of the + // unknown-path case the path-based version skipped. continue } - d, ok := diffByPath[f] - if !ok { - // Skip unknown file path + if seen[idx] { + // Skip duplicate — file already assigned to an earlier group. continue } - seen[f] = true - gDiffs = append(gDiffs, d) + seen[idx] = true + gDiffs = append(gDiffs, diffs[idx]) } if len(gDiffs) > 0 { groups = append(groups, FileGroup{Label: g.Label, Diffs: gDiffs}) @@ -298,8 +307,8 @@ func parseGroupingResponse(content string, diffs []model.Diff) ([]FileGroup, err } // Files not covered by any group get their own single-file group - for _, d := range diffs { - if !seen[d.NewPath] { + for i, d := range diffs { + if !seen[i] { groups = append(groups, FileGroup{Label: d.NewPath, Diffs: []model.Diff{d}}) } } diff --git a/internal/agent/grouping_test.go b/internal/agent/grouping_test.go index 6db32926f..7ff26c5e1 100644 --- a/internal/agent/grouping_test.go +++ b/internal/agent/grouping_test.go @@ -64,8 +64,8 @@ func TestParseGroupingResponse_Valid(t *testing.T) { {NewPath: "docs/README.md"}, } content := `[ - {"label": "auth handler", "files": ["internal/auth/handler.go", "internal/auth/handler_test.go"]}, - {"label": "docs", "files": ["docs/README.md"]} + {"label": "auth handler", "files": [0, 1]}, + {"label": "docs", "files": [2]} ]` groups, err := parseGroupingResponse(content, diffs) if err != nil { @@ -87,7 +87,7 @@ func TestParseGroupingResponse_MarkdownFenced(t *testing.T) { {NewPath: "a.go"}, {NewPath: "b.go"}, } - content := "```json\n" + `[{"label":"all","files":["a.go","b.go"]}]` + "\n```" + content := "```json\n" + `[{"label":"all","files":[0,1]}]` + "\n```" groups, err := parseGroupingResponse(content, diffs) if err != nil { t.Fatal(err) @@ -101,7 +101,7 @@ func TestParseGroupingResponse_DuplicateFile(t *testing.T) { diffs := []model.Diff{ {NewPath: "a.go"}, } - content := `[{"label":"g1","files":["a.go"]},{"label":"g2","files":["a.go"]}]` + content := `[{"label":"g1","files":[0]},{"label":"g2","files":[0]}]` groups, err := parseGroupingResponse(content, diffs) if err != nil { t.Fatal(err) @@ -120,7 +120,7 @@ func TestParseGroupingResponse_MissingFile(t *testing.T) { {NewPath: "a.go"}, {NewPath: "b.go"}, } - content := `[{"label":"g1","files":["a.go"]}]` + content := `[{"label":"g1","files":[0]}]` groups, err := parseGroupingResponse(content, diffs) if err != nil { t.Fatal(err) @@ -138,12 +138,12 @@ func TestParseGroupingResponse_UnknownFile(t *testing.T) { diffs := []model.Diff{ {NewPath: "a.go"}, } - content := `[{"label":"g1","files":["a.go","unknown.go"]}]` + content := `[{"label":"g1","files":[0,99]}]` groups, err := parseGroupingResponse(content, diffs) if err != nil { t.Fatal(err) } - // unknown.go is skipped; a.go still forms the group + // index 99 is out of range and skipped; a.go still forms the group if len(groups) != 1 { t.Fatalf("got %d groups, want 1", len(groups)) } @@ -160,6 +160,18 @@ func TestParseGroupingResponse_InvalidJSON(t *testing.T) { } } +func TestParseGroupingResponse_Truncated(t *testing.T) { + // A response cut off by the completion limit is no longer partially salvaged: + // json.Unmarshal fails, parseGroupingResponse returns an error, and the caller + // falls back to per-file dispatch — the same behavior the path-based version + // had. Indices make this case far rarer (smaller output), but not special. + diffs := []model.Diff{{NewPath: "a.go"}, {NewPath: "b.go"}} + content := `[{"label":"g1","files":[0,1]},{"label":"g2","fil` + if _, err := parseGroupingResponse(content, diffs); err == nil { + t.Fatal("expected an error for a truncated response") + } +} + func TestEnforceGroupTokenBudget_NoSplit(t *testing.T) { groups := []FileGroup{ {Label: "small", Diffs: []model.Diff{{NewPath: "a.go", Diff: "short"}}}, @@ -259,7 +271,7 @@ func TestGroupDiffs_LLMError_Fallback(t *testing.T) { func TestGroupDiffs_LLMSuccess(t *testing.T) { diffs := []model.Diff{{NewPath: "a.go"}, {NewPath: "b.go"}, {NewPath: "c.go"}} client := &fakeGroupingClient{ - response: `[{"label":"ab","files":["a.go","b.go"]},{"label":"c","files":["c.go"]}]`, + response: `[{"label":"ab","files":[0,1]},{"label":"c","files":[2]}]`, } tpl := template.Template{ GroupingTask: &template.LlmConversation{ @@ -403,7 +415,7 @@ func TestGroupDiffs_AtFileThresholdCallsLLM(t *testing.T) { {NewPath: "d.go", Insertions: 1}, } client := &fakeGroupingClient{ - response: `[{"label":"ab","files":["a.go","b.go"]},{"label":"cd","files":["c.go","d.go"]}]`, + response: `[{"label":"ab","files":[0,1]},{"label":"cd","files":[2,3]}]`, } result := groupDiffs(context.Background(), diffs, client, "fake", groupingSkipTemplate(4, 200), 0, nil) if !client.called { @@ -523,7 +535,7 @@ func TestCallGroupingLLM_UsesTemplateMaxTokens(t *testing.T) { Messages: []template.ChatMessage{{Role: "user", Content: "{{file_list}}"}}, } - client := &fakeGroupingClient{response: `[{"label":"a","files":["a.go"]}]`} + client := &fakeGroupingClient{response: `[{"label":"a","files":[0]}]`} if _, _, err := callGroupingLLM(context.Background(), diffs, client, "fake", task, 32000, nil); err != nil { t.Fatalf("callGroupingLLM: %v", err) } @@ -531,7 +543,7 @@ func TestCallGroupingLLM_UsesTemplateMaxTokens(t *testing.T) { t.Errorf("MaxTokens = %d, want 32000 (the template's own limit)", client.gotReq.MaxTokens) } - client = &fakeGroupingClient{response: `[{"label":"a","files":["a.go"]}]`} + client = &fakeGroupingClient{response: `[{"label":"a","files":[0]}]`} if _, _, err := callGroupingLLM(context.Background(), diffs, client, "fake", task, 0, nil); err != nil { t.Fatalf("callGroupingLLM: %v", err) } @@ -548,12 +560,14 @@ func TestBuildFileMetadataTable(t *testing.T) { {NewPath: "d.go", OldPath: "d.go", Insertions: 3, Deletions: 4}, } // The grouping file list shares formatDiffEntry with the other-changed-files - // block, so both prompts enumerate files the same way. Pin the exact shape, - // including the per-entry trailing newline the grouping template relies on. - want := "ADDED a.go (+10/-0)\n" + - "DELETED b.go (+0/-5)\n" + - "RENAMED c.go (+2/-1)\n" + - "MODIFIED d.go (+3/-4)\n" + // block, so both prompts enumerate files the same way. buildFileList adds a + // zero-based index prefix on top, which the model groups by. Pin the exact + // shape, including the per-entry trailing newline the grouping template + // relies on. + want := "[0] ADDED a.go (+10/-0)\n" + + "[1] DELETED b.go (+0/-5)\n" + + "[2] RENAMED c.go (+2/-1)\n" + + "[3] MODIFIED d.go (+3/-4)\n" if got := buildFileList(diffs); got != want { t.Errorf("got %q, want %q", got, want) } diff --git a/internal/agent/manifest_integration_test.go b/internal/agent/manifest_integration_test.go index 8c26225a8..7379d8972 100644 --- a/internal/agent/manifest_integration_test.go +++ b/internal/agent/manifest_integration_test.go @@ -456,7 +456,7 @@ type groupedBudgetPartialClient struct { func (c *groupedBudgetPartialClient) CompletionsWithCtx(_ context.Context, req llm.ChatRequest) (*llm.ChatResponse, error) { if len(req.Tools) == 0 { - content := `[{"label":"pair","files":["a.go","b.go"]}]` + content := `[{"label":"pair","files":[0,1]}]` return &llm.ChatResponse{ Choices: []llm.Choice{{Message: llm.ResponseMessage{Content: &content}}}, Model: "fake", diff --git a/internal/config/template/prompts/grouping_task_system.md b/internal/config/template/prompts/grouping_task_system.md index ce9eddfd6..57d36e251 100644 --- a/internal/config/template/prompts/grouping_task_system.md +++ b/internal/config/template/prompts/grouping_task_system.md @@ -6,8 +6,11 @@ Files in the same group typically: - Are i18n/config variants of the same resource (e.g. message_en.properties and message_zh.properties) - Share the same directory and work together on a single concern +Each file in the list is prefixed with a zero-based index in brackets, e.g. `[0] MODIFIED path/to/file (+12/-3)`. Refer to files by that integer index, never by path. + Rules: -- Every file must appear in exactly one group. +- Every file index must appear in exactly one group. - A group may contain 1 file if it is unrelated to others. - Maximum 10 files per group. +- The "files" field of each group is an array of the integer indices shown in brackets. - Output ONLY a JSON array, no other text. \ No newline at end of file diff --git a/internal/config/template/prompts/grouping_task_user.md b/internal/config/template/prompts/grouping_task_user.md index ff257aca9..ec8d174b6 100644 --- a/internal/config/template/prompts/grouping_task_user.md +++ b/internal/config/template/prompts/grouping_task_user.md @@ -2,5 +2,5 @@ Group the following changed files: {{file_list}} -Respond with a JSON array: -[{"label": "short theme description", "files": ["path1", "path2"]}] \ No newline at end of file +Respond with a JSON array, where "files" holds the integer indices shown in brackets beside each file: +[{"label": "short theme description", "files": [0, 1]}] \ No newline at end of file diff --git a/internal/viewer/handler_test.go b/internal/viewer/handler_test.go index 82373dc20..419587653 100644 --- a/internal/viewer/handler_test.go +++ b/internal/viewer/handler_test.go @@ -4,6 +4,7 @@ package viewer import ( + "encoding/json" "net/http" "net/http/httptest" "os" @@ -179,6 +180,58 @@ func TestHandleSession_Success(t *testing.T) { } } +func TestHandleSession_GroupingRendersPaths(t *testing.T) { + root := t.TempDir() + repoDir := filepath.Join(root, "repo") + if err := os.MkdirAll(repoDir, 0755); err != nil { + t.Fatal(err) + } + + // json.Marshal each record so the embedded newlines in the file list and the + // quotes in the response JSON are escaped correctly, instead of hand-writing + // the escapes into a raw JSONL literal. + mustJSON := func(v any) string { + b, err := json.Marshal(v) + if err != nil { + t.Fatal(err) + } + return string(b) + } + fileList := "[0] MODIFIED internal/agent/grouping.go (+66/-24)\n" + + "[1] ADDED internal/viewer/store.go (+120/-0)\n" + writeJSONL(t, filepath.Join(repoDir, "grp.jsonl"), + `{"type":"session_start","timestamp":"2025-06-01T10:00:00Z","cwd":"/my/proj","model":"claude"}`, + mustJSON(map[string]any{ + "type": "llm_request", "filePath": "__grouping__", "taskType": "grouping_task", "request_no": 1, + "messages": []any{map[string]any{"role": "user", "content": fileList}}, + }), + mustJSON(map[string]any{ + "type": "llm_response", "filePath": "__grouping__", "taskType": "grouping_task", + "content": `[{"label":"grouping index switch","files":[0,1]}]`, + }), + `{"type":"session_end","duration_seconds":30}`) + + req := httptest.NewRequest("GET", "/r/repo/grp", nil) + rr := httptest.NewRecorder() + handleSession(rr, req, root, "repo", "grp") + + if rr.Code != http.StatusOK { + t.Fatalf("status = %d, want 200", rr.Code) + } + body := rr.Body.String() + // The resolved paths and label must appear in the grouping-view markup. + for _, want := range []string{"grouping-view", "internal/agent/grouping.go", "internal/viewer/store.go", "grouping index switch"} { + if !strings.Contains(body, want) { + t.Errorf("rendered session missing %q", want) + } + } + // The raw index JSON is still available (collapsed) for audit, but the + // primary view is the path list, not a bare "files":[0,1]. + if !strings.Contains(body, "Raw LLM response") { + t.Error("raw LLM response fallback should still be present for audit") + } +} + func TestHandleSession_NotFound(t *testing.T) { root := t.TempDir() repoDir := filepath.Join(root, "repo") diff --git a/internal/viewer/server.go b/internal/viewer/server.go index eb66cfb5a..a3e842621 100644 --- a/internal/viewer/server.go +++ b/internal/viewer/server.go @@ -322,6 +322,8 @@ func parseTemplate(name string) (*template.Template, error) { return fp } }, + "isGrouping": func(tt TaskType) bool { return tt == GroupingTask }, + "groupingView": groupingView, "orderedTasks": func(tasks map[TaskType][]*TaskCard) []struct { Type TaskType Cards []*TaskCard diff --git a/internal/viewer/static/style.css b/internal/viewer/static/style.css index 669b9b259..f35db4bad 100644 --- a/internal/viewer/static/style.css +++ b/internal/viewer/static/style.css @@ -680,6 +680,38 @@ h3 { word-break: break-word; } +/* Grouping view: model-proposed file groups, indices resolved to paths */ +.grouping-view { + padding: 0.75rem 1.25rem; + background: var(--response-bg); + display: flex; + flex-direction: column; + gap: 0.75rem; + max-height: 450px; + overflow-y: auto; +} +.grouping-group { + border-left: 3px solid var(--task-grouping); + padding-left: 0.75rem; +} +.grouping-label { + display: block; + font-size: 0.85rem; + font-weight: 600; + color: var(--text-strong); + margin-bottom: 0.25rem; +} +.grouping-files { + margin: 0; + padding-left: 1.25rem; + font-size: 0.82rem; + line-height: 1.6; + color: var(--text-strong); + font-family: var(--mono); + word-break: break-word; +} +.grouping-unresolved { color: var(--text-faint); } + /* Scrollbar styling */ .response-text::-webkit-scrollbar { width: 6px; } .response-text::-webkit-scrollbar-track { background: transparent; } diff --git a/internal/viewer/store.go b/internal/viewer/store.go index 61ab59cbd..67282185f 100644 --- a/internal/viewer/store.go +++ b/internal/viewer/store.go @@ -16,7 +16,9 @@ import ( "io" "os" "path/filepath" + "regexp" "sort" + "strconv" "strings" "time" @@ -321,6 +323,143 @@ type ToolCallInfo struct { DurationMs int64 } +// GroupingFileRef is one file inside a grouping group, resolved from the integer +// index the model returned back to its path. Resolved is false when the index +// named no file in the request's list — rendered as "#" so an auditor sees +// the anomaly rather than a silently dropped entry. +type GroupingFileRef struct { + Index int + Path string + Resolved bool +} + +// GroupingGroupView is one semantic group as shown in the viewer: the model's +// label plus its files resolved back to paths. It represents the grouping LLM +// call's *proposed* partition — the record this card holds. The final partition +// the reviewer actually used can differ, because enforceMaxFilesPerGroup and +// enforceGroupTokenBudget may re-split it afterwards; that is not part of this +// record (it surfaces only in the CLI `--format json` "groups" field). +type GroupingGroupView struct { + Label string + Files []GroupingFileRef +} + +// groupingFileListRe matches one line of the numbered file list buildFileList +// emits into the grouping request, e.g. "[0] MODIFIED internal/x.go (+12/-3)". +// It couples the viewer to buildFileList's "[%d] " prefix + formatDiffEntry's +// "STATUS path (+N/-M)" shape (internal/agent/grouping.go, agent/agent.go); +// TestBuildGroupingIndex guards that coupling. The trailing "(+N/-M)" anchors +// the path capture so a path with spaces still resolves. +var groupingFileListRe = regexp.MustCompile(`(?m)^\[(\d+)\]\s+\S+\s+(.+?)\s+\(\+\d+/-\d+\)\s*$`) + +// buildGroupingIndex scans the grouping request messages for the numbered file +// list and returns an index→path map. reqMessages is the raw JSONL value +// (a []any of map[string]any with a string "content"). Returns nil if nothing +// parses, which makes groupingView fall back to the raw response text. +func buildGroupingIndex(reqMessages any) map[int]string { + msgs, ok := reqMessages.([]any) + if !ok { + return nil + } + index := make(map[int]string) + for _, m := range msgs { + mm, ok := m.(map[string]any) + if !ok { + continue + } + // Only the user message carries the actual file list. Scanning the system + // prompt too would let its worked example (grouping_task_system.md's + // "[0] MODIFIED path ...") — which users can reword onto its own line — + // seed a bogus index→path entry that silently mislabels an out-of-range + // index instead of showing it as "#idx". + if role, _ := mm["role"].(string); role != "user" { + continue + } + content, ok := mm["content"].(string) + if !ok { + continue + } + for _, match := range groupingFileListRe.FindAllStringSubmatch(content, -1) { + idx, err := strconv.Atoi(match[1]) + if err != nil { + continue + } + index[idx] = match[2] + } + } + if len(index) == 0 { + return nil + } + return index +} + +// groupingResponseView mirrors one element of the grouping LLM response (label +// plus integer file indices) as the viewer consumes it. +type groupingResponseView struct { + Label string `json:"label"` + Files []int `json:"files"` +} + +// parseGroupingGroups parses the grouping LLM response into label + integer +// indices. It mirrors parseGroupingResponse's one-shot Unmarshal and +// markdown-fence stripping. ok is false when the content is not the index-shaped +// JSON — a parse failure, or (notably) a session recorded before the index +// switch whose "files" held path strings; the caller then keeps showing the raw +// response, which for those older sessions is already the readable path form. +func parseGroupingGroups(content string) (groups []groupingResponseView, ok bool) { + content = strings.TrimSpace(content) + if strings.HasPrefix(content, "```") { + lines := strings.Split(content, "\n") + if len(lines) >= 2 { + lines = lines[1:] + } + if len(lines) > 0 && strings.HasPrefix(strings.TrimSpace(lines[len(lines)-1]), "```") { + lines = lines[:len(lines)-1] + } + content = strings.Join(lines, "\n") + } + if err := json.Unmarshal([]byte(content), &groups); err != nil { + return nil, false + } + return groups, true +} + +// groupingView resolves a grouping task card into a path-labelled view of the +// model's proposed groups, or nil when either side is missing/unparseable (the +// template then falls back to the raw response text). +func groupingView(card *TaskCard) []GroupingGroupView { + if card == nil { + return nil + } + groups, ok := parseGroupingGroups(card.ResponseContent) + if !ok { + return nil + } + index := buildGroupingIndex(card.RequestMessages) + if index == nil { + return nil + } + views := make([]GroupingGroupView, 0, len(groups)) + anyResolved := false + for _, g := range groups { + gv := GroupingGroupView{Label: g.Label, Files: make([]GroupingFileRef, 0, len(g.Files))} + for _, idx := range g.Files { + path, resolved := index[idx] + anyResolved = anyResolved || resolved + gv.Files = append(gv.Files, GroupingFileRef{Index: idx, Path: path, Resolved: resolved}) + } + views = append(views, gv) + } + // The request list matched the regex (index != nil) yet not one referenced + // index resolved: the list format has drifted from what the response indexes + // into. Rendering every file as "#idx" would be more misleading than the raw + // response, so fall back to it. + if !anyResolved { + return nil + } + return views +} + // LoadSession fully parses a JSONL file into a ViewSession. func LoadSession(root, encodedRepo, sessionID string) (*ViewSession, error) { path := filepath.Join(root, encodedRepo, sessionID+".jsonl") diff --git a/internal/viewer/store_test.go b/internal/viewer/store_test.go index ddc3ea6f7..57de11c4e 100644 --- a/internal/viewer/store_test.go +++ b/internal/viewer/store_test.go @@ -21,6 +21,143 @@ func writeJSONL(t *testing.T, path string, lines ...string) { } } +func TestBuildGroupingIndex(t *testing.T) { + // The list mirrors buildFileList([%d] ) + formatDiffEntry (STATUS path (+N/-M)) + // across every status, plus a path with a space to prove the "(+N/-M)" anchor. + content := "Group the following changed files:\n\n" + + "[0] ADDED internal/auth/handler.go (+10/-0)\n" + + "[1] MODIFIED internal/auth/handler_test.go (+5/-2)\n" + + "[2] RENAMED cmd/app/main.go (+2/-1)\n" + + "[3] DELETED docs/old notes.md (+0/-7)\n\n" + + "Respond with a JSON array" + msgs := []any{ + map[string]any{"role": "system", "content": "You are a file grouping assistant."}, + map[string]any{"role": "user", "content": content}, + } + index := buildGroupingIndex(msgs) + want := map[int]string{ + 0: "internal/auth/handler.go", + 1: "internal/auth/handler_test.go", + 2: "cmd/app/main.go", + 3: "docs/old notes.md", + } + if len(index) != len(want) { + t.Fatalf("got %d entries, want %d: %v", len(index), len(want), index) + } + for k, v := range want { + if index[k] != v { + t.Errorf("index[%d] = %q, want %q", k, index[k], v) + } + } + + if buildGroupingIndex(nil) != nil { + t.Error("nil messages should yield nil index") + } + if buildGroupingIndex([]any{map[string]any{"role": "user", "content": "no file list here"}}) != nil { + t.Error("content without a file list should yield nil index") + } +} + +func TestBuildGroupingIndex_IgnoresSystemMessage(t *testing.T) { + // Only the user message's list may seed the map. A worked example living in + // the system prompt (which users can reword onto its own line) must not + // pollute the index — otherwise an out-of-range index would render as a bogus + // path instead of "#idx". + msgs := []any{ + map[string]any{"role": "system", "content": "e.g.\n[0] MODIFIED bogus/from-prompt.go (+1/-1)\n"}, + map[string]any{"role": "user", "content": "[0] MODIFIED real/file.go (+2/-1)\n"}, + } + index := buildGroupingIndex(msgs) + if index[0] != "real/file.go" { + t.Errorf("index[0] = %q, want the user message's path (system example must be ignored)", index[0]) + } +} + +func TestParseGroupingGroups(t *testing.T) { + t.Run("plain index JSON", func(t *testing.T) { + groups, ok := parseGroupingGroups(`[{"label":"auth","files":[0,1]},{"label":"docs","files":[2]}]`) + if !ok { + t.Fatal("expected ok") + } + if len(groups) != 2 || groups[0].Label != "auth" || len(groups[0].Files) != 2 || groups[0].Files[1] != 1 { + t.Errorf("unexpected parse: %+v", groups) + } + }) + t.Run("markdown fenced", func(t *testing.T) { + groups, ok := parseGroupingGroups("```json\n" + `[{"label":"all","files":[0,1]}]` + "\n```") + if !ok || len(groups) != 1 || len(groups[0].Files) != 2 { + t.Errorf("fenced parse failed: ok=%v groups=%+v", ok, groups) + } + }) + t.Run("legacy path-string response is not index-shaped", func(t *testing.T) { + // Sessions recorded before the index switch had "files" as path strings. + if _, ok := parseGroupingGroups(`[{"label":"auth","files":["a.go","b.go"]}]`); ok { + t.Error("path-string files should report ok=false so the viewer falls back to raw text") + } + }) + t.Run("garbage", func(t *testing.T) { + if _, ok := parseGroupingGroups("not json at all"); ok { + t.Error("non-JSON should report ok=false") + } + }) + t.Run("truncated response is not index-shaped", func(t *testing.T) { + // One-shot Unmarshal of a cut-off array fails, so the viewer falls back to + // the raw text — the same call the backend recorded. + if _, ok := parseGroupingGroups(`[{"label":"g1","files":[0,1]},{"label":"g2","fil`); ok { + t.Error("a truncated response should report ok=false") + } + }) +} + +func TestGroupingView(t *testing.T) { + req := []any{map[string]any{"role": "user", "content": "" + + "[0] MODIFIED a.go (+1/-1)\n" + + "[1] MODIFIED b.go (+2/-0)\n"}} + + t.Run("resolves indices to paths, flags out-of-range", func(t *testing.T) { + card := &TaskCard{ + RequestMessages: req, + ResponseContent: `[{"label":"g","files":[0,1,9]}]`, + } + views := groupingView(card) + if len(views) != 1 || len(views[0].Files) != 3 { + t.Fatalf("unexpected views: %+v", views) + } + if !views[0].Files[0].Resolved || views[0].Files[0].Path != "a.go" { + t.Errorf("file 0 = %+v, want resolved a.go", views[0].Files[0]) + } + if views[0].Files[2].Resolved { + t.Errorf("index 9 should be unresolved, got %+v", views[0].Files[2]) + } + }) + t.Run("nil when request list missing", func(t *testing.T) { + card := &TaskCard{ResponseContent: `[{"label":"g","files":[0]}]`} + if groupingView(card) != nil { + t.Error("missing request list should yield nil (fall back to raw)") + } + }) + t.Run("nil when response not index-shaped", func(t *testing.T) { + card := &TaskCard{RequestMessages: req, ResponseContent: `[{"label":"g","files":["a.go"]}]`} + if groupingView(card) != nil { + t.Error("legacy path response should yield nil (fall back to raw)") + } + }) + t.Run("nil card", func(t *testing.T) { + if groupingView(nil) != nil { + t.Error("nil card should yield nil") + } + }) + t.Run("nil when no referenced index resolves (format drift)", func(t *testing.T) { + // Request list parses (index != nil) but every index the response cites is + // out of range: the list shape drifted from what the response indexes into, + // so a wall of "#idx" would mislead — fall back to raw instead. + card := &TaskCard{RequestMessages: req, ResponseContent: `[{"label":"g","files":[8,9]}]`} + if groupingView(card) != nil { + t.Error("all-unresolved indices should yield nil (fall back to raw)") + } + }) +} + func TestDiscoverRepos_Empty(t *testing.T) { root := t.TempDir() repos, err := DiscoverRepos(root) diff --git a/internal/viewer/templates/session.html b/internal/viewer/templates/session.html index d1e6df8d2..f98066bdf 100644 --- a/internal/viewer/templates/session.html +++ b/internal/viewer/templates/session.html @@ -261,6 +261,7 @@

{{$tg.Type}}

{{range $tg.Cards}} + {{$card := .}}
Request #{{.RequestNo}} @@ -291,10 +292,36 @@

{{end}} - {{with .ResponseContent}} -
-
{{.}}
-
+ {{if isGrouping $tg.Type}} + {{with groupingView $card}} +
+ {{range .}} +
+ {{.Label}} +
    + {{range .Files}} + {{if .Resolved}}
  • {{.Path}}
    • {{else}}
  • #{{.Index}}
    • {{end}} + {{end}} +
+
+ {{end}} +
+ {{with $card.ResponseContent}} +
+ + + Raw LLM response + +
+
{{.}}
+
+
+ {{end}} + {{else}} + {{template "responseBody" $card.ResponseContent}} + {{end}} + {{else}} + {{template "responseBody" .ResponseContent}} {{end}} {{if .ToolCalls}}
@@ -336,3 +363,11 @@

{{end}} + +{{define "responseBody"}} +{{with .}} +
+
{{.}}
+
+{{end}} +{{end}}