Skip to content

Commit 44d7c6b

Browse files
fix: address PR review feedback on tool dispatch sync
1 parent 9104d31 commit 44d7c6b

5 files changed

Lines changed: 28 additions & 34 deletions

File tree

CONTRIBUTING.md

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -145,11 +145,12 @@ See [`docs/architecture.md`](docs/architecture.md) for data flow, export state m
145145

146146
Claude Code assistant `tool_use` blocks carry a `name` string (e.g. `"Read"`, `"Bash"`). The browser coordinates that name across four sites; drift is caught by `tests/test_tool_dispatch_sync.py`.
147147

148-
1. **`utils/tool_dispatch.py`** — add the name to `KNOWN_TOOL_TYPE_NAMES` (keep alphabetical). Set `_FILE_ACTIVITY_HANDLERS[name]` to a tracker function or `None`. If the tool has a distinct `toolUseResult` JSON shape, add `(predicate, builder)` to `_TOOL_RESULT_DISPATCH` (respect ordering — see module docstring and `tests/test_tool_dispatch_ordering.py`).
149-
2. **`utils/md_exporter.py`** — add an `elif name == "…"` branch in `_render_tool_use` and include the name in `MD_EXPORTER_TOOL_TYPES`.
150-
3. **`static/js/render/registry.js`** — add a `TOOL_USE_RENDERERS` entry (and a `tool_use/*.js` renderer module).
151-
4. **Optional result UI** — if the backend emits a new `result_type`, add `TOOL_RESULT_RENDERERS` and a `tool_result/*.js` module.
152-
5. Run `pytest tests/test_tool_dispatch_sync.py -v` — failure names the site missing the new type.
148+
1. **`utils/tool_dispatch.py`** — add the name to `_FILE_ACTIVITY_HANDLERS` (`None` if no file/bash/web side effects); `KNOWN_TOOL_TYPES` is derived from its keys. If the tool has a distinct `toolUseResult` JSON shape, add `(predicate, builder)` to `_TOOL_RESULT_DISPATCH` (respect ordering — see module docstring and `tests/test_tool_dispatch_ordering.py`).
149+
2. **`models/tool_results.py`** — add the name to `ToolNameLiteral` and, when the tool has a distinct result payload, add the TypedDict, type guard (`is_*_tool_result`), and union member on `ToolResultUnion`.
150+
3. **`utils/md_exporter.py`** — add an `elif name == "…"` branch in `_render_tool_use` and include the name in `MD_EXPORTER_TOOL_TYPES`.
151+
4. **`static/js/render/registry.js`** — add a `TOOL_USE_RENDERERS` entry (and a `tool_use/*.js` renderer module).
152+
5. **Optional result UI** — if the backend emits a new `result_type`, add `TOOL_RESULT_RENDERERS` and a `tool_result/*.js` module.
153+
6. Run `pytest tests/test_tool_dispatch_sync.py -v` — failure names the site missing the new type.
153154

154155
## Getting help
155156

tests/test_tool_dispatch_sync.py

Lines changed: 10 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -51,16 +51,22 @@ def _format_set_diff(expected: frozenset[str], actual: frozenset[str], site: str
5151
[
5252
("utils/tool_dispatch.py (FILE_ACTIVITY_TOOL_TYPES)", FILE_ACTIVITY_TOOL_TYPES),
5353
("utils/md_exporter.py (MD_EXPORTER_TOOL_TYPES)", MD_EXPORTER_TOOL_TYPES),
54-
(
55-
"static/js/render/registry.js (TOOL_USE_RENDERERS)",
56-
_parse_frontend_tool_use_renderers(_FRONTEND_REGISTRY),
57-
),
5854
],
5955
)
6056
def test_tool_type_sets_match_known_registry(site: str, actual: frozenset[str]) -> None:
6157
if actual != KNOWN_TOOL_TYPES:
6258
pytest.fail(_format_set_diff(KNOWN_TOOL_TYPES, actual, site))
6359

6460

61+
def test_frontend_registry_matches_known_tool_types() -> None:
62+
site = "static/js/render/registry.js (TOOL_USE_RENDERERS)"
63+
try:
64+
actual = _parse_frontend_tool_use_renderers(_FRONTEND_REGISTRY)
65+
except ValueError as exc:
66+
pytest.fail(f"{site}: {exc}")
67+
if actual != KNOWN_TOOL_TYPES:
68+
pytest.fail(_format_set_diff(KNOWN_TOOL_TYPES, actual, site))
69+
70+
6571
def test_known_tool_types_nonempty() -> None:
6672
assert KNOWN_TOOL_TYPES

utils/jsonl_parser.py

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -388,5 +388,3 @@ def _process_progress(entry: dict[str, Any], messages: list[MessageDict]) -> Non
388388
"is_sidechain": entry.get("isSidechain", False),
389389
}
390390
)
391-
392-

utils/md_exporter.py

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,6 @@
88
from models.stats import SessionStatsDict
99
from utils.jsonl_helpers import strip_system_tags
1010
from utils.session_stats import format_duration
11-
from utils.tool_dispatch import KNOWN_TOOL_TYPES
1211

