Refactor/sync command closures#8
Merged
Merged
Conversation
Replace the monolithic Command enum and CommandDispatcher with a trait-based CommandRegistry that supports both async Command impls and lightweight SyncCommand closures. Architecture: - Add Command trait, SyncCommand<F> wrapper, AliasCommand, and CommandRegistry to registry.rs - Extract CommandContext from the old CommandDispatcher - 18 sync commands converted from struct+impl Command to closures registered via register_sync()/register_sync_with_usage() - 6 async commands kept as impl Command (need provider.lock().await): CompactCommand, InitCommand, ModelsCommand, ModelCommand, TimeoutCommand, RetriesCommand Benefits: - Eliminates ~18 struct definitions and impl Command blocks - Command registration is declarative and centralized in build_registry() - Sync commands no longer need Pin<Box<dyn Future>> boilerplate - Individual command files are simpler (just logic functions) - Net -231 lines across the codebase (789 added, 1020 removed) All 87 tests pass. Clippy clean. Formatted with cargo fmt.
- Convert CompactCommand, TimeoutCommand, RetriesCommand, ModelsCommand, InitCommand, and ConfigSettingsCommand to use the async_command! macro - Eliminates ~25 lines of boilerplate per async command - Net -105 lines across 6 files (144 additions, 249 deletions) - All 87 tests passing, clean clippy
- Adjust heuristic constants (chars/token 4.0→3.2, tokens/word 1.3→1.6, overhead 2→4) to better match real tokenizer behavior for tool outputs - Capture ground-truth prompt_eval_count from Ollama's final_data on every chat call, falling back to heuristic only when message count has changed - Invalidate cached count after slash commands that may mutate messages
The execute_tool_call conversion from serde_json::Value to HashMap<String, String> used v.as_str() which only works for JSON string values. When the LLM emits numeric params like "timeout": 60000 (common with many models), as_str() returned None, producing "" which silently failed parse::<u64>() and fell back to the 30000 default. Fixed by matching on Value::String vs everything else, using to_string() for non-string values (Number, Bool, etc.).
PTFOPlayer
added a commit
that referenced
this pull request
May 19, 2026
Merge pull request #8 from PTFOPlayer/refactor/sync-command-closures
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Merge pull request: Refactor command architecture + token tracking fixes
High-level:
Replace the monolithic
Commandenum with a trait-basedCommandRegistrysupporting lightweight sync closures. Convert all remaining async commands toasync_command!macro. Fix two bugs: JSON numeric tool argument parsing and context-token undercounting.Changes (
refactor/sync-command-closures, 4 commits, -231 net lines):a089f37— ReplaceCommandenum with trait-based registry + sync closuresCommandtrait,SyncCommand<F>wrapper,AliasCommand, andCommandRegistryincommands/registry.rsCommandContextfrom the oldCommandDispatcherstruct + impl Commandto closures registered viaregister_sync()/register_sync_with_usage()impl Command, later converted in the next commitimpl Commandboilerplate69561db— Convert remaining async commands toasync_command!macroCompactCommand,TimeoutCommand,RetriesCommand,ModelsCommand,InitCommand,ConfigSettingsCommande755f76— Use Ollamaprompt_eval_countfor accurate context token trackingRole::Toolwas being undercounted)prompt_eval_countfrom Ollama'sfinal_dataresponse, caching it alongside the message count at capture time03aef90— Handle JSON numeric tool arguments (timeout, etc.)execute_tool_callusedv.as_str()which returnedNonefor JSON number values, silently producing""that fell throughparse::<u64>()to defaultsValue::Stringvs everything else, usingto_string()for non-string values (numbers, booleans)