M5-01: Define backtesting architecture and package boundaries

[rustyeddy]

What changed

Adds ADR-035 recording the M5 backtesting architecture and package boundaries, reached via #209's design-notes-then-review cycle — mirroring the pattern issue #176 (M4-01) used to extract ADR-005/ADR-006 ahead of M4 implementation. Also updates docs/arch/package-boundaries.org's preliminary package map.

Why it changed

Issue #209 asks for M5's architecture to be settled before implementation hardens accidental boundaries — no code, per its own scope.

Package boundaries: new strategy/, journal/, report/, backtest/, service/backtest/, cmd/trader/backtest/ packages, following the exact layering M4 already proved for pipeline/service/execution/cmd/trader/execution.

Five decisions review tightened before treating the architecture as settled:

1. backtest reuses pipeline.Pipeline directly, never service/execution — service/execution serves an external caller's transport-neutral shape; backtest is itself an orchestration package at the same tier.
2. backtest never constructs adapters/broker/sim.Broker internally — Runner receives broker.Broker and *pipeline.Pipeline as constructor-injected dependencies, mirroring service/execution's own existing shape (issue M4-11: Add execution and risk application service #186), so backtest stays free of any concrete adapter import (direct application of package-boundaries.org rule 1).
3. No-lookahead is a layered invariant, not one package's job: replay guarantees ordering, the scheduler owns visibility timing, the strategy View owns what's actually accessible.
4. report is rendering-only; the transport-neutral result/metrics model lives in backtest itself — a presentation package must not become the owner of financial/statistical computation.
5. Listing resolution is an injected Runner dependency (instrument.Resolver), not embedded in the immutable backtest.Request — a resolver is runtime infrastructure, not run input. The resolved listing's reproducibility-relevant fields belong in the run manifest (M5-07: Define immutable backtest run manifest #215) instead.

