M5-04: backtest.Replay — deterministic multi-requirement bar replay

[rustyeddy]

What changed

Adds the backtest package's first deliverable: backtest.Replay, which merges one marketdata.BarReader per strategy.DataRequirement into a single, chronologically ordered Next(ctx) (strategy.BarEvent, error) stream over a requested marketdata.TimeRange, reading only already-published canonical data (never a provider or the network — true by construction via Manager.Bars).

Per the design-notes review on #212:

• Canonical merge order: (bar timestamp, instrument ID, interval) — an intrinsic property of the data, never caller-supplied requirement order. TestNewReplay_OrderIsIndependentOfRequirementInputOrder proves constructing the same requirement set in reverse input order produces an identical replay sequence.
• Duplicate requirements: a (instrument, interval) pair named twice is rejected up front with ErrDuplicateRequirement, before any coverage check or reader is opened — this is what guarantees the merge order above is a genuine total order.
• Full coverage preflight: NewReplay checks every requirement's coverage before opening any reader. Incomplete coverage returns a structured *CoverageError naming every failing requirement (not just the first) plus each one's marketdata.Coverage; errors.Is(err, marketdata.ErrDataUnavailable) succeeds against it via a custom Is method.
• Partial-open cleanup: if opening a later requirement's reader fails after earlier ones already opened, NewReplay closes everything already acquired before returning the error.
• Close is idempotent; Next continues returning io.EOF after exhaustion or after Close.
• WarmupBars is ignored entirely — Replay returns exactly the requested span, never silently widened; warm-up is M5-05: Implement deterministic backtest scheduler #213/M5-06: Define and enforce no-lookahead and warm-up semantics #214's concern.

Why it changed

Issue #212 (M5-04). backtest needs one deterministic, reproducible input stream before the scheduler (#213) and strategy runner (#214) can be built against it. ADR-035 assigns backtest sole ownership of no-lookahead-safe replay; this is the first of that layered invariant's three parts (replay ordering — scheduler visibility and strategy View access come later).

How it was tested

• go build ./..., go vet ./..., gofmt -l . all clean.
• go test ./... -race passes across the whole module.
• backtest package coverage: 88.9%.
• New tests (backtest/replay_test.go, black-box) and (backtest/replay_internal_test.go, white-box) cover: deterministic and input-order-independent merging across two intervals; exact span/requirement filtering; WarmupBars never widening the span; duplicate-requirement rejection; multi-failure and partial coverage preflight (*CoverageError, errors.Is against marketdata.ErrDataUnavailable); io.EOF repeat-after-exhaustion and repeat-after-Close; Close idempotency; context cancellation in both NewReplay and Next; and the merge tie-break's instrument/interval branches directly.
• Test fixture: backtest/testdata/raw/oanda/ — a committed copy of service/marketdata's own EUR/USD H1/D1 raw archive fixture (same convention: package-local copy, not a cross-package reference), built into canonical data at test time via Manager.Plan+Manager.Build, the same test-only shortcut service/marketdata's own tests use.

Which documentation changed

None — this issue is implementation against the already-accepted ADR-035/package-boundaries.org design; no architecture decision changed.

Closes #212

[Copilot]

🟡 Changes recommended

The merge tie-break currently depends on display-only Interval.String() (risking unstable “canonical” ordering), and the single-failure coverage error message can be misleading when partitions are non-current without gaps.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Adds the initial backtest package deliverable, backtest.Replay, to merge multiple canonical bar streams into a single deterministic Next(ctx) (strategy.BarEvent, error) stream over a requested marketdata.TimeRange, plus fixtures and tests to validate ordering, coverage behavior, and EOF/Close semantics.

Changes:

• Introduces backtest.Replay with coverage preflight, duplicate-requirement detection, deterministic merge ordering, and idempotent Close.
• Adds black-box + white-box tests covering ordering independence, coverage error aggregation, EOF/Close semantics, and context cancellation.
• Adds package docs, an architecture boundary test, and raw Oanda fixture CSVs used to build canonical data in tests.

File summaries

File	Description
backtest/replay.go	Implements Replay, CoverageError, and deterministic merge logic with coverage preflight.
backtest/replay_test.go	Black-box tests for ordering, filtering, coverage failures, EOF/Close, and cancellation.
backtest/replay_internal_test.go	White-box tests for tie-break branches and helper behaviors.
backtest/doc.go	Establishes package-level responsibilities and architectural intent for backtest.
backtest/boundary_test.go	Guards package boundary (no imports of adapters/service/cmd from backtest).
backtest/testdata/raw/oanda/EURUSD/2024/01/EURUSD-2024-01-h1.csv	Raw H1 fixture data for tests.
backtest/testdata/raw/oanda/EURUSD/2024/01/EURUSD-2024-01-d1.csv	Raw D1 fixture data for tests.
backtest/testdata/raw/oanda/EURUSD/2024/02/EURUSD-2024-02-h1.csv	Additional raw fixture partition for test archive completeness.

Review details

• Files reviewed: 8/8 changed files
• Comments generated: 5
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[rustyeddy]

The merge/replay mechanics look well structured, and I agree with Copilot's interval-ordering and diagnostic-message comments. I found one architectural issue that I think should be resolved before merge.

