Skip to content

Commit 19e44f4

Browse files
kylecarbskylecarbs
authored
fix: target specific chat in MarkStale instead of broadcasting to all workspace chats (#23883)
## Problem Subagent chats were receiving git context (branch, remote origin, PR status) from their parent or sibling chats' git operations. When a git operation triggers external auth, the workspace agent sends `chat_id` identifying which chat initiated it — but this was broken at two levels: 1. **Agent side:** `CODER_CHAT_ID` was never injected into process environments. `chatd` sets `Coder-Chat-Id` HTTP headers and the agent extracts them for process isolation, but never propagated `CODER_CHAT_ID` to `cmd.Env`. So `gitaskpass` always sent an empty `chat_id`. 2. **Server side:** `workspaceAgentsExternalAuth` ignored the `chat_id` query param. `MarkStale` broadcast git context to **all** chats on the workspace via `filterChatsByWorkspaceID`. ## Fix - Inject `CODER_CHAT_ID` into `cmd.Env` in `agentproc` when the chat ID is known, so `gitaskpass` can read and forward it. - Read `chat_id` from query params in `workspaceAgentsExternalAuth` and thread it through `chatGitRef`. - Refactor `MarkStale` to accept a `MarkStaleParams` struct. When `ChatID` is provided, target only that specific chat. When empty (legacy agents, non-chat git operations), fall back to the existing workspace-wide broadcast. - Extract `markStaleSingle` helper to deduplicate the upsert+publish logic. <details><summary>Investigation notes</summary> ### Data flow before fix ``` chatd → sets Coder-Chat-Id header on agent conn agent → extracts chatID, stores on process struct agent → does NOT set CODER_CHAT_ID in cmd.Env ← gap 1 gitaskpass → reads CODER_CHAT_ID (always empty), sends chat_id="" server handler → ignores chat_id query param ← gap 2 MarkStale → broadcasts to ALL workspace chats ``` ### Data flow after fix ``` chatd → sets Coder-Chat-Id header on agent conn agent → extracts chatID, stores on process struct agent → sets CODER_CHAT_ID in cmd.Env gitaskpass → reads CODER_CHAT_ID, sends chat_id=<uuid> server handler → reads chat_id, passes to MarkStale MarkStale → targets only that specific chat ``` </details>
1 parent 2ea89e1 commit 19e44f4

5 files changed

Lines changed: 240 additions & 44 deletions

File tree

‎agent/agentproc/process.go‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -148,6 +148,11 @@ func (m *manager) start(req workspacesdk.StartProcessRequest, chatID string) (*p
148148
for k, v := range req.Env {
149149
cmd.Env = append(cmd.Env, fmt.Sprintf("%s=%s", k, v))
150150
}
151+
// Propagate the chat ID so child processes (e.g.
152+
// GIT_ASKPASS) can send it back to the server.
153+
if chatID != "" {
154+
cmd.Env = append(cmd.Env, fmt.Sprintf("CODER_CHAT_ID=%s", chatID))
155+
}
151156

152157
if err := cmd.Start(); err != nil {
153158
cancel()

‎coderd/exp_chats.go‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -66,11 +66,12 @@ const (
6666
maxSystemPromptLenBytes = 131072 // 128 KiB
6767
)
6868

69-
// chatGitRef holds the branch and remote origin reported by the
70-
// workspace agent during a git operation.
69+
// chatGitRef holds the branch, remote origin, and optional chat
70+
// ID reported by the workspace agent during a git operation.
7171
type chatGitRef struct {
7272
Branch string
7373
RemoteOrigin string
74+
ChatID uuid.UUID
7475
}
7576

7677
type chatRepositoryRef struct {

‎coderd/workspaceagents.go‎

Lines changed: 20 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,7 @@ import (
4242
"github.com/coder/coder/v2/coderd/telemetry"
4343
maputil "github.com/coder/coder/v2/coderd/util/maps"
4444
"github.com/coder/coder/v2/coderd/wspubsub"
45+
"github.com/coder/coder/v2/coderd/x/gitsync"
4546
"github.com/coder/coder/v2/codersdk"
4647
"github.com/coder/coder/v2/codersdk/agentsdk"
4748
"github.com/coder/coder/v2/codersdk/workspacesdk"
@@ -1840,6 +1841,11 @@ func (api *API) workspaceAgentsExternalAuth(rw http.ResponseWriter, r *http.Requ
18401841
Branch: strings.TrimSpace(query.Get("git_branch")),
18411842
RemoteOrigin: strings.TrimSpace(query.Get("git_remote_origin")),
18421843
}
1844+
if raw := strings.TrimSpace(query.Get("chat_id")); raw != "" {
1845+
if parsed, err := uuid.Parse(raw); err == nil {
1846+
gitRef.ChatID = parsed
1847+
}
1848+
}
18431849
// Either match or configID must be provided!
18441850
match := query.Get("match")
18451851
if match == "" {
@@ -1938,7 +1944,13 @@ func (api *API) workspaceAgentsExternalAuth(rw http.ResponseWriter, r *http.Requ
19381944
// context is retained even if the flow requires an out-of-band login.
19391945
if gitRef.Branch != "" && gitRef.RemoteOrigin != "" {
19401946
//nolint:gocritic // Chat processor context required for cross-user chat lookup
1941-
api.gitSyncWorker.MarkStale(dbauthz.AsChatd(ctx), workspace.ID, workspace.OwnerID, gitRef.Branch, gitRef.RemoteOrigin)
1947+
api.gitSyncWorker.MarkStale(dbauthz.AsChatd(ctx), gitsync.MarkStaleParams{
1948+
WorkspaceID: workspace.ID,
1949+
OwnerID: workspace.OwnerID,
1950+
Branch: gitRef.Branch,
1951+
Origin: gitRef.RemoteOrigin,
1952+
ChatID: gitRef.ChatID,
1953+
})
19421954
}
19431955

19441956
var previousToken *database.ExternalAuthLink
@@ -2087,7 +2099,13 @@ func (api *API) workspaceAgentsExternalAuthListen(ctx context.Context, rw http.R
20872099
}
20882100
// MarkStale will trigger a refresh by coderd/gitsync.
20892101
//nolint:gocritic // Chat processor context required for cross-user chat lookup
2090-
api.gitSyncWorker.MarkStale(dbauthz.AsChatd(ctx), workspace.ID, workspace.OwnerID, gitRef.Branch, gitRef.RemoteOrigin)
2102+
api.gitSyncWorker.MarkStale(dbauthz.AsChatd(ctx), gitsync.MarkStaleParams{
2103+
WorkspaceID: workspace.ID,
2104+
OwnerID: workspace.OwnerID,
2105+
Branch: gitRef.Branch,
2106+
Origin: gitRef.RemoteOrigin,
2107+
ChatID: gitRef.ChatID,
2108+
})
20912109
httpapi.Write(ctx, rw, http.StatusOK, resp)
20922110
return
20932111
}

‎coderd/x/gitsync/worker.go‎

Lines changed: 63 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -274,25 +274,44 @@ func (w *Worker) tick(ctx context.Context) {
274274
}
275275
}
276276

277-
// MarkStale persists the git ref on all chats for a workspace,
278-
// setting stale_at to the past so the next tick picks them up.
279-
// Publishes a diff status event for each affected chat.
277+
// MarkStaleParams holds the arguments for Worker.MarkStale.
278+
type MarkStaleParams struct {
279+
WorkspaceID uuid.UUID
280+
OwnerID uuid.UUID
281+
Branch string
282+
Origin string
283+
// ChatID, when set, targets a single chat instead of
284+
// broadcasting to every chat on the workspace.
285+
ChatID uuid.UUID
286+
}
287+
288+
// MarkStale persists the git ref for a chat (or all chats on a
289+
// workspace when no ChatID is provided), setting stale_at to the
290+
// past so the next tick picks them up. Publishes a diff status
291+
// event for each affected chat.
280292
// Called from workspaceagents handlers. No goroutines spawned.
281-
func (w *Worker) MarkStale(
282-
ctx context.Context,
283-
workspaceID, ownerID uuid.UUID,
284-
branch, origin string,
285-
) {
286-
if branch == "" || origin == "" {
293+
func (w *Worker) MarkStale(ctx context.Context, p MarkStaleParams) {
294+
if p.Branch == "" || p.Origin == "" {
295+
return
296+
}
297+
298+
// When a specific chat is identified, target it directly
299+
// instead of broadcasting to every chat on the workspace.
300+
// Note: this path does not verify that the chat belongs to
301+
// WorkspaceID. This is safe because ChatID originates from
302+
// chatd via the agent (trusted data flow), but differs from
303+
// the broadcast path which filters by workspace.
304+
if p.ChatID != uuid.Nil {
305+
w.markStaleSingle(ctx, p.ChatID, p.Branch, p.Origin)
287306
return
288307
}
289308

290309
chatRows, err := w.store.GetChats(ctx, database.GetChatsParams{
291-
OwnerID: ownerID,
310+
OwnerID: p.OwnerID,
292311
})
293312
if err != nil {
294313
w.logger.Warn(ctx, "list chats for git ref storage",
295-
slog.F("workspace_id", workspaceID),
314+
slog.F("workspace_id", p.WorkspaceID),
296315
slog.Error(err))
297316
return
298317
}
@@ -302,30 +321,39 @@ func (w *Worker) MarkStale(
302321
chats[i] = row.Chat
303322
}
304323

305-
for _, chat := range filterChatsByWorkspaceID(chats, workspaceID) {
306-
_, err := w.store.UpsertChatDiffStatusReference(ctx,
307-
database.UpsertChatDiffStatusReferenceParams{
308-
ChatID: chat.ID,
309-
GitBranch: branch,
310-
GitRemoteOrigin: origin,
311-
StaleAt: w.clock.Now().Add(-time.Second),
312-
Url: sql.NullString{},
313-
},
314-
)
315-
if err != nil {
316-
w.logger.Warn(ctx, "store git ref on chat diff status",
317-
slog.F("chat_id", chat.ID),
318-
slog.F("workspace_id", workspaceID),
319-
slog.Error(err))
320-
continue
321-
}
322-
// Notify the frontend immediately so the UI shows the
323-
// branch info even before the worker refreshes PR data.
324-
if w.publishDiffStatusChangeFn != nil {
325-
if pubErr := w.publishDiffStatusChangeFn(ctx, chat.ID); pubErr != nil {
326-
w.logger.Debug(ctx, "publish diff status after mark stale",
327-
slog.F("chat_id", chat.ID), slog.Error(pubErr))
328-
}
324+
for _, chat := range filterChatsByWorkspaceID(chats, p.WorkspaceID) {
325+
w.markStaleSingle(ctx, chat.ID, p.Branch, p.Origin)
326+
}
327+
}
328+
329+
// markStaleSingle upserts the git ref for a single chat and
330+
// publishes a diff-status change event.
331+
func (w *Worker) markStaleSingle(
332+
ctx context.Context,
333+
chatID uuid.UUID,
334+
branch, origin string,
335+
) {
336+
_, err := w.store.UpsertChatDiffStatusReference(ctx,
337+
database.UpsertChatDiffStatusReferenceParams{
338+
ChatID: chatID,
339+
GitBranch: branch,
340+
GitRemoteOrigin: origin,
341+
StaleAt: w.clock.Now().Add(-time.Second),
342+
Url: sql.NullString{},
343+
},
344+
)
345+
if err != nil {
346+
w.logger.Warn(ctx, "store git ref on chat diff status",
347+
slog.F("chat_id", chatID),
348+
slog.Error(err))
349+
return
350+
}
351+
// Notify the frontend immediately so the UI shows the
352+
// branch info even before the worker refreshes PR data.
353+
if w.publishDiffStatusChangeFn != nil {
354+
if pubErr := w.publishDiffStatusChangeFn(ctx, chatID); pubErr != nil {
355+
w.logger.Debug(ctx, "publish diff status after mark stale",
356+
slog.F("chat_id", chatID), slog.Error(pubErr))
329357
}
330358
}
331359
}

‎coderd/x/gitsync/worker_test.go‎

Lines changed: 149 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -644,7 +644,12 @@ func TestWorker_MarkStale_UpsertAndPublish(t *testing.T) {
644644
refresher := newTestRefresher(t, mClock)
645645
worker := gitsync.NewWorker(store, refresher, pub, mClock, logger)
646646

647-
worker.MarkStale(ctx, workspaceID, ownerID, "feature", "https://github.com/owner/repo")
647+
worker.MarkStale(ctx, gitsync.MarkStaleParams{
648+
WorkspaceID: workspaceID,
649+
OwnerID: ownerID,
650+
Branch: "feature",
651+
Origin: "https://github.com/owner/repo",
652+
})
648653

649654
mu.Lock()
650655
defer mu.Unlock()
@@ -683,7 +688,12 @@ func TestWorker_MarkStale_NoMatchingChats(t *testing.T) {
683688
refresher := newTestRefresher(t, mClock)
684689
worker := gitsync.NewWorker(store, refresher, nil, mClock, logger)
685690

686-
worker.MarkStale(ctx, workspaceID, ownerID, "main", "https://github.com/x/y")
691+
worker.MarkStale(ctx, gitsync.MarkStaleParams{
692+
WorkspaceID: workspaceID,
693+
OwnerID: ownerID,
694+
Branch: "main",
695+
Origin: "https://github.com/x/y",
696+
})
687697
}
688698

689699
func TestWorker_MarkStale_UpsertFails_ContinuesNext(t *testing.T) {
@@ -723,7 +733,12 @@ func TestWorker_MarkStale_UpsertFails_ContinuesNext(t *testing.T) {
723733
refresher := newTestRefresher(t, mClock)
724734
worker := gitsync.NewWorker(store, refresher, pub, mClock, logger)
725735

726-
worker.MarkStale(ctx, workspaceID, ownerID, "dev", "https://github.com/a/b")
736+
worker.MarkStale(ctx, gitsync.MarkStaleParams{
737+
WorkspaceID: workspaceID,
738+
OwnerID: ownerID,
739+
Branch: "dev",
740+
Origin: "https://github.com/a/b",
741+
})
727742

728743
assert.Equal(t, int32(1), publishCount.Load())
729744
}
@@ -743,7 +758,12 @@ func TestWorker_MarkStale_GetChatsFails(t *testing.T) {
743758
refresher := newTestRefresher(t, mClock)
744759
worker := gitsync.NewWorker(store, refresher, nil, mClock, logger)
745760

746-
worker.MarkStale(ctx, uuid.New(), uuid.New(), "main", "https://github.com/x/y")
761+
worker.MarkStale(ctx, gitsync.MarkStaleParams{
762+
WorkspaceID: uuid.New(),
763+
OwnerID: uuid.New(),
764+
Branch: "main",
765+
Origin: "https://github.com/x/y",
766+
})
747767
}
748768

749769
func TestWorker_TickStoreError(t *testing.T) {
@@ -795,11 +815,135 @@ func TestWorker_MarkStale_EmptyBranchOrOrigin(t *testing.T) {
795815
refresher := newTestRefresher(t, mClock)
796816
worker := gitsync.NewWorker(store, refresher, nil, mClock, logger)
797817

798-
worker.MarkStale(ctx, uuid.New(), uuid.New(), tc.branch, tc.origin)
818+
worker.MarkStale(ctx, gitsync.MarkStaleParams{
819+
WorkspaceID: uuid.New(),
820+
OwnerID: uuid.New(),
821+
Branch: tc.branch,
822+
Origin: tc.origin,
823+
})
799824
})
800825
}
801826
}
802827

828+
func TestWorker_MarkStale_WithChatID(t *testing.T) {
829+
t.Parallel()
830+
ctx := testutil.Context(t, testutil.WaitShort)
831+
832+
targetChat := uuid.New()
833+
834+
var mu sync.Mutex
835+
var upsertRefCalls []database.UpsertChatDiffStatusReferenceParams
836+
var publishedIDs []uuid.UUID
837+
838+
ctrl := gomock.NewController(t)
839+
store := dbmock.NewMockStore(ctrl)
840+
841+
// GetChats should NOT be called when a specific chat ID is provided.
842+
store.EXPECT().GetChats(gomock.Any(), gomock.Any()).Times(0)
843+
store.EXPECT().UpsertChatDiffStatusReference(gomock.Any(), gomock.Any()).DoAndReturn(func(_ context.Context, arg database.UpsertChatDiffStatusReferenceParams) (database.ChatDiffStatus, error) {
844+
mu.Lock()
845+
upsertRefCalls = append(upsertRefCalls, arg)
846+
mu.Unlock()
847+
return database.ChatDiffStatus{ChatID: arg.ChatID}, nil
848+
}).Times(1)
849+
850+
pub := func(_ context.Context, chatID uuid.UUID) error {
851+
mu.Lock()
852+
publishedIDs = append(publishedIDs, chatID)
853+
mu.Unlock()
854+
return nil
855+
}
856+
857+
mClock := quartz.NewMock(t)
858+
now := mClock.Now()
859+
logger := slogtest.Make(t, &slogtest.Options{IgnoreErrors: true})
860+
refresher := newTestRefresher(t, mClock)
861+
worker := gitsync.NewWorker(store, refresher, pub, mClock, logger)
862+
863+
worker.MarkStale(ctx, gitsync.MarkStaleParams{
864+
WorkspaceID: uuid.New(),
865+
OwnerID: uuid.New(),
866+
Branch: "my-branch",
867+
Origin: "https://github.com/org/repo",
868+
ChatID: targetChat,
869+
})
870+
871+
mu.Lock()
872+
defer mu.Unlock()
873+
874+
require.Len(t, upsertRefCalls, 1)
875+
assert.Equal(t, targetChat, upsertRefCalls[0].ChatID)
876+
assert.Equal(t, "my-branch", upsertRefCalls[0].GitBranch)
877+
assert.Equal(t, "https://github.com/org/repo", upsertRefCalls[0].GitRemoteOrigin)
878+
assert.True(t, upsertRefCalls[0].StaleAt.Before(now),
879+
"stale_at should be in the past, got %v vs now %v", upsertRefCalls[0].StaleAt, now)
880+
881+
require.Len(t, publishedIDs, 1)
882+
assert.Equal(t, targetChat, publishedIDs[0])
883+
}
884+
885+
func TestWorker_MarkStale_NilChatID_Broadcasts(t *testing.T) {
886+
t.Parallel()
887+
ctx := testutil.Context(t, testutil.WaitShort)
888+
889+
workspaceID := uuid.New()
890+
ownerID := uuid.New()
891+
chat1 := uuid.New()
892+
893+
var mu sync.Mutex
894+
var upsertRefCalls []database.UpsertChatDiffStatusReferenceParams
895+
var publishedIDs []uuid.UUID
896+
897+
ctrl := gomock.NewController(t)
898+
store := dbmock.NewMockStore(ctrl)
899+
900+
// GetChats IS called because a nil ChatID triggers the
901+
// workspace-wide broadcast path.
902+
store.EXPECT().GetChats(gomock.Any(), gomock.Any()).
903+
DoAndReturn(func(_ context.Context, arg database.GetChatsParams) ([]database.GetChatsRow, error) {
904+
require.Equal(t, ownerID, arg.OwnerID)
905+
return []database.GetChatsRow{
906+
{Chat: database.Chat{ID: chat1, OwnerID: ownerID, WorkspaceID: uuid.NullUUID{UUID: workspaceID, Valid: true}}},
907+
}, nil
908+
})
909+
store.EXPECT().UpsertChatDiffStatusReference(gomock.Any(), gomock.Any()).DoAndReturn(func(_ context.Context, arg database.UpsertChatDiffStatusReferenceParams) (database.ChatDiffStatus, error) {
910+
mu.Lock()
911+
upsertRefCalls = append(upsertRefCalls, arg)
912+
mu.Unlock()
913+
return database.ChatDiffStatus{ChatID: arg.ChatID}, nil
914+
}).Times(1)
915+
916+
pub := func(_ context.Context, chatID uuid.UUID) error {
917+
mu.Lock()
918+
publishedIDs = append(publishedIDs, chatID)
919+
mu.Unlock()
920+
return nil
921+
}
922+
923+
mClock := quartz.NewMock(t)
924+
logger := slogtest.Make(t, &slogtest.Options{IgnoreErrors: true})
925+
refresher := newTestRefresher(t, mClock)
926+
worker := gitsync.NewWorker(store, refresher, pub, mClock, logger)
927+
928+
// Zero-value ChatID (uuid.Nil) triggers broadcast.
929+
worker.MarkStale(ctx, gitsync.MarkStaleParams{
930+
WorkspaceID: workspaceID,
931+
OwnerID: ownerID,
932+
Branch: "main",
933+
Origin: "https://github.com/org/repo",
934+
})
935+
936+
mu.Lock()
937+
defer mu.Unlock()
938+
939+
require.Len(t, upsertRefCalls, 1)
940+
assert.Equal(t, chat1, upsertRefCalls[0].ChatID)
941+
assert.Equal(t, "main", upsertRefCalls[0].GitBranch)
942+
943+
require.Len(t, publishedIDs, 1)
944+
assert.Equal(t, chat1, publishedIDs[0])
945+
}
946+
803947
// TestWorker exercises the worker tick against a
804948
// real PostgreSQL database to verify that the SQL queries, foreign key
805949
// constraints, and upsert logic work end-to-end.

0 commit comments

Comments
 (0)