From 2ebc8b2e18c63e84ecdcb40de92b5693ea3da3ba Mon Sep 17 00:00:00 2001 From: Elmehdi Aitbrahim Date: Thu, 27 Aug 2026 23:26:43 -0400 Subject: [PATCH] =?UTF-8?q?docs(decisions):=20the=20commands/=20survey=20?= =?UTF-8?q?=E2=80=94=20measured,=20classified,=20decided=20(#525)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ADR 0003: the #525 survey. commands/ is 13,552 lines (0.65x the engine, was 1.34x at filing) — 40.5% shared report assembly, 36.7% gate orchestration whose policy is engine-side, 13.9% glue, 5.3% CLI-only rendering, 31% of the layer prose. Ten decision-bearing paths named (_run_backtest's successors first); in every money case the deciding comparison lives in a compute module. Zero near-duplicate render paths across CLI/web/MCP. Decision: the shape stays; the one mechanical gap (the thinness AST pin does not scan keel/mcp) filed as #588. --- docs/decisions/0003-commands-layer-survey.md | 197 +++++++++++++++++++ 1 file changed, 197 insertions(+) create mode 100644 docs/decisions/0003-commands-layer-survey.md diff --git a/docs/decisions/0003-commands-layer-survey.md b/docs/decisions/0003-commands-layer-survey.md new file mode 100644 index 00000000..c85c0159 --- /dev/null +++ b/docs/decisions/0003-commands-layer-survey.md @@ -0,0 +1,197 @@ +# 0003 — The commands layer is the explanation engine, and stays — measured, not assumed + +Date: 2026-08-27 · Issue: #525 (raised in the SE walkthrough, `docs/presentations/se.md` §3 and +§9.2 — "`commands/` is 23,188 lines — bigger than the engine. Healthy separation, or a symptom?") · +Status: decided + +## Context + +`keel/commands/` was filed as larger than the engine it drives (23,188 vs 17,260 lines at filing). +#525 asked for a measurement-first survey before any refactor: split the layer by role, name any +decision-making code paths living there, quantify duplication across the front-ends, and record a +decision either way. This record is that survey and that decision. The method is reproducible from +the fenced snippets at the end; every number below is from `main` at 16725c1. + +**The premise dissolved before the survey ran.** #541 (PR #554) deleted the curses TUI and the +console layer reachable only from inside it: `commands/` went 24,709 → 13,552 lines (+297/−23,448 +with tests). The engine grew in the same window. Restating the issue's table with today's numbers: + +| module | at filing | today | | module | at filing | today | +|---------------|----------:|-------:|---------|-------------|----------:|-------:| +| commands/ | 23,188 | 13,552 | | strategy/ | 4,033 | 4,566 | +| | | | | sim/ | 3,375 | 3,449 | +| | | | | execution/ | 3,102 | 3,841 | +| | | | | data/ | 2,930 | 3,055 | +| | | | | research/ | 1,658 | 3,725 | +| | | | | analysis/ | 1,308 | 1,308 | +| | | | | compliance/ | 854 | 854 | +| | | | | **engine** | **17,260** | **20,798** | +| **ratio** | 1.34× | 0.65× | | | | | + +The CLI layer is now two-thirds the size of the compute modules combined (0.60× if `keel/agent.py`, +the live loop, is counted as engine). "Larger than the engine it drives" is no longer true; what +remains is the question the issue actually wanted answered — is what is left in the right place? + +### The role split (checklist item 1) + +Modules classified by dominant signal — what each imports from the compute trees, what shape its +functions have (`build_*`/`gather_*` assembly vs `render_*` formatting vs click bodies), and who +imports it back (the web API and MCP server import `keel.commands.*` as their service layer; +`keel/web/payload.py:108` states the contract: "…`keel.commands.*` is the SERVICE layer, which this +layer is allowed to read; the compute trees are not"): + +| role | modules (lines) | lines | share | +|------|-----------------|------:|------:| +| **A. Shared report assembly** — `build_*`/`gather_*` cores consumed by CLI *and* web/MCP | activity 1,539 · setup 1,131 · insights 932 · doctor 849 · status 578 · brokers 307 · jobs 150 | 5,486 | 40.5% | +| **B. Services & decision orchestration** — gate sequencing, backtest/pricing passes, screening; CLI-only today | rules 1,918 · update 1,151 · trials 637 · assets 705 · simulate 564 | 4,975 | 36.7% | +| **C. Front-end glue** — arg parsing, dispatch, I/O, launchers | fetch 352 · subscription 292 · confirm 197 · trading 183 · autonomy 153 · credentials 150 · monitor 125 · serve 108 · withdrawals 92 · versions 83 · db 49 · capabilities 48 · mcp 48 | 1,880 | 13.9% | +| **D. Report assembly + rendering, CLI-only** | admission 564 · pnl 107 · purification 51 | 722 | 5.3% | +| **E. Layer plumbing** | _common 238 · _products 222 · \_\_init\_\_ 29 | 489 | 3.6% | + +A line-level AST attribution of the whole layer (function bodies, by name class): 1,016 lines of +rendering functions, 3,734 of assembly/service functions, 1,292 of click command bodies, 3,320 of +module-private helpers — and 4,190 lines of module headers, docstrings and imports. That last +number is the tell: **31% of the layer is prose.** The docstrings are where the refusals explain +themselves, the gates cite their issues, and the display-vs-policy divergences are argued (e.g. +`status.py:164` spends eleven lines on why a display's `None` handling must *differ* from the +guard's). That is the explanatory surface the issue's "case that this is fine" describes, and it +does not exist anywhere else to move to. + +### The decision paths (checklist item 2) + +`_run_backtest`, named by #335, no longer exists: the #390 C4 / #387 C1 service extraction split it +into `resolve_rule_backtest` + `backtest_resolved` + `run_rule_backtest` (`keel/commands/rules.py`), +with the compute core in `keel.strategy.backtest`. The survey found ten further decision-bearing +paths. In every case that decides money or admission, the **policy** — the comparison that would +move funds — lives in a compute module; what lives in `commands/` is the orchestration that +sequences the gates and the refusals that explain them: + +| path | location | what it orchestrates | where the policy lives | +|------|----------|----------------------|------------------------| +| `attempt_promotion` | `rules.py:749` (~205 lines) | THE promotion gauntlet: row refusal, `--force` bypass, lookahead veto (fail-closed), fee resolution, pooled-sibling loop, PBO load, both readings, transition | `strategy.promotion` (`can_promote`, `transition`, `next_status`), `research.bias`, `research.cscv` | +| `run_rule_backtest` / `resolve_rule_backtest` / `backtest_resolved` | `rules.py:379/338/370` | the #335 pricing path; also `trials.py:284–367`'s core | `strategy.backtest` | +| `run_rule_lookahead` (+ `_lookahead_views`, `_lookahead_warmup`, `_atr_indicator`) | `rules.py:604/460/524` | the lookahead gate harness, wired into the gauntlet by #440 C1a | `research.bias.lookahead_analysis` | +| `add_rule_row` / `seed_rules_into` | `rules.py:1634/1108` | rails 18/19 validation before any write | `commands._products`, `compliance.screen` grammar | +| `run_simulation` | `simulate.py:355` | the GO-LIVE/TRAIN-MORE pass; tier sweeps; the trials-ledger row | `sim.report.build_verdict`, `sim.portfolio_sim`, `strategy.promotion.floor_for_class` | +| `screen_product` | `assets.py:175` | THE admission routing point every candidate source must pass ("none of them can drift onto a laxer path") | `compliance.screen.screen_asset`; the live path enforces independently via `execution.guards` rail 1 | +| `admissibility_findings` | `doctor.py:363` | re-runs the executor's own sizing (`execution.sizing.size`/`spend`) across a 1.5–2× ATR band vs `caps.max_per_order_usd` | the sizing formula is the engine's, imported; only the diagnostic banding convention is local | +| `allowance_findings` / `veto_findings` / `doctor_exit_code` | `doctor.py:233/268/638` | diagnostic severity policy: rail-14 headroom, veto clustering, exit status | reads `guards._monthly_buy_spend_usd` and executor events — the rail's own math, never restated | +| `_rail11_status` | `status.py:164` | display-side rail-11 comparison | `execution.guards` rail 11; the `>=` is mirrored, the `None`→"unknown" divergence is documented as deliberate (a display must not fail-safe silently) | +| `promote_rule` | `setup.py:982` | the web's gated promotion entry (`force` hard-wired `False` — the O3 contract) | delegates to `attempt_promotion` | +| `plan_update` / `select_production_wheels` / `is_newer_version` | `update.py:424/387/234` | self-update decisions (operational, not trading) | local by nature — it installs the app, nothing else may | + +The two deliberately-local comparisons (`_rail11_status`'s mirror and doctor's banding) are display +decisions, each with an inline argument for why it is not policy. No path was found where the +comparison that admits, promotes, sizes or refuses an order is computed in `commands/` rather than +delegated. + +### The duplication audit (checklist item 3) + +The issue's fear — four front-ends each rendering the same report — was real once: the TUI carried +its own renderings, and #554 deleted 11,103 lines of them. What remains, traced report by report +over today's three front-ends (CLI, web, MCP): + +| report | shared core (one copy) | CLI projection | web/MCP projection | near-duplicate render paths | +|--------|------------------------|----------------|--------------------|------------------------------| +| status | `status.gather_status` (42 lines) | `render_human` (77) | `web/payload.py::status_payload` (42) — imports even `_human_age`/`_human_remaining` from `status.py` so the two can never disagree | **0** | +| doctor | `doctor.gather_findings` (97) | `doctor_lines` + `render_json` (15+15) | `mcp/tools.py::_doctor` (~25) — serialises the same `Finding` rows | **0** | +| purification | `compliance.purification.build_report` (engine) | `render_purification_report` (33) | `mcp/tools.py::_purification` (~12) — JSON of the same report | **0** | +| insights/journal/equity | `insights.build_*` (205 across three builders) | `render_summary`/`render_journal` (55+46) | `web/payload.py` calls the same builders | **0** | +| admission/promotion | `assets.screen_product`, `rules.attempt_promotion` | CLI-only today | web `/api/rules` reads rule *rows* (a table read), not the report | N/A — one front-end | +| backtest summary | `rules` services + `trials` | CLI-only today | — | N/A — one front-end | + +Every load-bearing report is one shared core plus thin, format-specific projections (12–77 lines +each — text for terminals, JSON for pipes and browsers). There is nothing left to unify: the seam +the issue's option (b) would create already exists and is load-bearing in both directions — the web +imports the cores, and `attempt_promotion`'s second caller is the web's own setup action. + +### Test parity (checklist item 3b) + +`tests/commands/` is 11,031 lines and 527 test functions — more than any engine area's own suite +(execution 7,851; strategy 5,957; data 4,804; sim 4,314; research 4,180), plus `tests/web/` 6,046 +and `tests/mcp/` 866 over the same cores. Two suites are architectural pins, not behaviour tests: +`test_service_parity.py` (682 lines) drives identical fixtures through the CLI command path and a +direct service call and asserts byte-equality of output and state; `test_console_thinness.py` +(764 lines) is an AST scan enforcing five rules over the presentation layer — no compute-module +imports or calls, `Decimal` arithmetic is display-only, no broker construction outside the seams. + +## Options + +**A — Move the decision orchestration to the engine.** Relocate `rules.py`'s service section (651 +lines: `attempt_promotion` and siblings), `simulate.run_simulation`, and doctor's sizing band to +the compute trees, leaving `commands/` as pure rendering. + +**B — Unify the remaining front-end rendering** behind a shared formatter layer. + +**C — The shape stays**, recorded with numbers, with the one mechanical gap the survey found +closed by a pin rather than a move. + +## Decision + +**C. Nothing moves.** The layer is the explanation engine — 40.5% shared report assembly, 36.7% +gate orchestration whose policy is already engine-side, 13.9% glue, 5.3% CLI-only rendering — and +its size is dominated by prose and refusal wording that has no other home. Specifically: + +1. **A is refused on the evidence.** Every money-deciding comparison already lives in a compute + module (table above); what would move is sequencing and explanation, and moving it would put + operator-facing echo sinks inside the engine — inverting the very separation that keeps the + compute modules small (the stated benefit the issue's "case that this is fine" defends). The + extraction A would undo was done *for* multiple front-ends (#390 C4), and its non-CLI callers + still exist: `setup.promote_rule` (web) and `trials.py` both dispatch to `rules` services. The + engine-invariants argument also cuts against A: the gate sequence is pinned today by + byte-equality parity tests against the CLI a human actually runs; engine-side, those would + become engine-style invariant tests of a sequence no engine path executes. +2. **B is moot.** Zero near-duplicate render paths remain among the load-bearing reports. The + cores are shared, the projections are thin by design, and the thinness is *enforced* — for + `keel/web/`. The one gap: the AST pin's glob covers `keel/web/*.py` only; `keel/mcp/*.py` + (8 handlers, 664 lines, all thin today) is unscanned, so nothing mechanical prevents an MCP + handler from growing compute later. That is filed as #588 — a pin extension, not a refactor. +3. **The ratio is now a feature to hold, not a question to re-ask.** If `commands/` again + approaches engine size, the first suspect is a new front-end re-deriving reports — exactly what + happened with the TUI — and the answer is the one #554 gave: delete the front-end, keep the + cores. + +**Standing rules this decision fixes:** + +1. A compute module may grow operator-facing *explanations* only as data (reasons, verdicts, + notes); the wording and ordering of what a human reads stays in `commands/`. +2. Any new front-end (desktop, MCP tool, remote API) consumes `keel.commands.*` services or the + engine directly — it does not re-render another front-end's report from raw rows. +3. A decision-bearing path added to `commands/` must delegate its deciding comparison to a compute + module and cite this record (or argue an exception in the PRD that supersedes it). + +## Consequences + +- **The question stops being re-asked.** "Is `commands/` too big?" now has a dated answer with a + method: 13,552 lines, 0.65× the engine, no duplicated render paths, no engine-side decisions + stranded in the CLI. Re-open only on a trigger below, by writing a superseding record. +- **The MCP thinness gap is the one actionable item**, filed as #588: extend + `test_console_thinness.py`'s scan to `keel/mcp/*.py`. Today it would pass vacuously-green over + eight thin handlers — which is the point of adding it now, before anything grows there. +- **`doctor.py`'s sizing band stays local** (`admissibility_findings`): it imports the executor's + own `sizing.size`/`sizing.spend`, so a formula change cannot drift; only the 1.5–2× ATR banding + convention is local, and it is a diagnostic convention, not policy. +- **Costs accepted:** the layer's size still slows grep-driven navigation versus the engine; the + prose density (31%) is the price of refusals that explain themselves; and role classification is + judged at module grain — a future module mixing roles should split before it blurs this table. +- **Triggers that would reopen this:** (a) `commands/` again exceeds the engine total; (b) a + second near-duplicate render path appears for any load-bearing report; (c) an MCP handler or web + module grows a deciding comparison of its own; (d) a fourth front-end is proposed. + +### Method (reproducible) + +Line counts: `wc -l keel/commands/*.py` and per-directory `find … | xargs wc -l`, on `main` at +16725c1. Role classification: an AST pass per module collecting `keel.*` imports (engine deps vs +web/MCP deps), click decorators, and `build_*`/`render_*` name shapes; cross-checked against who +imports each module back (`grep -n "from keel.commands" keel/web/*.py keel/mcp/*.py`). Decision +paths: function inventory of the ten largest modules, then reading every candidate that calls a +guard, an executor seam, or compares a money value. Duplication: for each load-bearing report, +locate the core (the function whose output both front-ends consume) and count render functions over +it. Line-level attribution: AST `end_lineno` spans bucketed by function-name class: + +```python +# render / assembly / glue attribution, whole layer (function bodies only) +RENDER = r"^(render_|_human|_money|_fmt|_short_num|_clock|_stamp|_age|_describe_|_emit)" +ASSEMBLY = r"^(build_|gather_|screen_|summarise_|group_|apply_|resolve_|run_|attempt_|plan_|select_|parse_|inspect$|.*_findings)" +# click decorators on a def -> click-command body; spans sum vs wc -l -> headers/docstrings/imports +```