EQ-368: strategy/smatrend uses EnterWithStop for initial probation protection by rustyeddy · Pull Request #369 · rustyeddy/trader

rustyeddy · 2026-09-11T19:01:43Z

What changed

Wires strategy/smatrend's probation-trend ExitRule to the order.IntentEnterWithStop bracket capability PR #367 (ADR-059) added, closing the entry-fill-bar protection gap PR #350 review identified and #367 deliberately left as strategy-specific wiring out of its own scope.

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's 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 its ratchet-floor state from it instead of nil, preserving the never-loosen invariant from the very first bar. trailingStopExitRule/smaCrossExitRule ignore it — neither implements InitialStopProvider.
Strategy.onFlat now emits order.IntentEnterWithStop when the configured ExitRule implements InitialStopProvider, or a plain order.IntentEnter otherwise — both the initial entry and every re-entry get identical protection.

Why

Issue #368 (the follow-up #351/#367/ADR-059 explicitly deferred). The SMA Long Hold playbook's probation-trend rule exists specifically for a tight stop immediately on entry; without this, the entry fill bar itself traded unprotected.

How tested

Replaced TestSMATrend_ProbationEntryBarIntrabarGapIsAKnownLimitation (which only documented the gap) 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 fill bar itself — proving the fix, not just documenting the old bug.
Added TestSMATrend_PlainEntryStillWorksWithoutInitialStopProvider proving the default trailing-stop rule is unaffected.
Updated two existing end-to-end Strategy tests to expect IntentEnterWithStop with the correct pre-fill stop price at both initial entry and re-entry.
Verified the regression is meaningful: temporarily forced a plain Enter always, confirmed the new test fails (0 bracket intents, only 1 of 2 expected trades — reproducing the exact old known-limitation failure mode), then restored.
go build/go vet/gofmt -l/go test -race ./... all clean.

Documentation

strategy/smatrend/doc.go: documents the new capability.
docs/arch/adr-059-bracket-entry-with-stop.org: records this issue's completion of the deferral ADR-059 left open, as an additive note — its own historical Context/Decision text is left unchanged per this repo's non-rewrite convention for an Accepted ADR.
No new ADR: this is strategy-specific wiring onto an already-Accepted framework capability, not a new architectural decision.

Scope

Explicitly not addressed here (per issue #368's own scope): live/async-broker bracket semantics (#366) and rerunning the SMA Long Hold reference research (#365) — both remain tracked separately.

Closes #368

🤖 Generated with Claude Code

…rotection 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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015fVsVuQCgkrhiYaXLxyUF3

Copilot

🟡 Changes recommended

Unresolved compatibility, lifecycle, versioning, test coverage, and ADR documentation findings remain.

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

Pull request overview

This PR adds pre-fill protective-stop brackets for probation-trend entries and re-entries in strategy/smatrend.

Changes:

Adds initial-stop provisioning and bracket entry intents.
Updates lifecycle and regression tests with a February EURUSD fixture.
Documents the strategy-specific wiring and ADR follow-up.

File summaries

File	Reviewed changes
`strategy/smatrend/testdata/raw/oanda/EURUSD/2024/02/EURUSD-2024-02-d1.csv`	Adds dedicated regression data.
`strategy/smatrend/strategy.go`	Builds bracket or plain entry intents.
`strategy/smatrend/strategy_test.go`	Updates entry intent expectations.
`strategy/smatrend/regression_test.go`	Adds fill-bar protection regressions.
`strategy/smatrend/exitrule.go`	Adds initial-stop provisioning and state seeding.
`strategy/smatrend/exitrule_test.go`	Updates exit-rule tests.
`strategy/smatrend/doc.go`	Documents bracket-entry behavior.
`docs/arch/adr-059-bracket-entry-with-stop.org`	Records the deferred strategy wiring.

Review details

Suppressed comments (3)

docs/arch/adr-059-bracket-entry-with-stop.org:223

ADR-059 is marked Accepted, and the registry's partial-amendment rules prohibit editing its Context, Decision, or Consequences; they require a separate accepted amendment plus an Amended by ADR-NNN line under Status. This parenthetical changes the accepted ADR's Consequences while claiming the non-rewrite convention is followed. Please remove this historical rewrite and use the prescribed amendment process, or leave ADR-059 unchanged and keep the issue-specific status in the strategy documentation.

  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

strategy/smatrend/exitrule.go:265

The new non-nil path is not actually covered by a test that observes a surviving bracket entry: the end-to-end fixture stops both entries on their fill bars, so OnEntry is never called, while the strategy-level lifecycle tests use a higher next-bar SMA stop and would pass even if this assignment still reset to nil. Add a regression with a bracket entry that survives its fill bar but has a lower next SMA, and assert that no loosening AdjustStop is emitted.

	r.probationStop = initialStop

strategy/smatrend/strategy.go:363

The bracket stop is never recorded in s.lastStop. If the position survives its fill bar but the first OnLongBar computes a stop no higher than the seeded bracket floor, onLong emits no adjustment and lastStop remains nil; when the resting initial stop later triggers, onExit falls back to the exit bar's close instead of the intended stop. That corrupts exit-price-based re-entry rules such as reclaim-exit-price. Record this initial stop as the last intended stop alongside pendingInitialStop (later ratchets will overwrite it).

	s.pendingInitialStop = &stop
	return in, &stop, nil

Files reviewed: 8/8 changed files
Comments generated: 3
Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

+	// 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)


 				return nil, fmt.Errorf("smatrend: reading entry price: %w", err)
 			}
