EQ-383: SMA Long Hold out-of-tree reference implementation

[rustyeddy]

Closes #383 (scoped per discussion — see below).

What changed

Adds examples/sma-long-hold, a real, non-trivial strategysdk.Strategy proving Strategy Protocol v1 against something more than a hello-world guest: SMA/indicator state, a genuine cross-above entry decision, and a ratcheting protective stop.

Scoping decisions (confirmed with Rusty before implementation)

• Location: lives in this repo's examples/ tree (like examples/strategysdk-minimal), not a separate GitHub repository — the issue's own "separate repository" suggestion wasn't taken.
• Rule coverage: reproduces only strategy/smatrend's own default configuration (trailing-stop/fresh-cross/fresh-cross — EQS-01's original, only mechanism before issue smatrend: pluggable ExitRule/ReEntryRule (SMA-trend v2) #347/smatrend: implement full SMA Long Hold state machine (FLAT/PROBATION/TRENDING) #349 made the exit/re-entry/initial-entry rules pluggable), not the full pluggable-rule surface. For exactly this combination, the entry decision reduces to one condition (crossedAbove) regardless of position history — see main.go's own doc comment — so no re-entry-rule state machine or OnFill capability is needed at all.
• Equivalence expectation: "we will match the results against the in-tree version with the expectation that they produce identical results" — this PR includes that exact test.

Implementation

• examples/sma-long-hold/main.go: TRADER_STRATEGY_CONFIG-driven (JSON; defaults to EUR/USD H1, sma_period 20, trailing_stop_percent 0.10). Reuses indicator.SMA directly (a pure analytical package, no broker/execution surface) for bit-for-bit numerical agreement with strategy/smatrend's own use of it, rather than a hand-rolled second implementation. boundary_test.go mechanically enforces "no broker/risk/execution/pipeline/strategy/backtest/service/cmd/adapters access" (the issue's explicit constraint).
• cmd/trader/backtest/sma_long_hold_equivalence_test.go (package backtest, internal — needs environmentFactory/nextBarOpenPriceSource): drives strategy/smatrend in-process and examples/sma-long-hold as a real external.Launch subprocess through identical service/backtest compositions over identical canonical data, then asserts their closed/open trades (side, timestamps, RealizedPnL, Costs — deliberately excluding IDs, which are never expected to agree between two independent runs) and final account equity/RealizedPnL are identical.
• A new engineered fixture (testdata/raw/oanda/EURUSD/2024/06) was needed: the repo's other EURUSD months happen to stay continuously above their own short-period SMA once warmed up, so fresh-cross never actually fires against them and the comparison would have been trivial (both runs doing nothing). The new fixture engineers a below-SMA warm-up, a sharp cross-above, three bars ratcheting the trailing stop, then a sharp reversal that stops the position out via the real broker-side resting-order mechanism (ADR-026) — a genuine entry/ratchet/exit episode.
• config/arch_test.go: examples/sma-long-hold exempted from the no-os.Getenv rule, same reason strategysdk/cmd already are.
• .gitignore: guards against go build ./...-from-root writing stray example/cmd binaries into the repo root (a real accident hit while verifying this branch).

Testing

• examples/sma-long-hold gets direct, fast unit coverage (config validation, crossState, a full synthetic entry/ratchet/exit OnBar walk-through, Start, newFromEnv) independent of the slower cross-process equivalence proof — 89.8% package coverage.
• Full go test ./... green under -race, golangci-lint clean (only the 3 known pre-existing unrelated research-runs/ errcheck findings), no stray processes/sockets after the test run.

🤖 Generated with Claude Code

[Copilot]

Copilot review overview

🟡 Changes recommended

Three moderate findings remain unresolved in environment handling and configuration validation.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 2

Open (2)

• Reject identical base and quote currencies · New
• Reject non-positive or unity retention rates · New

What changed in this PR

Adds a configurable SMA long-hold strategysdk reference strategy and verifies equivalence with the in-tree implementation.

Changes:

• Implements SMA cross-entry and ratcheting trailing-stop behavior.
• Adds unit, boundary, fixture, and cross-process equivalence tests.
• Updates documentation, architecture exemptions, and build-output ignores.

File	Summary
examples/​sma-long-hold/​README.md	Usage and configuration documentation
examples/​sma-long-hold/​main.go	External strategy implementation
examples/​sma-long-hold/​main_test.go	Unit and behavior coverage
examples/​sma-long-hold/​boundary_test.go	Import-boundary enforcement
config/​arch_test.go	Guest environment exemption
cmd/​trader/​backtest/​testdata/​raw/​oanda/​EURUSD/​2024/​06/​EURUSD-2024-06-h1.csv	Engineered trading fixture
cmd/​trader/​backtest/​sma_long_hold_equivalence_test.go	Cross-process equivalence test
.gitignore	Example build-output exclusions

---

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

[rustyeddy]

Reviewed PR #394 (EQ-383: SMA Long Hold out-of-tree reference implementation). The overall direction is strong: this is a genuinely non-trivial external strategy, it stays behind the intended SDK boundary, and the cross-process equivalence test is much more valuable than another transport-only example.

