Skip to content

feat: native exit bracket, order reconciliation, and a per-tranche position ledger - #96

Merged
eaitbrahim merged 11 commits into
mainfrom
feat/native-bracket-and-reconciliation
Jul 20, 2026
Merged

feat: native exit bracket, order reconciliation, and a per-tranche position ledger#96
eaitbrahim merged 11 commits into
mainfrom
feat/native-bracket-and-reconciliation

Conversation

@eaitbrahim

@eaitbrahim eaitbrahim commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

Executes docs/superpowers/plans/2026-07-19-keel-native-bracket-and-reconciliation.md end to end (Part B, Tasks 1–5). Part A had already landed on this branch and is documented in the plan as design decisions to review against.

Goal: make keel's exit path trustworthy — the exchange owns the stop-vs-target race, every fill is observed and recorded, and every position is addressable per tranche rather than per product.

What's here

  • Task 1 — persist the resting bracket's order id, so roll_to_break_even/trail_stop_atr stop being unreachable by construction.
  • Task 2 — re-place a dead exit bracket instead of leaving the position naked; CRITICAL is now the fallback when replacement is impossible, not the resting state.
  • Task 3 — a per-tranche positions table (schema v3→v4) replaces position_rule:<product> as the carrier of entry context. Kills the last-write-wins attribution bug where an older tranche's bracket computed P&L against a newer tranche's entry price.
  • Task 4 — get_order/cancel_order on the Broker port and both adapters, so broker-port Phase B can delete cb_client.py without breaking reconciliation.
  • Task 5 — a sim-side equity/drawdown producer, making rail 11 sweepable in keel simulate.

Judgement calls worth a reviewer's attention

  1. Task 4's conformance tests round-trip the order id through place_order rather than hardcoding the plan's "cb-1"/"already-filled". The suite also runs against FakeAdapter, which has never heard of those ids — Coinbase-specific magic ids don't belong in a venue-agnostic contract.
  2. Task 5 arms BOTH legs of rail 11 (max_total_dd_pct and max_weekly_dd_pct), not just the total leg the plan named. A producer that left the weekly leg dormant in the sim would recreate the exact defect class the task exists to close.

Sweep response curve for max_total_dd_pct (trade count), evidence the rail is genuinely tunable:
0 → 0 | 0.01 → 7 | 0.02 → 15 | 0.03 → 22 | 0.04 → 30 | 0.05 → 38 | 0.06 → 45 | 0.08 → 61 | 0.10 → 73 | 0.20 → 73 | 0.90 → 73

Verification

  • .venv/bin/python -m pytest -q → 1015 passed
  • .venv/bin/ruff check . → clean
  • .venv/bin/mypy . → no issues, 148 source files
  • tests/fixtures/baseline_backtest.json byte-identical
  • Mutation checks per the plan: unhooking each new producer fails exactly its wiring test and nothing else.

🤖 Generated with Claude Code

eaitbrahim and others added 11 commits July 19, 2026 19:20
@eaitbrahim @claude
fix: a cancel that cannot reach the exchange must raise, not rewrite …
0d418ff 
…state

Both cancel sites did `cancel = getattr(broker, "cancel_order", None)` and
skipped silently when it was absent -- and the real broker has no
`cancel_order` at all. `cb_client`, the Coinbase adapter and the `Transport`
protocol all lack it; only the three test fakes define one. So in production
the cancel was always skipped while the order row was still marked `canceled`,
leaving a live resting SELL on the exchange that our own records said was
gone. After a stop filled, the target leg could still sell inventory we no
longer held.

`roll_to_break_even` is the worse of the two: it places the replacement stop
BEFORE cancelling the old one, so a skipped cancel leaves two live stops on a
single position and whichever fires second sells inventory the first already
sold.

The tests passed precisely because the fakes supply a method the real client
lacks -- the same reads-as-enforced-but-isn't pattern this branch exists to
kill, sitting on the cancel path.

Both sites now route through `_cancel_at_exchange`, which raises
`CancelUnavailable` on every failure mode (no method on the broker, no
broker-side id to name, or the call itself raising) and never marks local
state. The exchange is the source of truth, so the cancel must succeed there
before anything records it; our state must never claim a cancel that did not
happen.

No blast radius today: neither `handle_oco_fill` nor `roll_to_break_even` has
a production caller. This lands before that wiring, not after.

Mutation-checked: restoring the silent skip fails exactly the three new tests.

946 passing, ruff and mypy clean, backtest baseline byte-identical.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@eaitbrahim @claude
feat: use Coinbase's native trigger bracket instead of client-side OCO
da7e0ee 
`place_oco_bracket` placed two independent SELL legs and paired them
client-side in `agent_state`, so closing a position correctly depended on US
observing a fill and cancelling the survivor. A missed fill left a live order
able to sell an already-closed position. It also sized BOTH legs at the full
qty, committing a 1x position 2x -- on spot the second leg should simply have
been rejected for insufficient funds.

