claude-code-chat-browser: Tool dispatch coordination - single authoritative KNOWN_TOOL_TYPES registry#104
Merged
Conversation
Adding a Claude Code tool use name required coordinated edits across tool_dispatch, jsonl_parser file activity, md_exporter, and the frontend registry with no test-time guard against drift. Define KNOWN_TOOL_TYPES in utils/tool_dispatch.py, move file-activity tracking to track_tool_file_activity(), wire md_exporter to the registry, and add tests/test_tool_dispatch_sync.py to assert all four sites stay in sync. Document the add-a-tool-type procedure in CONTRIBUTING.md.
📝 WalkthroughWalkthroughAdds a shared tool-type registry, routes assistant file-activity tracking through it, updates tool-name typing and markdown export, adds a sync test for backend and frontend declarations, and documents the required update steps. ChangesTool Type Registry and Sync Enforcement
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
utils/md_exporter.py (1)
327-328: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe known-type
"Input"label is currently unreachable.Every entry in
KNOWN_TOOL_TYPESalready has a dedicatedif/elifbranch above, so anynamereaching thiselseis by definition not inKNOWN_TOOL_TYPES. The conditional therefore always yields"Input (unknown tool type)". This is harmless as forward-compat defense, but the"Input"arm only matters if a future known tool lacks an explicit branch — consider a comment to that effect or simplifying.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@utils/md_exporter.py` around lines 327 - 328, The fallback label in the `md_exporter.py` input-rendering branch is effectively unreachable because all `KNOWN_TOOL_TYPES` are already handled earlier in the `if/elif` chain. Update the `label` logic near the `lines.append(...)` path to either simplify it to the unknown-tool label only, or add a short comment in that branch explaining it is forward-compatibility for future known tool types without explicit handling.tests/test_tool_dispatch_sync.py (1)
49-58: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueParser runs at collection time, surfacing failures as a collection error.
_parse_frontend_tool_use_renderers(_FRONTEND_REGISTRY)is evaluated inside the@pytest.mark.parametrizedecorator, so a missing/renamedregistry.jsor a regex miss raisesValueErrorduring collection rather than as a clean test failure. Consider reading/parsing inside the test body (or a fixture) so the contract violation is reported as a failing assertion with the intended diagnostic.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_tool_dispatch_sync.py` around lines 49 - 58, The frontend registry parsing is happening too early in the test setup, which turns missing or malformed registry data into a collection error instead of a normal test failure. Move the `_parse_frontend_tool_use_renderers` call out of the `@pytest.mark.parametrize` arguments in `test_tool_dispatch_sync` and into the test body or a fixture, using `_FRONTEND_REGISTRY` there so any parse/contract violation is reported through an assertion with the intended diagnostic.utils/tool_dispatch.py (1)
238-251: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: derive the registry from a single source to cut in-file duplication.
KNOWN_TOOL_TYPE_NAMESand the keys of_FILE_ACTIVITY_HANDLERSare two hand-maintained lists in the same module that the sync test forces to be equal. You can make_FILE_ACTIVITY_HANDLERSthe single source and derive the others, removing one drift surface (the cross-file md_exporter/frontend checks still hold).♻️ Possible simplification
-KNOWN_TOOL_TYPE_NAMES: tuple[str, ...] = ( - "AskUserQuestion", - ... - "Write", -) -KNOWN_TOOL_TYPES: frozenset[str] = frozenset(KNOWN_TOOL_TYPE_NAMES) +# defined after _FILE_ACTIVITY_HANDLERS belowFILE_ACTIVITY_TOOL_TYPES: frozenset[str] = frozenset(_FILE_ACTIVITY_HANDLERS) +KNOWN_TOOL_TYPES: frozenset[str] = FILE_ACTIVITY_TOOL_TYPES +KNOWN_TOOL_TYPE_NAMES: tuple[str, ...] = tuple(sorted(_FILE_ACTIVITY_HANDLERS))Also applies to: 288-301
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@utils/tool_dispatch.py` around lines 238 - 251, `KNOWN_TOOL_TYPE_NAMES` is duplicating the tool registry already maintained in `_FILE_ACTIVITY_HANDLERS`, so reduce drift by making `_FILE_ACTIVITY_HANDLERS` the single source of truth and deriving `KNOWN_TOOL_TYPE_NAMES` and `KNOWN_TOOL_TYPES` from its keys. Update the related registry setup in `tool_dispatch.py` so the sync test still passes without keeping two hand-maintained lists in parallel, and keep the existing tool type symbols (`KNOWN_TOOL_TYPE_NAMES`, `KNOWN_TOOL_TYPES`, `_FILE_ACTIVITY_HANDLERS`) aligned.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CONTRIBUTING.md`:
- Around line 144-153: The new-tool checklist is missing the typed backend
contract update, so extend the `Adding a new tool type` guidance to mention
`models/tool_results.py` and the `ToolNameLiteral`/tool result type definitions
used for `AskUserQuestion`. Update the same checklist section that references
`utils/tool_dispatch.py`, `utils/md_exporter.py`, and
`static/js/render/registry.js` so it also tells contributors to add the new tool
name to the backend type literals and result model, ensuring the guide covers
both runtime dispatch and typed schema changes.
---
Nitpick comments:
In `@tests/test_tool_dispatch_sync.py`:
- Around line 49-58: The frontend registry parsing is happening too early in the
test setup, which turns missing or malformed registry data into a collection
error instead of a normal test failure. Move the
`_parse_frontend_tool_use_renderers` call out of the `@pytest.mark.parametrize`
arguments in `test_tool_dispatch_sync` and into the test body or a fixture,
using `_FRONTEND_REGISTRY` there so any parse/contract violation is reported
through an assertion with the intended diagnostic.
In `@utils/md_exporter.py`:
- Around line 327-328: The fallback label in the `md_exporter.py`
input-rendering branch is effectively unreachable because all `KNOWN_TOOL_TYPES`
are already handled earlier in the `if/elif` chain. Update the `label` logic
near the `lines.append(...)` path to either simplify it to the unknown-tool
label only, or add a short comment in that branch explaining it is
forward-compatibility for future known tool types without explicit handling.
In `@utils/tool_dispatch.py`:
- Around line 238-251: `KNOWN_TOOL_TYPE_NAMES` is duplicating the tool registry
already maintained in `_FILE_ACTIVITY_HANDLERS`, so reduce drift by making
`_FILE_ACTIVITY_HANDLERS` the single source of truth and deriving
`KNOWN_TOOL_TYPE_NAMES` and `KNOWN_TOOL_TYPES` from its keys. Update the related
registry setup in `tool_dispatch.py` so the sync test still passes without
keeping two hand-maintained lists in parallel, and keep the existing tool type
symbols (`KNOWN_TOOL_TYPE_NAMES`, `KNOWN_TOOL_TYPES`, `_FILE_ACTIVITY_HANDLERS`)
aligned.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1b04bcab-9f30-41a5-b8da-51ff257d8225
📒 Files selected for processing (6)
CONTRIBUTING.mdmodels/tool_results.pytests/test_tool_dispatch_sync.pyutils/jsonl_parser.pyutils/md_exporter.pyutils/tool_dispatch.py
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@utils/md_exporter.py`:
- Around line 326-327: The fallback in the markdown exporter is mislabeling
known-but-unhandled tool names as “unknown,” which can hide missing renderers.
Update the export logic in md_exporter.py around the Input formatting branch so
it distinguishes truly unknown tool types from names already listed in
MD_EXPORTER_TOOL_TYPES, and change the fallback message to reflect that the tool
is known but lacks a specialized renderer; use the surrounding exporter function
and the MD_EXPORTER_TOOL_TYPES check to locate the branch.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d00a0348-06fe-4be7-abdc-44ea8e2602ae
📒 Files selected for processing (5)
CONTRIBUTING.mdtests/test_tool_dispatch_sync.pyutils/jsonl_parser.pyutils/md_exporter.pyutils/tool_dispatch.py
💤 Files with no reviewable changes (1)
- utils/jsonl_parser.py
✅ Files skipped from review due to trivial changes (1)
- CONTRIBUTING.md
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/test_tool_dispatch_sync.py
- utils/tool_dispatch.py
timon0305
requested changes
Jun 30, 2026
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/test_tool_dispatch_sync.py (1)
66-81: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winParse
_render_tool_usewithastinstead of regex.This helper is tied to the current source spelling in
utils/md_exporter.py— double-quotedif/elifcomparisons,elif name in (...), and the next top-leveldefboundary. A harmless refactor there can make the sync test fail even when the handled tool set is unchanged.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_tool_dispatch_sync.py` around lines 66 - 81, Replace the regex-based parsing in _parse_md_exporter_tool_use_handlers with AST-based inspection of utils/md_exporter.py so the test derives handled tool names from the _render_tool_use function structure rather than source spelling. Locate the helper by its existing name, parse the module, find the _render_tool_use definition, and collect names from if/elif comparisons on name (including membership checks) in a way that is insensitive to quote style, formatting, or nearby top-level defs.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/test_tool_dispatch_sync.py`:
- Around line 66-81: Replace the regex-based parsing in
_parse_md_exporter_tool_use_handlers with AST-based inspection of
utils/md_exporter.py so the test derives handled tool names from the
_render_tool_use function structure rather than source spelling. Locate the
helper by its existing name, parse the module, find the _render_tool_use
definition, and collect names from if/elif comparisons on name (including
membership checks) in a way that is insensitive to quote style, formatting, or
nearby top-level defs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 77148343-a8e3-41b4-b554-a2d09d6cdb36
📒 Files selected for processing (4)
CONTRIBUTING.mdtests/test_tool_dispatch_sync.pyutils/md_exporter.pyutils/tool_dispatch.py
💤 Files with no reviewable changes (1)
- utils/tool_dispatch.py
✅ Files skipped from review due to trivial changes (2)
- CONTRIBUTING.md
- utils/md_exporter.py
timon0305
approved these changes
Jun 30, 2026
wpak-ai
approved these changes
Jun 30, 2026
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
3 participants
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.
Closes #99
Summary
Establishes a single authoritative
KNOWN_TOOL_TYPESregistry inutils/tool_dispatch.pyand wires the four dispatch sites that previously shared tool name strings only by convention. Addstests/test_tool_dispatch_sync.pyso CI fails with a clear message when any site drifts.Sprint item #3 (Tuesday PR 1 of 2). Companion PR #4 will generate
static/tool_types.jsonfromKNOWN_TOOL_TYPES.Problem
Adding a new Claude Code tool type required coordinated edits across:
utils/tool_dispatch.pyutils/jsonl_parser.pyutils/md_exporter.pyTOOL_USE_RENDERERSinstatic/js/render/registry.jsThere was no compile-time or test-time check that all four were updated together.
Changes
utils/tool_dispatch.py—KNOWN_TOOL_TYPE_NAMES/KNOWN_TOOL_TYPES;_FILE_ACTIVITY_HANDLERSandtrack_tool_file_activity()(one entry per known type,Nonewhen there are no side effects)utils/jsonl_parser.py— delegates file activity totrack_tool_file_activity(); removes local_track_file_activityutils/md_exporter.py— importsKNOWN_TOOL_TYPES; exportsMD_EXPORTER_TOOL_TYPESfor sync; labels unknown tool types in the fallback branchtests/test_tool_dispatch_sync.py(new) — assertsKNOWN_TOOL_TYPESmatchesFILE_ACTIVITY_TOOL_TYPES,MD_EXPORTER_TOOL_TYPES, and frontendTOOL_USE_RENDERERSkeysmodels/tool_results.py— addsAskUserQuestiontoToolNameLiteral(was missing vs frontend)CONTRIBUTING.md— documents the “add a new tool type” procedureOut of scope
static/tool_types.jsonmanifest (Tuesday PR 2 / issue #4)Test plan
pytest tests/test_tool_dispatch_sync.py -qpytest -q(417 passed)mypy -p api -p utils -p modelsmypy tests --config-file mypy-tests.ini --follow-imports skipruff checkon changed filesTOOL_USE_RENDERERS— confirm sync test fails naming the missing type and site, then revertSummary by CodeRabbit