From fd258ed08d60b7f1798d9bb65962f34ef80c63f7 Mon Sep 17 00:00:00 2001 From: Caleb Piekstra Date: Wed, 21 Oct 2026 11:07:40 -0400 Subject: [PATCH] fix(pipeline): keep diff paths that contain spaces Git leaves a path with spaces unquoted in the "diff --git a/ b/" header, and the parser split that header at the first space. Added and modified files recovered their new path from the "+++ b/" line, but a deleted file ("+++ /dev/null") kept the fragment after the last space. Deleting "data/2024-06-08 - a.json" and "data/2024-06-08 - b.json" gave both files the path "-", and selection stopped on a duplicate canonical path in the file manifest. Files with no "---"/"+++" lines (empty, binary and mode-only changes) kept both fragments, and an added file kept a truncated old path. Read an unquoted header whose two halves name the same file by splitting at its middle space. Renames keep the token parsing and take their paths from the "rename from" and "rename to" lines, which are now decoded when git C-quotes them. Fixes #678. --- internal/pipeline/diff.go | 38 ++++++++-- internal/pipeline/diff_test.go | 111 +++++++++++++++++++++++++++++ internal/pipeline/pipeline_test.go | 41 +++++++++++ 3 files changed, 186 insertions(+), 4 deletions(-) diff --git a/internal/pipeline/diff.go b/internal/pipeline/diff.go index ea6c05c8..ff0b5686 100644 --- a/internal/pipeline/diff.go +++ b/internal/pipeline/diff.go @@ -88,9 +88,9 @@ func parseUnifiedDiff(raw string) (ParsedDiff, error) { case strings.HasPrefix(trimmed, "new file mode"): current.Added = true case strings.HasPrefix(trimmed, "rename from "): - current.OldPath = strings.TrimPrefix(trimmed, "rename from ") + current.OldPath = unquoteGitPath(strings.TrimPrefix(trimmed, "rename from ")) case strings.HasPrefix(trimmed, "rename to "): - current.Path = strings.TrimPrefix(trimmed, "rename to ") + current.Path = unquoteGitPath(strings.TrimPrefix(trimmed, "rename to ")) case strings.HasPrefix(trimmed, "Binary files ") || strings.HasPrefix(trimmed, "GIT binary patch"): current.Binary = true case strings.HasPrefix(trimmed, "@@ "): @@ -118,6 +118,9 @@ func splitLines(raw string) []string { func parseDiffGitPaths(line string) (string, string) { rest := strings.TrimSpace(strings.TrimPrefix(line, "diff --git ")) + if path, ok := sameUnquotedDiffPath(rest); ok { + return path, path + } oldPath, rest, ok := consumeDiffPath(rest) if !ok { return "", "" @@ -129,17 +132,44 @@ func parseDiffGitPaths(line string) (string, string) { return trimDiffPrefix(oldPath), trimDiffPrefix(newPath) } +// sameUnquotedDiffPath reads an unquoted "a/ b/" pair that names +// one file on both sides. Git quotes a path for special characters but not for +// spaces, so splitting at the first space would cut such a path short. Equal +// halves around the middle space are unambiguous; a rename falls through to +// token parsing and takes its paths from the "rename from"/"rename to" lines. +func sameUnquotedDiffPath(rest string) (string, bool) { + if strings.HasPrefix(rest, `"`) || len(rest)%2 == 0 { + return "", false + } + mid := len(rest) / 2 + if rest[mid] != ' ' { + return "", false + } + oldPath, newPath := trimDiffPrefix(rest[:mid]), trimDiffPrefix(rest[mid+1:]) + if oldPath == "" || oldPath != newPath { + return "", false + } + return oldPath, true +} + func parseHeaderPath(raw string) string { value := strings.TrimSpace(raw) if value == "/dev/null" { return "" } + return trimDiffPrefix(unquoteGitPath(value)) +} + +// unquoteGitPath decodes a path git wrote as a C-style quoted string, which it +// does for names holding control characters, quotes, backslashes, or (with the +// default core.quotePath) non-ASCII bytes. +func unquoteGitPath(value string) string { if strings.HasPrefix(value, `"`) { if unquoted, err := strconv.Unquote(value); err == nil { - value = unquoted + return unquoted } } - return trimDiffPrefix(value) + return value } func trimDiffPrefix(path string) string { diff --git a/internal/pipeline/diff_test.go b/internal/pipeline/diff_test.go index 1c48512f..7dc836fa 100644 --- a/internal/pipeline/diff_test.go +++ b/internal/pipeline/diff_test.go @@ -150,6 +150,117 @@ func TestParseUnifiedDiffQuotedPathsWithSpaces(t *testing.T) { } } +// Git leaves paths with spaces unquoted in "diff --git" headers and appends a +// tab to "---"/"+++" paths that contain a space. These shapes mirror its output. +func TestParseUnifiedDiffUnquotedPathsWithSpaces(t *testing.T) { + raw := strings.Join([]string{ + "diff --git a/data/2024-06-08 - a.json b/data/2024-06-08 - a.json", + "deleted file mode 100644", + "index 1111111..0000000", + "--- a/data/2024-06-08 - a.json\t", + "+++ /dev/null", + "@@ -1 +0,0 @@", + "-{}", + "diff --git a/data/2024-06-08 - b.json b/data/2024-06-08 - b.json", + "deleted file mode 100644", + "index 2222222..0000000", + "--- a/data/2024-06-08 - b.json\t", + "+++ /dev/null", + "@@ -1 +0,0 @@", + "-{}", + "diff --git a/data/2024-06-09 - c.json b/data/2024-06-09 - c.json", + "new file mode 100644", + "index 0000000..3333333", + "--- /dev/null", + "+++ b/data/2024-06-09 - c.json\t", + "@@ -0,0 +1 @@", + "+{}", + "diff --git a/data/empty old.txt b/data/empty old.txt", + "deleted file mode 100644", + "index e69de29..0000000", + "diff --git a/data/empty new.txt b/data/empty new.txt", + "new file mode 100644", + "index 0000000..e69de29", + "diff --git a/data/blob old.bin b/data/blob old.bin", + "deleted file mode 100644", + "index 4444444..0000000", + "Binary files a/data/blob old.bin and /dev/null differ", + "diff --git a/data/blob new.bin b/data/blob new.bin", + "new file mode 100644", + "index 0000000..5555555", + "Binary files /dev/null and b/data/blob new.bin differ", + "diff --git a/scripts/run me.sh b/scripts/run me.sh", + "old mode 100644", + "new mode 100755", + "diff --git a/docs/old name.md b/docs/new name.md", + "similarity index 100%", + "rename from docs/old name.md", + "rename to docs/new name.md", + "diff --git a/docs/moved file.md b/docs/moved file 2.md", + "similarity index 80%", + "rename from docs/moved file.md", + "rename to docs/moved file 2.md", + "index 6666666..7777777 100644", + "--- a/docs/moved file.md\t", + "+++ b/docs/moved file 2.md\t", + "@@ -1 +1 @@", + "-old", + "+new", + "", + }, "\n") + + got, err := parseUnifiedDiff(raw) + if err != nil { + t.Fatalf("parseUnifiedDiff: %v", err) + } + want := []struct { + oldPath, path string + deleted, added, binary bool + }{ + {oldPath: "data/2024-06-08 - a.json", path: "data/2024-06-08 - a.json", deleted: true}, + {oldPath: "data/2024-06-08 - b.json", path: "data/2024-06-08 - b.json", deleted: true}, + {oldPath: "data/2024-06-09 - c.json", path: "data/2024-06-09 - c.json", added: true}, + {oldPath: "data/empty old.txt", path: "data/empty old.txt", deleted: true}, + {oldPath: "data/empty new.txt", path: "data/empty new.txt", added: true}, + {oldPath: "data/blob old.bin", path: "data/blob old.bin", deleted: true, binary: true}, + {oldPath: "data/blob new.bin", path: "data/blob new.bin", added: true, binary: true}, + {oldPath: "scripts/run me.sh", path: "scripts/run me.sh"}, + {oldPath: "docs/old name.md", path: "docs/new name.md"}, + {oldPath: "docs/moved file.md", path: "docs/moved file 2.md"}, + } + if len(got.Patches) != len(want) || len(got.PlanDiff.Files) != len(want) { + t.Fatalf("patches = %d, plan files = %d, want %d", len(got.Patches), len(got.PlanDiff.Files), len(want)) + } + for i, w := range want { + patch := got.Patches[i] + if patch.OldPath != w.oldPath || patch.Path != w.path || patch.Deleted != w.deleted || patch.Added != w.added || patch.Binary != w.binary { + t.Errorf("patch %d = old %q path %q deleted %v added %v binary %v, want %+v", + i, patch.OldPath, patch.Path, patch.Deleted, patch.Added, patch.Binary, w) + } + if plan := got.PlanDiff.Files[i]; plan.OldPath != w.oldPath || plan.Path != w.path { + t.Errorf("plan file %d = old %q path %q, want old %q path %q", i, plan.OldPath, plan.Path, w.oldPath, w.path) + } + } +} + +func TestParseUnifiedDiffQuotedRenamePaths(t *testing.T) { + raw := strings.Join([]string{ + `diff --git "a/docs/caf\303\251 notes.md" "b/docs/caf\303\251 notes 2.md"`, + "similarity index 100%", + `rename from "docs/caf\303\251 notes.md"`, + `rename to "docs/caf\303\251 notes 2.md"`, + "", + }, "\n") + + got, err := parseUnifiedDiff(raw) + if err != nil { + t.Fatalf("parseUnifiedDiff: %v", err) + } + if len(got.Patches) != 1 || got.Patches[0].OldPath != "docs/café notes.md" || got.Patches[0].Path != "docs/café notes 2.md" { + t.Fatalf("patches = %#v, want decoded rename paths", got.Patches) + } +} + func TestParseUnifiedDiffRejectsBadHunkHeader(t *testing.T) { _, err := parseUnifiedDiff("diff --git a/main.go b/main.go\n@@ bad @@\n") if err == nil { diff --git a/internal/pipeline/pipeline_test.go b/internal/pipeline/pipeline_test.go index 8c277656..a99f2fc2 100644 --- a/internal/pipeline/pipeline_test.go +++ b/internal/pipeline/pipeline_test.go @@ -6492,6 +6492,47 @@ func TestSelectionOnlyAcceptsRenameSourcePath(t *testing.T) { } } +func TestSelectionOnlyAcceptsDeletedPathsWithSpaces(t *testing.T) { + ctx := context.Background() + provider, req := dryRunHarness(t) + removeRepoAgentFixture(provider) + + deleted := []string{"data/2024-06-08 - a.json", "data/2024-06-08 - b.json"} + repoDir := provider.fixtureRepoDir + gitCommandMustSucceed(t, repoDir, "checkout", "main") + for _, path := range deleted { + writeFile(t, filepath.Join(repoDir, path), "{}\n") + } + gitCommandMustSucceed(t, repoDir, "add", "data") + gitCommandMustSucceed(t, repoDir, "commit", "-m", "add dated files") + provider.pr.Base.SHA = gitCommandMustSucceed(t, repoDir, "rev-parse", "HEAD") + gitCommandMustSucceed(t, repoDir, "checkout", "-B", "feature") + gitCommandMustSucceed(t, repoDir, append([]string{"rm", "--"}, deleted...)...) + gitCommandMustSucceed(t, repoDir, "commit", "-m", "delete dated files") + provider.pr.Head.SHA = gitCommandMustSucceed(t, repoDir, "rev-parse", "HEAD") + provider.diff.Raw = gitCommandOutput(t, repoDir, "diff", provider.pr.Base.SHA, provider.pr.Head.SHA) + + adapter := &llm.FakeAdapter{NameValue: "fake-llm"} + adapter.Queue(fakeLLMResult("selection-session", `{ + "schema_version": 1, + "selected_agents": [], + "thread_actions": [], + "reasoning": "the diff only deletes files" + }`, 10, 2)) + + result, err := selectionOnlyForTest(ctx, Options{ + Provider: provider, + Adapter: adapter, + Now: fixedNow, + }, selectionRequestFromReview(req, t.TempDir())) + if err != nil { + t.Fatalf("SelectionOnly: %v", err) + } + if !reflect.DeepEqual(result.ChangedFiles, deleted) { + t.Fatalf("changed files = %#v, want %#v", result.ChangedFiles, deleted) + } +} + func TestDryRunReviewerFindingOnDeletedPathIsDecoded(t *testing.T) { ctx := context.Background() store := openPipelineStore(t)