`place_bracket` places ONE native `trigger_bracket_gtc` order carrying the
take-profit as `limit_price` and the stop as `stop_trigger_price`. The
exchange owns the race between them, so the sibling-cancellation failure mode
does not exist rather than being handled carefully. `handle_oco_fill` and the
`oco_sibling:` state are deleted.

No new broker API surface was needed: the SDK's `trigger_bracket_order_gtc` is
a thin wrapper over `create_order` with that order_configuration, so it
reaches the exchange through the `place_order` path already in use.

One accepted regression. `_roll_stop` must now cancel before placing, because
the resting bracket commits the whole position and a replacement would be
rejected for insufficient funds. That opens a brief window with no protective
stop, which the old place-then-cancel ordering did not have. `edit_order`
cannot avoid it: limit-GTC only, and it edits size/price, never
`stop_trigger_price`. A rejected replacement now logs CRITICAL
`executor.position_unprotected` and returns None; any production caller of
`roll_to_break_even`/`trail_stop_atr` must handle it.

Three tests were deleted deliberately, each with a note in place: the two
`handle_oco_fill` sibling tests, whose invariant is now the exchange's, and
the two-live-stops test, whose hazard is unreachable under cancel-first. Five
added.

Mutation-checked: dropping the target price, silencing the CRITICAL, and
reverting to place-then-cancel each fail a test.

948 passing, ruff and mypy clean, backtest baseline byte-identical.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@eaitbrahim @claude
feat: reconcile resting orders against the exchange each cycle
a20c0d4 
Nothing ever re-read order status, so a bracket that filled at the exchange
was invisible. A stop-out -- the dominant source of losses -- closed a
position the agent never noticed: the row stayed `pending`, `_held_position`
kept counting sold inventory as held, `position_rule` was never cleared, and
no `trade_outcomes` row was written. Rail 16 therefore counted only voluntary
rule exits, systematically under-counting exactly the losing side it exists to
react to, and rail 11's equity was computed against positions that no longer
existed.

`reconcile_open_orders` runs at the top of every cycle, before equity and
before any entry, since a fill that already happened has changed both the
position and the cash balance.

It also upgrades two numbers from modelled to observed, retiring compromises
made earlier on this branch: `actual_fill` was the EXPECTED price and is now
`average_filled_price`; `fee` was the PREVIEWED commission and is now
`total_fees`. Realized P&L is now what the exchange actually charged.

Adds `get_order` and `cancel_order` to `CoinbaseClient` and its `Transport`.
`cancel_order` matters beyond reconciliation: the real client had no such
method at all, so `_cancel_at_exchange` would have raised `CancelUnavailable`
on every production cancel. Coinbase's batch_cancel reports success per order,
so a 200 does not mean the order is gone -- it returns True only on a
confirmed per-order success and treats an empty result set as failure, because
absence of a refusal is not a confirmation.

Deliberate semantics: a partial fill is left resting rather than recorded, as
it has not closed the position; a cancelled or expired bracket closes the row
without a trade outcome, since nothing sold; a broker error on one order never
abandons the rest; and an exit with no position context is skipped rather than
guessed, matching `record_closed_trade`.

Mutation-checked: unwiring it from the cycle, treating partials as full exits,
using the expected price, un-isolating the per-order error, and making
cancel_order trust the HTTP response each fail a test.

961 passing, ruff and mypy clean, backtest baseline byte-identical.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@eaitbrahim @claude
fix: critical and important findings from the second review pass
5c14395 
Three opus reviewers over the rewritten branch. Two independently found the
same functional break, which my own native-bracket rewrite introduced.

Rule-driven exits could never fill. `place_bracket` leaves a bracket
committing the entire base position, and `_handle_exits` then issued a
full-size market SELL for that same inventory -- on spot the base is locked,
so the sell is rejected, `position_rule` is never cleared, no outcome is
recorded, and the agent retries the same doomed sell every cycle while the
position rides a stale stop. The rewrite was validated against the path it
changed and not against the path it left behind. `execute` now clears any
resting bracket before a SELL, so every exit path gets it by construction, and
refuses the exit outright if the bracket cannot be cancelled.

`_cancel_at_exchange` discarded `cancel_order`'s return value, so a refused
cancel was recorded as a successful one -- the exact lie the module exists to
prevent. No test could catch it because every fake returned None.

A partial-then-cancelled bracket silently discarded the filled portion,
leaving `_held_position` reporting the full position held while the realized
loss never reached rails 11 or 16.

