fix(loader): name the MCP tool a load failure belongs to #11

Merged

thklein merged 2 commits from feat/mcp-load-error-tool-name into main 2026-09-15 05:57:32 +00:00

Conversation 1 Commits 2 Files changed 2 +123 -1

thklein commented 2026-09-15 05:06:43 +00:00

Owner

Copy link

The problem

An MCP load failure cannot say which tool it belongs to.

mcp.(*Local).Load reports the server key, and the one-server-per-tool
configuration bpai generates always keys it "default". So a program carrying
several MCP tools produces the same sentence whichever one failed:

error loading MCP tools: failed to load MCP session for server default: ...

processMCP has t.Name in hand at the wrap site and drops it.

Downstream, that forces a caller to treat every candidate as the culprit. In
bpai, markMCPOAuthWallHit marks every unconnected oauth-mode MCP tool on the
agent when a load 401s, because it genuinely cannot narrow. An agent carrying a
stale-API-key MCP server alongside an unrelated unconnected oauth-mode server
marks the oauth one every turn; the user connects and the run still fails.

The change

Append the tool name to the existing wrap:

error loading MCP tools for https-crm-basepeak-dev-mcp: failed to load MCP session for server default: ...

Appended, not interpolated, so error loading MCP tools is still a prefix.
Consumers match that text rather than an error type (the failure crosses the
sdkserver boundary as a string), and both shapes have to keep matching across a
pin bump in either direction so a rollback does not silently stop recognising
it. bpai's matcher uses strings.Contains, and a test case for the new shape
lands alongside the pin bump there.

An unnamed tool (inline MCP, no Name) keeps the plain wrap rather than
producing ... for : ....

Tests

pkg/loader/mcp_error_test.go, driving processMCP against a failing loader:

• the failing tool is identifiable, the error loading MCP tools prefix
  survives, and the cause stays unwrappable (errors.Is)
• an unnamed tool keeps the plain wrap
• a non-MCP program passes through untouched and never calls the loader

Plus one end-to-end case through ProgramFromSource, which closes the link the
others assume rather than prove: that the name a caller puts on a ToolDef
survives the SDK's "#!GPTSCRIPT" + JSON serialization and the parse back into
types.Tool. Dropped anywhere on that path, the wrap would still compile and
still read plausibly while naming nothing, and a caller narrowing on the name
would silently never match. Mutation-checked: inverting the name guard fails all
three.

go test ./pkg/loader/ is clean. pkg/loader/github fails on unmodified main
in this environment too (it fetches from raw.githubusercontent.com); unrelated.

Follow-up in bpai

Bumping the pin and narrowing markMCPOAuthWallHit to the named tool is a
separate PR there.

## The problem An MCP load failure cannot say which tool it belongs to. `mcp.(*Local).Load` reports the server **key**, and the one-server-per-tool configuration bpai generates always keys it `"default"`. So a program carrying several MCP tools produces the same sentence whichever one failed: ``` error loading MCP tools: failed to load MCP session for server default: ... ``` `processMCP` has `t.Name` in hand at the wrap site and drops it. Downstream, that forces a caller to treat every candidate as the culprit. In bpai, `markMCPOAuthWallHit` marks *every* unconnected oauth-mode MCP tool on the agent when a load 401s, because it genuinely cannot narrow. An agent carrying a stale-API-key MCP server alongside an unrelated unconnected oauth-mode server marks the oauth one every turn; the user connects and the run still fails. ## The change Append the tool name to the existing wrap: ``` error loading MCP tools for https-crm-basepeak-dev-mcp: failed to load MCP session for server default: ... ``` **Appended, not interpolated**, so `error loading MCP tools` is still a prefix. Consumers match that text rather than an error type (the failure crosses the sdkserver boundary as a string), and both shapes have to keep matching across a pin bump in either direction so a rollback does not silently stop recognising it. bpai's matcher uses `strings.Contains`, and a test case for the new shape lands alongside the pin bump there. An unnamed tool (inline MCP, no `Name`) keeps the plain wrap rather than producing `... for : ...`. ## Tests `pkg/loader/mcp_error_test.go`, driving `processMCP` against a failing loader: - the failing tool is identifiable, the `error loading MCP tools` prefix survives, and the cause stays unwrappable (`errors.Is`) - an unnamed tool keeps the plain wrap - a non-MCP program passes through untouched and never calls the loader Plus one end-to-end case through `ProgramFromSource`, which closes the link the others assume rather than prove: that the name a caller puts on a `ToolDef` survives the SDK's `"#!GPTSCRIPT" + JSON` serialization and the parse back into `types.Tool`. Dropped anywhere on that path, the wrap would still compile and still read plausibly while naming nothing, and a caller narrowing on the name would silently never match. Mutation-checked: inverting the name guard fails all three. `go test ./pkg/loader/` is clean. `pkg/loader/github` fails on unmodified `main` in this environment too (it fetches from raw.githubusercontent.com); unrelated. ## Follow-up in bpai Bumping the pin and narrowing `markMCPOAuthWallHit` to the named tool is a separate PR there.

