refactor: priority-based tool result dispatch selection#127
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughTool result classification now uses explicit dispatch entries with priorities. All matching predicates are evaluated, the highest-priority entry wins, and registration order resolves ties. Tests, fixtures, and documentation describe and validate the new behavior. ChangesTool dispatch priority refactor
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant Parser as _parse_tool_result
participant Registry as _TOOL_RESULT_DISPATCH
participant Selector as _winning_dispatch_entry
participant Builder as winning build function
Parser->>Registry: evaluate matching predicates
Registry-->>Selector: return matching entries
Selector->>Selector: choose highest priority
Selector-->>Parser: return winning entry
Parser->>Builder: build parsed result
Suggested reviewers: 🚥 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 (1)
utils/tool_dispatch.py (1)
236-270: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGuard against accidental duplicate
idvalues in the dispatch table.
_DISPATCH_ORDERis built via{entry.id: index for ...}; a duplicateid(easy to introduce when the whole point of this refactor is to let contributors append entries without reasoning about the full table) would silently collide, causing incorrect tie-break ordering with no test failure to catch it. A cheap assertion would fail fast on misconfiguration.♻️ Proposed guard
_DISPATCH_ORDER = {entry.id: index for index, entry in enumerate(_TOOL_RESULT_DISPATCH)} +assert len(_DISPATCH_ORDER) == len(_TOOL_RESULT_DISPATCH), ( + "Duplicate ToolResultDispatchEntry id in _TOOL_RESULT_DISPATCH" +)🤖 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 236 - 270, Add a fail-fast validation near _DISPATCH_ORDER that detects duplicate entry.id values in _TOOL_RESULT_DISPATCH before constructing the index mapping, raising a clear assertion or configuration error identifying the duplicated IDs. Preserve the existing registration-order mapping after validation.
🤖 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 `@docs/architecture.md`:
- Around line 74-80: Update the stale parenthetical in the backend dispatch
description near the referenced architecture section: replace “ordered
predicates” with wording that accurately describes priority-based predicate
matching, consistent with _parse_tool_result and _TOOL_RESULT_DISPATCH. Do not
alter the surrounding architectural guidance.
---
Nitpick comments:
In `@utils/tool_dispatch.py`:
- Around line 236-270: Add a fail-fast validation near _DISPATCH_ORDER that
detects duplicate entry.id values in _TOOL_RESULT_DISPATCH before constructing
the index mapping, raising a clear assertion or configuration error identifying
the duplicated IDs. Preserve the existing registration-order mapping after
validation.
🪄 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: 43342ceb-b8eb-47e6-bc4d-4ca339d050a6
📒 Files selected for processing (7)
CONTRIBUTING.mddocs/architecture.mdtests/test_jsonl_parser.pytests/test_real_session_fixtures.pytests/test_tool_dispatch_adversarial.pytests/test_tool_dispatch_ordering.pyutils/tool_dispatch.py
|
Looks good. All addressed. Dropping the priority boost on task_message and learning on registration order is the right call, and the ordering test now running _winning_dispatch_entry against real overlap blobs is a much stronger check than the old priority-int comparison. Adding the file_write beats task_message regression was a nice touch. |
3 participants
Closes #118
Adding a new tool result shape in utils/tool_dispatch.py used to mean figuring out where to insert it in _TOOL_RESULT_DISPATCH so the right predicate won on overlap.
Tuple position was load-bearing, and the two intentional exceptions (plan over file_write, task_message over the narrower task predicates) were easy to miss unless you already knew the whole table.
This branch replaces the (predicate, builder) tuples with ToolResultDispatchEntry records that carry an explicit priority.
Every matching predicate is considered; the highest priority wins, with registration order as the tie-break.
The overlap pairs set priority 1 on the broader shape and leave the narrower ones at 0.
Classification output is unchanged for existing fixtures.
Ordering invariants, adversarial overlap tests, and real-session coverage were updated to assert priority instead of index. docs/architecture.md and CONTRIBUTING.md now describe the priority rule.
Summary by CodeRabbit
toolUseResultclassification for overlapping payloads by using priority-based selection with deterministic tie-breaking.