Repository navigation
fix(opencode): surface MCP tools/list failure in status and logs - #20
Conversation
|
This PR doesn't fully meet our contributing guidelines and PR template. What needs to be fixed:
Please edit this PR description to address the above within 2 hours, or it will be automatically closed. If you believe this was flagged incorrectly, please let a maintainer know. |
|
Thanks for your contribution! This PR doesn't have a linked issue. All PRs must reference an existing issue. Please:
See CONTRIBUTING.md for details. |
|
The following comment was made by an LLM, it may be inaccurate: |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 4 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/opencode/src/mcp/index.ts">
<violation number="1" location="packages/opencode/src/mcp/index.ts:412">
P3: The new `failed to get tools` log in create() records the server under field `key`, but the identical log in McpCatalog.defs (used by watch() and authenticate()) records it under `clientName`. A Loki query filtering by one server field misses failures logged by the other path, which weakens the exact observability this PR is adding. Log the server name as `clientName` here to match defs()/fetch().</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| // status and a log line, instead of an unexplained "Failed to get tools". | ||
| const listed = mcpClient.getServerCapabilities()?.tools | ||
| ? yield* McpCatalog.listTools(mcpClient, mcp.timeout).pipe( | ||
| Effect.tapError((error) => Effect.logWarning("failed to get tools", { key, error: error.message })), |
There was a problem hiding this comment.
P3: The new failed to get tools log in create() records the server under field key, but the identical log in McpCatalog.defs (used by watch() and authenticate()) records it under clientName. A Loki query filtering by one server field misses failures logged by the other path, which weakens the exact observability this PR is adding. Log the server name as clientName here to match defs()/fetch().
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/opencode/src/mcp/index.ts, line 412:
<comment>The new `failed to get tools` log in create() records the server under field `key`, but the identical log in McpCatalog.defs (used by watch() and authenticate()) records it under `clientName`. A Loki query filtering by one server field misses failures logged by the other path, which weakens the exact observability this PR is adding. Log the server name as `clientName` here to match defs()/fetch().</comment>
<file context>
@@ -405,10 +405,14 @@ const layer = Layer.effect(
+ // status and a log line, instead of an unexplained "Failed to get tools".
+ const listed = mcpClient.getServerCapabilities()?.tools
+ ? yield* McpCatalog.listTools(mcpClient, mcp.timeout).pipe(
+ Effect.tapError((error) => Effect.logWarning("failed to get tools", { key, error: error.message })),
+ Effect.mapError((error) => new Error(`Failed to get tools: ${error.message}`)),
+ )
</file context>
| Effect.tapError((error) => Effect.logWarning("failed to get tools", { key, error: error.message })), | |
| Effect.tapError((error) => Effect.logWarning("failed to get tools", { clientName: key, error: error.message })), |
|
CI note: the two red jobs ( |
|
Pushed 7e6de95 fixing the red CI at its root instead of waiting it out:
|
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/opencode/src/id/id.ts">
<violation number="1" location="packages/opencode/src/id/id.ts:90">
P2: When a pre-wrap ID is within the one-hour skew window, this condition returns the current-era timestamp instead of unwrapping it, producing an error of about 795 days even though the ID is within one encoding wrap. `Truncate.cleanup` can then retain those old files for an extra retention window; pass a caller age/cutoff bound to disambiguate pre-wrap IDs from future-skewed IDs.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| const wrapped = BigInt("0x" + hex) / BigInt(0x1000) | ||
| const now = BigInt(Date.now()) | ||
| const candidate = (now / TIMESTAMP_WRAP_MS) * TIMESTAMP_WRAP_MS + wrapped | ||
| return Number(candidate > now + TIMESTAMP_SKEW_MS ? candidate - TIMESTAMP_WRAP_MS : candidate) |
There was a problem hiding this comment.
P2: When a pre-wrap ID is within the one-hour skew window, this condition returns the current-era timestamp instead of unwrapping it, producing an error of about 795 days even though the ID is within one encoding wrap. Truncate.cleanup can then retain those old files for an extra retention window; pass a caller age/cutoff bound to disambiguate pre-wrap IDs from future-skewed IDs.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/opencode/src/id/id.ts, line 90:
<comment>When a pre-wrap ID is within the one-hour skew window, this condition returns the current-era timestamp instead of unwrapping it, producing an error of about 795 days even though the ID is within one encoding wrap. `Truncate.cleanup` can then retain those old files for an extra retention window; pass a caller age/cutoff bound to disambiguate pre-wrap IDs from future-skewed IDs.</comment>
<file context>
@@ -69,12 +69,25 @@ export function create(prefix: string, direction: "descending" | "ascending", ti
+ const wrapped = BigInt("0x" + hex) / BigInt(0x1000)
+ const now = BigInt(Date.now())
+ const candidate = (now / TIMESTAMP_WRAP_MS) * TIMESTAMP_WRAP_MS + wrapped
+ return Number(candidate > now + TIMESTAMP_SKEW_MS ? candidate - TIMESTAMP_WRAP_MS : candidate)
}
</file context>
|
This pull request has been automatically closed because it was not updated to meet our contributing guidelines within the 2-hour window. Feel free to open a new pull request that follows our guidelines. |
A failed tools/list during MCP server creation was swallowed twice: McpCatalog.defs dropped the exception unlogged and create() collapsed it to a constant 'Failed to get tools' status. Fleet sandboxes have been reporting bedrock=failed with no way to see why (USE-2645). defs() now logs the underlying error (tagged with the server name, like the sibling fetch() helper), and create() lists tools via the strict McpCatalog.listTools so the real message lands in both a log line and the failed status. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
7e6de95 to
5a0ab51
Compare
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
|
This pull request has been automatically closed because it was not updated to meet our contributing guidelines within the 2-hour window. Feel free to open a new pull request that follows our guidelines. |
What
When an MCP server connects but its tool list fails to load, opencode currently throws the real error away and reports the string "Failed to get tools". The session then runs with zero tools from that server, for its entire life, and nobody can find out why.
This makes the failure diagnosable. It changes no control flow — the same sessions fail the same way — but the actual exception now reaches the logs and the server's status instead of being swallowed twice.
Why now
This is the root blocker on USE-2645, and it bit us in production again today.
In this thread, a Slack-triggered Replo agent was asked to file a ticket and answered "Linear isn't connected to this project, connect it in Integrations settings." Linear was connected — project scope, healthy, since July 31, never deleted. What had actually happened: the sandbox booted with
bedrock=failedtwo minutes earlier, so every bedrock tool was missing (Linear, Slack, Grafana, PostHog, the Replo DB), and the model rationalized an empty toolbox into a confident, wrong explanation. That cost the thread an afternoon chasing a Linear integration that was fine.The internal ops project behind every Slack-triggered session went from healthy through Aug 19 to 20 of 20 boots failing from Aug 20 onward. Fleet-wide it is roughly 1-9% of boots per day depending on the week.
The server side is provably innocent: bedrock-api serves
tools/listas a 200 in under 100ms and its own per-group registration degrades gracefully. The failure is client-side inlistTools, deterministic per sandbox, and content-keyed — which points at one malformed tool descriptor rejecting the whole strict parse. But that is a hypothesis, and it will stay a hypothesis until the swallowed exception is readable. This PR is what makes the next step possible rather than guesswork.Changes
McpCatalog.defsnow takes the server name (same argument order as the siblingfetch()helper) and logs the underlying error throughEffect.tapErrorbefore swallowing it, mirroring the patternfetch()already uses.create()lists tools through the newly exported strictMcpCatalog.listToolsand maps the failure toFailed to get tools: <real message>, so the per-server failed status thatGET /mcpreports carries the actual exception. It logs with the server key as well.The OAuth
authenticate()path keeps its combined!client || !listedhandling and its generic status, but picks up the newdefslog line for free.Once this reaches the fleet,
failed to get toolsin Loki names the real exception within hours, and USE-2645 becomes a fixable bug instead of an open question.Testing Done
New regression test in
lifecycle.test.ts— "failed tool discovery surfaces the underlying error in the status": the server'stools/listthrowsboom-tools-list, and the test asserts the failed status contains it. Verified red without the src change (generic string) and green with it.bun test test/mcp/— 63 pass, 0 fail, under bun 1.4.0.typecheckclean; the only tsc errors in the repo are pre-existing ones inpackages/tui/src/component/dialog-move-session.tsx, untouched here.Rebased onto current
dev. The branch previously carried a second, unrelated commit hardeningIdentifier.timestampagainst the 48-bit wrap; it only existed to unblock the date-sensitive truncation test, which #24 has since fixed a different way, andIdentifier.timestampnow has no callers insrc/. Dropped to keep this PR single-purpose — recoverable at7e6de952if we want it separately.Risk and Rollback
Diagnostics only. No behavior change for a healthy server, and a failing server fails identically — just with a readable reason. Rollback is a revert.
🤖 Generated with Claude Code
Summary by cubic
Surfaces MCP tools/list failures in
opencodeserver status and logs so broken tool discovery is diagnosable. Previously, list errors were swallowed (generic “Failed to get tools”, no logs); now the real error is logged and included in the per-server failed status (addresses USE-2645).McpCatalog.defs(clientName, client, timeout?)now logs the underlying error and returns undefined on failure; callers must pass the server name.McpCatalog.listTools(client, timeout?);create()uses it, logs with the server key, and maps failures to “Failed to get tools: ”.defs(name, client, timeout)to tag logs with the server; OAuth path keeps its combined handling but now benefits from thedefslog.Written for commit 5a0ab51. Summary will update on new commits.