Harden take-profit discipline - #419
Merged
Merged
Conversation
This was referenced Aug 6, 2026
DarriEy
force-pushed
the
codex/take-profit-discipline
branch
from
August 6, 2026 19:15
5bd99e7 to
9abd472
Compare
DarriEy
marked this pull request as ready for review
August 6, 2026 19:15
Until #420 the portfolio monitor's exit loop raised on every tick, so no live exit ever executed and this policy was tuned against a dead path. That path now runs, so every threshold here moves money on the next tick. Six amendments, in descending order of what they cost. 1. THE ENTRY-TIME FALLBACK, restored. The lifecycle target reads a position's age, and the new code sourced it only from a token-scoped reverse-inventory walk over `fills`. On a miss it set `entry_dt = now`, which pins `fraction_remaining` at exactly 1.0 on EVERY tick — not once, permanently — so the target freezes at the widest 75% band and can never tighten to 50% or reach the 25% near-expiry band at all. That miss is not rare. Measured against the live DB, 37% of open positions have no token-scoped fill ancestry: they predate the fills ledger, or they are live rows a venue mirror created without ever writing a fill (bot.py and kalshi.py both upsert cost_basis/portfolio directly). 40% of those DO have a `trades` row that the old lifecycle path used, and 65% of those in turn have a known end_date — so the near-expiry band was reachable for them before and is not now. Resolution order is now fills, then trades, then "just entered", and the last branch documents what it costs. The trades leg is book-scoped but NOT token-scoped, because `trades` has no token column (broker/ledger.py says so explicitly and reaches token scope only by joining back through fills.order_id — which is exactly what these positions lack). Coarser than the fills path, strictly more than nothing. The fills comparison also gains a size tolerance. `qty >= pos.size` is an exact float compare against a summed quantity: 0.7 + 0.1 is 0.7999999999999999, so a position whose fills plainly cover it reports no ancestry at all. `cost_basis` was considered as the source and rejected. #412 did make it the authoritative per-(market, is_paper, token) holdings table, and it agrees with portfolio.size for 97% of rows — but its only timestamp is `updated_at`, stamped on every write by PnLTracker, both venue mirrors and both settlement paths. It records when inventory last MOVED, not when it was entered, so it cannot answer this question at all. 2. THE ENTRY FEE, dropped from the gate. `binary_exit_economics` charged entry + exit fees. broker/pnl.py realizes `(fill.price - old_avg_cost) * size - fill.fee` — the exit leg alone — and builds the pnl_ledger event only on the SELL branch; the BUY branch stores `total_cost = price * size`, so avg_cost is fee-EXCLUSIVE and the entry fee never reaches the ledger anywhere. The gate was demanding a cost the books never record, holding every winner past its target for it. Measured drag removed across open binary positions: 1.47 points at the median, 4.00 at p90, 6.86 at the maximum. The entry leg stays on ExitEconomics as a diagnostic so calibration keeps the number without the decision paying for it. 3. THE HOLD TELEMETRY, off the money path. `_record_exit_decision` was awaited inline before `exits.append(...)`, taking the shared serialized write lock once per evaluated position — and the HOLD branch fires for every non-exiting position on every 60s tick. Observations now accumulate in a list and land in one `executemany` after the loop, by which time every exit is already in the returned list: one lock acquisition per cycle instead of one per position, a >99% reduction on a full book. Rows are pruned to `execution.exit_decision_retention_days` (default 14) by a bounded per-cycle delete on the indexed time column, following candidate_dispositions. HOLD rows are kept rather than dropped: they are the counterfactual, and a calibration that sees only exits cannot say what a different threshold would have done. The existing exception guard stays and is now tested — telemetry must never be able to trap a position. PR #411 moved a scan off the quoting loop for the same reason. 4. THE MIGRATION COLLISION CLASS, closed. Schema v50 is kept here (money path, small migration); #411 is a draft observatory and renumbers to v51. A duplicate `async def _migrate_vN_to_vM` in the class body is silently shadowed — Python keeps the last definition, ruff does not flag it, and one migration's DDL simply never runs while the version counter still advances. That has now happened twice, surviving review and CI both times. A new test AST-parses the class and asserts every step is declared once, dispatched exactly once, resolves to as many distinct function objects as there are steps, and forms an unbroken single-version chain up to SCHEMA_VERSION. 5. AN UNSCOPED READ NO LONGER DEFAULTS A LIVE POSITION TO PAPER. `mode = 1 if mode_flag is None else mode_flag` answered a live position's lifecycle question off the paper ledger. mode_flag is None when `settings.is_live` is not a bool and the tracker holds no settings of its own, leaving get_positions unscoped over BOTH books. A real `Settings.is_live` ANDs three bools and so always is one — with the kill switch armed it is False, which scopes the read to paper rather than unscoping it — so this is the duck-typed/no-settings path, not a live-trading one. It still must not hardcode a book: the wrong answer was silent and the caller had no way to notice. #420 gave Position an `is_paper` field populated from the row, so the position is now asked directly; only one that cannot answer falls back to an unscoped read, which is imprecise for everyone rather than wrong for live. The telemetry row resolves its book the same way. 6. ZERO NOW DISABLES THE TRAILING STOP. `Field(gt=0)` on trailing_stop_activation_pct and trailing_stop_giveback_fraction crashed startup on 0, stricter than the neighbouring stop_loss_pct / profit_target_pct, which carry no validators. Relaxing to ge=0 alone would have been worse than the crash: unguarded, activation 0 arms the tier against every non-negative peak and giveback 0 reduces the test to `peak > current`, so "disabled" would have sold every position on its first adverse tick. `trailing_stop_triggered` now returns False when either knob is 0, and both readings are tested. Stop-loss and trailing-stop band VALUES are unchanged and verified byte-identical to the originals. bot.py's exit loop and #421's exit-liveness readiness criterion are untouched. TESTS. Six new behaviours, each mutation-verified — the behaviour was broken, the test confirmed failing, then restored. 17 mutations run, 17 killed: a TRAILING_STOP emitted through the real check_exits (4 mutations: activation compare, giveback compare, peak ignored, wrong ExitReason); a profit-target case that clears GROSS but not NET at 0.61 (0.62 clears both once the entry fee is gone, and the suite's 0.63 could never discriminate); the telemetry batch raising while the exit still returns; an entry-time fixture whose NO and live fills are NEWER than the paper YES entry, so a lost token or book scope changes the answer (the old fixture's newest fill alone satisfied qty >= size and could not detect either); an IBKR pair whose round-trip commissions bracket take_profit_pct; and the migration-distinctness test above. Verification: 2304 passed, 5 skipped, 1 deselected; coverage 67.44% against the 65.0 floor; `ruff check .` clean. Assisted-by: Claude (Anthropic)
DarriEy
force-pushed
the
codex/take-profit-discipline
branch
from
August 6, 2026 19:25
9abd472 to
d469368
Compare
The per-cycle prune is `DELETE FROM exit_decisions WHERE observed_at <
datetime('now', ?)`, and the only index shipped for the table was
`(market_id, token, is_paper, observed_at)`. `observed_at` is its FOURTH
column and the predicate constrains none of the three before it, so
SQLite has no seekable prefix and cannot use that index at all.
EXPLAIN QUERY PLAN on the shipped schema:
SCAN exit_decisions
So every exit cycle full-scanned the table. That is the worst place to
put a scan: the delete runs inside the `portfolio.exit_decisions`
transaction span, holding the process-wide write lock, on the path that
closes positions — and the table it scans is one the same path appends
to on every tick, including a HOLD row per non-exiting position, so the
scan gets more expensive the longer the bot runs.
Add a dedicated single-column index on `observed_at`. The plan becomes:
SEARCH exit_decisions USING INDEX idx_exit_decisions_observed_at
(observed_at<?)
The composite index stays; it serves the calibration script's
per-position reads, which do constrain the leading columns.
The index goes in BOTH places the table is defined — `db/models.py`'s
TABLES script and the v49->v50 migration in `db/database.py` — so a
fresh database and a migrated one agree. `candidate_dispositions`, the
table this prune was modelled on, already carries exactly this index;
only the index was missed when the pattern was copied.
The regression test plans the SHIPPED statement rather than a copy of
it, which is why the SQL is now a named constant beside the insert. It
checks both schema paths, and it exercises the migration by calling the
step directly: `_init_schema` runs TABLES *before* dispatching
migrations, so a reconnect-based test would create the indexes from
TABLES and then run the migration against a schema that already
satisfies it — an index missing from the migration would pass. Verified
by mutation: removing the index from either file fails the
corresponding half.
Assisted-by: Claude (Anthropic)
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.
Summary
Calculates take-profit decisions from token-scoped current inventory and
fee-net executable economics; distinguishes trailing stops; makes the
lifecycle and trailing bands configurable; adds schema-v50 decision telemetry
plus a read-only chronological calibration report.
Rebased onto main after #420 and #421. Until #420 the portfolio monitor's
exit loop raised on every tick, so no live exit ever executed and this policy
was being tuned against a dead path. It runs now, so every threshold here
moves money on the next tick. Six amendments followed, in descending order of
what they cost:
token-scoped
fillswalk, and settingentry_dt = nowon a miss, pinsfraction_remainingat 1.0 on every tick — the target freezes in itswidest band and the near-expiry band becomes unreachable. 37% of open
positions have no fill ancestry (they predate the fills ledger, or are live
rows a venue mirror created without writing a fill); 40% of those have a
tradesrow the old path used. Resolution is now fills → trades → "justentered". The trades leg is book-scoped but not token-scoped, because
tradeshas no token column. A size tolerance was added too:qty >= sizeis an exact float compare against a summed quantity.
cost_basiswas evaluated and rejected as the source — its only timestampis
updated_at, which records when inventory last moved, not when it wasentered.
broker/pnl.pyrealizes the exit feeonly and builds the ledger event only on the SELL branch, so
avg_costisfee-exclusive and the entry fee is never booked. Charging it demanded a cost
the books never record. Drag removed: 1.47 points median, 4.00 p90, 6.86
max. The entry leg survives as a reported diagnostic.
exits.append(...), taking the shared serialized write lock once perevaluated position, every tick. It is now one
executemanyafter the loop(>99% fewer lock acquisitions on a full book), with
execution.exit_decision_retention_days(default 14) pruning via a boundedper-cycle delete. HOLD rows are kept as the calibration counterfactual.
v51. A duplicate
_migrate_vN_to_vMis silently shadowed by Python andruff does not catch it — that has now happened twice. A new test AST-parses
the class and asserts each step is declared once, dispatched once, resolves
to a distinct function object, and forms an unbroken chain to
SCHEMA_VERSION.mode_flagis None whensettings.is_liveis not a bool and the trackerholds no settings of its own, leaving the read unscoped over both books.
(A real
Settings.is_liveANDs three bools and so always is one — with thekill switch armed it is
False, which scopes the read to paper rather thanunscoping it — so this is the duck-typed path, not a live-trading one. It
still must not hardcode a book: the wrong answer was silent.) Give a Position the book it belongs to so live exits can run again #420 gave
Positionanis_paperfield, so the position is now asked directly.Field(gt=0)crashed startup on 0.Relaxing to
ge=0alone would have been worse: unguarded, zero arms themost aggressive possible trailing stop rather than turning it off.
Stop-loss and trailing-stop band values are unchanged (byte-identical to
the originals).
bot.py's exit loop and #421's readiness criterion areuntouched.
Verification
ruff check .cleanbehaviour was broken, the test confirmed failing, then restored
Migration
Upgrades schema v49 → v50, adding the
exit_decisionstelemetry table and aposition/time index.