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/boundary_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.

Part of #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).