I see three things to tighten before merge:

1. Reject base == quote in the external strategy config. Copilot's finding is valid. num.ParseCurrency validates each currency independently, but instrument.CurrencyPairID(base, quote) can produce a zero/invalid ID for something like USD/USD. The in-tree side constructs a real CurrencyPair and would reject that semantic invalidity earlier. Use instrument.NewCurrencyPair(base, quote) (or an explicit equality check) and derive the ID from that validated pair. Add a config regression test.

2. Match smatrend's strict trailing-stop range validation. This reference claims equivalence with the in-tree default configuration, but newLongHold currently only parses the rate; it does not enforce 0 < trailing_stop_percent < 1. strategy/smatrend.Config.Validate() explicitly enforces both bounds for the default trailing-stop rule. Negative, zero, 1, or >1 values therefore behave differently out-of-tree and can create nonsensical stop geometry. Mirror those exact bounds and test at least 0, 1, a negative value, and >1.

3. The equivalence assertion is slightly too weak for the claim “identical results.” tradeKey compares side/timestamps/PnL/costs but omits the trade's Listing/instrument, and the final account check compares only equity, realized PnL, and the number of positions:

assert.Equal(t, len(inTree.Account.Positions()), len(external.Account.Positions()))

For an equivalence gate, I would compare the economically meaningful final position contents too (instrument, side, average price, and quantity if present in the account position type), and include the trade's listing/instrument in tradeKey. IDs should still be excluded, as you already do. Otherwise two runs could theoretically end with the same aggregate money values and position count while differing in actual held state.

The engineered fixture and the requirement that the scenario actually produce a closed trade are both good choices; they make this a meaningful protocol/strategy equivalence test rather than a trivial no-op comparison.

The issue originally asked for a separate repository and broader re-entry/OnFill coverage, but the PR explicitly records that scope was narrowed by prior discussion, so I'm not treating those as review blockers here.

[rustyeddy]

Pushed 485f0e1 addressing all three findings:

1. base == quote not rejected (Copilot + Rusty M0-05 Add baseline GitHub Actions CI #1) — newLongHold now constructs the instrument via instrument.NewCurrencyPair(base, quote) instead of instrument.CurrencyPairID directly, so an invalid pair like USD/USD is rejected explicitly, matching what constructing a real in-tree strategy/smatrend Strategy against the same config would already reject one layer up. New regression test case ("base equals quote").

2. Trailing-stop range not validated (Copilot + Rusty M0-01 Expand the project README #2) — newLongHold now enforces 0 < trailing_stop_percent < 1, mirroring strategy/smatrend.Config.Validate's own exact bounds for the "trailing-stop" rule byte-for-byte. New test cases cover 0, a negative value, 1, and 1.50.

3. Equivalence assertion too weak (Rusty M0-02 Complete the initial ADR records #3) — tradeKey now includes the trade's own instrument (previously two runs trading different instruments could still compare equal), and the final open-position check now compares a new positionKey (instrument/side/quantity/avg-price) instead of just the position count.

Verification: go build/vet clean, gofmt clean, full go test ./... green under -race, golangci-lint clean (only the 3 known pre-existing unrelated research-runs/ errcheck findings), no stray processes. examples/sma-long-hold coverage 90.5%.

[rustyeddy]

Rereviewed latest head 485f0e1.

All three findings from the previous review are addressed correctly:

• instrument construction now goes through instrument.NewCurrencyPair, so semantically invalid pairs such as USD/USD fail locally before Handshake;
• trailing_stop_percent now enforces the same strict 0 < value < 1 bounds as strategy/smatrend.Config.Validate(), with regression coverage for zero, negative, one, and >1 values;
• the equivalence gate now includes trade instrument and compares full economically meaningful open-position state (instrument/side/quantity/average price), rather than only position count.

I rechecked the surrounding strategy logic and the engineered entry/ratchet/stop-out fixture as well. For the deliberately scoped default trailing-stop / fresh-cross / fresh-cross scenario, the external implementation and the equivalence harness now look consistent with the in-tree behavior.

I don't see another correctness blocker. PR #394 looks merge-ready from my review.

Two tiny test-hygiene nits, neither worth holding the PR:

• the once-built temporary binary directory is not explicitly removed, so the test process can leave a small temp directory behind;
• the equivalence test's cleanup calls Process.Stop(t.Context()); depending on Go's testing-context cleanup timing, using context.Background() there would make the intended shutdown behavior less coupled to test cancellation.

GitHub currently exposes no commit-status entries for this head through the connector, so the merge-ready assessment is based on the PR code/tests plus the reported local verification.

[rustyeddy]

Pushed a58ec7f addressing both nits from the merge-ready re-review:

• buildSMALongHoldBinary now builds into t.TempDir() instead of a package-level sync.Once-cached directory nothing ever removed.
• The equivalence test's process.Stop is now deferred on context.Background() instead of t.Context(), decoupling its shutdown grace period from test-context cancellation timing.

Verification: full go test ./... green under -race, golangci-lint clean (only the 3 known pre-existing unrelated research-runs/ errcheck findings), no stray processes.