[keitaj]

Summary

• Records the worst unrealized loss each position reaches while it is open, and appends mae_bps / mae_age to the [close-reason] log line.
• Motivation: unrealized_loss_close_bps cannot be calibrated from fill data. Fills show the realized outcome, not the price path during the hold, so there is no way to count the positions that breached a candidate threshold and then recovered. Those are precisely the positions an early close would turn from winners into losers, so they are the denominator for any threshold choice.
• Observation only — no effect on close decisions, and no additional API calls.

Changes

strategies/mm_position_closer.py

• _update_mae() keeps a per-coin running maximum of the adverse excursion plus the position age at which it occurred. Called from manage() before any close decision, so the reading reported with a close includes the final cycle.
• _record_close() appends mae_bps / mae_age and clears the state. Emitted for every close reason — maker, taker_age, external and unrealized_loss — because the recovered positions are the point of the measurement. A position that never moved against us logs 0.0 rather than being omitted, so the denominator is not biased.
• Sign convention matches the existing unrealized-loss early close: positive means the position moved against us.
• Independent of unrealized_loss_close_bps. The existing unrealized-bps arithmetic sits inside an if threshold > 0 branch, so when the feature is disabled nothing was measured — which is exactly the configuration that needs the data.

market_data.py

• New get_cached_market_data(): a cache-only read that returns None on a miss instead of falling through to a REST L2 fetch. get_market_data() does fall through, and this samples once per open position per cycle, so routing through the fetching accessor could add API load when WS is disabled.

strategies/mm_config.py, strategies/market_making_strategy.py

• CloseConfig.track_max_adverse_excursion (default true), wired through from_legacy_dict and into the closer. No CLI flag — strategy params go through MMConfig / JSON config per the existing convention.

tests/test_position_mae.py (new, 31 tests) covers: the running max surviving a full recovery; sign convention for shorts; 0.0 for never-adverse positions; mae_age placement; emission on each close reason; tracking with the early-close threshold at 0; config wiring; multi-coin isolation; state cleanup and re-registration; and edge cases (zero entry price, cache miss, zero mid).

Existing closer tests — manage() now reads market data unconditionally, so tests exercising it must state what the cache holds. The shared closer mocks default to a cache miss, which is a real production state the code handles by skipping the sample.

Two caveats, documented in code, README and tests

• The value is a lower bound. Sampling is bounded by the main-loop cadence, so an intra-cycle spike that retraces before the next cycle is not seen. This makes the data asymmetric evidence: it can show that a given threshold would have cut recoveries short, but it cannot establish the converse, because breach counts are under-counted. A test pins this behaviour so the number is not later mistaken for exact.
• Log compatibility. The fields append at the end of the line, so existing last_tier=(\S+) and max_age=(\d+)s parsers keep matching. A test asserts this against the prior regexes.

The "no extra API calls" requirement is asserted by comparing the fetching accessors' call counts with tracking on versus off, rather than by a bare "never called" assertion — the latter would have passed only by accident of the close-placement path's own calls.

Level 1 risk guards untouched. No new dependencies.

Test plan

• Unit tests added (tests/test_position_mae.py, 31 tests)
• flake8 --max-line-length=120 passes on all changed files
• pytest tests/ -q passes: 1317 passed
• No regressions in existing tests

🤖 Generated with Claude Code

[keitaj]

Review: max adverse excursion logging on close events

The core design is sound. Routing the sample through a new cache-only accessor is the right call — get_market_data() falls through to a REST L2 fetch on a miss, and this samples once per open position per cycle, so the fetching accessor would have added load whenever WS is off. Asserting the on/off call counts match, rather than a bare "never called", is also the correct form of that test. Seeding 0.0 so never-adverse positions stay in the denominator, and emitting on every close reason, are both right for the stated purpose.

Thread safety checks out: _record_close runs on the WS thread via on_position_closed/cleanup_closed while manage() samples on the main thread, but all _mae operations are single GIL-atomic dict ops, and a stale entry left by that race is never emitted (on_position_closed only logs when _open_positions still holds the coin) and is overwritten at the next registration. Same pattern as the existing _last_effective_max_age.

Two things to fix before merge, both in the integrity of the measurement itself.

---

1. The ghost-clear test is tautological, leaving three added lines uncovered (should-fix)

tests/test_position_mae.py — TestMaeEdgeCases::test_ghost_clear_drops_state:

_manage(closer, md, 99.0)
closer._open_positions.pop("BTC", None)
closer._mae.pop("BTC", None)      # the test does the pop
assert "BTC" not in closer._mae   # ... then asserts it happened

The test pops _mae itself and then asserts the pop occurred, so it passes regardless of production behaviour. AGENTS.md requires assertions that verify real behaviour.

This matters because it is the only test that looks like it covers the three _mae.pop() calls added at sites that do not go through _record_close:

