feat: add fill feature logging for ML dataset building

[keitaj]

Summary

• Persist per-fill order book features (spread, book imbalance, micro-price skew, top-of-book sizes) plus the adverse selection tracker's 5s/30s/60s markout samples as one JSON line per fill, in a daily-rotated JSONL file — a ready-made supervised dataset for offline adverse-selection modelling.
• Feature definitions live in a single pure function (fill_features.compute_fill_features) shared by the logger and any future in-bot inference, structurally preventing train/serve skew.
• Observation-only and fail-silent: the WebSocket thread performs no file IO (in-memory buffer, main-loop flush after a 65s maturity window so markout labels land in the same line); IO errors, a daily size cap, and a buffer cap are all counted and never affect trading. Default OFF preserves existing behaviour exactly.

Changes

• fill_features.py (new): shared pure feature computation + FEATURE_SCHEMA_VERSION.
• ws/fill_feature_writer.py (new): FillFeatureWriter — buffered, maturity-gated, daily-rotated JSONL writer with size/buffer caps and rate-limited error logging.
• ws/adverse_selection_tracker.py: optional raw_fill kwarg on on_fill (backward compatible), feature record composition (tid/oid/hash join keys + features), set_feature_writer() hook, record field on FillSnapshot.
• ws/fill_feed.py: pass the raw userFills object to the tracker (raw_fill=fill).
• bot.py: writer wiring (warns and disables when the adverse tracker is unavailable), main-loop maybe_flush(), shutdown flush_all(), config keys + defaults, argparse flags.
• ws/__init__.py: export FillFeatureWriter.
• README.md: human-facing description + AI YAML parameter reference entries.
• Tests: test_fill_features.py, test_fill_feature_writer.py (new), plus additions to tracker/fill-feed tests and a stub-attribute fix in test_risk_check_throttle.py.

Test plan

• Unit tests added/updated (feature math, maturity gating, rotation, size/buffer caps, fail-silent paths, join-key passthrough, backward compatibility of legacy on_fill calls)
• flake8 passes
• pytest passes (1,149 tests)
• No regressions in existing tests (default OFF path unchanged)

🤖 Generated with Claude Code

[keitaj]

Review: fill feature logging for ML dataset building

Solid, well-scoped observation-only feature: pure feature function shared for train/serve parity, WS thread stays IO-free, failures swallowed, size/buffer caps in place. Two real correctness gaps around record durability and a set of test-quality gaps where the intent isn't actually pinned. Observation-only, so nothing here affects trading — but the whole point is the dataset, and these let silent data loss through. Changes requested.

---

1. Buffered records lost on non-signal shutdown paths (should-fix)

bot.py:

flush_all() is wired only into _signal_handler. The main loop's internal exits — the daily-loss stop_bot action and the emergency-stop cooldown path — just set self.running = False and let run() return, with no flush (nothing follows run() in __main__). These exits are self-initiated, so no signal reaches _signal_handler. On those paths every record still buffered (the last maturity window plus anything accumulated while risk checks were failing) is silently dropped — and those are exactly the highest-value samples, since fills during maximum adverse selection are what tripped the stop, and the watchdog restart makes this a routine occurrence.

Suggestion: wrap the main loop in try/finally with if self.fill_feature_writer: self.fill_feature_writer.flush_all(), or call it unconditionally after the loop, so all exit paths flush.

---

2. One bad record / one IO error drops the whole flush batch (should-fix)

ws/fill_feature_writer.py _flush:

Records are popleft'd from the buffer before serialization, and json.dumps runs in a loop inside the same try. A [good, bad, good] batch discards all three (written=0, no dropped_* counter incremented, unlike the overflow/size-cap paths which are counted). The unserializable trigger is effectively unreachable through the production path today, but the identical batch-loss path is reachable via an open()/write OSError (disk full, permission) — a realistic VPS event — losing a full flush window of good rows, up to the whole buffer on a failed flush_all at shutdown.

test_unserializable_record_counted_not_raised buffers a single bad record, so it cannot distinguish "drop the bad record" from "drop the batch"; test_io_error_does_not_raise asserts only errors == 1 and pins no retention contract.

Suggestion: serialize before popping (or re-buffer on error), count error-path drops, and add a mixed-batch test that pins the intended contract.

---

3. Rotation test is tautological (should-fix)

tests/test_fill_feature_writer.py test_daily_rotation_by_utc_date:

The test patches writer._current_path with a side_effect returning two hardcoded filenames — the method that implements rotation — so the real datetime.now(timezone.utc).strftime('%Y%m%d') logic never runs. Mutating _current_path to local time / a different format passes all writer tests unchanged. On a non-UTC VPS a UTC→local regression would mislabel/split the daily partitions with no test signal.

Suggestion: patch ws.fill_feature_writer.datetime (or inject a clock) and assert the real filename for two UTC instants, including one where local date ≠ UTC date.

