This page captures the highest-impact refactoring ideas for the hirocli package (~12.5k lines, 75 files). Each item includes the problem, the proposed change, and what it fixes. Items are ranked by impact.
Per the project’s initial-development-mode rule, none of these require backward compatibility or migration wrappers.
1. Dependency injection and composition root
Problem. server_process.py::_main() is a ~140-line procedural bootstrap that manually constructs and wires 10+ components. Circular dependencies between CommunicationManager and ChannelManager are broken with post-construction setter calls (set_channel_manager, set_request_handler). workspace_path: Path is threaded through ~40 function signatures as a poor man’s service locator.
Additionally, http_server.py uses app.state.* as a second, parallel bag of attributes to pass references around.
Both CommunicationManager and ChannelManager maintain independent _device_name_cache dicts with near-identical lookup logic.
Proposed change.
- Introduce a
ServerContext dataclass that holds the shared runtime state: workspace_path, config, preferences, desktop_private_key, stop_event.
- Pass
ServerContext instead of threading workspace_path everywhere.
- Replace the bidirectional manager references with an event bus or mediator so neither manager imports or holds a direct reference to the other.
- Consolidate the duplicated device-name caches into a single
DeviceNameResolver on the context.
What it fixes.
- Fragile ordering of
set_* calls that are easy to forget or misordering.
workspace_path pollution across dozens of signatures.- Duplicated
_device_name_cache and _peer_label / _device_label logic.
- The
app.state.* bag of attributes in http_server.py.
Problem. tools/server.py was a 730-line file conflating at least four distinct responsibilities: result dataclasses, server process control, bootstrap helpers, autostart wrappers, a _NullConsole hack, and seven Tool classes.
The file was split into server.py, server_control.py, and server_models.py as a first pass. Further separation is possible.
Proposed change.
- Move bootstrap helpers (
ensure_mandatory_devices_channel, ensure_default_preferences) into domain/bootstrap.py — they are domain logic, not tool logic.
- Evaluate whether autostart wrappers belong in
domain/ rather than in server_control.py.
- Keep
server_control.py focused on process start/stop/graceful-shutdown only.
What it fixes.
- Single-responsibility violations.
- Navigability — finding the right code quickly.
- The
_NullConsole pattern (already eliminated in the split; keeping server_control functions console-free).
3. Add a test suite
Problem. There are zero test files despite dev dependencies (pytest, pytest-asyncio, httpx) being declared in pyproject.toml. The architecture lends itself well to testing — tools return plain dataclasses, domain functions are mostly pure with file I/O, and the adapter pipeline is a simple chain — but there is no safety net.
Proposed change. Build a test suite targeting the most critical paths first.
| Priority | Layer | Strategy |
|---|
| 1 | Domain (workspace.py, config.py, db.py, pairing.py) | Pure functions with a temp directory for the workspace. Easiest wins. |
| 2 | Tools (server.py, channel.py, device.py) | Each execute() returns a dataclass; mock the filesystem and assert results. |
| 3 | Runtime (communication_manager.py, channel_manager.py) | pytest-asyncio to test message routing with fake queues. |
| 4 | HTTP (http_server.py) | httpx + FastAPI TestClient for endpoint assertions. |
- No confidence when refactoring.
- Regressions land silently.
- Makes every other refactoring on this page safer to execute.
Tackle this alongside or before the other items so you can refactor with confidence.
Problem. channel_manager.py imports private symbols directly from communication_manager.py:
from .communication_manager import _LOG_IN, _LOG_OUT, _comm_extras, _comm_kind
CommunicationManager._peer_label vs ChannelManager._device_label).
Proposed change.
- Create
runtime/log_helpers.py with the shared constants and helper functions as proper public symbols.
- Consolidate device-name resolution into a single
DeviceNameResolver (ties into item 1).
- Evaluate whether these log helpers belong in
hiro-commons per the workspace rule about shared functionality.
What it fixes.
- Private-symbol imports across module boundaries.
- Duplicated cache and lookup logic.
- Tight coupling between peer managers.
Problem. Tool.execute() is strictly synchronous. The HTTP /invoke endpoint and admin UI call it from inside async handlers, blocking the event loop. The LangChain adapter wraps it in a sync StructuredTool.func instead of using the async coroutine parameter.
Assessment after tracing all four call paths.
| Caller | Context | Async needed? |
|---|
CLI (commands/*.py) | Typer — sync, no event loop | No |
LLM agent (langchain_adapter.py) | LangChain already runs sync func in a thread pool | Not urgent |
HTTP (http_server.py) | async def handler calls registry.invoke() synchronously | Yes, blocks event loop |
Admin UI (ui/pages/*.py) | NiceGUI’s async loop | Yes, blocks event loop |
StopTool / RestartTool which call _graceful_http_stop — that does HTTP polling with time.sleep(0.5) in a loop for up to 10 seconds.
Why this is deferred.
- Adding
aexecute() with a default asyncio.to_thread() wrapper introduces real OS threads where the code was previously single-threaded, creating potential race conditions.
- The surface area is large: 30+ CLI call sites and 15+ admin UI call sites would need to maintain both sync and async interfaces.
- A simpler targeted fix — wrapping just the
_graceful_http_stop polling in asyncio.to_thread() at the call site — solves the only real blocking pain without revamping the base class.
If you revisit this later, be cautious with asyncio.to_thread(). Tools that touch shared mutable state (device-name caches, workspace file writes, registry JSON I/O) could hit race conditions when running on thread pool workers.
