From 17420037857151d2d1ebf0a3c6e957c02860c457 Mon Sep 17 00:00:00 2001 From: Kyle Carberry Date: Thu, 25 Jun 2026 14:17:50 -0600 Subject: [PATCH] fix(agent): gate workspace context collection until the agent is ready (#26715) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Problem Workspace context surfaced in chat (Coder Agents) is incomplete and racy on a fresh boot: - The context panel is missing personal skills (only repo-level skills under `.claude/skills` show up). - The MCP section lists `.mcp.json` files but no MCP servers are registered. - The Issues panel reports instruction files as unreadable, e.g. `CLAUDE.md (file: unreadable)` and `.cursorrules (file: unreadable)` with `symlink resolve: lstat .../AGENTS.md: no such file or directory`. ## Root cause `agentcontext.Manager` collected and pushed context too eagerly: - `NewManager` ran an eager resolve at agent `init()`. - `RunPush` starts as a normal connection routine (`startAgentAPI210`) with no lifecycle gating, so the first snapshot was pushed (`Initial=true`) as soon as the agent API connected. Both happened **before startup scripts finish** and before the lifecycle reaches `ready`. At that point: - `CLAUDE.md` / `.cursorrules` symlinks to `AGENTS.md` don't resolve yet, so `EvalSymlinks` fails and the resolver emits `StatusUnreadable` "symlink resolve" issues. - Personal skills haven't synced yet, so they're missing. - MCP servers connect via `mcpManager.Reload(...)` only **after** `ready`, so only `.mcp.json` configs appear, with no servers. That partial, error-laden snapshot is persisted by coderd and can hydrate a chat. ## Fix Gate `agentcontext.Manager` until the agent is ready, unconditionally: - The Manager always starts gated. `NewManager` leaves the zero-value (version 0) snapshot in place and never walks the filesystem; `RunPush` withholds version-0 snapshots, so nothing reaches coderd. - The agent calls `Manager.SetReady()` from the lifecycle transition in `handleManifest`, right after startup scripts finish (`ready`, or terminal `start_error` / `start_timeout` so a failed startup still surfaces whatever context exists). - On `SetReady`, the Manager performs the first real resolve (version 1) and broadcasts it; `RunPush` ships it with `Initial=true`. Later changes (MCP connect, skill edits) re-resolve and push as before. Eager resolution before `ready` was the bug, not a mode worth preserving, so the gate is always on rather than an opt-in option. This aligns the agent-side push with chatd, which already waits for agent readiness before loading context. No proto/coderd/DB changes: coderd simply never receives a pre-ready snapshot.
Design notes & decisions - **Unconditional, not opt-in.** An earlier iteration added the gate as an opt-in `ManagerOptions.GateUntilReady`. Since the eager resolve-on-construct was the defect, the option, the eager first resolve, and the now-dead `resolveLocked` helper were all removed; the Manager is always gated until `SetReady`. - **Version 0 is the pre-ready sentinel.** The gated placeholder is just the zero-value snapshot (version 0); the first real resolve is version 1, so the push loop withholds anything at version 0. An earlier revision carried a dedicated `Snapshot.Initializing` bool plus an HTTP `/resync` field, but the push loop was the only consumer and nothing read the HTTP field, so both were dropped. - **Defer, don't retry symlinks.** Transient "unreadable" symlinks are an artifact of collecting before checkout. Deferring until `ready` fixes all three symptom classes at once and avoids masking genuine post-ready errors (a broken symlink at `ready` is still reported). - **Release on terminal startup states too** (`start_error`, `start_timeout`), so a failed startup still surfaces whatever context exists instead of gating forever. On reconnect the Manager instance is reused and stays ready.
## Tests - `agentcontext.TestManager_WithholdsCollectionUntilReady` simulates collection running before startup finishes (broken `CLAUDE.md` / `.cursorrules` -> `AGENTS.md` symlinks): asserts the gated snapshot is the empty version-0 placeholder with no resources and no `unreadable` issues, and that after `SetReady` (target now present) the inventory resolves cleanly to a single instruction file with no spurious issues. - `agentcontext.TestRunPush_WaitsForReady` asserts the push loop ships nothing while gated even when content exists, then ships the full inventory with `Initial=true` after `SetReady`. - `agentcontext.TestManager_SetReadyIsIdempotent` covers the version-0 placeholder before ready, the single resolve to version 1 on `SetReady`, and idempotency across repeated calls. - Updated `agent.TestAgent_ContextStatePushed`: the first push now already contains `AGENTS.md` with `Initial=true` and no `UNREADABLE` resources (no pre-startup empty/partial push). Validated on the changed packages: `go test -race ./agent/agentcontext/...`, `go test ./agent/ -run TestAgent_ContextStatePushed`, `golangci-lint run`, `go vet`, `gofmt` (all clean). --- 🤖 Generated by Coder Agents on behalf of @kylecarbs. --- agent/agent.go | 6 ++ agent/agent_context_test.go | 39 +++++++----- agent/agentcontext/doc.go | 5 ++ agent/agentcontext/manager.go | 83 +++++++++++++++---------- agent/agentcontext/manager_test.go | 99 ++++++++++++++++++++++++++++++ agent/agentcontext/push.go | 12 +++- agent/agentcontext/push_test.go | 54 ++++++++++++++++ agent/agentcontext/resolve.go | 6 +- 8 files changed, 251 insertions(+), 53 deletions(-) diff --git a/agent/agent.go b/agent/agent.go index 4d9feb807e..132aeb6a12 100644 --- a/agent/agent.go +++ b/agent/agent.go @@ -1552,6 +1552,12 @@ func (a *agent) handleManifest(manifestOK *checkpoint) func(ctx context.Context, a.metrics.startupScriptSeconds.WithLabelValues(label).Set(dur) a.scriptRunner.StartCron() + // Startup finished (success or terminal failure): release + // the context gate. MCP servers connect below and + // re-trigger a push once up, so we don't block readiness + // on them. + a.contextManager.SetReady() + // Connect to workspace MCP servers after the // lifecycle transition to avoid delaying Ready. // This runs inside the tracked goroutine so it diff --git a/agent/agent_context_test.go b/agent/agent_context_test.go index d0ac28586d..c154bb00ed 100644 --- a/agent/agent_context_test.go +++ b/agent/agent_context_test.go @@ -16,9 +16,11 @@ import ( "github.com/coder/coder/v2/testutil" ) -// TestAgent_ContextStatePushed verifies the agent's -// agentcontext.Manager pushes its initial Snapshot to coderd -// over the v2.10 PushContextState RPC during a normal boot. +// TestAgent_ContextStatePushed verifies the agent pushes its workspace +// context over the v2.10 PushContextState RPC, and that the readiness +// gate (SetReady, wired to the lifecycle transition) holds the push +// until startup completes. The first push therefore already contains +// the seeded AGENTS.md with Initial=true and no "unreadable" issues. func TestAgent_ContextStatePushed(t *testing.T) { t.Parallel() @@ -35,29 +37,32 @@ func TestAgent_ContextStatePushed(t *testing.T) { }, ) - // The first push is the initial empty-workspace snapshot - // because the manifest has not been fetched yet. Wait for a - // later push that includes the seeded AGENTS.md. + // The push is gated until the agent reaches lifecycle ready. Wait + // for that first push to land. var pushes []*agentproto.PushContextStateRequest require.Eventually(t, func() bool { pushes = client.ContextStatePushes() - for _, push := range pushes { - for _, r := range push.GetResources() { - if r.GetInstructionFile() != nil && - filepath.Base(r.GetSource()) == "AGENTS.md" { - return true - } - } - } - return false + return len(pushes) > 0 }, testutil.WaitMedium, testutil.IntervalFast, - "expected the seeded AGENTS.md to appear in a snapshot push; got %d pushes", len(pushes)) + "expected a context snapshot push after startup; got %d pushes", len(pushes)) - require.NotEmpty(t, pushes) first := pushes[0] assert.True(t, first.GetInitial(), "first push must carry Initial=true") assert.NotEmpty(t, first.GetAggregateHash(), "aggregate_hash must be populated") + // The first push must already reflect the ready workspace: the + // seeded AGENTS.md is present and no resource is UNREADABLE. + var foundAgents bool + for _, r := range first.GetResources() { + if r.GetInstructionFile() != nil && + filepath.Base(r.GetSource()) == "AGENTS.md" { + foundAgents = true + } + assert.NotEqualf(t, agentproto.ContextResource_UNREADABLE, r.GetStatus(), + "no resource should be UNREADABLE in the post-ready snapshot: %s", r.GetSource()) + } + assert.True(t, foundAgents, "first push must already include the seeded AGENTS.md") + // Subsequent pushes must not be Initial. for _, p := range pushes[1:] { assert.False(t, p.GetInitial(), "only the first push must be Initial") diff --git a/agent/agentcontext/doc.go b/agent/agentcontext/doc.go index 5063c3f470..890c795e6c 100644 --- a/agent/agentcontext/doc.go +++ b/agent/agentcontext/doc.go @@ -17,6 +17,11 @@ // the tree downward or up to a parent directory. // - A fixed-location fsnotify watcher that signals a re-resolve // when any recognized file changes. +// - A readiness gate (Manager.SetReady). The Manager starts gated, +// publishing only an empty version-0 snapshot until the agent calls +// SetReady from the workspace lifecycle transition once startup +// scripts finish. This keeps pre-startup partial state out of +// coderd and chats. // - An HTTP API at /api/v0/context/sources for source CRUD // and /api/v0/context/resync for synchronous push barriers. // - A Pusher abstraction so the latest Snapshot can be shipped diff --git a/agent/agentcontext/manager.go b/agent/agentcontext/manager.go index 705e2349a4..45d40d4acf 100644 --- a/agent/agentcontext/manager.go +++ b/agent/agentcontext/manager.go @@ -98,6 +98,12 @@ type Manager struct { // observe a change. trigger chan struct{} + // ready gates collection. While false (until the first SetReady + // call) the Manager does not scan; Snapshot() returns the empty + // version-0 value, which the push loop never sends to coderd. + // Guarded by mu. + ready bool + // running tracks Run lifetime. running bool closed bool @@ -108,10 +114,10 @@ type Manager struct { watcher *Watcher } -// NewManager validates options, canonicalizes initial sources, -// performs the first resolver pass synchronously, and returns -// the resulting Manager. Run must be called separately to start -// the watcher and re-resolve goroutine. +// NewManager validates options and canonicalizes initial sources. The +// returned Manager is gated, so its first snapshot is the empty +// version-0 placeholder and the first real resolve runs on SetReady. +// Call Run to start the watcher and re-resolve goroutine. func NewManager(opts ManagerOptions) *Manager { clock := opts.Clock if clock == nil { @@ -146,9 +152,8 @@ func NewManager(opts ManagerOptions) *Manager { // resources unless the resolver already has a provider (tests // inject one via Resolver). The engine owns the connection // lifecycle and notifies this Manager via Trigger when its - // catalog changes (see agent wiring). The provider must be wired - // before the eager first resolve below so the seam is present - // from the first snapshot. + // catalog changes (see agent wiring). Wire it before SetReady runs + // the first resolve. if resolver.MCPResources == nil && opts.MCPCatalog != nil { resolver.MCPResources = func() []Resource { return buildMCPServerResources(opts.MCPCatalog()) @@ -170,12 +175,8 @@ func NewManager(opts ManagerOptions) *Manager { m.addSourceLocked(identity) } - // First snapshot is computed eagerly. The push protocol - // requires a snapshot to be present before the agent signals - // lifecycle = ready, so callers can rely on Snapshot() being - // populated immediately after NewManager returns. - m.resolveLocked() - + // Start gated: m.snapshot stays the zero value (version 0) until + // SetReady runs the first resolve. return m } @@ -444,6 +445,12 @@ func (m *Manager) Resync(ctx context.Context) (Snapshot, error) { m.mu.Unlock() return m.Snapshot(), ErrManagerClosed } + if !m.ready { + // Gated until SetReady: return the version-0 placeholder, no scan. + snap := m.snapshot + m.mu.Unlock() + return snap, nil + } roots := m.scanRootsLocked() resolver := m.resolver watcher := m.watcher @@ -535,6 +542,31 @@ func (m *Manager) Trigger() { m.signal() } +// SetReady starts context collection: the agent calls it once startup +// scripts finish (or terminally fail) so context is never collected +// from a half-built workspace. Idempotent; the first call triggers the +// first resolve and push. +func (m *Manager) SetReady() { + m.mu.Lock() + if m.ready || m.closed { + m.mu.Unlock() + return + } + m.ready = true + running := m.running + m.mu.Unlock() + + if running { + // The Run loop owns the watcher; signal it to re-sync and resolve + // with ready=true. + m.signal() + return + } + // No Run loop yet (embedders or tests driving the Manager directly): + // resolve inline. + m.resolveAndBroadcast(context.Background()) +} + // scanRootsLocked returns the list of ScanRoots to feed the // resolver and watcher. The Manager's mutex must be held. func (m *Manager) scanRootsLocked() []ScanRoot { @@ -599,6 +631,11 @@ func (m *Manager) resolveAndBroadcast(ctx context.Context) { // RemoveSource, Snapshot, and SubscribeChanges for the // duration of the pass. m.mu.Lock() + if !m.ready { + // Gated until SetReady: no scan, no broadcast. + m.mu.Unlock() + return + } roots := m.scanRootsLocked() resolver := m.resolver watcher := m.watcher @@ -654,26 +691,6 @@ func (m *Manager) resolveAndBroadcast(ctx context.Context) { } } -// resolveLocked runs the resolver inline while m.mu is held. -// It is used by the synchronous initial resolve in NewManager, -// where there is no concurrent reader. Background re-resolves -// must use resolveAndBroadcast, which drops the lock around -// filesystem I/O. -func (m *Manager) resolveLocked() { - roots := m.scanRootsLocked() - snap := m.resolver.Resolve(roots) - m.version++ - snap.Version = m.version - // Surface watcher degradation as a snapshot-level error - // when the resolver did not already emit one. - if snap.SnapshotError == "" && m.watcher != nil { - if d := m.watcher.Degraded(); d != "" { - snap.SnapshotError = d - } - } - m.snapshot = snap -} - // ErrSourceNotFound is returned by RemoveSource when the // requested path is not in the source list. var ErrSourceNotFound = xerrors.New("source not found") diff --git a/agent/agentcontext/manager_test.go b/agent/agentcontext/manager_test.go index 3144121481..69eef5b2d0 100644 --- a/agent/agentcontext/manager_test.go +++ b/agent/agentcontext/manager_test.go @@ -43,6 +43,19 @@ func TestMain(m *testing.M) { } func newTestManager(t *testing.T, opts agentcontext.ManagerOptions) *agentcontext.Manager { + t.Helper() + m := newPendingTestManager(t, opts) + // Most tests want the ready behavior; release the gate so the first + // snapshot is the resolved inventory at version 1. Gate tests use + // newPendingTestManager directly. + m.SetReady() + return m +} + +// newPendingTestManager builds a gated Manager (first snapshot is the +// empty version-0 placeholder) for tests that drive SetReady +// themselves. +func newPendingTestManager(t *testing.T, opts agentcontext.ManagerOptions) *agentcontext.Manager { t.Helper() opts.Logger = testutil.Logger(t).Named("agentcontext-test") m := agentcontext.NewManager(opts) @@ -370,6 +383,92 @@ func TestManager_SeedSourcesLateBindsAfterManifest(t *testing.T) { require.Equal(t, late, snap.Resources[0].SourcePath) } +// TestManager_WithholdsCollectionUntilReady reproduces the boot-time +// race: collecting before startup finishes sees instruction-file +// symlinks (CLAUDE.md / .cursorrules -> AGENTS.md) with no target yet. +// The gated snapshot is the version-0 placeholder with no resources or +// errors; after SetReady the symlinks resolve cleanly. +func TestManager_WithholdsCollectionUntilReady(t *testing.T) { + t.Parallel() + if runtime.GOOS == "windows" { + t.Skip("symlinks require admin privileges on Windows runners") + } + dir := testutil.TempDirResolved(t) + // CLAUDE.md and .cursorrules symlink to an AGENTS.md that does not + // exist yet, so an eager resolve would report them unreadable. + require.NoError(t, os.Symlink(filepath.Join(dir, "AGENTS.md"), filepath.Join(dir, "CLAUDE.md"))) + require.NoError(t, os.Symlink(filepath.Join(dir, "AGENTS.md"), filepath.Join(dir, ".cursorrules"))) + + m := newPendingTestManager(t, agentcontext.ManagerOptions{ + WorkingDir: func() string { return dir }, + }) + + // Before SetReady the snapshot is the version-0 placeholder: no + // resources and no errors. The broken symlinks are NOT reported. + snap := m.Snapshot() + require.Zero(t, snap.Version, "gated snapshot must be the version-0 placeholder") + require.Empty(t, snap.Resources, "gated snapshot must not collect a partial inventory") + require.Empty(t, snap.SnapshotError, "gated snapshot must not surface transient errors") + + // Resync stays gated too, so callers see the version-0 placeholder + // instead of a partial result. + rs, err := m.Resync(testutil.Context(t, testutil.WaitShort)) + require.NoError(t, err) + require.Zero(t, rs.Version) + require.Empty(t, rs.Resources) + + // Startup finishes: AGENTS.md now exists, so the symlinks resolve. + mustWriteFile(t, filepath.Join(dir, "AGENTS.md"), "# rules\n") + + // Release the gate. SetReady performs the first real resolve. + m.SetReady() + + snap = m.Snapshot() + require.NotZero(t, snap.Version, "post-ready snapshot must be a real resolve") + require.NotEmpty(t, snap.Resources, "post-ready snapshot must include resolved files") + require.Empty(t, snap.SnapshotError) + var instr int + for _, r := range snap.Resources { + require.NotEqualf(t, agentcontext.StatusUnreadable, r.Status, + "resolved snapshot must not contain spurious unreadable issues: %s", r.Source) + if r.Kind == agentcontext.KindInstructionFile { + instr++ + } + } + // AGENTS.md and the two symlinks that resolve to it collapse to a + // single instruction-file resource. + require.Equal(t, 1, instr) +} + +// TestManager_SetReadyIsIdempotent verifies the first SetReady resolves +// once (version 1) and repeated calls neither panic nor re-resolve. +func TestManager_SetReadyIsIdempotent(t *testing.T) { + t.Parallel() + dir := t.TempDir() + mustWriteFile(t, filepath.Join(dir, "AGENTS.md"), "rules") + + // Gated: first snapshot is the version-0 placeholder. + m := newPendingTestManager(t, agentcontext.ManagerOptions{ + WorkingDir: func() string { return dir }, + }) + snap := m.Snapshot() + require.Zero(t, snap.Version) + require.Empty(t, snap.Resources) + + // First SetReady resolves once: version 1 with the inventory. + m.SetReady() + snap = m.Snapshot() + require.Equal(t, uint64(1), snap.Version) + require.Len(t, snap.Resources, 1) + + // Further calls are no-ops: version and inventory unchanged. + m.SetReady() + m.SetReady() + snap = m.Snapshot() + require.Equal(t, uint64(1), snap.Version) + require.Len(t, snap.Resources, 1) +} + func TestManager_CloseIsIdempotent(t *testing.T) { t.Parallel() m := newTestManager(t, agentcontext.ManagerOptions{ diff --git a/agent/agentcontext/push.go b/agent/agentcontext/push.go index b9c70579bb..99768aa295 100644 --- a/agent/agentcontext/push.go +++ b/agent/agentcontext/push.go @@ -101,10 +101,20 @@ func (m *Manager) RunPush(ctx context.Context, p Pusher, opts PushOptions) error changes, unsub := m.SubscribeChanges() defer unsub() - // First push uses the snapshot computed by NewManager. + // Until SetReady the snapshot is version 0: wait, don't push it. initial := true for { snap := m.Snapshot() + if snap.Version == 0 { + select { + case <-ctx.Done(): + return ctx.Err() + case <-m.closedCh: + return nil + case <-changes: + } + continue + } req := snapshotToPushRequest(snap, initial) err := pushWithRetry(ctx, p, req, initialBackoff, maxBackoff, clock, logger) diff --git a/agent/agentcontext/push_test.go b/agent/agentcontext/push_test.go index 865e114b88..fce3f28649 100644 --- a/agent/agentcontext/push_test.go +++ b/agent/agentcontext/push_test.go @@ -293,6 +293,60 @@ func TestRunPush_RejectedResponseProceeds(t *testing.T) { require.ErrorIs(t, <-pushDone, context.Canceled) } +// TestRunPush_WaitsForReady verifies the push loop ships nothing while +// the Manager is gated and ships the complete inventory once SetReady +// fires, so coderd never sees pre-startup partial state. +func TestRunPush_WaitsForReady(t *testing.T) { + t.Parallel() + dir := t.TempDir() + // Content exists from the start, but the Manager is gated: nothing + // is pushed until SetReady. + require.NoError(t, os.WriteFile(filepath.Join(dir, "AGENTS.md"), []byte("rules"), 0o600)) + + m := newPendingTestManager(t, agentcontext.ManagerOptions{ + WorkingDir: func() string { return dir }, + }) + + p := newFakePusher() + ctx, cancel := context.WithCancel(testutil.Context(t, testutil.WaitLong)) + defer cancel() + + pushDone := make(chan error, 1) + go func() { + pushDone <- m.RunPush(ctx, p, agentcontext.PushOptions{ + Logger: testutil.Logger(t).Named("push"), + }) + }() + + // While gated, no push must be sent even though AGENTS.md exists. + select { + case <-p.signal: + t.Fatal("push loop shipped a snapshot before SetReady") + case <-time.After(testutil.IntervalMedium): + } + require.Empty(t, p.snapshot(), "no push must happen before the gate releases") + + // Startup completes; the gate releases and the first real snapshot + // is pushed with Initial=true. + m.SetReady() + + select { + case <-p.signal: + case <-time.After(testutil.WaitShort): + t.Fatal("expected a push after SetReady") + } + + requests := p.snapshot() + require.NotEmpty(t, requests) + first := requests[0] + require.True(t, first.Initial, "first push after the gate releases must be Initial") + require.NotEmpty(t, first.Resources, "first push must carry the resolved inventory") + require.Empty(t, first.SnapshotError, "first push must not carry a transient snapshot error") + + cancel() + require.ErrorIs(t, <-pushDone, context.Canceled) +} + func TestRunPush_NilPusherErrors(t *testing.T) { t.Parallel() m := newTestManager(t, agentcontext.ManagerOptions{ diff --git a/agent/agentcontext/resolve.go b/agent/agentcontext/resolve.go index 78bf234b2f..92e713783b 100644 --- a/agent/agentcontext/resolve.go +++ b/agent/agentcontext/resolve.go @@ -953,8 +953,10 @@ type MCPTool struct { // Snapshot is the immutable bundle of resources produced by a // single resolver pass. type Snapshot struct { - // Version is monotonically increasing per Manager - // instance; resets when the agent process restarts. + // Version is monotonically increasing per Manager instance; resets + // when the agent process restarts. Version 0 is the gated pre-ready + // placeholder (the first real resolve is version 1), which the push + // loop withholds. Version uint64 // AggregateHash is sha256 over a canonical encoding of // (ID, Kind, Source, ContentHash, Status) for every