-			s.exitRule.OnEntry(event.Bar, entryPrice)
+			s.exitRule.OnEntry(event.Bar, entryPrice, s.pendingInitialStop)


+	action := "enter-long"
+	if initialStop != nil {
+		action = "enter-long-with-stop"
+	}


rustyeddy · 2026-09-11T19:35:19Z

Review of PR #369 at 94b78e4 against #368.

The basic direction is right: probation-trend can compute its initial stop pre-fill, EnterWithStop is the right mechanism, and the no-lookahead intent construction looks sound. But there are a few substantive lifecycle/correctness issues that need fixing before merge.

Blocker 1: same-bar fill+stop can bypass the strategy's exit transition entirely

This is the most important issue.

OnBar only calls onExit when side == Flat && sideLastBar == Long. With the new bracket semantics, an entry can fill at T+1 open and its attached stop can also trigger during T+1 intrabar before Strategy.OnBar(T+1) runs. In that case the strategy observes Flat both before and after the whole episode:

sideLastBar = Flat
entry fills at T+1 open
stop triggers during T+1 intrabar
OnBar(T+1) observes side = Flat

So neither OnEntry nor onExit runs. everExited remains false, ReEntryRule.OnExit is never called, and the next entry is incorrectly governed by InitialEntryRule instead of the configured re-entry rule.

The new regression currently masks this because the fixture uses fresh-cross in a way that does not expose the lifecycle distinction. We need an explicit regression proving that a same-fill-bar stop-out transitions the strategy into the post-exit/re-entry lifecycle, not back to "never entered" state.

This probably requires tracking a pending bracket entry/filled-and-stopped outcome explicitly rather than inferring the whole lifecycle solely from view position snapshots.

Blocker 2: initial bracket stop must become `lastStop`

buildEntryIntent stores the bracket stop in pendingInitialStop, but not in lastStop.

If the bracket entry survives its fill bar and the first subsequent OnLongBar does not ratchet the stop higher, lastStop remains nil even though a real resting stop exists. If that stop later triggers, onExit falls back to the exit bar Close instead of the intended stop level. That corrupts exit-price-based re-entry rules such as reclaim-exit-price.

When the bracket intent is constructed, the intended stop should also become the current stop reference (lastStop), and later ratchets can overwrite it as they already do.

Blocker 3: preserve the exported `ExitRule` contract if possible

ExitRule is exported, and changing

OnEntry(entryBar, entryPrice)

to

OnEntry(entryBar, entryPrice, initialStop)

breaks every external/custom implementation even though InitialStopProvider is explicitly optional.

I would preserve the existing required interface and add an optional hook for the seeded-stop case, e.g. a second capability such as OnEntryWithInitialStop(...), or otherwise keep the optional capability optional all the way through. The current change makes unrelated custom rules pay for a probation-trend-only enhancement.

Blocker 4: strategy version should change

