refactor(models): narrow Any-typed parse boundary with TypedDict unions#72
refactor(models): narrow Any-typed parse boundary with TypedDict unions#72clean6378-max-it wants to merge 4 commits into
Conversation
Replace NotRequired[Any] on MessageDict.tool_result and data with per-tool and per-record unions, type guards, and dispatch-aligned narrowing so mypy strict constrains schema drift without weakening tests.
📝 WalkthroughWalkthroughAdds TypedDict schemas and TypeGuard predicates for tool results and record data, narrows session message typings to those unions, and updates tool dispatch, JSONL parsing, and markdown exporter to use and validate the new typed contracts. ChangesType-narrowing refactor from Any to structured unions
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
models/__init__.py (1)
3-16:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winFix import sorting to pass ruff check.
The import block is flagged as unsorted by the pipeline. The imports should be in alphabetical order by module name.
📦 Proposed fix
from models.errors import ErrorResponse from models.export import ExportStateDict from models.project import ProjectDict, ProjectSessionRowDict, SessionListItemDict -from models.search import SearchHitDict from models.record_data import RecordDataUnion +from models.search import SearchHitDict from models.session import ( MessageDict, QuickSessionInfoDict, SessionDict, SessionMetadataDict, ToolUseDict, ) -from models.tool_results import ToolResultUnion from models.stats import FilesTouchedDict, SessionStatsDict +from models.tool_results import ToolResultUnion🤖 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 `@models/__init__.py` around lines 3 - 16, The import block in models/__init__.py is unsorted causing Ruff to fail; reorder the import lines alphabetically by module name so modules appear as: errors (ErrorResponse), export (ExportStateDict), project (ProjectDict, ProjectSessionRowDict, SessionListItemDict), record_data (RecordDataUnion), search (SearchHitDict), session (MessageDict, QuickSessionInfoDict, SessionDict, SessionMetadataDict, ToolUseDict), stats (FilesTouchedDict, SessionStatsDict), tool_results (ToolResultUnion); update the file to reflect that alphabetical module ordering while keeping the same imported symbols.Source: Pipeline failures
🤖 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 `@models/record_data.py`:
- Line 3: The import line in record_data.py is unsorted and fails ruff; reorder
the imports alphabetically (by module/member name) so the from typing import ...
line lists members in sorted order (e.g., ensure Literal, TypeGuard, TypedDict
are alphabetized) or split into multiple lines per project style so ruff
import-sorting passes; update the import of Literal, TypeGuard, TypedDict
accordingly in the file.
In `@models/tool_results.py`:
- Line 7: The import block in models/tool_results.py is unsorted; reorder the
imported names in the from typing import ... line so they are alphabetically
sorted (e.g., ensure Literal, TypeGuard, TypedDict are in the correct
alphabetical order) to satisfy the ruff sorting rule and rerun the linter; keep
the single from typing import ... statement and do not add or remove imports.
---
Outside diff comments:
In `@models/__init__.py`:
- Around line 3-16: The import block in models/__init__.py is unsorted causing
Ruff to fail; reorder the import lines alphabetically by module name so modules
appear as: errors (ErrorResponse), export (ExportStateDict), project
(ProjectDict, ProjectSessionRowDict, SessionListItemDict), record_data
(RecordDataUnion), search (SearchHitDict), session (MessageDict,
QuickSessionInfoDict, SessionDict, SessionMetadataDict, ToolUseDict), stats
(FilesTouchedDict, SessionStatsDict), tool_results (ToolResultUnion); update the
file to reflect that alphabetical module ordering while keeping the same
imported symbols.
🪄 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: 588107cc-972b-4a26-b183-6be6a98fbc80
📒 Files selected for processing (7)
models/__init__.pymodels/record_data.pymodels/session.pymodels/tool_results.pyutils/jsonl_parser.pyutils/md_exporter.pyutils/tool_dispatch.py
|
@CodeRabbit , Can you reeviewe this PR fully? |
|
✅ Action performedFull review finished. |
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 `@models/record_data.py`:
- Around line 6-8: The TypedDict BashProgressDataDict is currently total (all
keys required) which makes the TypeGuard is_bash_progress_data unsafe — change
the TypedDict to allow missing keys by declaring it as non-total (e.g., class
BashProgressDataDict(TypedDict, total=False): with the same type:
Literal["bash_progress"] and output: str), and/or mark output as
NotRequired[str]; additionally ensure the TypeGuard function
is_bash_progress_data either checks for the presence of "output" (e.g., "output"
in data) before narrowing or leave the TypedDict non-total so mypy won't assume
output exists after the type guard.
🪄 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: 4b2ee96e-d606-435b-9762-a7866daf186b
📒 Files selected for processing (7)
models/__init__.pymodels/record_data.pymodels/session.pymodels/tool_results.pyutils/jsonl_parser.pyutils/md_exporter.pyutils/tool_dispatch.py
There was a problem hiding this comment.
🧹 Nitpick comments (1)
models/record_data.py (1)
46-47: Don’t add extra TypeGuards for progress variants unless code actually narrows ondata["type"]
utils/jsonl_parser.py:_process_progressstoresdata: RecordDataUnionand derives onlyprogress_type = str(data.get("type", "")); there are no consumption sites that branch ondata["type"]forhook_progress,agent_progress, orsummary. Repo-wide, those discriminator strings only appear inmodels/record_data.pyTypedDict definitions (andcompact_boundaryis handled as a systemsubtype, not as recorddata.type). Also,is_bash_progress_data(models/record_data.py:46-47) is currently unused—add its intended consumption site or remove/defer it.🤖 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 `@models/record_data.py` around lines 46 - 47, The is_bash_progress_data TypeGuard is currently unused; either remove this TypeGuard from models/record_data.py or actually use it to narrow progress records where code branches on data["type"]. If you choose removal, delete the is_bash_progress_data function and any unused TypedDict-only discriminators; if you choose to keep it, update utils/jsonl_parser.py::_process_progress to inspect and branch on data["type"] (e.g., call is_bash_progress_data(data) and handle the bash_progress shape) so the TypeGuard is consumed and provides real narrowing.
🤖 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 `@models/record_data.py`:
- Around line 46-47: The is_bash_progress_data TypeGuard is currently unused;
either remove this TypeGuard from models/record_data.py or actually use it to
narrow progress records where code branches on data["type"]. If you choose
removal, delete the is_bash_progress_data function and any unused TypedDict-only
discriminators; if you choose to keep it, update
utils/jsonl_parser.py::_process_progress to inspect and branch on data["type"]
(e.g., call is_bash_progress_data(data) and handle the bash_progress shape) so
the TypeGuard is consumed and provides real narrowing.
timon0305
left a comment
timon0305
left a comment
There was a problem hiding this comment.
Solid parse-boundary refactor — mypy strict is green, the isinstance-guarded narrowing in the parser/exporter is the right shape, and no # type: ignore crept in. Main thing: the per-tool TypeGuards in tool_results.py are unused and duplicate tool_dispatch.py's predicates (drift risk) — wire them in or drop them. Two minor notes inline.
| def is_bash_tool_result(tr: ToolResultDict) -> TypeGuard[BashToolResultDict]: | ||
| return "stdout" in tr or "stderr" in tr | ||
|
|
||
|
|
||
| def is_file_edit_tool_result(tr: ToolResultDict) -> TypeGuard[FileEditToolResultDict]: | ||
| return "structuredPatch" in tr or ("filePath" in tr and "newString" in tr) | ||
|
|
||
|
|
||
| def is_plan_tool_result(tr: ToolResultDict) -> TypeGuard[PlanToolResultDict]: | ||
| return "plan" in tr and "filePath" in tr | ||
|
|
||
|
|
||
| def is_file_write_tool_result(tr: ToolResultDict) -> TypeGuard[FileWriteToolResultDict]: | ||
| return "filePath" in tr and "content" in tr | ||
|
|
||
|
|
||
| def is_glob_tool_result(tr: ToolResultDict) -> TypeGuard[GlobToolResultDict]: | ||
| filenames = tr.get("filenames") | ||
| return "filenames" in tr and isinstance(filenames, list) | ||
|
|
||
|
|
||
| def is_grep_tool_result(tr: ToolResultDict) -> TypeGuard[GrepToolResultDict]: | ||
| return "mode" in tr and "numFiles" in tr | ||
|
|
||
|
|
||
| def is_read_tool_result(tr: ToolResultDict) -> TypeGuard[ReadToolResultDict]: | ||
| file_obj = tr.get("file") | ||
| return "file" in tr and isinstance(file_obj, dict) | ||
|
|
||
|
|
||
| def is_web_search_tool_result(tr: ToolResultDict) -> TypeGuard[WebSearchToolResultDict]: | ||
| return "query" in tr and "results" in tr | ||
|
|
||
|
|
||
| def is_web_fetch_tool_result(tr: ToolResultDict) -> TypeGuard[WebFetchToolResultDict]: | ||
| return "url" in tr and "code" in tr |
There was a problem hiding this comment.
These nine per-tool TypeGuards (is_bash_tool_result … is_web_fetch_tool_result) have zero callers — utils/tool_dispatch.py does the actual dispatch with its own parallel _tool_result_pred_* functions carrying identical logic (e.g. "stdout" in tr or "stderr" in tr exists in both files). As written this is dead code that duplicates the dispatch predicates, so the two will silently drift when one side changes. Either wire tool_dispatch.py to use these TypeGuards (so the narrowing flows to the builders) or drop them and keep tool_dispatch.py as the single source of truth. (is_tool_result_dict at 153 is fine — it IS used.)
| ) | ||
|
|
||
|
|
||
| def is_bash_progress_data(data: RecordDataUnion) -> TypeGuard[BashProgressDataDict]: |
There was a problem hiding this comment.
is_bash_progress_data is the only TypeGuard among the five RecordDataUnion members and it's also unused. Either it's the start of a pattern (then the other four deserve the same + a caller), or drop it — a lone unused guard reads like a leftover.
|
|
||
| class ToolUseDict(TypedDict, total=False): | ||
| id: str | ||
| name: ToolNameLiteral | str |
There was a problem hiding this comment.
ToolNameLiteral | str collapses to str for type-checking, so the Literal gives no narrowing — it's documentation only. Fine as a "known names + escape hatch" intent, but worth a one-line comment saying that's deliberate, otherwise a reader expects exhaustiveness checking it won't get.
2 participants
closes #68
Summary
Replaces
NotRequired[Any]onMessageDict.tool_resultandMessageDict.datawith typed unions so mypy strict constrains the JSONL parse boundary without weakening tests or adding# type: ignore.Rebased onto Monday's ruff/pip-audit CI merge (#70).
Problem
tool_resultanddataare the highest-risk fields in the Claude Code JSONL format — structurally complex and prone to upstream drift.Anylet schema ambiguity flow silently into rendering, export, and search.Changes
models/tool_results.py(new) — per-toolTypedDicts for all 15_TOOL_RESULT_DISPATCHshapes (Bash, Read, Write, Edit, Glob, Grep, Task variants, TodoWrite, WebFetch, WebSearch, UserInput, Plan), plusToolResultUnion,ToolNameLiteral, andTypeGuardhelpersmodels/record_data.py(new) —RecordDataUnionfor progressdatapayloads (bash_progress,hook_progress,agent_progress,summary,compact_boundary) withdict[str, object]fallbackmodels/session.py—tool_result: ToolResultUnion | None,data: RecordDataUnion; addsToolUseDictandMessageUsageDictutils/tool_dispatch.py— predicates/builders typed withToolResultDict;_parse_tool_resultacceptsToolResultUnion | Noneutils/jsonl_parser.py—is_tool_result_dictnarrowing at image-extraction site; typedToolUseDictassembly in_process_assistantutils/md_exporter.py—_render_tool_useconsumesToolUseDictwith explicitdict[str, object]narrowingUnknown upstream shapes use
dict[str, object]instead ofAnyto force explicit narrowing at use sites.Acceptance criteria
tool_resulttyped with union covering all known tool result shapes from fixturesdatatyped with union covering known record typesTypeGuard/Literal(ToolNameLiteral,is_tool_result_dict)# type: ignorein changed production codemypy -p api -p utils -p modelspassesruff check .passes (rebased on feat: add ruff and pip-audit CI gates, fix style violations, and add … #70)Test plan