P0 [CRF-1] coderd rejects every PushContextState that contains an acp_harness body, so a workspace with any file in ~/.coder/acp/ stops syncing all context: instruction files, skills, and MCP servers. (Netero, Law)

marshalContextResourceBody in coderd/agentapi/context.go has no ContextResource_AcpHarness case and returns unsupported body variant, and validateAndConvertContextResources fails the whole request on that error; a scratch test in coderd/agentapi returned unsupported body variant *proto.ContextResource_AcpHarness. The push loop retries with the same payload, so context stays stale until the config is removed. TestAgent_ACPInterface passes because agenttest.Client records pushes without coderd validation. Fix: move the context integration to a separate PR that adds the coderd case, an acp_harness value in the workspace_agent_context_body_kind enum, and an Agent API minor version bump, and have the agent send the variant only to coderd versions that accept it.

🤖

This can only happen during a multi-replica rollout deployment, and it'll automatically recover once the rollout completes. We don't officially support rollout deployments. Not fixing.

P2 [CRF-2] ACP resources use the bare harness slug as Source, which is the same value MCP servers use for their name, so an ACP harness and an MCP server with the same name make coderd reject the push as duplicate source. (Netero)

MCP resources set Source: s.Name (agent/agentcontext/mcp.go:68,86) and coderd rejects any request with a repeated Source (coderd/agentapi/context.go:305). A user with a .mcp.json server claude and ~/.coder/acp/claude.json loses all context sync. Use a source that cannot collide with MCP server names, for example the config file path.

🤖

This is a valid concern, but I think the fix should be to stop enforcing the uniqueness of Source across all resource Kinds. We can enforce that the tuple of (Kind, Source) is unique instead. We should do this in a follow up PR.

Feels like we have too many timeouts spread out across the workspace agent submodules. I think we need a redesign soon on how chatd and workspace agent communicate and have clearly defined behavior and allowances controlled on a different level.

For now, though, this seems to control multiple knobs (polling interval and retry delay).

I changed it to only control the polling interval.

Why is this timeout defined in manager.go but used here? Echoing my previous disdain about the amount of timeouts we have.

The timeout function is there just to make it easier to construct a context with the quartz clock. We depend on that in some tests.

It's there because discovery.go implements methods on the Manager.

If true, should we be logging it without it having been explicitly enabled? I believe Warn makes it through even for the regular log level? Not sure if Debug is always written somewhere, but if there's any output on stderr, we'll essentially be logging it.

if there's any output on stderr, we'll essentially be logging it

Yeah, but only to workspace agent logs which usually stay on the workspace. We need to log them somewhere even if they contain sensitive info so the admin can debug why the workspace agent is failing to probe the harness they configured. The alternative would be to leave them in h.Error but that gets saved in Postgres later.

Are we sure something will be printed? [I may find the answer later, but jotting down the question for now]

Yes, the defer statement does it.

Considering we're using m.ctx here, we have no poll-related bound for reload. It could hang? Considering we have an interval, should the interval duration be an upper bound, or perhaps some multiplier of it?

Yeah. I now gave Reload its own context that inherits from m.ctx with its own timeout.

Looking at poll below, we're using 2x m.ctx here, both in lifetime and stop.

Got rid of lifetime and stop and made m.discover use the ctx directly.