Skip to content

feat(tui): show tool and MCP schema costs - #5653

Merged
Hmbown merged 2 commits into
mainfrom
codex/pr5603-rescue-20260827
Aug 27, 2026
Merged

feat(tui): show tool and MCP schema costs#5653
Hmbown merged 2 commits into
mainfrom
codex/pr5603-rescue-20260827

Conversation

@Hmbown

@Hmbown Hmbown commented Aug 27, 2026

Copy link
Copy Markdown
Owner

Summary

Rescue of @wuisabel-gif's #5603 onto current main. Authorship of the feature commit is preserved (Isabel Wu <231155141+wuisabel-gif@users.noreply.github.com>). The original PR and the later grok rebase (#5611) were closed as already integrated on a 0.9.12 integration branch, but the work never reached origin/main.

The context inspector now shows bounded schema-cost estimates from the last model tool catalog:

  • a catalog total and per-built-in-tool rows sorted by estimated token cost;
  • a bounded row list with an omitted-count summary for large catalogs;
  • one row per discovered MCP server with estimated cost and known/announced tool counts;
  • explicit zero-cost rows for servers with no announced tools.

Follow-up maintainer layout fix keeps this report in its own localized section (CtxInspToolSchemaCosts) so it does not share the "Recent Tools" heading.

No prompt assembly, tool catalog construction, MCP discovery, or cache behavior changes.

Original: #5603
Earlier rebase (also closed without landing): #5611

Testing

  • python3 scripts/check-coauthor-trailers.py --range origin/main..HEAD (passed for 2 commits)
  • cargo test -p codewhale-tui --lib 'tui::context_inspector' --locked (14 passed)
  • python3 scripts/check-tui-locale-parity.py (PASS, 1684 keys)
  • git diff --check

No-Issue: Re-lands the already-reviewed #5603 / #5553 display-only slice; does not close the broader #5553 issue.

wuisabel-gif and others added 2 commits August 27, 2026 05:21
Add bounded per-tool catalog estimates and per-server MCP announcement totals to the context inspector without changing prompt assembly or cache behavior. Cover ordering, truncation, and empty-server cases for #5553.
Separate the contributed tool/MCP schema-cost report from Recent Tools and add complete locale coverage, so the context inspector does not render two adjacent sections under the same heading.

Signed-off-by: CodeWhale Bot <bot@codewhale.net>
@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, add credits to your account and enable them for code reviews in your settings.

@claude

claude Bot commented Aug 27, 2026

Copy link
Copy Markdown
Contributor

Claude finished @Hmbown's task in 4m 30s —— View job


Review of PR #5653 — feat(tui): show tool and MCP schema costs

  • Gather context (diff, changed files)
  • Understand the request (code review)
  • Review context_inspector.rs changes for correctness
  • Review provider/model safety implications
  • Review reuse/simplification opportunities
  • Review test coverage
  • Review security considerations
  • Post final review

Summary

Display-only diagnostic addition to the context inspector — no prompt assembly, tool catalog construction, or MCP discovery/cache paths are touched, which matches the stated scope. The core logic in crates/tui/src/tui/context_inspector.rs (tool_schema_tokens, push_tool_schema_costs) is straightforward: serialize each Tool, estimate tokens via chars / 4 (rounded up), split rows into a bounded built-in catalog list (MAX_SCHEMA_COST_ROWS = 24, with an omitted-count summary) and one row per MCP server. Localization is complete and consistent — the new CtxInspToolSchemaCosts key is present in localization.rs (MessageId enum + ALL_MESSAGE_IDS) and in all 15 locale files. No provider/model-prefix inference or route logic is touched, so the EPIC #2608 invariant doesn't apply here.

Note (not a blocker): I was not able to execute cargo test/cargo check in this sandboxed review environment (commands required interactive approval I don't have here), so I relied on static reading of the diff plus the test results already reported in the PR description (14 passed). The new test (inspector_reports_catalog_and_mcp_schema_costs_with_bounded_rows) matches the actual Tool/McpServerSnapshot/McpManagerSnapshot struct shapes I found in crates/core/src/request.rs and crates/tui/src/mcp.rs, so it should compile and pass as claimed.

Findings

  1. Minor — silent gap for MCP meta-tool schema costs (crates/tui/src/tui/context_inspector.rs:583-591 and :600-620)
    The built-in bucket excludes anything McpPool::is_mcp_tool matches (mcp_ prefix or list_mcp_resources / list_mcp_resource_templates / read_mcp_resource), and the MCP-server loop only looks up tools that appear in a given server.tools announced list. The four synthetic meta-tools injected in mcp.rs (list_mcp_resources, list_mcp_resource_templates, mcp_read_resource, read_mcp_resource — see crates/tui/src/mcp.rs:3173-3241) are excluded from built_in but never announced per-server, so when MCP resources are configured, their schema cost silently disappears from both the [catalog] total and every [mcp:...] row. The report will therefore under-count real wire tokens whenever resource-backed MCP servers are present. Consider either including these in the [catalog] bucket (they're not server-specific) or a dedicated [mcp:meta] row, so the sum of displayed rows reconciles with the actual catalog.

  2. Nit — O(servers × tools_per_server × catalog_size) lookup (crates/tui/src/tui/context_inspector.rs:606-611)
    catalog.iter().find(...) runs once per announced tool per server. Fine at today's catalog sizes (this is a diagnostic view, not a hot path), but a HashMap<&str, usize> built once from catalog would make this O(n) and is a natural follow-up if catalog sizes grow.

Both are low severity and don't block merging; the feature is additive, bounded, and localization-complete. Nice touch bounding both the row count (MAX_SCHEMA_COST_ROWS) and explicitly rendering zero-cost MCP servers rather than omitting them.

Fix this →

@Hmbown
Hmbown merged commit d4d9a56 into main Aug 27, 2026
28 checks passed
@Hmbown
Hmbown deleted the codex/pr5603-rescue-20260827 branch August 27, 2026 13:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants