fix(engine): trading_hours_per_day is global while instruments resolve per symbol (mixed books get wrong bars-per-day)

[zachisit]

Follow-up to #264 / #265, raised as a minor in @shardul0701's round-2 review of #265.

Problem

trading_hours_per_day is a single process-wide number read from CONFIG, while instrument metadata already resolves per symbol — resolve_instrument(sym, CONFIG) branches per symbol on margin mode, integer units, borrow, and calendar (see helpers/instruments.py, added in #229).

Session length is the one instrument property that stayed global. A mixed book of equities (6.5h RTH) and 24h futures (CME_ETH) therefore gets the wrong bars-per-day for one side of the book no matter which value is configured.

Consumers of the assumption:

• helpers/timeframe_utils.py::get_bars_per_day() → the ADV window + rescale from fix(engine): scale ADV liquidity-cap window to timeframe, not a fixed 20 bars #265 (max_pct_adv liquidity cap, volume_impact_coeff market impact)
• helpers/timeframe_utils.py::get_bars_per_year() → annualization

Concretely, on 1-hour bars with trading_hours_per_day = 6.5, a 24h futures symbol gets bpd = 6 when its true session is 24 bars — a ~4× ADV understatement on that symbol, on top of #268's truncation error.

Severity

Pre-existing, out of scope for #265, and currently latent: it only bites once a single portfolio genuinely mixes asset classes with different session lengths. Filing it so it is tracked rather than rediscovered.

Design note / open question

This is the largest of the three #265 follow-ups because it changes a function signature that the annualization path also depends on. Proposed conservative shape:

• Resolve session hours per symbol via the existing instrument layer (the Instrument already carries calendar), falling back to CONFIG["trading_hours_per_day"] when the instrument doesn't specify one.
• Keep the existing global-config behaviour as the exact default so no current configuration changes behaviour.
• Thread the per-symbol value through the ADV path first (where correctness impact is concrete and fix(engine): scale ADV liquidity-cap window to timeframe, not a fixed 20 bars #265 already isolated the call sites); treat the annualization path as a separate decision, since changing it retroactively alters reported Sharpe/CAGR on existing futures runs.

Open question for review: whether the annualization consumer (get_bars_per_year) should move per-symbol in the same change or stay global pending an explicit decision — it has reporting-comparability consequences that the ADV path does not.

Dependency

Overlaps get_bars_per_day() with #268. #268 should land first; this builds on the exact-bars-per-day helper it introduces.

Acceptance criteria

• Per-symbol session length resolved via the instrument layer, global config as fallback
• ADV path uses the per-symbol value
• Existing single-asset-class configs byte-for-byte unchanged; golden master green
• Test covering a mixed equity + 24h-futures book
• Explicit decision recorded on the annualization path

[shardul0701]

Status: complete, pending merge — held for review

This is fully resolved by #274 (stacked on #271 → #272 → #274; merge order retargets automatically). Not merged yet — holding for Zach's review per the standing overnight rule, but the fix, tests, and two independent QA passes are done.

Why this is complete

Root cause fixed exactly as scoped. trading_hours_per_day was one process-wide number while every other instrument property (margin mode, integer units, borrow, calendar) already resolved per symbol via resolve_instrument(). Session length was the one holdout, so a mixed book — equities at 6.5h RTH alongside a 24h future — got the wrong bars-per-day for whichever side didn't match the single global value.

Design matches the acceptance criteria one-for-one:

• Per-symbol session length resolved via the instrument layer, global config as fallback — new resolve_session_hours(symbol, config) precedence ladder: explicit per-symbol override → opt-in calendar derivation (CALENDAR_SESSION_HOURS) → the existing global trading_hours_per_day → 6.5 default.
• ADV path uses the per-symbol value — threaded through helpers/timeframe_utils.py (new optional session_hours= arg) and helpers/portfolio_simulations.py (per-symbol window/bpd, cached).
• Existing single-asset-class configs byte-for-byte unchanged — steps 3–4 of the ladder are exactly pre-fix(engine): trading_hours_per_day is global while instruments resolve per symbol (mixed books get wrong bars-per-day) #270 behavior. Nothing new fires unless a config explicitly sets a per-symbol override or opts into calendar derivation. Verified: full suite was green before the new tests were added (1790 passed), confirming the default path is untouched by the refactor itself.
• Test covering a mixed equity + 24h-futures book — the engine-level tests assert exactly the failure mode in this issue: two symbols with identical per-bar volume in one portfolio get different ADV caps once one has a 23h session (CME_ETH — measured as 23h, not 24, accounting for the daily CME maintenance break, not just calendar day length). A single global bars-per-day cannot produce that; the old code would have given both symbols the same cap.
• Explicit decision recorded on the annualization path — get_bars_per_year() intentionally stays global. Reasoning: the ADV path is a sizing-correctness fix with contained blast radius (fix(engine): scale ADV liquidity-cap window to timeframe, not a fixed 20 bars #265 had already isolated its call sites); annualization feeds reported Sharpe/CAGR, and moving it per-symbol would retroactively change those numbers on existing futures runs — a reporting-comparability decision, not a bug fix. Documented in CLAUDE.md; left as a follow-up ticket if you want it moved too.

Verification depth

• 21 new tests in tests/test_per_symbol_session_hours.py — full precedence-ladder coverage, opt-in-defaults-off, override-beats-opt-in, unknown-calendar fallback, non-positive rejection, session_hours=None meaning "unset."
• Mutation-verified: reverting the per-symbol resolution back to one global value fails 2 tests — confirms the tests actually pin the mechanism, not just shape.
• Full suite: 1811 passed, 5 skipped, 7 deselected, 0 failed. Golden master green (byte-for-byte on D timeframe).
• Built on fix(engine): get_bars_per_day() truncation leaves ADV understated on H timeframes (#264 only half-closed) #268's exact-bars-per-day helper as specified in the dependency note.

Nothing left open on this ticket. PR #274 is MERGEABLE, sitting unmerged, ready for your review whenever you're back.