refactor(cli): extract command bodies into the shared service layer (#387 C1) - #399
Merged
Conversation
…387 C1) C1 of the TUI-operator-console PRD (O2: the TUI must dispatch to exactly what the CLI calls -- one implementation, two front-ends). Every CLI command body in keel/cli.py that carried logic now delegates to a service in keel/commands/*; the wrapper parses click options, builds the broker lazily at the _build_broker seam, calls the service, and prints/raises. keel/cli.py shrinks by ~1280 net lines and gains no behavior. Audit -> destination: command class service module (new home) ------------------ ----- ------------------------------------------- fetch (b) keel/commands/fetch.py (assess_products, render_freshness, run_fetch) monitor (b) keel/commands/monitor.py (monitor_cycle, run_monitor) simulate (b) keel/commands/simulate.py (run_simulation + the whole report assembly) assets screen/ (b) keel/commands/assets.py (screen_product = THE holdings/discover admission gate, market_facts, gather_holdings, run_discovery, renderers) agent (confirm (b) keel/commands/confirm.py (_interactive_confirm gate) + preview reading/rendering/markers) kill / resume / (b) keel/commands/trading.py (engage/disengage resume-entries / kill switch, clear_consecutive_loss_halt, record-flow / record_flow, reset_high_water_mark, reset-hwm render_loop_result) pnl (b) keel/commands/pnl.py (build_pnl_report + render_pnl_report) purification (b)* keel/commands/purification.py (renderer only; compute was already keel.compliance.purification) db/trials/withdrawals/ (a) already shared (keel/commands/* modules, autonomy/rules/subscription/ keel.agent, keel.analysis.pnl, status/insights/versions/ keel.data.*, Repository) assets attest*/exempt/ (a) thin over Repository writes list/propose agent (cycle/loop) (a) keel.agent.run_once/loop init/init-config/ (a) template read + keel.data.db.migrate; not in migrate the TUI menu tree, no extraction tui (a) views keel.commands.status/admission/activity Byte compatibility: same output strings, exit codes, error messages and echo ORDER; the services stream their progress through injected echo/ echo_err callables that the wrappers point at click.echo. The laziness of broker construction is preserved via build_client factories (--check / --no-fetch / all-current skip still never construct a broker). Every name the existing tests pin through keel.cli (_screen_product, _assess_products, _SIM_SLIPPAGE_PCT, _interactive_confirm + markers, history_mod/repair_mod aliases) is a re-import of the same service object -- not a copy -- and the pre-existing tests pass UNCHANGED (no test file modified). Parity + architecture pins (new tests, no existing assertions touched): - tests/commands/test_service_parity.py: for each extracted operation, the CLI invocation and a direct service call over the same fixtures produce identical stdout (minus the CLI's own disclaimer footer), identical report files (simulate), identical agent_state (trading mutations), and object identity for every re-exported name. - tests/commands/test_service_isolation.py: a fresh interpreter can import every service module AND keel.commands.tui without keel.cli appearing in sys.modules, and an AST scan pins that no keel/commands/* module imports the composition root at module load (the old lazy `from keel.cli import _screen_product` in tui.py is gone). Also: keel/commands/admission.py now imports the discovery defaults from keel/commands/assets.py instead of mirroring the CLI options by hand (one home for DEFAULT_MIN_QUOTE_24H_VOLUME / DEFAULT_DISCOVER_LIMIT). Gates: uv run pytest -q (3262 passed, 2 skipped); uv run ruff check keel tests packages; uv run mypy -- all clean. Fixes #387
…o the shared constant Review findings on #399: (1) the isolation scan checked top-level statements only, so the function-level 'from keel.cli import ...' dodge -- the exact regression the file's docstring claims to prevent -- passed both pins; ast.walk closes it, with a regression test feeding the scan the dodge itself (C1 re-pointed the TUI's last lazy keel.cli imports, so no legitimate dodge remains). (2) DAYS_PER_YEAR's docstring promised single-sourcing the discovery probe's four-year lookback, but run_discovery still hardcoded the literal -- wired. (3) fetch's 'never construct under --check' docstring now says precisely which legs pin construction vs method-calls.
This was referenced Aug 19, 2026
Merged
eaitbrahim
added a commit
that referenced
this pull request
Aug 20, 2026
Minor, not patch. Three things since v0.9.3 change what an implementor or a deployment can rely on: * A NEW DISTRIBUTION. `keel-broker-alpaca` (#382, #384) plus the paper-equities profile that selects it (#386), so a deployment can now be US equities via the broker port rather than crypto only. * THE PORT CONTRACT MOVED TWICE. `market_clock`/`market_schedule` made venues session-aware (#385), and `place_order` gained `idempotency_key` (#419). Both carry defaults so no CALLER breaks, but a third-party adapter that does not accept them is no longer a `Broker` -- the conformance suite now says so. That is exactly the kind of change a patch bump must not hide. * THE OPERATOR CONSOLE. The TUI became keel's console across #399-#408, and `keel update` (#415/#417) makes a deployment self-updating. Every pinned sibling moves with it. The four production distributions are required `==` at this exact version (`RELEASING.md`, "Release assets"), so a bump that missed one would install a mixed set -- the `keel-trader 0.5.7` against `keel-core 0.5.5` failure `keel versions` exists to catch, and which `~/keel` actually ran across two releases. Also in this window, on the Robinhood adapter: the best_bid_ask fixture corrected against the live venue (#414), a credential guard that catches the error it only claimed to (#416), pre-flight sizing reported on the preview (#418), transport backoff (#420), and the fenced one-order probe (#421). Co-authored-by: Claude Opus 5 (1M context) <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
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.
C1 of the TUI-operator-console PRD (§5): O2 — one implementation, two front-ends. Every CLI command body in
keel/cli.pythat carried logic now delegates to a service inkeel/commands/*; the wrapper parses click options, builds the broker lazily at the_build_brokerseam, calls the service, and prints/raises.keel/cli.pyshrinks by ~1280 net lines and gains no behavior.Audit → destination
fetchkeel/commands/fetch.py—assess_products(window-bounded sweep),render_freshness,run_fetch(check/skip/warm/repair flow,FetchResult)monitorkeel/commands/monitor.py—monitor_cycle,run_monitor(FR-9 session-aware loop, once-per-state-change skip lines)simulatekeel/commands/simulate.py—run_simulation+ the whole assembly (candles, #259 slippage pass, account metrics, tier matrix, verdict, trials row, report/artifact write)assets screen/holdings/discoverkeel/commands/assets.py—screen_product(THE admission gate),market_facts,VENUE,gather_holdings+render_holdings,run_discovery+render_discover,screen_products,broker_auth_hintagent's confirm gatekeel/commands/confirm.py—_interactive_confirm, preview reading/rendering, all markerskill/resume/resume-entries/record-flow/reset-hwmkeel/commands/trading.py— the state mutations +render_loop_result(typed prompts stay front-side per O3)pnlkeel/commands/pnl.py—build_pnl_report+render_pnl_reportpurificationkeel/commands/purification.py— render only; compute was alreadykeel.compliance.purificationdbtrialswithdrawalsautonomyrulessubscriptionstatusinsightsversionstuikeel/commands/*modules;keel.agent;keel.data.*assets attest/attest-instrument/exempt/unexempt/list/proposeRepositorywrites /keel.proposeragentcycle/loopkeel.agent.run_once/loopinit/init-config/migratekeel.data.db.migrate; scaffolding ops absent from the TUI menu treeAlso:
keel/commands/admission.pynow imports the discovery defaults fromkeel/commands/assets.pyinstead of mirroring the CLI options by hand (one home forDEFAULT_MIN_QUOTE_24H_VOLUME/DEFAULT_DISCOVER_LIMIT), andkeel/commands/tui.py's two lazyfrom keel.cli import _screen_productimports now come from the service layer — no cycle dodge left.Byte-compatibility guarantee
Same output strings, exit codes, error messages, log events and echo order — services stream progress through injected
echo/echo_errcallables the wrappers point atclick.echo. Broker construction stays lazy viabuild_clientfactories (--check,--no-fetchand the all-current skip still never construct a broker). Every name the existing tests pin throughkeel.cli(_screen_product,_assess_products,_SIM_SLIPPAGE_PCT,_interactive_confirm+ the preview markers, thehistory_mod/repair_modpatch aliases) is a re-import of the same service object, not a copy. The pre-existing suite passes unchanged — zero test files modified.Parity + architecture pins (new tests)
tests/commands/test_service_parity.py(19 tests): for each extracted operation, a CLI invocation and a direct service call over the same fixtures produce identical stdout (modulo the CLI's own disclaimer footer), identical report files (simulate), identicalagent_state(trading mutations), identical verdicts (assets screen), plus object-identity assertions for every re-exported name.tests/commands/test_service_isolation.py(2 tests): a fresh interpreter imports every service module andkeel.commands.tuiwithoutkeel.cliappearing insys.modules; an AST scan pins that nokeel/commands/*module imports the composition root at module load.Gates
uv run pytest -q— 3262 passed, 2 skippeduv run ruff check keel tests packages— cleanuv run mypy— cleanFixes #387