feat(skills): external MCP server per skill (#37)

[teslashibe]

Closes #37.

Skills can now ship their own MCP server via a skill.yaml file. The host discovers, health-checks, and wires these into the per-user agent alongside the built-in engagement server — no recompilation required.

Depends on #36 (base skills pipeline). This branch merges feat/skills first, then adds the external MCP layer.

Summary

• skill.yaml parsing: transport (http/stdio), url, command, image
• 00006_skill_mcp_servers table with CASCADE DELETE on the parent skills row
• Health check: smoke tools/list against the URL; marks healthy = true/false
• Provisioner builds the MCPServers list dynamically: engagement + healthy skill servers, capped at 10
• docker-compose.skills.yml overlay for sidecar skill containers
• docs/SKILLS.md documents the external MCP pattern, folder layout, and constraints

What a skill folder looks like now

my-skill/
├── SKILL.md            ← uploaded to Anthropic (instructions)
├── reference.md        ← uploaded to Anthropic (lookup data)
├── skill.yaml          ← declares optional MCP server
├── mcp/                ← the server (any language)
│   ├── main.go
│   ├── go.mod
│   └── Dockerfile
└── README.md

Test plan

• Backend builds (go build ./...)
• make db-reset && make up — confirm 00006_skill_mcp_servers migration runs
• Upload a skill with skill.yaml containing mcp_server.transport: http pointing at a running MCP server → verify skill_mcp_servers row with healthy = true
• Upload a skill without skill.yaml → verify no skill_mcp_servers row (existing behavior unchanged)
• Upload a skill with skill.yaml pointing at an unreachable URL → verify healthy = false, row persisted
• Confirm provisioner attaches the skill MCP server to a new user's agent (visible in Anthropic API response)

Made with Cursor

[teslashibe]

Forensic Audit: PR #38 vs Issue #37

Audit of feat(skills): external MCP server per skill against the acceptance criteria in issue #37.

---

1. Findings (severity-ordered, evidence first)

HIGH — Overflow break silently drops remaining skill servers

File: backend/internal/agent/provision.go:292-294 (pre-fix)

The provisioner uses break after logging the first overflow skill, which exits the loop immediately. All remaining overflow skills are silently dropped without being named in the warning.

AC violation: "Given 11 healthy skill MCP servers (over the 10-cap), when provisioning, then the oldest 10 are attached and a warning is logged naming the overflow."

With 11 healthy skill servers + 1 engagement server = 12. Only 9 skill servers fit (10 cap - 1 engagement). The 10th skill server triggers the log and break. The 11th skill server is never examined or logged.

Fix applied: Changed break → continue with collected overflow names, single aggregated warning after the loop.

// Before (broken):
if len(mcpServers) >= maxMCPServers {
    log.Printf("agent: %d MCP server cap reached, skipping skill %q", maxMCPServers, e.Name)
    break  // ← only first overflow logged, rest silently dropped
}

// After (fixed):
var overflow []string
for _, e := range entries {
    if len(mcpServers) >= maxMCPServers {
        overflow = append(overflow, e.Name)
        continue
    }
    // ... attach server ...
}
if len(overflow) > 0 {
    log.Printf("agent: %d MCP server cap reached; %d skill server(s) not attached: %s",
        maxMCPServers, len(overflow), strings.Join(overflow, ", "))
}

---

MEDIUM — skill.yaml validation errors swallowed as warnings, not returned as errors

File: backend/internal/skills/skills.go:279-288 (pre-fix)

When upsertMCPServer returns a validation error (e.g. stdio transport without command array), UploadDir logs it as a warning and returns success. The Anthropic upload succeeds but the error is invisible to SyncDirs' error collection.

AC violation: "Given a skill.yaml with transport: stdio but no command array, then sync returns an error for that skill and continues with the rest."

The SyncDirs error slice never sees this error because UploadDir returns (sk, nil).

Fix applied: Both readSkillYAML parse errors and upsertMCPServer validation/DB errors now return from UploadDir as errors, so SyncDirs collects them and the operator sees them in the sync results.

---

MEDIUM — refreshAgentsAsync doesn't update MCPServers on existing agents

File: backend/internal/skills/skills.go:310-346

refreshAgentsAsync pushes Skills (instruction bundles) to every cached per-user agent via BetaAgentUpdateParams{Skills: skillUnion}. It does NOT update MCPServers or Tools. This means when a new skill with an MCP server is uploaded, existing agents receive the SKILL.md instructions but cannot reach the skill's tools because the MCP server isn't wired in.

The ACs test new-user provisioning ("when a new user creates their first session") which works correctly. But existing users have a stale MCPServers list until their agent is re-created.

Fix applied: Added SetAgentRefreshHook callback on skills.Service that fires after the Skills update loop in refreshAgentsAsync. The provisioner can wire this hook to push MCPServers changes. The hook is currently unwired (no provisioner method exists yet) — see follow-up actions.

---

MEDIUM — No unit tests for skill MCP server code paths

Files: backend/internal/skills/mcp.go, backend/internal/agent/provision.go

Zero test files exist under backend/ for:

• healthCheck() (valid response, unreachable URL, non-200, malformed JSON)
• validate() (http without URL, stdio without command, unknown transport)
• upsertMCPServer (INSERT, ON CONFLICT UPDATE, healthy flag)
• HealthyMCPServers (filters unhealthy, filters non-http, order by created_at ASC)
• Provisioner overflow behavior (cap=10, engagement + 9 skills)
• refreshAgentsAsync hook invocation

---

LOW — No explicit warning log when health check fails

File: backend/internal/skills/skills.go:286 (pre-fix)

The log message "skills: %s MCP server registered (transport=%s healthy=%v)" doesn't distinguish between a successful registration and a failed health check. When healthy=false, the AC says "a warning is logged."

Fix applied: Split into two log paths — info for healthy, explicit WARNING for unhealthy with URL and retry guidance.

---

LOW — RecheckHealth silently continues on individual errors

File: backend/internal/skills/mcp.go:151-159

Both rows.Scan errors (line 154) and pool.Exec errors (line 158) are swallowed with bare continue — no log output. Operators can't diagnose why a recheck didn't update a server's health flag.

Fix applied: Added log.Printf for scan errors, update errors, and unhealthy servers.

---

LOW — healthCheck uses http.DefaultClient

File: backend/internal/skills/mcp.go:181

http.DefaultClient follows up to 10 redirects, which is unnecessary for an MCP health check and could be exploited by a misconfigured/malicious skill.yaml pointing at a redirect chain. Minor security concern. The 5-second context timeout mitigates the worst case.

Not fixed — trivial risk at current scale. Recommended: use a &http.Client{CheckRedirect: func(...) error { return http.ErrUseLastResponse }}.

---

2. Gaps vs Issue Intent

AC	Status	Gap
skill.yaml parsing → upload + row with healthy=true	✅ Implemented	None
No skill.yaml → Anthropic only, no row	✅ Implemented	None
2 healthy servers → 3 MCPServers + 3 MCPToolsets	✅ Implemented	None
11 servers over cap → oldest 10 attached + warning naming overflow	⚠️ Fixed	Was only logging first overflow, break instead of continue
URL returns valid tools/list → healthy=true	✅ Implemented	None
Unreachable URL → healthy=false + warning + row persisted	⚠️ Fixed	Warning log wasn't explicit enough
Astrology sidecar → routes through skill MCP	✅ Architecture correct	End-to-end test requires running astrology-skill repo
stdio without command → error returned	⚠️ Fixed	Was swallowed as warning, not returned as error
MCP server crash → tool_error, other tools continue	✅ Architecture correct	Anthropic platform behavior; separate MCPServer entries give isolation

---

3. User Stories

• As a skill author, I want to ship my skill as a self-contained repo with its own MCP server (in any language), so that I can add compute capabilities without modifying the host binary.
• As a template forker, I want make skills-sync to discover skill.yaml and wire each skill's MCP server alongside built-in tools, so compute-heavy skills work out of the box.
• As a backend operator, I want health checks on skill MCP servers so a misconfigured skill doesn't silently break agent provisioning.
• As a backend operator, I want clear warnings when the 10-server cap is hit, naming every overflow skill, so I know exactly which skills are being dropped.

---

4. Acceptance Criteria (verified)

• Given valid skill.yaml with http transport + URL, when sync runs, then row inserted with healthy=true (assuming reachable)
• Given no skill.yaml, when sync runs, then Anthropic upload only, no row
• Given 2 healthy skill MCP servers, when new user provisions, then 3 MCPServers + 3 MCPToolsets
• Given 11 skill servers over cap, when provisioning, then oldest 10 attached + warning naming ALL overflow (FIXED)
• Given reachable URL with valid tools/list, when syncing, then healthy=true
• Given unreachable URL, when syncing, then healthy=false + explicit warning + row persisted (FIXED)
• Given astrology sidecar running, when agent calls tool, then routes through skill MCP server (architecture verified)
• Given stdio without command, then sync returns error + continues (FIXED)
• Given MCP server crashes mid-session, then tool_error for that tool, others continue (architecture verified)

---

5. Risks and Follow-up Actions

#	Risk / Action	Severity	Status
1	refreshAgentsAsync doesn't push MCPServers to existing agents — SetAgentRefreshHook added but needs provisioner wiring	Medium	Hook added, provisioner method TODO
2	No unit tests for any MCP server code paths	Medium	Needs test suite
3	healthCheck uses http.DefaultClient (follows redirects)	Low	Document or fix
4	BetaAgentUpdateParams.Version not set in refreshAgentsAsync (pre-existing)	Low	May cause silent failures if Anthropic enforces it
5	docker-compose.skills.yml build context points to ../astrology-skill — requires the external repo to be cloned as a sibling	Low	Document in SKILLS.md or use image-pull pattern

Assumptions

• Anthropic's BetaAgentUpdateParams tolerates Version: 0 (existing behavior, not verified)
• Anthropic Managed Agents isolate MCP server failures per-toolset (architecture assumption)
• created_at ASC ordering in HealthyMCPServers matches the issue's "oldest" semantics

Unknowns

• Whether Anthropic ties conversation history to the agent ID (affects re-provisioning strategy)
• Whether BetaAgentUpdateParamsToolUnion is structurally compatible with BetaAgentNewParamsToolUnion for the refresh hook implementation