fix(agent): gate workspace context collection until the agent is ready - #26715
Conversation
The agentcontext.Manager resolved and pushed workspace context at agent init, before startup scripts finished. The boot-time snapshot captured unresolved instruction-file symlinks (CLAUDE.md / .cursorrules -> AGENTS.md) as "unreadable" issues, missed skills that had not synced yet, and listed .mcp.json configs with no connected MCP servers. That partial, error-laden snapshot was pushed to coderd and could hydrate a chat. Add an opt-in readiness gate to the Manager (GateUntilReady + SetReady). While gated it publishes only an Initializing snapshot and the push loop ships nothing, so pre-startup partial state never reaches coderd or hydrates a chat. The agent enables the gate and calls SetReady from the lifecycle transition in handleManifest once startup scripts finish (ready, start_error, or start_timeout). chatd already waits for agent readiness before loading context, so this aligns the agent-side push with that contract.
The readiness gate first landed as an opt-in option (GateUntilReady) layered on top of the original eager resolve-on-construct behavior. But eager resolution before the agent reaches lifecycle ready is the bug, not a mode worth keeping: it captured unresolved instruction-file symlinks (CLAUDE.md / .cursorrules -> AGENTS.md) as "unreadable", missed skills that had not synced, and listed .mcp.json configs with no connected MCP servers. Make the gate unconditional. The Manager always starts gated: NewManager publishes only an Initializing placeholder (version 0) and never walks the filesystem until SetReady runs the first real resolve (version 1). This removes the GateUntilReady option, drops the now-dead resolveLocked helper, and deletes the eager first resolve from NewManager. The agent already calls SetReady from the lifecycle transition in handleManifest once startup scripts finish (ready, start_error, or start_timeout), and chatd waits for agent readiness before loading context, so the push side now matches that contract by construction.
Tighten the readiness-gate comments added with the gate. The behavior is unchanged; the comments just restate it more concisely (and drop the play-by-play in the gate checks, SetReady, the push loop, and the gate tests).
…version 0 The push loop is the only consumer that needs to know a snapshot is the pre-ready placeholder, and Version already encodes that: the gated placeholder is the zero value (version 0) and the first real resolve is version 1. The HTTP /resync SnapshotResponse.Initializing field had no consumer (frontend, CLI, and coderd all ignore it). Remove Snapshot.Initializing and the HTTP field, and gate RunPush on snap.Version == 0. NewManager now leaves the zero-value snapshot in place instead of stamping a placeholder. Behavior is unchanged.
mafredri
left a comment
mafredri
left a comment
There was a problem hiding this comment.
Why do LLM comments have to be so frustratingly bad. 😭
Considering I haven't reviewed the full context refactor, this review was pretty shallow, but didn't find any immediate issues with the logic.
| // the context manager's gate so it collects and pushes the | ||
| // now-complete inventory instead of pre-startup partial | ||
| // state. | ||
| a.contextManager.SetReady() |
There was a problem hiding this comment.
Should this wait until after MCP manager reload? Or are they different code paths and one does not affect the other?
There was a problem hiding this comment.
Different code paths, and the ordering is intentional. SetReady() only releases the gate for instruction files and skills, which are complete once startup scripts finish. MCP servers connect asynchronously in mcpManager.Reload(...) (which can block while connecting, so it's deliberately placed after the lifecycle transition to avoid delaying ready). When the MCP engine's catalog changes it calls the Manager's Trigger (wired via SetOnReload(a.contextManager.Trigger)), driving a re-resolve and a follow-up push that includes the servers. Calling SetReady() after Reload(...) would only delay the instruction-file/skill push behind MCP connection for no benefit. Clarified the inline comment in e5e861c and left this thread open in case you want to weigh in on the ordering.
— 🤖 via Coder Agents, on behalf of @kylecarbs
2 participants
Problem
Workspace context surfaced in chat (Coder Agents) is incomplete and racy on a fresh boot:
.claude/skillsshow up)..mcp.jsonfiles but no MCP servers are registered.CLAUDE.md (file: unreadable)and.cursorrules (file: unreadable)withsymlink resolve: lstat .../AGENTS.md: no such file or directory.Root cause
agentcontext.Managercollected and pushed context too eagerly:NewManagerran an eager resolve at agentinit().RunPushstarts 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/.cursorrulessymlinks toAGENTS.mddon't resolve yet, soEvalSymlinksfails and the resolver emitsStatusUnreadable"symlink resolve" issues.mcpManager.Reload(...)only afterready, so only.mcp.jsonconfigs appear, with no servers.That partial, error-laden snapshot is persisted by coderd and can hydrate a chat.
Fix
Gate
agentcontext.Manageruntil the agent is ready, unconditionally:NewManagerleaves the zero-value (version 0) snapshot in place and never walks the filesystem;RunPushwithholds version-0 snapshots, so nothing reaches coderd.Manager.SetReady()from the lifecycle transition inhandleManifest, right after startup scripts finish (ready, or terminalstart_error/start_timeoutso a failed startup still surfaces whatever context exists).SetReady, the Manager performs the first real resolve (version 1) and broadcasts it;RunPushships it withInitial=true. Later changes (MCP connect, skill edits) re-resolve and push as before.Eager resolution before
readywas 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
ManagerOptions.GateUntilReady. Since the eager resolve-on-construct was the defect, the option, the eager first resolve, and the now-deadresolveLockedhelper were all removed; the Manager is always gated untilSetReady.Snapshot.Initializingbool plus an HTTP/resyncfield, but the push loop was the only consumer and nothing read the HTTP field, so both were dropped.readyfixes all three symptom classes at once and avoids masking genuine post-ready errors (a broken symlink atreadyis still reported).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_WithholdsCollectionUntilReadysimulates collection running before startup finishes (brokenCLAUDE.md/.cursorrules->AGENTS.mdsymlinks): asserts the gated snapshot is the empty version-0 placeholder with no resources and nounreadableissues, and that afterSetReady(target now present) the inventory resolves cleanly to a single instruction file with no spurious issues.agentcontext.TestRunPush_WaitsForReadyasserts the push loop ships nothing while gated even when content exists, then ships the full inventory withInitial=trueafterSetReady.agentcontext.TestManager_SetReadyIsIdempotentcovers the version-0 placeholder before ready, the single resolve to version 1 onSetReady, and idempotency across repeated calls.agent.TestAgent_ContextStatePushed: the first push now already containsAGENTS.mdwithInitial=trueand noUNREADABLEresources (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.