From 94b78e495471d16448871be5b535e5c1ef4998c5 Mon Sep 17 00:00:00 2001 From: Rusty Eddy Date: Fri, 11 Sep 2026 12:01:24 -0700 Subject: [PATCH 1/4] EQ-368: wire strategy/smatrend to EnterWithStop for probation entry protection MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit What changed: - strategy/smatrend/exitrule.go: new optional ExitRule capability InitialStopProvider (InitialStop(smaValue float64) (num.Price, error)), implemented only by probationTrendExitRule — its probation stop is already a pure function of the SMA value alone, so it is computable before the entry fill. ExitRule.OnEntry gains a third parameter, initialStop *num.Price: the stop already resting from a bracket entry, so probationTrendExitRule seeds r.probationStop from it instead of nil, preserving the never-loosen ratchet invariant from the very first bar. trailingStopExitRule and smaCrossExitRule ignore it (they never implement InitialStopProvider). - strategy/smatrend/strategy.go: onFlat now calls a new buildEntryIntent helper: it emits order.IntentEnterWithStop (ADR-059) when exitRule implements InitialStopProvider, or a plain order.IntentEnter otherwise, unchanged from before. A new pendingInitialStop field carries the stop price exactly one bar, to OnBar's own Flat->Long transition, which hands it to exitRule.OnEntry. - strategy/smatrend/regression_test.go: replaced TestSMATrend_ProbationEntryBarIntrabarGapIsAKnownLimitation with TestSMATrend_ProbationEntryBarBreachClosesSameBar, on a new dedicated February EURUSD fixture engineered so both an initial entry and a fresh-cross re-entry breach their own bracket stop intrabar on the entry fill bar itself — proving the position does not survive past its own fill bar for either episode. Added TestSMATrend_PlainEntryStillWorksWithoutInitialStopProvider proving the default trailing-stop rule is unaffected. Generalized runSMATrendFixtureWithConfig into runSMATrendFixtureForSpan so the new fixture doesn't disturb the original January fixture every other regression in this file depends on. - strategy/smatrend/exitrule_test.go, strategy_test.go: updated OnEntry call sites for the new parameter; updated two existing end-to-end Strategy tests (TestStrategy_ProbationTrendFullLifecyclePhaseTransitions, TestStrategy_AboveSMAReEntryFiresOnTheVeryNextEligibleFlatBar) to expect IntentEnterWithStop (with the correct pre-fill stop price) at both the initial entry and the re-entry, proving both get identical protection. - strategy/smatrend/doc.go, docs/arch/adr-059-bracket-entry-with-stop.org: documented the capability and recorded this issue's completion of the deferral ADR-059 explicitly left open, without rewriting ADR-059's own historical Context/Decision text. No new ADR: this is strategy-specific wiring onto an already-Accepted framework capability (ADR-059), not a new architectural decision. Design note: the design considered an ExitRule-facing contract shaped like the issue's own suggested `InitialStop(entryContext) (num.Price, bool)`. Implemented instead as `InitialStop(smaValue float64) (num.Price, error)` — no `ok bool`, since probationTrendExitRule's probation-stop formula always succeeds for any smaValue (barring an arithmetic error, already reported via error) — matching the existing phaseReporter optional- capability pattern already used in this package. Not in scope (explicitly, per issue #368): live/async-broker bracket semantics (issue #366) and rerunning the SMA Long Hold reference research (issue #365), both deferred as before. How tested: - go build ./..., go vet ./..., gofmt -l . all clean. - go test -race ./... passes (full suite). - Verified TestSMATrend_ProbationEntryBarBreachClosesSameBar is meaningful: temporarily forced buildEntryIntent to always emit a plain Enter, confirmed the test fails (0 bracket intents, only 1 of 2 expected trades — the second episode's stop never becomes a resting order until one bar too late, exactly reproducing the old known limitation), then restored the fix. Refs #351, #366, #368, PR #367 Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_015fVsVuQCgkrhiYaXLxyUF3 --- docs/arch/adr-059-bracket-entry-with-stop.org | 12 +- strategy/smatrend/doc.go | 10 ++ strategy/smatrend/exitrule.go | 107 ++++++++++--- strategy/smatrend/exitrule_test.go | 14 +- strategy/smatrend/regression_test.go | 146 +++++++++++++----- strategy/smatrend/strategy.go | 61 +++++++- strategy/smatrend/strategy_test.go | 42 +++-- .../EURUSD/2024/02/EURUSD-2024-02-d1.csv | 11 ++ 8 files changed, 326 insertions(+), 77 deletions(-) create mode 100644 strategy/smatrend/testdata/raw/oanda/EURUSD/2024/02/EURUSD-2024-02-d1.csv diff --git a/docs/arch/adr-059-bracket-entry-with-stop.org b/docs/arch/adr-059-bracket-entry-with-stop.org index 73a076a..53cfa8c 100644 --- a/docs/arch/adr-059-bracket-entry-with-stop.org +++ b/docs/arch/adr-059-bracket-entry-with-stop.org @@ -216,7 +216,17 @@ never through this service. the fix and observing the real, expected failure before restoring it. - =strategy/smatrend= is *not* rewired to use =IntentEnterWithStop= in this issue — a deliberate, explicit deferral (issue #351's own scope - boundary), not an oversight. + boundary), not an oversight. (Update: issue #368 performed that + rewiring for "probation-trend" — see its own doc comments and + =strategy/smatrend/doc.go=. The known-limitation test this Context + section names, + =TestSMATrend_ProbationEntryBarIntrabarGapIsAKnownLimitation=, was + replaced there by + =TestSMATrend_ProbationEntryBarBreachClosesSameBar=, which proves + the gap is closed rather than merely documenting it; this ADR's own + Context/Decision text above is left as originally written, per this + registry's non-rewrite convention for an already-Accepted + decision.) - Live/real-broker atomicity remains unsolved, tracked as issue #366. =ErrBracketEntryNotSynchronouslyFilled= is the concrete, tested proof that this decision fails loudly rather than silently in that case, 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..1cd8ab2 100644 --- a/strategy/smatrend/exitrule.go +++ b/strategy/smatrend/exitrule.go @@ -47,7 +47,19 @@ type ExitRule interface { // entry (for example probationTrendExitRule's trail activation). // A rule with no such dependency (trailingStopExitRule, // smaCrossExitRule) simply ignores it. - OnEntry(entryBar marketdata.Bar, entryPrice num.Price) + // + // initialStop is the protective stop already resting from a + // bracket entry (order.IntentEnterWithStop, ADR-059), when + // Strategy used one — see InitialStopProvider — or nil when the + // entry was a plain order.IntentEnter (issue #368). A rule that + // implements InitialStopProvider must seed whatever state governs + // its own next ratchet-only comparison from this exact value + // rather than from nil, so it never emits a decision that would + // loosen protection below what the entry itself already + // established. A rule that never implements InitialStopProvider + // always receives nil here and can ignore this parameter exactly + // as before. + OnEntry(entryBar marketdata.Bar, entryPrice num.Price, initialStop *num.Price) // OnLongBar is called once per bar while long, after the position // has survived the bar. smaValue is the strategy's own current // SMA value, supplied for a rule whose trigger depends on it (for @@ -56,6 +68,29 @@ 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) +} + // 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). @@ -84,7 +119,10 @@ func newTrailingStopExitRule(cfg Config) (ExitRule, error) { return &trailingStopExitRule{retainFraction: retain}, nil } -func (r *trailingStopExitRule) OnEntry(entryBar marketdata.Bar, _ num.Price) { +// OnEntry ignores initialStop: trailingStopExitRule never implements +// InitialStopProvider, since its first stop genuinely depends on the +// entry/fill bar's own High, not knowable before the fill. +func (r *trailingStopExitRule) OnEntry(entryBar marketdata.Bar, _ num.Price, _ *num.Price) { high := entryBar.High r.highWaterMark = &high r.lastStop = nil @@ -120,7 +158,10 @@ func newSMACrossExitRule(Config) (ExitRule, error) { return smaCrossExitRule{}, nil } -func (smaCrossExitRule) OnEntry(marketdata.Bar, num.Price) {} +// OnEntry ignores initialStop for the identical reason +// trailingStopExitRule's own OnEntry does: smaCrossExitRule never +// implements InitialStopProvider (it manages no resting stop at all). +func (smaCrossExitRule) OnEntry(marketdata.Bar, num.Price, *num.Price) {} func (smaCrossExitRule) OnLongBar(bar marketdata.Bar, smaValue float64) (ExitDecision, error) { // bar.Close.Float64() is ADR-045's explicit exact-to-analytical @@ -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 @@ -199,15 +247,38 @@ func newProbationTrendExitRule(cfg Config) (ExitRule, error) { // reads from. func (r *probationTrendExitRule) Phase() Phase { return r.phase } -func (r *probationTrendExitRule) OnEntry(entryBar marketdata.Bar, entryPrice num.Price) { +// OnEntry seeds r.probationStop from initialStop, when non-nil, +// instead of resetting it to nil (issue #368): initialStop is the +// stop this rule itself already returned from InitialStop and +// Strategy already placed via a bracket entry, so onProbationBar's +// own ratchet-only comparison (stop.Cmp(*r.probationStop) > 0) must +// treat it as the floor already earned, never emitting a decision +// that would loosen protection below it. When initialStop is nil (a +// plain entry — no InitialStopProvider was consulted, or Strategy +// chose not to use it), r.probationStop resets to nil exactly as +// before. +func (r *probationTrendExitRule) OnEntry(entryBar marketdata.Bar, entryPrice num.Price, initialStop *num.Price) { r.phase = PhaseProbation r.entryPrice = entryPrice high := entryBar.High r.highWaterMark = &high - r.probationStop = nil + r.probationStop = initialStop 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 +} + 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/exitrule_test.go b/strategy/smatrend/exitrule_test.go index 685bd11..ab9c211 100644 --- a/strategy/smatrend/exitrule_test.go +++ b/strategy/smatrend/exitrule_test.go @@ -33,7 +33,7 @@ func TestTrailingStopExitRule_RatchetsMonotonicallyUpward(t *testing.T) { rule, err := newTrailingStopExitRule(cfg) require.NoError(t, err) - rule.OnEntry(mustBar(t, "100", "110", "99", "105"), num.MustParsePrice("100")) + rule.OnEntry(mustBar(t, "100", "110", "99", "105"), num.MustParsePrice("100"), nil) decision, err := rule.OnLongBar(mustBar(t, "103", "110", "102", "105"), 90) require.NoError(t, err) @@ -56,7 +56,7 @@ func TestTrailingStopExitRule_RatchetsMonotonicallyUpward(t *testing.T) { func TestSMACrossExitRule_ExitsOnCloseAtOrBelowSMA(t *testing.T) { rule, err := newSMACrossExitRule(Config{}) require.NoError(t, err) - rule.OnEntry(mustBar(t, "100", "105", "99", "102"), num.MustParsePrice("100")) + rule.OnEntry(mustBar(t, "100", "105", "99", "102"), num.MustParsePrice("100"), nil) decision, err := rule.OnLongBar(mustBar(t, "103", "106", "102", "105"), 100) require.NoError(t, err) @@ -85,7 +85,7 @@ func newProbationTrendRuleForTest(t *testing.T) *probationTrendExitRule { // SMA on a later bar must never pull the stop back down. func TestProbationTrendExitRule_ProbationStopRatchetsFromSMAOnly(t *testing.T) { rule := newProbationTrendRuleForTest(t) - rule.OnEntry(mustBar(t, "100", "101", "99", "100"), num.MustParsePrice("100")) + rule.OnEntry(mustBar(t, "100", "101", "99", "100"), num.MustParsePrice("100"), nil) assert.Equal(t, PhaseProbation, rule.Phase()) // sma=99, close (101) above it and well under the 105 activation @@ -116,7 +116,7 @@ func TestProbationTrendExitRule_ProbationStopRatchetsFromSMAOnly(t *testing.T) { // currently rests. func TestProbationTrendExitRule_SMACrossExitsDuringProbation(t *testing.T) { rule := newProbationTrendRuleForTest(t) - rule.OnEntry(mustBar(t, "100", "101", "99", "100"), num.MustParsePrice("100")) + rule.OnEntry(mustBar(t, "100", "101", "99", "100"), num.MustParsePrice("100"), nil) decision, err := rule.OnLongBar(mustBar(t, "99", "100", "95", "97"), 100) require.NoError(t, err) @@ -132,7 +132,7 @@ func TestProbationTrendExitRule_SMACrossExitsDuringProbation(t *testing.T) { // itself only takes effect starting the *next* OnLongBar call. func TestProbationTrendExitRule_ActivatesOnCloseGainThresholdWithoutRetroactivelyTighteningTheBarItself(t *testing.T) { rule := newProbationTrendRuleForTest(t) - rule.OnEntry(mustBar(t, "100", "100", "99", "100"), num.MustParsePrice("100")) + rule.OnEntry(mustBar(t, "100", "100", "99", "100"), num.MustParsePrice("100"), nil) // A large spike High while still in probation, never exceeded // again — this is the "since entry, not since activation" case @@ -178,7 +178,7 @@ func TestProbationTrendExitRule_ActivatesOnCloseGainThresholdWithoutRetroactivel // already had in place. func TestProbationTrendExitRule_HandoffNeverLoosensProtection(t *testing.T) { rule := newProbationTrendRuleForTest(t) - rule.OnEntry(mustBar(t, "100", "100", "99", "100"), num.MustParsePrice("100")) + rule.OnEntry(mustBar(t, "100", "100", "99", "100"), num.MustParsePrice("100"), nil) // Activates this bar: sma=104, close=105 >= 100*1.05=105 // threshold. Probation stop = 104*0.99 = 102.96. @@ -209,7 +209,7 @@ func TestProbationTrendExitRule_HandoffNeverLoosensProtection(t *testing.T) { // trailingStopExitRule's own ratchet once activated. func TestProbationTrendExitRule_TrendingStopRatchetsMonotonicallyUpward(t *testing.T) { rule := newProbationTrendRuleForTest(t) - rule.OnEntry(mustBar(t, "100", "100", "99", "100"), num.MustParsePrice("100")) + rule.OnEntry(mustBar(t, "100", "100", "99", "100"), num.MustParsePrice("100"), nil) // Force activation immediately. _, err := rule.OnLongBar(mustBar(t, "100", "100", "99", "105"), 100) require.NoError(t, err) 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..e2924c2 100644 --- a/strategy/smatrend/strategy.go +++ b/strategy/smatrend/strategy.go @@ -80,6 +80,17 @@ 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 so OnBar's own Flat->Long transition can hand it to + // exitRule.OnEntry as the level already resting from that bracket + // entry (issue #368) — nil whenever the most recent entry was a + // plain Enter (no InitialStopProvider, or none configured). + // Always consumed (read and reset to nil) on the very next + // Flat->Long transition; a rejected/never-filled entry attempt is + // simply overwritten by the next actual entry attempt without + // ever being read. + pendingInitialStop *num.Price intents strategy.IntentFactory journal journal.Recorder // nil unless env.Journal was set @@ -220,7 +231,8 @@ func (s *Strategy) OnBar(ctx context.Context, event strategy.BarEvent, view stra if err != nil { return nil, fmt.Errorf("smatrend: reading entry price: %w", err) } - s.exitRule.OnEntry(event.Bar, entryPrice) + s.exitRule.OnEntry(event.Bar, entryPrice, s.pendingInitialStop) + s.pendingInitialStop = nil } s.sideLastBar = order.Long return s.onLong(ctx, event, close, smaValue) @@ -298,16 +310,59 @@ 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 (recorded via +// s.pendingInitialStop for exitRule.OnEntry to seed its own state +// from, see that field's own doc comment) and the value recordSignal +// journals as decision evidence. +// +// 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 + 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..050e582 100644 --- a/strategy/smatrend/strategy_test.go +++ b/strategy/smatrend/strategy_test.go @@ -728,7 +728,7 @@ func TestStrategy_BreakoutNReEntryObservesBelowSMABarsNotJustAboveSMAOnes(t *tes // does it. type ambiguousExitRule struct{} -func (ambiguousExitRule) OnEntry(marketdata.Bar, num.Price) {} +func (ambiguousExitRule) OnEntry(marketdata.Bar, num.Price, *num.Price) {} func (ambiguousExitRule) OnLongBar(bar marketdata.Bar, _ float64) (ExitDecision, error) { stop := bar.Close @@ -843,16 +843,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 +920,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 +967,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 +989,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" 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 From 6e4fb5dc672b3fcd4fe8db2ec48e98ef9123b887 Mon Sep 17 00:00:00 2001 From: Rusty Eddy Date: Fri, 11 Sep 2026 12:45:53 -0700 Subject: [PATCH 2/4] =?UTF-8?q?EQ-368:=20address=20PR=20#369=20review=20?= =?UTF-8?q?=E2=80=94=20same-bar=20round=20trip,=20lastStop,=20exported-int?= =?UTF-8?q?erface=20break,=20version=20bump?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Blocker 1 (same-bar fill+stop bypasses the exit transition entirely): a bracket entry's own attached stop can trigger within the same bar the entry fills, so sideLastBar never observes order.Long at all — onExit never ran, everExited stayed false, and the next entry was incorrectly governed by InitialEntryRule instead of the configured ReEntryRule. Fixed by tracking view.Account().RealizedPnL() bar over bar (a new lastRealizedPnL field, refreshed via defer on every OnBar return path): RealizedPnL only changes when a position actually closes, so a change observed while pendingInitialStop is non-nil and side is Flat is conclusive proof of a same-bar round trip, and triggers the same onExit path the classic Long->Flat transition uses. No change is conclusive proof the entry never filled at all (discardUnfilledBracket). New regression: TestStrategy_BracketEntryStopSameBarStillTransitionsLifecycle, proven behaviorally (above-sma re-entry fires immediately on the fill bar itself, which fresh-cross/InitialEntryRule never would) rather than by inspecting unexported state. Verified meaningful by disabling the detection branch and confirming the test fails. Blocker 2 (bracket stop never recorded in lastStop): buildEntryIntent now sets s.lastStop alongside s.pendingInitialStop when building a bracket intent, so a position that survives its fill bar with no ratcheting AdjustStop on the very next OnLongBar call still has the correct reference level if its resting stop later triggers (would otherwise corrupt exit-price-based re-entry rules like "reclaim-exit-price"). Both fields are speculative until the outcome is known; discardUnfilledBracket rolls both back if the entry never filled. Blocker 3 (exported ExitRule.OnEntry signature break): reverted OnEntry back to its original two-argument signature. Added a second, separate optional capability, InitialStopSeeder (SeedInitialStop(num.Price)), which Strategy calls immediately after OnEntry only when a bracket entry was actually used — probation-trend implements it; OnEntry's own default reset runs first, then SeedInitialStop overrides just the one field it cares about. No external/custom ExitRule implementation is affected. Blocker 4 (strategy version): bumped smatrend.Version from "v1" to "v2" per ADR-044 — persisted signal semantics changed (a new "enter-long-with-stop" action) and entry-bar stop behavior changed, so prior "v1" runs are not directly comparable. Also fixed (per review's "also fix before merge" list): - New unit regression TestProbationTrendExitRule_SeedInitialStopPreventsLoosening: a bracket entry that survives its fill bar with a lower next-bar SMA stop must not loosen below the seeded floor (Copilot's own suppressed-comment finding, same root cause as blocker 2). - Reverted the docs/arch/adr-059-bracket-entry-with-stop.org edit from the previous commit: it rewrote a Consequences bullet on an Accepted ADR outside this registry's own amendment process (Copilot finding). ADR-059 is now back to exactly its merged text; the issue #368 completion note lives only in strategy/smatrend/doc.go and exitrule.go's own doc comments, as Copilot's suggested alternative. How tested: - go build ./..., go vet ./..., gofmt -l . all clean. - go test -race ./... passes (full suite). - Verified TestStrategy_BracketEntryStopSameBarStillTransitionsLifecycle is meaningful: temporarily disabled the RealizedPnL round-trip detection branch, confirmed the test fails (no re-entry intent on the fill bar — the lifecycle never transitioned), then restored. Refs #351, #366, #368, PR #369 Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_015fVsVuQCgkrhiYaXLxyUF3 --- docs/arch/adr-059-bracket-entry-with-stop.org | 12 +- strategy/smatrend/exitrule.go | 67 ++++---- strategy/smatrend/exitrule_test.go | 14 +- strategy/smatrend/strategy.go | 151 ++++++++++++++++-- strategy/smatrend/strategy_test.go | 109 ++++++++++++- 5 files changed, 278 insertions(+), 75 deletions(-) diff --git a/docs/arch/adr-059-bracket-entry-with-stop.org b/docs/arch/adr-059-bracket-entry-with-stop.org index 53cfa8c..73a076a 100644 --- a/docs/arch/adr-059-bracket-entry-with-stop.org +++ b/docs/arch/adr-059-bracket-entry-with-stop.org @@ -216,17 +216,7 @@ never through this service. the fix and observing the real, expected failure before restoring it. - =strategy/smatrend= is *not* rewired to use =IntentEnterWithStop= in this issue — a deliberate, explicit deferral (issue #351's own scope - boundary), not an oversight. (Update: issue #368 performed that - rewiring for "probation-trend" — see its own doc comments and - =strategy/smatrend/doc.go=. The known-limitation test this Context - section names, - =TestSMATrend_ProbationEntryBarIntrabarGapIsAKnownLimitation=, was - replaced there by - =TestSMATrend_ProbationEntryBarBreachClosesSameBar=, which proves - the gap is closed rather than merely documenting it; this ADR's own - Context/Decision text above is left as originally written, per this - registry's non-rewrite convention for an already-Accepted - decision.) + boundary), not an oversight. - Live/real-broker atomicity remains unsolved, tracked as issue #366. =ErrBracketEntryNotSynchronouslyFilled= is the concrete, tested proof that this decision fails loudly rather than silently in that case, diff --git a/strategy/smatrend/exitrule.go b/strategy/smatrend/exitrule.go index 1cd8ab2..88f7850 100644 --- a/strategy/smatrend/exitrule.go +++ b/strategy/smatrend/exitrule.go @@ -47,19 +47,7 @@ type ExitRule interface { // entry (for example probationTrendExitRule's trail activation). // A rule with no such dependency (trailingStopExitRule, // smaCrossExitRule) simply ignores it. - // - // initialStop is the protective stop already resting from a - // bracket entry (order.IntentEnterWithStop, ADR-059), when - // Strategy used one — see InitialStopProvider — or nil when the - // entry was a plain order.IntentEnter (issue #368). A rule that - // implements InitialStopProvider must seed whatever state governs - // its own next ratchet-only comparison from this exact value - // rather than from nil, so it never emits a decision that would - // loosen protection below what the entry itself already - // established. A rule that never implements InitialStopProvider - // always receives nil here and can ignore this parameter exactly - // as before. - OnEntry(entryBar marketdata.Bar, entryPrice num.Price, initialStop *num.Price) + OnEntry(entryBar marketdata.Bar, entryPrice num.Price) // OnLongBar is called once per bar while long, after the position // has survived the bar. smaValue is the strategy's own current // SMA value, supplied for a rule whose trigger depends on it (for @@ -91,6 +79,24 @@ 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). @@ -119,10 +125,7 @@ func newTrailingStopExitRule(cfg Config) (ExitRule, error) { return &trailingStopExitRule{retainFraction: retain}, nil } -// OnEntry ignores initialStop: trailingStopExitRule never implements -// InitialStopProvider, since its first stop genuinely depends on the -// entry/fill bar's own High, not knowable before the fill. -func (r *trailingStopExitRule) OnEntry(entryBar marketdata.Bar, _ num.Price, _ *num.Price) { +func (r *trailingStopExitRule) OnEntry(entryBar marketdata.Bar, _ num.Price) { high := entryBar.High r.highWaterMark = &high r.lastStop = nil @@ -158,10 +161,7 @@ func newSMACrossExitRule(Config) (ExitRule, error) { return smaCrossExitRule{}, nil } -// OnEntry ignores initialStop for the identical reason -// trailingStopExitRule's own OnEntry does: smaCrossExitRule never -// implements InitialStopProvider (it manages no resting stop at all). -func (smaCrossExitRule) OnEntry(marketdata.Bar, num.Price, *num.Price) {} +func (smaCrossExitRule) OnEntry(marketdata.Bar, num.Price) {} func (smaCrossExitRule) OnLongBar(bar marketdata.Bar, smaValue float64) (ExitDecision, error) { // bar.Close.Float64() is ADR-045's explicit exact-to-analytical @@ -247,22 +247,12 @@ func newProbationTrendExitRule(cfg Config) (ExitRule, error) { // reads from. func (r *probationTrendExitRule) Phase() Phase { return r.phase } -// OnEntry seeds r.probationStop from initialStop, when non-nil, -// instead of resetting it to nil (issue #368): initialStop is the -// stop this rule itself already returned from InitialStop and -// Strategy already placed via a bracket entry, so onProbationBar's -// own ratchet-only comparison (stop.Cmp(*r.probationStop) > 0) must -// treat it as the floor already earned, never emitting a decision -// that would loosen protection below it. When initialStop is nil (a -// plain entry — no InitialStopProvider was consulted, or Strategy -// chose not to use it), r.probationStop resets to nil exactly as -// before. -func (r *probationTrendExitRule) OnEntry(entryBar marketdata.Bar, entryPrice num.Price, initialStop *num.Price) { +func (r *probationTrendExitRule) OnEntry(entryBar marketdata.Bar, entryPrice num.Price) { r.phase = PhaseProbation r.entryPrice = entryPrice high := entryBar.High r.highWaterMark = &high - r.probationStop = initialStop + r.probationStop = nil r.trendStop = nil } @@ -279,6 +269,17 @@ func (r *probationTrendExitRule) InitialStop(smaValue float64) (num.Price, error 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/exitrule_test.go b/strategy/smatrend/exitrule_test.go index ab9c211..685bd11 100644 --- a/strategy/smatrend/exitrule_test.go +++ b/strategy/smatrend/exitrule_test.go @@ -33,7 +33,7 @@ func TestTrailingStopExitRule_RatchetsMonotonicallyUpward(t *testing.T) { rule, err := newTrailingStopExitRule(cfg) require.NoError(t, err) - rule.OnEntry(mustBar(t, "100", "110", "99", "105"), num.MustParsePrice("100"), nil) + rule.OnEntry(mustBar(t, "100", "110", "99", "105"), num.MustParsePrice("100")) decision, err := rule.OnLongBar(mustBar(t, "103", "110", "102", "105"), 90) require.NoError(t, err) @@ -56,7 +56,7 @@ func TestTrailingStopExitRule_RatchetsMonotonicallyUpward(t *testing.T) { func TestSMACrossExitRule_ExitsOnCloseAtOrBelowSMA(t *testing.T) { rule, err := newSMACrossExitRule(Config{}) require.NoError(t, err) - rule.OnEntry(mustBar(t, "100", "105", "99", "102"), num.MustParsePrice("100"), nil) + rule.OnEntry(mustBar(t, "100", "105", "99", "102"), num.MustParsePrice("100")) decision, err := rule.OnLongBar(mustBar(t, "103", "106", "102", "105"), 100) require.NoError(t, err) @@ -85,7 +85,7 @@ func newProbationTrendRuleForTest(t *testing.T) *probationTrendExitRule { // SMA on a later bar must never pull the stop back down. func TestProbationTrendExitRule_ProbationStopRatchetsFromSMAOnly(t *testing.T) { rule := newProbationTrendRuleForTest(t) - rule.OnEntry(mustBar(t, "100", "101", "99", "100"), num.MustParsePrice("100"), nil) + rule.OnEntry(mustBar(t, "100", "101", "99", "100"), num.MustParsePrice("100")) assert.Equal(t, PhaseProbation, rule.Phase()) // sma=99, close (101) above it and well under the 105 activation @@ -116,7 +116,7 @@ func TestProbationTrendExitRule_ProbationStopRatchetsFromSMAOnly(t *testing.T) { // currently rests. func TestProbationTrendExitRule_SMACrossExitsDuringProbation(t *testing.T) { rule := newProbationTrendRuleForTest(t) - rule.OnEntry(mustBar(t, "100", "101", "99", "100"), num.MustParsePrice("100"), nil) + rule.OnEntry(mustBar(t, "100", "101", "99", "100"), num.MustParsePrice("100")) decision, err := rule.OnLongBar(mustBar(t, "99", "100", "95", "97"), 100) require.NoError(t, err) @@ -132,7 +132,7 @@ func TestProbationTrendExitRule_SMACrossExitsDuringProbation(t *testing.T) { // itself only takes effect starting the *next* OnLongBar call. func TestProbationTrendExitRule_ActivatesOnCloseGainThresholdWithoutRetroactivelyTighteningTheBarItself(t *testing.T) { rule := newProbationTrendRuleForTest(t) - rule.OnEntry(mustBar(t, "100", "100", "99", "100"), num.MustParsePrice("100"), nil) + rule.OnEntry(mustBar(t, "100", "100", "99", "100"), num.MustParsePrice("100")) // A large spike High while still in probation, never exceeded // again — this is the "since entry, not since activation" case @@ -178,7 +178,7 @@ func TestProbationTrendExitRule_ActivatesOnCloseGainThresholdWithoutRetroactivel // already had in place. func TestProbationTrendExitRule_HandoffNeverLoosensProtection(t *testing.T) { rule := newProbationTrendRuleForTest(t) - rule.OnEntry(mustBar(t, "100", "100", "99", "100"), num.MustParsePrice("100"), nil) + rule.OnEntry(mustBar(t, "100", "100", "99", "100"), num.MustParsePrice("100")) // Activates this bar: sma=104, close=105 >= 100*1.05=105 // threshold. Probation stop = 104*0.99 = 102.96. @@ -209,7 +209,7 @@ func TestProbationTrendExitRule_HandoffNeverLoosensProtection(t *testing.T) { // trailingStopExitRule's own ratchet once activated. func TestProbationTrendExitRule_TrendingStopRatchetsMonotonicallyUpward(t *testing.T) { rule := newProbationTrendRuleForTest(t) - rule.OnEntry(mustBar(t, "100", "100", "99", "100"), num.MustParsePrice("100"), nil) + rule.OnEntry(mustBar(t, "100", "100", "99", "100"), num.MustParsePrice("100")) // Force activation immediately. _, err := rule.OnLongBar(mustBar(t, "100", "100", "99", "105"), 100) require.NoError(t, err) diff --git a/strategy/smatrend/strategy.go b/strategy/smatrend/strategy.go index e2924c2..8a1d1d9 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 @@ -82,15 +91,50 @@ type Strategy struct { directExitReference *num.Price // pendingInitialStop is the stop price used by the most recent // EnterWithStop intent onFlat emitted, carried forward exactly one - // bar so OnBar's own Flat->Long transition can hand it to - // exitRule.OnEntry as the level already resting from that bracket - // entry (issue #368) — nil whenever the most recent entry was a - // plain Enter (no InitialStopProvider, or none configured). - // Always consumed (read and reset to nil) on the very next - // Flat->Long transition; a rejected/never-filled entry attempt is - // simply overwritten by the next actual entry attempt without - // ever being read. + // 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 the account's own RealizedPnL unchanged + // since the last bar observed: the entry never filled at all + // (rejected outright) — simply discarded, see + // discardUnfilledBracket. + // - Flat->Flat, with RealizedPnL changed: 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 lastRealizedPnL's own doc comment. pendingInitialStop *num.Price + // lastRealizedPnL is this Strategy's own last-observed + // view.Account().RealizedPnL(), captured at the end of every OnBar + // call regardless of return path (issue #368, PR #369 review). It + // exists solely to detect the one lifecycle transition a bare + // Flat/Long position-side comparison cannot see at all: a bracket + // entry (order.IntentEnterWithStop) whose own attached stop + // triggers within the same bar the entry itself fills. + // sideLastBar stays order.Flat across both the bar before and the + // bar of such an episode — OnBar never observes order.Long in + // between — so without this signal, onExit would never run: + // everExited would stay false, ReEntryRule.OnExit would never be + // called, and the next entry would be incorrectly governed by + // InitialEntryRule instead of the configured ReEntryRule. + // RealizedPnL only changes when a position actually closes + // (opening one does not move it), so a change observed while + // pendingInitialStop is non-nil and side is Flat is conclusive + // proof a full bracket round trip occurred; no change is + // conclusive proof the entry never filled at all. This assumes + // smatrend is the only strategy trading this account (already + // assumed elsewhere — see OnBar's own order.Short case) and a + // non-zero commission/fee model in the exceptionally narrow case + // where a round trip's realized gain is exactly zero and no + // RealizedPnL change would otherwise register. + lastRealizedPnL num.Money intents strategy.IntentFactory journal journal.Recorder // nil unless env.Journal was set @@ -201,6 +245,14 @@ 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) { + // Captured once, up front: used both by the Flat-branch same-bar- + // round-trip check below and to refresh lastRealizedPnL, via + // defer, on every return path (issue #368, PR #369 review) — + // including the warm-up early return, so lastRealizedPnL is never + // stale relative to what this bar actually observed. + currentRealizedPnL := view.Account().RealizedPnL() + defer func() { s.lastRealizedPnL = currentRealizedPnL }() + // 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. @@ -220,8 +272,23 @@ 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: + roundTrip, err := realizedPnLChanged(currentRealizedPnL, s.lastRealizedPnL) + if err != nil { + return nil, fmt.Errorf("smatrend: comparing realized pnl: %w", err) + } + if roundTrip { + // The bracket entry filled and its own attached stop + // triggered within this same bar (PR #369 review, + // blocker 1) — invisible to sideLastBar, which never + // observed order.Long in between. + s.onExit(event.Bar) + } else { + s.discardUnfilledBracket() + } } s.sideLastBar = order.Flat return s.onFlat(ctx, event, crossedAbove, aboveSMA, close, smaValue) @@ -231,8 +298,13 @@ func (s *Strategy) OnBar(ctx context.Context, event strategy.BarEvent, view stra if err != nil { return nil, fmt.Errorf("smatrend: reading entry price: %w", err) } - s.exitRule.OnEntry(event.Bar, entryPrice, s.pendingInitialStop) - s.pendingInitialStop = nil + 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) @@ -249,6 +321,29 @@ func (s *Strategy) OnBar(ctx context.Context, event strategy.BarEvent, view stra } } +// realizedPnLChanged reports whether cur differs from prev, both +// account.Snapshot.RealizedPnL() values from the same account and +// therefore always the same currency (issue #368) — a mismatch would +// indicate the account itself changed underneath this Strategy, which +// is reported as an error rather than silently guessed at. +func realizedPnLChanged(cur, prev num.Money) (bool, error) { + cmp, err := cur.Cmp(prev) + if err != nil { + return false, err + } + return cmp != 0, 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 "no realized-pnl change" 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 @@ -259,7 +354,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 { @@ -272,6 +375,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 @@ -330,10 +434,22 @@ func (s *Strategy) onFlat(ctx context.Context, event strategy.BarEvent, crossedA // 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 (recorded via -// s.pendingInitialStop for exitRule.OnEntry to seed its own state -// from, see that field's own doc comment) and the value recordSignal -// journals as decision evidence. +// 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 @@ -360,6 +476,7 @@ func (s *Strategy) buildEntryIntent(smaValue float64) (order.Intent, *num.Price, return order.Intent{}, nil, err } s.pendingInitialStop = &stop + s.lastStop = &stop return in, &stop, nil } diff --git a/strategy/smatrend/strategy_test.go b/strategy/smatrend/strategy_test.go index 050e582..f3a5cc0 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,14 @@ 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) — needed to simulate a bracket entry + // whose own attached stop triggers within the same bar the entry + // fills (issue #368, PR #369 review): h.side stays order.Flat for + // that whole episode (OnBar never observes order.Long at all), so + // a nonzero RealizedPnL is the only way this harness can simulate + // the one signal Strategy itself uses to detect it. + realizedPnL string } func newTestHarness(t *testing.T, config Config) *testHarness { @@ -167,7 +176,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 +197,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 @@ -728,7 +737,7 @@ func TestStrategy_BreakoutNReEntryObservesBelowSMABarsNotJustAboveSMAOnes(t *tes // does it. type ambiguousExitRule struct{} -func (ambiguousExitRule) OnEntry(marketdata.Bar, num.Price, *num.Price) {} +func (ambiguousExitRule) OnEntry(marketdata.Bar, num.Price) {} func (ambiguousExitRule) OnLongBar(bar marketdata.Bar, _ float64) (ExitDecision, error) { stop := bar.Close @@ -1008,3 +1017,89 @@ 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 (see Strategy.lastRealizedPnL's +// own doc comment). h.realizedPnL simulates the one signal Strategy +// itself has to detect it. +// +// 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. sma(99,102,102)=101, close + // (102) above it — still above the SMA, no fresh cross (already + // above since bar 5). + h.realizedPnL = "5" + 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") +} + +// 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()) +} From a5c957a4f345afe9589ff55c2969c2a3c24f2c98 Mon Sep 17 00:00:00 2001 From: Rusty Eddy Date: Fri, 11 Sep 2026 13:05:19 -0700 Subject: [PATCH 3/4] EQ-368: close the zero-RealizedPnL gap-through-stop edge case (PR #369 re-review) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit RealizedPnL alone misses one real same-bar round trip: a long bracket entry filling at this bar's own Open, already at or below its own protective stop, gaps through at that exact price (ADR-026's own gap rule fills the stop at the identical Open), so entry and exit realize exactly zero PnL and RealizedPnL never changes — the entry was misclassified as never having filled at all. Fixed by adding a second, independent check that needs no account state: event.Bar.Open compared directly against the bracket's own stop price. The two checks together are exhaustive for every fill outcome this codebase's own fill models produce: a non-gap intrabar touch always realizes a strictly negative PnL (caught by the RealizedPnL check), and a gap-through-Open fill always realizes exactly zero (caught by the new Open check). Documented the one residual, narrower coincidence this still cannot resolve — a bracket entry rejected outright on a bar whose Open independently happens to sit at or below where the stop would have been — as a known limitation requiring a real fill-event signal (a wired FillHandler capability) to close fully, out of scope here. New regression: TestStrategy_BracketEntryGapExactlyToStopStillTransitionsLifecycle, with RealizedPnL deliberately left at its zero default throughout, so only the new Open-vs-stop check can make it pass. How tested: - go build ./..., go vet ./..., gofmt -l . all clean. - go test -race ./... passes (full suite). - Verified meaningful: temporarily disabled the new gap check, confirmed the new test fails, then restored. Refs #351, #366, #368, PR #369 Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_015fVsVuQCgkrhiYaXLxyUF3 --- strategy/smatrend/strategy.go | 41 +++++++++++++++++++++------ strategy/smatrend/strategy_test.go | 45 ++++++++++++++++++++++++++++++ 2 files changed, 78 insertions(+), 8 deletions(-) diff --git a/strategy/smatrend/strategy.go b/strategy/smatrend/strategy.go index 8a1d1d9..c5cdb9e 100644 --- a/strategy/smatrend/strategy.go +++ b/strategy/smatrend/strategy.go @@ -127,13 +127,28 @@ type Strategy struct { // RealizedPnL only changes when a position actually closes // (opening one does not move it), so a change observed while // pendingInitialStop is non-nil and side is Flat is conclusive - // proof a full bracket round trip occurred; no change is - // conclusive proof the entry never filled at all. This assumes - // smatrend is the only strategy trading this account (already - // assumed elsewhere — see OnBar's own order.Short case) and a - // non-zero commission/fee model in the exceptionally narrow case - // where a round trip's realized gain is exactly zero and no - // RealizedPnL change would otherwise register. + // proof a full bracket round trip occurred. + // + // A change is sufficient but not necessary (PR #369 re-review): a + // long bracket entry filling at this bar's own Open, already at or + // below its own protective stop, realizes exactly zero PnL — + // ADR-026's gap rule fills the stop at that identical Open price, + // so entry and exit share one price. OnBar's own Flat branch + // covers that case with a second, independent check + // (event.Bar.Open against the bracket's own stop) that needs no + // account state at all. Together the two checks are exhaustive for + // every fill outcome this codebase's own fill models produce + // today: a non-gap intrabar touch always realizes a strictly + // negative PnL (the stop necessarily fills below the entry's own + // Open), and a gap-through-Open fill always realizes exactly zero. + // What remains unprovable from account/bar state alone is the + // reverse case — a bracket entry rejected outright (no fill at + // all) on a bar whose Open independently happens to sit at or + // below whatever stop price would have been used. That coincidence + // is unrelated to genuine price/risk causality and is not resolved + // here; closing it fully would need a real fill-event signal (for + // example a wired FillHandler capability) rather than inference + // over account/bar snapshots. lastRealizedPnL num.Money intents strategy.IntentFactory @@ -280,7 +295,17 @@ func (s *Strategy) OnBar(ctx context.Context, event strategy.BarEvent, view stra if err != nil { return nil, fmt.Errorf("smatrend: comparing realized pnl: %w", err) } - if roundTrip { + // A RealizedPnL change alone misses one real case (PR #369 + // re-review): a long bracket entry filling at this bar's + // own Open, already at or below its own protective stop + // (ADR-026's gap rule then fills that stop at the + // identical Open price) realizes exactly zero PnL — entry + // and exit at the same price. event.Bar.Open is knowable + // directly from this bar's own data, independent of + // account state, and conclusively proves that outcome + // whenever it holds. + gappedThroughStop := event.Bar.Open.Cmp(*s.pendingInitialStop) <= 0 + if roundTrip || gappedThroughStop { // The bracket entry filled and its own attached stop // triggered within this same bar (PR #369 review, // blocker 1) — invisible to sideLastBar, which never diff --git a/strategy/smatrend/strategy_test.go b/strategy/smatrend/strategy_test.go index f3a5cc0..0adae12 100644 --- a/strategy/smatrend/strategy_test.go +++ b/strategy/smatrend/strategy_test.go @@ -1076,6 +1076,51 @@ func TestStrategy_BracketEntryStopSameBarStillTransitionsLifecycle(t *testing.T) 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_BracketEntryGapExactlyToStopStillTransitionsLifecycle +// is PR #369's own re-review regression: RealizedPnL alone misses one +// real same-bar round trip — a long bracket entry filling at this +// bar's own Open, already at or below its own protective stop, gaps +// through at that exact price (ADR-026's own gap rule), so entry and +// exit realize exactly zero PnL and RealizedPnL never changes at all. +// h.realizedPnL is deliberately left at its zero default throughout — +// only event.Bar.Open's own relationship to the bracket's stop price +// can prove this case (see Strategy.lastRealizedPnL's own doc +// comment). +func TestStrategy_BracketEntryGapExactlyToStopStillTransitionsLifecycle(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(99,102's own bar4/5)... see the sibling test's + // own identical bar-5 comment: 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.IntentEnterWithStop, intents[0].Kind) + require.Equal(t, "99.33", intents[0].StopPrice.String()) + h.side = order.Flat // see the sibling test's own identical note. + + // Bar 6: the entry's own fill bar gaps down — Open (99) already at + // or below the 99.33 bracket stop. RealizedPnL stays "0" for this + // entire test; only the gap-through-Open check can prove this bar + // was a real round trip. Still above the SMA (sma(99,102,102)=101, + // close 102 above it) with no fresh cross, so above-sma fires + // immediately if the lifecycle correctly transitioned. + intents, _ = h.onBar(6, bar{open: 99, high: 100, low: 95, close: 102}) + require.Len(t, intents, 1, "the lifecycle must have transitioned to post-exit/re-entry mode even though RealizedPnL never changed") + assert.Equal(t, order.IntentEnterWithStop, intents[0].Kind) +} + // 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 From 0ff200debe3aa13bcabf6cfbab0b3069f7f19339 Mon Sep 17 00:00:00 2001 From: Rusty Eddy Date: Sun, 13 Sep 2026 10:27:29 -0700 Subject: [PATCH 4/4] =?UTF-8?q?Issue=20#370:=20strategy.FillHandler=20?= =?UTF-8?q?=E2=80=94=20authoritative=20per-bar=20fill=20signal=20(ADR-060)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit What changed: - strategy/fill.go (new): FillEvent (wraps order.Fill directly — it already carries OrderID/Listing/Side/Metadata, enough identity for a strategy to tell what filled) and FillHandler, a new optional Strategy capability: OnFill(ctx, FillEvent, View) error, matching History's own "small required core plus capability discovery" pattern. OnFill deliberately returns only error, not []order.Intent — emitting intents from a fill callback needs its own design (ordering/recursion within a batch, next-bar-open eligibility, warm-up, live semantics) no current strategy needs yet (issue #370 review). - backtest/scheduler.go: runBatch now delivers every fill a batch produced — Phase 2's flush and Phase 3's IntrabarAdvancer triggers alike, already accumulated into s.fills by drainAndJournal — to a FillHandler-implementing Strategy, one OnFill call per fill in delivery order, against the identical frozen View that batch's own OnBar calls use, strictly before the OnBar loop runs. Delivery is account-wide, not filtered by Describe().Requirements (issue #370 review: requirements describe market-data needs, not execution ownership — conflating the two would silently hide a fill). - strategy/smatrend/strategy.go: implements OnFill, replacing PR #369's own two heuristics (RealizedPnL delta, event.Bar.Open vs. stop) entirely, not retaining either as a fallback. entryFilledThisBatch/stopFilledThisBatch (named per issue #370 review — no generic "some fill happened" boolean) are set from a same-instrument fill's own Side. OnBar's Flat branch now reads entryFilledThisBatch directly: if the bracket's entry genuinely filled this batch and the position is Flat again at the end of it, its own attached stop is structurally the only thing that could have closed it. This is authoritative, not a heuristic — a rejected bracket entry produces no fill at all, so it can no longer be misclassified as a round trip regardless of that bar's own Open, closing PR #369 review's own residual gap. - docs/arch/adr-060-fill-handler-capability.org (new): records the decision, including the three alternatives considered (letting OnFill return intents now; extending View with a pull-style fill accessor instead of a push capability; a narrower smatrend-only signal) and why each was rejected. Indexed in adr-decisions.org. - strategy/doc.go: FillHandler is no longer on the "not published here yet" list. Tests: - strategy/smatrend/strategy_test.go: added testHarness.deliverFill(side), simulating Scheduler's own OnFill delivery. Rewrote TestStrategy_BracketEntryStopSameBarStillTransitionsLifecycle to use it instead of h.realizedPnL. Replaced the now-obsolete gap-specific test with TestStrategy_RejectedBracketOnGapThroughBarDoesNotFabricateExit — ADR-060's own required regression: no deliverFill call at all (simulating a rejected entry), on a bar whose Open coincidentally gaps through the hypothetical stop, must not fabricate an exit. Verified meaningful by reintroducing the old event.Bar.Open heuristic and confirming this new test fails. - backtest/scheduler_test.go: added TestScheduler_FillHandlerDeliversFillsBeforeOnBarInSameBatch, a real Scheduler.Run end-to-end proof (reusing TestScheduler_BracketEntryProtectsFromTheFillBarItself's own fixture) that both the bracket's entry and stop-triggered fills are delivered via OnFill, in delivery order, strictly before that same bar's own OnBar call — not merely that OnFill fires at all. Verified meaningful by disabling the Scheduler-side wiring and confirming the test fails. How tested: - go build ./..., go vet ./..., gofmt -l . all clean. - go test -race ./... passes (full suite). - Both new regressions independently verified meaningful (see above). Refs #351, #366, #368, #370, PR #369 Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_015fVsVuQCgkrhiYaXLxyUF3 --- backtest/scheduler.go | 39 +++ backtest/scheduler_test.go | 95 ++++++++ docs/arch/adr-060-fill-handler-capability.org | 223 ++++++++++++++++++ docs/arch/adr-decisions.org | 1 + strategy/doc.go | 18 +- strategy/fill.go | 42 ++++ strategy/smatrend/strategy.go | 165 +++++++------ strategy/smatrend/strategy_test.go | 113 ++++++--- 8 files changed, 567 insertions(+), 129 deletions(-) create mode 100644 docs/arch/adr-060-fill-handler-capability.org create mode 100644 strategy/fill.go 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/strategy.go b/strategy/smatrend/strategy.go index c5cdb9e..a4e624d 100644 --- a/strategy/smatrend/strategy.go +++ b/strategy/smatrend/strategy.go @@ -102,54 +102,41 @@ type Strategy struct { // 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 the account's own RealizedPnL unchanged - // since the last bar observed: the entry never filled at all - // (rejected outright) — simply discarded, see + // - Flat->Flat, with entryFilledThisBatch false: the entry never + // filled at all (rejected outright) — simply discarded, see // discardUnfilledBracket. - // - Flat->Flat, with RealizedPnL changed: the entry filled *and* - // its own attached stop triggered within the same bar, + // - 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 lastRealizedPnL's own doc comment. + // review, blocker 1) — see entryFilledThisBatch's own doc + // comment. pendingInitialStop *num.Price - // lastRealizedPnL is this Strategy's own last-observed - // view.Account().RealizedPnL(), captured at the end of every OnBar - // call regardless of return path (issue #368, PR #369 review). It - // exists solely to detect the one lifecycle transition a bare - // Flat/Long position-side comparison cannot see at all: a bracket - // entry (order.IntentEnterWithStop) whose own attached stop - // triggers within the same bar the entry itself fills. - // sideLastBar stays order.Flat across both the bar before and the - // bar of such an episode — OnBar never observes order.Long in - // between — so without this signal, onExit would never run: - // everExited would stay false, ReEntryRule.OnExit would never be - // called, and the next entry would be incorrectly governed by - // InitialEntryRule instead of the configured ReEntryRule. - // RealizedPnL only changes when a position actually closes - // (opening one does not move it), so a change observed while - // pendingInitialStop is non-nil and side is Flat is conclusive - // proof a full bracket round trip occurred. + // 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. // - // A change is sufficient but not necessary (PR #369 re-review): a - // long bracket entry filling at this bar's own Open, already at or - // below its own protective stop, realizes exactly zero PnL — - // ADR-026's gap rule fills the stop at that identical Open price, - // so entry and exit share one price. OnBar's own Flat branch - // covers that case with a second, independent check - // (event.Bar.Open against the bracket's own stop) that needs no - // account state at all. Together the two checks are exhaustive for - // every fill outcome this codebase's own fill models produce - // today: a non-gap intrabar touch always realizes a strictly - // negative PnL (the stop necessarily fills below the entry's own - // Open), and a gap-through-Open fill always realizes exactly zero. - // What remains unprovable from account/bar state alone is the - // reverse case — a bracket entry rejected outright (no fill at - // all) on a bar whose Open independently happens to sit at or - // below whatever stop price would have been used. That coincidence - // is unrelated to genuine price/risk causality and is not resolved - // here; closing it fully would need a real fill-event signal (for - // example a wired FillHandler capability) rather than inference - // over account/bar snapshots. - lastRealizedPnL num.Money + // 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 @@ -260,13 +247,16 @@ 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) { - // Captured once, up front: used both by the Flat-branch same-bar- - // round-trip check below and to refresh lastRealizedPnL, via - // defer, on every return path (issue #368, PR #369 review) — - // including the warm-up early return, so lastRealizedPnL is never - // stale relative to what this bar actually observed. - currentRealizedPnL := view.Account().RealizedPnL() - defer func() { s.lastRealizedPnL = currentRealizedPnL }() + // 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 @@ -290,30 +280,22 @@ func (s *Strategy) OnBar(ctx context.Context, event strategy.BarEvent, view stra 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: - roundTrip, err := realizedPnLChanged(currentRealizedPnL, s.lastRealizedPnL) - if err != nil { - return nil, fmt.Errorf("smatrend: comparing realized pnl: %w", err) - } - // A RealizedPnL change alone misses one real case (PR #369 - // re-review): a long bracket entry filling at this bar's - // own Open, already at or below its own protective stop - // (ADR-026's gap rule then fills that stop at the - // identical Open price) realizes exactly zero PnL — entry - // and exit at the same price. event.Bar.Open is knowable - // directly from this bar's own data, independent of - // account state, and conclusively proves that outcome - // whenever it holds. - gappedThroughStop := event.Bar.Open.Cmp(*s.pendingInitialStop) <= 0 - if roundTrip || gappedThroughStop { - // The bracket entry filled and its own attached stop - // triggered within this same bar (PR #369 review, - // blocker 1) — invisible to sideLastBar, which never - // observed order.Long in between. - s.onExit(event.Bar) - } else { - s.discardUnfilledBracket() - } + // 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) @@ -346,24 +328,37 @@ func (s *Strategy) OnBar(ctx context.Context, event strategy.BarEvent, view stra } } -// realizedPnLChanged reports whether cur differs from prev, both -// account.Snapshot.RealizedPnL() values from the same account and -// therefore always the same currency (issue #368) — a mismatch would -// indicate the account itself changed underneath this Strategy, which -// is reported as an error rather than silently guessed at. -func realizedPnLChanged(cur, prev num.Money) (bool, error) { - cmp, err := cur.Cmp(prev) - if err != nil { - return false, err +// 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 } - return cmp != 0, 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 "no realized-pnl change" case. +// the Flat branch's own entryFilledThisBatch-false case. func (s *Strategy) discardUnfilledBracket() { s.pendingInitialStop = nil s.lastStop = nil diff --git a/strategy/smatrend/strategy_test.go b/strategy/smatrend/strategy_test.go index 0adae12..2a498b8 100644 --- a/strategy/smatrend/strategy_test.go +++ b/strategy/smatrend/strategy_test.go @@ -99,12 +99,11 @@ type testHarness struct { // AvgPrice Strategy reads via currentPositionAvgPrice (issue #349). avgPrice string // realizedPnL overrides the harness-reported account's own - // RealizedPnL ("0" otherwise) — needed to simulate a bracket entry - // whose own attached stop triggers within the same bar the entry - // fills (issue #368, PR #369 review): h.side stays order.Flat for - // that whole episode (OnBar never observes order.Long at all), so - // a nonzero RealizedPnL is the only way this harness can simulate - // the one signal Strategy itself uses to detect it. + // 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 } @@ -146,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 @@ -1028,9 +1047,10 @@ func TestStrategy_AboveSMAReEntryFiresOnTheVeryNextEligibleFlatBar(t *testing.T) // // 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 (see Strategy.lastRealizedPnL's -// own doc comment). h.realizedPnL simulates the one signal Strategy -// itself has to detect it. +// 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 @@ -1067,26 +1087,36 @@ func TestStrategy_BracketEntryStopSameBarStillTransitionsLifecycle(t *testing.T) // order.Long for this episode at all. h.side = order.Flat - // Bar 6: the entry's own fill bar. sma(99,102,102)=101, close - // (102) above it — still above the SMA, no fresh cross (already - // above since bar 5). - h.realizedPnL = "5" + // 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_BracketEntryGapExactlyToStopStillTransitionsLifecycle -// is PR #369's own re-review regression: RealizedPnL alone misses one -// real same-bar round trip — a long bracket entry filling at this -// bar's own Open, already at or below its own protective stop, gaps -// through at that exact price (ADR-026's own gap rule), so entry and -// exit realize exactly zero PnL and RealizedPnL never changes at all. -// h.realizedPnL is deliberately left at its zero default throughout — -// only event.Bar.Open's own relationship to the bracket's stop price -// can prove this case (see Strategy.lastRealizedPnL's own doc -// comment). -func TestStrategy_BracketEntryGapExactlyToStopStillTransitionsLifecycle(t *testing.T) { +// 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", @@ -1101,24 +1131,33 @@ func TestStrategy_BracketEntryGapExactlyToStopStillTransitionsLifecycle(t *testi } // Bar 5: entry decision — a bracket order.IntentEnterWithStop with - // stop 99.33 (sma(99,102's own bar4/5)... see the sibling test's - // own identical bar-5 comment: sma(100,99,102)=100.33333333, - // stop=100.33333333*0.99=99.33). + // 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 // see the sibling test's own identical note. - - // Bar 6: the entry's own fill bar gaps down — Open (99) already at - // or below the 99.33 bracket stop. RealizedPnL stays "0" for this - // entire test; only the gap-through-Open check can prove this bar - // was a real round trip. Still above the SMA (sma(99,102,102)=101, - // close 102 above it) with no fresh cross, so above-sma fires - // immediately if the lifecycle correctly transitioned. + 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}) - require.Len(t, intents, 1, "the lifecycle must have transitioned to post-exit/re-entry mode even though RealizedPnL never changed") - assert.Equal(t, order.IntentEnterWithStop, intents[0].Kind) + 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