Two of my own ledger claims were false, and the reviewers refuted both. The
cancel-first ordering was unheld: my test asserted a cancel HAPPENED, not that
it happened FIRST, and place-then-cancel survived. The entry-fee wiring was
unheld: every downstream test hand-seeded `entry_fee` into a fixture, so
mutating the producer to stop writing it left the suite green -- silently
reverting pnl_net to net-of-exit-fee-only.

Also: stop/target state no longer leaks past a rule exit; `place_bracket` is
the single writer of that pair; market orders now upgrade to observed
economics so rail 16 stops counting two different definitions of pnl_net; a
dead bracket on a held position escalates CRITICAL instead of INFO;
`_record_fill` moved inside the per-order isolation; a zero-price fill no
longer fabricates a phantom loss; four previously unheld guards are pinned; a
tripwire test fails the moment `scale_out` is wired; and five docstrings that
had become false were corrected.

Independently verified rather than trusted: the bracket price mapping is
correct against the installed SDK, and no test coverage was silently lost
across the branch (656 -> 748 test defs, diffed per commit).

980 passing, ruff and mypy clean, backtest baseline byte-identical.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@eaitbrahim @claude
plan: native exit bracket and order reconciliation
dbeffd1 
Seam B was built out of a review pass rather than a plan, which is what let a
break in the rule-exit path go unnoticed. This document is the correction.

Part A records what already landed as design decisions with their rationale
and accepted trade-offs, to be reviewed against rather than executed --
retroactive TDD steps for passing code would be theatre. Part B is the
genuinely unbuilt work in normal task form: persist the bracket order id,
re-bracket a dead bracket, a per-tranche position ledger (the live blocker),
get_order/cancel_order on the broker port, and a rail-11 producer for the
simulator.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@eaitbrahim @claude
feat: persist the resting bracket's order id
b294f0e 
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@eaitbrahim @claude
feat: re-place a dead exit bracket instead of leaving the position naked
789e36b 
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@eaitbrahim @claude
feat: per-tranche position ledger replaces position_rule for entry co…
1529c84 
…ntext

Adds a v4 `positions` table, one row per tranche, carrying the entry context
(`entry_fill`, `qty`, `entry_fee`, `rule_name`, `opened_at`) that
`agent_state["position_rule:<product>"]` used to hold as a single per-product
blob. That blob was last-write-wins, so averaging up overwrote the first
tranche's entry and an older tranche's bracket booked its P&L against a price it
never paid.

`positions.bracket_order_id` is the one linkage direction, with a FK to
`orders(id)`. `ExecutionResult` now surfaces `bracket_order_id` so
`run_once` can point a tranche at its bracket -- `execute` places the bracket
internally, so its return was the only way to learn the id. The per-product
`agent_state["bracket_order:<product>"]` key added in b294f0e is dropped as
superseded: it was write-only and duplicated this fact at the wrong granularity.

Both exit paths are rewired, not just reconcile: `_handle_exits` closes every
open tranche FIFO and records one outcome each, apportioning the exit order's
single fee pro-rata by qty. Wiring only the reconcile path would have silently
stopped voluntary exits from producing outcomes at all, since
`record_closed_trade` skips a position with no `entry_fill`.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@eaitbrahim @claude
feat: get_order/cancel_order on the broker port and Coinbase adapter
c466a73 
`execution.reconcile` duck-typed both against `keel/data/cb_client.py`, a module
broker-port Phase B deletes -- so reconciliation would have broken the moment
Phase B landed. Adds `OrderStatus` to the port's result types and both methods
to the `Broker` protocol, implemented on the Coinbase and Fake adapters.

The conformance tests round-trip the order id through `place_order` rather than
hardcoding one: the suite runs against both adapters, and a Coinbase-shaped
magic id would test a fixture instead of the contract.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@eaitbrahim @claude
feat: sim-side equity/drawdown producer so rail 11 is sweepable
9348a29 
Third instance of the dormant-rail pattern: `keel simulate` could not trip
rail 11, so its thresholds could not be swept. Nothing in the sim ever moved
`drawdown_total_pct`/`drawdown_weekly_pct` off their 0 defaults, so
`0 >= max_total_dd_pct` was never true whatever the sweep varied it to.

`SimAccount.update_equity` is the missing producer -- the sim-side twin of
`execution.equity.update_drawdown`, mirrored arithmetic step for step, called
once per bar by `portfolio_sim` before signal evaluation. `can_open` gains the
rail-11 check beside rail 16 (entries only, DCA exempt, `>=`, matching
`guards.check`).

Both legs are armed, not just the total one the plan named: a producer that
left `max_weekly_dd_pct` dormant in the sim would recreate the exact defect
class this task exists to close.

`deposit` rebases the HWM and equity history itself, so a monthly contribution
cannot ratchet the monotonic HWM into a phantom drawdown. Folding it into the
single method that moves cash makes it a writer that cannot be forgotten --
unlike live, where the operator must remember `keel record-flow`.

