refactor(voice): move CLI adapter to controller registry#1005
Conversation
📝 WalkthroughWalkthroughIntroduces a namespace-based CLI adapter registry in the core module. New public types define handlers and adapters; a lazy global store initializes with a "voice" adapter. The CLI dispatch logic is updated to query this registry for namespace handlers instead of relying on hard-coded subcommands. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/core/cli.rs (1)
350-376:⚠️ Potential issue | 🟠 MajorDispatch to CLI adapters before controller-schema lookup.
Right now the adapter is only consulted for empty/help args after
grouped.get(namespace), so real invocations likeopenhuman voice --hotkey ...are parsed as controller functions and never reachrun_standalone_subcommand. That also leaves CLI-only aliases likedictatedead unless they happen to have schemas.Suggested fix
- let Some(schemas) = grouped.get(namespace) else { - return Err(anyhow::anyhow!( - "unknown namespace '{namespace}'. Run `openhuman --help` to see available namespaces." - )); - }; - let preparsed = autocomplete_cli_adapter::preparse_namespace(namespace, args); let args: &[String] = &preparsed.args; if let Some((verbose, scope)) = preparsed.init_logging { crate::core::logging::init_for_cli_run(verbose, scope); } - if args.is_empty() || is_help(&args[0]) { - // If there's a domain-specific CLI handler for this namespace, use it as the default. - if let Some(cli_handler) = all::cli_handler_for_namespace(namespace) { - return cli_handler(args); - } - print_namespace_help(namespace, schemas); - return Ok(()); + if let Some(cli_handler) = all::cli_handler_for_namespace(namespace) { + if args.is_empty() || is_help(&args[0]) || args[0].starts_with('-') { + return cli_handler(args); + } } + + let Some(schemas) = grouped.get(namespace) else { + return Err(anyhow::anyhow!( + "unknown namespace '{namespace}'. Run `openhuman --help` to see available namespaces." + )); + };As per coding guidelines, expose domain functionality through the controller registry and avoid one-off transport logic in
src/core/cli.rs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/cli.rs` around lines 350 - 376, Move the autocomplete adapter dispatch (autocomplete_cli_adapter::preparse_namespace and the call to all::cli_handler_for_namespace) to run before the controller schema lookup (grouped.get(namespace)) so adapter-handled invocations and CLI-only aliases are resolved prior to treating args as controller functions; specifically, call preparse_namespace early, check preparsed.init_logging, then if preparsed.args is empty or is_help OR if all::cli_handler_for_namespace(namespace) returns Some, invoke that cli handler and return; only after adapter dispatch fails should you call grouped.get(namespace) and proceed to schema lookup and run_standalone_subcommand. Ensure you preserve existing logging initialization (crate::core::logging::init_for_cli_run) when moving the preparse call.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/core/all.rs`:
- Around line 74-79: The CLI registry currently only registers namespace "voice"
so "openhuman dictate" won't match; update the CLI_ADAPTERS initialization to
also register the "dictate" alias by adding a second RegisteredCliAdapter (or
otherwise include both namespace strings) that uses the same handler
crate::openhuman::voice::cli::run_standalone_subcommand so both "voice" and
"dictate" resolve to the same entrypoint.
---
Outside diff comments:
In `@src/core/cli.rs`:
- Around line 350-376: Move the autocomplete adapter dispatch
(autocomplete_cli_adapter::preparse_namespace and the call to
all::cli_handler_for_namespace) to run before the controller schema lookup
(grouped.get(namespace)) so adapter-handled invocations and CLI-only aliases are
resolved prior to treating args as controller functions; specifically, call
preparse_namespace early, check preparsed.init_logging, then if preparsed.args
is empty or is_help OR if all::cli_handler_for_namespace(namespace) returns
Some, invoke that cli handler and return; only after adapter dispatch fails
should you call grouped.get(namespace) and proceed to schema lookup and
run_standalone_subcommand. Ensure you preserve existing logging initialization
(crate::core::logging::init_for_cli_run) when moving the preparse call.
🪄 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: e635be14-9833-462e-9378-68d1f2ba9846
📒 Files selected for processing (2)
src/core/all.rssrc/core/cli.rs
| CLI_ADAPTERS.get_or_init(|| { | ||
| vec![RegisteredCliAdapter { | ||
| namespace: "voice", | ||
| handler: crate::openhuman::voice::cli::run_standalone_subcommand, | ||
| }] | ||
| }) |
There was a problem hiding this comment.
Register the dictate alias alongside voice.
run_standalone_subcommand is documented to handle both openhuman voice and openhuman dictate, but the registry only exposes "voice". With exact namespace matching, openhuman dictate will still miss the adapter and fall back to an unknown-namespace error.
Suggested fix
CLI_ADAPTERS.get_or_init(|| {
- vec![RegisteredCliAdapter {
- namespace: "voice",
- handler: crate::openhuman::voice::cli::run_standalone_subcommand,
- }]
+ vec![
+ RegisteredCliAdapter {
+ namespace: "voice",
+ handler: crate::openhuman::voice::cli::run_standalone_subcommand,
+ },
+ RegisteredCliAdapter {
+ namespace: "dictate",
+ handler: crate::openhuman::voice::cli::run_standalone_subcommand,
+ },
+ ]
})As per coding guidelines, wire domain exports into src/core/all.rs so transport layers stay generic.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/core/all.rs` around lines 74 - 79, The CLI registry currently only
registers namespace "voice" so "openhuman dictate" won't match; update the
CLI_ADAPTERS initialization to also register the "dictate" alias by adding a
second RegisteredCliAdapter (or otherwise include both namespace strings) that
uses the same handler crate::openhuman::voice::cli::run_standalone_subcommand so
both "voice" and "dictate" resolve to the same entrypoint.
…i#1005) Co-authored-by: Jwalin Shah <jshah1331@gmail.com>
3 participants
Summary
"voice" | "dictate"branch fromsrc/core/cli.rsdispatchRegisteredCliAdapter+CLI_ADAPTERSstatic registry insrc/core/all.rscli_handler_for_namespace()lookup so the voice CLI is wired the same way as RPC controllers — via the registry, not ad-hoc branchingrun_namespace_commandnow falls through to the registry before printing help, making future CLI adapters zero-touch incli.rsTest plan
cargo checkpasses clean (pre-existingprivate_interfaceswarning only, unrelated)cargo fmt --checkpassesopenhuman voice/openhuman dictatesubcommands still dispatch correctlyall.rs, no change tocli.rsSummary by CodeRabbit