fix: fall back to local git watcher for chat diff drawer (#24512)

The Ctrl+D diff drawer in `coder exp agents` only rendered PR-backed diffs returned by `/api/experimental/chats/{id}/diff`. Local working tree changes in a chat's workspace returned an empty diff, so the drawer showed "No diff contents" with no file summary. Centralise diff loading behind a single `fetchChatDiffContents` helper that first hits `/diff`, then falls back to the chat git watcher WebSocket (`/stream/git`) when the remote diff is empty. Aggregate the agent's `WorkspaceAgentRepoChanges` into a `ChatDiffContents` value so the drawer can derive the file summary and styled body from the local unified diff. Missing workspaces, missing agents, and watcher timeouts are treated as graceful fallbacks that render the empty-diff placeholder instead of a hard error. > Mux is opening this PR on Mike's behalf.
2026-09-24 15:04:27 +08:00 · 2026-04-22 18:08:02 +02:00
parent c23abc691f
commit 7904bed947
12 changed files with 1914 additions and 101 deletions
@@ -555,43 +555,317 @@ func TestExpAgentsRender(t *testing.T) {
 		branch := "feature/chat-ui"
 		prURL := "https://example.com/pulls/123"
 		for _, tt := range []struct {
-			name    string
-			diff    codersdk.ChatDiffContents
-			changes []codersdk.ChatGitChange
-			assert  func(t *testing.T, output string)
+			name   string
+			diff   codersdk.ChatDiffContents
+			assert func(t *testing.T, output string)
 		}{
 			{name: "ShowsMetadataWhenPresent", diff: codersdk.ChatDiffContents{Branch: &branch, PullRequestURL: &prURL}, assert: func(t *testing.T, output string) {
 				require.Contains(t, output, "Branch: feature/chat-ui")
 				require.Contains(t, output, "PR: https://example.com/pulls/123")
 			}},
-			{name: "ShowsDiffContent", diff: codersdk.ChatDiffContents{Diff: "diff --git a/a.txt b/a.txt\n+added line"}, changes: []codersdk.ChatGitChange{{FilePath: "a.txt", ChangeType: "modified"}}, assert: func(t *testing.T, output string) {
+			{name: "ShowsDiffContent", diff: codersdk.ChatDiffContents{Diff: "diff --git a/a.txt b/a.txt\n--- a/a.txt\n+++ b/a.txt\n@@ -1 +1 @@\n+added line"}, assert: func(t *testing.T, output string) {
+				require.Contains(t, output, "1 file changed:")
+				require.Contains(t, output, "modified a.txt (+1)")
 				require.Contains(t, output, "diff --git a/a.txt b/a.txt")
 				require.Contains(t, output, "+added line")
 			}},
-			{name: "ShowsPlaceholderForEmptyDiff", assert: func(t *testing.T, output string) { require.Contains(t, output, "No diff contents.") }},
+			{name: "ShowsPlaceholderForEmptyDiff", assert: func(t *testing.T, output string) {
+				require.Contains(t, output, "No diff contents.")
+				require.Contains(t, output, "No changes detected.")
+			}},
+			{name: "ShowsFallbackForUnparsableNonEmptyDiff", diff: codersdk.ChatDiffContents{Diff: "Total diff too large to show. Size: 12MB. Showing branch and remote only."}, assert: func(t *testing.T, output string) {
+				// When agent/agentgit substitutes a placeholder for
+				// an oversized diff, the text is non-empty but not in
+				// `diff --git` format. renderChatDiffSummary should
+				// report "Changes present but could not be summarized."
+				// instead of claiming no changes were detected.
+				require.Contains(t, output, "Changes present but could not be summarized.")
+				require.NotContains(t, output, "No changes detected.")
+			}},
+			{name: "FlagsPartiallyUnparsableMultiRepoDiff", diff: codersdk.ChatDiffContents{Diff: "diff --git a/a.txt b/a.txt\n--- a/a.txt\n+++ b/a.txt\n@@ -1 +1 @@\n+added line\nTotal diff too large to show. Size: 12 MiB. Showing branch and remote only."}, assert: func(t *testing.T, output string) {
+				// Multi-repo aggregates can legitimately interleave
+				// real `diff --git` chunks from small repos with
+				// agent/agentgit's oversize placeholder for repos
+				// whose UnifiedDiff exceeded maxTotalDiffSize.
+				// renderChatDiffSummary must both count the real
+				// chunks and flag the omitted oversized repo, so
+				// the user is not misled into thinking the files
+				// listed are the whole changeset.
+				require.Contains(t, output, "1 file changed:")
+				require.Contains(t, output, "modified a.txt")
+				require.Contains(t, output, "some repositories omitted")
+			}},
 		} {
 			tt := tt
 			t.Run(tt.name, func(t *testing.T) {
 				t.Parallel()
 				var output string
-				require.NotPanics(t, func() { output = plainText(renderDiffDrawer(styles, tt.diff, tt.changes, 90, 20)) })
+				require.NotPanics(t, func() {
+					output = plainText(renderDiffDrawer(styles, tt.diff, renderChatDiffSummary(tt.diff), "", 90, 20))
+				})
 				tt.assert(t, output)
 			})
 		}
 	})
+	t.Run("ParseChatGitChangesFromUnifiedDiff", func(t *testing.T) {
+		t.Parallel()
+
+		diff := strings.Join([]string{
+			"diff --git a/a.txt b/a.txt",
+			"--- a/a.txt",
+			"+++ b/a.txt",
+			"@@ -1 +1 @@",
+			"-old",
+			"+new",
+			"diff --git a/new.txt b/new.txt",
+			"new file mode 100644",
+			"--- /dev/null",
+			"+++ b/new.txt",
+			"@@ -0,0 +1 @@",
+			"+hello",
+			"diff --git a/old.txt b/old.txt",
+			"deleted file mode 100644",
+			"--- a/old.txt",
+			"+++ /dev/null",
+			"@@ -1 +0,0 @@",
+			"-bye",
+			"diff --git a/old-name.txt b/new-name.txt",
+			"similarity index 100%",
+			"rename from old-name.txt",
+			"rename to new-name.txt",
+		}, "\n")
+
+		changes := parseChatGitChangesFromUnifiedDiff(codersdk.ChatDiffContents{Diff: diff})
+		require.Len(t, changes, 4)
+		require.Equal(t, "a.txt", changes[0].FilePath)
+		require.Equal(t, "modified", changes[0].ChangeType)
+		require.NotNil(t, changes[0].DiffSummary)
+		require.Equal(t, "+1 -1", *changes[0].DiffSummary)
+		require.Equal(t, "new.txt", changes[1].FilePath)
+		require.Equal(t, "added", changes[1].ChangeType)
+		require.NotNil(t, changes[1].DiffSummary)
+		require.Equal(t, "+1", *changes[1].DiffSummary)
+		require.Equal(t, "old.txt", changes[2].FilePath)
+		require.Equal(t, "deleted", changes[2].ChangeType)
+		require.NotNil(t, changes[2].DiffSummary)
+		require.Equal(t, "-1", *changes[2].DiffSummary)
+		require.Equal(t, "new-name.txt", changes[3].FilePath)
+		require.Equal(t, "renamed", changes[3].ChangeType)
+		require.NotNil(t, changes[3].OldPath)
+		require.Equal(t, "old-name.txt", *changes[3].OldPath)
+		require.Nil(t, changes[3].DiffSummary)
+	})
+
+	t.Run("ParseChatGitChangesFromUnifiedDiffPathsWithSpaces", func(t *testing.T) {
+		t.Parallel()
+
+		// Git does not quote paths that only contain spaces, so the
+		// `diff --git` header is ambiguous without help from the body.
+		// Verify that modifications, binary or mode-only diffs, and
+		// renames all resolve to the correct paths and change types.
+		diff := strings.Join([]string{
+			"diff --git a/foo bar.txt b/foo bar.txt",
+			"--- a/foo bar.txt",
+			"+++ b/foo bar.txt",
+			"@@ -1 +1 @@",
+			"-old",
+			"+new",
+			"diff --git a/foo bar.bin b/foo bar.bin",
+			"index 0f49c4a..9100462 100644",
+			"Binary files a/foo bar.bin and b/foo bar.bin differ",
+			"diff --git a/new empty.txt b/new empty.txt",
+			"new file mode 100644",
+			"index 0000000..e69de29",
+			"diff --git a/old name.txt b/new name.txt",
+			"similarity index 100%",
+			"rename from old name.txt",
+			"rename to new name.txt",
+		}, "\n")
+
+		changes := parseChatGitChangesFromUnifiedDiff(codersdk.ChatDiffContents{Diff: diff})
+		require.Len(t, changes, 4)
+
+		// The buggy parser used to split the unquoted header on any
+		// whitespace, producing truncated paths and marking simple edits
+		// as renames. Verify that each change now reports the full path
+		// and the correct change type.
+		require.Equal(t, "foo bar.txt", changes[0].FilePath)
+		require.Equal(t, "modified", changes[0].ChangeType)
+
+		require.Equal(t, "foo bar.bin", changes[1].FilePath)
+		require.Equal(t, "modified", changes[1].ChangeType)
+
+		require.Equal(t, "new empty.txt", changes[2].FilePath)
+		require.Equal(t, "added", changes[2].ChangeType)
+
+		require.Equal(t, "new name.txt", changes[3].FilePath)
+		require.Equal(t, "renamed", changes[3].ChangeType)
+		require.NotNil(t, changes[3].OldPath)
+		require.Equal(t, "old name.txt", *changes[3].OldPath)
+	})
+
+	t.Run("ParseChatGitChangesFromUnifiedDiffQuotedPaths", func(t *testing.T) {
+		t.Parallel()
+
+		// Git C-quotes paths when they contain bytes above 0x7f (with
+		// the default core.quotepath setting) or control characters.
+		diff := strings.Join([]string{
+			`diff --git "a/f\303\266\303\266bar.txt" "b/f\303\266\303\266bar.txt"`,
+			`--- "a/f\303\266\303\266bar.txt"`,
+			`+++ "b/f\303\266\303\266bar.txt"`,
+			"@@ -1 +1 @@",
+			"-old",
+			"+new",
+		}, "\n")
+
+		changes := parseChatGitChangesFromUnifiedDiff(codersdk.ChatDiffContents{Diff: diff})
+		require.Len(t, changes, 1)
+		require.Equal(t, "fööbar.txt", changes[0].FilePath)
+		require.Equal(t, "modified", changes[0].ChangeType)
+	})
+
+	t.Run("ParseChatGitChangesFromUnifiedDiffQuotedRename", func(t *testing.T) {
+		t.Parallel()
+
+		// Git C-quotes `rename from`/`rename to` paths when they contain
+		// non-ASCII bytes (like `ä`). The parser should decode them so
+		// the diff summary shows a readable file name rather than the
+		// raw quoted octal escape.
+		diff := strings.Join([]string{
+			`diff --git "a/b\303\244r old.txt" "b/b\303\244r new.txt"`,
+			"similarity index 100%",
+			`rename from "b\303\244r old.txt"`,
+			`rename to "b\303\244r new.txt"`,
+		}, "\n")
+
+		changes := parseChatGitChangesFromUnifiedDiff(codersdk.ChatDiffContents{Diff: diff})
+		require.Len(t, changes, 1)
+		require.Equal(t, "renamed", changes[0].ChangeType)
+		require.Equal(t, "bär new.txt", changes[0].FilePath)
+		require.NotNil(t, changes[0].OldPath)
+		require.Equal(t, "bär old.txt", *changes[0].OldPath)
+	})
+
+	t.Run("ParseChatGitChangesFromUnifiedDiffRenameWithLiteralAPrefix", func(t *testing.T) {
+		t.Parallel()
+
+		// rename from/rename to paths are repository-relative and never
+		// carry the a/ or b/ prefix, so real directories named a/ must
+		// survive parsing intact.
+		diff := strings.Join([]string{
+			"diff --git a/a/foo.txt b/a/bar.txt",
+			"similarity index 100%",
+			"rename from a/foo.txt",
+			"rename to a/bar.txt",
+		}, "\n")
+
+		changes := parseChatGitChangesFromUnifiedDiff(codersdk.ChatDiffContents{Diff: diff})
+		require.Len(t, changes, 1)
+		require.Equal(t, "renamed", changes[0].ChangeType)
+		require.Equal(t, "a/bar.txt", changes[0].FilePath)
+		require.NotNil(t, changes[0].OldPath)
+		require.Equal(t, "a/foo.txt", *changes[0].OldPath)
+	})
+
+	t.Run("ParseChatGitChangesFromUnifiedDiffIgnoresHunkContentLookalikes", func(t *testing.T) {
+		t.Parallel()
+
+		// Added/removed diff lines can legitimately start with `+++ ` or
+		// `--- ` (the content happens to begin with `++ ` or `-- `). The
+		// parser must treat those as content after the first `@@` hunk
+		// header instead of overwriting the already-resolved FilePath
+		// and change counts.
+		diff := strings.Join([]string{
+			"diff --git a/a.txt b/a.txt",
+			"--- a/a.txt",
+			"+++ b/a.txt",
+			"@@ -1,2 +1,2 @@",
+			"--- not a header",
+			"+++ also not a header",
+			"-left",
+			"+right",
+		}, "\n")
+
+		changes := parseChatGitChangesFromUnifiedDiff(codersdk.ChatDiffContents{Diff: diff})
+		require.Len(t, changes, 1)
+		require.Equal(t, "a.txt", changes[0].FilePath)
+		require.Equal(t, "modified", changes[0].ChangeType)
+		require.NotNil(t, changes[0].DiffSummary)
+		// Inside the hunk, both "--- not a header" and "-left" are
+		// deletion lines, and both "+++ also not a header" and "+right"
+		// are addition lines. The header "--- a/a.txt" and "+++ b/a.txt"
+		// lines before @@ are not counted.
+		require.Equal(t, "+2 -2", *changes[0].DiffSummary)
+	})
+
+	t.Run("ParseUnifiedDiffHeaderPaths", func(t *testing.T) {
+		t.Parallel()
+
+		for _, tc := range []struct {
+			name    string
+			line    string
+			oldPath string
+			newPath string
+			ok      bool
+		}{
+			{
+				name:    "Simple",
+				line:    "diff --git a/foo.txt b/foo.txt",
+				oldPath: "foo.txt", newPath: "foo.txt", ok: true,
+			},
+			{
+				name:    "Rename",
+				line:    "diff --git a/old.txt b/new.txt",
+				oldPath: "old.txt", newPath: "new.txt", ok: true,
+			},
+			{
+				name:    "SpacesNonRename",
+				line:    "diff --git a/foo bar.txt b/foo bar.txt",
+				oldPath: "foo bar.txt", newPath: "foo bar.txt", ok: true,
+			},
+			{
+				name: "SpacesRenameIsAmbiguous",
+				line: "diff --git a/old name.txt b/new name.txt",
+				ok:   false,
+			},
+			{
+				name:    "QuotedTabEscape",
+				line:    `diff --git "a/a\tb.txt" "b/a\tb.txt"`,
+				oldPath: "a\tb.txt", newPath: "a\tb.txt", ok: true,
+			},
+			{
+				name:    "NestedBPrefix",
+				line:    "diff --git a/b/foo.txt b/b/foo.txt",
+				oldPath: "b/foo.txt", newPath: "b/foo.txt", ok: true,
+			},
+			{
+				name: "Empty",
+				line: "diff --git ",
+				ok:   false,
+			},
+			{
+				name: "MissingAPrefix",
+				line: "diff --git foo.txt bar.txt",
+				ok:   false,
+			},
+		} {
+			t.Run(tc.name, func(t *testing.T) {
+				t.Parallel()
+				gotOld, gotNew, gotOK := parseUnifiedDiffHeaderPaths(tc.line)
+				require.Equal(t, tc.ok, gotOK)
+				if gotOK {
+					require.Equal(t, tc.oldPath, gotOld)
+					require.Equal(t, tc.newPath, gotNew)
+				}
+			})
+		}
+	})
+
 	t.Run("RenderDiffDrawerSanitizesUntrustedContent", func(t *testing.T) {
 		t.Parallel()

-		rawOutput := renderDiffDrawer(
-			styles,
-			codersdk.ChatDiffContents{Diff: "diff --git a/a.txt b/a.txt\n+safe\x1b]52;c;clipboard\x07line"},
-			[]codersdk.ChatGitChange{{
-				FilePath:   "a.txt\x1b]52;c;clipboard\x07",
-				ChangeType: "modified",
-			}},
-			90,
-			20,
-		)
+		diff := codersdk.ChatDiffContents{Diff: "diff --git a/a.txt b/a.txt\n+safe\x1b]52;c;clipboard\x07line"}
+		rawOutput := renderDiffDrawer(styles, diff, renderChatDiffSummary(diff), "", 90, 20)
 		output := plainText(rawOutput)

 		require.Contains(t, output, "diff --git a/a.txt b/a.txt")