feat(agent): serve context sources over the agent socket#26526
feat(agent): serve context sources over the agent socket#26526kylecarbs wants to merge 1 commit into
Conversation
|
/coder-agents-review |
|
Chat: Spend limit reached | View chat Review historydeep-review v0.8.0 | Round 4 | Last posted: Round 4, 20 findings (2 P2, 8 P3, 8 Nit, 2 Note), APPROVE. Review Finding inventoryFindings
Law analysisEffective LOC: 1476 (813 production, 663 test, 1429 generated). Head SHA: 0c574da. Verdict: Don't split. Enforcement: Advisory. Contested and acknowledgedCRF-13 (Note, contextmcp.go:39) - Comment verbosity pattern
Round logRound 1Panel (20 reviewers). 2 P2, 5 P3, 3 Nit, 2 Note new. 3 dropped. Reviewed against 023a1c9..0c574da. Round 2Churn guard: PROCEED. 12 addressed, 1 acknowledged. Panel (6 reviewers). 3 P3, 2 Nit new. 0 dropped. Reviewed against 023a1c9..a9283c6. Round 3BLOCKED. All 5 open findings silent. No review. Round 4Churn guard: PROCEED. 5 addressed. Panel (4 reviewers). 2 Nit new. 0 P-level findings. All R2 fixes verified correct. Reviewed against 023a1c9..f634c18. About deep-reviewCRF = Coder Review Finding (P0-P4, Nit, Note)
|
There was a problem hiding this comment.
Well-structured agent-side feature. The provider seam, drift-hash exclusion, and feedback-loop dedup are clean. The test suite (44.9% density) covers every new code path including real subprocess failures and hash sensitivity. Particularly nice: the slices.Equal(paths, last) guard that prevents reload storms from MCP tool broadcasts, and the default-include switch in driftResources that makes new resource kinds hash-safe by default.
Severity count: 2 P2, 5 P3, 3 Nit, 2 Note.
The two P2s are (1) false claims in doc comments about aggregate-hash participation, and (2) a stale-data window between installServers and recordServerHealth that Resync can observe. Both are fixable in-place.
"Two tools identical in name and description but differing only in schema would hash identically, producing a false negative in drift detection." (Hisoka, on the silent schema drop)
agent/x/agentmcp/configwatcher_internal_test.go:35
Nit [CRF-1] The awaitTools doc comment still says cachedTools after the rename to CachedTools.
(Netero)
🤖
🤖 This review was automatically generated with Coder Agents.
025644d to
a9283c6
Compare
|
Reworked the MCP wiring and addressed the prior review (all 12 threads replied to and resolved). What changed since the first push
Force-pushed as a single clean commit ( This update was made by Coder Agents on behalf of @kylecarbs. |
|
/coder-agents-review |
There was a problem hiding this comment.
Clean rework. The agentmcp revert eliminates both R1 P2s (the false-claim comments and the cross-lock race), and the MCP-to-Resource conversion landing in agentcontext dissolves the duplication findings. All 13 R1 findings are resolved.
The reworked code is well-structured: the provider reads the manager's existing Tools cache via an atomic.Pointer, the sync loop deduplicates by both path-set and tool-set equality, and the marshal-error sentinel from CRF-7 is properly carried over.
R2 severity count: 3 P3, 2 Nit.
"When a tool changes which parameters are required without changing property definitions, hashMCPServer produces the same hash." (Meruem, on the Required gap)
🤖 This review was automatically generated with Coder Agents.
a9283c6 to
5d4b950
Compare
|
Follow-up simplification (per review feedback): removed the This update was made by Coder Agents on behalf of @kylecarbs. |
|
/coder-agents-review |
There was a problem hiding this comment.
Further review is blocked until the author responds to or pushes fixes for the 5 open R2 findings. The 5d4b950 push removed the MCPProvider interface but did not address any of the open items.
Open findings awaiting response:
- P3 CRF-17 (
mcp.go:82):buildMCPServerResourcesdropsRequiredfromMCPToolInfo. Required-field changes are invisible to the content hash. Fix: addRequired []stringtoMCPTooland include it inhashMCPServer. - P3 CRF-18 (
agent.go:1619):lastPathscommitted beforeReloadsucceeds. A transient failure permanently skips reload for those paths. Fix: setlastPathsafter Reload succeeds. - P3 CRF-19 (
service.go:309):contextSnapshotToProtoduplicatessnapshotResponseatapi.go:180.Namefield has already drifted between the two. - Nit CRF-20 (
mcp.go:92): Map-to-sorted-slice manual pattern reintroduced (CRF-10 fix was applied tomcpConfigPathsbut not here). - Nit CRF-21 (
resolve.go:510):validateMCPConfigiterates map nondeterministically; error message flickers for multi-entry invalid files.
For each: fix in this PR, file a GitHub issue, or explain why it should not be fixed.
🤖 This review was automatically generated with Coder Agents.
5d4b950 to
f634c18
Compare
|
/coder-agents-review |
There was a problem hiding this comment.
All 18 prior findings resolved (13 from R1, 5 from R2). The R2 fixes in f634c18 are verified correct by all four reviewers:
- CRF-17 (Required in hash):
MCPTool.Requiredpopulated, hashed, tested. - CRF-18 (lastPaths after success): assignment moved to the else branch; transient failures retry.
- CRF-19 (converter drift): Name synced, cross-reference comments added.
- CRF-20/21 (sorted iteration): both now use
slices.Sorted(maps.Keys(...)).
The MCPProvider-to-MCPToolSource simplification (5d4b950) is clean: plain function type, no interface indirection, conversion and hashing in-package. The sync loop terminates correctly, the atomic.Pointer usage is safe, and the socket RPCs follow the existing nil-guard + thin-delegation pattern.
Two new Nits below (cosmetic, non-blocking).
"The code held up." (Hisoka)
agent/agentsocket/client.go:15
Nit [CRF-22] The Option type comment says "for NewClient" but NewServer (server.go:38) also accepts ...Option, and WithContextManager is exclusively a server concern. Consider: "Option configures the agent socket client or server."
(Mafuuu)
🤖
🤖 This review was automatically generated with Coder Agents.
Split from #26466 (agent-only changes); no behavior outside agent/. - agentsocket: add context source CRUD (ContextSources, GetContextSource, AddContextSource, RemoveContextSource) plus GetContextSnapshot and ResyncContext RPCs, with matching client methods and proto. The server takes the context Manager via WithContextManager and returns a clean error when it is absent. - agentcontext: include the per-resource Name in the resync JSON response so the HTTP resync payload stays in sync with the drpc PushContextState path in agentsocket. - agent: pass the context Manager to the socket server via WithContextManager. No MCP wiring and no changes to agent/x/agentmcp or the agentcontext resolver; the socket simply serves whatever context resources the Manager already resolves.
f634c18 to
750fad8
Compare
|
Scope update (force-pushed
Kept: the The prior approval was on Posted by Coder Agents on behalf of @kylecarbs. |
|
/coder-agents-review |
|
@kylecarbs ⛔ This review has reached its per-chat spend limit ($105.93 / $100.00). Further review rounds are paused. To raise the limit and continue, comment: This is a per-chat budget, separate from any account-level usage limit.
|
Overview
Split from #26466, scoped to agent-only changes. This PR exposes the
agent's context sources and snapshots over the existing agent socket. There
are no changes outside
agent/.What's included
ContextSources,GetContextSource,AddContextSource,RemoveContextSource) plusGetContextSnapshotandResyncContextRPCs, with matching client methods and proto. The serverreceives the context
ManagerviaWithContextManagerand returns a cleanerror when it is absent.
Name, keeping the HTTP resync payload in sync with the drpcPushContextStatepath inagentsocket.Managerto the socket server viaWithContextManager.What's intentionally NOT here
agent.gooragentcontext.MCP ownership will land later in
agentcontext; this PR does not build onagent/x/agentmcp.agent/x/agentmcpor theagentcontextresolver. The socketserves whatever context resources the
Manageralready resolves.Context for reviewers
This is one of several PRs split out of #26466. Earlier revisions also wired
live MCP servers through the socket; that scope was removed so this PR stays
purely socket + context plumbing inside
agent/. The agentcontext resolver,agent/x/agentmcp, andagent.goMCP startup behavior are unchanged frommain.Created by Coder Agents on behalf of @kylecarbs.