Skip to content

refactor: consolidate MCP logic into utils as single source of truth#353

Merged
khaliqgant merged 2 commits intomainfrom
feature/mcp-sdk-consolidation
Feb 1, 2026
Merged

refactor: consolidate MCP logic into utils as single source of truth#353
khaliqgant merged 2 commits intomainfrom
feature/mcp-sdk-consolidation

Conversation

@khaliqgant
Copy link
Copy Markdown
Member

@khaliqgant khaliqgant commented Jan 30, 2026

Summary

  • Socket discovery (discoverSocket, detectCloudWorkspace, getCloudSocketPath, discoverAgentName, etc.) moved from MCP's cloud.ts (522 lines) to @agent-relay/utils/discovery.ts — MCP now a thin re-export (41 lines, -92%)
  • Error classes (RelayError, DaemonNotRunningError, etc.) moved from MCP's errors.ts to @agent-relay/utils/errors.ts — MCP now a thin re-export (17 lines, -69%)
  • SDK enhanced: RelayClient now uses discoverSocket() for cloud-aware socket resolution instead of hardcoded /tmp/agent-relay.sock; uses typed errors (DaemonNotRunningError, ConnectionError) on connect failure
  • ~560 lines of duplication eliminated, no breaking changes, all public APIs preserved via re-exports
  • Subsumes MCPSocketFixer's socket resolution fix (commit 3a3c095 preserved in consolidated code)

Dependency graph (no cycles)

protocol (no deps)
config → protocol
utils → protocol, config
sdk → protocol, utils
mcp → config, protocol, utils

Test plan

  • 206 utils tests pass (51 new: discovery, errors, consolidation verification)
  • 50 SDK tests pass (all existing, including cloud-aware socket resolution)
  • 102 MCP tests pass (all existing, including 20 discover tests via re-exports)
  • 358 total tests, 0 failures
  • TypeScript compilation clean for utils, SDK, and MCP packages
  • No cyclic dependency issues (verified with turbo)
  • Manual verification: MCP tools work end-to-end with daemon
  • Manual verification: SDK client connects using discovered socket

🤖 Generated with Claude Code


Open with Devin

Copy link
Copy Markdown
Contributor

@devin-ai-integration devin-ai-integration Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Devin Review: No Issues Found

Devin Review analyzed this PR and found no potential bugs to report.

View in Devin Review to see 5 additional flags.

Open in Devin Review

Copy link
Copy Markdown
Member Author

Code Review: PR #353 — MCP/SDK Consolidation

Reviewer: CodexReviewer (automated thorough review)
Verdict: APPROVE


1. Code Correctness & Consolidation Completeness ✅

The consolidation is complete and correctly implemented:

  • @agent-relay/utils/discovery.ts (524 lines) — canonical implementation of all socket discovery, cloud workspace detection, agent identity, and cloud API helpers
  • @agent-relay/utils/errors.ts (56 lines) — canonical implementation of all 7 error classes (RelayError, DaemonNotRunningError, AgentNotFoundError, TimeoutError, ConnectionError, ChannelNotFoundError, SpawnError)
  • @agent-relay/mcp/src/cloud.ts — reduced from 522 lines to 41-line pure re-export (−92%)
  • @agent-relay/mcp/src/errors.ts — reduced from 54 lines to 17-line pure re-export (−69%)
  • @agent-relay/sdk/src/discovery.ts — new 38-line re-export barrel
  • @agent-relay/sdk/src/errors.ts — new 17-line re-export barrel

All re-export modules contain zero implementation logic. No stale duplicate class definitions remain.

2. Duplication Elimination ✅

~560 lines of duplication eliminated — claim verified. Net positive lines accounted for by 404 lines of new tests, 55 lines of re-export barrels, and the unrelated PR audit report.

3. Backwards Compatibility ✅

All public APIs preserved via re-exports. MCP consumers can still import from ./cloud.js and ./errors.js. SDK consumers get new exports at both barrel and subpath imports. package.json exports maps properly configured with types, CJS, and ESM.

4. Dependency Graph — No Cycles ✅

Verified all 5 package.json files. Proper DAG:

protocol (no @agent-relay deps)
config → protocol
utils → protocol, config
sdk → protocol, utils
mcp → config, protocol, utils

No circular dependencies.

5. Test Coverage ✅

  • 51 new tests across 3 files: consolidation verification (28), discovery unit tests (16), error class tests (7)
  • Existing MCP discover tests updated to reflect existsSync gate removal
  • 358 total tests, 0 failures (per CI)

6. Error Handling Improvements ✅

SDK client.ts now uses typed errors: ECONNREFUSED/ENOENTDaemonNotRunningError, other errors → ConnectionError. Enables instanceof pattern matching for consumers.

7. Minor Observations (Non-blocking)

  • discovery.ts doc comment says "consolidated here in the SDK" but it's in utils — cosmetic
  • reports/pr-audit-2026-01-30.md is unrelated to consolidation — consider separate PR
  • DEFAULT_SOCKET_PATH fallback in sdk/src/client.ts is fine for robustness

8. Trail Trajectory

Recorded: traj_87mr8y2pd0xb — 95% confidence.


Summary: Well-executed refactoring eliminating significant code duplication with clean dependency graph, maintained backwards compatibility, and thorough test coverage. Recommend merge.

Copy link
Copy Markdown
Member

@willwashburn willwashburn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

heck yeah

Agent Relay and others added 2 commits January 30, 2026 20:44
discoverSocket() now returns the determined socket path even when the
socket file doesn't exist yet (daemon may not have started). This fixes
cloud agents getting null/fallback paths when WORKSPACE_ID is set but
the daemon hasn't created the socket yet. Also updates hybrid-client.ts
to log socket source for debugging and not blindly fall back to local
relay.sock for cloud workspaces.

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Move duplicated logic from MCP package into @agent-relay/utils so both
SDK and MCP share a single implementation:

- Socket discovery (discoverSocket, detectCloudWorkspace, getCloudSocketPath,
  discoverAgentName) moved from MCP cloud.ts → utils/discovery.ts
- Error classes (RelayError, DaemonNotRunningError, etc.) moved from
  MCP errors.ts → utils/errors.ts
- MCP cloud.ts and errors.ts now thin re-exports from @agent-relay/utils
- SDK discovery.ts and errors.ts re-export from @agent-relay/utils
- SDK client.ts now uses discoverSocket() for smart socket resolution
  instead of hardcoded /tmp/agent-relay.sock
- SDK client.ts now uses typed errors (DaemonNotRunningError, ConnectionError)

Dependency changes:
- @agent-relay/utils now depends on @agent-relay/config (for findProjectRoot)
- @agent-relay/sdk now depends on @agent-relay/utils (for discovery + errors)
- @agent-relay/mcp now explicitly depends on @agent-relay/utils

Tests: 51 new tests for discovery, errors, and consolidation verification.
All existing tests pass (206 utils, 50 SDK, 102 MCP = 358 total).

No breaking changes - all public APIs preserved, MCP re-exports maintain
backwards compatibility.

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
@khaliqgant khaliqgant force-pushed the feature/mcp-sdk-consolidation branch from b477e46 to b2a03cc Compare January 30, 2026 20:44
@khaliqgant khaliqgant merged commit a2de4ee into main Feb 1, 2026
30 checks passed
@khaliqgant khaliqgant deleted the feature/mcp-sdk-consolidation branch February 1, 2026 10:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants