refactor(core): move CLI adapters out of src/core/ into per-domain cli modules#758
Merged
Merged
Conversation
Move the blocking `openhuman voice` / `openhuman dictate` dictation-server subcommand out of `src/core/cli.rs` and into a new `src/openhuman/voice/cli.rs` owned by the voice domain. The core CLI dispatcher now routes to `voice::cli::run_standalone_subcommand` and no longer imports voice internals. Why this shape: the standalone server blocks forever on the hotkey listener, which doesn't fit the controller registry's request/response contract. A domain-owned CLI adapter is the right home for long-lived operational commands; the controller registry stays for RPC-style capabilities. Establishes the exposure rule documented in PLAN.md for the rest of the backend overhaul. - src/openhuman/voice/cli.rs: new 118-line adapter (flag parsing, tokio runtime, run_standalone call) - src/openhuman/voice/mod.rs: declare `pub mod cli` - src/core/cli.rs: delete 92-line run_voice_server_command, dispatch only - PLAN.md: new scope doc + exposure rule
Move src/core/text_input_cli.rs to src/openhuman/text_input/cli.rs and drop the stale `pub mod text_input_cli` from src/core/mod.rs. Core CLI dispatcher now routes `text-input` to the domain-owned adapter. The moved file has a mixed shape that validates the exposure rule from the previous voice commit: - `run` starts a long-lived HTTP JSON-RPC server (domain cli.rs shape) - `read / insert / ghost / dismiss` are short-lived UX wrappers around `text_input::rpc::*` (pretty-print, flag parsing — CLI affordance, not transport duplication) Both shapes legitimately belong in the domain. The controller registry stays for transport-agnostic capabilities; CLI UX wrappers live next to their domain RPC. - git mv src/core/text_input_cli.rs src/openhuman/text_input/cli.rs - src/openhuman/text_input/mod.rs: declare `pub mod cli` - src/core/mod.rs: drop `pub mod text_input_cli` - src/core/cli.rs: update dispatch path
git mv src/core/tree_summarizer_cli.rs src/openhuman/tree_summarizer/cli.rs and drop the stale `pub mod tree_summarizer_cli` from src/core/mod.rs. Core CLI dispatcher now routes `tree-summarizer` to the domain-owned adapter. All five subcommands (ingest / run / query / status / rebuild) are short -lived UX wrappers over registry handlers that already exist in tree_summarizer/schemas.rs — no long-running server, no business logic duplicated. Straight relocation keeps transport generic. - git mv src/core/tree_summarizer_cli.rs src/openhuman/tree_summarizer/cli.rs - src/openhuman/tree_summarizer/mod.rs: declare `pub mod cli` - src/core/mod.rs: drop `pub mod tree_summarizer_cli` - src/core/cli.rs: update dispatch path
Move src/core/screen_intelligence_cli.rs (699 lines, 7 subcommands) into
a domain-owned cli/ submodule, split by responsibility so no single file
exceeds the project's 500-line soft limit.
Layout:
screen_intelligence/cli/mod.rs (204) dispatch + shared helpers
(CliOpts, parse_opts,
bootstrap_engine, logging,
print_help)
screen_intelligence/cli/server.rs (79) run_server (long-running
capture+vision loop)
screen_intelligence/cli/session.rs (150) status / start / stop
screen_intelligence/cli/capture.rs (173) capture / vision (inspect pair)
screen_intelligence/cli/doctor.rs (107) readiness diagnostics
Classification before moving:
- no business-logic duplication vs schemas.rs handlers
- heavy fns (doctor 103, capture 97, start 89) were 80%+ pretty-printing
and CLI-side orchestration (polling loop, flag-driven save) — legit
CLI UX, not extracted domain logic
- shared helpers stayed pub(super) inside mod.rs; no new domain APIs
introduced
Core transport changes:
- delete src/core/screen_intelligence_cli.rs
- drop `pub mod screen_intelligence_cli` from src/core/mod.rs
- src/core/cli.rs dispatch routes to domain cli::
After this commit, src/core/cli.rs contains only dispatch lines for all
four migrated domains (voice, text_input, tree_summarizer,
screen_intelligence) — no embedded domain logic.
…ure save + tighten cli visibility
Follow-ups from codex + gemini review of the branch:
1. Extract CaptureFrame construction + disk save from the CLI.
`capture.rs --keep` was building a CaptureFrame inline (stamping
timestamps, copying context fields) and calling
`AccessibilityEngine::save_screenshot_to_disk` directly. Added
`AccessibilityEngine::save_capture_test_result(workspace_dir,
&CaptureTestResult, reason) -> Option<Result<PathBuf, String>>`
so the CLI only passes the result through and picks a reason
label. Domain owns the frame shape, not the CLI.
2. Tighten CLI module visibility across all four migrated domains.
`pub mod cli` -> `pub(crate) mod cli` and
`pub fn run_*_command` -> `pub(crate) fn` for voice, text_input,
tree_summarizer, screen_intelligence. Only src/core/cli.rs needs
to see these; they're transport plumbing, not domain public API.
No behavior change.
Contributor
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 31 minutes and 56 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (15)
📝 WalkthroughWalkthroughThis PR migrates CLI subcommand implementations from centralized Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 |
senamakel
previously approved these changes
Apr 22, 2026
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
src/openhuman/screen_intelligence/engine.rs (1)
486-490: Consider keeping this helper crate-private.
AccessibilityEngineis publicly re-exported, so this newpub fnbecomes public API even though the CLI caller only needs crate access. Narrowing it matches the rest of the visibility-tightening in this PR.Proposed visibility tightening
- pub fn save_capture_test_result( + pub(crate) fn save_capture_test_result( workspace_dir: &std::path::Path, result: &CaptureTestResult, reason: &str,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/openhuman/screen_intelligence/engine.rs` around lines 486 - 490, The helper function save_capture_test_result is unnecessarily public and should be restricted to crate scope to avoid expanding the public API; change its signature from pub fn save_capture_test_result(...) to pub(crate) fn save_capture_test_result(...) (or remove the pub to make it private) so only crate-internal callers (including the CLI) can use it, keeping AccessibilityEngine's public re-exports unaffected.src/openhuman/screen_intelligence/cli/mod.rs (1)
98-105: Minor: the-h/--helpbranch inparse_optsis dead code.Every sibling caller (
capture.rs,session.rs,doctor.rs,server.rs) doesif args.iter().any(|a| is_help(a)) { print help; return }before invokingparse_opts, sorestnever actually receives-h/--helpin practice. Either drop the branch or, better, fold help-detection intoparse_optsso each subcommand can't forget it:- "-h" | "--help" => { - rest.push(args[i].clone()); - i += 1; - } _ => { rest.push(args[i].clone()); i += 1; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/openhuman/screen_intelligence/cli/mod.rs` around lines 98 - 105, The "-h"/"--help" arm in parse_opts is dead because callers (capture.rs, session.rs, doctor.rs, server.rs) already check is_help; refactor parse_opts to own help-detection instead of keeping the unreachable branch: modify parse_opts to scan its input args with is_help at the start and return a clear Help outcome (e.g., Result/enum variant or Err with a Help marker) so callers can act uniformly, then remove the explicit "-h"/"--help" match arm from the main match; update callers to handle the new Help outcome rather than performing their own is_help checks.PLAN.md (1)
96-98: Nit: tag the fenced block with a language (markdownlint MD040).-``` -! grep -rE 'openhuman::[a-z_]+::' src/core/cli.rs src/core/jsonrpc.rs src/rpc/dispatch.rs -``` +```sh +! grep -rE 'openhuman::[a-z_]+::' src/core/cli.rs src/core/jsonrpc.rs src/rpc/dispatch.rs +```🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@PLAN.md` around lines 96 - 98, The fenced code block containing the grep command lacks a language tag; update the block to use a shell language tag (e.g., change the triple backticks to ```sh) so markdownlint MD040 is satisfied. Locate the block with the command "! grep -rE 'openhuman::[a-z_]+::' src/core/cli.rs src/core/jsonrpc.rs src/rpc/dispatch.rs" and replace the opening fence with ```sh (leave the command and closing fence unchanged).src/openhuman/screen_intelligence/cli/session.rs (1)
81-112: Consider wiring Ctrl+C explicitly so the session stops cleanly.The poll loop exits only when
status.session.activeflips to false (TTL or external stop). When the user presses Ctrl+C, the process dies immediately andengine.stop_session(...)never runs — any in-flight capture/vision work is dropped and the stop reason recorded by the engine won't be"cli_stop".The voice standalone server (
src/openhuman/voice/server.rs:1028-1053) usestokio::signal::ctrl_cto callstop()before shutdown; mirroring that here would make--ttland Ctrl+C behave symmetrically:+ let stop_engine = engine.clone(); + tokio::spawn(async move { + if tokio::signal::ctrl_c().await.is_ok() { + let _ = stop_engine.stop_session(Some("cli_sigint".to_string())).await; + } + }); let mut tick = tokio::time::interval(std::time::Duration::from_secs(5));Not a correctness bug — just a UX/observability improvement worth considering before the follow-up CLI smoke tests land.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/openhuman/screen_intelligence/cli/session.rs` around lines 81 - 112, The polling loop only exits when status.session.active becomes false, so Ctrl+C currently kills the process without calling engine.stop_session and recording the "cli_stop" reason; fix this by wiring tokio::signal::ctrl_c into the loop (use tokio::select! between tick.tick().await and tokio::signal::ctrl_c().await) and when the Ctrl+C branch fires call engine.stop_session(Some("cli_stop".into())) (or the appropriate engine.stop/stop_session method) and then break; reference the existing tick/tick.tick().await loop, engine.status(), and engine.stop_session for where to insert the select and shutdown 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 `@PLAN.md`:
- Around line 92-100: The Phase 4 check is too broad and will flag the intended
dispatcher references like crate::openhuman::voice::cli::… in src/core/cli.rs
and src/rpc/dispatch.rs; tighten the grep so it forbids domain layer
imports/types but permits dispatcher call-sites—for example, change the rule to
only match import statements that begin with "use crate::openhuman::" or to
exclude occurrences that are followed by "::cli::run_" (i.e., allow
crate::openhuman::<domain>::cli::run_* calls) so the dispatch lines introduced
in src/core/cli.rs and src/core/jsonrpc.rs are not rejected while still
preventing other domain leaks.
In `@src/openhuman/screen_intelligence/cli/server.rs`:
- Around line 46-70: The server_config currently sets keep_screenshots to only
opts.keep, which can disagree with the reported status that uses opts.keep ||
config.screen_intelligence.keep_screenshots; update the construction of
crate::openhuman::screen_intelligence::server::SiServerConfig (server_config) to
compute an effective_keep value using the same expression (opts.keep ||
config.screen_intelligence.keep_screenshots) and assign that to keep_screenshots
so the server behavior matches the CLI output (referencing server_config,
SiServerConfig, keep_screenshots, opts.keep, and
config.screen_intelligence.keep_screenshots).
In `@src/openhuman/voice/cli.rs`:
- Around line 79-87: Replace the silent config fallback and permissive mode
parsing: do not call unwrap_or_default() on Config::load_or_init().await;
instead handle the Result from Config::load_or_init(), log or return the error
(e.g., propagate it up from the CLI entrypoint) so malformed config or I/O
failures are visible instead of silently using Config::default(), then still
call config.apply_env_overrides() on success; also validate the CLI `mode` when
constructing `activation_mode` (the `mode` variable / match that sets
`activation_mode`), returning an error or printing usage if the value is not
exactly "tap" or "push" rather than silently treating unknown values as Push.
---
Nitpick comments:
In `@PLAN.md`:
- Around line 96-98: The fenced code block containing the grep command lacks a
language tag; update the block to use a shell language tag (e.g., change the
triple backticks to ```sh) so markdownlint MD040 is satisfied. Locate the block
with the command "! grep -rE 'openhuman::[a-z_]+::' src/core/cli.rs
src/core/jsonrpc.rs src/rpc/dispatch.rs" and replace the opening fence with
```sh (leave the command and closing fence unchanged).
In `@src/openhuman/screen_intelligence/cli/mod.rs`:
- Around line 98-105: The "-h"/"--help" arm in parse_opts is dead because
callers (capture.rs, session.rs, doctor.rs, server.rs) already check is_help;
refactor parse_opts to own help-detection instead of keeping the unreachable
branch: modify parse_opts to scan its input args with is_help at the start and
return a clear Help outcome (e.g., Result/enum variant or Err with a Help
marker) so callers can act uniformly, then remove the explicit "-h"/"--help"
match arm from the main match; update callers to handle the new Help outcome
rather than performing their own is_help checks.
In `@src/openhuman/screen_intelligence/cli/session.rs`:
- Around line 81-112: The polling loop only exits when status.session.active
becomes false, so Ctrl+C currently kills the process without calling
engine.stop_session and recording the "cli_stop" reason; fix this by wiring
tokio::signal::ctrl_c into the loop (use tokio::select! between
tick.tick().await and tokio::signal::ctrl_c().await) and when the Ctrl+C branch
fires call engine.stop_session(Some("cli_stop".into())) (or the appropriate
engine.stop/stop_session method) and then break; reference the existing
tick/tick.tick().await loop, engine.status(), and engine.stop_session for where
to insert the select and shutdown call.
In `@src/openhuman/screen_intelligence/engine.rs`:
- Around line 486-490: The helper function save_capture_test_result is
unnecessarily public and should be restricted to crate scope to avoid expanding
the public API; change its signature from pub fn save_capture_test_result(...)
to pub(crate) fn save_capture_test_result(...) (or remove the pub to make it
private) so only crate-internal callers (including the CLI) can use it, keeping
AccessibilityEngine's public re-exports unaffected.
🪄 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: 87844e0a-39ba-447b-8beb-bcffdc4336c0
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (17)
PLAN.mdsrc/core/cli.rssrc/core/mod.rssrc/core/screen_intelligence_cli.rssrc/openhuman/screen_intelligence/cli/capture.rssrc/openhuman/screen_intelligence/cli/doctor.rssrc/openhuman/screen_intelligence/cli/mod.rssrc/openhuman/screen_intelligence/cli/server.rssrc/openhuman/screen_intelligence/cli/session.rssrc/openhuman/screen_intelligence/engine.rssrc/openhuman/screen_intelligence/mod.rssrc/openhuman/text_input/cli.rssrc/openhuman/text_input/mod.rssrc/openhuman/tree_summarizer/cli.rssrc/openhuman/tree_summarizer/mod.rssrc/openhuman/voice/cli.rssrc/openhuman/voice/mod.rs
💤 Files with no reviewable changes (2)
- src/core/mod.rs
- src/core/screen_intelligence_cli.rs
- screen_intelligence/cli/server: use effective keep_screenshots (opts.keep || config flag) for both SiServerConfig and status output, so the server behaves consistently with what the CLI reports. - voice/cli: surface config load errors with a warning instead of silently falling back to Config::default(); reject unknown --mode values instead of silently coercing them to Push. - PLAN.md: narrow the Phase 4 enforcement grep so it forbids domain imports/non-dispatcher references in the transport layer while still allowing crate::openhuman::<domain>::cli::run_*(...) dispatch calls, which this PR legitimately introduces.
…e) (addresses @coderabbitai nitpick on engine.rs:486) AccessibilityEngine::save_capture_test_result is only called from the domain's own cli/capture.rs (crate-internal). Making it pub leaks it into the public API surface unnecessarily — tighten to pub(crate) to match the visibility-tightening done across the rest of this PR. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Member
PR Manager — Review Triage CompleteTriaged all open review comments on this PR. Here is what was found and done: Review comments processed (4 total)3 actionable comments — already addressed in commits 62fe1ed–4e1e4e1:
1 nitpick — applied in this session (commit e664431):
Standards pass
Quality results
Merge conflict noteThe PR shows Commits pushed
Outstanding issues requiring human attention
|
Contributor
|
Tip For best results, initiate chat on the files or code changes.
The only remaining gate before merge is the Resolving all open review comments and approving. [resolve] 🧠 Learnings used |
4 tasks
AusAgentSmith
pushed a commit
to AusAgentSmith/openhuman
that referenced
this pull request
May 23, 2026
…i modules (tinyhumansai#758) * refactor(voice): move standalone CLI adapter into voice domain Move the blocking `openhuman voice` / `openhuman dictate` dictation-server subcommand out of `src/core/cli.rs` and into a new `src/openhuman/voice/cli.rs` owned by the voice domain. The core CLI dispatcher now routes to `voice::cli::run_standalone_subcommand` and no longer imports voice internals. Why this shape: the standalone server blocks forever on the hotkey listener, which doesn't fit the controller registry's request/response contract. A domain-owned CLI adapter is the right home for long-lived operational commands; the controller registry stays for RPC-style capabilities. Establishes the exposure rule documented in PLAN.md for the rest of the backend overhaul. - src/openhuman/voice/cli.rs: new 118-line adapter (flag parsing, tokio runtime, run_standalone call) - src/openhuman/voice/mod.rs: declare `pub mod cli` - src/core/cli.rs: delete 92-line run_voice_server_command, dispatch only - PLAN.md: new scope doc + exposure rule * refactor(text_input): move CLI adapter into domain Move src/core/text_input_cli.rs to src/openhuman/text_input/cli.rs and drop the stale `pub mod text_input_cli` from src/core/mod.rs. Core CLI dispatcher now routes `text-input` to the domain-owned adapter. The moved file has a mixed shape that validates the exposure rule from the previous voice commit: - `run` starts a long-lived HTTP JSON-RPC server (domain cli.rs shape) - `read / insert / ghost / dismiss` are short-lived UX wrappers around `text_input::rpc::*` (pretty-print, flag parsing — CLI affordance, not transport duplication) Both shapes legitimately belong in the domain. The controller registry stays for transport-agnostic capabilities; CLI UX wrappers live next to their domain RPC. - git mv src/core/text_input_cli.rs src/openhuman/text_input/cli.rs - src/openhuman/text_input/mod.rs: declare `pub mod cli` - src/core/mod.rs: drop `pub mod text_input_cli` - src/core/cli.rs: update dispatch path * refactor(tree_summarizer): move CLI adapter into domain git mv src/core/tree_summarizer_cli.rs src/openhuman/tree_summarizer/cli.rs and drop the stale `pub mod tree_summarizer_cli` from src/core/mod.rs. Core CLI dispatcher now routes `tree-summarizer` to the domain-owned adapter. All five subcommands (ingest / run / query / status / rebuild) are short -lived UX wrappers over registry handlers that already exist in tree_summarizer/schemas.rs — no long-running server, no business logic duplicated. Straight relocation keeps transport generic. - git mv src/core/tree_summarizer_cli.rs src/openhuman/tree_summarizer/cli.rs - src/openhuman/tree_summarizer/mod.rs: declare `pub mod cli` - src/core/mod.rs: drop `pub mod tree_summarizer_cli` - src/core/cli.rs: update dispatch path * refactor(screen_intelligence): move + split CLI adapter into domain Move src/core/screen_intelligence_cli.rs (699 lines, 7 subcommands) into a domain-owned cli/ submodule, split by responsibility so no single file exceeds the project's 500-line soft limit. Layout: screen_intelligence/cli/mod.rs (204) dispatch + shared helpers (CliOpts, parse_opts, bootstrap_engine, logging, print_help) screen_intelligence/cli/server.rs (79) run_server (long-running capture+vision loop) screen_intelligence/cli/session.rs (150) status / start / stop screen_intelligence/cli/capture.rs (173) capture / vision (inspect pair) screen_intelligence/cli/doctor.rs (107) readiness diagnostics Classification before moving: - no business-logic duplication vs schemas.rs handlers - heavy fns (doctor 103, capture 97, start 89) were 80%+ pretty-printing and CLI-side orchestration (polling loop, flag-driven save) — legit CLI UX, not extracted domain logic - shared helpers stayed pub(super) inside mod.rs; no new domain APIs introduced Core transport changes: - delete src/core/screen_intelligence_cli.rs - drop `pub mod screen_intelligence_cli` from src/core/mod.rs - src/core/cli.rs dispatch routes to domain cli:: After this commit, src/core/cli.rs contains only dispatch lines for all four migrated domains (voice, text_input, tree_summarizer, screen_intelligence) — no embedded domain logic. * refactor(screen_intelligence): address external review — extract capture save + tighten cli visibility Follow-ups from codex + gemini review of the branch: 1. Extract CaptureFrame construction + disk save from the CLI. `capture.rs --keep` was building a CaptureFrame inline (stamping timestamps, copying context fields) and calling `AccessibilityEngine::save_screenshot_to_disk` directly. Added `AccessibilityEngine::save_capture_test_result(workspace_dir, &CaptureTestResult, reason) -> Option<Result<PathBuf, String>>` so the CLI only passes the result through and picks a reason label. Domain owns the frame shape, not the CLI. 2. Tighten CLI module visibility across all four migrated domains. `pub mod cli` -> `pub(crate) mod cli` and `pub fn run_*_command` -> `pub(crate) fn` for voice, text_input, tree_summarizer, screen_intelligence. Only src/core/cli.rs needs to see these; they're transport plumbing, not domain public API. No behavior change. * style: cargo fmt * address review comments from coderabbitai - screen_intelligence/cli/server: use effective keep_screenshots (opts.keep || config flag) for both SiServerConfig and status output, so the server behaves consistently with what the CLI reports. - voice/cli: surface config load errors with a warning instead of silently falling back to Config::default(); reject unknown --mode values instead of silently coercing them to Push. - PLAN.md: narrow the Phase 4 enforcement grep so it forbids domain imports/non-dispatcher references in the transport layer while still allowing crate::openhuman::<domain>::cli::run_*(...) dispatch calls, which this PR legitimately introduces. * style: cargo fmt voice cli * style: cargo fmt + bump tauri-cef vendor * fix(screen_intelligence): narrow save_capture_test_result to pub(crate) (addresses @coderabbitai nitpick on engine.rs:486) AccessibilityEngine::save_capture_test_result is only called from the domain's own cli/capture.rs (crate-internal). Making it pub leaks it into the public API surface unnecessarily — tighten to pub(crate) to match the visibility-tightening done across the rest of this PR. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * update * chore: update .gitignore to include scheduled_tasks.lock in app/.claude * ran formatter --------- Co-authored-by: Jwalin Shah <jshah1331@gmail.com> Co-authored-by: Steven Enamakel <enamakel@tinyhumans.ai> Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
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
3 participants
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.
Summary
Moves four domain-specific CLI adapters out of
src/core/and into their owningdomains under
src/openhuman/<domain>/cli[/]. Establishes a clear exposure rulefor CLI surfaces going forward (documented in
PLAN.mdand enforced by keepingsrc/core/cli.rsstrictly a router).Scope: voice, text_input, tree_summarizer, screen_intelligence.
Intentionally not in scope: memory (pending sanil-23 memory-tree PRs #732
/#733), agent (21K adapter warranting its own classification pass), and the
JSON-RPC adapter leakage in
src/core/jsonrpc.rs(different shape — transportglue vs CLI UX, separate scoping exercise).
Exposure rule (see
PLAN.md)Two legitimate shapes for exposing a domain capability:
<domain>/schemas.rs+src/core/all.rs). Forrequest/response RPC-style capabilities. CLI access is free via the generic
namespace dispatcher.
<domain>/cli.rsor<domain>/cli/). Forstandalone long-running / blocking operational commands and for
CLI-specific UX wrappers (flag names, pretty-print) over existing registry
handlers — i.e. anything that doesn't fit the registry's fire-and-return
JSON contract or wants nicer terminal UX than the generic dispatcher.
Hard rule: no domain-specific imports or logic in
src/core/cli.rs,src/core/jsonrpc.rs, orsrc/rpc/dispatch.rs. Core transport layer isgeneric; the only remaining reference to a domain in
src/core/cli.rsis thesingle dispatch line that routes the subcommand name to the domain's entry
function.
What moved
src/core/cli.rssrc/openhuman/voice/cli.rssrc/core/text_input_cli.rs(388 lines)src/openhuman/text_input/cli.rsrunserver +read/insert/ghost/dismissUX wrapperssrc/core/tree_summarizer_cli.rs(386 lines)src/openhuman/tree_summarizer/cli.rssrc/core/screen_intelligence_cli.rs(699 lines)src/openhuman/screen_intelligence/cli/{mod,server,session,capture,doctor}.rsrun+ UX wrappers, split into 5 files (none > 204 lines)Total:
src/core/loses ~1,500 lines of domain-specific code; domains owntheir CLI.
src/core/cli.rsis now strictly a router.Addressed review feedback
External review by codex (gpt-5.3-codex) and gemini
(gemini-3.1-pro-preview) ran against an earlier form of this branch and
surfaced two concerns that are fixed in this PR's final commit
(`0d560e72`):
Extracted
CaptureFrameconstruction + disk save out of the CLI.capture.rs --keephad inlineCaptureFrameconstruction and adirect call to
AccessibilityEngine::save_screenshot_to_disk. AddedAccessibilityEngine::save_capture_test_result(workspace_dir, &CaptureTestResult, reason)so the domain owns the frame shape andthe CLI only passes the result through with a reason label.
Tightened CLI module visibility from
pub mod clitopub(crate) mod cli(and entry fns topub(crate) fn) across allfour migrated domains. These adapters are transport plumbing for
src/core/cli.rs, not domain public API.Gemini additionally argued
tree_summarizershould've had its CLIdeleted and users forced through the generic namespace dispatcher. That
conflicts with this PR's stated exposure rule (
PLAN.md): CLI UXwrappers over registry handlers are legitimate when the generic
dispatcher gives worse UX (JSON params vs
--content <text>). Theregistry coverage already exists in
tree_summarizer/schemas.rs; theCLI is sugar.
Pre-existing tech debt noted but not in scope:
ops::accessibility_doctor_cli_json()returns CLI-shaped JSON from adomain op; should be a typed
RpcOutcome<DoctorStatus>with CLI doingthe formatting. Separate follow-up.
Test plan
cargo check --bin openhuman-coreclean after each commitopenhuman voice --help,openhuman text-input read,openhuman tree-summarizer status <ns>,openhuman screen-intelligence doctorall behave identically tomainhandler changes)
Follow-ups (tracked separately)
agent_cli.rsmigration — dedicated classification passmemory_cli.rsmigration — after sanil-23 feat(memory): Phase 1 memory tree - multi-source ingestion & canonical chunks (#707) #732 / feat(memory): Phase 2 memory tree - preprocessing, scoring, admission gate (#708) #733 landsrc/core/jsonrpc.rstransport layer cleanup — different shape fromCLI migration, needs its own scope
screen_intelligence/ops.rsdoctor pathSummary by CodeRabbit