From 325f0198531e1fcc9ac7b58692ab8ae7aaafb653 Mon Sep 17 00:00:00 2001 From: Jwalin Shah Date: Wed, 22 Apr 2026 14:30:31 -0700 Subject: [PATCH] refactor(core): move CLI adapters out of src/core/ into per-domain cli modules (#758) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * 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>` 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::::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 * update * chore: update .gitignore to include scheduled_tasks.lock in app/.claude * ran formatter --------- Co-authored-by: Jwalin Shah Co-authored-by: Steven Enamakel Co-authored-by: Claude Sonnet 4.6 --- .gitignore | 1 + PLAN.md | 139 ++++ .../settings/panels/PrivacyPanel.tsx | 15 +- .../panels/__tests__/PrivacyPanel.test.tsx | 9 +- app/src/pages/onboarding/Onboarding.tsx | 4 +- .../pages/onboarding/steps/WelcomeStep.tsx | 4 +- app/src/utils/tauriCommands/aboutApp.ts | 8 +- app/test/e2e/helpers/artifacts.ts | 17 +- app/test/e2e/specs/agent-review.spec.ts | 10 +- app/test/wdio.conf.ts | 2 +- src/core/cli.rs | 101 +-- src/core/mod.rs | 3 - src/core/screen_intelligence_cli.rs | 699 ------------------ .../screen_intelligence/cli/capture.rs | 155 ++++ .../screen_intelligence/cli/doctor.rs | 107 +++ src/openhuman/screen_intelligence/cli/mod.rs | 204 +++++ .../screen_intelligence/cli/server.rs | 78 ++ .../screen_intelligence/cli/session.rs | 152 ++++ src/openhuman/screen_intelligence/engine.rs | 21 + src/openhuman/screen_intelligence/mod.rs | 1 + .../text_input/cli.rs} | 2 +- src/openhuman/text_input/mod.rs | 1 + .../tree_summarizer/cli.rs} | 2 +- src/openhuman/tree_summarizer/mod.rs | 1 + src/openhuman/voice/cli.rs | 122 +++ src/openhuman/voice/mod.rs | 1 + 26 files changed, 1008 insertions(+), 851 deletions(-) create mode 100644 PLAN.md delete mode 100644 src/core/screen_intelligence_cli.rs create mode 100644 src/openhuman/screen_intelligence/cli/capture.rs create mode 100644 src/openhuman/screen_intelligence/cli/doctor.rs create mode 100644 src/openhuman/screen_intelligence/cli/mod.rs create mode 100644 src/openhuman/screen_intelligence/cli/server.rs create mode 100644 src/openhuman/screen_intelligence/cli/session.rs rename src/{core/text_input_cli.rs => openhuman/text_input/cli.rs} (99%) rename src/{core/tree_summarizer_cli.rs => openhuman/tree_summarizer/cli.rs} (99%) create mode 100644 src/openhuman/voice/cli.rs diff --git a/.gitignore b/.gitignore index 07ffa5dd2..69683d52d 100644 --- a/.gitignore +++ b/.gitignore @@ -65,3 +65,4 @@ workflow .fastembed_cache overlay/src-tauri/target/ .claude/*.lock +app/.claude/scheduled_tasks.lock diff --git a/PLAN.md b/PLAN.md new file mode 100644 index 000000000..f3a652695 --- /dev/null +++ b/PLAN.md @@ -0,0 +1,139 @@ +# Backend Overhaul — Controller Registry Consolidation + +Branch: `feat/backend-overhaul` +Base: `main` @ 515c4f4b + +## Why + +Project rule (CLAUDE.md): domain functionality exposed to CLI + JSON-RPC **only** via the controller registry (`schemas.rs` per domain + `src/core/all.rs`). Current state is 90% there — 34 domains fully migrated — but six CLI adapters and one JSON-RPC fallback still live in the transport layer. Finishing this lets us delete ~95KB of adapter code and enforce the rule with a lint/grep check. + +## Goals + +1. Zero domain-specific code in `src/core/cli.rs`, `src/core/jsonrpc.rs`, `src/rpc/dispatch.rs`. +2. Every CLI command and JSON-RPC method dispatched through the shared registry. +3. No behavior change — all existing CLI flags and RPC methods preserve names, params, outputs. +4. Unregistered infrastructure modules documented as intentionally transport-agnostic. + +## Non-goals + +- No new domain functionality. +- No changes to skills QuickJS runtime exposure (stays runtime-only). +- No renames of existing CLI flags or RPC method names. +- No touching memory-tree domain (sanil-23 has in-flight PRs #732 #733). + +## Current leakage + +| File | LOC | What's there | Target | +|---|---|---|---| +| `src/core/agent_cli.rs` | 21.4K | agent CLI adapter | `openhuman/agent/schemas.rs` | +| `src/core/memory_cli.rs` | 15.3K | memory CLI adapter | `openhuman/memory/schemas.rs` | +| `src/core/text_input_cli.rs` | 12.4K | text_input CLI adapter | `openhuman/text_input/schemas.rs` | +| `src/core/screen_intelligence_cli.rs` | 26.5K | screen_intelligence CLI adapter | `openhuman/screen_intelligence/schemas.rs` | +| `src/core/tree_summarizer_cli.rs` | 13.2K | tree_summarizer CLI adapter | `openhuman/tree_summarizer/schemas.rs` | +| `src/core/cli.rs` (voice block) | ~2K | `run_voice_server_command()` | `openhuman/voice/schemas.rs` | +| `src/rpc/dispatch.rs` | 76 | `openhuman.security_policy_info` fallback | `openhuman/security/schemas.rs` (or about_app) | + +Total: ~95KB of adapter code to fold into registry. + +## Exposure rule (decision tree) + +Two legitimate shapes for exposing a domain capability. Pick by command shape, not by convenience: + +1. **Controller registry** (`src/openhuman//schemas.rs` + `src/core/all.rs`). + Use for **request/response RPC-style capabilities** — take params, do work, return JSON, done. The vast majority of domain methods. CLI access is free via the generic namespace dispatcher (`openhuman --args`). + +2. **Domain-owned CLI adapter** (`src/openhuman//cli.rs`). + Use only for **standalone long-running / blocking operational commands** — e.g. `openhuman voice` runs a hotkey listener forever, holds the process, logs to stderr. These don't fit the registry's fire-and-return JSON contract. The adapter lives *inside* the domain (not in `src/core/cli.rs`) so transport layer stays generic. + +Hard rule: no domain-specific imports or logic in `src/core/cli.rs`, `src/core/jsonrpc.rs`, or `src/rpc/dispatch.rs`. The only allowed reference is the dispatch line that routes `args[0]` to the domain's CLI function. Everything else lives in the domain. + +Don't invent a third shape. If a capability feels like it needs one, it probably belongs in one of the two above with a thin adjustment. + +## Approach + +Each migration is a self-contained diff: + +1. Confirm domain already has `schemas.rs` with `all__registered_controllers()`. +2. For each CLI subcommand in `_cli.rs`: + - Identify the underlying domain function it calls. + - Add a `handle_` fn in `schemas.rs` that takes `Map` and returns `ControllerFuture`. + - Register it in `all__registered_controllers()`. +3. Delete the `_cli.rs` file. +4. Remove its import from `src/core/cli.rs` / `mod.rs`. +5. Verify CLI flags still work via generic dispatcher — the registry already exposes them by method name. +6. `cargo check` + `cargo test -p openhuman` per step. + +The generic CLI adapter in `cli.rs` already knows how to route ` --key value` to registry method `openhuman._`. Migration is mostly deletion once handlers exist. + +## Phases + +### Phase 1 — Smallest adapter first (shakedown) + +**voice** (~2K inline in `cli.rs`). Lowest risk: one server command, confirms the approach end-to-end before touching bigger files. + +- Commit: `refactor(voice): move CLI adapter to controller registry` + +### Phase 2 — Mechanical migrations (one PR each) + +Order = smallest file first, so each PR is reviewable and CI catches issues early. + +1. `text_input` (12.4K) — likely thin wrappers. +2. `tree_summarizer` (13.2K). +3. `memory_cli` (15.3K) — **coordinate with sanil-23 memory-tree PRs**; rebase after #732/#733 merge or pair-review. +4. `agent_cli` (21.4K). +5. `screen_intelligence` (26.5K) — largest, leave for last. + +Each: one commit, one PR, self-contained. No bundling. + +### Phase 3 — JSON-RPC dispatch fallback + +Move `openhuman.security_policy_info` from `src/rpc/dispatch.rs` into whichever domain owns security policy (likely `about_app` or a new `security` module). Delete `rpc/dispatch.rs` if it becomes empty, else leave as a thin generic dispatch shim. + +### Phase 4 — Enforce the rule + +Add a build-time check or CI grep. The rule must forbid domain **imports/types** in the transport layer while still allowing the dispatcher to call the generic `::cli::run_*` entrypoints that live inside each domain: + +```bash +# Forbid `use crate::openhuman::::…` imports in transport files. +! grep -rE '^\s*use\s+crate::openhuman::[a-z_]+::' \ + src/core/cli.rs src/core/jsonrpc.rs src/rpc/dispatch.rs + +# Forbid any other domain reference that is NOT a call into ::cli::run_*(…). +# (The dispatcher is allowed to invoke `crate::openhuman::::cli::run_(...)`.) +! grep -rE 'crate::openhuman::[a-z_]+::' \ + src/core/cli.rs src/core/jsonrpc.rs src/rpc/dispatch.rs \ + | grep -vE 'crate::openhuman::[a-z_]+::cli::run_[a-z_]+\(' +``` + +Fails if any domain-specific import or non-dispatcher reference creeps back in. Commit as `chore(core): fence domain imports out of transport layer`. + +### Phase 5 — Document unregistered modules + +Add a short table to `CLAUDE.md` listing intentionally-unregistered infra modules (accessibility, approval, context, embeddings, integrations, node_runtime, overlay, providers, routing, tokenjuice, tool_timeout) with one-liner "why not in registry" for each. Also document skills as runtime-only. + +## Risks + +- **agent_cli / screen_intelligence size** — large files may hide subtle flag-parsing logic not obvious from a first read. Mitigation: test-drive each CLI subcommand manually after migration; add CLI smoke tests to the registry dispatcher if missing. +- **memory_cli collision with sanil-23** — memory-tree PRs touch adjacent paths. Mitigation: do phase 2.3 last, rebase on top of merged PRs. +- **CLI flag drift** — generic adapter parses flags by schema. If current `_cli.rs` does custom parsing (e.g. positional args, subcommand nesting), need to extend schema before deleting. Mitigation: read each CLI file top-to-bottom before writing handler. +- **Behavior diff at registry boundary** — registry handlers return `ControllerFuture` with JSON; `_cli.rs` may print formatted text. Mitigation: keep a thin formatter in the generic CLI path, not per-domain. + +## Verification per step + +```bash +cargo check --manifest-path Cargo.toml +cargo test -p openhuman --test json_rpc_e2e +# Manual CLI smoke for migrated domain: +./target/debug/openhuman --args... +``` + +Plus `yarn test:rust` for the full mock-backed integration suite before each PR. + +## Milestones + +- M1: Phase 1 done, shakedown merged. +- M2: Phases 2.1–2.5 done, all `*_cli.rs` deleted. +- M3: Phase 3 done, `rpc/dispatch.rs` cleaned. +- M4: Phase 4+5 done, rule enforced, docs updated. + +Estimate: M1 = 0.5d, M2 = 3–4d (one domain/day), M3 = 0.5d, M4 = 0.5d. Total ~1 week of focused work. diff --git a/app/src/components/settings/panels/PrivacyPanel.tsx b/app/src/components/settings/panels/PrivacyPanel.tsx index bf8353867..7a86f40c2 100644 --- a/app/src/components/settings/panels/PrivacyPanel.tsx +++ b/app/src/components/settings/panels/PrivacyPanel.tsx @@ -5,8 +5,8 @@ import { useCoreState } from '../../../providers/CoreStateProvider'; import { type Capability, type CapabilityPrivacy, - type PrivacyDataKind, listCapabilities, + type PrivacyDataKind, } from '../../../utils/tauriCommands/aboutApp'; import SettingsHeader from '../components/SettingsHeader'; import { useSettingsNavigation } from '../hooks/useSettingsNavigation'; @@ -94,9 +94,7 @@ const PrivacyPanel = () => {

Loading privacy details…

)} {loadState === 'error' && ( -

+

Couldn’t load the live privacy list. Analytics controls below still work.

)} @@ -106,14 +104,9 @@ const PrivacyPanel = () => {

)} {loadState === 'ready' && capabilities.length > 0 && ( -
    +
      {capabilities.map(cap => ( -
    • +

    • {cap.name}

      diff --git a/app/src/components/settings/panels/__tests__/PrivacyPanel.test.tsx b/app/src/components/settings/panels/__tests__/PrivacyPanel.test.tsx index f42be3690..cb7e78bc6 100644 --- a/app/src/components/settings/panels/__tests__/PrivacyPanel.test.tsx +++ b/app/src/components/settings/panels/__tests__/PrivacyPanel.test.tsx @@ -5,15 +5,10 @@ import { renderWithProviders } from '../../../../test/test-utils'; import { type Capability, listCapabilities } from '../../../../utils/tauriCommands/aboutApp'; import PrivacyPanel from '../PrivacyPanel'; -vi.mock('../../../../utils/tauriCommands/aboutApp', () => ({ - listCapabilities: vi.fn(), -})); +vi.mock('../../../../utils/tauriCommands/aboutApp', () => ({ listCapabilities: vi.fn() })); vi.mock('../../../../providers/CoreStateProvider', () => ({ - useCoreState: () => ({ - snapshot: { analyticsEnabled: false }, - setAnalyticsEnabled: vi.fn(), - }), + useCoreState: () => ({ snapshot: { analyticsEnabled: false }, setAnalyticsEnabled: vi.fn() }), })); vi.mock('../../hooks/useSettingsNavigation', () => ({ diff --git a/app/src/pages/onboarding/Onboarding.tsx b/app/src/pages/onboarding/Onboarding.tsx index 4c83205e6..c4a7959c4 100644 --- a/app/src/pages/onboarding/Onboarding.tsx +++ b/app/src/pages/onboarding/Onboarding.tsx @@ -103,7 +103,9 @@ const Onboarding = ({ onComplete, onDefer }: OnboardingProps) => { }; return ( -
      +
      {onDefer && (