M5-02: Define strategy runtime contract - #228
Merged
Merged
Conversation
Adds the strategy package: Trader's broker-neutral strategy runtime contract (ADR-005), following #210's own design-notes-then-review cycle. Strategy is deliberately small -- Describe, Start, OnBar -- per review: TickHandler, FillHandler, AccountEventHandler, StateManager, and DataRequirement.NeedTicks are not published yet, each additive once a real M5 consumer needs it rather than speculated into place now. The central design point from review: a strategy never touches Trader's ID-generation machinery directly. Environment.Intents is a narrow IntentFactory capability the runtime injects -- it generates deterministic IntentID/EventID/CorrelationID values and calls order.NewIntent on the strategy's behalf, so every returned order.Intent is valid from the instant it exists (order.NewIntent's own contract, #177). The strategy still owns trading semantics: whether several intents from one OnBar call belong to one correlation group (e.g. a reversal expressed as an exit + enter pair) is the strategy's own explicit choice via NewCorrelationID/WithCorrelation, never silently decided by the runtime after the fact. This keeps deterministic ID generation as runtime infrastructure that can later cross an out-of-process strategy protocol boundary cleanly, rather than something every strategy author reimplements. View stays minimal (Account() account.Snapshot only) -- a historical-bar lookup method is deliberately deferred until #212/#213 prove the access pattern a real backtest needs, rather than freezing the wrong shape now. A boundary_test.go guard (prefix-aware, matching execution/risk's own established isForbiddenImport pattern) mechanically forbids importing broker, execution, risk, or pipeline. Tested: go build ./..., go vet ./..., gofmt -l ., go test ./... -race all clean. strategy package coverage: 85.0%. TestStrategy_RepresentativeInvocation drives a minimal representative Strategy (issue #210's own "test double, not a real trading strategy" scope, matching risk's own fakeRule precedent) through Describe/Start/OnBar plus real IntentFactory-built order.Intent construction end to end.
rustyeddy
commented
Aug 28, 2026
rustyeddy
left a comment
rustyeddy
left a comment
Owner
Author
There was a problem hiding this comment.
Good direction overall and the implementation follows the #210 design discussion well, especially the narrow IntentFactory, deferred View history API, and prefix-aware boundary guard. I found one acceptance-test issue that should be fixed before merge.
TestStrategy_RepresentativeInvocation does not actually demonstrate strategy intent emission. buyOnFirstBarStrategy.OnBar sets entered and returns nil; the test then calls env.Intents.Enter(...) itself outside the strategy. That proves IntentFactory works (already covered thoroughly by intent_test.go), but not the issue's acceptance criterion that a representative Strategy invocation emits an order.Intent through OnBar.
Please have the fixture retain the injected IntentFactory in Start, call it from OnBar, and return the resulting intent. Then assert the returned slice contains the canonical Enter intent. That also exercises the important lifecycle contract: the runtime injects capabilities in Start, and strategy logic uses those capabilities during callbacks.
Everything else looks consistent with #210 from this pass.
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The contract, boundary guard, and tests appear coherent and consistent with existing intent/ID semantics, with only minor doc/naming nits noted.
Pull request overview
Introduces a new strategy package that defines Trader’s broker-neutral strategy runtime contract (ADR-005), including the core Strategy interface plus minimal supporting contracts (Environment, View, Descriptor/DataRequirement, BarEvent) and a runtime-injected IntentFactory for deterministic, valid order.Intent construction without exposing ID generation to strategy authors.
Changes:
- Added the core strategy runtime interfaces (
Strategy,Environment,View) and data contracts (Descriptor,DataRequirement,BarEvent). - Added
IntentFactory+ implementation to mint deterministic IDs and build validated canonicalorder.Intentvalues viaorder.NewIntent. - Added tests covering representative invocation, intent construction semantics (kinds, correlation behavior, determinism), and a package-boundary import guard.
File summaries
| File | Description |
|---|---|
| strategy/doc.go | Package-level contract, scope, dependency direction, and intent/correlation rationale. |
| strategy/strategy.go | Defines the core Strategy interface (Describe, Start, OnBar). |
| strategy/environment.go | Defines injected runtime capabilities (Clock, Intents, Logger). |
| strategy/view.go | Defines minimal read-only View surface (Account() snapshot). |
| strategy/descriptor.go | Defines Descriptor and DataRequirement for strategy identity and bar requirements. |
| strategy/event.go | Defines BarEvent as the OnBar trigger payload. |
| strategy/intent.go | Defines IntentFactory and its implementation for deterministic, validated intent creation. |
| strategy/intent_test.go | Verifies intent kinds, correlation behavior, immutability of WithCorrelation, and determinism. |
| strategy/strategy_test.go | Representative contract invocation test scaffolding and end-to-end intent creation via factory. |
| strategy/boundary_test.go | Enforces dependency boundaries (strategy must not import broker/execution/risk/pipeline). |
Review details
- Files reviewed: 10/10 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+28
to
+30
| // Start is called once, before any OnBar call, with this run's own | ||
| // Environment. A strategy performs one-time setup here — it does | ||
| // not yet have a View, since no bar has been replayed. |
Comment on lines
+55
to
+60
| func mustEurUsdListing(t *testing.T) instrument.ID { | ||
| t.Helper() | ||
| inst, err := instrument.NewCurrencyPair(num.MustParseCurrency("EUR"), num.MustParseCurrency("USD")) | ||
| require.NoError(t, err) | ||
| return inst.ID() | ||
| } |
Comment on lines
+62
to
+69
| // buyOnFirstBarStrategy is a minimal, representative Strategy | ||
| // implementation (issue #210's own "tests demonstrate representative | ||
| // strategy invocation and intent emission" acceptance criterion, the | ||
| // same "test double, not a real trading strategy" scope risk's own | ||
| // fakeRule established): it enters long on the first bar it ever | ||
| // sees, and does nothing on every bar after — enough to exercise | ||
| // Describe/Start/OnBar and IntentFactory together without inventing | ||
| // real trading logic this issue does not need. |
Rusty's substantive finding: TestStrategy_RepresentativeInvocation didn't actually demonstrate strategy intent emission -- OnBar returned nil, and the test called env.Intents.Enter(...) itself, outside the strategy. That only re-proved IntentFactory works (already covered by intent_test.go), not the issue's own "tests demonstrate representative strategy invocation and intent emission" acceptance criterion. Fixed by having buyOnFirstBarStrategy retain the IntentFactory Start receives and call it from OnBar, returning the resulting order.Intent in the slice OnBar returns -- exercising the real lifecycle contract: the runtime injects capabilities once in Start, and strategy logic uses those retained capabilities across every later OnBar call. Also addressed Copilot's two smaller findings: renamed mustEurUsdListing to mustEurUsdInstrumentID (it returns an instrument.ID, not a venue-specific Listing -- Intents intentionally name instruments, not listings), and clarified Strategy's own doc comment that OnBar never receives an Environment, so a strategy needing Intents (or any other capability) must retain what it needs from Start. Tested: go build ./..., go vet ./..., gofmt -l ., go test ./... -race all clean. strategy package coverage unchanged at 85.0%.
Owner
Author
|
Addressed all three:
Full suite (`go build`, `go vet`, `gofmt -l`, `go test ./... -race`) clean. Coverage unchanged at 85.0%. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changed
Adds the
strategypackage: Trader's broker-neutral strategy runtime contract (ADR-005), per #210's design-notes-then-review cycle.Strategyis deliberately small —Describe,Start,OnBar— per review:TickHandler,FillHandler,AccountEventHandler,StateManager, andDataRequirement.NeedTicksare not published yet; each is additive once a real M5 consumer needs it, rather than speculated into place now.Why it changed
The central design point from review: a strategy never touches Trader's ID-generation machinery directly.
Environment.Intentsis a narrowIntentFactorycapability the runtime injects — it generates deterministicIntentID/EventID/CorrelationIDvalues and callsorder.NewIntenton the strategy's behalf, so every returnedorder.Intentis valid from the instant it exists (order.NewIntent's own contract, #177). The strategy still owns trading semantics: whether several intents from oneOnBarcall belong to one correlation group (for example a reversal expressed as an exit + enter pair) is the strategy's own explicit choice viaNewCorrelationID/WithCorrelation, never silently decided by the runtime after the fact. This keeps deterministic ID generation as runtime infrastructure that can later cross an out-of-process strategy protocol boundary cleanly, rather than something every strategy author reimplements.Viewstays minimal (Account() account.Snapshotonly) — a historical-bar lookup method is deliberately deferred until #212/#213 prove the access pattern a real backtest needs, rather than freezing the wrong shape now.A
boundary_test.goguard (prefix-aware, matchingexecution/risk's own establishedisForbiddenImportpattern) mechanically forbids importingbroker,execution,risk, orpipeline— satisfying this issue's own acceptance criterion, not just convention.How it was tested
TestStrategy_RepresentativeInvocationdrives a minimal representativeStrategy(issue M5-02: Define strategy runtime contract #210's own "test double, not a real trading strategy" scope, matchingrisk's ownfakeRuleprecedent) throughDescribe/Start/OnBarplus realIntentFactory-builtorder.Intentconstruction end to end — satisfying this issue's own "tests demonstrate representative strategy invocation and intent emission" acceptance criterion.TestIntentFactory_*: all four intent kinds (Enter/Exit/AdjustStop/TargetExposure), default fresh-correlation-per-call behavior, explicitWithCorrelationgrouping (and that it doesn't mutate the original factory), and cross-instance determinism.TestStrategyNeverImportsBrokerExecutionRiskOrPipeline/TestIsForbiddenImport: the boundary guard, including the exact-or-prefix-with-"/" matching regression PR M4-04: Define execution planning contracts #194 established forexecution's own guard.go build ./...,go vet ./...,gofmt -l .,go test ./... -raceall clean.strategypackage coverage: 85.0%.Which documentation changed
None required beyond ADR-005/ADR-035, both already accepted and unchanged by this issue — #210 implements the contract those decisions already describe.
Closes #210.