`read_code` rejects the integer symbol id that `search_symbols` just handed it, breaking the one documented zero-hop read #121

New issue

Closed

opened 2026-09-04 17:01:27 +02:00 by buildagent · 1 comment

buildagent commented

2026-09-04 17:01:27 +02:00

Member

Dogfood finding from #51, whose lane built the agent-task benchmark using our own tools throughout.

The defect

search_symbols returns rows whose id is a JSON integer. Every id-taking tool accepts it — find_callers, find_callees, find_references, change_impact, get_symbol.

read_code does not. It rejects the integer with invalid_argument_type.

And read_code's own description advertises exactly this call as the efficient path:

source bytes → read_code("path") for the whole file, read_code("path:120-160"), or read_code("<symbol_id>") — ONE call, no lookup hop

So the single documented no-lookup-hop path is the one call an agent cannot make by pasting the field it was just given. The workaround is to stringify, which an agent has no reason to guess and which the description does not mention.

Why this is worse than an ergonomic wart

The whole value proposition measured in #51 is round trips: 62 against ripgrep's 247. The zero-hop read is one of the mechanisms that buys that, and it is the mechanism most likely to be abandoned after one invalid_argument_type — an agent that hits a type error on the documented path will fall back to path:line-line, spending the lookup hop the design exists to avoid.

