ToolCall optimizations + compaction disabling by default#174
Conversation
- Else Pi auto compact and this interfere with the response from stdout, which Tiles is not expecting atm. Due to this we were not getting response from the model from Pi to repl. - chore: replaced "bluesky based pds" to ATmosphere pds
|
Warning Review limit reached
Next review available in: 44 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: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis PR adds an Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant EventProcessor as Event Processor
participant Optimizer as optimize_arguments
participant JSON as Serialized Tool Call
EventProcessor->>Optimizer: pass tool_name, model_args
Optimizer->>Optimizer: rename cmd -> command
Optimizer->>Optimizer: strip -R from ls (bash only)
Optimizer->>Optimizer: default/cap timeout to 60 (bash only)
Optimizer-->>EventProcessor: optimized arguments
EventProcessor->>JSON: serialize optimized arguments
sequenceDiagram
participant ReplStartup as start_pi_rpc
participant ConfigHandler as handle_pi_settings_config
participant FileSystem as settings.json
ReplStartup->>ConfigHandler: settings_file_path
ConfigHandler->>FileSystem: read existing settings.json
alt file missing
ConfigHandler->>ConfigHandler: use PiSettings::default()
else file exists
ConfigHandler->>ConfigHandler: parse PiSettings
end
ConfigHandler->>ConfigHandler: ensure compaction set
ConfigHandler->>FileSystem: write pretty-printed settings.json
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 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 |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
![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)
tiles/src/utils/config.rs (1)
621-621: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPrefer
&Pathover&PathBufin the signature.Taking
&PathBuftriggers clippy'sptr_arglint;&Pathis more general and idiomatic (callers can pass any path-like reference). No body changes needed since&PathBufderefs to&Path.♻️ Proposed change
-pub fn handle_pi_settings_config(settings_path: &PathBuf) -> Result<()> { +pub fn handle_pi_settings_config(settings_path: &Path) -> Result<()> {Ensure
use std::path::Path;is in scope.🤖 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 `@tiles/src/utils/config.rs` at line 621, The `handle_pi_settings_config` signature should accept `&Path` instead of `&PathBuf` to avoid the `ptr_arg` lint and make the API more idiomatic. Update the function parameter type in `handle_pi_settings_config`, ensure `std::path::Path` is imported in `config.rs`, and leave the function body unchanged since `&PathBuf` already dereferences to `&Path`.Source: Path instructions
server/backend/commons.py (1)
465-468: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDead code left over from the
optimize_argumentsrefactor.Lines 465-467 build
new_argsvia the old inline dict comprehension, but it's immediately discarded and reassigned fromoptimize_arguments(...)on line 468 (usingarguments_map, not the oldnew_args). This is leftover dead code that should be removed.🧹 Proposed cleanup
- new_args = { - ("command" if k == "cmd" else k): v for k, v in arguments_map.items() - } new_args = optimize_arguments(str(tool_name), arguments_map)🤖 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 `@server/backend/commons.py` around lines 465 - 468, Remove the leftover dead code in `optimize_arguments` refactor: the inline dict comprehension assigned to `new_args` is immediately overwritten and never used. Keep the `optimize_arguments(str(tool_name), arguments_map)` assignment as the only source of `new_args`, and delete the obsolete comprehension in `commons.py` so the `new_args`/`arguments_map` flow is clear.server/tests/test_commons.py (1)
151-203: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winGood coverage, but missing edge cases matching the bugs in
optimize_arguments.Consider adding tests for:
timeout: 0forbash(currently falls through both the default and clamp branches, per the correctness issue flagged incommons.py).- A non-
lscommand sharing thelsprefix (e.g.,lsof -R .) to catch thestartswith("ls")over-match.- A chained command (e.g.,
"ls -R . && grep -R pattern file") to catch-Rbeing stripped outside thelsinvocation.🤖 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 `@server/tests/test_commons.py` around lines 151 - 203, Add missing regression tests in test_optimize_arguments_* to cover the optimize_arguments edge cases: verify bash calls with timeout=0 preserve the explicit zero timeout instead of falling through to defaults, verify a non-ls command like lsof -R . is not rewritten by the ls-specific prefix logic, and verify chained commands such as ls -R . && grep -R pattern file only strip -R from the ls invocation and do not alter other command segments.
🤖 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 `@server/backend/commons.py`:
- Around line 618-637: Fix optimize_arguments so it uses proper None checks,
restricts the ls cleanup to the actual ls command only, and handles timeout
values consistently. In optimize_arguments, replace the None comparisons with
identity checks, tighten the ls detection so it matches only the ls executable
rather than any prefix like lsof or lsblk, and avoid rewriting the full command
string in a way that strips -R from unrelated chained commands. Also treat a
provided timeout of 0 as an explicit value that should still be clamped or
defaulted according to the intended 60s cap.
---
Nitpick comments:
In `@server/backend/commons.py`:
- Around line 465-468: Remove the leftover dead code in `optimize_arguments`
refactor: the inline dict comprehension assigned to `new_args` is immediately
overwritten and never used. Keep the `optimize_arguments(str(tool_name),
arguments_map)` assignment as the only source of `new_args`, and delete the
obsolete comprehension in `commons.py` so the `new_args`/`arguments_map` flow is
clear.
In `@server/tests/test_commons.py`:
- Around line 151-203: Add missing regression tests in test_optimize_arguments_*
to cover the optimize_arguments edge cases: verify bash calls with timeout=0
preserve the explicit zero timeout instead of falling through to defaults,
verify a non-ls command like lsof -R . is not rewritten by the ls-specific
prefix logic, and verify chained commands such as ls -R . && grep -R pattern
file only strip -R from the ls invocation and do not alter other command
segments.
In `@tiles/src/utils/config.rs`:
- Line 621: The `handle_pi_settings_config` signature should accept `&Path`
instead of `&PathBuf` to avoid the `ptr_arg` lint and make the API more
idiomatic. Update the function parameter type in `handle_pi_settings_config`,
ensure `std::path::Path` is imported in `config.rs`, and leave the function body
unchanged since `&PathBuf` already dereferences to `&Path`.
🪄 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: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: ad47e7df-c574-467d-8c31-9b6cf176b2a8
📒 Files selected for processing (5)
server/backend/commons.pyserver/schemas.pyserver/tests/test_commons.pytiles/src/repl.rstiles/src/utils/config.rs
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
- fix: removed alphanumeric pattern check for toolname
- Due to harmony parsing issue sometimes model can have non-alphanum
names. The bigger problem is we append previous history to every request.
So once we get a 422, we will always get a 422.
- fix: cmpring exact `ls` + refactor
madclaws
force-pushed
the
fix/tool-call-improvements
branch
from
0918f9e to
a45d8b8
Compare
1 participant
No description provided.