---

4. No test covers the bot.py wiring (should-fix)

bot.py:

The only bot-side test change is fixture maintenance (b.fill_feature_writer = None). The new true-branches are never exercised: the main-loop maybe_flush(), the shutdown flush_all(), and the run()-time warning-and-disable path when the feature is enabled without --enable-ws / --enable-adverse-selection-log. If maybe_flush is dropped in a future refactor, no test fails and the writer buffers to its cap then silently evicts — the dataset is never written despite the feature being "enabled." test_risk_check_throttle already builds a mocked bot and calls the trading loop, so a maybe_flush.assert_called() with a mock writer is cheap.

---

5. Fills near UTC midnight land in the next day's file (nit)

ws/fill_feature_writer.py _flush:

The path is resolved at flush time via datetime.now(timezone.utc). Because records are held ~65s for maturity plus up to one flush interval, any fill in roughly the last ~2 minutes of a UTC day is written into the following day's file (and counts toward that day's size cap). The per-record ts keeps joins correct, but consumers that partition by filename attribute those fills to the wrong day.

Suggestion: derive the path from the record's fill wall-time, or document that the file date is the write date, not the fill date.

---

6. Buffer-overflow test doesn't assert which record is dropped (nit)

tests/test_fill_feature_writer.py test_buffer_overflow_drops_oldest:

Asserts only the count and dropped_overflow; a regression to drop-newest would pass. Assert the surviving records are the newest ones so the eviction policy the name claims is actually pinned.

---

Verdict: Changes requested

No blockers and no risk-guard concerns — the feature is observation-only and the defaults preserve existing behaviour. But findings 1 and 2 are real durability gaps that silently lose exactly the samples this dataset most needs, and 3/4 mean the rotation and wiring behaviour isn't actually tested. Worth addressing before merge.

[keitaj]

Follow-up: review findings addressed

Pushed fixes for the findings above:

• #1 shutdown durability — added a post-loop flush_all() in run() so the internal stop_bot (daily-loss) and cooldown exits flush the final maturity window; it's a no-op when the signal handler already flushed.
• #2 batch loss — _flush now serializes per-record (one unserializable record is dropped + counted via a new dropped_serialize counter, not the whole batch) and re-buffers serializable records at the front on a transient write OSError so they retry instead of being lost.
• #3 rotation test — now drives ws.fill_feature_writer.datetime and asserts the real YYYYMMDD.jsonl filename across a UTC-midnight boundary (plus a direct UTC-vs-local check), so a UTC→local regression is caught.
• #4 wiring — new test_fill_feature_wiring.py covers the main-loop maybe_flush, the post-loop flush_all on a stop_bot/cooldown exit, and the prerequisite warning-and-disable path.
• #5 midnight (nit) — documented that the file date is the write date, not the fill date, and that consumers needing strict fill-date partitioning should key on the per-record ts.
• #6 overflow test (nit) — now asserts the oldest record is the one evicted.

flake8 clean, full suite green. Ready for re-review.

[keitaj]

Re-review: fix commit verifying the earlier findings

Re-reviewed fix: harden fill feature writer durability… against the six findings from the prior review. All addressed, and the fix itself introduces no new correctness issues:

• Shutdown durability (was #1) — post-loop flush_all() added after the main loop, guarded by if self.fill_feature_writer. No double-flush (the signal handler flushes then nulls the writer), fail-silent. ✓
• Batch-loss resilience (was #2) — _flush now serializes per-record (poison record → dropped_serialize, discarded, not retried), and re-buffers the serializable records at the front on a write OSError for the next flush to retry. Verified extendleft(reversed(serialized)) preserves front-order; poison is not re-buffered so there's no retry loop; the outer except keeps the pre-existing fail-silent contract for unexpected errors. ✓
• Rotation test (was #3) — now drives ws.fill_feature_writer.datetime and asserts the real YYYYMMDD.jsonl across a UTC-midnight boundary, plus a direct UTC-vs-local assertion. A UTC→local regression would now fail. ✓
• Wiring coverage (was #4) — test_fill_feature_wiring.py exercises the main-loop maybe_flush, the post-loop flush_all on a stop_bot/cooldown-style exit, and the prerequisite warning-and-disable path. ✓
• Midnight (was #5, nit) — documented write-date-not-fill-date semantics on _current_path; joins stay correct via per-record ts. ✓
• Overflow test (was #6, nit) — now asserts the oldest record is the one evicted. ✓

Minor residuals, both acceptable as-is (observation-only feature): on a persistent disk-full, extendleft onto a full deque evicts newest records — bounded-buffer trade-off, documented; and truly-unexpected (non-OSError) exceptions still drop the popped batch, which is intentional to avoid a poison retry loop.

Verdict: LGTM

All prior findings resolved, no new blockers or should-fix, CI green. Proceeding to merge.