diff --git a/apps/backend/internal/agentctl/server/process/git_log.go b/apps/backend/internal/agentctl/server/process/git_log.go index b7bb8ed8a9..d85a2920dd 100644 --- a/apps/backend/internal/agentctl/server/process/git_log.go +++ b/apps/backend/internal/agentctl/server/process/git_log.go @@ -6,6 +6,7 @@ import ( "fmt" "os/exec" "sort" + "strconv" "strings" "github.com/kandev/kandev/internal/common/unidiff" @@ -270,7 +271,13 @@ func (g *GitOperator) GetCumulativeDiff(ctx context.Context, baseCommit string) // `-` with nothing to compare — but both routes report 0/0 for them anyway, // since a binary block carries no hunk.) Adding the flag would change a // production git invocation for a difference no test can observe. - diffOutput, err := g.runGitCommand(ctx, "diff", baseCommit) + diffOutput, err := g.runGitCommand( + ctx, + "diff", + "--src-prefix=a/", + "--dst-prefix=b/", + baseCommit, + ) if err != nil { result.Error = fmt.Sprintf("failed to get diff: %s", err.Error()) return result, nil @@ -406,6 +413,8 @@ func (g *GitOperator) ShowCommit(ctx context.Context, commitSHA string) (*Commit "--stat", "--numstat", "-p", + "--src-prefix=a/", + "--dst-prefix=b/", commitSHA, ) if err != nil { @@ -447,45 +456,36 @@ func (g *GitOperator) parseCommitDiff(output string) map[string]interface{} { func (g *GitOperator) parseCommitDiffWithOptions(output string, opts parseCommitDiffOptions) map[string]interface{} { files := make(map[string]interface{}) - // Split by "diff --git" to get individual file diffs - parts := strings.Split(output, "diff --git ") - if len(parts) <= 1 { + // Split into per-file sections. The split is line-aware: only a line that + // STARTS with `diff --git ` begins a new section, because git only emits + // that token at the start of a section header. Splitting the raw byte + // stream would truncate a real section when a patch body or filename + // contains the literal sequence (e.g. an added line + // `+diff --git a/example b/example`) and fabricate a bogus section. + prefix, sections := splitDiffSections(output) + if len(sections) == 0 { return files } // When the caller's git command included --numstat, git printed authoritative // per-file counts ahead of the first "diff --git" — exactly the text the split - // above discards. Recover them; files without a row fall back to counting + // discards. Recover them; files without a row fall back to counting // their own patch body. - numstat := numstatByPath(parts[0]) + numstat := numstatByPath(prefix) totalDiffBytes := 0 - for _, part := range parts[1:] { - if part == "" { + for _, diffContent := range sections { + // Extract the file path. git's per-file `+++ b/` line is the + // authoritative source: it carries exactly one path, so spaces and + // literal ` b/` sequences inside a filename can't be confused with + // the old/new separator, and git C-quotes paths containing tabs, + // non-ASCII bytes, quotes, or backslashes (core.quotePath, default + // on) — a header-only split would drop those entirely. + filePath, ok := diffSectionPath(diffContent) + if !ok { continue } - // Re-add the "diff --git " prefix - diffContent := "diff --git " + part - - // Extract file path from the diff header - // Format: diff --git a/path/to/file b/path/to/file - lines := strings.SplitN(diffContent, "\n", 2) - if len(lines) == 0 { - continue - } - - header := lines[0] - // Extract path from "diff --git a/ b/". - // We cannot split by space because paths may contain spaces. - // Instead, strip the known prefix and find the " b/" separator. - pathsPart := strings.TrimPrefix(header, "diff --git ") - bIdx := strings.Index(pathsPart, " b/") - if bIdx == -1 { - continue - } - filePath := pathsPart[bIdx+3:] - // Determine file status from diff content status := fileStatusModified switch { @@ -504,6 +504,7 @@ func (g *GitOperator) parseCommitDiffWithOptions(output string, opts parseCommit totalDiffBytes += len(diffOut) fileEntry := map[string]interface{}{ + "path": filePath, "status": status, "staged": false, "additions": additions, @@ -519,6 +520,308 @@ func (g *GitOperator) parseCommitDiffWithOptions(output string, opts parseCommit return files } +// diffSectionPath extracts the new-side path of one `diff --git` section. +// The `+++ b/` file line is the authoritative source: git writes it +// once per section, exactly one path per line, so spaces and literal ` b/` +// sequences inside filenames can't confuse the old/new split, and git +// C-quotes paths containing special bytes (tabs, non-ASCII, quotes, +// backslashes) the same way it quotes the header. Sections that carry no +// file lines fall back to other unambiguous per-section lines before the +// `diff --git` header: +// - deleted files: `--- a/` (the `+++` side is `/dev/null`); +// - pure renames without hunks: `rename to `; +// - binary sections: `Binary files a/ and b/ differ`; +// - mode-only changes: the quoted/unquoted header itself. +// +// A single trailing tab on a file line is git's marker for paths that end in +// whitespace; it is stripped but any real trailing whitespace is kept. +func diffSectionPath(diffContent string) (string, bool) { + lines := strings.Split(diffContent, "\n") + + // First pass prefers the `+++ b/` line: in a rename section the + // `--- a/` line precedes it, and the new-side path is what the + // changes panel shows. `+++ /dev/null` (deleted files) yields nothing, + // so the second pass falls back to the `--- a/` old-side line. + for _, line := range lines { + if path, ok := diffFileLinePath("+++", line); ok { + return path, true + } + } + for _, line := range lines { + if path, ok := diffFileLinePath("---", line); ok { + return path, true + } + } + // Pure renames (similarity 100%, no hunks) emit rename from/to lines with + // no ---/+++ file lines; `rename to ` is one path per line. + for _, line := range lines { + if path, ok := diffWholeLinePath("rename to ", line); ok { + return path, true + } + } + // Binary sections emit `Binary files a/ and b/ differ` with no + // ---/+++ lines; parse the new-side path off that line. + for _, line := range lines { + if path, ok := diffBinaryFilesNewPath(line); ok { + return path, true + } + } + // Mode-only changes (old mode/new mode) have no path-bearing line at all; + // parse the header, handling git's C-quoted form. + return diffHeaderNewPath(lines[0]) +} + +// splitDiffSections splits git diff/show output into the pre-header prefix +// (numstat rows when --numstat was used) and one section per file, each +// section starting with its `diff --git ` header line. The split only breaks +// at lines that begin with the token — git's actual section-boundary +// grammar — so the token appearing inside a patch body or filename cannot +// truncate a real section or create a bogus one. +func splitDiffSections(output string) (prefix string, sections []string) { + first := firstDiffHeaderIndex(output) + prefix = output[:first] + rest := output[first:] + for rest != "" { + // rest starts at a `diff --git ` header line; the next section + // begins at the next line that starts with the token. + next := strings.Index(rest, "\ndiff --git ") + if next == -1 { + sections = append(sections, rest) + break + } + sections = append(sections, rest[:next+1]) + rest = rest[next+1:] + } + return prefix, sections +} + +// firstDiffHeaderIndex returns the byte offset of the first line that starts +// with `diff --git `, or len(s) when there is none. +func firstDiffHeaderIndex(s string) int { + for i := 0; i < len(s); { + lineEnd := i + for lineEnd < len(s) && s[lineEnd] != '\n' { + lineEnd++ + } + if strings.HasPrefix(s[i:lineEnd], "diff --git ") { + return i + } + if lineEnd >= len(s) { + return len(s) + } + i = lineEnd + 1 + } + return len(s) +} + +// diffHeaderNewPath parses the new-side path out of a `diff --git` header, +// supporting git's C-quoted form. The unquoted form (`a/ b/`) can +// only be split at the first ` b/` — a path that itself contains ` b/` +// renders unquoted and is inherently ambiguous in git's text output; the +// per-line sources above disambiguate every section that carries them. +func diffHeaderNewPath(header string) (string, bool) { + rest, ok := strings.CutPrefix(header, "diff --git ") + if !ok { + return "", false + } + if strings.HasPrefix(rest, `"`) { + // Quoted: `"a/" "b/"`. Both tokens are C-quoted, so scan + // the first token's closing quote escape-aware: a `\"` inside the + // path must not be read as the end of the token. The second token + // starts right after the closing quote and the separating space. + oldEnd := quotedTokenEnd(rest) + if oldEnd < 0 || oldEnd >= len(rest) || rest[oldEnd] != ' ' { + return "", false + } + after := rest[oldEnd+1:] + if !strings.HasPrefix(after, `"`) { + return "", false + } + newEnd := quotedTokenEnd(after) + if newEnd < 0 { + return "", false + } + newPath := unquoteGitPath(after[:newEnd]) + return strings.TrimPrefix(newPath, "b/"), true + } + // Unquoted: `a/ b/`. A path that itself contains ` b/` renders + // unquoted and the first occurrence splits inside the old path. Mode-only + // changes (the main consumer of this fallback) always carry identical + // old/new paths, so scan candidates and prefer the separator whose two + // sides unquote to equal paths — the same rule the binary parser uses. + // Fall back to the first candidate when none match (a hypothetical + // rename-without-rename-lines would land here). + firstSep := -1 + search := rest + base := 0 + for { + sep := strings.Index(search, " b/") + if sep < 0 { + break + } + // `abs` points at the space; the `b/` prefix of the new side starts + // one byte later, so the right side (prefix included) is rest[abs+1:]. + abs := base + sep + if firstSep == -1 { + firstSep = abs + } + left, leftOK := headerSidePath(rest[:abs], "a/") + right, rightOK := headerSidePath(rest[abs+1:], "b/") + if leftOK && rightOK && left == right { + return right, true + } + base = abs + 3 + search = rest[base:] + } + if firstSep == -1 { + return "", false + } + return unquoteGitPath(rest[firstSep+3:]), true +} + +// headerSidePath strips a fixed a/ or b/ prefix (optionally inside git's +// C-quotes) off one side of a diff header or Binary files line, returning the +// unquoted path. +func headerSidePath(side, prefix string) (string, bool) { + if strings.HasPrefix(side, `"`) { + unquoted, err := strconv.Unquote(side) + if err != nil { + return "", false + } + side = unquoted + } + if !strings.HasPrefix(side, prefix) { + return "", false + } + return strings.TrimPrefix(side, prefix), true +} + +// diffWholeLinePath reads a path off a line whose entire remainder is the +// path (`rename to `, `rename from `), C-unquoting when git +// quoted it. One path per line, so spaces inside the path are unambiguous. +func diffWholeLinePath(prefix, line string) (string, bool) { + rest, ok := strings.CutPrefix(line, prefix) + if !ok { + return "", false + } + if rest == "" { + return "", false + } + return unquoteGitPath(rest), true +} + +// quotedTokenEnd returns the index just past the closing quote of the first +// C-quoted token in s (which must start with `"`), or -1 when unterminated. +// Escapes are honored: a `\"` inside the path is content, not a terminator. +func quotedTokenEnd(s string) int { + if !strings.HasPrefix(s, `"`) { + return -1 + } + for i := 1; i < len(s); i++ { + switch s[i] { + case '\\': + i++ // skip the escaped byte + case '"': + return i + 1 + } + } + return -1 +} + +// diffBinaryFilesNewPath parses the new-side path off a +// `Binary files a/ and b/ differ` line (git's shape for binary +// sections, which carry no ---/+++ file lines). Both sides may be C-quoted: +// `Binary files "a/" and "b/" differ`. +// +// For a binary content change git emits the SAME path on both sides, so the +// separator whose two sides unquote to equal paths is authoritative — this +// resolves paths that themselves contain ` and b/` (a valid filename; the +// first-candidate scan would split inside the old path). When no candidate +// matches (binary rename with differing paths), the first ` and ` whose +// right side starts the `b/` token wins. +func diffBinaryFilesNewPath(line string) (string, bool) { + rest, ok := strings.CutPrefix(line, "Binary files ") + if !ok { + return "", false + } + rest = strings.TrimSuffix(rest, " differ") + if rest == "" { + return "", false + } + + firstSep := -1 + search := rest + base := 0 + for { + sep := strings.Index(search, " and ") + if sep < 0 { + break + } + abs := base + sep + right := search[sep+5:] + rightBare := strings.TrimPrefix(right, `"`) + if strings.HasPrefix(rightBare, "b/") { + if firstSep == -1 { + firstSep = abs + } + // Equal-paths check: the left side is everything before this + // candidate separator; both sides may be quoted. + leftPath, leftOK := binarySidePath(rest[:abs]) + rightPath, rightOK := binarySidePath(right) + if leftOK && rightOK && leftPath == rightPath { + return rightPath, true + } + } + base = abs + 5 + search = rest[base:] + } + if firstSep == -1 { + return "", false + } + newSide, ok := binarySidePath(rest[firstSep+5:]) + if !ok { + return "", false + } + return newSide, true +} + +// binarySidePath strips the `a/` or `b/` prefix (optionally inside git's +// C-quotes) off one side of a `Binary files` line, returning the unquoted +// path. +func binarySidePath(side string) (string, bool) { + if strings.HasPrefix(side, `"`) { + unquoted, err := strconv.Unquote(side) + if err != nil { + return "", false + } + side = unquoted + } + if !strings.HasPrefix(side, "a/") && !strings.HasPrefix(side, "b/") { + return "", false + } + return side[2:], true +} + +// diffFileLinePath reads the path off one `+++ ` or `--- ` file +// line (per git's per-section file headers), returning ok=false for +// `/dev/null` sides and non-file lines. Quoted paths (git C-quoting) carry +// the `a/`/`b/` prefix inside the quotes; the trailing-tab whitespace marker +// sits outside them, so it is stripped before unquoting. +func diffFileLinePath(prefix, line string) (string, bool) { + rest, ok := strings.CutPrefix(line, prefix+" ") + if !ok { + return "", false + } + if strings.HasPrefix(rest, "/dev/null") { + return "", false + } + path := unquoteGitPath(strings.TrimSuffix(rest, "\t")) + if prefix == "+++" { + return strings.TrimPrefix(path, "b/"), true + } + return strings.TrimPrefix(path, "a/"), true +} + // numstatByPath indexes the `\t\t` rows git writes for // --numstat by their new-side path (parseNumstatEntry already expands git's // `dir/{old => new}` rename notation, so the key matches the path diff --git a/apps/backend/internal/agentctl/server/process/git_log_diffstats_special_test.go b/apps/backend/internal/agentctl/server/process/git_log_diffstats_special_test.go new file mode 100644 index 0000000000..5f6d5233b0 --- /dev/null +++ b/apps/backend/internal/agentctl/server/process/git_log_diffstats_special_test.go @@ -0,0 +1,313 @@ +package process + +// Special-path and section-boundary parser regressions for +// parseCommitDiffWithOptions. Kept in their own file (per the backend +// convention that new tests go in a new file rather than growing an +// already-large test file): the cases cover git's C-quoted paths (tab, +// non-ASCII octal, embedded quotes), ` b/`- and ` and b/`-containing names, +// binary/rename/mode-only sections, and header tokens inside patch bodies. + +import ( + "context" + "os" + "path/filepath" + "strings" + "testing" +) + +// TestParseCommitDiffWithOptions_SpecialPaths guards path extraction against +// git's quoting and separator rules. git C-quotes paths containing tabs, +// non-ASCII bytes, quotes, or backslashes (core.quotePath, default on) — +// a naive first-` b/` split drops them entirely — and a path containing the +// literal sequence ` b/` mis-splits the header. The parser must read the +// path off the per-file `+++ b/` line, which git writes once per +// section and is unambiguous. +func TestParseCommitDiffWithOptions_SpecialPaths(t *testing.T) { + tests := []struct { + name string + output string + wantPath string + }{ + { + name: "tab in path is C-quoted by git", + output: "diff --git \"a/a\\tb.txt\" \"b/a\\tb.txt\"\n" + + "new file mode 100644\n" + + "index 0000000..e69de29\n" + + "--- /dev/null\n" + + "+++ \"b/a\\tb.txt\"\n" + + "@@ -0,0 +1 @@\n" + + "+content\n", + wantPath: "a\tb.txt", + }, + { + name: "non-ASCII path is C-quoted with octal escapes", + output: "diff --git \"a/caf\\303\\251.txt\" \"b/caf\\303\\251.txt\"\n" + + "new file mode 100644\n" + + "--- /dev/null\n" + + "+++ \"b/caf\\303\\251.txt\"\n" + + "@@ -0,0 +1 @@\n" + + "+caf\u00e9\n", + wantPath: "caf\u00e9.txt", + }, + { + name: "quote char in path is C-quoted", + output: "diff --git \"a/has\\\"quote.txt\" \"b/has\\\"quote.txt\"\n" + + "new file mode 100644\n" + + "--- /dev/null\n" + + "+++ \"b/has\\\"quote.txt\"\n" + + "@@ -0,0 +1 @@\n" + + "+content\n", + wantPath: "has\"quote.txt", + }, + { + name: "path containing literal b-slash splits via the +++ line", + output: "diff --git a/name b/dir.txt b/name b/dir.txt\n" + + "new file mode 100644\n" + + "--- /dev/null\n" + + "+++ b/name b/dir.txt\n" + + "@@ -0,0 +1 @@\n" + + "+content\n", + wantPath: "name b/dir.txt", + }, + { + name: "trailing-whitespace path keeps the tab marker stripped", + output: "diff --git a/trail.txt b/trail.txt \n" + + "index 111..222 100644\n" + + "--- a/trail.txt \t\n" + + "+++ b/trail.txt \t\n" + + "@@ -1 +1 @@\n" + + "-old\n" + + "+new\n", + wantPath: "trail.txt ", + }, + { + name: "quoted path ending in whitespace strips the outer marker", + output: "diff --git \"a/weird \\\"quote.txt \" \"b/weird \\\"quote.txt \"\n" + + "index 111..222 100644\n" + + "--- \"a/weird \\\"quote.txt \"\t\n" + + "+++ \"b/weird \\\"quote.txt \"\t\n" + + "@@ -1 +1 @@\n" + + "-old\n" + + "+new\n", + wantPath: "weird \"quote.txt ", + }, + { + name: "renamed file keeps the new path from the +++ line", + output: "diff --git a/apps/old/file.go b/apps/new/file.go\n" + + "similarity index 90%\n" + + "rename from apps/old/file.go\n" + + "rename to apps/new/file.go\n" + + "--- a/apps/old/file.go\n" + + "+++ b/apps/new/file.go\n" + + "@@ -1,2 +1,3 @@\n" + + " package p\n" + + "+added\n", + wantPath: "apps/new/file.go", + }, + { + name: "pure rename without hunks keeps the rename to line path", + output: "diff --git a/old b.txt b/new b.txt\n" + + "similarity index 100%\n" + + "rename from old b.txt\n" + + "rename to new b.txt\n", + wantPath: "new b.txt", + }, + { + name: "binary section with b-slash path parses the Binary files line", + output: "diff --git a/name b/binary.bin b/name b/binary.bin\n" + + "index 6735744..d7bf111 100644\n" + + "Binary files a/name b/binary.bin and b/name b/binary.bin differ\n", + wantPath: "name b/binary.bin", + }, + { + name: "quoted binary section unquotes the Binary files line", + output: "diff --git \"a/caf\\303\\251 b.bin\" \"b/caf\\303\\251 b.bin\"\n" + + "index a6a3e7f..073ea92 100644\n" + + "Binary files \"a/caf\\303\\251 b.bin\" and \"b/caf\\303\\251 b.bin\" differ\n", + wantPath: "caf\u00e9 b.bin", + }, + { + name: "mode-only change on a quoted path parses the quoted header", + output: "diff --git \"a/caf\\303\\251 b/mode.sh\" \"b/caf\\303\\251 b/mode.sh\"\n" + + "old mode 100644\n" + + "new mode 100755\n", + wantPath: "caf\u00e9 b/mode.sh", + }, + { + name: "mode-only change with escaped quote in path parses the quoted header", + output: "diff --git \"a/has\\\"quote.sh\" \"b/has\\\"quote.sh\"\n" + + "old mode 100644\n" + + "new mode 100755\n", + wantPath: "has\"quote.sh", + }, + { + name: "mode-only change with b-slash path splits on the equal-paths separator", + output: "diff --git a/mode name b/dir.sh b/mode name b/dir.sh\n" + + "old mode 100644\n" + + "new mode 100755\n", + wantPath: "mode name b/dir.sh", + }, + { + name: "mode-only change with quote and b-slash in path", + output: "diff --git \"a/weird\\\"quote b/file.sh\" \"b/weird\\\"quote b/file.sh\"\n" + + "old mode 100644\n" + + "new mode 100755\n", + wantPath: "weird\"quote b/file.sh", + }, + { + name: "binary path containing and b-slash splits on the equal-paths separator", + output: "diff --git a/old and b/thing.bin b/old and b/thing.bin\n" + + "index 6735744..d7bf111 100644\n" + + "Binary files a/old and b/thing.bin and b/old and b/thing.bin differ\n", + wantPath: "old and b/thing.bin", + }, + { + name: "binary rename with and b-slash in new path keeps the new path", + output: "diff --git a/x.bin b/old and b/thing.bin\n" + + "similarity index 50%\n" + + "rename from x.bin\n" + + "rename to old and b/thing.bin\n" + + "Binary files a/x.bin and b/old and b/thing.bin differ\n", + wantPath: "old and b/thing.bin", + }, + } + + gitOp := &GitOperator{} + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + files := gitOp.parseCommitDiffWithOptions(tt.output, parseCommitDiffOptions{}) + entry, ok := files[tt.wantPath].(map[string]interface{}) + if !ok { + t.Fatalf("no entry for %q; got keys %v", tt.wantPath, keysOf(files)) + } + if got, _ := entry["path"].(string); got != tt.wantPath { + t.Errorf("entry path = %#v, want %#v", got, tt.wantPath) + } + }) + } +} + +// TestGetCumulativeDiff_ModeOnlyBSlashPath covers the round-5 blocker with +// real git output: a chmod-only change on a path containing the literal +// sequence ` b/` produces an unquoted header with no ---/+++ lines +// (`diff --git a/mode name b/dir.sh b/mode name b/dir.sh` + old mode/new +// mode). The first ` b/` lies inside the old path, so the equal-paths +// separator rule must select the correct split. +func TestGetCumulativeDiff_ModeOnlyBSlashPath(t *testing.T) { + repoDir, cleanup := setupTestRepo(t) + t.Cleanup(cleanup) + + if err := os.MkdirAll(filepath.Join(repoDir, "mode name b"), 0o755); err != nil { + t.Fatalf("mkdir: %v", err) + } + writeFile(t, repoDir, "mode name b/dir.sh", "#!/bin/sh\necho hi\n") + runGit(t, repoDir, "add", ".") + runGit(t, repoDir, "commit", "-m", "seed: add script") + base := strings.TrimSpace(runGit(t, repoDir, "rev-parse", "HEAD")) + + // Pin core.filemode so the chmod below reliably produces a mode-only + // diff. Git's default (auto) disables executable-bit tracking on + // filesystems without chmod support; on those the test cannot produce + // the fixture it exists to prove, so skip with an explicit reason rather + // than fail. + runGit(t, repoDir, "config", "core.filemode", "true") + if err := os.Chmod(filepath.Join(repoDir, "mode name b/dir.sh"), 0o755); err != nil { + t.Fatalf("chmod: %v", err) + } + if diff := runGit(t, repoDir, "diff"); !strings.Contains(diff, "old mode") { + t.Skipf("filesystem does not track executable bits; skipping mode-only regression (diff=%q)", diff) + } + + gitOp := NewGitOperator(repoDir, newTestLogger(t), nil) + result, err := gitOp.GetCumulativeDiff(context.Background(), base) + if err != nil { + t.Fatalf("GetCumulativeDiff returned error: %v", err) + } + if !result.Success { + t.Fatalf("GetCumulativeDiff failed: %s", result.Error) + } + + const wantPath = "mode name b/dir.sh" + if _, ok := result.Files[wantPath].(map[string]interface{}); !ok { + t.Fatalf("no entry for %q; got keys %v", wantPath, keysOf(result.Files)) + } +} + +func TestGetCumulativeDiff_StablePrefixesIgnoreGitDiffConfig(t *testing.T) { + for _, diffConfig := range []struct { + name string + key string + value string + }{ + {name: "no prefix", key: "diff.noprefix", value: "true"}, + {name: "mnemonic prefix", key: "diff.mnemonicPrefix", value: "true"}, + } { + t.Run(diffConfig.name, func(t *testing.T) { + repoDir, cleanup := setupTestRepo(t) + t.Cleanup(cleanup) + + if err := os.MkdirAll(filepath.Join(repoDir, "src"), 0o755); err != nil { + t.Fatalf("mkdir src: %v", err) + } + writeFile(t, repoDir, "src/config.txt", "before\n") + runGit(t, repoDir, "add", ".") + runGit(t, repoDir, "commit", "-m", "seed config file") + base := strings.TrimSpace(runGit(t, repoDir, "rev-parse", "HEAD")) + runGit(t, repoDir, "config", diffConfig.key, diffConfig.value) + writeFile(t, repoDir, "src/config.txt", "after\n") + + result, err := NewGitOperator(repoDir, newTestLogger(t), nil).GetCumulativeDiff( + context.Background(), base, + ) + if err != nil { + t.Fatalf("GetCumulativeDiff returned error: %v", err) + } + if !result.Success { + t.Fatalf("GetCumulativeDiff failed: %s", result.Error) + } + if _, ok := result.Files["src/config.txt"].(map[string]interface{}); !ok { + t.Fatalf("no entry for src/config.txt; got keys %v", keysOf(result.Files)) + } + }) + } +} + +// TestParseCommitDiffWithOptions_HeaderTokenInsideBody guards the section +// boundary against a patch body (or filename) containing the literal +// `diff --git ` byte sequence. git only emits that token at the start of a +// section header line, so the split must be line-aware: an added line like +// `+diff --git a/example b/example` must not truncate the real section or +// fabricate a bogus one. +func TestParseCommitDiffWithOptions_HeaderTokenInsideBody(t *testing.T) { + output := "diff --git a/real.txt b/real.txt\n" + + "index 111..222 100644\n" + + "--- a/real.txt\n" + + "+++ b/real.txt\n" + + "@@ -1 +1,2 @@\n" + + "-old\n" + + "+diff --git a/example b/example\n" + + "+new\n" + + gitOp := &GitOperator{} + files := gitOp.parseCommitDiffWithOptions(output, parseCommitDiffOptions{}) + if len(files) != 1 { + t.Fatalf("got %d files, want exactly real.txt (%v)", len(files), keysOf(files)) + } + entry, ok := files["real.txt"].(map[string]interface{}) + if !ok { + t.Fatalf("no entry for real.txt; got keys %v", keysOf(files)) + } + if got, _ := entry["path"].(string); got != "real.txt" { + t.Errorf("entry path = %#v, want real.txt", got) + } + diff, _ := entry["diff"].(string) + if !strings.Contains(diff, "+diff --git a/example b/example") || !strings.Contains(diff, "+new") { + t.Errorf("real.txt diff truncated by header-token line: %q", diff) + } + if got, _ := entry["additions"].(int); got != 2 { + t.Errorf("real.txt additions = %d, want 2 (the added header-token line counts)", got) + } + if _, ok := files["example"]; ok { + t.Errorf("bogus example entry created from patch-body token: %v", keysOf(files)) + } +} diff --git a/apps/backend/internal/agentctl/server/process/git_log_diffstats_test.go b/apps/backend/internal/agentctl/server/process/git_log_diffstats_test.go index d6a5b89e45..f0a1196d89 100644 --- a/apps/backend/internal/agentctl/server/process/git_log_diffstats_test.go +++ b/apps/backend/internal/agentctl/server/process/git_log_diffstats_test.go @@ -309,6 +309,44 @@ func TestShowCommit_CountsDashAndPlusPrefixedContent(t *testing.T) { } } +// TestParseCommitDiffWithOptions_EntriesCarryPath guards the file-entry +// contract the frontend's changes tree depends on: every entry must include +// its own `path` field, not just sit under the map key. The DB-snapshot +// fallback replays these entries as a git status_update, and the frontend +// builds its file tree from each entry's `path` — an entry without one +// crashes the changes panel (`Cannot read properties of undefined (reading +// 'split')`) for archived tasks whose live execution is gone. +func TestParseCommitDiffWithOptions_EntriesCarryPath(t *testing.T) { + output := "diff --git a/apps/a.go b/apps/a.go\n" + + "--- a/apps/a.go\n" + + "+++ b/apps/a.go\n" + + "@@ -1,3 +1,3 @@\n" + + " package a\n" + + "-old\n" + + "+new\n" + + "diff --git a/readme.md b/readme.md\n" + + "--- a/readme.md\n" + + "+++ b/readme.md\n" + + "@@ -1 +1 @@\n" + + "-hello\n" + + "+world\n" + + gitOp := &GitOperator{} + files := gitOp.parseCommitDiffWithOptions(output, parseCommitDiffOptions{}) + if len(files) != 2 { + t.Fatalf("got %d files, want 2 (%v)", len(files), keysOf(files)) + } + for _, path := range []string{"apps/a.go", "readme.md"} { + entry, ok := files[path].(map[string]interface{}) + if !ok { + t.Fatalf("no file entry for %q", path) + } + if got, _ := entry["path"].(string); got != path { + t.Errorf("entry %q path = %#v, want %q", path, entry["path"], path) + } + } +} + // TestGetCumulativeDiff_CountsDashAndPlusPrefixedContent covers the second // caller, which builds its diff from `git diff ` rather than `git show`. func TestGetCumulativeDiff_CountsDashAndPlusPrefixedContent(t *testing.T) { diff --git a/apps/backend/internal/backendapp/helpers.go b/apps/backend/internal/backendapp/helpers.go index 66ebfc5631..7c12241bbc 100644 --- a/apps/backend/internal/backendapp/helpers.go +++ b/apps/backend/internal/backendapp/helpers.go @@ -405,8 +405,13 @@ func appendDBSnapshotGitStatus(ctx context.Context, taskRepo *sqliterepo.Reposit "status": map[string]interface{}{ branchFieldKey: latestSnapshot.Branch, "remote_branch": latestSnapshot.RemoteBranch, + "head_commit": latestSnapshot.HeadCommit, + "base_commit": latestSnapshot.BaseCommit, "ahead": latestSnapshot.Ahead, "behind": latestSnapshot.Behind, + "remote_ahead": metadata["remote_ahead"], + "remote_behind": metadata["remote_behind"], + "remote_head_commit": metadata["remote_head_commit"], "files": latestSnapshot.Files, "modified": metadata["modified"], addedFieldKey: metadata[addedFieldKey], diff --git a/apps/backend/internal/common/securityutil/git.go b/apps/backend/internal/common/securityutil/git.go index 7f8e59bc75..fa44a0d55c 100644 --- a/apps/backend/internal/common/securityutil/git.go +++ b/apps/backend/internal/common/securityutil/git.go @@ -77,6 +77,7 @@ func IsKnownSafeGitFlag(arg string) bool { "--amend", "--allow-empty", "--soft", "--mixed", "--hard", "--cached", "--force", "--source=HEAD", "--staged", "--worktree", "--dry-run", "--get-all", "--first-parent", "--is-ancestor", + "--src-prefix=", "--dst-prefix=", } for _, safe := range safeFlags { if strings.HasPrefix(arg, safe) { diff --git a/apps/backend/internal/common/securityutil/git_test.go b/apps/backend/internal/common/securityutil/git_test.go index 8d3cb62a9e..284300ef7c 100644 --- a/apps/backend/internal/common/securityutil/git_test.go +++ b/apps/backend/internal/common/securityutil/git_test.go @@ -2,6 +2,18 @@ package securityutil import "testing" +// TestIsKnownSafeGitFlagAllowsDiffPrefixFlags pins the output-formatting flags +// GetCumulativeDiff and ShowCommit pass to force stable a/ and b/ path +// prefixes regardless of a user's diff.noprefix config. They only affect diff +// header formatting (never repository mutation), so any value form is safe. +func TestIsKnownSafeGitFlagAllowsDiffPrefixFlags(t *testing.T) { + for _, flag := range []string{"--src-prefix=a/", "--dst-prefix=b/", "--src-prefix=old/", "--dst-prefix=new/"} { + if !IsKnownSafeGitFlag(flag) { + t.Fatalf("IsKnownSafeGitFlag(%q) = false, want true — the cumulative-diff parser needs it", flag) + } + } +} + func TestIsValidBaseBranchRef(t *testing.T) { cases := map[string]bool{ "main": true, diff --git a/apps/backend/internal/orchestrator/task_operations.go b/apps/backend/internal/orchestrator/task_operations.go index dba2df2e99..8c0fc141a3 100644 --- a/apps/backend/internal/orchestrator/task_operations.go +++ b/apps/backend/internal/orchestrator/task_operations.go @@ -17,7 +17,7 @@ import ( "go.uber.org/zap" agentruntime "github.com/kandev/kandev/internal/agent/runtime" - "github.com/kandev/kandev/internal/agent/runtime/agentctl" + client "github.com/kandev/kandev/internal/agent/runtime/agentctl" "github.com/kandev/kandev/internal/agent/runtime/lifecycle" "github.com/kandev/kandev/internal/agent/runtime/routingerr" "github.com/kandev/kandev/internal/common/constants" @@ -3435,13 +3435,27 @@ func (s *Service) captureArchiveDiff(ctx context.Context, sessionID, baseCommit return } - if err := s.repo.CreateGitSnapshot(ctx, &models.GitSnapshot{ + snapshot := &models.GitSnapshot{ SessionID: sessionID, SnapshotType: models.SnapshotTypeArchive, HeadCommit: diffResult.HeadCommit, BaseCommit: diffResult.BaseCommit, Files: diffResult.Files, - }); err != nil { + } + status, statusErr := s.agentManager.GetGitStatusFresh(ctx, sessionID) + if statusErr != nil { + s.logger.Warn("failed to capture git status metadata for archive", + zap.String("session_id", sessionID), + zap.Error(statusErr)) + } else if status != nil && status.Success { + snapshot.Branch = status.Branch + snapshot.RemoteBranch = status.RemoteBranch + snapshot.Ahead = status.Ahead + snapshot.Behind = status.Behind + snapshot.Metadata = archiveGitStatusMetadata(status, diffResult.Files) + } + + if err := s.repo.CreateGitSnapshot(ctx, snapshot); err != nil { s.logger.Warn("failed to save archive snapshot", zap.String("session_id", sessionID), zap.Error(err)) @@ -3454,6 +3468,29 @@ func (s *Service) captureArchiveDiff(ctx context.Context, sessionID, baseCommit zap.Int("total_commits", diffResult.TotalCommits)) } +func archiveGitStatusMetadata(status *client.GitStatusResult, files map[string]interface{}) map[string]interface{} { + if status == nil { + return nil + } + return map[string]interface{}{ + "timestamp": status.Timestamp, + // The archive's files map is a cumulative diff, while these status + // lists describe the working tree at capture time. Keep an explicit + // count for summary consumers so they do not add two different views. + "changed_files": len(files), + "modified": status.Modified, + "added": status.Added, + "deleted": status.Deleted, + "untracked": status.Untracked, + "renamed": status.Renamed, + "remote_ahead": status.RemoteAhead, + "remote_behind": status.RemoteBehind, + "remote_head_commit": status.RemoteHeadCommit, + "branch_additions": status.BranchAdditions, + "branch_deletions": status.BranchDeletions, + } +} + // parseCommitTime parses a commit timestamp from git log output. // Returns UTC time to ensure consistent timestamps across environments. func parseCommitTime(s string) time.Time { diff --git a/apps/backend/internal/orchestrator/task_operations_archive_test.go b/apps/backend/internal/orchestrator/task_operations_archive_test.go new file mode 100644 index 0000000000..a121449704 --- /dev/null +++ b/apps/backend/internal/orchestrator/task_operations_archive_test.go @@ -0,0 +1,50 @@ +package orchestrator + +import ( + "testing" + + client "github.com/kandev/kandev/internal/agent/runtime/agentctl" +) + +func TestArchiveGitStatusMetadataPreservesStatusSummary(t *testing.T) { + status := &client.GitStatusResult{ + Timestamp: "2026-08-20T14:00:00Z", + Modified: []string{"src/app.ts"}, + Added: []string{"src/new.ts"}, + Deleted: []string{"src/old.ts"}, + Untracked: []string{"tmp.txt"}, + Renamed: []string{"src/old-name.ts -> src/new-name.ts"}, + RemoteAhead: 2, + RemoteBehind: 1, + RemoteHeadCommit: "remote-head", + BranchAdditions: 8, + BranchDeletions: 3, + } + + metadata := archiveGitStatusMetadata(status, map[string]interface{}{ + "src/app.ts": map[string]interface{}{"status": "modified"}, + "src/new.ts": map[string]interface{}{"status": "added"}, + "src/old.ts": map[string]interface{}{"status": "deleted"}, + "src/renamed": map[string]interface{}{"status": "renamed"}, + }) + if metadata["timestamp"] != status.Timestamp { + t.Errorf("timestamp = %#v, want %q", metadata["timestamp"], status.Timestamp) + } + if metadata["changed_files"] != 4 { + t.Errorf("changed_files = %#v, want 4", metadata["changed_files"]) + } + if metadata["branch_additions"] != status.BranchAdditions { + t.Errorf("branch_additions = %#v, want %d", metadata["branch_additions"], status.BranchAdditions) + } + if metadata["branch_deletions"] != status.BranchDeletions { + t.Errorf("branch_deletions = %#v, want %d", metadata["branch_deletions"], status.BranchDeletions) + } + if metadata["remote_head_commit"] != status.RemoteHeadCommit { + t.Errorf("remote_head_commit = %#v, want %q", metadata["remote_head_commit"], status.RemoteHeadCommit) + } + for _, key := range []string{"modified", "added", "deleted", "untracked", "renamed"} { + if metadata[key] == nil { + t.Errorf("metadata[%q] is nil", key) + } + } +} diff --git a/apps/backend/internal/task/repository/git_snapshots_upsert_test.go b/apps/backend/internal/task/repository/git_snapshots_upsert_test.go index 647eb8a16e..e395a2c4a3 100644 --- a/apps/backend/internal/task/repository/git_snapshots_upsert_test.go +++ b/apps/backend/internal/task/repository/git_snapshots_upsert_test.go @@ -3,6 +3,7 @@ package repository import ( "context" "testing" + "time" "github.com/kandev/kandev/internal/task/models" "github.com/kandev/kandev/internal/task/repository/sqlite" @@ -262,6 +263,448 @@ func TestGetLatestGitSnapshot_PrefersAgentCompletedOverNewerLiveMonitor(t *testi } } +// TestGetLatestGitSnapshot_PrefersArchiveOverAgentCompleted guards the +// terminal-replay invariant: while a task is still archived, its archive +// snapshot (final cumulative diff captured at archive time) must be selected +// over any agent_completed row. Before the ordering fix, GetLatestGitSnapshot +// preferred triggered_by='agent_completed' regardless of snapshot_type, so +// archived tasks that had completed a turn were replayed from a stale +// live-status row and the archive snapshot (with the cumulative-diff files +// carrying `path`) was never surfaced. See round-2 review blocker on +// d18c1a312. +func TestGetLatestGitSnapshot_PrefersArchiveOverAgentCompleted(t *testing.T) { + ctx := context.Background() + repo, cleanup := createTestSQLiteRepo(t) + defer cleanup() + + const taskID = "task-arch" + const sessionID = "session-arch" + seedTaskAndSession(t, ctx, repo, taskID, sessionID) + + // The task must be archived for the archive snapshot to be authoritative; + // an unarchived task's archive row is stale (see + // TestGetLatestGitSnapshot_UnarchivedResumePrefersCompleted). + if err := repo.ArchiveTask(ctx, taskID); err != nil { + t.Fatalf("archive task: %v", err) + } + + // An agent_completed row exists from the last completed turn (older). + completed := &models.GitSnapshot{ + SessionID: sessionID, + Branch: "feature", + HeadCommit: "completed-head", + TriggeredBy: "agent_completed", + Metadata: map[string]interface{}{"branch_additions": float64(5), "branch_deletions": float64(2)}, + } + if err := repo.CreateGitSnapshot(ctx, completed); err != nil { + t.Fatalf("create agent_completed: %v", err) + } + + // The archive snapshot carries the final cumulative diff (with path fields). + archive := &models.GitSnapshot{ + SessionID: sessionID, + SnapshotType: models.SnapshotTypeArchive, + Branch: "feature", + HeadCommit: "archive-head", + BaseCommit: "base000", + Files: map[string]interface{}{ + "file.txt": map[string]interface{}{ + "path": "file.txt", + "status": "modified", + "additions": float64(3), + "deletions": float64(1), + }, + }, + } + if err := repo.CreateGitSnapshot(ctx, archive); err != nil { + t.Fatalf("create archive: %v", err) + } + + t.Run("single query", func(t *testing.T) { + got, err := repo.GetLatestGitSnapshot(ctx, sessionID) + if err != nil { + t.Fatalf("GetLatestGitSnapshot: %v", err) + } + if got.HeadCommit != "archive-head" { + t.Errorf("expected archive-head, got %q — archive snapshot shadowed by agent_completed row", got.HeadCommit) + } + if got.SnapshotType != models.SnapshotTypeArchive { + t.Errorf("expected snapshot_type=archive, got %q", got.SnapshotType) + } + }) + + t.Run("batch query", func(t *testing.T) { + got, err := repo.GetLatestGitSnapshotsBySessionIDs(ctx, []string{sessionID}) + if err != nil { + t.Fatalf("GetLatestGitSnapshotsBySessionIDs: %v", err) + } + if snap := got[sessionID]; snap == nil { + t.Fatal("expected a snapshot for session, got none") + } else if snap.HeadCommit != "archive-head" { + t.Errorf("expected archive-head, got %q — batch query shadowed by agent_completed row", snap.HeadCommit) + } + }) +} + +// TestGetLatestGitSnapshot_UnarchivedBeforeResumeKeepsArchive guards the +// unarchive-before-resume window: httpUnarchiveTask / UnarchiveTaskTree only +// clear archived_at and restore branches/workspaces — no snapshot is written +// until the session is resumed and the agent completes another turn. During +// that window the terminal archive row must remain authoritative; demoting it +// below an older agent_completed row would make a reload or task-list fallback +// show pre-archive files and diff counts. The archive row loses rank-0 only +// once a non-archive snapshot newer than it exists. Codex review P1 on PR +// #2851. +func TestGetLatestGitSnapshot_UnarchivedBeforeResumeKeepsArchive(t *testing.T) { + ctx := context.Background() + repo, cleanup := createTestSQLiteRepo(t) + defer cleanup() + + const taskID = "task-unarchive-pending" + const sessionID = "session-unarchive-pending" + seedTaskAndSession(t, ctx, repo, taskID, sessionID) + + // Older agent_completed row from a turn before the archive. + completed := &models.GitSnapshot{ + SessionID: sessionID, + Branch: "feature", + HeadCommit: "completed-before-archive", + TriggeredBy: "agent_completed", + } + if err := repo.CreateGitSnapshot(ctx, completed); err != nil { + t.Fatalf("create agent_completed: %v", err) + } + + // Task is archived; the terminal archive snapshot is captured. + if err := repo.ArchiveTask(ctx, taskID); err != nil { + t.Fatalf("archive task: %v", err) + } + archive := &models.GitSnapshot{ + SessionID: sessionID, + SnapshotType: models.SnapshotTypeArchive, + Branch: "feature", + HeadCommit: "archive-head", + BaseCommit: "base000", + Files: map[string]interface{}{ + "file.txt": map[string]interface{}{ + "path": "file.txt", + "status": "modified", + "additions": float64(3), + "deletions": float64(1), + }, + }, + } + if err := repo.CreateGitSnapshot(ctx, archive); err != nil { + t.Fatalf("create archive: %v", err) + } + + // Task is unarchived but NOT yet resumed: no newer snapshot exists. + if _, err := repo.UnarchiveTask(ctx, taskID); err != nil { + t.Fatalf("unarchive task: %v", err) + } + + // The archive row must still win — it is the newest snapshot and the + // only terminal state; the older agent_completed row is pre-archive. + t.Run("single query", func(t *testing.T) { + got, err := repo.GetLatestGitSnapshot(ctx, sessionID) + if err != nil { + t.Fatalf("GetLatestGitSnapshot: %v", err) + } + if got.HeadCommit != "archive-head" { + t.Errorf("expected archive-head, got %q — pre-archive agent_completed row outranks terminal archive before resume", got.HeadCommit) + } + }) + t.Run("batch query", func(t *testing.T) { + got, err := repo.GetLatestGitSnapshotsBySessionIDs(ctx, []string{sessionID}) + if err != nil { + t.Fatalf("GetLatestGitSnapshotsBySessionIDs: %v", err) + } + if snap := got[sessionID]; snap == nil { + t.Fatal("expected a snapshot for session, got none") + } else if snap.HeadCommit != "archive-head" { + t.Errorf("expected archive-head in batch, got %q", snap.HeadCommit) + } + }) +} + +// TestGetLatestGitSnapshot_UnarchivedResumePrefersCompleted guards the +// lifecycle-conditional archive ranking: an archive snapshot is authoritative +// only while its owning task is still archived. ArchiveTask captures one, but +// UnarchiveTask does not invalidate it, and an archive-cancelled session +// resumes on the SAME session ID (task_operations.go: resume-reason path for +// archive-cancelled sessions). Once the task is unarchived and the agent runs +// again, newer agent_completed / live_monitor rows describe the current +// execution and must win over the stale archive row — otherwise the DB-fallback +// status and the sidebar badge (loadTaskGitObservations) show the pre-resume +// state forever. Round-3 review blocker on 62bd42f3a. +func TestGetLatestGitSnapshot_UnarchivedResumePrefersCompleted(t *testing.T) { + ctx := context.Background() + + t.Run("completed row after unarchive wins over older archive row", func(t *testing.T) { + repo, cleanup := createTestSQLiteRepo(t) + defer cleanup() + + const taskID = "task-resume" + const sessionID = "session-resume" + seedTaskAndSession(t, ctx, repo, taskID, sessionID) + + // Task is archived; the archive snapshot is captured. + if err := repo.ArchiveTask(ctx, taskID); err != nil { + t.Fatalf("archive task: %v", err) + } + archive := &models.GitSnapshot{ + SessionID: sessionID, + SnapshotType: models.SnapshotTypeArchive, + Branch: "feature", + HeadCommit: "archive-head", + BaseCommit: "base000", + } + if err := repo.CreateGitSnapshot(ctx, archive); err != nil { + t.Fatalf("create archive: %v", err) + } + + // Task is unarchived and resumes; the agent completes a turn and + // writes a newer agent_completed row. + if _, err := repo.UnarchiveTask(ctx, taskID); err != nil { + t.Fatalf("unarchive task: %v", err) + } + completed := &models.GitSnapshot{ + SessionID: sessionID, + Branch: "feature", + HeadCommit: "completed-after-resume", + TriggeredBy: "agent_completed", + Metadata: map[string]interface{}{"branch_additions": float64(7), "branch_deletions": float64(3)}, + } + if err := repo.CreateGitSnapshot(ctx, completed); err != nil { + t.Fatalf("create agent_completed: %v", err) + } + + // The resumed agent_completed row must win — the archive row is stale. + t.Run("single query", func(t *testing.T) { + got, err := repo.GetLatestGitSnapshot(ctx, sessionID) + if err != nil { + t.Fatalf("GetLatestGitSnapshot: %v", err) + } + if got.HeadCommit != "completed-after-resume" { + t.Errorf("expected completed-after-resume, got %q — stale archive row outranks resumed execution", got.HeadCommit) + } + }) + + t.Run("batch query", func(t *testing.T) { + got, err := repo.GetLatestGitSnapshotsBySessionIDs(ctx, []string{sessionID}) + if err != nil { + t.Fatalf("GetLatestGitSnapshotsBySessionIDs: %v", err) + } + if snap := got[sessionID]; snap == nil { + t.Fatal("expected a snapshot for session, got none") + } else if snap.HeadCommit != "completed-after-resume" { + t.Errorf("expected completed-after-resume, got %q — batch query served stale archive row", snap.HeadCommit) + } + }) + }) + + t.Run("live_monitor row after unarchive wins over older archive row", func(t *testing.T) { + repo, cleanup := createTestSQLiteRepo(t) + defer cleanup() + + const taskID = "task-resume-live" + const sessionID = "session-resume-live" + seedTaskAndSession(t, ctx, repo, taskID, sessionID) + + if err := repo.ArchiveTask(ctx, taskID); err != nil { + t.Fatalf("archive task: %v", err) + } + archive := &models.GitSnapshot{ + SessionID: sessionID, + SnapshotType: models.SnapshotTypeArchive, + Branch: "feature", + HeadCommit: "archive-head", + } + if err := repo.CreateGitSnapshot(ctx, archive); err != nil { + t.Fatalf("create archive: %v", err) + } + + if _, err := repo.UnarchiveTask(ctx, taskID); err != nil { + t.Fatalf("unarchive task: %v", err) + } + live := &models.GitSnapshot{ + SessionID: sessionID, + Branch: "feature", + HeadCommit: "live-after-resume", + } + if err := repo.UpsertLatestLiveGitSnapshot(ctx, live); err != nil { + t.Fatalf("upsert live: %v", err) + } + + got, err := repo.GetLatestGitSnapshot(ctx, sessionID) + if err != nil { + t.Fatalf("GetLatestGitSnapshot: %v", err) + } + if got.HeadCommit != "live-after-resume" { + t.Errorf("expected live-after-resume, got %q — stale archive row outranks resumed live status", got.HeadCommit) + } + + // The batch selector has a separate ROW_NUMBER() expression; it must + // agree for the rank-2 live-monitor vs rank-3 stale-archive case too. + gotBatch, err := repo.GetLatestGitSnapshotsBySessionIDs(ctx, []string{sessionID}) + if err != nil { + t.Fatalf("GetLatestGitSnapshotsBySessionIDs: %v", err) + } + if snap := gotBatch[sessionID]; snap == nil { + t.Fatal("expected a snapshot for session, got none") + } else if snap.HeadCommit != "live-after-resume" { + t.Errorf("expected live-after-resume in batch query, got %q — batch served stale archive row", snap.HeadCommit) + } + }) + + t.Run("archive still wins while the task remains archived", func(t *testing.T) { + repo, cleanup := createTestSQLiteRepo(t) + defer cleanup() + + const taskID = "task-still-archived" + const sessionID = "session-still-archived" + seedTaskAndSession(t, ctx, repo, taskID, sessionID) + + if err := repo.ArchiveTask(ctx, taskID); err != nil { + t.Fatalf("archive task: %v", err) + } + archive := &models.GitSnapshot{ + SessionID: sessionID, + SnapshotType: models.SnapshotTypeArchive, + Branch: "feature", + HeadCommit: "archive-head", + } + if err := repo.CreateGitSnapshot(ctx, archive); err != nil { + t.Fatalf("create archive: %v", err) + } + // A turn completed before the archive; the archive must still win. + completed := &models.GitSnapshot{ + SessionID: sessionID, + Branch: "feature", + HeadCommit: "completed-before-archive", + TriggeredBy: "agent_completed", + } + if err := repo.CreateGitSnapshot(ctx, completed); err != nil { + t.Fatalf("create agent_completed: %v", err) + } + + got, err := repo.GetLatestGitSnapshot(ctx, sessionID) + if err != nil { + t.Fatalf("GetLatestGitSnapshot: %v", err) + } + if got.HeadCommit != "archive-head" { + t.Errorf("expected archive-head, got %q — archive lost while task is still archived", got.HeadCommit) + } + }) + + t.Run("current live row beats pre-archive completion row", func(t *testing.T) { + repo, cleanup := createTestSQLiteRepo(t) + defer cleanup() + + const taskID = "task-resume-generation" + const sessionID = "session-resume-generation" + seedTaskAndSession(t, ctx, repo, taskID, sessionID) + base := time.Date(2026, 8, 20, 12, 0, 0, 0, time.UTC) + + if err := repo.CreateGitSnapshot(ctx, &models.GitSnapshot{ + SessionID: sessionID, + HeadCommit: "pre-archive-completed", + TriggeredBy: "agent_completed", + CreatedAt: base, + }); err != nil { + t.Fatalf("create pre-archive completion: %v", err) + } + if err := repo.ArchiveTask(ctx, taskID); err != nil { + t.Fatalf("archive task: %v", err) + } + if err := repo.CreateGitSnapshot(ctx, &models.GitSnapshot{ + SessionID: sessionID, + SnapshotType: models.SnapshotTypeArchive, + HeadCommit: "archive-head", + CreatedAt: base.Add(time.Minute), + }); err != nil { + t.Fatalf("create archive: %v", err) + } + if _, err := repo.UnarchiveTask(ctx, taskID); err != nil { + t.Fatalf("unarchive task: %v", err) + } + if err := repo.UpsertLatestLiveGitSnapshot(ctx, &models.GitSnapshot{ + SessionID: sessionID, + HeadCommit: "live-after-resume", + CreatedAt: base.Add(2 * time.Minute), + }); err != nil { + t.Fatalf("create resumed live snapshot: %v", err) + } + + got, err := repo.GetLatestGitSnapshot(ctx, sessionID) + if err != nil { + t.Fatalf("GetLatestGitSnapshot: %v", err) + } + if got.HeadCommit != "live-after-resume" { + t.Errorf("head commit = %q, want current live row instead of pre-archive completion", got.HeadCommit) + } + + gotBatch, err := repo.GetLatestGitSnapshotsBySessionIDs(ctx, []string{sessionID}) + if err != nil { + t.Fatalf("GetLatestGitSnapshotsBySessionIDs: %v", err) + } + if gotBatch[sessionID] == nil || gotBatch[sessionID].HeadCommit != "live-after-resume" { + t.Errorf("batch head commit = %v, want current live row", gotBatch[sessionID]) + } + }) +} + +// TestGetLatestGitSnapshot_TieBreakerIsDeterministic guards the equal-rank +// ordering: Postgres stores created_at at microsecond precision, so distinct +// writes (e.g. two archive rows from re-archive flows) can collapse to the +// same timestamp. Both selectors must break the tie identically via the ID +// column so the selected row is never database-plan dependent. +func TestGetLatestGitSnapshot_TieBreakerIsDeterministic(t *testing.T) { + ctx := context.Background() + repo, cleanup := createTestSQLiteRepo(t) + defer cleanup() + + const taskID = "task-tie" + const sessionID = "session-tie" + seedTaskAndSession(t, ctx, repo, taskID, sessionID) + if err := repo.ArchiveTask(ctx, taskID); err != nil { + t.Fatalf("archive task: %v", err) + } + + at := time.Date(2026, 3, 4, 5, 6, 7, 123456000, time.UTC) + for _, snap := range []*models.GitSnapshot{ + {ID: "tie-aaa", SessionID: sessionID, SnapshotType: models.SnapshotTypeArchive, + Branch: "feature", HeadCommit: "older-id", CreatedAt: at}, + {ID: "tie-bbb", SessionID: sessionID, SnapshotType: models.SnapshotTypeArchive, + Branch: "feature", HeadCommit: "newer-id", CreatedAt: at}, + } { + if err := repo.CreateGitSnapshot(ctx, snap); err != nil { + t.Fatalf("create snapshot %s: %v", snap.ID, err) + } + } + + // Both rows tie on created_at and rank; the id DESC tie-breaker picks + // tie-bbb deterministically in both selectors. + t.Run("single query", func(t *testing.T) { + got, err := repo.GetLatestGitSnapshot(ctx, sessionID) + if err != nil { + t.Fatalf("GetLatestGitSnapshot: %v", err) + } + if got.ID != "tie-bbb" { + t.Errorf("expected tie-bbb (id DESC tie-break), got %q", got.ID) + } + }) + t.Run("batch query", func(t *testing.T) { + got, err := repo.GetLatestGitSnapshotsBySessionIDs(ctx, []string{sessionID}) + if err != nil { + t.Fatalf("GetLatestGitSnapshotsBySessionIDs: %v", err) + } + if snap := got[sessionID]; snap == nil || snap.ID != "tie-bbb" { + t.Errorf("expected tie-bbb in batch, got %+v", got[sessionID]) + } + }) +} + func TestDeleteLiveMonitorSnapshots(t *testing.T) { ctx := context.Background() repo, cleanup := createTestSQLiteRepo(t) diff --git a/apps/backend/internal/task/repository/sqlite/git_snapshots.go b/apps/backend/internal/task/repository/sqlite/git_snapshots.go index eaac48121d..5c36fee591 100644 --- a/apps/backend/internal/task/repository/sqlite/git_snapshots.go +++ b/apps/backend/internal/task/repository/sqlite/git_snapshots.go @@ -17,6 +17,52 @@ import ( // UpsertLatestLiveGitSnapshot so we don't disturb archive/completion snapshots. const TriggeredByLiveMonitor = "live_monitor" +// snapshotRankExpr is the ORDER BY ranking shared by GetLatestGitSnapshot and +// GetLatestGitSnapshotsBySessionIDs. Both selectors must pick the same row for +// a session, so the expression lives in one place: +// +// - rank 0: an archive snapshot that is still authoritative — either its +// owning task is still archived (terminal cumulative diff), or the task +// was unarchived but not yet resumed and no non-archive snapshot newer +// than this archive row exists yet (a reload in that window must not +// regress to pre-archive state). +// - rank 1: agent_completed snapshots from the current execution generation. +// - rank 2: live_monitor snapshots (periodic polls; may race completion). +// - rank 3: pre-archive completions and archive snapshots that are stale +// (task unarchived AND a newer non-archive snapshot exists). +// +// Ties within a rank break by created_at DESC, then id DESC so the selection +// is deterministic even when Postgres stores multiple writes at the same +// microsecond timestamp. +const snapshotRankExpr = ` + CASE + WHEN snapshot_type = 'archive' AND ( + EXISTS ( + SELECT 1 FROM task_sessions ts + JOIN tasks t ON t.id = ts.task_id + WHERE ts.id = task_session_git_snapshots.session_id + AND t.archived_at IS NOT NULL + ) + OR NOT EXISTS ( + SELECT 1 FROM task_session_git_snapshots newer + WHERE newer.session_id = task_session_git_snapshots.session_id + AND newer.snapshot_type <> 'archive' + AND newer.created_at > task_session_git_snapshots.created_at + ) + ) THEN 0 + WHEN triggered_by = 'agent_completed' AND EXISTS ( + SELECT 1 FROM task_session_git_snapshots archive + WHERE archive.session_id = task_session_git_snapshots.session_id + AND archive.snapshot_type = 'archive' + AND archive.created_at > task_session_git_snapshots.created_at + ) THEN 3 + WHEN triggered_by = 'agent_completed' THEN 1 + WHEN snapshot_type = 'archive' THEN 3 + ELSE 2 + END, + created_at DESC, + id DESC` + // UpsertLatestLiveGitSnapshot keeps at most one cached "live monitor" snapshot // per session by deleting any previous live row and inserting the new one in a // single transaction. This is the cache that backs the sidebar diff badge for @@ -66,6 +112,10 @@ func (r *Repository) UpsertLatestLiveGitSnapshot(ctx context.Context, snapshot * return tx.Commit() } +// serializeSnapshotJSON marshals a snapshot's Files and Metadata maps into +// their JSON-text column forms. Nil maps serialize to "{}" (the sentinel +// that scans back to a nil map), preserving the row shape for both SQLite +// and Postgres. func serializeSnapshotJSON(snapshot *models.GitSnapshot) (string, string, error) { filesJSON := "{}" if snapshot.Files != nil { @@ -162,10 +212,11 @@ func (r *Repository) getGitSnapshotByOrder(ctx context.Context, sessionID, order } // GetLatestGitSnapshot retrieves the best git snapshot for a session. -// Prefers agent_completed snapshots (captured at exact completion time) over -// live_monitor snapshots (periodic polls that may contain stale data if the -// poll raced with agent completion). Falls back to most-recent-by-time when -// no agent_completed snapshot exists. +// An archive snapshot (terminal cumulative diff captured at archive time) is +// authoritative while its owning task is still archived, and also while the +// task is unarchived but not yet resumed (no newer non-archive snapshot +// exists yet); once a newer agent_completed / live_monitor row describes the +// current execution, it wins over the stale archive row. // Returns sql.ErrNoRows if no snapshot is found. func (r *Repository) GetLatestGitSnapshot(ctx context.Context, sessionID string) (*models.GitSnapshot, error) { query := ` @@ -173,9 +224,7 @@ func (r *Repository) GetLatestGitSnapshot(ctx context.Context, sessionID string) ahead, behind, files, triggered_by, metadata, created_at FROM task_session_git_snapshots WHERE session_id = ? - ORDER BY - CASE WHEN triggered_by = 'agent_completed' THEN 0 ELSE 1 END, - created_at DESC + ORDER BY` + snapshotRankExpr + ` LIMIT 1 ` return scanGitSnapshot(r.ro.QueryRowContext(ctx, r.ro.Rebind(query), sessionID)) @@ -183,7 +232,7 @@ func (r *Repository) GetLatestGitSnapshot(ctx context.Context, sessionID string) // GetLatestGitSnapshotsBySessionIDs loads one authoritative snapshot per // session in one query per placeholder-sized chunk. It mirrors -// GetLatestGitSnapshot's agent_completed preference without introducing an +// GetLatestGitSnapshot's ranking (snapshotRankExpr) without introducing an // N+1 read when task-list summaries repair historical rows. func (r *Repository) GetLatestGitSnapshotsBySessionIDs( ctx context.Context, @@ -203,8 +252,7 @@ func (r *Repository) GetLatestGitSnapshotsBySessionIDs( ahead, behind, files, triggered_by, metadata, created_at, ROW_NUMBER() OVER ( PARTITION BY session_id - ORDER BY CASE WHEN triggered_by = 'agent_completed' THEN 0 ELSE 1 END, - created_at DESC + ORDER BY` + snapshotRankExpr + ` ) AS row_number FROM task_session_git_snapshots WHERE session_id IN (` + placeholders + `) @@ -239,6 +287,9 @@ type gitSnapshotScanner interface { Scan(dest ...interface{}) error } +// scanGitSnapshot scans one task_session_git_snapshots row (all 13 columns) +// into a GitSnapshot, decoding the JSON-text files/metadata columns. The "{}" +// sentinel scans back to nil maps so empty rows round-trip cleanly. func scanGitSnapshot(scanner gitSnapshotScanner) (*models.GitSnapshot, error) { snapshot := &models.GitSnapshot{} var snapshotType string diff --git a/apps/backend/internal/task/repository/sqlite/git_snapshots_postgres_test.go b/apps/backend/internal/task/repository/sqlite/git_snapshots_postgres_test.go index a42c673e67..4945d3053a 100644 --- a/apps/backend/internal/task/repository/sqlite/git_snapshots_postgres_test.go +++ b/apps/backend/internal/task/repository/sqlite/git_snapshots_postgres_test.go @@ -34,6 +34,20 @@ func TestPostgresGitSnapshotLifecycle(t *testing.T) { } } + // Archive the task so the archive snapshot's conditional rank-0 branch + // (`snapshot_type='archive' AND task archived`) is the one being tested — + // with the task unarchived the row would rank as a stale archive instead. + // Set archived_at directly rather than calling ArchiveTask: that method + // also purges the messagequeue in the same transaction, and on Postgres a + // missing queue table aborts the tx so the later Commit() fails with + // "commit unexpectedly resulted in rollback". The snapshot ordering only + // needs the archived_at flag. + if _, err := repo.db.Exec(repo.db.Rebind( + `UPDATE tasks SET archived_at = ?, updated_at = ? WHERE id = ?`), + time.Now().UTC(), time.Now().UTC(), "task-git-pg"); err != nil { + t.Fatalf("archive task: %v", err) + } + base := time.Date(2026, 3, 4, 5, 6, 7, 123456000, time.UTC) want := fullGitSnapshot("session-git-pg-a", base) if err := repo.CreateGitSnapshot(ctx, want); err != nil { @@ -45,8 +59,8 @@ func TestPostgresGitSnapshotLifecycle(t *testing.T) { } assertGitSnapshotEqual(t, got, want) - // A newer live_monitor row must lose to the older agent_completed row in - // both the single-row read and the window-function batch read. + // A newer live_monitor row must lose to the older archive row (task still + // archived) in both the single-row read and the window-function batch read. if err := repo.CreateGitSnapshot(ctx, &models.GitSnapshot{ ID: "snap-pg-live", SessionID: "session-git-pg-a", SnapshotType: models.SnapshotTypeStatusUpdate, Branch: "main", @@ -67,7 +81,7 @@ func TestPostgresGitSnapshotLifecycle(t *testing.T) { t.Fatalf("GetLatestGitSnapshot: %v", err) } if latest.ID != "snapshot-full" { - t.Errorf("GetLatestGitSnapshot = %q, want the agent_completed snapshot-full", latest.ID) + t.Errorf("GetLatestGitSnapshot = %q, want the archive snapshot-full", latest.ID) } batch, err := repo.GetLatestGitSnapshotsBySessionIDs(ctx, []string{"session-git-pg-a", "session-git-pg-b", "session-git-pg-missing"}) @@ -86,6 +100,36 @@ func TestPostgresGitSnapshotLifecycle(t *testing.T) { } assertJSONMapEqual(t, "batch[a].Files", batch["session-git-pg-a"].Files, want.Files) + // Unarchive + resume: a newer agent_completed row must now outrank the + // stale archive row in both selectors (round-3 lifecycle fix parity). + if _, err := repo.db.Exec(repo.db.Rebind( + `UPDATE tasks SET archived_at = NULL, updated_at = ? WHERE id = ?`), + time.Now().UTC(), "task-git-pg"); err != nil { + t.Fatalf("unarchive task: %v", err) + } + if err := repo.CreateGitSnapshot(ctx, &models.GitSnapshot{ + ID: "snap-pg-resumed", SessionID: "session-git-pg-a", + SnapshotType: models.SnapshotTypeStatusUpdate, Branch: "main", + TriggeredBy: "agent_completed", HeadCommit: "resumed-head", CreatedAt: base.Add(3 * time.Hour), + }); err != nil { + t.Fatalf("CreateGitSnapshot(resumed): %v", err) + } + latestResumed, err := repo.GetLatestGitSnapshot(ctx, "session-git-pg-a") + if err != nil { + t.Fatalf("GetLatestGitSnapshot(resumed): %v", err) + } + if latestResumed.ID != "snap-pg-resumed" { + t.Errorf("after unarchive GetLatestGitSnapshot = %q, want the resumed agent_completed row", latestResumed.ID) + } + batchResumed, err := repo.GetLatestGitSnapshotsBySessionIDs(ctx, []string{"session-git-pg-a"}) + if err != nil { + t.Fatalf("GetLatestGitSnapshotsBySessionIDs(resumed): %v", err) + } + if batchResumed["session-git-pg-a"].ID != "snap-pg-resumed" { + t.Errorf("after unarchive batch[a] = %q, want the resumed agent_completed row", + batchResumed["session-git-pg-a"].ID) + } + // The single-live-row upsert transaction. if err := repo.UpsertLatestLiveGitSnapshot(ctx, &models.GitSnapshot{ ID: "snap-pg-live-2", SessionID: "session-git-pg-a", Branch: "main", @@ -100,16 +144,16 @@ func TestPostgresGitSnapshotLifecycle(t *testing.T) { t.Errorf("live_monitor rows = %d, want exactly 1", liveRows) } if got := countRows(t, repo, - `SELECT COUNT(1) FROM task_session_git_snapshots WHERE session_id = ?`, "session-git-pg-a"); got != 2 { - t.Errorf("total rows = %d, want 2 (archive snapshot + one live row)", got) + `SELECT COUNT(1) FROM task_session_git_snapshots WHERE session_id = ?`, "session-git-pg-a"); got != 3 { + t.Errorf("total rows = %d, want 3 (archive + live + resumed agent_completed)", got) } if err := repo.DeleteLiveMonitorSnapshots(ctx, "session-git-pg-a"); err != nil { t.Fatalf("DeleteLiveMonitorSnapshots: %v", err) } if got := countRows(t, repo, - `SELECT COUNT(1) FROM task_session_git_snapshots WHERE session_id = ?`, "session-git-pg-a"); got != 1 { - t.Errorf("rows after live delete = %d, want 1", got) + `SELECT COUNT(1) FROM task_session_git_snapshots WHERE session_id = ?`, "session-git-pg-a"); got != 2 { + t.Errorf("rows after live delete = %d, want 2 (archive + resumed agent_completed)", got) } if _, err := repo.GetLatestGitSnapshot(ctx, "session-git-pg-missing"); !errors.Is(err, sql.ErrNoRows) { diff --git a/apps/backend/internal/task/repository/sqlite/git_snapshots_test.go b/apps/backend/internal/task/repository/sqlite/git_snapshots_test.go index 56a9274f7a..7541b52a3c 100644 --- a/apps/backend/internal/task/repository/sqlite/git_snapshots_test.go +++ b/apps/backend/internal/task/repository/sqlite/git_snapshots_test.go @@ -64,7 +64,11 @@ func fullGitSnapshot(sessionID string, createdAt time.Time) *models.GitSnapshot "additions": float64(12), }, }, - TriggeredBy: "agent_completed", + // Production archive rows (captureArchiveDiff) carry no trigger; the + // trigger field must stay empty so ordering tests exercise the + // conditional `archive AND task archived` rank, not a + // triggered_by=agent_completed rank. + TriggeredBy: "", Metadata: map[string]interface{}{ "reason": "task archived", "attempt": float64(2), @@ -152,6 +156,15 @@ func TestCreateGitSnapshotRoundTripsEveryField(t *testing.T) { t.Fatalf("CreateGitSnapshot: %v", err) } + // Archive the task so GetLatestGitSnapshot selects the archive row via + // its rank-0 branch (snapshot_type='archive' AND task archived) rather + // than merely being the only row — the fixture's intent is the + // authoritative-archive read, and the row is only the sole row here by + // construction. (Claude review note on PR #2851.) + if err := repo.ArchiveTask(ctx, "task-snap-full"); err != nil { + t.Fatalf("ArchiveTask: %v", err) + } + got, err := repo.GetLatestGitSnapshot(ctx, "session-snap-full") if err != nil { t.Fatalf("GetLatestGitSnapshot: %v", err) diff --git a/apps/backend/internal/task/service/service_status_summary_rebuild.go b/apps/backend/internal/task/service/service_status_summary_rebuild.go index 5b895436ba..ad88c97c6e 100644 --- a/apps/backend/internal/task/service/service_status_summary_rebuild.go +++ b/apps/backend/internal/task/service/service_status_summary_rebuild.go @@ -613,6 +613,9 @@ func changedFilesFromSnapshot(snapshot *models.GitSnapshot) int { if snapshot == nil { return 0 } + if _, ok := snapshot.Metadata["changed_files"]; ok { + return nonNegative(snapshot.Metadata, "changed_files") + } count := 0 for _, key := range []string{"modified", "added", "deleted", "untracked", "renamed"} { count += collectionLength(snapshot.Metadata[key]) diff --git a/apps/web/components/task/changes-panel-tree.test.tsx b/apps/web/components/task/changes-panel-tree.test.tsx index 2570f8743f..60dc7b8ad5 100644 --- a/apps/web/components/task/changes-panel-tree.test.tsx +++ b/apps/web/components/task/changes-panel-tree.test.tsx @@ -105,6 +105,16 @@ describe("ChangesTree", () => { expect(isSelected).toHaveBeenCalledWith(FOO_TS); }); + it("skips a file entry without a path instead of crashing the route", () => { + // Regression: archived-task DB snapshots can replay cumulative-diff + // entries lacking a `path` field; the tree used to crash with + // "Cannot read properties of undefined (reading 'split')". + const pathless = { ...file(FOO_TS), path: undefined as unknown as string }; + render(); + expect(screen.getAllByTestId("file-row")).toHaveLength(1); + expect(screen.getByTestId("file-row").textContent).toBe(BAR_TS); + }); + it("indents root files beneath a repository header", () => { render( { expect(result.files?.["vendor/lib\u0000src/lib.ts"]?.repository_name).toBe("vendor/lib"); }); }); + +describe("legacy composite file replay", () => { + it("repairs pathless composite files before the Review dialog reads them", () => { + const result = buildReviewGitStatusFiles( + status({ + "frontend\u0000src/app.ts": { status: "modified", staged: false } as FileInfo, + }), + [], + 2, + ); + + expect(result.files?.["frontend\u0000src/app.ts"]).toMatchObject({ + path: "src/app.ts", + repository_name: "frontend", + }); + }); + + it("does not duplicate a composite key when a named status replays it", () => { + const result = buildReviewGitStatusFiles( + undefined, + [ + { + repository_name: "frontend", + status: status({ + "frontend\u0000src/app.ts": { status: "modified", staged: false } as FileInfo, + }), + }, + ], + 2, + ); + + expect(Object.keys(result.files ?? {})).toEqual(["frontend\u0000src/app.ts"]); + }); +}); diff --git a/apps/web/components/task/use-review-dialog.ts b/apps/web/components/task/use-review-dialog.ts index 672a6f01ae..79c87803c4 100644 --- a/apps/web/components/task/use-review-dialog.ts +++ b/apps/web/components/task/use-review-dialog.ts @@ -23,6 +23,7 @@ import { generateUUID } from "@/lib/utils"; import { useToast } from "@/components/toast-provider"; import type { DiffComment } from "@/lib/diff/types"; import type { FileInfo, GitStatusEntry } from "@/lib/state/slices/session-runtime/types"; +import { normalizeGitStatusFiles } from "@/lib/state/slices/session-runtime/git-status-normalizer"; import { t } from "@/lib/i18n"; type ReviewGitStatusFiles = { @@ -36,7 +37,8 @@ function buildMultiRepoReviewFiles( const files: Record = {}; for (const { repository_name, status } of statusByRepo) { if (!status?.files) continue; - for (const [path, file] of Object.entries(status.files)) { + for (const [keyPath, file] of Object.entries(normalizeGitStatusFiles(status.files) ?? {})) { + const path = file.path || keyPath; const key = reviewFileKey({ path, repository_name }); files[key] = { ...file, @@ -63,11 +65,12 @@ export function buildReviewGitStatusFiles( ); if (!isMultiRepo) { const sourceStatus = reviewGitStatus?.files ? reviewGitStatus : named[0]?.status; - const files = sourceStatus?.files + const sourceFiles = normalizeGitStatusFiles(sourceStatus?.files); + const files = sourceFiles ? Object.fromEntries( - Object.entries(sourceStatus.files).map(([path, file]) => [ + Object.entries(sourceFiles).map(([path, file]) => [ path, - { ...file, is_submodule: file.is_submodule ?? sourceStatus.is_submodule }, + { ...file, is_submodule: file.is_submodule ?? sourceStatus?.is_submodule }, ]), ) : null; @@ -78,7 +81,7 @@ export function buildReviewGitStatusFiles( } if (named.length === 0) { return { - files: reviewGitStatus?.files ?? null, + files: normalizeGitStatusFiles(reviewGitStatus?.files) ?? null, isMultiRepo: true, }; } diff --git a/apps/web/hooks/domains/session/use-session-git-grouping.test.ts b/apps/web/hooks/domains/session/use-session-git-grouping.test.ts index 23770ff48a..d7bb0f6098 100644 --- a/apps/web/hooks/domains/session/use-session-git-grouping.test.ts +++ b/apps/web/hooks/domains/session/use-session-git-grouping.test.ts @@ -1,5 +1,75 @@ import { describe, it, expect } from "vitest"; -import { groupPathsByRepoName, repositoryScopesForMutation } from "./use-session-git"; +import { + aggregateFilesAcrossRepos, + groupPathsByRepoName, + repositoryScopesForMutation, +} from "./use-session-git"; + +describe("aggregateFilesAcrossRepos", () => { + it("stamps path from the map key when a status entry lacks its own path", () => { + // Archived-task DB snapshots can replay cumulative-diff entries whose + // older shape only sat under the key (no `path` field). The changes tree + // splits on file.path, so the fallback must not surface undefined. + const out = aggregateFilesAcrossRepos([], { + files: { + "apps/a.go": { path: "apps/a.go", status: "modified", staged: false }, + "readme.md": { status: "modified", staged: false } as never, + }, + } as never); + expect(out.map((f) => f.path).sort()).toEqual(["apps/a.go", "readme.md"]); + }); + + it("stamps path per repo in multi-repo statuses lacking it", () => { + const out = aggregateFilesAcrossRepos( + [ + { + repository_name: "frontend", + status: { + files: { "app.tsx": { status: "modified", staged: false } as never }, + }, + } as never, + ], + undefined, + ); + expect(out).toEqual([ + { status: "modified", staged: false, path: "app.tsx", repository_name: "frontend" }, + ]); + }); + + it("preserves existing paths and falls back to empty list without status", () => { + expect(aggregateFilesAcrossRepos([], undefined)).toEqual([]); + }); + + it("restores path and repository from composite keys", () => { + // Multi-repo cumulative payloads key files as `\x00`. If such + // an entry ever lacks the stamped `path`, the backfill must split the key + // (like splitReviewFileKey) instead of stamping the NUL composite into + // FileInfo.path — the changes tree splits on '/', so a composite path + // would surface as one bogus filename and break per-repo grouping. + const out = aggregateFilesAcrossRepos([], { + files: { + "repoA\u0000lib/util.ts": { status: "added", staged: false } as never, + }, + } as never); + expect(out).toEqual([ + { status: "added", staged: false, path: "lib/util.ts", repository_name: "repoA" }, + ]); + }); + + it("keeps same relative paths distinct across repositories", () => { + const out = aggregateFilesAcrossRepos([], { + files: { + "repoA\u0000README.md": { status: "modified", staged: false } as never, + "repoB\u0000README.md": { status: "modified", staged: false } as never, + }, + } as never); + + expect(out).toEqual([ + { status: "modified", staged: false, path: "README.md", repository_name: "repoA" }, + { status: "modified", staged: false, path: "README.md", repository_name: "repoB" }, + ]); + }); +}); describe("groupPathsByRepoName", () => { it("splits paths by repository_name, preserving insertion order per bucket", () => { diff --git a/apps/web/hooks/domains/session/use-session-git.ts b/apps/web/hooks/domains/session/use-session-git.ts index dc44995cfa..293385fad1 100644 --- a/apps/web/hooks/domains/session/use-session-git.ts +++ b/apps/web/hooks/domains/session/use-session-git.ts @@ -23,6 +23,7 @@ import { import { useMultiRepoSummary } from "./use-session-git-summary"; import { deriveSessionGitValues } from "./use-session-git-derived"; import { useScopedStageOperations } from "./use-scoped-stage-operations"; +import { normalizeGitStatusFiles } from "@/lib/state/slices/session-runtime/git-status-normalizer"; /** * Per-repo result emitted by frontend-side fan-outs (commit, push, pull, @@ -209,8 +210,16 @@ export function repositoryScopesForMutation( * Builds the SessionGit's flat file list. For multi-repo workspaces it * stamps each FileInfo with its repository_name so consumers can group; * for single-repo it returns the legacy single-status files unchanged. + * + * Each entry is stamped with `path` from the map key when the payload entry + * omits it. Git-status payloads always carry `path`, but the DB-snapshot + * fallback can replay archived cumulative-diff entries whose older shape only + * sat under the key (no `path` field) — the changes tree splits on + * `file.path` and crashes on `undefined`, so a missing path must never reach + * consumers. Multi-repo cumulative keys are `\x00` composites; + * the normalizer restores both the path and repository scope from that key. */ -function aggregateFilesAcrossRepos( +export function aggregateFilesAcrossRepos( statusByRepo: ReturnType, gitStatus: ReturnType, ): FileInfo[] { @@ -218,13 +227,13 @@ function aggregateFilesAcrossRepos( const out: FileInfo[] = []; for (const { repository_name, status } of statusByRepo) { if (!status?.files) continue; - for (const f of Object.values(status.files)) { - out.push(repository_name ? { ...f, repository_name } : f); + for (const file of Object.values(normalizeGitStatusFiles(status.files) ?? {})) { + out.push(repository_name ? { ...file, repository_name } : file); } } return out; } - return gitStatus?.files ? Object.values(gitStatus.files) : []; + return Object.values(normalizeGitStatusFiles(gitStatus?.files) ?? {}); } type StageDispatchArgs = { diff --git a/apps/web/lib/state/slices/session-runtime/git-status-normalizer.test.ts b/apps/web/lib/state/slices/session-runtime/git-status-normalizer.test.ts new file mode 100644 index 0000000000..4cd517313d --- /dev/null +++ b/apps/web/lib/state/slices/session-runtime/git-status-normalizer.test.ts @@ -0,0 +1,56 @@ +import { describe, expect, it } from "vitest"; +import { normalizeGitStatusEntry, normalizeGitStatusFiles } from "./git-status-normalizer"; +import type { FileInfo, GitStatusEntry } from "./types"; + +function file(overrides: Partial = {}): FileInfo { + return { path: "src/app.ts", status: "modified", staged: false, ...overrides }; +} + +describe("normalizeGitStatusFiles", () => { + it("fills a missing path from a legacy map key", () => { + const files = { "src/app.ts": { status: "modified", staged: false } as FileInfo }; + + expect(normalizeGitStatusFiles(files)).toEqual({ + "src/app.ts": file(), + }); + }); + + it("fills both path and repository from a composite map key", () => { + const files = { + "frontend\u0000src/app.ts": { status: "modified", staged: false } as FileInfo, + "backend\u0000src/app.ts": { status: "added", staged: false } as FileInfo, + }; + + expect(normalizeGitStatusFiles(files)).toEqual({ + "frontend\u0000src/app.ts": { + path: "src/app.ts", + repository_name: "frontend", + status: "modified", + staged: false, + }, + "backend\u0000src/app.ts": { + path: "src/app.ts", + repository_name: "backend", + status: "added", + staged: false, + }, + }); + }); + + it("preserves explicit values, including an explicit root repository", () => { + const files = { + "frontend\u0000src/app.ts": file({ path: "renamed.ts", repository_name: "other" }), + "\u0000README.md": file({ path: "README.md", repository_name: "" }), + }; + + expect(normalizeGitStatusFiles(files)).toBe(files); + }); +}); + +describe("normalizeGitStatusEntry", () => { + it("returns the same entry when all file records already use the current shape", () => { + const entry = { files: { "src/app.ts": file() } } as unknown as GitStatusEntry; + + expect(normalizeGitStatusEntry(entry)).toBe(entry); + }); +}); diff --git a/apps/web/lib/state/slices/session-runtime/git-status-normalizer.ts b/apps/web/lib/state/slices/session-runtime/git-status-normalizer.ts new file mode 100644 index 0000000000..ded8810b4f --- /dev/null +++ b/apps/web/lib/state/slices/session-runtime/git-status-normalizer.ts @@ -0,0 +1,52 @@ +import type { FileInfo, GitStatusEntry } from "./types"; + +const FILE_KEY_SEPARATOR = "\u0000"; + +type ParsedFileKey = { + path: string; + repositoryName?: string; +}; + +function parseFileKey(key: string): ParsedFileKey { + const separator = key.indexOf(FILE_KEY_SEPARATOR); + if (separator < 0) return { path: key }; + return { + repositoryName: key.slice(0, separator), + path: key.slice(separator + 1), + }; +} + +/** + * Repairs file records from older Git-status snapshots. + * + * New records carry `path` and `repository_name` on each value. Older + * cumulative-diff records stored those values only in the map key. + */ +export function normalizeGitStatusFiles( + files: Record | undefined, +): Record | undefined { + if (!files) return files; + + let normalized: Record | undefined; + for (const [key, file] of Object.entries(files)) { + const parsed = parseFileKey(key); + const path = typeof file.path === "string" && file.path.length > 0 ? file.path : parsed.path; + const repositoryName = file.repository_name ?? parsed.repositoryName; + if (path === file.path && repositoryName === file.repository_name) continue; + + normalized ??= { ...files }; + normalized[key] = { + ...file, + path, + ...(repositoryName === undefined ? {} : { repository_name: repositoryName }), + }; + } + return normalized ?? files; +} + +/** Normalize the file contract before a Git-status entry enters the store. */ +export function normalizeGitStatusEntry(gitStatus: GitStatusEntry): GitStatusEntry { + const files = normalizeGitStatusFiles(gitStatus.files); + if (files === gitStatus.files) return gitStatus; + return { ...gitStatus, files: files ?? {} }; +} diff --git a/apps/web/lib/state/slices/session-runtime/session-runtime-slice.ts b/apps/web/lib/state/slices/session-runtime/session-runtime-slice.ts index 5cc5c98c41..034b2bbff5 100644 --- a/apps/web/lib/state/slices/session-runtime/session-runtime-slice.ts +++ b/apps/web/lib/state/slices/session-runtime/session-runtime-slice.ts @@ -7,6 +7,7 @@ import type { FileInfo, } from "./types"; import { createDebugLogger, isDebug } from "@/lib/debug/log"; +import { normalizeGitStatusEntry } from "./git-status-normalizer"; const debugGit = createDebugLogger("git-status:store"); @@ -546,7 +547,7 @@ export const createSessionRuntimeSlice: StateCreator< setGitStatus: (sessionId, gitStatus) => { let changed = false; set((draft) => { - changed = applyGitStatus(draft, sessionId, gitStatus); + changed = applyGitStatus(draft, sessionId, normalizeGitStatusEntry(gitStatus)); }); return changed; }, diff --git a/apps/web/lib/state/slices/session-runtime/set-git-status-return.test.ts b/apps/web/lib/state/slices/session-runtime/set-git-status-return.test.ts index 35721bb482..ab64def10e 100644 --- a/apps/web/lib/state/slices/session-runtime/set-git-status-return.test.ts +++ b/apps/web/lib/state/slices/session-runtime/set-git-status-return.test.ts @@ -70,6 +70,26 @@ describe("setGitStatus change reporting (single deep compare)", () => { expect(store.getState().setGitStatus(SESSION, status({ is_submodule: true }))).toBe(true); }); + it("normalizes legacy composite file keys before storing the status", () => { + store.getState().setGitStatus( + SESSION, + status({ + files: { + "frontend\u0000src/app.ts": { status: "modified", staged: false } as never, + }, + }), + ); + + expect(store.getState().gitStatus.byEnvironmentId[SESSION]?.files).toEqual({ + "frontend\u0000src/app.ts": { + path: "src/app.ts", + repository_name: "frontend", + status: "modified", + staged: false, + }, + }); + }); + it("returns false when the submodule marker is unchanged", () => { store.getState().setGitStatus(SESSION, status({ is_submodule: true }));