EQ-384: milestone-completion equivalence gate for SMA Long Hold

[rustyeddy]

Closes #384.

What changed

Upgrades cmd/trader/backtest/sma_long_hold_equivalence_test.go (from #383/PR #394) into issue #384's own milestone-completion gate for External Strategies v1 — "a hello-world RPC round trip alone is not sufficient proof."

Both strategy/smatrend (in-process) and examples/sma-long-hold (a real external.Launch subprocess) run through the identical service/backtest composition, and are now compared across every dimension issue #384's "compare at minimum" list names:

• strategy descriptor / data requirements
• every journaled record kind this scenario produces — RunStarted, Intent, Proposal, Decision, Request, ReplaceRequest, Order, Fill, Trade, Signal, RunCompleted — compared kind-by-kind and field-by-field, in order, so intent ordering and correlation relationships are actually proven, not just final aggregates
• closed/open trades and final account state (carried over from external-strategy: out-of-tree SMA Long Hold reference implementation #383)
• the equity curve
• manifest universe/dataset identity

Key design points

• ID normalization: every opaque identifier (RunID, AccountID, IntentID, OrderID, FillID, EventID/CorrelationID/CausationID) is normalized through a new idNormalizer — a close port of backtest/determinism_test.go's own identical type from issue M5-15: Add deterministic backtest regression/golden suite #223. That suite proved two independently-seeded runs of the same strategy share a causal shape despite different literal ULIDs; the identical technique applies unchanged here to two different Strategy implementations of the same trading logic.
• Explicit, documented exclusions: every intentionally different, transport-only field — each side's own Descriptor.Name/Version and the resulting Manifest.StrategyName/StrategyParameters/journal.Signal.Strategy — is named explicitly in the test's own doc comment and excluded from comparison, not silently skipped (issue external-strategy: deterministic equivalence test for in-tree vs out-of-tree SMA Long Hold #384's own "explicitly normalized/documented" requirement).
• Pinpointing the first divergence: sub-tests (t.Run per dimension) plus per-record/per-field require/assert.Equalf with the record index in every message.
• examples/sma-long-hold/main.go now also emits journal.Signal decision evidence on entry/adjust-stop, mirroring strategy/smatrend.Strategy.recordSignal's own Values map shape and keys exactly — needed for the new signal comparison to be meaningful rather than trivially empty on both sides.

Testing

Full go test ./... green under -race, golangci-lint clean (only the 3 known pre-existing unrelated research-runs/ errcheck findings), no stray processes/sockets. examples/sma-long-hold coverage 91.5%.

🤖 Generated with Claude Code

[rustyeddy]

Reviewed PR #395 (EQ-384: milestone-completion equivalence gate for SMA Long Hold). This is the right direction for the milestone gate: it compares the actual causal/journal shape rather than only final P&L, and adding signal evidence to the external reference makes that comparison meaningful.

I see two issues to address before merge:

1. The journal semantic comparison is materially weaker than the determinism test it is described as porting.

Several fields that backtest/determinism_test.go already treats as part of the canonical semantics are omitted here:

• KindDecision: only Allowed and rule names are compared; decision-level violations/warnings and each RuleResult's violations/warnings are not.
• KindOrder: AcceptedLimitPrice and AcceptedQuantity are not compared.
• KindFill: commission, BrokerOrderID, and the Fill's own Metadata/correlation/causation are not compared.
• KindRequest/other order payloads should be checked against the current canonical comparer as well; this gate should not silently become a looser fork as those types evolve.

That means the new milestone gate can pass even if risk evidence, accepted order terms, commissions, or fill causal metadata diverge between the in-tree and external paths. Since the file explicitly says this is a close port of the existing determinism comparator, I would copy the current semantic checks from backtest/determinism_test.go wholesale and then add the two things unique here: KindReplaceRequest and KindSignal. This also reduces the chance that the two equivalence/determinism gates drift semantically.

2. The required manifest strategy/config identity is currently excluded rather than normalized and compared.

Issue #384 explicitly lists “manifest strategy/config identity” under “Compare at minimum.” This test deliberately excludes Manifest.StrategyName, StrategyParameters, and ConfigDigest, and compareRecordSemantics does not compare RunStarted.Header at all. On the external side, svc.Run is also invoked without StrategyParameters, while the in-tree side passes strat.Config().

So the test proves universe/dataset identity, but it does not prove the manifest/config provenance requirement. In fact, a regression that completely drops or changes the external run's strategy parameters would still pass this section.

I agree the literal strategy names/versions cannot be equal because these are intentionally separate implementations. But that calls for normalization, not omission. I would define a normalized semantic strategy identity for this gate — e.g. SMA period + trailing-stop percent + instrument + interval + selected rule names — and assert both manifests encode that same semantic configuration, while separately documenting/name-normalizing the implementation identity. At minimum, ensure the external RunRequest.StrategyParameters contains the external config actually used and compare the two parameter sets after normalization. Then compare the normalized RunStarted.Header/manifest too.

A smaller follow-on: the manifest dataset comparison currently checks provider/instrument/interval/revision, while the canonical determinism gate also checks span, basis, and raw fingerprint. For a milestone-completion equivalence gate, I would carry those over as well so “identical dataset” means the full semantic dataset identity, not just its digest.

The signal/correlation addition looks good, and the required-kind guard is a strong idea. Once the gate compares the full canonical payload semantics and normalized manifest/config identity, it will be a much stronger Milestone 19 completion criterion.

[Copilot]

Copilot review overview

🟡 Changes recommended

Several observable journal, intent, execution, manifest, and account fields remain unchecked by the equivalence gate.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 6

Open (6)

• Compare metadata in all causal payloads · New
• Compare intent kind fields and payload metadata · New
• Compare complete proposal semantics and metadata · New
• Compare aggregate and per-rule diagnostics · New
• Compare accepted order prices and quantity · New
• Compare complete fill fields and metadata · New

What changed in this PR

This PR strengthens the SMA Long Hold equivalence test into an External Strategies v1 milestone gate.

Changes:

• Adds correlated signal evidence to the external strategy.
• Expands comparisons across journals, trades, equity, descriptors, and datasets.
• Updates documentation and unit tests.

File	Summary
examples/​sma-long-hold/​README.md	Documents expanded equivalence coverage.
examples/​sma-long-hold/​main.go	Emits correlated entry and stop-adjustment signals.
examples/​sma-long-hold/​main_test.go	Tests emitted signal evidence.
cmd/​trader/​backtest/​sma_long_hold_equivalence_test.go	Adds comprehensive cross-implementation comparisons.

---

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

[rustyeddy]

Pushed 8acb407 addressing both findings (Rusty's two points, and Copilot's 6 findings — all aligned with Rusty's point 1):

1. Weaker journal comparison than the determinism test it ports (Rusty M0-05 Add baseline GitHub Actions CI #1, Copilot's 6 findings) — compareRecordSemantics is now a full copy of backtest/determinism_test.go's own current function, restoring every field an earlier version silently dropped: KindDecision's violations/warnings (both decision-level and per-RuleResult), KindOrder's AcceptedLimitPrice/AcceptedQuantity, and KindFill's commission/BrokerOrderID/Metadata event-correlation-causation. compareViolations/compareWarnings/compareMoney copied alongside it. Only two additions beyond that copy remain: KindReplaceRequest (this scenario's three ratcheting AdjustStop intents) and KindSignal (decision evidence, unique to this gate).

2. Manifest strategy/config identity excluded rather than normalized (Rusty M0-01 Expand the project README #2) — new smaLongHoldSemanticIdentity type (sma_period/trailing_stop_percent/exit_rule/reentry_rule/initial_entry_mode) with JSON tags matching smatrend.Config's own exactly, so both manifests' real StrategyParameters decode into it symmetrically and compare directly:

   • The in-tree Config now sets ExitRuleName/ReEntryRuleName/InitialEntryModeName explicitly (previously left empty → would have recorded "" in the manifest rather than the actual resolved rule names).
   • The external run's RunRequest.StrategyParameters — previously omitted entirely — now carries the guest's actual decoded configuration. This is a deliberate, documented divergence from run.go's own real --strategy-exec convention (which records launch metadata, not decoded guest config, since the host never learns that value over the wire) — this dedicated equivalence test needs the real semantic configuration to assert against, which it happens to know because it wrote the guest's own config file moments before launch. Documented inline on the new type.
   • Smaller follow-on also addressed: dataset comparison now also checks span, basis, and raw fingerprint, matching the canonical determinism gate's own full dataset-identity comparison.

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.

[rustyeddy]

Rereviewed latest head 8acb407.

The two issues from my previous review are substantially fixed:

• the canonical determinism checks for decision diagnostics, accepted order values, fill commission/BrokerOrderID, and fill metadata are now restored;
• semantic strategy/config identity is now normalized and compared explicitly on both manifests, and the dataset comparison now includes span, basis, raw fingerprint, and revision.

I still see one remaining class of blocker before calling this a Milestone 19 completion gate:

The journal comparison still omits semantic fields from Intent / Proposal / Request (and part of Fill identity). Copilot's new findings are valid here.

Concretely:

1. Intent
   KindIntent compares Kind/Instrument/Side/IntentID, but not:

   • Quantity
   • StopPrice
   • the Intent payload's own Metadata

   This matters immediately in this fixture because every AdjustStop intent carries a non-nil StopPrice. Two implementations could choose different stop prices while still passing the Intent comparison, with the discrepancy only potentially surfacing later. Since issue #384 explicitly asks for emitted intent semantics and correlation relationships, compare these fields at the Intent stage itself.

2. Proposal
   KindProposal omits:

   • LimitPrice
   • StopPrice
   • ReduceOnly
   • Proposal Metadata

   These are execution-planning semantics. A proposal with a different stop/limit or reduce-only setting should fail this gate immediately at the proposal record where the divergence occurred.

3. Request
   KindRequest compares only instrument/side/quantity/OrderID even though Request embeds the full Proposal. Compare the embedded Proposal semantics here too (Type, TIF, limit/stop, ReduceOnly, AccountID, Metadata), not merely a subset. Otherwise a request-level divergence can hide behind the earlier Proposal record and the failure is no longer guaranteed to pinpoint the first semantic divergence.

4. Order
   The accepted values are now covered, which is good, but the embedded Request is still only represented by OrderID. For a true field-by-field order comparison, compare the embedded request semantics as well, or factor a helper so Proposal/Request/Order all reuse the same canonical comparison.

5. Fill
   Commission/BrokerOrderID/Metadata are now covered, but BrokerFillID is still omitted. BrokerFillID is observable execution identity and is specifically used by Trader for duplicate execution detection. If it is intentionally normalized rather than compared literally, add a brokerFillID normalizer analogous to brokerOrderID.

I would factor this rather than continuing to hand-copy subsets:

• compareMetadata
• compareIntent
• compareProposal
• compareRequest
• then let KindOrder call compareRequest before checking broker-effective fields.

That would make this gate both stricter and much harder to drift as the order model evolves.

One documentation cleanup is also needed: the file-level/test comments still say Manifest.StrategyParameters are intentionally excluded, but the code now correctly normalizes and compares them. Update those comments so the stated gate matches the implementation.

The manifest/config fix itself looks good now; my remaining concern is the completeness of the causal/journal semantics comparison.

[rustyeddy]

Pushed 632b437 addressing the remaining gap from your re-review of 8acb407 (Copilot's 6 findings from the previous round all fall under this same issue):

• New compareMetadata/compareIntent/compareProposal/compareRequest helpers — Proposal's shared embedded-Proposal semantics are now checked identically everywhere they appear (KindProposal, KindRequest, and KindOrder's own embedded Request), not three independent, driftable hand-copies.
• Intent: now compares Quantity, StopPrice, and the intent's own Metadata — previously omitted, meaning two implementations could choose different stop prices at the intent stage (every AdjustStop intent in this scenario carries one) and still pass.
• Proposal: now compares LimitPrice, StopPrice, ReduceOnly, and Metadata.
• Request: now compares the embedded Proposal's full semantics via compareRequest, not just instrument/side/quantity/OrderID.
• Order: now compares its own embedded Request via the identical compareRequest helper, so a divergence there can no longer hide behind an earlier, separately-passing KindRequest record for the same order.
• Fill: now also compares BrokerFillID (currently always "" on both sides since adapters/broker/sim never populates it — checked explicitly rather than silently dropped, per your suggestion, so this stops protecting the field the moment that changes rather than needing a brokerFillID normalizer today).
• Fixed the stale doc comments (top of file and the test function's own) that still said Manifest.StrategyParameters was excluded — it's been normalized and compared since the previous round; only StrategyName is excluded.

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.

[rustyeddy]

Rereviewed latest head 632b437.

The remaining journal-semantic gaps from the prior review are fixed cleanly:

• compareIntent now covers Quantity, StopPrice, IntentID, and the Intent's own Metadata;
• compareProposal now covers limit/stop prices, ReduceOnly, AccountID, and Metadata in addition to the existing order mechanics;
• compareRequest reuses that full Proposal comparison and is used both for KindRequest and the Request embedded in KindOrder;
• Fill now explicitly compares BrokerFillID in addition to commission, BrokerOrderID, IDs, and causal Metadata;
• the shared compareMetadata helper keeps correlation/causation comparisons consistent instead of duplicating partial checks;
• the stale comments saying Manifest.StrategyParameters was excluded are corrected; semantic config is normalized and compared as intended.

I also rechecked the earlier manifest/config and dataset-identity fixes. The gate now exercises descriptor/data requirements, normalized strategy config, full dataset identity, ordered journal semantics/correlation, trades, equity curve, and final account state through the normal backtest runtime.

I don't see another correctness blocker. PR #395 looks merge-ready from my review and is a credible Milestone 19 equivalence gate.

One future-proofing note only: BrokerFillID is compared literally because sim currently leaves it empty. If sim later begins assigning run-specific broker fill IDs, that field will likely need normalization analogous to BrokerOrderID. The current explicit equality is correct for today's simulator behavior and is not a blocker.

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