chore(fidelity): close Phase 9's loose ends — alias, stale docstrings, telemetry overflow guard - #336
Merged
Merged
Conversation
… to the engine floor, refresh stale #259-era docstrings, guard the warning's arithmetic From the independent phase review: _SIM_SLIPPAGE_PCT is now an alias of SLIPPAGE_FLOOR_PCT (the report's flat-cost claim becomes structurally true, pinned by test, instead of true by numeric coincidence); the executor's 50bp-threshold rationale cites the floor constant rather than the CLI literal and names the coincidence with #259's 50bp cap; the intent-divergence docstring no longer claims the liquidity model does not exist; the entry-override arithmetic moved inside a try (an extreme-but-finite exponent raises ArithmeticError on division, and telemetry must never fail a routing). Promotion-gate opt-in now tracked in #335.
This was referenced Aug 17, 2026
Re-measure under the honest cost model: simulate re-run, DCA dip ablation, paper DCA re-enabled
#339
Closed
Merged
eaitbrahim
added a commit
that referenced
this pull request
Aug 18, 2026
* feat(executor): routing-time max-spread gate for live BUY entries Fixes #350. WHAT. A live BUY whose previewed book shows `(best_ask - best_bid) / mid` at or beyond `execution.max_entry_spread_pct` (new config section, default 0.005 = 50bp) is REFUSED after the preview and before the confirm gate and placement -- so a thin book cannot be entered at a moment its spread alone makes the fill economics materially worse than the cost model assumes. The refusal is recorded in `ExecutionResult.vetoed_by` (the tokens `max_entry_spread` / `book_unreadable`, the same one-legible-token shape rail violations use) and logged at WARNING as a structured event carrying the measured spread, the threshold and the product. WHY. The issue is the rail-work agreement CONTRIBUTING requires; the operator proposed it in the Phase 10 expansion review. It is the live-path half of the spread guardrail whose sizing half (#358) caps every Tier-2 addition at a 2% target weight. Design decisions: - Post-preview placement, deliberately: `guards.check` is broker-less by design, and the book exists only in the `broker.preview_order` result -- so this is a routing-time gate BESIDE the eighteen rails, not a numbered `guards.check` rail. It consumes the SAME preview #332's `_warn_if_market_routing_overrides_entry` reads: one helper (`_preview_book`, bid/ask/mid-safe: missing keys, NaN, non-finite, non-positive all read as unreadable), two consumers. The warning keeps its exact behavior and position (its tests pass unchanged); the gate runs after it, before confirm/place. - BUY-only: exits, brackets, stop rolls and scale-outs are never gated -- the same principle that makes rail 17 halt entries, not exits. - Paper mode never runs the gate: `_paper_enter` fills synthetically without a preview, so the paper-hourly profile accrues NO evidence about it -- a reason this ships before any live resumption, not after paper expansion. - Fail-closed on an unreadable book: a live BUY whose preview carries no readable bid AND ask is refused with the distinct `book_unreadable` reason and logged loudly. "Cannot know" is a different fact from "too wide"; the real venue's preview carries both sides (`cb_client.preview_order` maps best_bid/best_ask to Decimal), so an unreadable book means a degraded response -- the moment not to spend. The spread arithmetic also refuses (rather than swallows, as #336 taught the warning to do) on extreme-exponent Overflow. - 50bp default anchored to #334's `SLIPPAGE_CAP_PCT`: the backtest never assumes more than 50bp per-leg slippage on even the thinnest book, so a spread AT the cap has consumed the model's entire worst-case cost and the taker fee rides outside it -- hence the boundary is >= (fail-closed), unlike #332's strictly-greater visibility threshold. Validated on load to (0, 0.10], ConfigError naming `execution.max_entry_spread_pct`. Test-side consequence, honestly reported: the shared test fakes' bookless default previews modelled a shape the real venue does not return, and under a fail-closed gate every such "normal successful BUY" test would refuse. The fakes (test_executor, test_agent, test_cli, test_reconcile) now carry both book sides, and the one #332 test that borrowed the default preview as its degraded/bookless shape constructs that shape explicitly -- meaning unchanged, sourcing changed. Golden config fixtures regenerated via the documented script; the defaults golden now pins the 0.005. * test(executor): pin the spread gate's fail-closed arm and gate-before-confirm ordering Two review findings on #350's money-path arms, both test-only, plus one docstring enumeration: - The extreme-exponent fail-closed arm of `_entry_spread_gate` (the `except ArithmeticError` whose spread is uncomputable) had no test. Pinned both ways a readable-sides book can still break the arithmetic: an `Overflow` on `bid + ask` at `1E+999999999` magnitudes, and a `DivisionByZero` from a subnormal pair whose mid half-even rounds to zero while the difference survives nonzero. #332's warning swallows these (telemetry); the gate must refuse with `book_unreadable` -- each case verified to fail under a swallow-like-telemetry mutation before landing. - Every gate test ran autonomous mode, so nothing pinned that the gate sits BEFORE the confirm gate. A wide-book BUY in `mode="confirm"` with an approving confirm_fn is still refused and the approver is never consulted -- a human cannot approve around the gate. Verified to fail under a confirm-first reordering mutation. - `summarise_cycle`'s docstring enumerated two non-placement causes (rails, confirm gate); added the third honestly -- the routing-time entry-spread gate refusing a live BUY. Docstring only, no behavior change.
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.
What & why
Closes Phase 9's loose ends, from the independent whole-phase review (all findings non-blocking; this PR is bookkeeping and robustness, not behavior):
cli._SIM_SLIPPAGE_PCTis now an alias ofbacktest.SLIPPAGE_FLOOR_PCT, not a repeated0.0005literal. The simulate report asserts its flat-priced dollar sections cost "the flat SLIPPAGE_FLOOR_PCT per leg" — that claim was true by numeric coincidence; a retuned floor would have made the report's own cost statement silently false. Now structurally true, pinned by a test at the site that renders the claim. (portfolio_simandpaperstill carry their own literals — folding them is bundled into Opt the promotion gate into per-product slippage — prerequisite-gated on a thin-asset candidate or a load-bearing restatement #335, the promotion-gate opt-in, so all remaining flat-rate sites move together.)_log_intent_divergenceno longer claims the per-asset liquidity model "does not exist yet (slippage is one global 5bp constant applied from BTC to TON #259)" — it exists on the research side; the live path just has no statistic at fill time.ENTRY_OVERRIDE_WARN_BP's rationale now citesSLIPPAGE_FLOOR_PCT(not the CLI literal) and states honestly that "10x the slippage assumption" holds at the liquid end and narrows toward slippage is one global 5bp constant applied from BTC to TON #259's 50bp cap — plus an explicit note that the threshold's 50bp and slippage is one global 5bp constant applied from BTC to TON #259's cap 50bp are different, uncoupled constants.is_finite()admits extreme exponents (a rule bug like1E+999999999parses, is finite, compares fine), and the division raisesArithmeticError— which telemetry must swallow, matching the siblingintent_divergence's inside-the-try arithmetic. Regression-tested.Tests-first evidence
The overflow guard was written failing-first conceptually and verified directly (
Decimal('1E+999999999')against a quoted preview raises through the old code, returns silently on the new); the regression case is now intest_a_preview_without_a_book_quote_is_silent_not_fatal. The alias pin lives intest_simulate_reports_per_product_slippage_beside_the_results, where the claim it guards is rendered.Gates
uv run ruff check keel tests packages— All checks passed!uv run mypy— Success: no issues found in 237 source filesuv run pytest -q— 2887 passed, 1 skippedScope check