This is a real behavior change: persisted signal semantics change (enter-long-with-stop), entry-bar stop behavior changes, and resulting runs are not directly comparable with prior smatrend v1 runs. If ADR-044 uses Descriptor.Version as the discriminator for logic changes, bump the strategy version and update the affected tests/docs.

Also fix before merge

Add a regression where a bracket entry survives the fill bar and the next SMA-derived probation stop would be lower, proving the seeded initial stop floor prevents loosening.
Do not rewrite accepted ADR-059 historical sections if the ADR registry's amendment convention forbids that. Keep the completion note in strategy docs or add the prescribed amendment reference instead.

Once those are addressed, I think the PR will be in good shape. The core architectural choice—optional pre-fill stop provider + EnterWithStop for probation-trend—is sound.

…ted-interface break, version bump 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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015fVsVuQCgkrhiYaXLxyUF3

rustyeddy · 2026-09-11T19:46:06Z

Fixed in 6e4fb5d. Addressing each finding:

Blocker 1 (same-bar fill+stop bypasses the exit transition entirely): this was real — a bracket entry's own attached stop can trigger within the same bar the entry fills, so sideLastBar never observes order.Long, onExit never runs, everExited stays false, and the next entry is incorrectly governed by InitialEntryRule instead of the configured ReEntryRule. Fixed by tracking view.Account().RealizedPnL() bar over bar: it only changes when a position actually closes, so a change observed while pendingInitialStop is set and side is Flat is conclusive proof of a same-bar round trip, and now drives the same onExit path the classic transition uses. New regression TestStrategy_BracketEntryStopSameBarStillTransitionsLifecycle proves it behaviorally (above-sma re-entry fires immediately on the fill bar itself, which fresh-cross never would), not 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 for the bracket path, so a position that survives its fill bar with no ratcheting AdjustStop on the next OnLongBar call still has the correct exit-price reference if its resting stop later triggers. Both fields roll back together via discardUnfilledBracket if the entry never filled.

Blocker 3 (exported ExitRule.OnEntry signature break): reverted OnEntry to its original two-argument signature. Added a separate optional capability, InitialStopSeeder.SeedInitialStop(num.Price), called immediately after OnEntry only when a bracket entry was actually used — OnEntry's own default reset runs first, then SeedInitialStop overrides just the one field it cares about. No external/custom ExitRule is affected.

Blocker 4 (strategy version): bumped smatrend.Version from "v1" to "v2" per ADR-044 — persisted signal semantics and entry-bar stop behavior both changed.

Also fixed: added TestProbationTrendExitRule_SeedInitialStopPreventsLoosening (Copilot's suppressed-comment finding — same root cause as blocker 2: a bracket entry surviving its fill bar with a lower next-bar SMA stop must not loosen below the seeded floor). Reverted the ADR-059 edit from the previous commit — Copilot correctly flagged it as rewriting an Accepted ADR's Consequences outside the registry's amendment process; ADR-059 is back to its exactly-merged text, and the issue #368 completion note now lives only in strategy/smatrend/doc.go/exitrule.go's own doc comments.

go build/go vet/gofmt -l/go test -race ./... all clean.

rustyeddy · 2026-09-11T19:59:23Z

Re-review at head 6e4fb5d: the prior findings are addressed well — ExitRule.OnEntry compatibility is restored via optional InitialStopSeeder, lastStop is seeded with the bracket stop, smatrend.Version is now v2, the no-loosening regression was added, and the ADR-059 historical rewrite was removed.

I found one remaining correctness edge case in the new same-bar round-trip detection before I’d call this merge-ready.

OnBar currently decides that a pending bracket filled-and-stopped in the same bar only when account-level RealizedPnL() changed. That is not guaranteed for every genuine same-bar round trip. In particular, a long bracket can fill at the T+1 open after a gap below the precomputed protective stop; ADR-026’s stop gap rule then also fills the protective sell stop at that same (worse) bar open. Entry and exit can therefore occur at the identical price, producing exactly zero realized P&L. In that case:

side == Flat
pendingInitialStop != nil
RealizedPnL() is unchanged
discardUnfilledBracket() runs
the real fill+stop episode is mistaken for a never-filled/rejected entry
onExit() is skipped, so everExited / ReEntryRule.OnExit are wrong again.

The simulator already explicitly supports gap-through stops filling at the bar open, so this is not just theoretical.

I would add a regression with a bracket entry whose fill-bar Open is already below its long protective stop and verify lifecycle transitions to re-entry even though realized P&L is zero. More generally, using account P&L as a fill/exit detector is an indirect proxy; if there is an existing per-run fill/closed-trade signal available to the strategy/view, that would be a stronger discriminator. If there isn’t, we should at least make this zero-P&L case unambiguous before merge.

Aside from that edge, the latest changes look good.

…re-review) 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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015fVsVuQCgkrhiYaXLxyUF3

