Conversation
Acting on a full-surface review of the eight kegg_* tools.
P0 — kegg_pathway_graph could exceed the 1 MB MCP transport limit, and the
caller then received NOTHING: the rejection happens at the transport layer, so
even the truncation note meant to aid recovery was lost.
Root cause was worse than reported. The review inferred that metabolic_gaps was
not counted toward the cap; in fact `_bounded`'s dict branch only LABELLED an
oversized payload — it set payload["truncated"] and serialized everything anyway
— so the cap was decorative for ALL FOUR dict-returning tools, not just for one
section. The branch now genuinely shrinks, dropping named sections in priority
order and reporting {returned, total} per section, with `stats` untouched so a
trimmed response still states the real node/edge/gap counts.
Fixing it surfaced two bad calls of my own, both found by measuring rather than
by reasoning:
* the first drop-order dropped metabolic_gaps FIRST, discarding all 25 of
hsa00010's gaps to save 2 KB — sacrificing the tool's unique answer to
protect the bulk. Order is now bulkiest-and-tunable first (edges, nodes),
unique-and-cheap last (gaps).
* the 90 KB cap was inherited from a row-listing tool and is far too tight for
a graph — hsa05200's 255 nodes alone are 77 KB because each carries its
paralog member list. It was firing on ORDINARY maps and cutting hsa05200
from 311 edges to 43: bounded, and useless. Graph payloads now have their own
250 KB cap (~60k tokens, still far under the transport limit).
All six validation maps now return complete untruncated graphs (hsa00010 gets
its 25 gaps back, hsa05200 its 311 edges). New `max_gaps` bounds gaps at the
source; the true count is always stats.metabolic_gap_count.
P1 — two false docstring statements, both mine, both found by testing:
* "global/overview maps like 01100 have no KGML" — they do, hsa01100 parses to
6,382 nodes. Corrected here and in the empty-KGML error message.
* the glycolysis example was measured on ko00010 and written up as
"glycolysis". On hsa00010, C00031 (D-Glucose) is an ISOLATED node returning
nothing at ANY max_length; the chain starts at C00267 (alpha-D-Glucose).
The example is now the measured hsa00010 one, plus the anomer trap.
kegg_pathway_paths also gained `isolated_endpoints`, so a degree-0 endpoint
is no longer indistinguishable from "merely far" — that ambiguity is what
made the wrong example look plausible for so long.
P2 — enzyme-detour routes over an identical reaction sequence are collapsed
(`enzyme_detours_collapsed`), so max_paths is not spent on the same chemistry
drawn twice; different reactions between the same pair stay distinct. kegg_find
gained `entry_id` alongside `entry`, and the identifier-form policy for all four
ID-returning tools is now stated ONCE in the module docstring.
Tests: 392 pass. The P0 regression is a SYNTHETIC global-map-shaped KGML (2,500
isolated ortholog boxes + 400 map pointers) — no test may touch rest.kegg.jp, and
the shape is what matters, not the data. Every case in the review's
"must not break" table re-verified against the cached maps.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Second review round. P0-2 is the swing back from my own previous fix, and the
pair makes the actual principle clear.
Round one: a fixed "gaps first" drop order threw away all 25 of hsa00010's
metabolic gaps to save 2 KB — the tool's unique answer spent to protect bulk.
Round two: reversing it to "edges first" let a whole-metabolism map's 186 KB of
gaps (84% of the payload) push `edges` to ZERO under raised count caps. A
pathway graph with no graph in it is worse than an error: it does not look like
one, and reads as "these molecules are unconnected".
So the rule is not which section is more precious. It is that the section which
OCCUPIES the budget must be the one that pays, and the answer must keep a floor.
The backstop now reduces in two tiers — supporting detail (map_links, groups,
metabolic_gaps) before the graph itself (edges, nodes, floored at 50 rows) — and
WITHIN a tier takes the biggest section first, so bytes are reclaimed where they
actually are rather than by zeroing something small for nothing.
Measured on a synthetic hsa01100-shaped map (2,073 gaps / 169 map pointers / 900
genes), raised caps: edges 0 -> 899.
Also fixed the diagnostics, which had the perverse property that the section
responsible for the truncation was the ONLY one missing from the report — it had
returned == total, so nothing flagged it, leaving "why are there so few edges?"
unanswerable. `truncated` now carries:
* `reasons` as a LIST, so a count-cap trim and a size-backstop firing stay
distinguishable instead of collapsing into one string (the previous commit
lost that distinction);
* `capped_by` per section — "count" / "size_budget" / null;
* `section_bytes` when the size backstop fires, naming the occupant.
Default arguments are unchanged and pinned by the same test (nodes 400, gaps
100, edges > 0, no size firing). All six cached validation maps still return
complete untruncated graphs. 393 tests pass; the new regression is synthetic, as
no test may touch rest.kegg.jp.
Round-one items P0-1 / P1-1 / P1-2 / P2-1 / P2-2 were all confirmed fixed by the
reviewer, including their correction that KEGG itself varies prefixed vs bare by
database — which the shipped docstring already stated correctly.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Third review round. P0-3 is a correctness regression I introduced in the second, and it and P1-3 have one cause and one fix, exactly as the review predicted. Trimming `nodes` and `edges` as two independent flat lists let the size backstop cut a node that a surviving edge still referenced. Measured on a synthetic whole-metabolism map with raised caps: ALL 50 returned edges dangled, and every one of 1,062 returned nodes looked isolated on a densely connected map. Both are wrong ANSWERS rather than small ones, and the second is the dangerous kind — "this map is nearly disconnected" is a plausible reading of it, and nothing about the response says otherwise. The unit of reduction is now the NODE SET: a binary search finds the largest degree-ordered node prefix whose INDUCED subgraph fits the budget. Edges are derived from the returned nodes rather than clipped alongside them, so the invariant holds by construction rather than by assertion — verified across all six validation maps and both capped paths, dangling 0 everywhere. P1-4, the default-behaviour regression, had a separate cause worth recording: the budget was computed against each section's UNREDUCED size, so `edges` at its full 8,124-row cost (~370 KB) made the budget look spent and the supporting sections were zeroed — while the response actually came to 222 KB against a 250 KB cap, and the 100 gaps it discarded cost 11.5 KB. metabolic_gaps is this tool's headline output and it had disappeared from every organism global map at default arguments, which is precisely where it means most. Budget is now computed on what is really being returned, and the (already count-capped, ~22 KB) supporting sections are reserved BEFORE the graph is fitted into the remainder. Also corrected a mislabel the review did not ask about: an induced edge count bound by max_nodes was reported as capped_by "size_budget", which tells a caller to narrow the question when raising max_nodes is what helps. Edges now inherit the node limit's reason and say they are the induced subgraph. And section_bytes_if_complete is emitted whenever the graph is reduced, not only when the backstop fires, so the section that DROVE the reduction is visible even though it has returned == total. Acceptance, synthetic hsa01100-shaped map: default nodes 400 edges 356 gaps 100 links 100 dangling 0 raised nodes 105 edges 70 gaps 2073 links 100 dangling 0, isolated 45% 393 tests pass. Round-two items P0-2's four requirements remain satisfied. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Raising every cap on kegg_pathway_graph returned a SMALLER graph than the defaults. On hsa01100, max_gaps=5000 let 1,555 metabolic gaps take 74% of the payload, so nodes fell to the 50-node floor — against the 191 nodes / 560 edges the same map returns at the defaults. No contract was broken (max_* are ceilings, not floors), but the tool did the opposite of what asking for more means, and the hint's "raise max_nodes/max_edges/max_gaps" was actively wrong: the only move that bought graph was LOWERING max_gaps. The caps are ceilings on ONE shared budget, and the supporting sections were reserving from it first. nodes+edges now take _GRAPH_BUDGET_SHARE (half) before metabolic_gaps/map_links may spend any of it; those are then count-capped as before and fitted into the remainder, biggest-first. Same case now returns 138 nodes / 346 edges with 876 gaps. Defaults are unaffected — gaps are ~10% of the payload there, well inside the remainder — so hsa01100 still returns 191/560/100 and hsa00010 keeps all 25 of its gaps. A section cut for the reserve reports capped_by "size_budget", not "count": the report must never suggest raising a cap the caller already maxed out. And when the supporting sections hold enough budget to matter (>25% of what the graph costs), the hint says to lower max_gaps and gives the byte split. Strict monotonicity against the defaults is deliberately NOT the guarantee: a caller who typed max_gaps=5000 asked for gaps, and honoring that inside the remaining half still costs some graph (138 < 191). The guarantee is a floor — the graph can no longer be starved — and the hint names the move that recovers the rest. Tests: the three invariants that must hold on EVERY return (no dangling edge, under the 1 MB transport limit, not overwhelmingly isolated nodes) are now asserted on all four reduction paths — no cap, count cap, size cap, all caps raised — via a shared helper, plus a dedicated regression for the reserve, the capped_by distinction and the situational hint.
Bump + uv.lock in the same commit, and promote the accumulated [Unreleased] section to a dated [2.4.1] heading with its compare link. PATCH under the agent-pragmatic policy: every entry is a bug fix that makes kegg_pathway_graph return what its schema already promised. No tool is added or renamed, no parameter changes, and the return object keeps its shape — what changed is which rows come back inside it (a dangling edge, a vanished metabolic_gaps section, a graph starved by a raised max_gaps) plus the wording of `truncated.hint` and one `capped_by` value. Also corrects the [Unreleased] compare link, which was left pointing at v2.3.0 by the 2.4.0 release. No `whatsnew:` marker: these fixes are invisible to the hosted audience the intro page addresses — KEGG is stdio-only and opt-in. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
PATCH release. Four review rounds against the live KEGG API, all on how
kegg_pathway_graphreduces an oversized response. Every fix is a case where the toolreturned something that looked like a valid answer and was not.
nodesandedgesas two independent lists left edgespointing at node ids that had been cut (in one synthetic case all 50 of them), while
hundreds of returned nodes appeared edgeless on a densely connected map. The unit of
reduction is now the NODE SET: a binary search finds the largest degree-ordered prefix
whose INDUCED subgraph fits, so every returned edge resolves by construction.
metabolic_gapsvanished from every organism global map at default arguments — thebudget was computed against unreduced section sizes. It is now computed on what is
actually being returned.
max_gaps=5000on hsa01100 took 74% of the payloadand left 50 nodes, fewer than the 191 the defaults return.
nodes+edgesnow take halfthe budget before the supporting sections may spend any of it (138/346 in that case),
and the hint says to lower
max_gapswhen they are what the graph is competing with.truncatedgainsreasons, per-sectioncapped_by(
count/size_budget) andsection_bytes_if_complete; a section cut to protect thegraph's reserve reports
size_budget, nevercount.measured on the wrong map), enzyme-detour collapsing in
kegg_pathway_paths, andentry_idonkegg_find.PATCH, not MINOR: no tool added or renamed, no parameter change, return shape unchanged —
only which rows come back inside it. Affects the stdio +
TOGOMCP_ENABLE_KEGG=1audienceonly; the hosted server does not mount KEGG.
Tests: 395 pass. The three invariants that must hold on every
kegg_pathway_graphreturn(no dangling edge, under the 1 MB transport limit, not overwhelmingly isolated nodes) are
now asserted on all four reduction paths — no cap, count cap, size cap, all caps raised.
No test touches rest.kegg.jp; the global-map cases use a synthetic KGML fixture.
🤖 Generated with Claude Code