• the ghost-position clear after _GHOST_CLEAR_THRESHOLD consecutive failures
• the "already closed before force close" early return in _handle_force_close
• the "already closed, skipping close order" early return in _place_take_profit

tests/test_ghost_position_fix.py has no _mae references, so all three lines are currently untested. If any of them were dropped, the suite would still pass and a stale peak could be carried into the next position on that coin.

Suggestion: drive the real path instead of mutating state. For the ghost clear, make placement fail for _GHOST_CLEAR_THRESHOLD cycles and assert _mae is gone afterwards; for the two early returns, set get_all_positions to report a flat position and assert _mae is cleared without a [close-reason] line being emitted.

---

2. _update_mae can store a negative value, breaking the documented 0.0 floor (should-fix)

strategies/mm_position_closer.py, _update_mae:

prev = self._mae.get(coin)
if prev is None or adverse_bps > prev[0]:
    self._mae[coin] = (adverse_bps, age)

On the prev is None branch the sample is stored unconditionally, including when the position is in profit. Confirmed reachable:

long, entry=100, mid=100.5, _mae entry absent
=> _mae["BTC"] == (-50.0, 5.0)

The seed normally prevents this, but _record_close pops _mae and can run on the WS thread while a manage() call is already past the registration check, so the branch is live — and any future caller reaching _update_mae without the seed hits it directly. Both the method docstring and the README state that positive means adverse and that a position which never moved against us records 0.0; the code does not guarantee that. For a measurement whose only job is to be trusted by a later parameter decision, the invariant should hold in the code rather than depend on the seeding path.

Suggestion:

floor = prev[0] if prev is not None else 0.0
if adverse_bps > floor:
    self._mae[coin] = (adverse_bps, age)

This also makes the -? in downstream parse patterns unnecessary rather than load-bearing.

---

3. The new README YAML line is the only one without a flag reference (nit)

README.md, ## Parameter Reference for AI Agents:

track_max_adverse_excursion: true  # (log mae_bps/mae_age on the close-event line; ...)

Every other entry in that block carries a # --flag-name reference. Omitting it is correct here — there deliberately is no CLI flag — but it reads as an oversight. Stating it explicitly, e.g. # (no CLI flag; JSON config only) ..., makes the intent visible.

---

Also noted, no change requested

The running max is taken across samples that may use different entry_price references if a position is averaged mid-hold, since bps are computed against the current entry each cycle. In practice the strategy does not re-quote while holding a position, so the exposure is partial fills of a single order at near-identical prices, and the existing unrealized_loss_close_bps has the same semantics — so this is pre-existing, not introduced here. Worth keeping in mind if averaging-up is ever added.

The lower-bound sampling caveat is documented in the method, the README and a test, and the asymmetry it implies (can show an early close would have cut recoveries short, cannot establish the converse) is stated in the PR body. That is the right level of care for data that will drive a threshold choice.

---

Verdict: Changes requested

No blockers; CI is green on all six checks. Both should-fix items are small, but they sit in the trustworthiness of the measurement: one leaves the state-cleanup paths unverified behind a test that cannot fail, and the other lets the code violate an invariant that the docs assert. Worth closing before this data is used to pick a threshold.

[keitaj]

Re-review: findings addressed

Both should-fix items from the previous review are resolved, and the fixes are verified rather than asserted.

1. Negative-value floor — _update_mae now computes floor = prev[0] if prev is not None else 0.0 and updates only above it, so the documented 0.0 floor holds in the code instead of depending on the registration seed having run. The reasoning for why the unseeded branch is live in production (_record_close pops the entry and can fire on the WS thread mid-manage()) is captured in the comment, which is the part a future reader would otherwise have to re-derive.

2. Tautological ghost-clear test — replaced with three tests that drive the real paths: repeated placement failure through _GHOST_CLEAR_THRESHOLD, and the two flat-position races (before force close, and before placement). The force-close one additionally asserts that no [close-reason] line is emitted, which pins the distinguishing property of these paths rather than just the end state. Three more cover the floor, including that it does not swallow a genuine adverse sample — the obvious way an over-eager floor would break.

Every new assertion was mutation-tested: removing each of the three _mae.pop() calls, and reverting the floor to its previous form, each fails the matching test and only that test. That is the check the original test could not pass.

3. README flag reference — now states (no CLI flag; JSON config only), consistent with the surrounding entries carrying a flag reference.

Suite is 1322 passed (up from 1317, +5 net after removing the vacuous test), flake8 clean on all changed files.

---

Verdict: LGTM

No blockers, no remaining should-fix items. Merging once the pending checks finish.

One item carried forward as an observation, not a change request: the running max is taken across samples computed against the current entry_price each cycle, so averaging a position mid-hold would mix reference prices. The strategy does not re-quote while holding, and unrealized_loss_close_bps already has identical semantics, so this is pre-existing — worth revisiting only if averaging-up is ever introduced.