The Coverage() preflight makes replay depend on the raw archive, not just the persisted canonical dataset. marketdata.Coverage explicitly calls rawInventoryLookup, requires RawRoot for M1/H1/H4/D1, and uses the current raw fingerprint to classify a canonical partition as stale. That conflicts with #212's reproducibility boundary and with this PR's own statement that replay reads only already-published canonical data. A canonical dataset that Manager.Bars() can successfully replay can fail NewReplay merely because RawRoot is absent, or because the raw archive has changed since that canonical revision was published. In other words, replay validity can currently change because of state outside the persisted canonical input being replayed.

For a backtest, I think Manager.Bars() should be the authority for whether canonical history is replayable. Its contract already says it is strictly read-only, canonical-store-only, returns no partial result, and proves complete coverage of the requested range before returning a fully materialized BarReader. Replay should not additionally require the source material from which those canonical bars happened to be built.

I would either:

• pre-open Bars() for every requirement and aggregate requirement-qualified ErrDataUnavailable failures before constructing Replay (the readers are already fully materialized and own no external resource), or
• if M5 truly needs rich canonical-only gap diagnostics, add/use a canonical-store-only coverage primitive whose semantics do not include raw freshness. I would not reuse the existing M2 Coverage(), because its notion of "current" answers a data-maintenance question, not the backtest question "is this persisted canonical revision complete and readable?"

This distinction will become even more important when run manifests/provenance arrive: a reproducible historical run should be able to replay a pinned canonical revision even if the original raw archive is moved, removed, or later superseded.

The other open Copilot findings are worth fixing in the same pass: use intrinsic interval fields rather than display String() for canonical ordering, and don't report 0 gap(s) when failure is actually a non-current partition. But the raw-archive dependency is the one I consider substantive for #212's architecture/acceptance criteria.

[rustyeddy]

Thanks both — addressed in e4238a4:

• Raw-archive dependency (Rusty's substantive finding): NewReplay no longer calls manager.Coverage. It now opens every requirement directly via manager.Bars — the same canonical-store-only call Replay itself later drains — and aggregates any ErrDataUnavailable failures into *CoverageError, closing every already-opened reader before returning on failure. This matches the first option you proposed: Bars() is now the sole authority for whether canonical history is replayable, and replay validity no longer depends on RawRoot/raw-archive freshness at all. CoverageError.Failures now carries FailedRequirement{Requirement, Err} — Err is Bars' own wrapped error — instead of a marketdata.Coverage value.
• Interval ordering (Copilot): lessStream's interval tie-break now compares Interval.Unit()/Interval.Count() (intrinsic) instead of Interval.String() (display-only, no stability guarantee).
• Misleading "0 gap(s)" message (Copilot): no longer possible — the error message is now Bars' own error text directly, which already describes the actual failure (missing coverage range) accurately for every case, not just the Gaps-derived case.

Tests updated to match (coverage-failure assertions now check FailedRequirement.Err/Requirement; tie-break tests use Unit()/Count() ordering, which flips H1/D1 relative to the old lexical ordering). Full suite green, backtest package coverage 89.5%.

[rustyeddy]

The major issue from my first pass is fixed correctly: NewReplay now uses Manager.Bars() as the replayability authority, so backtests depend only on the persisted canonical dataset and no longer require or reinterpret the raw archive. The interval tie-break is also now based on intrinsic Unit()/Count(), and the misleading 0 gap(s) diagnostic is gone.

I found one remaining error-classification issue before merge.

CoverageError now aggregates all Manager.Bars() failures, but its Is method still unconditionally reports true for marketdata.ErrDataUnavailable:

func (e *CoverageError) Is(target error) bool {
    return target == marketdata.ErrDataUnavailable
}

That was valid when CoverageError represented only an explicit coverage-preflight failure. It is no longer valid now that a FailedRequirement.Err can be any Bars() failure: invalid query/configuration, listing-resolution failure, ErrInconsistentData, store corruption, etc. A caller could therefore get errors.Is(err, marketdata.ErrDataUnavailable) == true for an error that is actually a data-integrity or configuration problem, which is exactly the kind of distinction the service/CLI layer will eventually need to preserve.

Please make CoverageError.Is reflect its contained causes instead of manufacturing ErrDataUnavailable. For example, return true only if at least one contained Failure.Err matches the requested target (or implement Unwrap() []error over the contained errors and let Go's normal errors.Is traversal do the work). If the intent is for this wrapper to contain only unavailable-data failures, then non-ErrDataUnavailable errors should instead abort NewReplay directly rather than being accumulated as coverage failures.

I prefer the latter semantic split: aggregate genuine ErrDataUnavailable failures across requirements, but return configuration/resolution/inconsistent-data errors immediately because they are not 'coverage unavailable' conditions.

With that classification fixed, this looks ready to merge from my pass.

[rustyeddy]

Fixed in bbf2966: NewReplay now classifies each `Bars` failure — only errors matching `marketdata.ErrDataUnavailable` are accumulated into `CoverageError`; any other error (invalid query, listing-resolution failure, `ErrInconsistentData`, etc.) aborts `NewReplay` immediately with that error, closing any readers already opened. `CoverageError` drops the hand-rolled `Is` method in favor of `Unwrap() []error` over its contained causes, so `errors.Is`/`errors.As` traverse the real errors instead of an unconditional match.

Added a test proving a non-`ErrDataUnavailable` failure (unregistered instrument) aborts immediately rather than landing in `CoverageError`, plus one proving `CoverageError`'s `Unwrap` doesn't falsely match an unrelated sentinel. Full suite green, `backtest` coverage now 93.7%.