diff --git a/backtest/scheduler.go b/backtest/scheduler.go index 9620434..7696fe6 100644 --- a/backtest/scheduler.go +++ b/backtest/scheduler.go @@ -329,6 +329,19 @@ func (d SchedulerDeps) validate() error { // Scheduler depends on it as a separate, narrow, structurally-satisfied // capability instead of widening broker.Broker. // +// # Fill delivery (FillHandler) +// +// Every fill this batch produced — from Phase 2's flush and Phase 3's +// IntrabarAdvancer triggers alike — is delivered, in delivery order, +// to Strategy's own optional strategy.FillHandler capability, against +// the same frozen View the batch's own OnBar calls use, strictly +// before any of them run (ADR-060). This is Strategy's one +// authoritative source of "did a fill actually happen," for the rare +// case View/History state genuinely cannot answer that (see +// strategy/smatrend's own same-bar bracket round-trip detection, +// issue #368/#370, for the motivating example) — most strategies never +// need it. +// // # Cancellation // // ctx is checked at the start of every batch, before each flush/OnBar/ @@ -526,6 +539,12 @@ func (s *Scheduler) runBatch(ctx context.Context, batch []strategy.BarEvent) err return err } + // Captured before anything in this batch can append to s.fills + // (Phase 2's flush, Phase 3's IntrabarAdvancer triggers), so the + // slice below the FillHandler delivery loop uses is exactly this + // batch's own new fills (ADR-060). + fillsBefore := len(s.fills) + // Phase 0: every event's own (instrument, interval) must be one of // Strategy's own declared DataRequirements (issue #214 review). An // undeclared stream would be invisible to History/warm-up @@ -654,6 +673,26 @@ func (s *Scheduler) runBatch(ctx context.Context, batch []strategy.BarEvent) err }, } + // Deliver every fill this batch produced (Phase 2's flush, Phase + // 3's IntrabarAdvancer triggers — both already drained/journaled + // above) to Strategy's own optional FillHandler capability, in + // delivery order, against the identical frozen view the OnBar loop + // below is about to use — authoritative execution history and + // OnBar both reason from one consistent account snapshot (ADR-060). + // A FillHandler must return no intents in this v0 (see + // strategy.FillHandler's own doc comment); nothing here queues or + // submits anything on Strategy's behalf. + if fh, ok := s.deps.Strategy.(strategy.FillHandler); ok { + for _, f := range s.fills[fillsBefore:] { + if err := ctx.Err(); err != nil { + return err + } + if err := fh.OnFill(ctx, strategy.FillEvent{Fill: f}, view); err != nil { + return fmt.Errorf("backtest: scheduler: OnFill for fill %s at %s: %w", f.FillID, t, err) + } + } + } + type collected struct { key requirementKey intents []order.Intent diff --git a/backtest/scheduler_test.go b/backtest/scheduler_test.go index f6eb26e..9355a9e 100644 --- a/backtest/scheduler_test.go +++ b/backtest/scheduler_test.go @@ -1486,6 +1486,101 @@ func TestScheduler_BracketEntryProtectsFromTheFillBarItself(t *testing.T) { "the closing fill must land on bar 2 (01:00) itself, the same bar the entry filled on — proving the stop was already resting before that bar's own IntrabarAdvancer check, not one bar later (02:00, as the equivalent two-step strategy needs — see TestScheduler_IntrabarAdvancerTriggersRestingStop)") } +// recordedEvent is one entry in fillRecordingStrategy's own ordered +// log — either a delivered fill or an OnBar call — kept structured +// rather than string-formatted so the test asserting on it never +// depends on instrument.ID's own String() format. +type recordedEvent struct { + kind string // "fill" or "onbar" + instrument instrument.ID + timestamp time.Time + side order.Side // only meaningful for "fill" +} + +// fillRecordingStrategy wraps bracketEntryStrategy's own bracket +// submission, additionally implementing strategy.FillHandler +// (ADR-060) and recording, in delivery order, one recordedEvent per +// OnFill call and one per OnBar call — proving Scheduler delivers +// every fill for a batch strictly before that batch's own OnBar +// calls, not merely that OnFill is called at all. +type fillRecordingStrategy struct { + bracketEntryStrategy + log []recordedEvent +} + +func (s *fillRecordingStrategy) OnFill(ctx context.Context, event strategy.FillEvent, view strategy.View) error { + s.log = append(s.log, recordedEvent{ + kind: "fill", + instrument: event.Fill.Listing.InstrumentID(), + timestamp: event.Fill.Timestamp, + side: event.Fill.Side, + }) + return nil +} + +func (s *fillRecordingStrategy) OnBar(ctx context.Context, ev strategy.BarEvent, view strategy.View) ([]order.Intent, error) { + s.log = append(s.log, recordedEvent{kind: "onbar", instrument: ev.Instrument, timestamp: ev.Bar.Time}) + return s.bracketEntryStrategy.OnBar(ctx, ev, view) +} + +// TestScheduler_FillHandlerDeliversFillsBeforeOnBarInSameBatch is +// ADR-060's own required end-to-end proof (issue #370): every fill a +// batch produces is delivered to strategy.FillHandler.OnFill, in +// delivery order, strictly before that same batch's own OnBar calls — +// not merely that OnFill fires at all. Reuses +// TestScheduler_BracketEntryProtectsFromTheFillBarItself's own +// fixture: both the bracket's entry fill and its own stop-triggered +// closing fill land on bar 2 (01:00), the same bar whose own OnBar +// call (for EUR/USD) fillRecordingStrategy also logs. +func TestScheduler_FillHandlerDeliversFillsBeforeOnBarInSameBatch(t *testing.T) { + mgr := newSchedulerTestManager(t) + replay := newTwoInstrumentReplay(t, mgr) + t.Cleanup(func() { _ = replay.Close() }) + + h := newSchedulerHarness(t, schedulerSpan(t).Start()) + strat := &fillRecordingStrategy{bracketEntryStrategy: bracketEntryStrategy{ + requirements: bothInstrumentsRequirements(t), + instID: eurusdID(t), + side: order.Sell, + stopPrice: "1.10065", + }} + deps := newSchedulerDeps(t, replay, strat, h) + + sched, err := backtest.NewScheduler(deps) + require.NoError(t, err) + require.NoError(t, sched.Run(context.Background())) + + bar2 := time.Date(2024, time.January, 8, 1, 0, 0, 0, time.UTC) + eurusd := eurusdID(t) + + var fillIdxs []int + onBarIdx := -1 + for i, ev := range strat.log { + if !ev.timestamp.Equal(bar2) || !ev.instrument.Equal(eurusd) { + continue + } + switch ev.kind { + case "fill": + fillIdxs = append(fillIdxs, i) + case "onbar": + if onBarIdx == -1 { + onBarIdx = i + } + } + } + require.Len(t, fillIdxs, 2, "both the entry fill and the stop-triggered closing fill must have been delivered via OnFill") + require.NotEqual(t, -1, onBarIdx, "OnBar must still have been called for EUR/USD's own bar 2") + for _, fi := range fillIdxs { + assert.Less(t, fi, onBarIdx, "every fill for a batch must be delivered before that batch's own OnBar call") + } + + // The two fills themselves are delivered in delivery order: entry + // (Sell) before the stop-triggered closing fill (Buy) — matching + // sched.Fills()' own already-asserted order in the sibling test. + assert.Equal(t, order.Sell, strat.log[fillIdxs[0]].side) + assert.Equal(t, order.Buy, strat.log[fillIdxs[1]].side) +} + // TestScheduler_BracketEntryRejectedDoesNotAbortRun is the harmless // half of PR #367 review's blocker 2: when the *entry* leg itself is // rejected, nothing was ever opened — the identical harmless case an diff --git a/docs/arch/adr-060-fill-handler-capability.org b/docs/arch/adr-060-fill-handler-capability.org new file mode 100644 index 0000000..0c16ca2 --- /dev/null +++ b/docs/arch/adr-060-fill-handler-capability.org @@ -0,0 +1,223 @@ +#+title: ADR-060: strategy.FillHandler — an authoritative per-bar fill-delivery capability + +* Status + +Accepted + +* Context + +Issue #368 (PR #369) wired =strategy/smatrend='s ="probation-trend"= +=ExitRule= to =order.IntentEnterWithStop= (ADR-059), so the entry-fill +bar itself gets a real resting protective stop instead of only starting +protection one bar late. That surfaced a genuine lifecycle gap PR #369 +review worked through across several rounds: a bracket entry's own +attached stop can trigger within the *same bar* the entry itself +fills, so =Strategy.OnBar= never observes =order.Long= for that +episode at all — =sideLastBar= stays =order.Flat= across both the bar +before and the bar of the episode. Without an extra signal, +=onExit= never runs: =everExited= stays incorrectly =false=, and the +next entry is governed by =InitialEntryRule= instead of the configured +=ReEntryRule=. + +PR #369 tried two successive account/bar-state heuristics to detect +this without any new capability: + +1. A change in =view.Account().RealizedPnL()= bar over bar — catches + every ordinary intrabar stop touch (always a strictly negative + realized PnL), but misses... +2. ...a gap-through-Open fill (ADR-026's own gap rule: entry and stop + both fill at the identical Open price), which realizes *exactly + zero* PnL. Added a second check, =event.Bar.Open= compared directly + against the bracket's own stop price, to catch this too. + +Those two checks together are exhaustive for every *filled* bracket +outcome — but neither, nor any combination of account/bar-snapshot +state, can distinguish a genuine zero-PnL round trip from a bracket +entry that was *rejected outright* (no fill at all) on a bar whose +Open independently happens to sit at or below wherever the stop would +have been. That coincidence is real (a risk rejection is decided +independently of that same bar's own price action) and, if it occurs, +the heuristic fabricates an =onExit= call — and therefore +=everExited=true=/=ReEntryRule.OnExit= — for an episode that never +happened. + +Rusty's review of PR #369 (2026-09-11) declined to accept a third +heuristic layered on the first two, and asked for a tightly-scoped +prerequisite issue instead: give =Strategy= an authoritative signal +that a fill actually happened, rather than continuing to infer it. +Issue #370 sketched three options (wire the already-documented-but- +never-implemented =strategy.FillHandler= capability; extend +=strategy.View= with a fill accessor; a narrower smatrend-only signal) +and asked for a short design pass before implementation, per this +repository's own established "stop and flag genuinely unresolved +architecture" convention (=CONTRIBUTING.org=) for a change to the +public =strategy= package. + +* Decision + +** =strategy.FillEvent= / =strategy.FillHandler= + +#+BEGIN_SRC go +package strategy + +import ( + "context" + + "github.com/rustyeddy/trader/order" +) + +type FillEvent struct { + Fill order.Fill +} + +type FillHandler interface { + OnFill(ctx context.Context, event FillEvent, view View) error +} +#+END_SRC + +=FillHandler= is an *optional* =Strategy= capability — the identical +"small required core plus capability discovery" pattern +=strategy.History= already established (issue #214) — not a widening +of the required =Strategy= interface. Most strategies never need +per-fill visibility; =View.Account()='s own =Positions()= snapshot is +normally enough. + +=FillEvent= wraps =order.Fill= directly rather than defining a parallel +set of fields. =order.Fill= already carries =OrderID=, =Listing=, +=Side=, =Price=, =Quantity=, and =Metadata= (=CorrelationID=/ +=CausationID=, propagated unchanged from the originating +=order.Intent= through Proposal/Decision/Request/Order per ADR-005's +own correlation convention) — enough identity/causality for a strategy +to tell what filled: an instrument it does not own +(=Listing.InstrumentID()=), an entry versus a protective exit for a +long-only strategy (=Side=), or a specific submission +(=Metadata.CorrelationID=), without =FillEvent= inventing a second +vocabulary for information =order.Fill= already carries. + +** =OnFill= returns only =error=, not =[]order.Intent= + +The architecture document's own original =FillHandler= sketch returned +=[]Intent=, matching =OnBar='s own shape. This decision deliberately +narrows that for v0: emitting intents directly from a fill callback +needs its own design — ordering and recursion within a batch, +next-bar-open eligibility, warm-up interaction, and eventual live/paper +semantics all matter, and no current strategy needs it. Publishing a +wider signature and then hard-rejecting every non-empty result would +freeze half-designed behavior into a public contract for no present +benefit. See Alternatives Considered. + +** Delivery scope: every fill for the account, not requirement-filtered + +=Scheduler= delivers *every* fill on the strategy's own account, +letting the strategy filter by instrument/side itself — mirroring the +existing =View.Account().Positions()= convention every =OnBar= +implementation already uses (for example +=strategy/smatrend='s own =currentPositionSide=). +=Strategy.Describe().Requirements= is deliberately *not* used to +pre-filter fill delivery: requirements describe market-data needs, not +execution ownership, and conflating the two contracts would silently +hide a fill for an instrument outside the strategy's declared +requirements — a case that should not arise given one-provider-per- +account (ADR-016) today, but is not this capability's job to assume +away by filtering. + +** Backtest delivery ordering + +=backtest.Scheduler.runBatch= already accumulates every fill produced +during a batch — from Phase 2's flush (each =submit= call drains/ +journals as it goes) and Phase 3's =IntrabarAdvancer= triggers alike — +into =s.fills= *before* the batch's frozen =account.Snapshot=/=View= +is constructed and the =OnBar= loop runs. Wiring =FillHandler= is +therefore small: capture =len(s.fills)= at the top of =runBatch=, and +once the frozen =view= is built, deliver every fill added since that +point — one =OnFill= call per fill, in delivery order — to a +=FillHandler=-implementing =Strategy=, immediately before the =OnBar= +loop. =OnFill= and every =OnBar= call in the same batch therefore +reason from one identical frozen account snapshot: authoritative +execution history first, then the strategy's own bar-close decision, +never the reverse. + +A =FillHandler.OnFill= error aborts =Run= exactly like any other +=Scheduler=-stage failure — it is not treated as a recoverable, skip- +and-continue condition, matching every other =Scheduler= failure mode +(=submit=, =journalRecord=, =drainAndJournal=). + +** =strategy/smatrend= rewire + +=Strategy= implements =OnFill=: a fill whose =Listing.InstrumentID()= +matches this strategy's own instrument and whose =Side= is =Buy= sets +=entryFilledThisBatch=; a matching =Sell= sets =stopFilledThisBatch=. +Both are named for what they mean to smatrend specifically (per PR +#369/#370 review — avoid a generic "some fill happened" boolean), even +though =OnBar='s own detection today only needs +=entryFilledThisBatch=: if the bracket's entry genuinely filled this +batch (=entryFilledThisBatch= true) and the position is observed +=order.Flat= again at the end of the same batch, its own attached +protective stop is *structurally* the only thing that could have +closed it — nothing else can act on an open position before =OnBar= +itself ever runs. This is authoritative, not a heuristic: +=lastRealizedPnL=, =realizedPnLChanged=, and the +=event.Bar.Open=-vs-stop gap check PR #369 added are deleted entirely, +not retained as a fallback. + +* Consequences + +- =strategy.Strategy= gains its first optional capability beyond + =strategy.History= (issue #214) — =strategy.doc.go='s own "not + published here yet" list for =TickHandler=/=FillHandler=/ + =AccountEventHandler=/=StateManager= loses one entry, on the same + "concrete consumer" basis =History= itself was added under. +- =strategy/smatrend='s same-bar bracket round-trip detection is now + provably correct rather than heuristic: a rejected/unfilled bracket + entry can no longer fabricate an =onExit= call, regardless of what + that bar's own Open happens to be relative to the hypothetical stop + — the exact residual gap PR #369 review would not accept a third + heuristic to paper over. +- Any future backtest-runtime capability (paper/live sessions, + eventually) implementing =strategy.Strategy='s own composition must + also drive =FillHandler= with the identical ordering guarantee + (fills before =OnBar=, against one consistent snapshot) if it wants + to support a =FillHandler=-implementing strategy at all; the live + path itself is out of scope for this ADR (see Alternatives + Considered) but the *contract* — not merely =backtest.Scheduler='s + own implementation of it — is what a live composition would need to + honor. +- A =FillHandler= that returns a non-empty intent list has no path to + do so today: the type signature itself makes this impossible rather + than requiring a runtime check. + +* Alternatives Considered + +- *Let =OnFill= return =[]order.Intent= now, matching =OnBar='s own + shape, and reject non-empty results at the =Scheduler= level.* + Rejected: publishing intent-return semantics only to hard-error on + every actual use freezes an API surface around behavior nobody has + designed yet. The narrower =error=-only signature can be widened + additively later (a new optional capability, or a signature change + before any external consumer depends on it) once a real + fill-triggered-intent use case exists to design against — the + identical "small required core plus capability discovery" reasoning + =strategy.History= itself was built under. +- *Extend =strategy.View= with a fill accessor* (for example + =Fills() []order.Fill=) instead of a push-style capability. Rejected + in favor of the push model: a pull-style accessor would need its own + answer for "fills since when" (a cursor/cutoff concept =View= does + not otherwise have — =History='s own cutoff is about bar visibility, + not fill delivery), while =FillHandler= reuses the same "called once + per relevant occurrence, before =OnBar=, against a frozen View" + shape =OnBar= itself already has, requiring no new View-side state. +- *A narrower, smatrend-only signal* (for example a =Scheduler=-side + callback specific to bracket intents) rather than a general + =Strategy=-level capability. Rejected: the underlying problem + (Strategy cannot always tell whether a fill occurred from + account/bar state alone) is not specific to brackets or to + smatrend — a general, reusable capability is the smaller total + surface area, and issue #370 was explicitly scoped as a + =strategy=-level prerequisite for this reason. +- *Design and wire live/paper delivery in this same change.* Rejected + as premature: no live/paper session composition exists yet in this + codebase to validate the ordering guarantee against; the contract is + written generally enough not to assume backtest specifically, but + the live wiring itself is deferred until a live composition exists to + need it — the same scoping precedent ADR-059 set for live/async + broker semantics (issue #366). diff --git a/docs/arch/adr-decisions.org b/docs/arch/adr-decisions.org index f002efa..b71f969 100644 --- a/docs/arch/adr-decisions.org +++ b/docs/arch/adr-decisions.org @@ -167,6 +167,7 @@ the original decided, use full supersession instead. | 057 | Rounding every simulated fill price to the listing's tick size (in adr-057-sim-fill-price-tick-rounding.org) | Accepted | | 058 | A chart research package for deterministic static backtest visualization (in adr-058-chart-research-package.org) | Accepted | | 059 | Atomic entry-with-protective-stop submission (order.IntentEnterWithStop) (in adr-059-bracket-entry-with-stop.org) | Accepted | +| 060 | strategy.FillHandler — an authoritative per-bar fill-delivery capability (in adr-060-fill-handler-capability.org) | Accepted | Note: ADR-005 and ADR-006 are likewise maintained in their own files rather than inline in this registry — diff --git a/strategy/doc.go b/strategy/doc.go index d98fd6f..6d68dac 100644 --- a/strategy/doc.go +++ b/strategy/doc.go @@ -6,13 +6,17 @@ // # Scope // // Strategy is deliberately small — Describe, Start, and OnBar — per -// the M5-02 design review: TickHandler, FillHandler, -// AccountEventHandler, StateManager, and DataRequirement.NeedTicks are -// not published here. Each is additive, optional-capability surface -// area for a later issue with a concrete consumer to shape it against, -// not something to speculate into place now (the architecture -// document's own "small required core plus capability discovery" -// guidance). +// the M5-02 design review: TickHandler, AccountEventHandler, +// StateManager, and DataRequirement.NeedTicks are not published here. +// Each is additive, optional-capability surface area for a later issue +// with a concrete consumer to shape it against, not something to +// speculate into place now (the architecture document's own "small +// required core plus capability discovery" guidance). FillHandler is +// the first of that originally-deferred list to actually get one +// (ADR-060, issue #370): a concrete consumer (strategy/smatrend's own +// same-bar bracket round-trip detection, issue #368) needed +// authoritative fill visibility that no combination of View/History +// state could provide. // // View's required surface is similarly minimal: Account() // account.Snapshot is the one read every View exposes. Historical-bar diff --git a/strategy/fill.go b/strategy/fill.go new file mode 100644 index 0000000..0c26b48 --- /dev/null +++ b/strategy/fill.go @@ -0,0 +1,42 @@ +package strategy + +import ( + "context" + + "github.com/rustyeddy/trader/order" +) + +// FillEvent is one order.Fill delivered to a strategy implementing +// FillHandler. order.Fill already carries OrderID, Listing, Side, +// Price, Quantity, and Metadata (CorrelationID/CausationID), which is +// enough identity/causality for a strategy to tell what filled — an +// instrument it does not own (Listing.InstrumentID()), an entry versus +// a protective exit for a long-only strategy (Side), or a specific +// bracket attempt (Metadata.CorrelationID, propagated unchanged from +// the originating order.Intent through Proposal/Decision/Request/Order +// per ADR-005's own correlation convention) — so FillEvent wraps it +// directly rather than duplicating or renaming any of its fields +// (ADR-060). +type FillEvent struct { + Fill order.Fill +} + +// FillHandler is an optional Strategy capability (ADR-060): a strategy +// implementing it receives every order.Fill for its own account, in +// delivery order, before the OnBar call for whichever bar the fill is +// attributed to. Most strategies do not need this — View.Account()'s +// own Positions() snapshot is normally enough — so this stays an +// optional capability, matching History's own "small required core +// plus capability discovery" pattern rather than widening the +// required Strategy interface. +// +// OnFill returns only error, deliberately not []order.Intent: emitting +// intents directly from a fill callback needs its own design (ordering +// and recursion within a batch, next-bar-open eligibility, warm-up +// interaction, and eventual live/paper semantics all matter) that no +// current strategy needs yet. See ADR-060's own Alternatives +// Considered for why this was decided now rather than deferred as a +// TODO on a wider signature. +type FillHandler interface { + OnFill(ctx context.Context, event FillEvent, view View) error +} diff --git a/strategy/smatrend/doc.go b/strategy/smatrend/doc.go index b349682..85bec9e 100644 --- a/strategy/smatrend/doc.go +++ b/strategy/smatrend/doc.go @@ -98,6 +98,16 @@ // a spike observed during PROBATION still governs the TRENDING stop // after activation. // +// "probation-trend" additionally implements the optional +// InitialStopProvider ExitRule capability (issue #368, see +// exitrule.go): its initial probation stop is a pure function of the +// SMA value alone, so it is computable *before* the entry fill, and +// Strategy uses it to submit a bracket order.IntentEnterWithStop +// (ADR-059) instead of a plain order.IntentEnter — the position is +// protected starting the entry fill bar itself, not one bar later. +// "trailing-stop" and "sma-cross" never implement this capability and +// keep using a plain entry, unchanged. +// // New rules are a new small type plus one registry entry in // exitrule.go/reentryrule.go/initialentryrule.go — never a change to // Strategy's own control flow. See those files for the exact diff --git a/strategy/smatrend/exitrule.go b/strategy/smatrend/exitrule.go index 51910c7..88f7850 100644 --- a/strategy/smatrend/exitrule.go +++ b/strategy/smatrend/exitrule.go @@ -56,6 +56,47 @@ type ExitRule interface { OnLongBar(bar marketdata.Bar, smaValue float64) (ExitDecision, error) } +// InitialStopProvider is an optional ExitRule capability (issue #368), +// mirroring phaseReporter's own type-assertion pattern in strategy.go: +// an ExitRule implements it only when its intended first protective +// stop is computable *before* the entry fill, from information +// available at the entry-decision bar. smaValue is that bar's own +// current SMA value — the only signal Strategy already has in hand at +// decision time — so a rule computes InitialStop from exactly the +// same inputs it would otherwise wait one bar to use. Strategy uses +// this to emit order.IntentEnterWithStop (ADR-059) instead of a plain +// order.IntentEnter wherever it is available, closing the entry-fill- +// bar protection gap for whichever ExitRule can offer one. +// +// trailingStopExitRule and smaCrossExitRule never implement this: +// trailingStopExitRule's first stop is defined in terms of the entry/ +// fill bar's own High, not known before the fill; smaCrossExitRule +// manages no resting stop at all. Only probationTrendExitRule +// implements it today, since its probation stop is already, by +// design, a pure function of the SMA value alone (Config. +// InitialStopBelowSMA) — never the fill price. +type InitialStopProvider interface { + InitialStop(smaValue float64) (num.Price, error) +} + +// InitialStopSeeder is a second, separate optional ExitRule capability +// (PR #369 review): an ExitRule that implements InitialStopProvider +// may additionally implement this to receive the stop Strategy +// actually placed via the resulting bracket entry, so it can seed its +// own ratchet-floor state from that exact value instead of whatever +// default OnEntry itself establishes — without widening OnEntry's own +// required, exported signature. ExitRule is a public interface any +// external/custom implementation may satisfy; adding a parameter to +// OnEntry would have broken every one of them for a +// probation-trend-only enhancement. Strategy calls SeedInitialStop +// immediately after OnEntry, and only when Strategy actually used a +// bracket entry for this position — never for a plain entry, and +// never before OnEntry has already run its own (now-superseded) +// default initialization. +type InitialStopSeeder interface { + SeedInitialStop(stop num.Price) +} + // exitRuleRegistry maps a Config.ExitRuleName to its constructor. A // new ExitRule is a new small type plus one entry here — never a // change to Strategy's own control flow (issue #347). @@ -154,19 +195,26 @@ func (smaCrossExitRule) OnLongBar(bar marketdata.Bar, smaValue float64) (ExitDec // high-water-mark formula would imply a lower stop than that, nothing // is emitted until it genuinely ratchets past it — see onProbationBar. // -// Known limitation (PR #350 review): the entry fill bar itself is not -// protected by this rule's own intended probation stop at all. That -// stop is only computed and emitted as an AdjustStop intent on the -// bar OnBar first observes the fresh Long position, but by then -// backtest.Scheduler's own broker-side resting-order machinery has -// already resolved that same bar's own intrabar price action (see -// Strategy.OnBar's own doc comment) — the emitted stop only becomes a -// checked resting order starting the *following* bar. Solving this -// needs an execution/pipeline capability (an entry submitted -// atomically with its own protective stop) this codebase does not -// have yet; see regression_test.go's own -// TestSMATrend_ProbationEntryBarIntrabarGapIsAKnownLimitation for a -// regression-locked proof against a real fixture. +// Formerly known limitation (PR #350 review; closed by issue #368): +// the entry fill bar itself used to be unprotected by this rule's own +// intended probation stop, since that stop was only computed and +// emitted as an AdjustStop intent on the bar OnBar first observed the +// fresh Long position — one bar after backtest.Scheduler's own +// broker-side resting-order machinery had already resolved that same +// bar's own intrabar price action (see Strategy.OnBar's own doc +// comment). This rule now implements InitialStopProvider: its initial +// probation stop is computable from the SMA value at the entry +// *decision* bar, before the fill, so Strategy emits +// order.IntentEnterWithStop (ADR-059) instead of a plain +// order.IntentEnter, and the stop is already resting by the time the +// fill bar's own intrabar action is checked. trailingStopExitRule and +// smaCrossExitRule still have the gap in principle (their first stop +// genuinely depends on the fill/entry bar's own High, or manages no +// resting stop at all), but neither is the playbook's own +// probation/tight-stop-on-entry use case this issue exists for. See +// regression_test.go's own +// TestSMATrend_ProbationEntryBarBreachClosesSameBar for the +// regression proving the fix. // // Phase reports this rule's own current lifecycle state; Strategy // mirrors it via Strategy.Phase (see phase.go) so it is directly @@ -208,6 +256,30 @@ func (r *probationTrendExitRule) OnEntry(entryBar marketdata.Bar, entryPrice num r.trendStop = nil } +// InitialStop implements InitialStopProvider: the same probation-stop +// formula onProbationBar uses, computed from smaValue alone so it is +// knowable before the entry fill (issue #368) — never from the fill +// bar's own High/Low/Close, which is not yet known when Strategy asks +// for this value at the entry-decision bar. +func (r *probationTrendExitRule) InitialStop(smaValue float64) (num.Price, error) { + stop, err := probationStopFromSMA(smaValue, r.belowSMA) + if err != nil { + return num.Price{}, fmt.Errorf("smatrend: computing initial probation stop: %w", err) + } + return stop, nil +} + +// SeedInitialStop implements InitialStopSeeder (PR #369 review): +// called by Strategy immediately after OnEntry, only when a bracket +// entry was actually used, this overrides the nil OnEntry just +// established with the stop Strategy actually placed — the floor +// onProbationBar's own ratchet-only comparison +// (stop.Cmp(*r.probationStop) > 0) must never emit a decision that +// would loosen protection below. +func (r *probationTrendExitRule) SeedInitialStop(stop num.Price) { + r.probationStop = &stop +} + func (r *probationTrendExitRule) OnLongBar(bar marketdata.Bar, smaValue float64) (ExitDecision, error) { // The high-water mark is tracked every bar regardless of phase: // the playbook's own trailing stop, once activated, is based on diff --git a/strategy/smatrend/regression_test.go b/strategy/smatrend/regression_test.go index 9f5543d..95b2a0b 100644 --- a/strategy/smatrend/regression_test.go +++ b/strategy/smatrend/regression_test.go @@ -229,11 +229,21 @@ func runSMATrendFixture(t *testing.T) (svcbacktest.RunResponse, *memoryRecorder) // the exact same real M4/M5 composition path rather than a second, // parallel fixture. func runSMATrendFixtureWithConfig(t *testing.T, cfg smatrend.Config) (svcbacktest.RunResponse, *memoryRecorder) { + t.Helper() + return runSMATrendFixtureForSpan(t, cfg, smatrendFixtureSpan(t)) +} + +// runSMATrendFixtureForSpan generalizes runSMATrendFixtureWithConfig +// over the query span (issue #368), so a second, dedicated fixture +// under testdata/raw/oanda/EURUSD's own other months can be exercised +// through the identical real M4/M5 composition path without touching +// the original 11-bar January fixture every other regression in this +// file depends on. +func runSMATrendFixtureForSpan(t *testing.T, cfg smatrend.Config, span marketdata.TimeRange) (svcbacktest.RunResponse, *memoryRecorder) { t.Helper() resolver := instrument.NewMemoryResolver() require.NoError(t, resolver.Register(eurusdListing(t, "oanda"))) - span := smatrendFixtureSpan(t) c := clock.NewSimulated(span.Start()) manager, err := marketdata.New(marketdata.Config{ @@ -407,53 +417,111 @@ func TestSMATrend_BreakoutReEntryChangesRealOutcome(t *testing.T) { assert.Empty(t, resp.Account.Positions(), "the account must remain flat for the rest of the fixture") } -// TestSMATrend_ProbationEntryBarIntrabarGapIsAKnownLimitation documents -// a known limitation of the "probation-trend" ExitRule raised in PR -// #350 review, using the exact same real EURUSD bars -// TestSMATrend_EndToEndRegression exercises: bar 5 (2024-01-12, the -// entry fill bar) has Low 1.10000 and SMA(bar3,4,5)=1.11667, so the -// intended probation stop — 1.11667 * (1-0.01) = 1.1055 — is breached -// intrabar on this very bar, yet its Close (1.15000) recovers back -// above the SMA. +// smatrendProbationEntryBarFixtureSpan covers strategy/smatrend/ +// testdata's own dedicated 9-bar D1 EURUSD February fixture (issue +// #368), a separate month from smatrendFixtureSpan's own January +// fixture so this test's own engineered price path — designed +// specifically to breach the "probation-trend" bracket's own initial +// stop intrabar, on the entry fill bar itself, for both the initial +// entry and a re-entry — never has to share numbers with (or risk +// disturbing) the assertions every other regression in this file +// makes against the January data. +func smatrendProbationEntryBarFixtureSpan(t *testing.T) marketdata.TimeRange { + t.Helper() + span, err := marketdata.NewTimeRange( + time.Date(2024, time.February, 1, 0, 0, 0, 0, time.UTC), + time.Date(2024, time.February, 14, 0, 0, 0, 0, time.UTC), + ) + require.NoError(t, err) + return span +} + +// TestSMATrend_ProbationEntryBarBreachClosesSameBar is issue #368's +// own required regression, replacing +// TestSMATrend_ProbationEntryBarIntrabarGapIsAKnownLimitation (PR +// #350 review's documented gap): "probation-trend" now implements +// InitialStopProvider, so Strategy emits a bracket +// order.IntentEnterWithStop (ADR-059) instead of a plain +// order.IntentEnter, and the intended probation stop is a real +// resting order *before* the entry fill bar's own intrabar price +// action is checked, not one bar later. // -// The playbook's own intent is that this should stop the position out -// intrabar. It does not, for a real sequencing reason rather than a -// bug in the stop math itself: Strategy only ever observes a fresh -// Long position — and therefore only ever computes and emits its -// first protective stop — on the bar *after* the entry decision (the -// fill bar itself), but backtest.Scheduler's own broker-side -// resting-order machinery has already resolved that fill bar's own -// intrabar price action *before* OnBar ever runs for it (see -// Strategy.OnBar's own doc comment). The AdjustStop intent bar 5's own -// OnBar call emits only becomes a resting order checked against -// intrabar price action starting bar 6 onward. Every other ExitRule -// in this package has the identical gap; "probation-trend" simply -// makes it matter most, since its whole purpose is a *tight* stop -// immediately after entry. +// On this fixture (see the February CSV's own values): // -// Solving this needs an execution/pipeline capability this codebase -// does not have yet — submitting a protective stop atomically with -// the entry order itself, rather than one bar later — which is out of -// scope for issue #349's current implementation pass. This test -// exists so the gap is proven and regression-locked rather than -// silently assumed away: if a future change to Strategy or the -// pipeline closes this gap, this test's own assertion (that the -// position survives bar 5 unprotected) will fail and must be updated -// deliberately, not accidentally. -func TestSMATrend_ProbationEntryBarIntrabarGapIsAKnownLimitation(t *testing.T) { - resp, _ := runSMATrendFixtureWithConfig(t, smatrend.Config{ +// - bar 4 (2024-02-06) is the entry decision bar: +// sma(bar2,3,4)=(1.10+1.09+1.20)/3=1.13, close (1.20) crosses +// above it. The bracket's own initial stop, computed from this +// bar's SMA alone — never bar 5's own Close/High/Low, which is +// not yet known — is 1.13*(1-0.01)=1.1187. +// - bar 5 (2024-02-07) is the entry fill bar: fills at this bar's +// own Open (1.19), and its Low (1.05) breaches the 1.1187 +// bracket stop intrabar (an ordinary touch, not a gap: Open is +// above the stop). The position closes on this same bar, at the +// stop price itself (ADR-026) — it does not survive to bar 6. +// - bars 6-7 stay below the SMA: no re-entry. +// - bar 8 (2024-02-12) is a fresh cross-above re-entry decision: +// sma(bar6,7,8)=(1.05+1.00+1.30)/3=1.11667, initial stop= +// 1.11667*0.99=1.1055 — proving the re-entry gets the identical +// immediate protection the initial entry did. +// - bar 9 (2024-02-13) is the re-entry's own fill bar: fills at +// Open (1.29), and its Low (1.05) breaches the 1.1055 bracket +// stop intrabar, again closing on this same bar. +func TestSMATrend_ProbationEntryBarBreachClosesSameBar(t *testing.T) { + resp, rec := runSMATrendFixtureForSpan(t, smatrend.Config{ SMAPeriod: 3, ExitRuleName: "probation-trend", ReEntryRuleName: "fresh-cross", InitialStopBelowSMA: num.MustParseRate("0.01"), TrailActivationGain: num.MustParseRate("0.05"), TrailingStopPercent: num.MustParseRate("0.10"), - }) + }, smatrendProbationEntryBarFixtureSpan(t)) + + // The actual intent kind Strategy emitted for both entries — never + // a plain IntentEnter — is the real proof the bracket path was + // taken, not merely that a trade eventually closed. + var enterWithStopCount int + for _, rec := range rec.kinds(journal.KindIntent) { + if rec.Intent.Kind == order.IntentEnterWithStop { + enterWithStopCount++ + } + } + assert.Equal(t, 2, enterWithStopCount, "both the initial entry and the re-entry must be bracket entries") + + bar5Fill := time.Date(2024, time.February, 7, 22, 0, 0, 0, time.UTC) + bar9Fill := time.Date(2024, time.February, 13, 22, 0, 0, 0, time.UTC) + + require.Len(t, resp.Trades, 2, "both episodes must have closed as realized trades, each on its own fill bar") - bar5Fill := time.Date(2024, time.January, 12, 22, 0, 0, 0, time.UTC) - require.NotEmpty(t, resp.Trades, "the position must have closed eventually for this run to produce a trade at all") first := resp.Trades[0] assert.True(t, first.OpenedAt.Equal(bar5Fill), "got %s", first.OpenedAt) - assert.True(t, first.ClosedAt.After(bar5Fill), - "known limitation: the position must survive bar 5's own intrabar Low (1.10000), which breaches the 1.1055 probation stop that same bar's SMA implies — the resting stop is not active until the following bar") + assert.True(t, first.ClosedAt.Equal(bar5Fill), + "the position must not survive past its own entry fill bar: the bracket's initial stop is already resting when bar 5's own intrabar Low (1.05) breaches it", "got %s", first.ClosedAt) + + second := resp.Trades[1] + assert.True(t, second.OpenedAt.Equal(bar9Fill), "got %s", second.OpenedAt) + assert.True(t, second.ClosedAt.Equal(bar9Fill), + "the re-entry must receive the identical immediate protection: bar 9's own intrabar Low (1.05) breaches its own bracket stop on the same bar it fills", "got %s", second.ClosedAt) + + assert.Empty(t, resp.OpenTrades, "both episodes closed intrabar on their own fill bar; nothing should remain open") + assert.Empty(t, resp.Account.Positions()) +} + +// TestSMATrend_PlainEntryStillWorksWithoutInitialStopProvider proves +// an ExitRule that does not implement InitialStopProvider (issue +// #368) is entirely unaffected: Strategy must keep emitting a plain +// order.IntentEnter for it, exactly as before this issue, on the same +// fixture and entry bar the previous test's bracket path uses. +func TestSMATrend_PlainEntryStillWorksWithoutInitialStopProvider(t *testing.T) { + resp, rec := runSMATrendFixtureForSpan(t, smatrend.Config{ + SMAPeriod: 3, + TrailingStopPercent: num.MustParseRate("0.10"), + }, smatrendProbationEntryBarFixtureSpan(t)) + + intents := rec.kinds(journal.KindIntent) + require.NotEmpty(t, intents) + for _, rec := range intents { + assert.NotEqual(t, order.IntentEnterWithStop, rec.Intent.Kind, "the default trailing-stop exit rule never implements InitialStopProvider") + } + + require.NotEmpty(t, resp.Trades, "the default trailing-stop rule must still trade this fixture normally") } diff --git a/strategy/smatrend/strategy.go b/strategy/smatrend/strategy.go index 41e8af5..a4e624d 100644 --- a/strategy/smatrend/strategy.go +++ b/strategy/smatrend/strategy.go @@ -20,7 +20,16 @@ import ( const Name = "sma-trend" // Version distinguishes revisions of this strategy's own logic. -const Version = "v1" +// +// Bumped to "v2" by issue #368 (PR #369 review, blocker 4): a +// probation-trend entry/re-entry now submits a bracket +// order.IntentEnterWithStop instead of a plain order.IntentEnter, +// changing both entry-bar stop behavior and persisted signal +// semantics (a new "enter-long-with-stop" KindSignal action, see +// recordSignal). Per ADR-044, Descriptor.Version is the discriminator +// for exactly this kind of logic change: a "v1" run and a "v2" run of +// an otherwise-identical config are not directly comparable results. +const Version = "v2" // Strategy is the SMA-trend baseline strategy.Strategy implementation // (issue #335, EQS-01), extended with pluggable exit/re-entry rules @@ -80,6 +89,54 @@ type Strategy struct { // #348 review). nil whenever the most recent exit was a // broker-triggered stop instead, in which case lastStop is used. directExitReference *num.Price + // pendingInitialStop is the stop price used by the most recent + // EnterWithStop intent onFlat emitted, carried forward exactly one + // bar (issue #368) — nil whenever the most recent entry was a + // plain Enter (no InitialStopProvider, or none configured). It is + // consumed on the very next bar OnBar observes, one of three ways: + // + // - Flat->Long transition: the bracket entry filled and survived + // the bar. exitRule.OnEntry runs its own default + // initialization, then — if exitRule also implements + // InitialStopSeeder — SeedInitialStop overrides the one field + // that default reset with this exact value (PR #369 review; + // see InitialStopSeeder's own doc comment for why this is a + // second call rather than a parameter on OnEntry itself). + // - Flat->Flat, with entryFilledThisBatch false: the entry never + // filled at all (rejected outright) — simply discarded, see + // discardUnfilledBracket. + // - Flat->Flat, with entryFilledThisBatch true: the entry filled + // *and* its own attached stop triggered within the same bar, + // invisible to a side-only Flat/Long comparison (PR #369 + // review, blocker 1) — see entryFilledThisBatch's own doc + // comment. + pendingInitialStop *num.Price + // entryFilledThisBatch and stopFilledThisBatch are set by OnFill + // (ADR-060, issue #370) — strategy.FillHandler delivers every fill + // for this strategy's own account, in order, strictly before OnBar + // runs for the same batch. A fill for this Strategy's own + // instrument with Side Buy sets entryFilledThisBatch; + // order.Sell sets stopFilledThisBatch. Both are cleared, + // unconditionally, at the end of every OnBar call (see OnBar's own + // defer) — they describe only "this batch," never carried forward. + // + // OnBar's own Flat branch consumes only entryFilledThisBatch today + // (stopFilledThisBatch is tracked because PR #369/#370 review asked + // for named, distinguishable fill identity rather than one generic + // "some fill happened" boolean, not because current logic needs + // both): if the bracket's entry genuinely filled this batch and the + // position is observed order.Flat again at the end of the same + // batch, its own attached protective stop is *structurally* the + // only thing that could have closed it — nothing else can act on + // an open position before OnBar itself ever runs. This is + // authoritative, not a heuristic (unlike the RealizedPnL/ + // event.Bar.Open checks this field set replaces): a bracket entry + // that was rejected outright never produces a fill at all, so + // entryFilledThisBatch correctly stays false regardless of what + // that bar's own Open happens to be relative to the hypothetical + // stop — the exact residual gap those heuristics could not close. + entryFilledThisBatch bool + stopFilledThisBatch bool intents strategy.IntentFactory journal journal.Recorder // nil unless env.Journal was set @@ -190,6 +247,17 @@ func (s *Strategy) Start(ctx context.Context, env strategy.Environment) error { // never compares this bar's own price action against a stop level // computed from this same bar. func (s *Strategy) OnBar(ctx context.Context, event strategy.BarEvent, view strategy.View) ([]order.Intent, error) { + // entryFilledThisBatch/stopFilledThisBatch describe only this + // batch (ADR-060): OnFill, if called at all, already set them for + // this exact batch before this OnBar call runs; clearing them here + // via defer — regardless of return path, including the warm-up + // early return below — keeps them from ever leaking into a later + // batch that produces no fill of its own. + defer func() { + s.entryFilledThisBatch = false + s.stopFilledThisBatch = false + }() + // event.Bar.Close.Float64() is ADR-045's explicit exact-to-analytical // conversion boundary: a direct numeric conversion, never a // String()/strconv.ParseFloat() round-trip. @@ -209,8 +277,25 @@ func (s *Strategy) OnBar(ctx context.Context, event strategy.BarEvent, view stra switch side { case order.Flat: - if s.sideLastBar == order.Long { + switch { + case s.sideLastBar == order.Long: s.onExit(event.Bar) + case s.pendingInitialStop != nil && s.entryFilledThisBatch: + // The bracket entry genuinely filled this batch (OnFill + // already observed a Buy fill for this instrument), and + // the position is Flat again at the end of the same + // batch: its own attached protective stop is + // structurally the only thing that could have closed it + // — nothing else can act on an open position before + // OnBar itself ever runs (ADR-060, issue #368/#370, + // replacing PR #369's own RealizedPnL/event.Bar.Open + // heuristics with this authoritative signal). + s.onExit(event.Bar) + case s.pendingInitialStop != nil: + // entryFilledThisBatch is false: the bracket entry was + // never filled at all (rejected outright) — no episode + // occurred. + s.discardUnfilledBracket() } s.sideLastBar = order.Flat return s.onFlat(ctx, event, crossedAbove, aboveSMA, close, smaValue) @@ -221,6 +306,12 @@ func (s *Strategy) OnBar(ctx context.Context, event strategy.BarEvent, view stra return nil, fmt.Errorf("smatrend: reading entry price: %w", err) } s.exitRule.OnEntry(event.Bar, entryPrice) + if s.pendingInitialStop != nil { + if seeder, ok := s.exitRule.(InitialStopSeeder); ok { + seeder.SeedInitialStop(*s.pendingInitialStop) + } + s.pendingInitialStop = nil + } } s.sideLastBar = order.Long return s.onLong(ctx, event, close, smaValue) @@ -237,6 +328,42 @@ func (s *Strategy) OnBar(ctx context.Context, event strategy.BarEvent, view stra } } +// OnFill implements the optional strategy.FillHandler capability +// (ADR-060, issue #370): backtest.Scheduler (or any other runtime +// implementing the identical contract) delivers every fill for this +// strategy's own account here, in order, strictly before the OnBar +// call for whichever bar the fill belongs to. A fill for an +// instrument other than s.instrumentID is ignored outright — this +// assumes smatrend is the only strategy trading this account (already +// assumed elsewhere, see OnBar's own order.Short case). Side +// distinguishes an entry fill (order.Buy, the only side smatrend ever +// enters with) from a protective-exit fill (order.Sell) — see +// entryFilledThisBatch/stopFilledThisBatch's own doc comment for how +// OnBar consumes this. +func (s *Strategy) OnFill(ctx context.Context, event strategy.FillEvent, view strategy.View) error { + fill := event.Fill + if !fill.Listing.InstrumentID().Equal(s.instrumentID) { + return nil + } + switch fill.Side { + case order.Buy: + s.entryFilledThisBatch = true + case order.Sell: + s.stopFilledThisBatch = true + } + return nil +} + +// discardUnfilledBracket rolls back the speculative state +// buildEntryIntent's bracket path set for an entry attempt that never +// actually filled (issue #368): pendingInitialStop and lastStop, both +// set optimistically before the outcome was known. Called only from +// the Flat branch's own entryFilledThisBatch-false case. +func (s *Strategy) discardUnfilledBracket() { + s.pendingInitialStop = nil + s.lastStop = nil +} + // onExit notifies reEntryRule that a position just closed — whether // via a broker-triggered stop (ADR-026) or a direct // ExitRule.ExitNow — using the best available reference level for @@ -247,7 +374,15 @@ func (s *Strategy) OnBar(ctx context.Context, event strategy.BarEvent, view stra // later bar this exit is observed on), else lastStop when it was a // broker-triggered stop, else — only possible for a custom ExitRule // that manages neither — exitBar's own Close as a last resort. Called -// exactly once per exit, from OnBar's own Flat-transition detection. +// exactly once per exit, from OnBar's own Flat-transition detection — +// either the classic Long->Flat transition, or (issue #368) a bracket +// entry whose own attached stop triggered within the same bar the +// entry itself filled, detected via a RealizedPnL change while +// sideLastBar never observed order.Long at all. lastStop is already +// the correct reference for that second case too: buildEntryIntent's +// bracket path sets it to the bracket's own stop price at the moment +// the entry intent was built, and nothing overwrites it before onExit +// runs, since OnEntry/onLong were never reached. func (s *Strategy) onExit(exitBar marketdata.Bar) { exitPrice := exitBar.Close switch { @@ -260,6 +395,7 @@ func (s *Strategy) onExit(exitBar marketdata.Bar) { s.everExited = true s.lastStop = nil s.directExitReference = nil + s.pendingInitialStop = nil } // onFlat handles a bar observed with no open position. The very first @@ -298,16 +434,72 @@ func (s *Strategy) onFlat(ctx context.Context, event strategy.BarEvent, crossedA return nil, nil } - in, err := s.intents.Enter(s.instrumentID, order.Buy) + in, initialStop, err := s.buildEntryIntent(smaValue) if err != nil { return nil, err } - if err := s.recordSignal(ctx, event, close, smaValue, PhaseFlat, "enter-long", nil, []order.Intent{in}); err != nil { + action := "enter-long" + if initialStop != nil { + action = "enter-long-with-stop" + } + if err := s.recordSignal(ctx, event, close, smaValue, PhaseFlat, action, initialStop, []order.Intent{in}); err != nil { return nil, err } return []order.Intent{in}, nil } +// buildEntryIntent builds this entry's own order.Intent: a bracket +// order.IntentEnterWithStop when exitRule implements +// InitialStopProvider (issue #368, ADR-059) — closing the entry-fill- +// bar protection gap for whichever ExitRule can compute its first +// stop before the fill — or a plain order.IntentEnter otherwise, +// unchanged from before this issue. The returned *num.Price, when +// non-nil, is both the stop actually placed and the value +// recordSignal journals as decision evidence. +// +// The bracket path sets both s.pendingInitialStop (for OnBar's own +// Flat->Long/Flat->Flat consumption, see that field's own doc +// comment) and s.lastStop (PR #369 review, blocker 2): without this, +// a bracket entry that survives its own fill bar with no ratcheting +// AdjustStop on the very next OnLongBar call (its computed stop no +// higher than the seeded floor) would leave lastStop nil despite a +// real resting stop existing, so onExit would later fall back to the +// exit bar's own Close instead of the real intended stop level when +// that resting stop eventually triggers — corrupting exit-price-based +// re-entry rules such as "reclaim-exit-price". Both are speculative +// until the outcome is known: OnBar's own Flat branch rolls them back +// via discardUnfilledBracket if the entry turns out to have never +// filled at all. +// +// smaValue is the entry-decision bar's own current SMA value — the +// only signal available before the fill — so InitialStop is +// necessarily computed from information no later than this bar; it +// never uses the eventual fill bar's own Close/High/Low, which is not +// yet known (issue #368's own explicit no-lookahead requirement). +func (s *Strategy) buildEntryIntent(smaValue float64) (order.Intent, *num.Price, error) { + provider, ok := s.exitRule.(InitialStopProvider) + if !ok { + in, err := s.intents.Enter(s.instrumentID, order.Buy) + if err != nil { + return order.Intent{}, nil, err + } + s.pendingInitialStop = nil + return in, nil, nil + } + + stop, err := provider.InitialStop(smaValue) + if err != nil { + return order.Intent{}, nil, fmt.Errorf("smatrend: computing initial stop for bracket entry: %w", err) + } + in, err := s.intents.EnterWithStop(s.instrumentID, order.Buy, stop) + if err != nil { + return order.Intent{}, nil, err + } + s.pendingInitialStop = &stop + s.lastStop = &stop + return in, &stop, nil +} + // onLong handles a bar observed with an open long position that // survived the bar (see OnBar's own doc comment for why any stop // trigger against this bar's own price action has already resolved by diff --git a/strategy/smatrend/strategy_test.go b/strategy/smatrend/strategy_test.go index 14df58a..2a498b8 100644 --- a/strategy/smatrend/strategy_test.go +++ b/strategy/smatrend/strategy_test.go @@ -53,16 +53,17 @@ func mustListing(t *testing.T) instrument.Listing { return l } -func mustSnapshot(t *testing.T, accountID id.AccountID, position *order.Position) account.Snapshot { +func mustSnapshot(t *testing.T, accountID id.AccountID, position *order.Position, realizedPnL string) account.Snapshot { t.Helper() var positions []order.Position if position != nil { positions = []order.Position{*position} } snap, err := tradertest.NewSnapshot(tradertest.SnapshotParams{ - AccountID: accountID, - Broker: "OANDA", - Positions: positions, + AccountID: accountID, + Broker: "OANDA", + Positions: positions, + RealizedPnL: realizedPnL, }) require.NoError(t, err) return snap @@ -97,6 +98,13 @@ type testHarness struct { // activation threshold, which is computed directly from the real // AvgPrice Strategy reads via currentPositionAvgPrice (issue #349). avgPrice string + // realizedPnL overrides the harness-reported account's own + // RealizedPnL ("0" otherwise). Strategy itself no longer reads + // this for same-bar round-trip detection (ADR-060 replaced that + // heuristic with deliverFill/OnFill); retained only as a general + // account-shape override for a test that needs one for some other + // reason. + realizedPnL string } func newTestHarness(t *testing.T, config Config) *testHarness { @@ -137,6 +145,26 @@ func (h *testHarness) triggerStop() { h.side = order.Flat } +// deliverFill simulates backtest.Scheduler's own FillHandler delivery +// (ADR-060): a minimal, valid order.Fill for h's own listing/account, +// with side, passed to the strategy's OnFill directly — the same call +// Scheduler makes before OnBar for the same batch. Panics on +// construction failure (a harness bug, never a test assertion). +func (h *testHarness) deliverFill(side order.Side) { + h.t.Helper() + fill, err := order.NewFill(order.Fill{ + FillID: tradertest.MustFillID(h.ids), + OrderID: tradertest.MustOrderID(h.ids), + AccountID: h.accountID, + Listing: h.listing, + Side: side, + Price: num.MustParsePrice("1.10000"), + Quantity: num.MustParseQuantity("1"), + }) + require.NoError(h.t, err) + require.NoError(h.t, h.strategy.OnFill(context.Background(), strategy.FillEvent{Fill: fill}, fakeView{snap: mustSnapshot(h.t, h.accountID, nil, h.realizedPnL)})) +} + // buildBar constructs bar barNum's own BarEvent/View pair (bars are // spaced one D1 interval apart from testStart), reflecting the // harness's current position, and advances h's clock to that bar's own @@ -167,7 +195,7 @@ func (h *testHarness) buildBar(barNum int, b bar) (strategy.BarEvent, strategy.V Close: mp(b.close), } event := strategy.BarEvent{Instrument: h.instID, Interval: marketdata.D1, Bar: mdBar} - view := fakeView{snap: mustSnapshot(h.t, h.accountID, position)} + view := fakeView{snap: mustSnapshot(h.t, h.accountID, position, h.realizedPnL)} return event, view, barTime } @@ -188,7 +216,7 @@ func (h *testHarness) onBar(barNum int, b bar) ([]order.Intent, time.Time) { // changes-side vocabulary. for _, in := range intents { switch in.Kind { - case order.IntentEnter: + case order.IntentEnter, order.IntentEnterWithStop: h.side = order.Long case order.IntentExit: h.side = order.Flat @@ -843,16 +871,25 @@ func TestStrategy_ProbationTrendFullLifecyclePhaseTransitions(t *testing.T) { for i, c := range []float64{100, 100, 100, 99} { h.onBar(i+1, bar{open: c, high: c, low: c, close: c}) } + // probation-trend implements InitialStopProvider (issue #368): the + // entry decision itself is now a bracket order.IntentEnterWithStop, + // with its stop computed from this bar's own sma(100,99,102)= + // 100.33333333, before the fill — 100.33333333*0.99 = 99.33. intents, _ := h.onBar(5, bar{open: 102, high: 102, low: 102, close: 102}) require.Len(t, intents, 1) - require.Equal(t, order.IntentEnter, intents[0].Kind) + require.Equal(t, order.IntentEnterWithStop, intents[0].Kind) + require.NotNil(t, intents[0].StopPrice) + assert.Equal(t, "99.33", intents[0].StopPrice.String()) h.side = order.Long h.avgPrice = "102" // the real fill price this episode entered at; activation threshold = 102 * 1.05 = 107.1 assert.Equal(t, PhaseFlat, h.strategy.Phase(), "OnEntry has not yet been observed — this was only the entry-decision bar") - // Bar 6: first bar observed Long — OnEntry seeds Probation. - // sma(99,102,102)=101, stop=101*0.99=99.99. + // Bar 6: first bar observed Long — OnEntry seeds Probation, this + // time from the 99.33 bracket stop already resting (issue #368) + // rather than nil. sma(99,102,102)=101, raw stop=101*0.99=99.99 — + // still above that 99.33 floor, so the ratchet fires exactly as + // it always did. intents, _ = h.onBar(6, bar{open: 103, high: 105, low: 102, close: 102}) require.Len(t, intents, 1) require.Equal(t, order.IntentAdjustStop, intents[0].Kind) @@ -911,10 +948,15 @@ func TestStrategy_ProbationTrendFullLifecyclePhaseTransitions(t *testing.T) { assert.Empty(t, intents) assert.Equal(t, PhaseFlat, h.strategy.Phase()) - // A genuine fresh cross back above the SMA re-enters. + // A genuine fresh cross back above the SMA re-enters — again a + // bracket order.IntentEnterWithStop (issue #368), proving re-entry + // gets the identical immediate protection as the initial entry: + // sma(55,40,70)=55, stop=55*0.99=54.45. intents, _ = h.onBar(12, bar{open: 45, high: 72, low: 44, close: 70}) require.Len(t, intents, 1) - require.Equal(t, order.IntentEnter, intents[0].Kind) + require.Equal(t, order.IntentEnterWithStop, intents[0].Kind) + require.NotNil(t, intents[0].StopPrice) + assert.Equal(t, "54.45", intents[0].StopPrice.String()) h.side = order.Long h.avgPrice = "70" assert.Equal(t, PhaseFlat, h.strategy.Phase(), "still only the entry-decision bar") @@ -953,13 +995,18 @@ func TestStrategy_AboveSMAReEntryFiresOnTheVeryNextEligibleFlatBar(t *testing.T) for i, c := range []float64{100, 100, 100, 99} { h.onBar(i+1, bar{open: c, high: c, low: c, close: c}) } + // probation-trend implements InitialStopProvider (issue #368): + // sma(100,99,102)=100.33333333, stop=100.33333333*0.99=99.33. intents, _ := h.onBar(5, bar{open: 102, high: 102, low: 102, close: 102}) require.Len(t, intents, 1) - require.Equal(t, order.IntentEnter, intents[0].Kind) + require.Equal(t, order.IntentEnterWithStop, intents[0].Kind) + require.NotNil(t, intents[0].StopPrice) + assert.Equal(t, "99.33", intents[0].StopPrice.String()) h.side = order.Long h.avgPrice = "102" - // Bar 6: first Long bar, Probation stop placed at 99.99. + // Bar 6: first Long bar, Probation stop placed at 99.99 — above + // the 99.33 floor bracketed at entry, so the ratchet still fires. intents, _ = h.onBar(6, bar{open: 103, high: 105, low: 102, close: 102}) require.Len(t, intents, 1) assert.Equal(t, "99.99", intents[0].StopPrice.String()) @@ -970,9 +1017,14 @@ func TestStrategy_AboveSMAReEntryFiresOnTheVeryNextEligibleFlatBar(t *testing.T) // back above the SMA (103): "stop-out while still above the SMA," // exactly the case above-sma exists for. h.triggerStop() + // sma(102,102,105)=103, stop=103*0.99=101.97 — this re-entry gets + // the identical immediate bracket protection the initial entry did + // (issue #368). intents, _ = h.onBar(7, bar{open: 100, high: 106, low: 99, close: 105}) require.Len(t, intents, 1) - assert.Equal(t, order.IntentEnter, intents[0].Kind, "above-sma must re-enter on the very next eligible flat bar, with no fresh cross or reclaim/breakout threshold required") + assert.Equal(t, order.IntentEnterWithStop, intents[0].Kind, "above-sma must re-enter on the very next eligible flat bar, with no fresh cross or reclaim/breakout threshold required") + require.NotNil(t, intents[0].StopPrice) + assert.Equal(t, "101.97", intents[0].StopPrice.String()) h.side = order.Long h.avgPrice = "105" @@ -984,3 +1036,154 @@ func TestStrategy_AboveSMAReEntryFiresOnTheVeryNextEligibleFlatBar(t *testing.T) assert.Equal(t, "103.29", intents[0].StopPrice.String()) assert.Equal(t, PhaseProbation, h.strategy.Phase(), "re-entry must start a fresh Probation episode, never stale Trending") } + +// TestStrategy_BracketEntryStopSameBarStillTransitionsLifecycle is PR +// #369 review's own required regression for blocker 1: a bracket +// entry whose own attached stop triggers within the same bar the +// entry itself fills must still transition the strategy's lifecycle +// into post-exit/re-entry mode (everExited=true, ReEntryRule.OnExit +// called) — not silently leave it looking like the strategy never +// entered at all. +// +// h.side deliberately stays order.Flat for the whole episode: OnBar +// never observes order.Long in between, exactly the case a bare +// sideLastBar comparison cannot see. deliverFill simulates +// backtest.Scheduler's own FillHandler delivery (ADR-060, issue #370) +// — the authoritative signal that replaced PR #369's own +// RealizedPnL/event.Bar.Open heuristics. +// +// The proof is behavioral, not internal-state inspection: configured +// with ReEntryRuleName "above-sma" (fires on the very next eligible +// flat bar, no fresh cross required) but the default +// InitialEntryModeName "fresh-cross" (requires an actual cross), bar +// 6 — the fill bar, already above the SMA with no fresh cross of its +// own — must itself immediately re-enter *if and only if* the +// lifecycle actually transitioned. If the bug were still present, +// everExited would stay false, bar 6's entry decision would fall +// through to InitialEntryRule's own "fresh-cross" gate instead, and +// no intent would be emitted at all (crossedAbove is false at bar 6: +// price was already above the SMA since bar 5). +func TestStrategy_BracketEntryStopSameBarStillTransitionsLifecycle(t *testing.T) { + h := newTestHarness(t, Config{ + SMAPeriod: 3, + ExitRuleName: "probation-trend", + ReEntryRuleName: "above-sma", + InitialStopBelowSMA: num.MustParseRate("0.01"), + TrailActivationGain: num.MustParseRate("0.05"), + TrailingStopPercent: num.MustParseRate("0.10"), + }) + + for i, c := range []float64{100, 100, 100, 99} { + h.onBar(i+1, bar{open: c, high: c, low: c, close: c}) + } + + // Bar 5: entry decision — a bracket order.IntentEnterWithStop. + intents, _ := h.onBar(5, bar{open: 102, high: 102, low: 102, close: 102}) + require.Len(t, intents, 1) + require.Equal(t, order.IntentEnterWithStop, intents[0].Kind) + // h.onBar's own auto-derivation (Enter/EnterWithStop -> Long) does + // not apply here: this test deliberately overrides it back to + // Flat, since the whole point is that OnBar never observes + // order.Long for this episode at all. + h.side = order.Flat + + // Bar 6: the entry's own fill bar. OnFill delivers both the entry + // fill (Buy) and the attached protective stop's own fill (Sell), + // exactly as Scheduler would before calling OnBar for this same + // batch. sma(99,102,102)=101, close (102) above it — still above + // the SMA, no fresh cross (already above since bar 5). + h.deliverFill(order.Buy) + h.deliverFill(order.Sell) + intents, _ = h.onBar(6, bar{open: 103, high: 105, low: 95, close: 102}) + require.Len(t, intents, 1, "the lifecycle must have transitioned to post-exit/re-entry mode: above-sma must fire immediately on this still-above-SMA bar, which fresh-cross (the initial-entry gate) never would") + assert.Equal(t, order.IntentEnterWithStop, intents[0].Kind, "the re-entry must also be a fresh bracket entry, protected from its own fill bar too") +} + +// TestStrategy_RejectedBracketOnGapThroughBarDoesNotFabricateExit is +// ADR-060's own required regression (issue #370, PR #369 re-review): +// a bracket entry that is rejected outright (no fill at all) must +// never be misclassified as a same-bar round trip, even on a bar +// whose Open happens to coincidentally sit at or below wherever the +// hypothetical stop would have been — exactly the residual gap PR +// #369's own RealizedPnL/event.Bar.Open heuristics could not resolve +// (a heuristic based on bar/account state alone cannot distinguish +// this from a genuine zero-PnL round trip; only real fill visibility +// can). +// +// No deliverFill call happens anywhere in this test: OnFill is simply +// never invoked for this instrument, the same as a real rejected +// entry produces no fill event at all. h.side stays Flat throughout, +// and bar 6's own Open (99) is deliberately at/below the 99.33 stop +// bar 5's decision would have used — the exact coincidence that used +// to fabricate an exit. +func TestStrategy_RejectedBracketOnGapThroughBarDoesNotFabricateExit(t *testing.T) { + h := newTestHarness(t, Config{ + SMAPeriod: 3, + ExitRuleName: "probation-trend", + ReEntryRuleName: "above-sma", + InitialStopBelowSMA: num.MustParseRate("0.01"), + TrailActivationGain: num.MustParseRate("0.05"), + TrailingStopPercent: num.MustParseRate("0.10"), + }) + + for i, c := range []float64{100, 100, 100, 99} { + h.onBar(i+1, bar{open: c, high: c, low: c, close: c}) + } + + // Bar 5: entry decision — a bracket order.IntentEnterWithStop with + // stop 99.33 (sma(100,99,102)=100.33333333, stop=100.33333333* + // 0.99=99.33). The pipeline/risk engine rejecting this in a real + // run is exactly what "no deliverFill call" simulates here. + intents, _ := h.onBar(5, bar{open: 102, high: 102, low: 102, close: 102}) + require.Len(t, intents, 1) + require.Equal(t, order.IntentEnterWithStop, intents[0].Kind) + require.Equal(t, "99.33", intents[0].StopPrice.String()) + h.side = order.Flat // never actually filled — no position ever opened. + + // Bar 6: no fill delivered at all. This bar's own Open (99) is + // deliberately at/below the 99.33 stop — the exact coincidence PR + // #369's own event.Bar.Open heuristic could not tell apart from a + // genuine round trip. Still above the SMA with no fresh cross + // (sma(99,102,102)=101, close 102 above it), so if the lifecycle + // were incorrectly considered "exited," above-sma would fire + // immediately here. + intents, _ = h.onBar(6, bar{open: 99, high: 100, low: 95, close: 102}) + assert.Empty(t, intents, "a rejected/unfilled bracket must never fabricate an exit: everExited must still be false, so above-sma (which only applies post-exit) must not fire") + + // Confirm directly: the strategy still believes it has never + // entered at all, so the *next* flat bar is still governed by + // InitialEntryRule ("fresh-cross", which requires an actual cross) + // rather than the configured "above-sma" ReEntryRule (which would + // fire unconditionally on any above-SMA flat bar) — a bar that + // stays above the SMA with no fresh cross must produce no entry. + intents, _ = h.onBar(7, bar{open: 100, high: 101, low: 99, close: 100}) + assert.Empty(t, intents, "still no fresh cross: InitialEntryRule, not ReEntryRule, must still be governing entry decisions") +} + +// TestProbationTrendExitRule_SeedInitialStopPreventsLoosening is PR +// #369 review's own required "also fix before merge" regression: a +// bracket entry that *survives* its own fill bar, whose next +// SMA-derived probation stop would actually be lower than the +// bracket's own initial stop, must never loosen protection down to +// that lower value — SeedInitialStop's own floor must hold exactly +// like PROBATION's ordinary ratchet-only comparison already does for +// every later bar. +func TestProbationTrendExitRule_SeedInitialStopPreventsLoosening(t *testing.T) { + rule := newProbationTrendRuleForTest(t) + rule.OnEntry(mustBar(t, "100", "100", "99", "100"), num.MustParsePrice("100")) + rule.SeedInitialStop(num.MustParsePrice("99.5")) + + // sma=99: 99*0.99=98.01, below the 99.5 floor SeedInitialStop just + // established — must emit nothing, never loosen down to 98.01. + decision, err := rule.OnLongBar(mustBar(t, "100", "101", "99", "100"), 99) + require.NoError(t, err) + assert.Nil(t, decision.NewStop, "must never loosen from the seeded 99.5 floor down to 98.01") + + // sma=101, close (102) safely above it (avoiding the independent + // SMA-cross override): 101*0.99=99.99, finally above the 99.5 + // floor — normal ratcheting resumes once it's genuinely earned. + decision, err = rule.OnLongBar(mustBar(t, "101", "103", "100", "102"), 101) + require.NoError(t, err) + require.NotNil(t, decision.NewStop) + assert.Equal(t, "99.99", decision.NewStop.String()) +} diff --git a/strategy/smatrend/testdata/raw/oanda/EURUSD/2024/02/EURUSD-2024-02-d1.csv b/strategy/smatrend/testdata/raw/oanda/EURUSD/2024/02/EURUSD-2024-02-d1.csv new file mode 100644 index 0000000..61639b1 --- /dev/null +++ b/strategy/smatrend/testdata/raw/oanda/EURUSD/2024/02/EURUSD-2024-02-d1.csv @@ -0,0 +1,11 @@ +# schema=raw-v1 source=oanda instrument=EURUSD tf=d1 year=2024 month=02 +time,bid_o,bid_h,bid_l,bid_c,ask_o,ask_h,ask_l,ask_c,volume,complete +2024-02-01T22:00:00Z,1.10000,1.10000,1.10000,1.10000,1.10002,1.10002,1.10002,1.10002,1000,true +2024-02-02T22:00:00Z,1.10000,1.10000,1.10000,1.10000,1.10002,1.10002,1.10002,1.10002,1000,true +2024-02-05T22:00:00Z,1.09000,1.09000,1.09000,1.09000,1.09002,1.09002,1.09002,1.09002,1000,true +2024-02-06T22:00:00Z,1.20000,1.20000,1.20000,1.20000,1.20002,1.20002,1.20002,1.20002,1000,true +2024-02-07T22:00:00Z,1.19000,1.20000,1.05000,1.15000,1.19002,1.20002,1.05002,1.15002,1000,true +2024-02-08T22:00:00Z,1.05000,1.06000,1.05000,1.05000,1.05002,1.06002,1.05002,1.05002,1000,true +2024-02-09T22:00:00Z,1.00000,1.00000,1.00000,1.00000,1.00002,1.00002,1.00002,1.00002,1000,true +2024-02-12T22:00:00Z,1.30000,1.30000,1.30000,1.30000,1.30002,1.30002,1.30002,1.30002,1000,true +2024-02-13T22:00:00Z,1.29000,1.30000,1.05000,1.20000,1.29002,1.30002,1.05002,1.20002,1000,true