Sweep response curve for max_total_dd_pct (trade count):
0 -> 0 | 0.01 -> 7 | 0.02 -> 15 | 0.03 -> 22 | 0.04 -> 30 | 0.05 -> 38
0.06 -> 45 | 0.08 -> 61 | 0.10 -> 73 | 0.15 -> 73 | 0.20 -> 73 | 0.90 -> 73

Monotonic, saturating at the unhalted count of 73 once the threshold clears the
scenario's ~9.6% actual drawdown -- the rail is genuinely tunable.

Mutation check: commenting out the per-bar `update_equity` call fails exactly
the two sweep tests and nothing else.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@eaitbrahim @claude
docs: mark the native-bracket and reconciliation plan complete
6ca3104 
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@eaitbrahim
eaitbrahim merged commit d2aaece into main Jul 20, 2026
@eaitbrahim
eaitbrahim deleted the feat/native-bracket-and-reconciliation branch July 20, 2026 03:59
eaitbrahim added a commit that referenced this pull request Jul 20, 2026
@eaitbrahim @claude
docs(kb): sources 66, 67, 70 — shariah trading fiqh, Islamic finance …
4b6385b 
…handbook, Fib/pivots

The compliance pair (66, 67) is the most consequential material since §28, and
the two reconcile against each other:

- §66 CORRECTS §56.1, which was stated too bluntly. "T+2 implies riba" is wrong
  as written: a T+2 window is compatible with spot provided real delivery
  concludes. The actual defect is perpetual rollover so delivery never happens.
  Delivery-concluding lag != non-delivery-by-design.
- §66.2 grounds "a BUY tranche must be confirmed-settled before it is
  exit-eligible" in fiqh (the hadith forbidding resale before possession),
  not merely operational hygiene. PR #96's per-tranche ledger and reconciliation
  already implement it; the rationale is now recorded.
- §66 logged custodial/digital qabd as an OPEN GAP. §67.1 CLOSES it: OIC Fiqh
  Academy Res. 53/4-6 holds electronic constructive possession via a custodial
  account satisfies qabd. Exchange custody is now sourced, not an interpretive
  stance. Cross-referenced both rows so the README does not contradict itself.
- §67.2 extends the sarf/spot grounding from currency to GOLD, closing a
  PAXG-specific citation gap §30.1 never covered.
- §66.1 triangulates bay' al-sarf across two more sources: the spot-settlement
  mandate now rests on four independent sources.
- §66.7 NEGATIVE FINDING: no compliance source in this KB addresses crypto
  directly. Silence is not permission. Next compliance source should be a
  targeted AAOIFI/OIC digital-asset resolution.

Also: Deve.pdf is a DUPLICATE of Source 23, confirmed by text diff rather than
title match. Its 2026 stamp is the generator re-dating on download.

Source 70: the Fibonacci booklet is promotional, not the debunking its title
suggested. Its own admissions ("which level holds is unknowable"; anchor choice
"becomes a guessing game") mildly reinforce the deferred-to-v2 judgement without
settling it. Floor-trader pivots logged as an untested candidate whose
formula-vs-S/R status §58.6/§58.9 genuinely does not resolve.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
eaitbrahim added a commit that referenced this pull request Jul 20, 2026
@eaitbrahim @claude
docs(kb): sources 75-77 - Warrior Trading sweep; bound the rank-and-f…
d5c599e 
…ill under-deployment claim

Sources 75 (Cameron scanner/TA lead-magnets) and 76 (Chart Patterns v2) extracted;
77 (candlestick infographic + filled-in trading-plan worksheet) logged as an
audit-trail-only exclusion, no file, per the source-53 precedent.

Headline is a NEGATIVE structural result. Cameron's scanner is a BREADTH engine:
its trade count comes from |universe| ~5,000, not from ranking. So SS60.2's
'allowlist-size-independent' claim does not survive -- ranking only bites when
qualifying candidates outnumber slots, and at |allowlist| = 3 it never does.
Under-deployment is a signal-PRODUCTION problem; ranking cannot produce signals.

Orchestrator correction applied on top of the extraction: the demotion covers the
RANKING half only. SS60.2's CONCURRENT-SLOT half is not a no-op, it is untestable --
PR #96 lifted the one-tranche-per-product limit on the live path while
keel/sim/portfolio_sim.py:600 still enforces one RULE position per asset. That
reframes the S1+S2 ensemble rejection as an artifact of line 600 rather than a
finding about the ensemble.

Module-map rows for money_mgmt/backtest and the SS60 log row all bounded to match.

Refs: docs/superpowers/references/trading-knowledge-base/

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@eaitbrahim eaitbrahim mentioned this pull request Jul 20, 2026
Merged
@eaitbrahim eaitbrahim added the feature New capability (groups under Features) label Jul 21, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

feature New capability (groups under Features)

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@eaitbrahim