rustyeddy · 2026-09-11T20:05:29Z

Fixed in a5c957a.

You're right — the zero-PnL gap case was real and RealizedPnL alone couldn't catch it. Added a second, independent check that needs no account state at all: event.Bar.Open compared directly against the bracket's own stop price. A gap-through-Open fill (ADR-026's own gap rule: the stop fills at that same Open, identical to the entry's own fill price) is now caught regardless of what RealizedPnL does.

The two checks together are exhaustive for every fill outcome this codebase's fill models actually produce: a non-gap intrabar touch always realizes a strictly negative PnL (caught by the RealizedPnL check — the stop necessarily fills below the entry's own Open), and a gap-through-Open fill always realizes exactly zero (caught by the new Open check).

New regression TestStrategy_BracketEntryGapExactlyToStopStillTransitionsLifecycle leaves RealizedPnL at its zero default throughout, so only the new check can make it pass. Verified meaningful by disabling it and confirming the test fails.

One residual, narrower gap I want to be upfront about rather than paper over: a bracket entry rejected outright (no fill at all) on a bar whose Open independently happens to sit at or below wherever the stop would have been is still unprovable from account/bar state alone — that specific coincidence is unrelated price/risk causality colliding, and I don't think it's closeable without a real fill-event signal (a wired FillHandler capability, which doesn't exist yet in backtest.Scheduler — a materially larger change). I've documented this explicitly in lastRealizedPnL's own doc comment rather than leaving it implicit. Happy to file a follow-up issue for a proper FillHandler-based fix if you'd like it tracked, but didn't want to unilaterally expand this issue's scope to build that now.

go build/go vet/gofmt -l/go test -race ./... all clean.

rustyeddy · 2026-09-11T20:17:31Z

Re-review at a5c957a: the zero-P&L gap-through case is now covered, and the new regression is meaningful. The prior blockers remain addressed.

I do still see one correctness hole before merge, which your latest note also identifies: a rejected/unfilled bracket can be misclassified as a same-bar round trip if the next bar's Open happens to be at/below the hypothetical protective stop.

That is not just a documentation caveat, because it changes strategy state:

pendingInitialStop != nil from the attempted bracket
entry is rejected, account remains Flat, realized P&L unchanged
bar Open happens to be <= the stop
the new gap test infers a fill+stop anyway
onExit() runs even though no trade occurred
everExited becomes true and the strategy switches from InitialEntryRule to ReEntryRule incorrectly

So the current two-signal inference (PnL change OR Open through stop) is exhaustive for filled bracket outcomes, but it cannot distinguish a zero-PnL round trip from a never-filled/rejected entry. That distinction is semantically important to the state machine.

I would not add another heuristic here. The clean fix is to give smatrend an authoritative indication that the pending bracket entry actually filled (or that a fill occurred and was later closed), likely through a small fill/order-outcome signal from the scheduler/view. If wiring that capability is too large for this PR, I would rather keep #368 open and split that infrastructure into a tightly-scoped prerequisite issue than merge a known path that can fabricate an exit.

Everything else in the latest changes looks good: source compatibility restored, lastStop seeded, v2 versioning correct, no-loosening regression added, accepted ADR left untouched, and the real same-bar stop lifecycle path is now covered.

rustyeddy · 2026-09-11T20:19:40Z

Agreed — this needs an authoritative signal, not a third heuristic, and I won't merge a path that can fabricate an exit.

Filed #370 as the prerequisite issue, scoped to exactly this gap: giving Strategy an authoritative per-bar fill-outcome signal (rather than inferring from account/bar state), with three design options sketched (wiring the already-documented-but-unwired strategy.FillHandler capability, extending strategy.View with a fill accessor, or a narrower smatrend-only signal) and explicit acceptance criteria including replacing the two heuristics in strategy/smatrend/strategy.go once it lands.

This PR (#369) stays open and unmerged pending that issue — not closing #368 either, since everything else in this PR (the bracket wiring itself, blockers 1-4, the no-loosening regression, the zero-PnL gap case) is real, tested, and correct; it's specifically the rejected-entry-plus-coincidental-gap residual that needs #370's own work first.

I'll pause here rather than unilaterally pick a design option and start wiring a public strategy.View/strategy.Strategy interface change into this PR — that's exactly the kind of genuinely-unresolved architecture call this repo's own CONTRIBUTING.org asks to stop and flag rather than guess at.

rustyeddy · 2026-09-11T20:22:52Z

Re-check of PR #369: the head is still a5c957a4f345afe9589ff55c2969c2a3c24f2c98, the same commit I reviewed last time, so there are no newer code changes for me to validate yet.

The remaining blocker therefore still stands: a bracket entry that is rejected/unfilled can be misclassified as a same-bar round trip when the following bar's Open is at/below the hypothetical stop. Because that path mutates everExited / ReEntryRule.OnExit, I would not merge until the strategy gets an authoritative fill/order-outcome signal (or an equivalent unambiguous mechanism) and a regression proving rejected brackets cannot trigger onExit().

Once a new commit lands, I can re-review immediately.

rustyeddy · 2026-09-11T20:54:08Z

Re-review: PR #369 is still at head a5c957a4f345afe9589ff55c2969c2a3c24f2c98, so there are no new code changes beyond the version already reviewed. The remaining correctness blocker is unchanged: rejected/unfilled brackets can still be misclassified as same-bar round trips when the bar Open happens to be at/below the hypothetical stop. Issue #370 is now tracking the authoritative fill/outcome signal needed to remove that ambiguity. I would keep #369 open/unmerged until #370 lands and this PR is updated to use that signal, then rerun the targeted regressions.

Copilot AI lite review requested due to automatic review settings September 11, 2026 19:01

Copilot started reviewing on behalf of rustyeddy September 11, 2026 19:02 View session

Copilot AI reviewed Sep 11, 2026

View reviewed changes

rustyeddy mentioned this pull request Sep 11, 2026

strategy: authoritative per-bar fill outcome signal for Strategy (needed by smatrend bracket round-trip detection) #370

Open

5 tasks

Navigation Menu

EQ-368: strategy/smatrend uses EnterWithStop for initial probation protection - #369

EQ-368: strategy/smatrend uses EnterWithStop for initial probation protection#369
rustyeddy wants to merge 3 commits into
mainfrom
feature/368-smatrend-enter-with-stop

rustyeddy commented Sep 11, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

rustyeddy commented Sep 11, 2026

Uh oh!

rustyeddy commented Sep 11, 2026

Uh oh!

rustyeddy commented Sep 11, 2026

Uh oh!

rustyeddy commented Sep 11, 2026

Uh oh!

rustyeddy commented Sep 11, 2026

Uh oh!

rustyeddy commented Sep 11, 2026

Uh oh!

rustyeddy commented Sep 11, 2026

Uh oh!

rustyeddy commented Sep 11, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

rustyeddy commented Sep 11, 2026

What changed

Why

How tested

Documentation

Scope

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Suppressed comments (3)

Uh oh!

rustyeddy commented Sep 11, 2026

Blocker 1: same-bar fill+stop can bypass the strategy's exit transition entirely

Blocker 2: initial bracket stop must become lastStop

Blocker 3: preserve the exported ExitRule contract if possible

Blocker 4: strategy version should change

Also fix before merge

Uh oh!

rustyeddy commented Sep 11, 2026

Uh oh!

rustyeddy commented Sep 11, 2026

Uh oh!

rustyeddy commented Sep 11, 2026

Uh oh!

rustyeddy commented Sep 11, 2026

Uh oh!

rustyeddy commented Sep 11, 2026

Uh oh!

rustyeddy commented Sep 11, 2026

Uh oh!

rustyeddy commented Sep 11, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Blocker 2: initial bracket stop must become `lastStop`

Blocker 3: preserve the exported `ExitRule` contract if possible