diff --git a/cli/exp_agents_chat.go b/cli/exp_agents_chat.go index 6b60cff1ed..f3bde23257 100644 --- a/cli/exp_agents_chat.go +++ b/cli/exp_agents_chat.go @@ -304,9 +304,20 @@ type chatViewModel struct { interrupting bool diffStatus *codersdk.ChatDiffStatus - gitChanges []codersdk.ChatGitChange diffContents *codersdk.ChatDiffContents - diffErr error + // diffSummary caches the rendered "N files changed" summary + // for diffContents so renderDiffDrawer can reuse it across + // View() redraws. parseChatGitChangesFromUnifiedDiff walks the + // full (potentially 4 MiB) diff text, so recomputing it on every + // keypress or resize stalls the TUI for large diffs. + diffSummary string + // diffStyledBody caches the lipgloss-styled unified-diff body for + // diffContents. renderStyledDiffBody sanitizes, splits, and styles + // every line of the (potentially 4 MiB) diff, and styles are stable + // across redraws (setRenderer runs once at startup), so we + // invalidate on the same trigger as diffSummary. + diffStyledBody string + diffErr error modelPickerFlat []codersdk.ChatModel modelPickerCursor int @@ -471,8 +482,9 @@ func (m *chatViewModel) setChat(chat codersdk.Chat) { m.activeChatID = chat.ID m.chatStatus = chat.Status m.diffStatus = chat.DiffStatus - m.gitChanges = nil m.diffContents = nil + m.diffSummary = "" + m.diffStyledBody = "" m.diffErr = nil } @@ -1168,17 +1180,6 @@ func (m chatViewModel) Update(msg tea.Msg) (chatViewModel, tea.Cmd) { } return m, nil - case gitChangesMsg: - if !m.matchesGeneration(msg.generation) { - return m, nil - } - if msg.err != nil { - m.diffErr = msg.err - return m, nil - } - m.gitChanges = msg.changes - return m, nil - case diffContentsMsg: if !m.matchesGeneration(msg.generation) { return m, nil @@ -1189,6 +1190,13 @@ func (m chatViewModel) Update(msg tea.Msg) (chatViewModel, tea.Cmd) { } diff := msg.diff m.diffContents = &diff + // Pre-render the summary and styled body once so View() + // redraws reuse them instead of re-parsing and re-styling + // the full diff on every keypress. Styles are stable after + // setRenderer, so these caches only need to be refreshed + // when diffContents changes. + m.diffSummary = renderChatDiffSummary(diff) + m.diffStyledBody = renderStyledDiffBody(m.styles, diff.Diff) return m, nil default: diff --git a/cli/exp_agents_cmds.go b/cli/exp_agents_cmds.go index bda3540d72..eae87c8960 100644 --- a/cli/exp_agents_cmds.go +++ b/cli/exp_agents_cmds.go @@ -56,12 +56,6 @@ type ( catalog codersdk.ChatModelsResponse err error } - gitChangesMsg struct { - generation uint64 - chatID uuid.UUID - changes []codersdk.ChatGitChange - err error - } diffContentsMsg struct { generation uint64 chatID uuid.UUID diff --git a/cli/exp_agents_diff.go b/cli/exp_agents_diff.go new file mode 100644 index 0000000000..2ff5e8c96d --- /dev/null +++ b/cli/exp_agents_diff.go @@ -0,0 +1,332 @@ +package cli + +import ( + "context" + "encoding/json" + "errors" + "fmt" + "net/http" + "slices" + "strings" + "time" + + "github.com/google/uuid" + "golang.org/x/xerrors" + + "github.com/coder/coder/v2/codersdk" + "github.com/coder/websocket" +) + +const localChatDiffWatchTimeout = 5 * time.Second + +// localChatDiffReadLimit bounds the size of the Changes message the +// client is willing to receive from the chat git watcher. agentgit +// caps each repository's UnifiedDiff at ~3 MiB (maxTotalDiffSize), +// and a Changes payload can aggregate many repos plus metadata, so +// 4 MiB is too tight for realistic multi-repo worktrees. 32 MiB +// covers ~10 maxed-out repos; pathological payloads beyond that still +// fall back to the remote empty diff via errLocalDiffWatchClosed / +// shouldIgnoreLocalDiffFallbackError. +const localChatDiffReadLimit = 32 << 20 // 32 MiB + +// errLocalDiffWatchClosed is returned when the chat git watcher +// websocket closes during the Changes read loop with one of the +// known-safe close statuses: +// +// - StatusMessageTooBig: the Changes payload exceeded our local +// 32 MiB client read limit (localChatDiffReadLimit). +// - StatusGoingAway: the coderd watchChatGit proxy tore the +// client stream down. This is the status the proxy always uses +// in coderd/exp_chats.go, so it also covers the upstream 4 MiB +// read limit on agent->coderd messages (see +// workspacesdk/agentconn.go): when that limit is exceeded the +// agent closes with StatusMessageTooBig, but the proxy does not +// propagate that status and the client only ever observes +// StatusGoingAway. +// +// Both cases degrade to the remote empty diff returned by /diff: +// the local watcher is a supplementary enrichment source that +// cannot improve on the remote when its stream is cut short. Other +// close statuses (StatusInternalError, StatusProtocolError, ...) +// and non-close read errors still surface as hard errors so real +// protocol regressions are not hidden behind the fallback. +var errLocalDiffWatchClosed = xerrors.New("chat git watcher connection closed before delivering a Changes message") + +func fetchChatDiffContents( + ctx context.Context, + client *codersdk.ExperimentalClient, + chatID uuid.UUID, +) (codersdk.ChatDiffContents, error) { + remoteDiff, err := client.GetChatDiffContents(ctx, chatID) + if err != nil { + return codersdk.ChatDiffContents{}, err + } + if strings.TrimSpace(remoteDiff.Diff) != "" { + return remoteDiff, nil + } + + localDiff, localSingleRepo, err := fetchLocalChatDiffContents(ctx, client, chatID) + if err != nil { + if shouldIgnoreLocalDiffFallbackError(err) { + return remoteDiff, nil + } + return codersdk.ChatDiffContents{}, err + } + if strings.TrimSpace(localDiff.Diff) == "" { + return remoteDiff, nil + } + + // Backfill metadata from the remote diff only when the local + // watcher produced a single contributing repository. Gate this on + // the explicit single-repo signal from buildLocalChatDiffContents + // rather than on Branch/RemoteOrigin being non-nil, because a + // single contributing repo can legitimately have an empty branch + // (detached HEAD) or no origin remote and we still want remote + // fields like Provider/PullRequestURL to flow through. Multi-repo + // aggregates cannot be described by a single remote's metadata, so + // we leave them alone. + if localSingleRepo { + if localDiff.Provider == nil { + localDiff.Provider = remoteDiff.Provider + } + if localDiff.RemoteOrigin == nil { + localDiff.RemoteOrigin = remoteDiff.RemoteOrigin + } + if localDiff.Branch == nil { + localDiff.Branch = remoteDiff.Branch + } + if localDiff.PullRequestURL == nil { + localDiff.PullRequestURL = remoteDiff.PullRequestURL + } + } + return localDiff, nil +} + +// fetchLocalChatDiffContents returns the aggregated local-watcher diff +// and a singleRepo flag that indicates whether that aggregate came from +// exactly one contributing repository. The caller uses singleRepo to +// decide whether it is safe to backfill remote-only metadata onto the +// local diff. All error paths return singleRepo=false. +// +// This intentionally bypasses wsjson.NewStream and reads the websocket +// directly so we can inspect the close status: an oversized Changes +// payload must degrade to the remote empty diff via +// errLocalDiffWatchClosed + shouldIgnoreLocalDiffFallbackError, +// but wsjson.Decoder swallows the read error (logs at debug) and +// closes the channel, which would collapse that specific case into +// the same generic "connection closed" bucket as server crashes or +// decode failures. Reading directly lets us narrowly fall back only +// for read-limit violations while still surfacing real protocol +// regressions. +func fetchLocalChatDiffContents( + parentCtx context.Context, + client *codersdk.ExperimentalClient, + chatID uuid.UUID, +) (codersdk.ChatDiffContents, bool, error) { + ctx, cancel := context.WithTimeout(parentCtx, localChatDiffWatchTimeout) + defer cancel() + + conn, err := dialChatGit(ctx, client, chatID) + if err != nil { + return codersdk.ChatDiffContents{}, false, err + } + defer func() { + _ = conn.Close(websocket.StatusNormalClosure, "") + }() + conn.SetReadLimit(localChatDiffReadLimit) + + refreshPayload, err := json.Marshal(codersdk.WorkspaceAgentGitClientMessage{ + Type: codersdk.WorkspaceAgentGitClientMessageTypeRefresh, + }) + if err != nil { + return codersdk.ChatDiffContents{}, false, xerrors.Errorf("marshal git refresh: %w", err) + } + if err := conn.Write(ctx, websocket.MessageText, refreshPayload); err != nil { + return codersdk.ChatDiffContents{}, false, xerrors.Errorf("request git refresh: %w", err) + } + + for { + msgType, payload, err := conn.Read(ctx) + if err != nil { + // Context expiration gets its own wrapping so it threads + // cleanly through shouldIgnoreLocalDiffFallbackError's + // context.DeadlineExceeded case. + if ctxErr := ctx.Err(); ctxErr != nil { + return codersdk.ChatDiffContents{}, false, xerrors.Errorf("watch chat git: %w", ctxErr) + } + // A Changes payload that exceeds localChatDiffReadLimit + // causes coder/websocket to close the connection with + // StatusMessageTooBig. The coderd watchChatGit proxy + // also always closes the client with StatusGoingAway + // (see coderd/exp_chats.go), which is how we observe + // the upstream 4 MiB agent->coderd read-limit breach: + // the agent closes its own hop with StatusMessageTooBig, + // but the proxy does not propagate that status, so the + // client only ever sees StatusGoingAway. Map both onto + // the narrow sentinel so shouldIgnoreLocalDiffFallbackError + // can degrade to the remote empty diff instead of + // surfacing a hard error. Every other close status + // (StatusInternalError, StatusProtocolError, ...) and + // every non-close read error still propagates so real + // protocol regressions reach the user. + switch websocket.CloseStatus(err) { + case websocket.StatusMessageTooBig, websocket.StatusGoingAway: + return codersdk.ChatDiffContents{}, false, errLocalDiffWatchClosed + } + return codersdk.ChatDiffContents{}, false, xerrors.Errorf("read git watch: %w", err) + } + // Ignore unexpected frame types instead of erroring; the + // watcher only emits text frames today and a future binary + // heartbeat should not break the overlay. + if msgType != websocket.MessageText { + continue + } + var msg codersdk.WorkspaceAgentGitServerMessage + if err := json.Unmarshal(payload, &msg); err != nil { + return codersdk.ChatDiffContents{}, false, xerrors.Errorf("decode git watch message: %w", err) + } + switch msg.Type { + case codersdk.WorkspaceAgentGitServerMessageTypeError: + message := strings.TrimSpace(msg.Message) + if message == "" { + message = "git watch returned an unknown error" + } + return codersdk.ChatDiffContents{}, false, xerrors.New(message) + case codersdk.WorkspaceAgentGitServerMessageTypeChanges: + diff, singleRepo := buildLocalChatDiffContents(chatID, msg.Repositories) + return diff, singleRepo, nil + } + } +} + +// dialChatGit opens the chat git-watcher WebSocket. We dial the socket +// manually instead of using codersdk.Client.Dial because that helper +// closes the HTTP response body before surfacing the error, which +// prevents codersdk.ReadBodyAsError from extracting the status code and +// message that shouldIgnoreLocalDiffFallbackError needs to decide +// whether to degrade to the empty remote diff. Keep this handrolled +// path as long as the shared helper has that limitation. +func dialChatGit( + ctx context.Context, + client *codersdk.ExperimentalClient, + chatID uuid.UUID, +) (*websocket.Conn, error) { + requestURL, err := client.URL.Parse( + fmt.Sprintf("/api/experimental/chats/%s/stream/git", chatID), + ) + if err != nil { + return nil, err + } + + dialOptions := &websocket.DialOptions{ + HTTPClient: client.HTTPClient, + CompressionMode: websocket.CompressionDisabled, + } + client.SessionTokenProvider.SetDialOption(dialOptions) + + conn, resp, err := websocket.Dial(ctx, requestURL.String(), dialOptions) + if resp != nil && resp.Body != nil { + defer resp.Body.Close() + } + if err != nil { + if resp != nil { + return nil, codersdk.ReadBodyAsError(resp) + } + return nil, err + } + return conn, nil +} + +// buildLocalChatDiffContents aggregates the local watcher's +// per-repository changes into a single ChatDiffContents. The returned +// singleRepo flag is true iff the aggregated diff came from exactly +// one contributing repository (one repo with a non-empty UnifiedDiff +// that has not been removed). Callers use this flag to decide whether +// it is safe to backfill remote-only metadata onto the local diff: +// multi-repo aggregates cannot be described by a single remote's +// branch/origin/PR URL, but a single-repo aggregate can even when the +// contributing repo has an empty branch (detached HEAD) or no origin +// remote configured. +func buildLocalChatDiffContents( + chatID uuid.UUID, + repositories []codersdk.WorkspaceAgentRepoChanges, +) (codersdk.ChatDiffContents, bool) { + result := codersdk.ChatDiffContents{ChatID: chatID} + if len(repositories) == 0 { + return result, false + } + + repositories = slices.Clone(repositories) + slices.SortFunc(repositories, func(a, b codersdk.WorkspaceAgentRepoChanges) int { + return strings.Compare(a.RepoRoot, b.RepoRoot) + }) + + diffSegments := make([]string, 0, len(repositories)) + diffRepositories := make([]codersdk.WorkspaceAgentRepoChanges, 0, len(repositories)) + for _, repo := range repositories { + if repo.Removed || strings.TrimSpace(repo.UnifiedDiff) == "" { + continue + } + diffRepositories = append(diffRepositories, repo) + diffSegments = append(diffSegments, strings.TrimRight(repo.UnifiedDiff, "\n")) + } + if len(diffSegments) == 0 { + return result, false + } + + result.Diff = strings.Join(diffSegments, "\n") + singleRepo := len(diffRepositories) == 1 + if singleRepo { + if branch := strings.TrimSpace(diffRepositories[0].Branch); branch != "" { + result.Branch = &branch + } + if origin := strings.TrimSpace(diffRepositories[0].RemoteOrigin); origin != "" { + result.RemoteOrigin = &origin + } + } + return result, singleRepo +} + +func shouldIgnoreLocalDiffFallbackError(err error) bool { + if errors.Is(err, context.DeadlineExceeded) { + return true + } + // A watcher stream closed with StatusMessageTooBig or + // StatusGoingAway is a best-effort degradation point: the + // remote /diff endpoint already returns the empty placeholder + // in this case, so fall back to it instead of surfacing a hard + // error. See errLocalDiffWatchClosed for the rationale on why + // those two close statuses are safe while others still surface. + if errors.Is(err, errLocalDiffWatchClosed) { + return true + } + + sdkErr, ok := codersdk.AsError(err) + if !ok { + return false + } + + switch sdkErr.StatusCode() { + case http.StatusNotFound: + return true + case http.StatusForbidden: + // authorizeChatWorkspaceExec returns 403 when the chat owner's + // workspace permissions have been revoked. The remote diff + // endpoint (getChatDiffContents) does not re-check workspace + // permissions, so degrade to its empty response the same way + // we do for the 400 variants below. + return true + case http.StatusBadRequest: + // These correspond to the 400 responses from watchChatGit in + // coderd/exp_chats.go when the chat cannot be observed through + // a workspace agent (no workspace bound, workspace deleted, no + // agents, or an agent that is not yet connected). Each should + // fall back to the empty remote diff the same way a missing + // chat (404) does instead of surfacing a hard error. + // codersdk.IsChatGitWatchFallbackMessage keeps this list + // mechanically linked to the server-side messages. + return codersdk.IsChatGitWatchFallbackMessage(sdkErr.Message) + default: + return false + } +} diff --git a/cli/exp_agents_diff_test.go b/cli/exp_agents_diff_test.go new file mode 100644 index 0000000000..3ff1f3a4fb --- /dev/null +++ b/cli/exp_agents_diff_test.go @@ -0,0 +1,743 @@ +package cli //nolint:testpackage // Tests unexported local diff fallback helpers. + +import ( + "context" + "encoding/json" + "fmt" + "net/http" + "strings" + "testing" + + "github.com/google/uuid" + "github.com/stretchr/testify/require" + + "github.com/coder/coder/v2/codersdk" + "github.com/coder/coder/v2/testutil" + "github.com/coder/websocket" +) + +func TestFetchChatDiffContents(t *testing.T) { + t.Parallel() + + t.Run("FallsBackToLocalGitWatcher", func(t *testing.T) { + t.Parallel() + + ctx := t.Context() + chatID := uuid.New() + path := fmt.Sprintf("/api/experimental/chats/%s", chatID) + client := newTestExperimentalClient(t, http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) { + switch r.URL.Path { + case path + "/diff": + rw.Header().Set("Content-Type", "application/json") + require.NoError(t, json.NewEncoder(rw).Encode(codersdk.ChatDiffContents{ChatID: chatID})) + case path + "/stream/git": + conn, err := websocket.Accept(rw, r, nil) + require.NoError(t, err) + defer conn.Close(websocket.StatusNormalClosure, "") + + _, payload, err := conn.Read(ctx) + require.NoError(t, err) + var refresh codersdk.WorkspaceAgentGitClientMessage + require.NoError(t, json.Unmarshal(payload, &refresh)) + require.Equal(t, codersdk.WorkspaceAgentGitClientMessageTypeRefresh, refresh.Type) + + writer, err := conn.Writer(ctx, websocket.MessageText) + require.NoError(t, err) + require.NoError(t, json.NewEncoder(writer).Encode(codersdk.WorkspaceAgentGitServerMessage{ + Type: codersdk.WorkspaceAgentGitServerMessageTypeChanges, + Repositories: []codersdk.WorkspaceAgentRepoChanges{{ + RepoRoot: "/workspace/repo", + Branch: "feature/local-diff", + RemoteOrigin: "https://github.com/coder/coder.git", + UnifiedDiff: "diff --git a/a.txt b/a.txt\n--- a/a.txt\n+++ b/a.txt\n@@ -1 +1 @@\n-old\n+new\n", + }}, + })) + require.NoError(t, writer.Close()) + default: + http.NotFound(rw, r) + } + })) + + diff, err := fetchChatDiffContents(ctx, client, chatID) + require.NoError(t, err) + require.NotNil(t, diff.Branch) + require.Equal(t, "feature/local-diff", *diff.Branch) + require.NotNil(t, diff.RemoteOrigin) + require.Equal(t, "https://github.com/coder/coder.git", *diff.RemoteOrigin) + require.Contains(t, diff.Diff, "diff --git a/a.txt b/a.txt") + require.Contains(t, diff.Diff, "+new") + }) + + t.Run("IgnoresTimedOutWatcherFallbackErrors", func(t *testing.T) { + t.Parallel() + + ctx, cancel := context.WithTimeout(t.Context(), testutil.IntervalMedium) + defer cancel() + + handlerDone := make(chan struct{}) + chatID := uuid.New() + path := fmt.Sprintf("/api/experimental/chats/%s", chatID) + client := newTestExperimentalClient(t, http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) { + switch r.URL.Path { + case path + "/diff": + rw.Header().Set("Content-Type", "application/json") + require.NoError(t, json.NewEncoder(rw).Encode(codersdk.ChatDiffContents{ChatID: chatID})) + case path + "/stream/git": + defer close(handlerDone) + + conn, err := websocket.Accept(rw, r, nil) + require.NoError(t, err) + defer conn.Close(websocket.StatusNormalClosure, "") + + _, payload, err := conn.Read(r.Context()) + require.NoError(t, err) + var refresh codersdk.WorkspaceAgentGitClientMessage + require.NoError(t, json.Unmarshal(payload, &refresh)) + require.Equal(t, codersdk.WorkspaceAgentGitClientMessageTypeRefresh, refresh.Type) + + // Keep the WebSocket open until the client disconnects + // (either from fetchChatDiffContents hitting its watch + // timeout or test cleanup closing the connection) + // instead of sleeping for a fixed duration. The second + // Read blocks on the socket and unblocks with an error + // when the peer closes the connection, so this handler + // drains cleanly without time.Sleep (see WORKFLOWS.md). + _, _, _ = conn.Read(r.Context()) + default: + http.NotFound(rw, r) + } + })) + + diff, err := fetchChatDiffContents(ctx, client, chatID) + require.NoError(t, err) + require.Equal(t, chatID, diff.ChatID) + require.Empty(t, diff.Diff) + require.Eventually(t, func() bool { + select { + case <-handlerDone: + return true + default: + return false + } + }, testutil.WaitShort, testutil.IntervalFast) + }) + + t.Run("IgnoresMissingWorkspaceFallbackErrors", func(t *testing.T) { + t.Parallel() + + // Each message here matches a 400 response that watchChatGit can + // return when the chat cannot be observed through the workspace + // agent. fetchChatDiffContents should swallow the error and fall + // back to the empty remote diff instead of surfacing a hard + // error in the TUI. Drive the subtests from the shared codersdk + // constants so a server-side rewording automatically flows + // through the test matrix. + for _, message := range []string{ + codersdk.ChatGitWatchNoWorkspaceMessage, + codersdk.ChatGitWatchWorkspaceNotFoundMessage, + codersdk.ChatGitWatchWorkspaceNoAgentsMessage, + codersdk.ChatGitWatchAgentStateMessage(codersdk.WorkspaceAgentConnecting), + } { + t.Run(message, func(t *testing.T) { + t.Parallel() + + ctx := t.Context() + chatID := uuid.New() + path := fmt.Sprintf("/api/experimental/chats/%s", chatID) + client := newTestExperimentalClient(t, http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) { + switch r.URL.Path { + case path + "/diff": + rw.Header().Set("Content-Type", "application/json") + require.NoError(t, json.NewEncoder(rw).Encode(codersdk.ChatDiffContents{ChatID: chatID})) + case path + "/stream/git": + rw.Header().Set("Content-Type", "application/json") + rw.WriteHeader(http.StatusBadRequest) + require.NoError(t, json.NewEncoder(rw).Encode(codersdk.Response{Message: message})) + default: + http.NotFound(rw, r) + } + })) + + diff, err := fetchChatDiffContents(ctx, client, chatID) + require.NoError(t, err) + require.Equal(t, chatID, diff.ChatID) + require.Empty(t, diff.Diff) + }) + } + }) + + t.Run("IgnoresForbiddenWatcherFallbackErrors", func(t *testing.T) { + t.Parallel() + + // authorizeChatWorkspaceExec in coderd/exp_chats.go returns 403 + // when the chat owner's workspace exec permission is revoked. + // The remote /diff endpoint does not re-check workspace + // permissions, so fetchChatDiffContents must swallow the 403 + // and fall back to the empty remote diff just like it does for + // the 400 variants above. Without this subtest, removing the + // `case http.StatusForbidden` branch in + // shouldIgnoreLocalDiffFallbackError would silently regress. + ctx := t.Context() + chatID := uuid.New() + path := fmt.Sprintf("/api/experimental/chats/%s", chatID) + client := newTestExperimentalClient(t, http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) { + switch r.URL.Path { + case path + "/diff": + rw.Header().Set("Content-Type", "application/json") + require.NoError(t, json.NewEncoder(rw).Encode(codersdk.ChatDiffContents{ChatID: chatID})) + case path + "/stream/git": + rw.Header().Set("Content-Type", "application/json") + rw.WriteHeader(http.StatusForbidden) + require.NoError(t, json.NewEncoder(rw).Encode(codersdk.Response{Message: "forbidden"})) + default: + http.NotFound(rw, r) + } + })) + + diff, err := fetchChatDiffContents(ctx, client, chatID) + require.NoError(t, err) + require.Equal(t, chatID, diff.ChatID) + require.Empty(t, diff.Diff) + }) + + t.Run("IgnoresNotFoundWatcherFallbackErrors", func(t *testing.T) { + t.Parallel() + + // watchChatGit in coderd/exp_chats.go returns 404 for missing + // chats (httpapi.ResourceNotFound). The remote /diff endpoint + // already handles the missing-chat case on its own, so + // fetchChatDiffContents must swallow the 404 from /stream/git + // and fall back to whatever the remote diff returned, the + // same way it does for the 400 and 403 variants above. + // Without this subtest, removing the `case http.StatusNotFound` + // branch in shouldIgnoreLocalDiffFallbackError would silently + // regress (mirrors the 403 coverage added for DEREM-16). + ctx := t.Context() + chatID := uuid.New() + path := fmt.Sprintf("/api/experimental/chats/%s", chatID) + client := newTestExperimentalClient(t, http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) { + switch r.URL.Path { + case path + "/diff": + rw.Header().Set("Content-Type", "application/json") + require.NoError(t, json.NewEncoder(rw).Encode(codersdk.ChatDiffContents{ChatID: chatID})) + case path + "/stream/git": + rw.Header().Set("Content-Type", "application/json") + rw.WriteHeader(http.StatusNotFound) + require.NoError(t, json.NewEncoder(rw).Encode(codersdk.Response{Message: "not found"})) + default: + http.NotFound(rw, r) + } + })) + + diff, err := fetchChatDiffContents(ctx, client, chatID) + require.NoError(t, err) + require.Equal(t, chatID, diff.ChatID) + require.Empty(t, diff.Diff) + }) + + t.Run("BackfillsRemoteMetadataWhenLocalDiffIsSingleRepo", func(t *testing.T) { + t.Parallel() + + // The scenario this PR was written for: a chat has remote + // metadata (provider, pull-request URL, etc.) but the server + // returns an empty Diff because the remote watcher has not + // observed changes yet. The CLI fetches the local watcher + // diff and must carry the remote metadata forward so the + // Diff overlay still shows the PR URL / origin. + ctx := t.Context() + chatID := uuid.New() + path := fmt.Sprintf("/api/experimental/chats/%s", chatID) + remoteBranch := "feature/remote-branch" + remoteOrigin := "https://github.com/coder/coder.git" + remotePR := "https://github.com/coder/coder/pull/42" + remoteProvider := "github" + client := newTestExperimentalClient(t, http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) { + switch r.URL.Path { + case path + "/diff": + rw.Header().Set("Content-Type", "application/json") + require.NoError(t, json.NewEncoder(rw).Encode(codersdk.ChatDiffContents{ + ChatID: chatID, + Provider: &remoteProvider, + RemoteOrigin: &remoteOrigin, + Branch: &remoteBranch, + PullRequestURL: &remotePR, + })) + case path + "/stream/git": + conn, err := websocket.Accept(rw, r, nil) + require.NoError(t, err) + defer conn.Close(websocket.StatusNormalClosure, "") + + _, payload, err := conn.Read(ctx) + require.NoError(t, err) + var refresh codersdk.WorkspaceAgentGitClientMessage + require.NoError(t, json.Unmarshal(payload, &refresh)) + require.Equal(t, codersdk.WorkspaceAgentGitClientMessageTypeRefresh, refresh.Type) + + writer, err := conn.Writer(ctx, websocket.MessageText) + require.NoError(t, err) + // Return exactly one repo so buildLocalChatDiffContents + // sets Branch/RemoteOrigin, which is the signal that + // fetchChatDiffContents uses to backfill missing + // metadata from the remote response (Provider, PR URL) + // without overwriting fields the local watcher + // already populated. + require.NoError(t, json.NewEncoder(writer).Encode(codersdk.WorkspaceAgentGitServerMessage{ + Type: codersdk.WorkspaceAgentGitServerMessageTypeChanges, + Repositories: []codersdk.WorkspaceAgentRepoChanges{{ + RepoRoot: "/workspace/repo", + Branch: "feature/local-branch", + RemoteOrigin: "https://github.com/coder/local.git", + UnifiedDiff: "diff --git a/a.txt b/a.txt\n--- a/a.txt\n+++ b/a.txt\n@@ -1 +1 @@\n-old\n+new\n", + }}, + })) + require.NoError(t, writer.Close()) + default: + http.NotFound(rw, r) + } + })) + + diff, err := fetchChatDiffContents(ctx, client, chatID) + require.NoError(t, err) + + // The aggregated diff comes from the local watcher. + require.Contains(t, diff.Diff, "diff --git a/a.txt b/a.txt") + require.Contains(t, diff.Diff, "+new") + + // Branch and RemoteOrigin were populated by the single-repo + // local watcher result, so they must NOT be overwritten by + // the remote response. + require.NotNil(t, diff.Branch) + require.Equal(t, "feature/local-branch", *diff.Branch) + require.NotNil(t, diff.RemoteOrigin) + require.Equal(t, "https://github.com/coder/local.git", *diff.RemoteOrigin) + + // Provider and PullRequestURL were nil on the local diff, + // so they must be backfilled from the remote metadata. + require.NotNil(t, diff.Provider) + require.Equal(t, remoteProvider, *diff.Provider) + require.NotNil(t, diff.PullRequestURL) + require.Equal(t, remotePR, *diff.PullRequestURL) + }) + + t.Run("BackfillsRemoteMetadataWhenSingleRepoHasBlankBranchAndOrigin", func(t *testing.T) { + t.Parallel() + + // A single contributing repo can legitimately be in detached + // HEAD with no origin remote configured: buildLocalChatDiffContents + // then leaves both Branch and RemoteOrigin nil even though + // exactly one repository produced the aggregated diff. Before + // the singleRepo flag was introduced, the gate on + // `localDiff.Branch != nil || localDiff.RemoteOrigin != nil` + // skipped the backfill in this case and the drawer silently + // lost remote Provider/PullRequestURL. fetchChatDiffContents + // must now use the explicit singleRepo signal so remote + // metadata still flows through, and must also populate the + // nil Branch/RemoteOrigin from the remote response to keep the + // drawer display consistent with all other single-repo diffs. + ctx := t.Context() + chatID := uuid.New() + path := fmt.Sprintf("/api/experimental/chats/%s", chatID) + remoteBranch := "feature/remote-branch" + remoteOrigin := "https://github.com/coder/coder.git" + remotePR := "https://github.com/coder/coder/pull/42" + remoteProvider := "github" + client := newTestExperimentalClient(t, http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) { + switch r.URL.Path { + case path + "/diff": + rw.Header().Set("Content-Type", "application/json") + require.NoError(t, json.NewEncoder(rw).Encode(codersdk.ChatDiffContents{ + ChatID: chatID, + Provider: &remoteProvider, + RemoteOrigin: &remoteOrigin, + Branch: &remoteBranch, + PullRequestURL: &remotePR, + })) + case path + "/stream/git": + conn, err := websocket.Accept(rw, r, nil) + require.NoError(t, err) + defer conn.Close(websocket.StatusNormalClosure, "") + + _, payload, err := conn.Read(ctx) + require.NoError(t, err) + var refresh codersdk.WorkspaceAgentGitClientMessage + require.NoError(t, json.Unmarshal(payload, &refresh)) + require.Equal(t, codersdk.WorkspaceAgentGitClientMessageTypeRefresh, refresh.Type) + + writer, err := conn.Writer(ctx, websocket.MessageText) + require.NoError(t, err) + // Exactly one repository contributes, but both + // Branch and RemoteOrigin are empty (detached HEAD, + // no origin remote). buildLocalChatDiffContents + // still flags this as singleRepo=true, so the + // backfill must run and populate every nil field + // from the remote response. + require.NoError(t, json.NewEncoder(writer).Encode(codersdk.WorkspaceAgentGitServerMessage{ + Type: codersdk.WorkspaceAgentGitServerMessageTypeChanges, + Repositories: []codersdk.WorkspaceAgentRepoChanges{{ + RepoRoot: "/workspace/repo", + Branch: "", + RemoteOrigin: "", + UnifiedDiff: "diff --git a/a.txt b/a.txt\n--- a/a.txt\n+++ b/a.txt\n@@ -1 +1 @@\n-old\n+new\n", + }}, + })) + require.NoError(t, writer.Close()) + default: + http.NotFound(rw, r) + } + })) + + diff, err := fetchChatDiffContents(ctx, client, chatID) + require.NoError(t, err) + + // The aggregated diff still comes from the local watcher. + require.Contains(t, diff.Diff, "diff --git a/a.txt b/a.txt") + require.Contains(t, diff.Diff, "+new") + + // Every remote-only field is backfilled because + // buildLocalChatDiffContents flagged the aggregate as + // singleRepo=true even with blank branch/origin. + require.NotNil(t, diff.Branch) + require.Equal(t, remoteBranch, *diff.Branch) + require.NotNil(t, diff.RemoteOrigin) + require.Equal(t, remoteOrigin, *diff.RemoteOrigin) + require.NotNil(t, diff.Provider) + require.Equal(t, remoteProvider, *diff.Provider) + require.NotNil(t, diff.PullRequestURL) + require.Equal(t, remotePR, *diff.PullRequestURL) + }) + + t.Run("IgnoresWatcherMessageTooBigCloses", func(t *testing.T) { + t.Parallel() + + // agentgit caps each repository's UnifiedDiff at ~3 MiB and a + // Changes payload aggregates every repo plus metadata, so a + // realistic multi-repo workspace can legitimately produce a + // payload that exceeds the client's websocket read limit. + // When that happens coder/websocket closes the connection + // with StatusMessageTooBig. fetchChatDiffContents must map + // that specific close status onto errLocalDiffWatchClosed + // and fall back to the remote empty diff rather than + // surfacing a hard error to the TUI. Without this subtest, + // removing the StatusMessageTooBig branch in + // fetchLocalChatDiffContents or the errLocalDiffWatchClosed + // branch in shouldIgnoreLocalDiffFallbackError would + // silently regress the large-multi-repo case this feature is + // meant to improve. + ctx := t.Context() + chatID := uuid.New() + path := fmt.Sprintf("/api/experimental/chats/%s", chatID) + client := newTestExperimentalClient(t, http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) { + switch r.URL.Path { + case path + "/diff": + rw.Header().Set("Content-Type", "application/json") + require.NoError(t, json.NewEncoder(rw).Encode(codersdk.ChatDiffContents{ChatID: chatID})) + case path + "/stream/git": + conn, err := websocket.Accept(rw, r, nil) + require.NoError(t, err) + // Drain the refresh before closing so the client + // surfaces the close status from its next Read, not + // an unrelated write error. + _, _, err = conn.Read(ctx) + require.NoError(t, err) + require.NoError(t, conn.Close(websocket.StatusMessageTooBig, "too big")) + default: + http.NotFound(rw, r) + } + })) + + diff, err := fetchChatDiffContents(ctx, client, chatID) + require.NoError(t, err) + require.Equal(t, chatID, diff.ChatID) + require.Empty(t, diff.Diff) + }) + + t.Run("IgnoresWatcherGoingAwayCloses", func(t *testing.T) { + t.Parallel() + + // The coderd watchChatGit proxy always closes the client + // stream with StatusGoingAway regardless of why the + // upstream agent->coderd hop failed. In particular, when + // that hop's 4 MiB read limit (workspacesdk/agentconn.go) + // is exceeded, the agent closes its end with + // StatusMessageTooBig but the proxy does not propagate + // that status, so the client only observes + // StatusGoingAway. That is the exact scenario this PR's + // 32 MiB client read limit is meant to handle, so the + // TUI must degrade to the remote empty diff for + // StatusGoingAway just like it does for + // StatusMessageTooBig. Without this subtest, narrowing + // the close-status match back to StatusMessageTooBig + // only would silently regress multi-repo worktrees whose + // aggregate Changes payload sits between the 4 MiB + // upstream limit and the 32 MiB client limit. + ctx := t.Context() + chatID := uuid.New() + path := fmt.Sprintf("/api/experimental/chats/%s", chatID) + client := newTestExperimentalClient(t, http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) { + switch r.URL.Path { + case path + "/diff": + rw.Header().Set("Content-Type", "application/json") + require.NoError(t, json.NewEncoder(rw).Encode(codersdk.ChatDiffContents{ChatID: chatID})) + case path + "/stream/git": + conn, err := websocket.Accept(rw, r, nil) + require.NoError(t, err) + _, _, err = conn.Read(ctx) + require.NoError(t, err) + require.NoError(t, conn.Close(websocket.StatusGoingAway, "proxy tear-down")) + default: + http.NotFound(rw, r) + } + })) + + diff, err := fetchChatDiffContents(ctx, client, chatID) + require.NoError(t, err) + require.Equal(t, chatID, diff.ChatID) + require.Empty(t, diff.Diff) + }) + + t.Run("SurfacesUnexpectedWatcherCloseErrors", func(t *testing.T) { + t.Parallel() + + // The StatusMessageTooBig fallback is intentionally narrow: + // a generic websocket close (for example the server + // crashing and closing with StatusInternalError) should + // surface as an error rather than silently degrading, + // because that would hide real protocol regressions behind + // the best-effort fallback. This subtest pins that + // distinction so a future attempt to blanket-ignore every + // close reason immediately breaks the test. + ctx := t.Context() + chatID := uuid.New() + path := fmt.Sprintf("/api/experimental/chats/%s", chatID) + client := newTestExperimentalClient(t, http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) { + switch r.URL.Path { + case path + "/diff": + rw.Header().Set("Content-Type", "application/json") + require.NoError(t, json.NewEncoder(rw).Encode(codersdk.ChatDiffContents{ChatID: chatID})) + case path + "/stream/git": + conn, err := websocket.Accept(rw, r, nil) + require.NoError(t, err) + _, _, err = conn.Read(ctx) + require.NoError(t, err) + require.NoError(t, conn.Close(websocket.StatusInternalError, "boom")) + default: + http.NotFound(rw, r) + } + })) + + _, err := fetchChatDiffContents(ctx, client, chatID) + require.Error(t, err) + }) + + t.Run("ReturnsRemoteDiffWithoutDialingWatcher", func(t *testing.T) { + t.Parallel() + + // When the remote /diff endpoint returns a non-empty diff the + // CLI short-circuits the WebSocket fallback. If the git stream + // handler ever fires, the test fails the request explicitly so + // an inverted condition regresses loudly. + ctx := t.Context() + chatID := uuid.New() + path := fmt.Sprintf("/api/experimental/chats/%s", chatID) + branch := "feature/remote" + prURL := "https://example.com/pr/1" + remoteDiff := codersdk.ChatDiffContents{ + ChatID: chatID, + Branch: &branch, + PullRequestURL: &prURL, + Diff: "diff --git a/remote.txt b/remote.txt\n--- a/remote.txt\n+++ b/remote.txt\n@@ -1 +1 @@\n-old\n+new\n", + } + client := newTestExperimentalClient(t, http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) { + switch r.URL.Path { + case path + "/diff": + rw.Header().Set("Content-Type", "application/json") + require.NoError(t, json.NewEncoder(rw).Encode(remoteDiff)) + case path + "/stream/git": + t.Errorf("local git watcher should not be dialed when the remote diff is non-empty") + rw.WriteHeader(http.StatusInternalServerError) + default: + http.NotFound(rw, r) + } + })) + + got, err := fetchChatDiffContents(ctx, client, chatID) + require.NoError(t, err) + require.Equal(t, chatID, got.ChatID) + require.Equal(t, remoteDiff.Diff, got.Diff) + require.NotNil(t, got.Branch) + require.Equal(t, branch, *got.Branch) + require.NotNil(t, got.PullRequestURL) + require.Equal(t, prURL, *got.PullRequestURL) + }) + + t.Run("PropagatesRemoteDiffAPIErrors", func(t *testing.T) { + t.Parallel() + + // A 500 from /diff is a hard failure that the CLI must surface + // rather than silently fall back. The local watcher must not + // be dialed when the remote endpoint returned an error. + ctx := t.Context() + chatID := uuid.New() + path := fmt.Sprintf("/api/experimental/chats/%s", chatID) + client := newTestExperimentalClient(t, http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) { + switch r.URL.Path { + case path + "/diff": + rw.Header().Set("Content-Type", "application/json") + rw.WriteHeader(http.StatusInternalServerError) + require.NoError(t, json.NewEncoder(rw).Encode(codersdk.Response{Message: "boom"})) + case path + "/stream/git": + t.Errorf("local git watcher should not be dialed when /diff errors") + rw.WriteHeader(http.StatusInternalServerError) + default: + http.NotFound(rw, r) + } + })) + + _, err := fetchChatDiffContents(ctx, client, chatID) + require.Error(t, err) + sdkErr, ok := codersdk.AsError(err) + require.True(t, ok) + require.Equal(t, http.StatusInternalServerError, sdkErr.StatusCode()) + }) + + t.Run("SurfacesNonIgnorableWatcherErrors", func(t *testing.T) { + t.Parallel() + + // A 500 from the git stream is not in the ignorable set, so + // fetchChatDiffContents must return it verbatim instead of + // silently collapsing to the empty remote diff. + ctx := t.Context() + chatID := uuid.New() + path := fmt.Sprintf("/api/experimental/chats/%s", chatID) + client := newTestExperimentalClient(t, http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) { + switch r.URL.Path { + case path + "/diff": + rw.Header().Set("Content-Type", "application/json") + require.NoError(t, json.NewEncoder(rw).Encode(codersdk.ChatDiffContents{ChatID: chatID})) + case path + "/stream/git": + rw.Header().Set("Content-Type", "application/json") + rw.WriteHeader(http.StatusInternalServerError) + require.NoError(t, json.NewEncoder(rw).Encode(codersdk.Response{Message: "internal git watcher failure"})) + default: + http.NotFound(rw, r) + } + })) + + _, err := fetchChatDiffContents(ctx, client, chatID) + require.Error(t, err) + sdkErr, ok := codersdk.AsError(err) + require.True(t, ok) + require.Equal(t, http.StatusInternalServerError, sdkErr.StatusCode()) + }) +} + +func TestBuildLocalChatDiffContents(t *testing.T) { + t.Parallel() + + t.Run("SortsMultipleReposByRepoRoot", func(t *testing.T) { + t.Parallel() + + chatID := uuid.New() + diff, singleRepo := buildLocalChatDiffContents(chatID, []codersdk.WorkspaceAgentRepoChanges{ + { + RepoRoot: "/workspace/z-repo", + UnifiedDiff: "diff --git a/z.txt b/z.txt\n+z\n", + }, + { + RepoRoot: "/workspace/a-repo", + Branch: "feature/local", + RemoteOrigin: "https://github.com/coder/coder.git", + UnifiedDiff: "diff --git a/a.txt b/a.txt\n+a\n", + }, + }) + + // Multi-repo aggregation drops the per-repo metadata because + // Branch/RemoteOrigin only make sense for a single repo. The + // singleRepo flag must be false so callers know not to + // backfill remote metadata onto a multi-repo aggregate. + require.Equal(t, chatID, diff.ChatID) + require.Contains(t, diff.Diff, "diff --git a/a.txt b/a.txt") + require.Contains(t, diff.Diff, "diff --git a/z.txt b/z.txt") + require.Less(t, strings.Index(diff.Diff, "a.txt"), strings.Index(diff.Diff, "z.txt")) + require.Nil(t, diff.Branch) + require.Nil(t, diff.RemoteOrigin) + require.False(t, singleRepo) + }) + + t.Run("ReturnsEmptyForNoRepositories", func(t *testing.T) { + t.Parallel() + + chatID := uuid.New() + // No repos: exercise the early-return in buildLocalChatDiffContents + // so the empty case is mechanically covered. singleRepo must + // be false because no repository contributed any diff. + for _, repos := range [][]codersdk.WorkspaceAgentRepoChanges{nil, {}} { + diff, singleRepo := buildLocalChatDiffContents(chatID, repos) + require.Equal(t, chatID, diff.ChatID) + require.Empty(t, diff.Diff) + require.Nil(t, diff.Branch) + require.Nil(t, diff.RemoteOrigin) + require.False(t, singleRepo) + } + }) + + t.Run("SkipsRemovedAndEmptyRepositories", func(t *testing.T) { + t.Parallel() + + chatID := uuid.New() + // Removed repos (Removed=true) and repos with whitespace-only + // UnifiedDiff must not contribute to the aggregated diff. With + // a single contributing repo, the per-repo Branch and + // RemoteOrigin should still propagate to the result and + // singleRepo must be true because only one repository + // contributed. + diff, singleRepo := buildLocalChatDiffContents(chatID, []codersdk.WorkspaceAgentRepoChanges{ + { + RepoRoot: "/workspace/removed", + Removed: true, + UnifiedDiff: "diff --git a/removed.txt b/removed.txt\n+removed\n", + }, + { + RepoRoot: "/workspace/empty", + UnifiedDiff: " \n", + }, + { + RepoRoot: "/workspace/only", + Branch: "feature/only", + RemoteOrigin: "https://github.com/coder/coder.git", + UnifiedDiff: "diff --git a/only.txt b/only.txt\n+only\n", + }, + }) + + require.Equal(t, chatID, diff.ChatID) + require.Contains(t, diff.Diff, "diff --git a/only.txt b/only.txt") + require.NotContains(t, diff.Diff, "removed.txt") + require.NotContains(t, diff.Diff, "empty") + require.NotNil(t, diff.Branch) + require.Equal(t, "feature/only", *diff.Branch) + require.NotNil(t, diff.RemoteOrigin) + require.Equal(t, "https://github.com/coder/coder.git", *diff.RemoteOrigin) + require.True(t, singleRepo) + }) + + t.Run("ReturnsEmptyWhenAllRepositoriesAreSkipped", func(t *testing.T) { + t.Parallel() + + chatID := uuid.New() + // If every repo is removed or empty, buildLocalChatDiffContents + // returns the empty remote-diff shape so the caller falls back + // to the placeholder overlay instead of rendering a diff-less + // summary. singleRepo must be false because no repository + // contributed any diff content. + diff, singleRepo := buildLocalChatDiffContents(chatID, []codersdk.WorkspaceAgentRepoChanges{ + {RepoRoot: "/workspace/removed", Removed: true, UnifiedDiff: "diff --git a/removed.txt b/removed.txt\n+removed\n"}, + {RepoRoot: "/workspace/empty"}, + }) + + require.Equal(t, chatID, diff.ChatID) + require.Empty(t, diff.Diff) + require.Nil(t, diff.Branch) + require.Nil(t, diff.RemoteOrigin) + require.False(t, singleRepo) + }) +} diff --git a/cli/exp_agents_model.go b/cli/exp_agents_model.go index eba9e6517b..75fc4cfac0 100644 --- a/cli/exp_agents_model.go +++ b/cli/exp_agents_model.go @@ -344,15 +344,13 @@ func (m *expChatsTUIModel) toggleDiffDrawerCmd() tea.Cmd { if !m.toggleOverlay(overlayDiffDrawer) { return nil } - if m.chat.gitChanges == nil || m.chat.diffContents == nil || m.chat.diffErr != nil { + if m.chat.diffContents == nil || m.chat.diffErr != nil { m.chat.diffErr = nil chatID := m.chat.chat.ID generation := m.chat.chatGeneration - return tea.Batch(apiCmd(func() ([]codersdk.ChatGitChange, error) { return m.client.GetChatGitChanges(m.ctx, chatID) }, func(changes []codersdk.ChatGitChange, err error) tea.Msg { - return gitChangesMsg{generation: generation, chatID: chatID, changes: changes, err: err} - }), apiCmd(func() (codersdk.ChatDiffContents, error) { return m.client.GetChatDiffContents(m.ctx, chatID) }, func(diff codersdk.ChatDiffContents, err error) tea.Msg { + return apiCmd(func() (codersdk.ChatDiffContents, error) { return fetchChatDiffContents(m.ctx, m.client, chatID) }, func(diff codersdk.ChatDiffContents, err error) tea.Msg { return diffContentsMsg{generation: generation, chatID: chatID, diff: diff, err: err} - })) + }) } return nil } @@ -376,7 +374,7 @@ func (m expChatsTUIModel) diffOverlayView() string { case m.chat.diffErr != nil: return m.renderOverlay("Diff", m.styles.errorText.Render(wrapPreservingNewlines(m.chat.diffErr.Error(), contentWidth(m.width, 6)))) case m.chat.diffContents != nil: - return renderDiffDrawer(m.styles, *m.chat.diffContents, m.chat.gitChanges, m.width, m.height) + return renderDiffDrawer(m.styles, *m.chat.diffContents, m.chat.diffSummary, m.chat.diffStyledBody, m.width, m.height) default: return m.renderOverlay("Diff", m.styles.dimmedText.Render("Loading diff…")) } @@ -468,7 +466,7 @@ func (m expChatsTUIModel) Update(msg tea.Msg) (tea.Model, tea.Cmd) { return m.updateChild(msg, viewChat) case chatsListedMsg: return m.updateChild(msg, viewList) - case chatOpenedMsg, chatHistoryMsg, chatStreamEventMsg, messageSentMsg, chatCreatedMsg, chatInterruptedMsg, gitChangesMsg, diffContentsMsg: + case chatOpenedMsg, chatHistoryMsg, chatStreamEventMsg, messageSentMsg, chatCreatedMsg, chatInterruptedMsg, diffContentsMsg: return m.updateChild(msg, viewChat) case modelsListedMsg: if msg.err != nil { diff --git a/cli/exp_agents_render.go b/cli/exp_agents_render.go index 8ff7fda066..b60046b6b8 100644 --- a/cli/exp_agents_render.go +++ b/cli/exp_agents_render.go @@ -310,39 +310,407 @@ func diffMetadataLines(diff codersdk.ChatDiffContents) []string { return lines } -func renderChatDiffSummary(diff codersdk.ChatDiffContents, changes []codersdk.ChatGitChange) string { - lines := diffMetadataLines(diff) - if len(changes) == 0 { - if len(lines) > 0 { - lines = append(lines, "") +func parseChatGitChangesFromUnifiedDiff(diff codersdk.ChatDiffContents) []codersdk.ChatGitChange { + rawDiff := sanitizeTerminalRenderableText(diff.Diff) + if strings.TrimSpace(rawDiff) == "" { + return nil + } + + var ( + changes []codersdk.ChatGitChange + current *codersdk.ChatGitChange + currentAdditions int + currentDeletions int + inHunk bool + ) + flush := func() { + if current == nil { + return } - lines = append(lines, "No changes detected.") - return strings.Join(lines, "\n") + if current.FilePath == "" { + current = nil + currentAdditions = 0 + currentDeletions = 0 + return + } + if currentAdditions > 0 || currentDeletions > 0 { + stats := make([]string, 0, 2) + if currentAdditions > 0 { + stats = append(stats, fmt.Sprintf("+%d", currentAdditions)) + } + if currentDeletions > 0 { + stats = append(stats, fmt.Sprintf("-%d", currentDeletions)) + } + summary := strings.Join(stats, " ") + current.DiffSummary = &summary + } + changes = append(changes, *current) + current = nil + currentAdditions = 0 + currentDeletions = 0 } - if len(lines) > 0 { - lines = append(lines, "") + + for line := range strings.SplitSeq(rawDiff, "\n") { + switch { + case strings.HasPrefix(line, "diff --git "): + flush() + inHunk = false + // parseUnifiedDiffHeaderPaths may return ("", "", false) when + // the unquoted header form is ambiguous, such as a rename with + // spaces in the paths. We still want to start a new entry so + // the follow-up rename from / rename to / --- / +++ lines can + // populate the correct paths. flush() drops entries that never + // received a FilePath. + oldPath, newPath, _ := parseUnifiedDiffHeaderPaths(line) + current = &codersdk.ChatGitChange{ + ChatID: diff.ChatID, + FilePath: newPath, + ChangeType: "modified", + } + if oldPath != "" && newPath != "" && oldPath != newPath { + oldPathCopy := oldPath + current.OldPath = &oldPathCopy + current.ChangeType = "renamed" + } + case current == nil: + continue + case strings.HasPrefix(line, "@@"): + // Entering a hunk. Everything from here until the next + // "diff --git " header is diff content, including any + // added/removed lines that happen to start with "--- " + // or "+++ ". Those must no longer be treated as file + // headers. + inHunk = true + case !inHunk && strings.HasPrefix(line, "new file mode "): + current.ChangeType = "added" + case !inHunk && strings.HasPrefix(line, "deleted file mode "): + current.ChangeType = "deleted" + case !inHunk && strings.HasPrefix(line, "rename from "): + // rename from/rename to paths are repository-relative and + // never carry the a/ or b/ prefix, so we must not strip + // those segments: a real file at a/foo.txt would otherwise + // be truncated to foo.txt. + oldPath := decodeQuotedDiffLinePath(strings.TrimPrefix(line, "rename from ")) + if oldPath != "" { + oldPathCopy := oldPath + current.OldPath = &oldPathCopy + } + current.ChangeType = "renamed" + case !inHunk && strings.HasPrefix(line, "rename to "): + newPath := decodeQuotedDiffLinePath(strings.TrimPrefix(line, "rename to ")) + if newPath != "" { + current.FilePath = newPath + } + current.ChangeType = "renamed" + case !inHunk && strings.HasPrefix(line, "--- /dev/null"): + current.ChangeType = "added" + case !inHunk && strings.HasPrefix(line, "+++ /dev/null"): + current.ChangeType = "deleted" + case !inHunk && strings.HasPrefix(line, "--- "): + if current.ChangeType == "added" { + continue + } + if oldPath := trimUnifiedDiffPath(strings.TrimPrefix(line, "--- ")); oldPath != "" && oldPath != "/dev/null" { + oldPathCopy := oldPath + current.OldPath = &oldPathCopy + } + case !inHunk && strings.HasPrefix(line, "+++ "): + if current.ChangeType == "deleted" { + continue + } + if newPath := trimUnifiedDiffPath(strings.TrimPrefix(line, "+++ ")); newPath != "" && newPath != "/dev/null" { + current.FilePath = newPath + } + case inHunk && strings.HasPrefix(line, "+"): + currentAdditions++ + case inHunk && strings.HasPrefix(line, "-"): + currentDeletions++ + } } - lines = append(lines, "Files changed:") + flush() + return changes +} + +// parseUnifiedDiffHeaderPaths extracts the old and new paths from a +// `diff --git ...` header line. Git emits paths in one of two forms: +// +// 1. Quoted: `diff --git "a/" "b/"`. Used when paths contain +// control characters, backslashes, double quotes, or (with the default +// core.quotepath setting) bytes above 0x7f. The contents are C-quoted. +// 2. Unquoted: `diff --git a/ b/`. Used for simple paths, which +// may still contain spaces. Because there is no delimiter between the +// two paths, this form is ambiguous when paths contain spaces: we rely +// on the git convention that non-rename diffs repeat the same path in +// both halves. +// +// For the unquoted form we first search for a split point at ` b/` where +// the left and right halves are equal after stripping the `a/` and `b/` +// prefixes (the non-rename case). If that fails but the line contains only +// a single space, we split there for simple renames with no embedded +// whitespace. Otherwise we return ok=false and let the caller rely on the +// subsequent `rename from`, `rename to`, `--- `, and `+++ ` lines. +func parseUnifiedDiffHeaderPaths(line string) (oldPath string, newPath string, ok bool) { + raw := strings.TrimSpace(strings.TrimPrefix(line, "diff --git ")) + if raw == "" { + return "", "", false + } + + if strings.HasPrefix(raw, `"`) { + old, rest, ok := consumeQuotedDiffPath(raw) + if !ok { + return "", "", false + } + rest = strings.TrimLeft(rest, " ") + newp, _, ok := consumeQuotedDiffPath(rest) + if !ok { + return "", "", false + } + // The unquoted values already have their surrounding quotes removed, + // so we must not feed them to trimUnifiedDiffPath (which would strip + // any legitimate leading or trailing quote characters in the file + // name). Only strip the a/ or b/ prefix here. + return stripUnifiedDiffPrefix(old), stripUnifiedDiffPrefix(newp), true + } + + if !strings.HasPrefix(raw, "a/") { + return "", "", false + } + for offset := 0; offset < len(raw); { + idx := strings.Index(raw[offset:], " b/") + if idx < 0 { + break + } + pos := offset + idx + left := trimUnifiedDiffPath(raw[:pos]) + right := trimUnifiedDiffPath(raw[pos+1:]) + if left == right { + return left, right, true + } + offset = pos + 1 + } + // No equal split was found. If the line only contains a single space, + // the split is unambiguous and this is a simple rename whose paths + // happen to differ. Splitting the quoted-path form was handled above, + // so we know the raw form has no quoting to worry about here. + if strings.Count(raw, " ") == 1 { + idx := strings.Index(raw, " b/") + if idx > 0 { + return trimUnifiedDiffPath(raw[:idx]), trimUnifiedDiffPath(raw[idx+1:]), true + } + } + return "", "", false +} + +// consumeQuotedDiffPath reads one C-quoted path from the start of s and +// returns the unquoted value along with the remainder of the string. The +// leading character of s must be `"`. git's C-quoting matches Go's quoted +// string syntax closely enough for strconv.Unquote to handle the common +// cases (octal byte escapes like `\303`, and the usual `\t`, `\n`, `\"`, +// `\\`). +func consumeQuotedDiffPath(s string) (path string, rest string, ok bool) { + if !strings.HasPrefix(s, `"`) { + return "", "", false + } + for i := 1; i < len(s); i++ { + switch s[i] { + case '\\': + // Skip the next byte so an escaped quote does not terminate + // the literal early. Bounds-check to avoid running off the + // end of a malformed input. + if i+1 >= len(s) { + return "", "", false + } + i++ + case '"': + unq, err := strconv.Unquote(s[:i+1]) + if err != nil { + return "", "", false + } + return unq, s[i+1:], true + } + } + return "", "", false +} + +// trimUnifiedDiffPath decodes a path taken from a `--- ` or `+++ ` line +// of a unified diff. Those lines always prefix the path with `a/` or `b/`, +// so the prefix is stripped after any C-quote decoding. +func trimUnifiedDiffPath(path string) string { + return stripUnifiedDiffPrefix(decodeQuotedDiffLinePath(path)) +} + +// decodeQuotedDiffLinePath decodes a git-emitted path without stripping +// any `a/` or `b/` prefix. Git only adds those prefixes to `diff --git`, +// `--- `, and `+++ ` lines, so `rename from`, `rename to`, and similar +// lines must use this helper to avoid truncating a real leading `a/` or +// `b/` directory component. +func decodeQuotedDiffLinePath(path string) string { + path = strings.TrimSpace(path) + // Git quotes the whole path with double quotes and C-style escapes when + // it contains control characters, backslashes, double quotes, or (with + // the default core.quotepath setting) bytes above 0x7f. strconv.Unquote + // understands the same escape vocabulary for the common cases. + if len(path) >= 2 && strings.HasPrefix(path, `"`) && strings.HasSuffix(path, `"`) { + if unq, err := strconv.Unquote(path); err == nil { + return unq + } + return strings.Trim(path, `"`) + } + return path +} + +func stripUnifiedDiffPrefix(path string) string { + switch { + case strings.HasPrefix(path, "a/"), strings.HasPrefix(path, "b/"): + return path[2:] + default: + return path + } +} + +// agentgitOversizePlaceholderPrefix matches the literal prefix that +// agent/agentgit substitutes for a repository's UnifiedDiff when the +// raw diff exceeds maxTotalDiffSize (3 MiB). See +// agent/agentgit/agentgit.go. Multi-repo aggregates assembled by +// buildLocalChatDiffContents can mix real `diff --git` chunks with +// this placeholder, in which case parseChatGitChangesFromUnifiedDiff +// returns a non-zero count for the real chunks while silently +// dropping the placeholder repo. Detecting the prefix separately +// lets renderChatDiffSummary flag the omission so the user is not +// misled into thinking the summary is exhaustive. Kept as a local +// prefix match because the coupling is narrow and the string is +// stable. +const agentgitOversizePlaceholderPrefix = "Total diff too large to show. Size:" + +// hasOversizedRepoPlaceholder reports whether the combined unified +// diff contains at least one agentgit oversize-repo placeholder. +// Matching is scoped to lines that start with the placeholder prefix +// so a false positive from a diff body that legitimately contains the +// phrase (e.g. as a `+` added line inside a real patch) cannot +// trigger the omission notice. agentgit always writes the +// placeholder as the entire UnifiedDiff for a repo, and +// buildLocalChatDiffContents joins segments with "\n", so a real +// placeholder repo always appears on its own line after the join. +func hasOversizedRepoPlaceholder(diff string) bool { + for _, line := range strings.Split(diff, "\n") { + if strings.HasPrefix(line, agentgitOversizePlaceholderPrefix) { + return true + } + } + return false +} + +func renderChatDiffSummary(diff codersdk.ChatDiffContents) string { + changes := parseChatGitChangesFromUnifiedDiff(diff) + if len(changes) == 0 { + // The diff text might be non-empty but not in `diff --git` + // format (for example `agent/agentgit` emits a "Total diff + // too large to show..." placeholder when the raw diff exceeds + // the read limit). Report that changes exist but could not + // be summarized so we do not mislead the user into thinking + // the workspace is clean. + if strings.TrimSpace(diff.Diff) != "" { + return "Changes present but could not be summarized." + } + return "No changes detected." + } + + label := "files" + if len(changes) == 1 { + label = "file" + } + lines := []string{fmt.Sprintf("%d %s changed:", len(changes), label)} for _, change := range changes { path := sanitizeTerminalRenderableText(change.FilePath) if change.ChangeType == "renamed" && change.OldPath != nil && *change.OldPath != "" { path = fmt.Sprintf("%s → %s", sanitizeTerminalRenderableText(*change.OldPath), path) } - lines = append(lines, fmt.Sprintf(" %-8s %s", change.ChangeType, path)) + line := fmt.Sprintf(" %-8s %s", change.ChangeType, path) + if change.DiffSummary != nil && strings.TrimSpace(*change.DiffSummary) != "" { + line = fmt.Sprintf("%s (%s)", line, sanitizeTerminalRenderableText(*change.DiffSummary)) + } + lines = append(lines, line) + } + // A multi-repo aggregate can mix real diff chunks (counted + // above) with agentgit's oversize placeholder for repos whose + // raw diff exceeds maxTotalDiffSize. The placeholder does not + // contribute to the files-changed count because it is not in + // `diff --git` format, so without this notice the summary would + // silently underreport the changeset. + if hasOversizedRepoPlaceholder(diff.Diff) { + lines = append(lines, " (some repositories omitted: diff too large to summarize)") } return strings.Join(lines, "\n") } -func renderDiffDrawer(styles tuiStyles, diff codersdk.ChatDiffContents, changes []codersdk.ChatGitChange, width, height int) string { +func renderStyledDiffBody(styles tuiStyles, diff string) string { + diff = sanitizeTerminalRenderableText(diff) + if strings.TrimSpace(diff) == "" { + return styles.dimmedText.Render("No diff contents.") + } + lines := strings.Split(diff, "\n") + inHunk := false + for i, line := range lines { + // Track whether we're inside a hunk body so styling can + // distinguish legitimate header `--- `/`+++ ` lines from + // additions/deletions whose content happens to start with + // those prefixes (for example a `+++ ` content line whose + // text begins with `++ `). Matches the parser's inHunk + // bookkeeping in parseChatGitChangesFromUnifiedDiff. + switch { + case strings.HasPrefix(line, "diff --git "): + inHunk = false + case strings.HasPrefix(line, "@@"): + inHunk = true + } + lines[i] = styleUnifiedDiffLine(styles, line, inHunk) + } + return strings.Join(lines, "\n") +} + +func styleUnifiedDiffLine(styles tuiStyles, line string, inHunk bool) string { + switch { + case strings.HasPrefix(line, "diff --git "): + return styles.selectedItem.Render(line) + case strings.HasPrefix(line, "index "), + strings.HasPrefix(line, "new file mode "), + strings.HasPrefix(line, "deleted file mode "), + strings.HasPrefix(line, "rename from "), + strings.HasPrefix(line, "rename to "), + strings.HasPrefix(line, "Binary files "): + return styles.subtitle.Render(line) + case !inHunk && (strings.HasPrefix(line, "--- ") || strings.HasPrefix(line, "+++ ")): + return styles.subtitle.Render(line) + case strings.HasPrefix(line, "@@"): + return styles.warningText.Render(line) + case strings.HasPrefix(line, "+"): + return styles.toolSuccess.Render(line) + case strings.HasPrefix(line, "-"): + return styles.errorText.Render(line) + default: + return line + } +} + +// renderDiffDrawer builds the diff overlay contents. The caller is +// responsible for producing summary with renderChatDiffSummary and +// styledBody with renderStyledDiffBody so that every View() redraw +// does not walk the full (potentially 4 MiB) diff through +// parseChatGitChangesFromUnifiedDiff or re-style every line through +// lipgloss. chatViewModel caches both in diffSummary and +// diffStyledBody for this reason. If styledBody is empty the caller +// had no cache (for example tests that construct diffs directly), so +// fall back to computing it here instead of silently rendering an +// empty body. +func renderDiffDrawer(styles tuiStyles, diff codersdk.ChatDiffContents, summary, styledBody string, width, height int) string { innerWidth := contentWidth(width, 6) headerBits := []string{styles.title.Render("Diff")} if meta := diffMetadataLines(diff); len(meta) > 0 { headerBits = append(headerBits, styles.subtitle.Render(strings.Join(meta, " • "))) } - summary := renderChatDiffSummary(diff, changes) - diffBody := sanitizeTerminalRenderableText(diff.Diff) - if strings.TrimSpace(diffBody) == "" { - diffBody = styles.dimmedText.Render("No diff contents.") + diffBody := styledBody + if diffBody == "" { + diffBody = renderStyledDiffBody(styles, diff.Diff) } help := styles.helpText.Render("Esc to close") overhead := countRenderedLines(strings.Join(headerBits, "\n")) + countRenderedLines(summary) + countRenderedLines(help) + 4 diff --git a/cli/exp_agents_render_test.go b/cli/exp_agents_render_test.go index 8be00ff220..115ac22dc8 100644 --- a/cli/exp_agents_render_test.go +++ b/cli/exp_agents_render_test.go @@ -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") diff --git a/cli/exp_agents_test.go b/cli/exp_agents_test.go index 121151ec0a..6a8ed14453 100644 --- a/cli/exp_agents_test.go +++ b/cli/exp_agents_test.go @@ -543,7 +543,7 @@ func TestExpAgents(t *testing.T) { updatedModel, cmd := model.Update(toggleDiffDrawerMsg{}) updated, cmd := mustTUIModelWithCmd(t, updatedModel, cmd) require.Equal(t, overlayDiffDrawer, updated.overlay) - require.Len(t, mustBatchMsg(t, cmd), 2) + require.NotNil(t, cmd) require.Contains(t, plainText(updated.View()), "Loading diff") }) @@ -558,11 +558,48 @@ func TestExpAgents(t *testing.T) { updatedModel, cmd := model.Update(toggleDiffDrawerMsg{}) updated, _ := mustTUIModelWithCmd(t, updatedModel, cmd) - updatedModel, cmd = updated.Update(gitChangesMsg{err: xerrors.New("connection refused")}) + updatedModel, cmd = updated.Update(diffContentsMsg{err: xerrors.New("connection refused")}) updated, _ = mustTUIModelWithCmd(t, updatedModel, cmd) require.Contains(t, plainText(updated.View()), "connection refused") }) + t.Run("DiffDrawerMemoizesSummary", func(t *testing.T) { + t.Parallel() + model := newExpChatsTUIModel(context.Background(), nil, nil, nil, nil, uuid.Nil) + model.currentView = viewChat + model.width = 80 + chat := testChat(codersdk.ChatStatusCompleted) + model.chat.chat = &chat + generation := model.chat.chatGeneration + + // A successful diffContentsMsg pre-renders the summary + // and the lipgloss-styled body so View() redraws do not + // re-parse or re-style the full diff on every keypress + // (see chatViewModel.diffSummary and diffStyledBody). + diff := codersdk.ChatDiffContents{ + ChatID: chat.ID, + Diff: "diff --git a/a.txt b/a.txt\n--- a/a.txt\n+++ b/a.txt\n@@ -1 +1 @@\n-old\n+new", + } + updatedModel, cmd := model.Update(diffContentsMsg{generation: generation, chatID: chat.ID, diff: diff}) + updated, _ := mustTUIModelWithCmd(t, updatedModel, cmd) + require.NotNil(t, updated.chat.diffContents) + require.Equal(t, "1 file changed:\n modified a.txt (+1 -1)", updated.chat.diffSummary) + require.NotEmpty(t, updated.chat.diffStyledBody) + // The cached styled body still contains the diff text + // verbatim: lipgloss wraps lines in escape codes without + // replacing them, so every original line of the input + // diff must survive the round-trip. + require.Contains(t, plainText(updated.chat.diffStyledBody), "diff --git a/a.txt b/a.txt") + require.Contains(t, plainText(updated.chat.diffStyledBody), "+new") + + // setChat clears both caches so a new chat does not + // inherit stale render output from the previous session. + (&updated.chat).setChat(testChat(codersdk.ChatStatusCompleted)) + require.Empty(t, updated.chat.diffSummary) + require.Empty(t, updated.chat.diffStyledBody) + require.Nil(t, updated.chat.diffContents) + }) + t.Run("OverlayDismissedOnViewSwitch", func(t *testing.T) { t.Parallel() model := newExpChatsTUIModel(context.Background(), nil, nil, nil, nil, uuid.Nil) @@ -603,7 +640,6 @@ func TestExpAgents(t *testing.T) { model.catalog = &catalog chat := testChat(codersdk.ChatStatusCompleted) model.chat.chat = &chat - model.chat.gitChanges = []codersdk.ChatGitChange{} updatedModel, cmd := model.Update(toggleDiffDrawerMsg{}) updated, _ := mustTUIModelWithCmd(t, updatedModel, cmd) @@ -837,7 +873,6 @@ func TestExpAgents(t *testing.T) { {name: "Draft/chatOpenedMsg", msg: chatOpenedMsg{generation: 1, chatID: uuid.New(), chat: testChat(codersdk.ChatStatusCompleted)}, draft: true}, {name: "Draft/chatHistoryMsg", msg: chatHistoryMsg{generation: 1, chatID: uuid.New(), messages: []codersdk.ChatMessage{testMessage(1, codersdk.ChatMessageRoleUser, codersdk.ChatMessagePart{Type: codersdk.ChatMessagePartTypeText, Text: "hi"})}}, draft: true}, {name: "Draft/chatStreamEventMsg", msg: chatStreamEventMsg{generation: 1, chatID: uuid.New(), event: testTextPartEvent("stale")}, draft: true}, - {name: "Draft/gitChangesMsg", msg: gitChangesMsg{generation: 1, chatID: uuid.New()}, draft: true}, {name: "Draft/diffContentsMsg", msg: diffContentsMsg{generation: 1, chatID: uuid.New()}, draft: true}, } for _, tt := range tests { @@ -2094,7 +2129,6 @@ func TestExpAgents(t *testing.T) { chat := testChat(codersdk.ChatStatusCompleted) model.chat.setChat(chat) model.chat.messages = overflowingMessages(10) - model.chat.gitChanges = []codersdk.ChatGitChange{} diff := codersdk.ChatDiffContents{ChatID: chat.ID, Diff: "diff --git a/file b/file"} model.chat.diffContents = &diff model.chat.rebuildBlocks() diff --git a/coderd/exp_chats.go b/coderd/exp_chats.go index 180867a1fe..baa46bc07a 100644 --- a/coderd/exp_chats.go +++ b/coderd/exp_chats.go @@ -1711,7 +1711,7 @@ func (api *API) authorizeChatWorkspaceExec( workspace, err := api.Database.GetWorkspaceByID(ctx, chat.WorkspaceID.UUID) if httpapi.Is404Error(err) { httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ - Message: "Chat workspace not found.", + Message: codersdk.ChatGitWatchWorkspaceNotFoundMessage, }) return database.Workspace{}, false } @@ -1742,7 +1742,7 @@ func (api *API) watchChatGit(rw http.ResponseWriter, r *http.Request) { logger = api.Logger.Named("chat_git_watcher").With(slog.F("chat_id", chat.ID)) ) - if _, ok := api.authorizeChatWorkspaceExec(rw, r, chat, "Chat has no workspace to watch."); !ok { + if _, ok := api.authorizeChatWorkspaceExec(rw, r, chat, codersdk.ChatGitWatchNoWorkspaceMessage); !ok { return } @@ -1756,7 +1756,7 @@ func (api *API) watchChatGit(rw http.ResponseWriter, r *http.Request) { } if len(agents) == 0 { httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ - Message: "Chat workspace has no agents.", + Message: codersdk.ChatGitWatchWorkspaceNoAgentsMessage, }) return } @@ -1780,7 +1780,7 @@ func (api *API) watchChatGit(rw http.ResponseWriter, r *http.Request) { } if apiAgent.Status != codersdk.WorkspaceAgentConnected { httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ - Message: fmt.Sprintf("Agent state is %q, it must be in the %q state.", apiAgent.Status, codersdk.WorkspaceAgentConnected), + Message: codersdk.ChatGitWatchAgentStateMessage(apiAgent.Status), }) return } diff --git a/codersdk/chats.go b/codersdk/chats.go index 4da8a35baf..5814464bba 100644 --- a/codersdk/chats.go +++ b/codersdk/chats.go @@ -1164,6 +1164,47 @@ type ChatDiffContents struct { Diff string `json:"diff,omitempty"` } +// Chat git watch error messages. These are the user-visible messages +// the server returns in 400 responses from +// /api/experimental/chats/{id}/stream/git when the chat cannot be +// observed through a workspace agent. They are exported so the CLI +// (and any future consumer) can match them structurally via +// IsChatGitWatchFallbackMessage instead of coupling to exact wording. +// Keep these in sync with coderd/exp_chats.go. +const ( + ChatGitWatchNoWorkspaceMessage = "Chat has no workspace to watch." + ChatGitWatchWorkspaceNotFoundMessage = "Chat workspace not found." + ChatGitWatchWorkspaceNoAgentsMessage = "Chat workspace has no agents." + // ChatGitWatchAgentStatePrefix is the common prefix of the + // message produced by ChatGitWatchAgentStateMessage. The CLI + // uses it as a mechanical fingerprint for the "agent not yet + // connected" case without depending on the formatted values. + ChatGitWatchAgentStatePrefix = "Agent state is " +) + +// ChatGitWatchAgentStateMessage is the user-visible error message +// returned from /api/experimental/chats/{id}/stream/git when the +// chat workspace's agent is not in the connected state. +func ChatGitWatchAgentStateMessage(actual WorkspaceAgentStatus) string { + return fmt.Sprintf("%s%q, it must be in the %q state.", ChatGitWatchAgentStatePrefix, actual, WorkspaceAgentConnected) +} + +// IsChatGitWatchFallbackMessage reports whether msg matches one of +// the 400-response messages /api/experimental/chats/{id}/stream/git +// emits when the chat cannot be observed through a workspace agent. +// Clients should treat these cases as "no diff available" and fall +// back to the empty remote diff instead of surfacing a hard error. +func IsChatGitWatchFallbackMessage(msg string) bool { + trimmed := strings.TrimSpace(msg) + switch trimmed { + case ChatGitWatchNoWorkspaceMessage, + ChatGitWatchWorkspaceNotFoundMessage, + ChatGitWatchWorkspaceNoAgentsMessage: + return true + } + return strings.HasPrefix(trimmed, ChatGitWatchAgentStatePrefix) +} + // ChatStreamEventType represents the kind of chat stream update. type ChatStreamEventType string @@ -2580,20 +2621,6 @@ func (c *ExperimentalClient) ProposeChatTitle(ctx context.Context, chatID uuid.U return resp, json.NewDecoder(res.Body).Decode(&resp) } -// GetChatGitChanges returns git changes for a chat. -func (c *ExperimentalClient) GetChatGitChanges(ctx context.Context, chatID uuid.UUID) ([]ChatGitChange, error) { - res, err := c.Request(ctx, http.MethodGet, fmt.Sprintf("/api/experimental/chats/%s/git-changes", chatID), nil) - if err != nil { - return nil, err - } - defer res.Body.Close() - if res.StatusCode != http.StatusOK { - return nil, ReadBodyAsError(res) - } - var changes []ChatGitChange - return changes, json.NewDecoder(res.Body).Decode(&changes) -} - // GetChatDiffContents returns resolved diff contents for a chat. func (c *ExperimentalClient) GetChatDiffContents(ctx context.Context, chatID uuid.UUID) (ChatDiffContents, error) { res, err := c.Request(ctx, http.MethodGet, fmt.Sprintf("/api/experimental/chats/%s/diff", chatID), nil) diff --git a/site/src/api/api.ts b/site/src/api/api.ts index bf9cb3868d..dc847f4eba 100644 --- a/site/src/api/api.ts +++ b/site/src/api/api.ts @@ -372,15 +372,6 @@ export type GetTemplatesQuery = Readonly<{ readonly q: string; }>; -interface ChatGitChangeResponse extends TypesGen.ChatGitChange { - readonly patch?: string; - readonly diff_patch?: string; - readonly unified_diff?: string; - readonly diffs_url?: string; - readonly diff_url?: string; - readonly diffs_link?: string; -} - function normalizeGetTemplatesOptions( options: GetTemplatesOptions | GetTemplatesQuery = {}, ): Record { @@ -3226,15 +3217,6 @@ class ExperimentalApiMethods { return response.data; }; - getChatGitChanges = async ( - chatId: string, - ): Promise => { - const response = await this.axios.get( - `/api/experimental/chats/${chatId}/git-changes`, - ); - return response.data; - }; - getChatDiffContents = async ( chatId: string, ): Promise => { diff --git a/site/src/api/typesGenerated.ts b/site/src/api/typesGenerated.ts index 600339c0f5..c0abec1ba3 100644 --- a/site/src/api/typesGenerated.ts +++ b/site/src/api/typesGenerated.ts @@ -1653,6 +1653,59 @@ export interface ChatGitChange { readonly detected_at: string; } +// From codersdk/chats.go +/** + * Chat git watch error messages. These are the user-visible messages + * the server returns in 400 responses from + * /api/experimental/chats/{id}/stream/git when the chat cannot be + * observed through a workspace agent. They are exported so the CLI + * (and any future consumer) can match them structurally via + * IsChatGitWatchFallbackMessage instead of coupling to exact wording. + * Keep these in sync with coderd/exp_chats.go. + * ChatGitWatchAgentStatePrefix is the common prefix of the + * message produced by ChatGitWatchAgentStateMessage. The CLI + * uses it as a mechanical fingerprint for the "agent not yet + * connected" case without depending on the formatted values. + */ +export const ChatGitWatchAgentStatePrefix = "Agent state is "; + +// From codersdk/chats.go +/** + * Chat git watch error messages. These are the user-visible messages + * the server returns in 400 responses from + * /api/experimental/chats/{id}/stream/git when the chat cannot be + * observed through a workspace agent. They are exported so the CLI + * (and any future consumer) can match them structurally via + * IsChatGitWatchFallbackMessage instead of coupling to exact wording. + * Keep these in sync with coderd/exp_chats.go. + */ +export const ChatGitWatchNoWorkspaceMessage = "Chat has no workspace to watch."; + +// From codersdk/chats.go +/** + * Chat git watch error messages. These are the user-visible messages + * the server returns in 400 responses from + * /api/experimental/chats/{id}/stream/git when the chat cannot be + * observed through a workspace agent. They are exported so the CLI + * (and any future consumer) can match them structurally via + * IsChatGitWatchFallbackMessage instead of coupling to exact wording. + * Keep these in sync with coderd/exp_chats.go. + */ +export const ChatGitWatchWorkspaceNoAgentsMessage = + "Chat workspace has no agents."; + +// From codersdk/chats.go +/** + * Chat git watch error messages. These are the user-visible messages + * the server returns in 400 responses from + * /api/experimental/chats/{id}/stream/git when the chat cannot be + * observed through a workspace agent. They are exported so the CLI + * (and any future consumer) can match them structurally via + * IsChatGitWatchFallbackMessage instead of coupling to exact wording. + * Keep these in sync with coderd/exp_chats.go. + */ +export const ChatGitWatchWorkspaceNotFoundMessage = "Chat workspace not found."; + // From codersdk/chats.go /** * ChatInputPart is a single user input part for creating a chat.