diff --git a/coderd/chats.go b/coderd/chats.go index a6f0b4eb84..1586e7a282 100644 --- a/coderd/chats.go +++ b/coderd/chats.go @@ -1138,15 +1138,6 @@ func chatWorkspaceAuditStatus(err error) int { func (api *API) resolveChatDiffStatus( ctx context.Context, chat database.Chat, -) (*database.ChatDiffStatus, error) { - return api.resolveChatDiffStatusWithOptions(ctx, chat, false) -} - -//nolint:revive // Boolean forces cache refresh bypass. -func (api *API) resolveChatDiffStatusWithOptions( - ctx context.Context, - chat database.Chat, - forceRefresh bool, ) (*database.ChatDiffStatus, error) { status, found, err := api.getCachedChatDiffStatus(ctx, chat.ID) if err != nil { @@ -1172,26 +1163,25 @@ func (api *API) resolveChatDiffStatusWithOptions( if !found { return nil, nil //nolint:nilnil // Callers handle nil status explicitly. } - if reference.PullRequestURL == "" { - return &status, nil - } - if !shouldRefreshChatDiffStatus(status, now, forceRefresh) { + if !chatDiffStatusIsStale(status, now) { return &status, nil } - refreshed, err := api.refreshChatDiffStatus( - ctx, - chat.OwnerID, - chat.ID, - reference.PullRequestURL, + // Use the same refresh pipeline as the background worker + // so both paths share identical provider/token resolution. + refreshed, err := api.gitSyncWorker.RefreshChat( + ctx, status, chat.OwnerID, ) + if err == nil && refreshed != nil { + return refreshed, nil + } if err == nil { - return &refreshed, nil + // No PR exists yet; return what we have. + return &status, nil } api.Logger.Warn(ctx, "failed to refresh chat diff status", slog.F("chat_id", chat.ID), - slog.F("pull_request_url", reference.PullRequestURL), slog.Error(err), ) @@ -1207,14 +1197,6 @@ func (api *API) resolveChatDiffStatusWithOptions( return &backoffStatus, nil } -//nolint:revive // Boolean forces cache refresh bypass. -func shouldRefreshChatDiffStatus(status database.ChatDiffStatus, now time.Time, forceRefresh bool) bool { - if forceRefresh { - return true - } - return chatDiffStatusIsStale(status, now) -} - func (api *API) resolveChatDiffContents( ctx context.Context, chat database.Chat, @@ -1488,68 +1470,6 @@ func chatDiffStatusIsStale(status database.ChatDiffStatus, now time.Time) bool { return !status.StaleAt.After(now) } -func (api *API) refreshChatDiffStatus( - ctx context.Context, - chatOwnerID uuid.UUID, - chatID uuid.UUID, - pullRequestURL string, -) (database.ChatDiffStatus, error) { - // Find a provider that can handle this PR URL. - var gp gitprovider.Provider - var ref gitprovider.PRRef - for _, extAuth := range api.ExternalAuthConfigs { - p := extAuth.Git(api.HTTPClient) - if p == nil { - continue - } - if parsed, ok := p.ParsePullRequestURL(pullRequestURL); ok { - gp = p - ref = parsed - break - } - } - if gp == nil { - return database.ChatDiffStatus{}, xerrors.Errorf("no git provider found for PR URL %q", pullRequestURL) - } - - origin := gp.BuildRepositoryURL(ref.Owner, ref.Repo) - token, err := api.resolveChatGitAccessToken(ctx, chatOwnerID, origin) - if err != nil { - return database.ChatDiffStatus{}, xerrors.Errorf("resolve git access token: %w", err) - } else if token == nil { - return database.ChatDiffStatus{}, xerrors.New("nil git access token") - } - status, err := gp.FetchPullRequestStatus(ctx, *token, ref) - if err != nil { - return database.ChatDiffStatus{}, err - } - - refreshedAt := time.Now().UTC() - refreshedStatus, err := api.Database.UpsertChatDiffStatus( - ctx, - database.UpsertChatDiffStatusParams{ - ChatID: chatID, - Url: sql.NullString{String: pullRequestURL, Valid: true}, - PullRequestState: sql.NullString{ - String: string(status.State), - Valid: status.State != "", - }, - PullRequestTitle: status.Title, - PullRequestDraft: status.Draft, - ChangesRequested: status.ChangesRequested, - Additions: status.DiffStats.Additions, - Deletions: status.DiffStats.Deletions, - ChangedFiles: status.DiffStats.ChangedFiles, - RefreshedAt: refreshedAt, - StaleAt: refreshedAt.Add(chatDiffStatusTTL), - }, - ) - if err != nil { - return database.ChatDiffStatus{}, xerrors.Errorf("upsert chat diff status: %w", err) - } - return refreshedStatus, nil -} - func (api *API) resolveChatGitAccessToken( ctx context.Context, userID uuid.UUID, @@ -1565,7 +1485,9 @@ func (api *API) resolveChatGitAccessToken( if config.Regex == nil || !config.Regex.MatchString(origin) { continue } - link, err := api.Database.GetExternalAuthLink(ctx, + //nolint:gocritic // System access needed to read external auth + // links when called from the gitsync worker (chatd context). + link, err := api.Database.GetExternalAuthLink(dbauthz.AsSystemRestricted(ctx), database.GetExternalAuthLinkParams{ ProviderID: config.ID, UserID: userID, @@ -1574,7 +1496,8 @@ func (api *API) resolveChatGitAccessToken( if err != nil { continue } - refreshed, refreshErr := config.RefreshToken(ctx, api.Database, link) + //nolint:gocritic // System context carried through for token refresh. + refreshed, refreshErr := config.RefreshToken(dbauthz.AsSystemRestricted(ctx), api.Database, link) if refreshErr == nil { link = refreshed } @@ -1602,8 +1525,10 @@ func (api *API) resolveChatGitAccessToken( } seen[providerID] = struct{}{} + //nolint:gocritic // System access needed to read external auth + // links when called from the gitsync worker (chatd context). link, err := api.Database.GetExternalAuthLink( - ctx, + dbauthz.AsSystemRestricted(ctx), database.GetExternalAuthLinkParams{ ProviderID: providerID, UserID: userID, @@ -1617,7 +1542,8 @@ func (api *API) resolveChatGitAccessToken( // the same code path used by provisionerdserver when handing // tokens to provisioners. if cfg, ok := configs[providerID]; ok { - refreshed, refreshErr := cfg.RefreshToken(ctx, api.Database, link) + //nolint:gocritic // System context carried through for token refresh. + refreshed, refreshErr := cfg.RefreshToken(dbauthz.AsSystemRestricted(ctx), api.Database, link) if refreshErr != nil { api.Logger.Debug(ctx, "failed to refresh external auth token for chat diff", slog.F("provider_id", providerID), diff --git a/coderd/chats_test.go b/coderd/chats_test.go index 458c74e714..f93532e289 100644 --- a/coderd/chats_test.go +++ b/coderd/chats_test.go @@ -6,6 +6,7 @@ import ( "encoding/json" "fmt" "net/http" + "net/http/httptest" "regexp" "strings" "testing" @@ -15,6 +16,7 @@ import ( "github.com/stretchr/testify/require" "github.com/coder/coder/v2/coderd/coderdtest" + "github.com/coder/coder/v2/coderd/coderdtest/oidctest" "github.com/coder/coder/v2/coderd/database" "github.com/coder/coder/v2/coderd/database/dbauthz" "github.com/coder/coder/v2/coderd/database/dbfake" @@ -2565,9 +2567,8 @@ func TestGetChatDiffStatus(t *testing.T) { }) require.NoError(t, err) - refreshedAt := time.Date(2026, time.January, 15, 12, 0, 0, 0, time.UTC) - staleAt := time.Date(2026, time.January, 15, 13, 0, 0, 0, time.UTC) - + refreshedAt := time.Now().UTC().Truncate(time.Second) + staleAt := refreshedAt.Add(time.Hour) _, err = db.UpsertChatDiffStatusReference( dbauthz.AsSystemRestricted(ctx), database.UpsertChatDiffStatusReferenceParams{ @@ -2638,6 +2639,130 @@ func TestGetChatDiffStatus(t *testing.T) { _, err = otherClient.GetChatDiffStatus(ctx, createdChat.ID) requireSDKError(t, err, http.StatusNotFound) }) + + // Integration test: exercises the full HTTP handler refresh + // path with a real DB, dbauthz, a mock GitHub API, and an + // external-auth-linked user. Verifies that a stale chat diff + // status is refreshed end-to-end via the gitsync worker's + // Refresh pipeline (provider resolution, token acquisition + // through external auth, and PR status fetch). + t.Run("RefreshesStaleStatusWithExternalAuth", func(t *testing.T) { + t.Parallel() + + ctx := testutil.Context(t, testutil.WaitLong) + + // Mock GitHub API over TLS so the git provider's URL patterns + // (which require https://) match our PR URLs. + ghAPI := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + switch { + // PR status: GET /repos/{owner}/{repo}/pulls/{number} + case r.URL.Path == "/repos/testorg/testrepo/pulls/42" && r.URL.Query().Get("per_page") == "": + _, _ = w.Write([]byte(`{ + "state": "open", + "merged": false, + "draft": false, + "additions": 25, + "deletions": 7, + "changed_files": 4, + "head": {"sha": "abc123"} + }`)) + // PR reviews: GET /repos/{owner}/{repo}/pulls/{number}/reviews + case strings.HasSuffix(r.URL.Path, "/reviews"): + _, _ = w.Write([]byte(`[]`)) + default: + http.NotFound(w, r) + } + })) + t.Cleanup(ghAPI.Close) + + // The git provider derives webBaseURL from apiBaseURL. + // For a TLS server at https://127.0.0.1:PORT, webBaseURL + // is the same, and PR URL patterns match + // https://127.0.0.1:PORT/{owner}/{repo}/pull/{number}. + ghWebHost := strings.TrimPrefix(ghAPI.URL, "https://") + prURL := fmt.Sprintf("https://%s/testorg/testrepo/pull/42", ghWebHost) + remoteOrigin := fmt.Sprintf("https://%s/testorg/testrepo.git", ghWebHost) + + // Set up a fake OIDC IDP for external auth login. + const providerID = "test-github" + fake := oidctest.NewFakeIDP(t, oidctest.WithServing()) + + client, _, api := coderdtest.NewWithAPI(t, &coderdtest.Options{ + DeploymentValues: chatDeploymentValues(t), + ExternalAuthConfigs: []*externalauth.Config{ + fake.ExternalAuthConfig(t, providerID, nil, func(cfg *externalauth.Config) { + cfg.Type = codersdk.EnhancedExternalAuthProviderGitHub.String() + // Point the git provider at our mock API server. + cfg.APIBaseURL = ghAPI.URL + // Match the remote origin (127.0.0.1 host). + cfg.Regex = regexp.MustCompile(regexp.QuoteMeta(ghWebHost)) + }), + }, + }) + db := api.Database + + // Use the TLS mock server's HTTP client (which trusts its + // self-signed cert) for git provider API calls. + api.HTTPClient = ghAPI.Client() + + user := coderdtest.CreateFirstUser(t, client) + modelConfig := createChatModelConfig(t, client) + + // Log in to the external auth provider so the user has an + // ExternalAuthLink row in the DB. This is what + // resolveChatGitAccessToken reads via GetExternalAuthLink. + fake.ExternalLogin(t, client) + + // Insert a chat owned by the user. + chat, err := db.InsertChat(dbauthz.AsSystemRestricted(ctx), database.InsertChatParams{ + OwnerID: user.UserID, + LastModelConfigID: modelConfig.ID, + Title: "rbac integration test", + }) + require.NoError(t, err) + + // Store a pre-resolved PR URL so the refresh path uses + // ParsePullRequestURL directly (skipping branch-to-PR + // resolution, which isn't what we're testing). The status + // is stale (stale_at in the past) so the handler triggers + // a full refresh through RefreshChat. + _, err = db.UpsertChatDiffStatusReference( + dbauthz.AsSystemRestricted(ctx), + database.UpsertChatDiffStatusReferenceParams{ + ChatID: chat.ID, + Url: sql.NullString{String: prURL, Valid: true}, + GitBranch: "feature/rbac-fix", + GitRemoteOrigin: remoteOrigin, + StaleAt: time.Now().Add(-time.Minute), + }, + ) + require.NoError(t, err) + + // Call the HTTP endpoint. This exercises the full code + // path: resolveChatDiffStatus -> RefreshChat (with + // AsSystemRestricted) -> Refresher.Refresh -> + // resolveChatGitAccessToken (GetExternalAuthLink with + // AsSystemRestricted) -> FetchPullRequestStatus (mock). + // + // Without the AsSystemRestricted fix, GetExternalAuthLink + // would fail under the chatd RBAC context (missing + // ActionReadPersonal), causing ErrNoTokenAvailable and a + // refresh failure that silently returns stale data. + status, err := client.GetChatDiffStatus(ctx, chat.ID) + require.NoError(t, err) + + // The mock GitHub API returned PR #42 with 25 additions, + // 7 deletions, 4 changed files, state "open". + require.NotNil(t, status.RefreshedAt, "status should have been refreshed") + require.NotNil(t, status.PullRequestState) + require.Equal(t, "open", *status.PullRequestState) + require.EqualValues(t, 25, status.Additions) + require.EqualValues(t, 7, status.Deletions) + require.EqualValues(t, 4, status.ChangedFiles) + require.NotNil(t, status.URL) + require.Contains(t, *status.URL, "pull/42") + }) } func TestGetChatDiffContents(t *testing.T) { diff --git a/coderd/gitsync/worker.go b/coderd/gitsync/worker.go index 222b8dd074..17992ba95d 100644 --- a/coderd/gitsync/worker.go +++ b/coderd/gitsync/worker.go @@ -6,6 +6,7 @@ import ( "time" "github.com/google/uuid" + "golang.org/x/xerrors" "cdr.dev/slog/v3" "github.com/coder/coder/v2/coderd/database" @@ -238,6 +239,52 @@ func (w *Worker) MarkStale( } } +// RefreshChat synchronously refreshes a single chat's diff +// status using the same Refresher pipeline as the background +// worker. Returns nil, nil when no PR exists yet for the +// branch. Called from HTTP handlers for instant feedback. +func (w *Worker) RefreshChat( + ctx context.Context, + row database.ChatDiffStatus, + ownerID uuid.UUID, +) (*database.ChatDiffStatus, error) { + requests := []RefreshRequest{{ + Row: row, + OwnerID: ownerID, + }} + + results, err := w.refresher.Refresh(ctx, requests) + if err != nil { + return nil, xerrors.Errorf("refresh chat diff status: %w", err) + } + + if len(results) == 0 { + return nil, nil + } + res := results[0] + if res.Error != nil { + return nil, xerrors.Errorf("refresh chat diff status: %w", res.Error) + } + if res.Params == nil { + return nil, nil + } + + upserted, err := w.store.UpsertChatDiffStatus(ctx, *res.Params) + if err != nil { + return nil, xerrors.Errorf("upsert chat diff status: %w", err) + } + + if w.publishDiffStatusChangeFn != nil { + if err := w.publishDiffStatusChangeFn(ctx, row.ChatID); err != nil { + w.logger.Debug(ctx, "publish diff status change", + slog.F("chat_id", row.ChatID), + slog.Error(err)) + } + } + + return &upserted, nil +} + // filterChatsByWorkspaceID returns only chats associated with // the given workspace. func filterChatsByWorkspaceID( diff --git a/coderd/gitsync/worker_test.go b/coderd/gitsync/worker_test.go index 36e4d4fb54..8a228bd614 100644 --- a/coderd/gitsync/worker_test.go +++ b/coderd/gitsync/worker_test.go @@ -742,3 +742,164 @@ func TestWorker(t *testing.T) { expectedStaleAt := mClock.Now().Add(gitsync.DiffStatusTTL) assert.WithinDuration(t, expectedStaleAt, status.StaleAt, time.Second) } + +func TestRefreshChat_Success(t *testing.T) { + t.Parallel() + ctx := testutil.Context(t, testutil.WaitShort) + + chatID := uuid.New() + ownerID := uuid.New() + + row := database.ChatDiffStatus{ + ChatID: chatID, + GitBranch: "feature", + GitRemoteOrigin: "https://github.com/owner/repo", + } + + ctrl := gomock.NewController(t) + store := dbmock.NewMockStore(ctrl) + + upsertedStatus := database.ChatDiffStatus{ + ChatID: chatID, + Url: sql.NullString{String: "https://github.com/o/r/pull/1", Valid: true}, + Additions: 10, + Deletions: 3, + ChangedFiles: 2, + } + store.EXPECT().UpsertChatDiffStatus(gomock.Any(), gomock.Any()). + DoAndReturn(func(_ context.Context, arg database.UpsertChatDiffStatusParams) (database.ChatDiffStatus, error) { + assert.Equal(t, chatID, arg.ChatID) + return upsertedStatus, nil + }) + + var publishCalled atomic.Bool + pub := func(_ context.Context, id uuid.UUID) error { + assert.Equal(t, chatID, id) + publishCalled.Store(true) + return nil + } + + mClock := quartz.NewMock(t) + logger := slogtest.Make(t, &slogtest.Options{IgnoreErrors: true}) + refresher := newTestRefresher(t, mClock) + worker := gitsync.NewWorker(store, refresher, pub, mClock, logger) + + result, err := worker.RefreshChat(ctx, row, ownerID) + require.NoError(t, err) + require.NotNil(t, result) + assert.Equal(t, chatID, result.ChatID) + assert.Equal(t, upsertedStatus.Url, result.Url) + assert.True(t, publishCalled.Load(), "publish should have been called") +} + +func TestRefreshChat_NoPR(t *testing.T) { + t.Parallel() + ctx := testutil.Context(t, testutil.WaitShort) + + chatID := uuid.New() + ownerID := uuid.New() + + row := database.ChatDiffStatus{ + ChatID: chatID, + GitBranch: "feature", + GitRemoteOrigin: "https://github.com/owner/repo", + } + + ctrl := gomock.NewController(t) + store := dbmock.NewMockStore(ctrl) + // UpsertChatDiffStatus should NOT be called. + + var publishCalled atomic.Bool + pub := func(_ context.Context, _ uuid.UUID) error { + publishCalled.Store(true) + return nil + } + + mClock := quartz.NewMock(t) + logger := slogtest.Make(t, &slogtest.Options{IgnoreErrors: true}) + + // ResolveBranchPullRequest returns nil → no PR exists yet. + refresher := newTestRefresher(t, mClock, withResolveBranchPR( + func(context.Context, string, gitprovider.BranchRef) (*gitprovider.PRRef, error) { + return nil, nil + }, + )) + worker := gitsync.NewWorker(store, refresher, pub, mClock, logger) + + result, err := worker.RefreshChat(ctx, row, ownerID) + require.NoError(t, err) + assert.Nil(t, result, "result should be nil when no PR exists") + assert.False(t, publishCalled.Load(), "publish should not be called when no PR exists") +} + +func TestRefreshChat_RefreshError(t *testing.T) { + t.Parallel() + ctx := testutil.Context(t, testutil.WaitShort) + + chatID := uuid.New() + ownerID := uuid.New() + + row := database.ChatDiffStatus{ + ChatID: chatID, + Url: sql.NullString{String: "https://github.com/org/repo/pull/1", Valid: true}, + GitBranch: "feature", + GitRemoteOrigin: "https://github.com/owner/repo", + } + + ctrl := gomock.NewController(t) + store := dbmock.NewMockStore(ctrl) + // UpsertChatDiffStatus should NOT be called. + + // Provider resolver returns nil → "no provider" error. + providers := func(string) gitprovider.Provider { return nil } + tokens := func(context.Context, uuid.UUID, string) (*string, error) { + return ptr.Ref("tok"), nil + } + + mClock := quartz.NewMock(t) + logger := slogtest.Make(t, &slogtest.Options{IgnoreErrors: true}) + refresher := gitsync.NewRefresher(providers, tokens, logger, mClock) + worker := gitsync.NewWorker(store, refresher, nil, mClock, logger) + + result, err := worker.RefreshChat(ctx, row, ownerID) + require.Error(t, err) + assert.Contains(t, err.Error(), "no provider") + assert.Nil(t, result) +} + +func TestRefreshChat_UpsertError(t *testing.T) { + t.Parallel() + ctx := testutil.Context(t, testutil.WaitShort) + + chatID := uuid.New() + ownerID := uuid.New() + + row := database.ChatDiffStatus{ + ChatID: chatID, + GitBranch: "feature", + GitRemoteOrigin: "https://github.com/owner/repo", + } + + ctrl := gomock.NewController(t) + store := dbmock.NewMockStore(ctrl) + + store.EXPECT().UpsertChatDiffStatus(gomock.Any(), gomock.Any()). + Return(database.ChatDiffStatus{}, fmt.Errorf("db write error")) + + var publishCalled atomic.Bool + pub := func(_ context.Context, _ uuid.UUID) error { + publishCalled.Store(true) + return nil + } + + mClock := quartz.NewMock(t) + logger := slogtest.Make(t, &slogtest.Options{IgnoreErrors: true}) + refresher := newTestRefresher(t, mClock) + worker := gitsync.NewWorker(store, refresher, pub, mClock, logger) + + result, err := worker.RefreshChat(ctx, row, ownerID) + require.Error(t, err) + assert.Contains(t, err.Error(), "upsert chat diff status") + assert.Nil(t, result) + assert.False(t, publishCalled.Load(), "publish should not be called when upsert fails") +}