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

New issue

Open

opened 2026-09-25 09:21:20 +00:00 by thklein · 0 comments

thklein commented

2026-09-25 09:21:20 +00:00

Owner

When two tools collide, pkg/loader/loader.go:314-318 reports:

return nil, parser.NewErrLine(tool.Source.Location, tool.Source.LineNo,
    fmt.Errorf("duplicate tool name [%s] in %s found at lines %d and %d", tool.Name, tool.Source.Location,
        tool.Source.LineNo, existing.Source.LineNo))

For tools that came out of an MCP bundle, both line numbers are 0 and both
locations are the same string, so the message reads

duplicate tool name [bpcrm_search] in found at lines 0 and 0

which names one side twice and locates neither.

Why the zeros

pkg/mcp/loader.go never sets Source on the tools it synthesises — there is
no Source assignment in the file at all. loader.go:262-264 then fills in
part of it for every tool:

tool.Source.Location = base.Location
tool.Source.Repo     = base.Repo

Source.LineNo is not among them, so it keeps its zero value. An MCP-expanded
tool has no line to point at, because it was never parsed from a file — and
Location is overwritten with base.Location for both sides of the collision,
so the two operands are identical by construction.

Why it is worth fixing

This is the diagnostic for a whole defect class, and it currently carries no
information. bpai#1077 is the live example: gptscript expands a bundle into
<toolRefName>_<rawName> (pkg/mcp/loader.go:150, namespacedToolName)
inside processMCP (loader.go:251) — before this check — so an agent or a
project custom tool named bpcrm_search collides with a sub-tool of the
bpcrm bundle. What the user sees is that nothing in the project works, and
the error names neither the bundle nor their own tool as the other side.

Fixing the message does not fix bpai#1077 — the collision still happens.
It turns "nothing works and the message is useless" into a five-second
diagnosis, for that bug and for every other member of the class.

Suggested shape

Two pieces of information are missing and both are available at the call site:

Which side is which. existing and tool are distinct objects; the
message should describe both, not print one twice.
Where an MCP tool came from. A synthesised tool has no line but it does
have a bundle — the namespace namespacedToolName prefixed it with. Naming
that ("from MCP bundle bpcrm" vs "declared in <file> line N") is what
makes the message actionable.

Degrading gracefully when LineNo == 0 is the minimum: printing "lines 0 and
0" is worse than omitting the line numbers entirely.

Precedent in this fork: b9cd975 ("fix(loader): name the MCP tool a load
failure belongs to", #11) is the same kind of fix on the neighbouring error
path.

bpai#1077 — the collision itself, still undecided; it needs a call on whether
render may reserve the namespaced child names from Status.Tool.Params, at
the cost of making program assembly depend on controller status that is stale
by construction. This issue is independent of that decision and worth doing
either way.

When two tools collide, `pkg/loader/loader.go:314-318` reports: ```go return nil, parser.NewErrLine(tool.Source.Location, tool.Source.LineNo, fmt.Errorf("duplicate tool name [%s] in %s found at lines %d and %d", tool.Name, tool.Source.Location, tool.Source.LineNo, existing.Source.LineNo)) ``` For tools that came out of an MCP bundle, both line numbers are **0** and both locations are **the same string**, so the message reads > duplicate tool name [bpcrm_search] in <location> found at lines 0 and 0 which names one side twice and locates neither. ## Why the zeros `pkg/mcp/loader.go` never sets `Source` on the tools it synthesises — there is no `Source` assignment in the file at all. `loader.go:262-264` then fills in part of it for every tool: ```go tool.Source.Location = base.Location tool.Source.Repo = base.Repo ``` `Source.LineNo` is not among them, so it keeps its zero value. An MCP-expanded tool has no line to point at, because it was never parsed from a file — and `Location` is overwritten with `base.Location` for both sides of the collision, so the two operands are identical by construction. ## Why it is worth fixing This is the diagnostic for a whole defect class, and it currently carries no information. bpai#1077 is the live example: gptscript expands a bundle into `<toolRefName>_<rawName>` (`pkg/mcp/loader.go:150`, `namespacedToolName`) inside `processMCP` (`loader.go:251`) — *before* this check — so an agent or a project custom tool named `bpcrm_search` collides with a sub-tool of the `bpcrm` bundle. What the user sees is that nothing in the project works, and the error names neither the bundle nor their own tool as the other side. Fixing the message does **not** fix bpai#1077 — the collision still happens. It turns "nothing works and the message is useless" into a five-second diagnosis, for that bug and for every other member of the class. ## Suggested shape Two pieces of information are missing and both are available at the call site: 1. **Which side is which.** `existing` and `tool` are distinct objects; the message should describe both, not print one twice. 2. **Where an MCP tool came from.** A synthesised tool has no line but it does have a bundle — the namespace `namespacedToolName` prefixed it with. Naming that ("from MCP bundle `bpcrm`" vs "declared in `<file>` line N") is what makes the message actionable. Degrading gracefully when `LineNo == 0` is the minimum: printing "lines 0 and 0" is worse than omitting the line numbers entirely. Precedent in this fork: `b9cd975` ("fix(loader): name the MCP tool a load failure belongs to", #11) is the same kind of fix on the neighbouring error path. ## Related - bpai#1077 — the collision itself, still undecided; it needs a call on whether render may reserve the namespaced child names from `Status.Tool.Params`, at the cost of making program assembly depend on controller status that is stale by construction. This issue is independent of that decision and worth doing either way.