thklein added 1 commit 2026-09-15 05:06:43 +00:00

fix(loader): name the MCP tool a load failure belongs to ... 90d778a158

mcp.(*Local).Load reports the server KEY, and bpai's one-server-per-tool
configuration always keys it "default", so a program carrying several MCP tools
produced the same sentence whichever one failed. A caller could not tell a stale
API key on one server from an unauthenticated second server, and had to treat
every candidate as the culprit.

Append the tool name to the existing wrap rather than interpolating it, so
"error loading MCP tools" stays a prefix. Consumers match that text (it does not
survive the sdkserver boundary as a typed error), and both shapes have to keep
matching across a pin bump in either direction.

thklein commented 2026-09-15 05:23:28 +00:00

Author

Owner

Copy link

CI note: test (ubuntu-22.04) and validate-docs fail on this PR for a reason that predates it. make tidy cannot resolve basepeak-ai/broadcaster, a private module, because the runner has no credentials:

fatal: could not read Username for 'https://git.basepeak.dev': terminal prompts disabled
make: *** [Makefile:12: tidy] Error 1

Both jobs fail identically on every recent commit in this repo, merged ones included (runs #197/#198 on 7738a4c, #192/#193 on the PR before it). check-label fails on every pull_request event for the same structural reason it passed on push.

Locally, go test ./pkg/loader/ is clean. pkg/loader/github fails on unmodified main here too — it fetches from raw.githubusercontent.com.

CI note: `test (ubuntu-22.04)` and `validate-docs` fail on this PR for a reason that predates it. `make tidy` cannot resolve `basepeak-ai/broadcaster`, a private module, because the runner has no credentials: ``` fatal: could not read Username for 'https://git.basepeak.dev': terminal prompts disabled make: *** [Makefile:12: tidy] Error 1 ``` Both jobs fail identically on every recent commit in this repo, merged ones included (runs #197/#198 on `7738a4c`, #192/#193 on the PR before it). `check-label` fails on every `pull_request` event for the same structural reason it passed on push. Locally, `go test ./pkg/loader/` is clean. `pkg/loader/github` fails on unmodified `main` here too — it fetches from raw.githubusercontent.com.

thklein referenced this pull request from a commit 2026-09-15 05:40:31 +00:00

fix(mcp): mark only the MCP tool the load failure names

thklein added 1 commit 2026-09-15 05:42:12 +00:00

test(loader): pin that the ToolDef name reaches processMCP through the SDK ... 03425e9642

The unit test above assumed it rather than proved it. bpai builds the def as
mcp.ToolDef(toolRef.Name, cfg) and the SDK serializes it as "#!GPTSCRIPT" +
JSON before the loader parses it back. If Name were dropped anywhere on that
path the wrap would still compile and still read plausibly while naming nothing,
and a caller narrowing on the name would silently never match.

thklein merged commit b9cd9755cf into main 2026-09-15 05:57:32 +00:00

thklein deleted branch feat/mcp-load-error-tool-name 2026-09-15 05:57:32 +00:00

thklein referenced this pull request from a commit 2026-09-15 05:57:33 +00:00

fix(loader): name the MCP tool a load failure belongs to (#11)

thklein referenced this pull request from a commit 2026-09-15 06:24:55 +00:00

fix(mcp): list an auth-walled MCP server so its connect card can render (#1000)

thklein referenced this pull request from a commit 2026-09-15 06:47:56 +00:00

fix(mcp): mark only the MCP tool the load failure names

thklein referenced this pull request from a commit 2026-09-15 06:52:49 +00:00

fix(mcp): mark only the MCP tool the load failure names (#1003)

thklein referenced this pull request 2026-09-25 09:21:20 +00:00

fix(loader): the duplicate-tool-name error names one side twice, at line 0 #12

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels   

area/engine

  

clarity/clear

  

enhancement

  

security

  

size/M

No labels

area/engine

clarity/clear

enhancement

security

size/M

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

1 participant

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference

basepeak-ai/gptscript!11

No description provided.