It also violates a rule this project applies elsewhere without exception: a field a tool emits must be usable as the field another tool takes. The handle work (#31/#32, cih2_ stable handles) exists precisely so identity round-trips across calls; this is the same contract broken in the simplest possible place.

The fix

Accept both shapes in read_code's target: the integer id as emitted, and the string form. The tolerant direction is the only safe one — an id that arrives as a number is a correct use of the documented API, and refusing it is the bug.

Then a gate in the shape this repo already uses for wire contracts: for every tool that takes a symbol id, the value search_symbols emits must be accepted verbatim. A test that checks read_code alone would go stale the first time a new id-taking tool is added — the same staleness that made #107's registry find six ungraded URIs where the filing named four.

Mutation: revert the tolerant parse; the registry test must go red naming read_code, not merely one e2e.

Found alongside #118 (a #[tool] method gets a falsely earned zero from search_symbols) — both are defects in the seam between what a tool emits and what the next tool can consume.

Dogfood finding from #51, whose lane built the agent-task benchmark using our own tools throughout. ## The defect `search_symbols` returns rows whose `id` is a **JSON integer**. Every id-taking tool accepts it — `find_callers`, `find_callees`, `find_references`, `change_impact`, `get_symbol`. **`read_code` does not.** It rejects the integer with `invalid_argument_type`. And `read_code`'s own description advertises exactly this call as the efficient path: > source bytes → `read_code("path")` for the whole file, `read_code("path:120-160")`, or **`read_code("<symbol_id>")` — ONE call, no lookup hop** So the single documented no-lookup-hop path is the one call an agent cannot make by pasting the field it was just given. The workaround is to stringify, which an agent has no reason to guess and which the description does not mention. ## Why this is worse than an ergonomic wart The whole value proposition measured in #51 is **round trips**: 62 against ripgrep's 247. The zero-hop read is one of the mechanisms that buys that, and it is the mechanism most likely to be abandoned after one `invalid_argument_type` — an agent that hits a type error on the documented path will fall back to `path:line-line`, spending the lookup hop the design exists to avoid. It also violates a rule this project applies elsewhere without exception: **a field a tool emits must be usable as the field another tool takes.** The handle work (#31/#32, `cih2_` stable handles) exists precisely so identity round-trips across calls; this is the same contract broken in the simplest possible place. ## The fix Accept both shapes in `read_code`'s `target`: the integer id as emitted, and the string form. The tolerant direction is the only safe one — an id that arrives as a number is a *correct* use of the documented API, and refusing it is the bug. Then a gate in the shape this repo already uses for wire contracts: **for every tool that takes a symbol id, the value `search_symbols` emits must be accepted verbatim.** A test that checks `read_code` alone would go stale the first time a new id-taking tool is added — the same staleness that made #107's registry find six ungraded URIs where the filing named four. Mutation: revert the tolerant parse; the registry test must go red naming `read_code`, not merely one e2e. ## Related Found alongside #118 (a `#[tool]` method gets a falsely *earned* zero from `search_symbols`) — both are defects in the seam between what a tool emits and what the next tool can consume.

buildagent commented

2026-09-04 18:28:21 +02:00

Author

Member

Fixed — and the registry found three tools, not the one this issue named.

read_code.target, get_symbol.id and get_dependencies.target are all string-declared union arguments ("<path> … or <symbol_id>", "a numeric symbol id … or path:line", "Path or symbol_id"). Every one of them rejected the integer search_symbols emits. The filing named only read_code — exactly the #107 pattern, where a registry over the ungraded URIs came back with six where the issue listed four.

The fix is a schema widening, not a special case

A SymbolIdOrString marker publishes "type": ["string","integer"], plus de_symbol_id_or_string, which normalises the integer to the decimal string the handlers already parse with str::parse::<i64>(). Widening the declaration teaches the pre-dispatch argcheck ladder, the published schema and serde in one move; downstream behaviour is unchanged.

One detail worth recording because it is the kind of thing that quietly disables a gate: the union type is inlined, never a $ref. A $ref makes argcheck::check_types step aside and stop grading that argument altogether — so the "tolerant" form would have been tolerant of everything, including genuinely wrong types.

The gate

crates/mcp-server/tests/symbol_id_args_registry.rs — every published tool must appear with an explicit classification of its symbol-id arguments, checked in both directions against tools/list. Six gates:

registry ↔ published parity;
classified arguments must be real schema properties;
the registry's argument names must equal SYMBOL_ID_ARGS in server.rs;
every classified argument's published type must admit an integer;
a runtime sweep passing the id verbatim as JSON into all 11 arguments;
a naming rule, so a property called id / *_id / *_ids cannot hide behind id_args: &[].

Where both spellings are published, the two calls must return byte-identical bodies.

Gate 6 is the one that makes this durable: without it, a future tool could add a symbol-id argument and declare itself as having none, which is precisely how a registry goes stale while staying green.

Mutations, run

Revert the tolerant parse on ReadCodeArgs::target:

`read_code`'s `target` takes a symbol id but is published as "string" — so the integer
`search_symbols` emits in its `id` field is rejected before dispatch with `invalid_argument_type`.
…
`read_code` REJECTED the integer `search_symbols` emitted, called as {"target":5}.
Reply: {"error":"invalid_argument_type","hint":"`read_code`'s `target` takes a string, not an integer…"}
test result: FAILED. 0 passed; 2 failed

Delete the read_code row from the registry: these published tools have no row in REGISTRY … ["read_code"].

Payload

The three type widenings cost ~9 tokens, and the same three descriptions were trimmed to more than pay for it — get_symbol.id's "(string-encoded)" had become actively wrong. Startup payload is 16,223, five tokens BELOW the 16,228 it started at, and 332 under the ceiling. project_overview untouched.

## Fixed — and the registry found **three** tools, not the one this issue named. `read_code.target`, **`get_symbol.id`** and **`get_dependencies.target`** are all string-declared union arguments (*"`<path>` … or `<symbol_id>`"*, *"a numeric symbol id … or `path:line`"*, *"Path or symbol_id"*). Every one of them rejected the integer `search_symbols` emits. The filing named only `read_code` — exactly the #107 pattern, where a registry over the ungraded URIs came back with six where the issue listed four. ### The fix is a schema widening, not a special case A `SymbolIdOrString` marker publishes `"type": ["string","integer"]`, plus `de_symbol_id_or_string`, which normalises the integer to the decimal string the handlers already parse with `str::parse::<i64>()`. Widening the *declaration* teaches the pre-dispatch argcheck ladder, the published schema and serde in one move; downstream behaviour is unchanged. One detail worth recording because it is the kind of thing that quietly disables a gate: **the union type is inlined, never a `$ref`.** A `$ref` makes `argcheck::check_types` step aside and stop grading that argument altogether — so the "tolerant" form would have been tolerant of everything, including genuinely wrong types. ### The gate `crates/mcp-server/tests/symbol_id_args_registry.rs` — every published tool must appear with an **explicit classification** of its symbol-id arguments, checked in both directions against `tools/list`. Six gates: 1. registry ↔ published parity; 2. classified arguments must be real schema properties; 3. the registry's argument names must equal `SYMBOL_ID_ARGS` in `server.rs`; 4. every classified argument's published type must admit an integer; 5. a **runtime sweep** passing the id **verbatim as JSON** into all 11 arguments; 6. a naming rule, so a property called `id` / `*_id` / `*_ids` cannot hide behind `id_args: &[]`. Where both spellings are published, the two calls must return **byte-identical** bodies. Gate 6 is the one that makes this durable: without it, a future tool could add a symbol-id argument and declare itself as having none, which is precisely how a registry goes stale while staying green. ### Mutations, run **Revert the tolerant parse on `ReadCodeArgs::target`:** ``` `read_code`'s `target` takes a symbol id but is published as "string" — so the integer `search_symbols` emits in its `id` field is rejected before dispatch with `invalid_argument_type`. … `read_code` REJECTED the integer `search_symbols` emitted, called as {"target":5}. Reply: {"error":"invalid_argument_type","hint":"`read_code`'s `target` takes a string, not an integer…"} test result: FAILED. 0 passed; 2 failed ``` **Delete the `read_code` row from the registry:** `these published tools have no row in REGISTRY … ["read_code"]`. ### Payload The three type widenings cost **~9 tokens**, and the same three descriptions were trimmed to more than pay for it — `get_symbol.id`'s "(string-encoded)" had become actively wrong. **Startup payload is 16,223, five tokens BELOW the 16,228 it started at**, and 332 under the ceiling. `project_overview` untouched.

buildagent referenced this issue

2026-09-04 18:29:36 +02:00

change_impact returns an empty, confident answer where find_callers finds 5 call sites — and nothing in its payload says why #122

buildagent closed this issue

2026-09-04 18:29:39 +02:00