1312

1413
def session_to_markdown(session: SessionDict, stats: SessionStatsDict | None = None) -> str:
@@ -324,8 +323,8 @@ def _render_tool_use(tool: ToolUseDict) -> str:
324323
continue
325324
lines.append(f">\n> Q: {q.get('question', '')}")
326325
else:
327-
label = "Input" if name in KNOWN_TOOL_TYPES else "Input (unknown tool type)"
328-
lines.append(f">\n> {label}: `{str(inp)}`")
326+
# Unknown names, or known types listed in MD_EXPORTER_TOOL_TYPES before an elif exists.
327+
lines.append(f">\n> Input (unknown tool type): `{str(inp)}`")
329328

330329
return "\n".join(lines)
331330

utils/tool_dispatch.py

Lines changed: 10 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -18,13 +18,15 @@
1818
1919
Adding a new Claude Code **tool use** name (e.g. ``"Read"``, ``"Bash"``):
2020
21-
1. Add the name to ``KNOWN_TOOL_TYPE_NAMES`` below (alphabetical order).
22-
2. Wire ``_FILE_ACTIVITY_HANDLERS`` (``None`` if no file/bash/web side effects).
21+
1. Add the name to ``_FILE_ACTIVITY_HANDLERS`` below (``None`` if no file/bash/web
22+
side effects); ``KNOWN_TOOL_TYPES`` is derived from its keys.
23+
2. Add the name to ``ToolNameLiteral`` in ``models/tool_results.py`` and, if the
24+
tool has a distinct ``toolUseResult`` JSON shape, add the TypedDict, predicate,
25+
and ``(predicate, builder)`` pair in ``_TOOL_RESULT_DISPATCH`` (respect ordering
26+
— see notes above and ``tests/test_tool_dispatch_ordering.py``).
2327
3. Add a Markdown branch in ``utils/md_exporter.py`` ``_render_tool_use``.
2428
4. Add ``TOOL_USE_RENDERERS`` entry in ``static/js/render/registry.js``.
25-
5. Add ``(predicate, builder)`` to ``_TOOL_RESULT_DISPATCH`` when the tool has a
26-
distinct ``toolUseResult`` shape (see ordering notes above).
27-
6. Run ``pytest tests/test_tool_dispatch_sync.py -v`` — it fails with the
29+
5. Run ``pytest tests/test_tool_dispatch_sync.py -v`` — it fails with the
2830
missing site if any step was skipped.
2931
3032
See ``CONTRIBUTING.md`` § "Adding a new tool type".
@@ -235,20 +237,7 @@ def _tool_result_build_user_input(tr: ToolResultDict, base: dict[str, object]) -
235237

236238
# Claude Code assistant tool_use ``name`` values coordinated across parser file
237239
# activity, Markdown export, and the SPA ``TOOL_USE_RENDERERS`` map.
238-
KNOWN_TOOL_TYPE_NAMES: tuple[str, ...] = (
239-
"AskUserQuestion",
240-
"Bash",
241-
"Edit",
242-
"Glob",
243-
"Grep",
244-
"Read",
245-
"Task",
246-
"TodoWrite",
247-
"WebFetch",
248-
"WebSearch",
249-
"Write",
250-
)
251-
KNOWN_TOOL_TYPES: frozenset[str] = frozenset(KNOWN_TOOL_TYPE_NAMES)
240+
# ``_FILE_ACTIVITY_HANDLERS`` is the single registry; ``KNOWN_TOOL_TYPES`` is derived.
252241

253242

254243
def _file_activity_read(tool_input: dict[str, Any], metadata: dict[str, Any]) -> None:
@@ -284,7 +273,6 @@ def _file_activity_web(tool_input: dict[str, Any], metadata: dict[str, Any]) ->
284273
metadata["web_fetches"].append(url_or_query)
285274

286275

287-
# Every ``KNOWN_TOOL_TYPES`` entry must appear here (``None`` = no side effects).
288276
_FILE_ACTIVITY_HANDLERS: dict[str, Callable[[dict[str, Any], dict[str, Any]], None] | None] = {
289277
"AskUserQuestion": None,
290278
"Bash": _file_activity_bash,
@@ -298,6 +286,8 @@ def _file_activity_web(tool_input: dict[str, Any], metadata: dict[str, Any]) ->
298286
"WebSearch": _file_activity_web,
299287
"Write": _file_activity_write,
300288
}
289+
KNOWN_TOOL_TYPE_NAMES: tuple[str, ...] = tuple(sorted(_FILE_ACTIVITY_HANDLERS))
290+
KNOWN_TOOL_TYPES: frozenset[str] = frozenset(_FILE_ACTIVITY_HANDLERS)
301291
FILE_ACTIVITY_TOOL_TYPES: frozenset[str] = frozenset(_FILE_ACTIVITY_HANDLERS)
302292

303293

0 commit comments

Comments
 (0)