Also confirmed and recorded: v0 targets one account/one broker (portfolio aggregation deferred to #224), and the scheduler is not a policy owner (strategy emits intent, pipeline owns execution/risk semantics, broker owns authoritative state, journal observes, metrics derive from authoritative outcomes after the fact).

package-boundaries.org update: adds pipeline and the service/* tier to the dependency diagram (both introduced in M4 without a corresponding doc update) and corrects a stale "strategy deferred to M6" note — strategy is built in M5 (#210).

How it was tested

Documentation-only change. go build ./... confirmed clean (no code touched). Every existing-code reference in the new ADR (clock.Simulated.Advance, marketdata.Manager.Bars/BarQuery, id.RunID, clock/arch_test.go's TestDomainCodeDoesNotCallTimeDirectly) was verified directly against the actual source before citing it.

Which documentation changed

This PR is the documentation change — see above.

Closes #209.

[Copilot]

🟡 Changes recommended

ADR-035 contains a couple of incorrect concrete references (test file name and “previously unused” RunID claim) that should be corrected to keep the documentation accurate.

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

Pull request overview

This PR captures Milestone 5 (M5) backtesting architecture decisions as ADR-035 and updates the architecture documentation to reflect the intended package layering and dependency direction before implementation begins.

Changes:

• Adds ADR-035 documenting M5 backtesting package boundaries, layering, and invariants (pipeline reuse, injected dependencies, no-lookahead layering, metrics vs rendering, etc.).
• Updates the ADR registry (adr-decisions.org) to include ADR-035 and adds an explanatory note pointing to the standalone ADR file.
• Updates package-boundaries.org’s diagram and narrative to include the pipeline and service/* tiers and to correct the stale strategy milestone note.

File summaries

File	Description
docs/arch/package-boundaries.org	Updates the package dependency diagram and narrative to include pipeline, service/*, and M5 package placement.
docs/arch/adr-decisions.org	Registers ADR-035 and adds a note explaining it is maintained as a standalone ADR file.
docs/arch/adr-035-m5-backtesting-architecture.org	Introduces ADR-035 documenting M5 backtesting architecture decisions and package boundaries.

Review details

• Files reviewed: 3/3 changed files
• Comments generated: 2
• 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]

Architecture review: the substantive M5 decisions are in good shape and accurately incorporate the #209 review: concrete simulator construction stays out of backtest, no-lookahead is layered across replay/scheduler/View, metrics are separated from rendering, listing resolution is injected rather than embedded in the run request, and scheduler policy ownership is constrained appropriately.

I agree with Copilot's two documentation corrections and would fix them before merge:

1. ADR-035 cites clock/boundary_test.go for TestDomainCodeDoesNotCallTimeDirectly; the actual file is clock/arch_test.go.
2. Describing id.RunID as "previously unused" is inaccurate; it already participates in the existing ID/logging infrastructure. It is enough to say that id.RunID already exists and is the appropriate run identity type for M5.

One additional wording correction: the ADR's Consequences section says a future live package is expected in M7. Our current roadmap has OANDA/live brokerage as M6 after the M5/M6 flip. I recommend avoiding a milestone number there entirely (a future live package) so this ADR does not become stale again if roadmap sequencing changes.

With those three documentation fixes, I consider ADR-035/package-boundaries direction ready to merge. No architectural blocker beyond them.

[rustyeddy]

Addressed all three:

1. clock/boundary_test.go → clock/arch_test.go (Copilot + Rusty): verified directly — that's the real filename.
2. id.RunID "previously unused" (Copilot + Rusty): verified directly — it already has GenerateRunID/ParseRunID support and a logging.RunID attribute. Corrected to describe it as the appropriate existing run identity type for M5, not something new.
3. Stale "M7" for a future live package (Rusty): found and fixed in both ADR-035's Consequences section and package-boundaries.org (the second one wasn't flagged directly but had the identical issue) — dropped the milestone number entirely from both so this documentation doesn't go stale again if roadmap sequencing shifts further.

go build ./... clean (docs-only change).

[rustyeddy]

Fresh re-review after the earlier documentation fixes: those fixes are present, but I found one architectural inconsistency in the new boundary documentation that I think should be corrected before merge.

package-boundaries.org currently draws service/* as a dependency above application orchestration (backtest, live, scanner), while ADR-035 explicitly says backtest consumes pipeline directly and never service/execution. More importantly, the new service/backtest is described as an ADR-022 wrapper around backtest, so its dependency direction must be service/backtest -> backtest, not backtest -> service/backtest. The diagram/text currently imply the reverse relationship.

There is a related sentence in ADR-035 under "Runtime dependencies" that says the composition root may be service/backtest or cmd/trader/backtest and that it constructs adapters/broker/sim.Broker. I would not allow service/backtest to construct/import the concrete simulator. The M4 precedent cited immediately afterward is actually cmd/trader/execution/service.go constructing sim.Broker and injecting only the broker.Broker port into service/execution. M5 should preserve that same boundary: cmd/trader/backtest (or another outer composition root) constructs the simulator/pipeline/resolver and injects them into service/backtest/backtest; service/backtest itself stays adapter-neutral.

I recommend fixing the diagram so service/backtest is clearly an outer wrapper/consumer of backtest rather than a dependency of it, and tightening the ADR wording so concrete adapter construction belongs only to an outer composition root, not service/backtest.

Everything else I reviewed in ADR-035 still looks consistent with the #209 decisions: layered no-lookahead, rendering-only report, injected listing resolution, one-account/one-broker v0 scope, and reuse of the M4 pipeline are all good.

[rustyeddy]

Addressed the architectural inconsistency:

The diagram drew service/* as a dependency above "application orchestration (backtest, live, scanner)" — implying backtest depends on service/* — while ADR-035's own text says the reverse: service/backtest wraps backtest.Runner (the ADR-022 pattern), so the real direction is service/backtest -> backtest. Compounding it, ADR-035's "Runtime dependencies" section let service/backtest construct adapters/broker/sim.Broker itself, which would violate the exact boundary the cited M4 precedent (cmd/trader/execution/service.go, not service/execution, builds sim.Broker) actually establishes.

Redrew package-boundaries.org's diagram to show the correct chain: cmd/trader/backtest (outer composition root, constructs the concrete simulator/pipeline/resolver) → service/backtest (ADR-022 wrapper, adapter-neutral) → backtest (domain orchestration) → pipeline. Moved journal/report to sit alongside pipeline/backtest at the domain tier, not implied to be part of the service/* chain.

Fixed the identical inconsistency in ADR-035 itself: backtest is now described as depending on pipeline at the same tier service/execution occupies (not "the same tier as service/*", which had it backwards), and "Runtime dependencies" now states unambiguously that only cmd/trader/backtest constructs adapters/broker/sim.Broker — service/backtest never does.

go build ./... clean (docs-only change).