EQ-385: external strategy provenance and operator documentation

[rustyeddy]

Closes #385 — the final issue in the "Add gRPC to trader for remote out of tree strategies" milestone.

What changed

Provenance (cmd/trader/backtest/run.go): externalStrategyParams gains:

• Mode: "external" — explicit marker
• StrategyName/StrategyVersion — the guest's own Handshake Descriptor
• ProtocolVersion — the Strategy Protocol version actually negotiated (protocol/strategy/v1.ProtocolVersion)
• Transport: "unix" — explicit rather than assumed
• Exec — resolved to an absolute path, not the CLI's possibly-relative spelling
• ExecDigest — a "sha256:<hex>" content digest of the executable file at launch time, matching backtest.Manifest.ConfigDigest's own convention, so two different builds behind the identical --strategy-exec path are distinguishable

The ephemeral Unix-domain socket path Launch generates per run is never recorded anywhere in this struct — issue #385's own explicit "no ... machine-specific ephemeral socket paths ... as semantic identity" exclusion. New TestVerticalSlice_RunWithStrategyExec_RecordsProvenance reads every field back from a real trader backtest run JSON report (not run.go internals directly) and asserts the ephemeral socket directory name never appears anywhere in the recorded provenance, even as raw text.

Documentation (docs/UsersGuide.md): trader backtest run now documents three strategy paths (not two) plus the three new flags, and a new "External strategies" section covers:

• building a Go external strategy (strategysdk, with pointers to examples/strategysdk-minimal and examples/sma-long-hold)
• running it
• process lifecycle and logs (environment inheritance, stderr capture, graceful SessionEnd vs. SIGTERM/SIGKILL, mid-run crash detection)
• protocol/capability mismatch behavior at Handshake
• reproducibility provenance fields with a worked JSON example
• v1's explicit limitations/non-goals (local executable only, one strategy per run, no sandboxing beyond OS process isolation, config schema is the author's own responsibility)

Testing

Full go test ./... green under -race, golangci-lint clean (only the 3 known pre-existing unrelated research-runs/ errcheck findings), no stray processes. Package coverage 82.1%.

🤖 Generated with Claude Code

[Copilot]

Copilot review overview

🟡 Changes recommended

Resolve executable path and digest handling so provenance matches the launched artifact and avoids unnecessary memory spikes.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1

Open (1)

• Hash the exact executable artifact before launching · New

What changed in this PR

Adds external-strategy provenance tracking, executable digests, end-to-end coverage, and operator documentation for trader backtest run.

Changes:

• Records strategy identity, protocol, transport, executable path, and digest.
• Adds vertical-slice provenance validation.
• Documents external strategy workflows, lifecycle, and limitations.

File	Summary
cmd/​trader/​backtest/​run.go	Adds external provenance fields and executable hashing.
cmd/​trader/​backtest/​external_strategy_test.go	Verifies persisted provenance through a vertical-slice test.
docs/​UsersGuide.md	Documents external strategy usage and provenance.

---

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

[rustyeddy]

Reviewed PR #396 (EQ-385: external strategy provenance and operator documentation). The provenance shape and operator docs are generally strong, but I see two reproducibility issues to fix before merge:

1. Resolve/hash the exact executable before launch, and launch that exact resolved artifact.

Copilot's finding is valid. The current order is:

process, err := external.Launch(ctx, launchCfg)
...
execAbs, err := filepath.Abs(flags.strategyExec)
execDigest, err := execContentDigest(execAbs)

So the digest is computed only after the child has already started and completed Handshake. If the file is replaced/removed during startup, the recorded digest can describe different bytes than the process actually executed, or fail an otherwise valid run.

There is also a path-resolution mismatch: filepath.Abs("my-strategy") is not equivalent to exec.Command("my-strategy") when PATH lookup is allowed. The former means $PWD/my-strategy; the latter may execute /usr/local/bin/my-strategy.

Resolve once before launch using the same semantics the launcher will use (for example exec.LookPath when the command has no path separator, then filepath.Abs/Clean), hash that resolved file, and pass that resolved path as LaunchConfig.Command. Record that same path/digest in provenance. That creates one artifact identity used consistently for both launch and manifest.

I would also stream the hash via os.Open + io.Copy(sha256.New(), f) rather than os.ReadFile; executables are usually modest, but there is no reason provenance hashing needs to allocate the entire binary.

2. The strategy config is identified only by pathname, not by content.

Issue #385 explicitly asks for effective strategy config/config digest and unambiguous provenance. Today externalStrategyParams.Config records only the absolute config path. The manifest's own ConfigDigest hashes StrategyParameters, which therefore includes that path but not the bytes at that path.

That means:

• run A uses /configs/strategy.json containing {"period":20}
• the file is edited in place
• run B uses the same path containing {"period":50}

Both runs can record the same external strategy parameters/config identity even though the guest consumed different configuration. That defeats the reproducibility goal this issue is closing.

Add a ConfigDigest (or similarly named field) containing a SHA-256 of the exact config bytes handed to the guest. Compute it before launch for the same TOCTOU reason as the executable. If no --strategy-config is supplied, omit/empty it explicitly. The vertical provenance test should assert the digest shape and ideally verify it against the fixture file's actual bytes.

If you want the strongest invariant, resolve and hash both executable and config before creating the child, then use exactly those resolved paths in the launch environment/command and exactly those digests in externalStrategyParams.

The remaining fields — mode, guest Descriptor identity, protocol version, transport kind, no ephemeral socket path, and the documentation additions — look appropriate for the v1 scope.

[rustyeddy]

Pushed ad4a588 addressing both findings (Rusty's two points, aligned with Copilot's own finding):

1. Resolve/hash the exact executable before launch (Copilot + Rusty M0-05 Add baseline GitHub Actions CI #1) — new resolveStrategyExecutable mirrors os/exec's own PATH-search behavior exactly (exec.LookPath for a bare name with no path separator, plain filepath.Abs for a literal path) — the previous unconditional filepath.Abs wasn't equivalent to what exec.Command actually does for a bare command name on PATH. buildExternalLaunchConfig now resolves the executable itself and sets LaunchConfig.Command to that exact resolved path, so launch and provenance are guaranteed to agree on which file "the executable" means. Both digests are now computed immediately before external.Launch runs, not after the child has already started and completed Handshake, closing most of the TOCTOU window. fileContentDigest now streams via os.Open + io.Copy(sha256.New(), f) instead of os.ReadFile.

2. Config identified only by pathname, not content (Rusty M0-01 Expand the project README #2) — new externalStrategyParams.ConfigDigest: a sha256 digest of the exact bytes at --strategy-config's path, computed before launch. Previously two runs using the same path whose file was edited in place between them could record identical provenance despite the guest consuming different configuration. Empty when no --strategy-config is given.

New tests: resolveStrategyExecutable's PATH-search vs. literal-path resolution (plus an unresolvable-bare-name error case), and the provenance vertical-slice test now verifies ConfigDigest against the fixture file's own real sha256 sum, not just its shape. One existing test updated: a nonexistent --strategy-exec now fails during digest computation (before Launch is ever called) rather than inside Launch itself — strictly earlier, since no child process is ever attempted for a file that doesn't exist.

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. Package coverage 82.4%.

[rustyeddy]

Pushed 858dc5e addressing an automated /code-review pass on PR #396 (2 findings, head ad4a588):

1. fileContentDigest had no context.Context parameter despite performing blocking file I/O, unlike every other I/O call in this file. It now takes ctx and checks it before opening the file, failing fast on an already-canceled/expired context rather than reading and hashing a file no caller will use the result of.

2. Missing direct unit tests for fileContentDigest's failure/edge modes — previously covered only indirectly (not-found, via a full CLI integration test). Added four direct tests: empty-file hashing (the well-known sha256 of zero bytes — the boundary case for the hash loop), not-found, permission-denied (a distinct, realistic failure mode from not-found, skipped when running as root), and an already-canceled context failing before any file I/O.

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. Package coverage 82.5%.

[rustyeddy]

Rereviewed latest head 858dc5e.

The two provenance blockers from the previous review are addressed well:

• --strategy-exec is now resolved once using the same PATH/literal-path semantics as os/exec, and that exact resolved path is used for LaunchConfig.Command, executable hashing, and recorded provenance;
• executable hashing happens before launch and streams through SHA-256 rather than loading the whole binary;
• external config now has its own content digest, also computed before launch, so an in-place config edit changes provenance even if the path stays identical;
• the vertical-slice provenance test verifies the config digest against the actual fixture bytes;
• direct tests cover executable resolution and digest edge/failure cases.

I also rechecked the rest of #385's acceptance surface: guest Descriptor identity, protocol version, external mode, Unix transport, absolute executable/config paths, executable digest, config digest, and exclusion of the ephemeral socket path are all represented. The operator documentation covers the expected build/run/lifecycle/mismatch/reproducibility/non-goal workflow.

There is still the unavoidable narrow filesystem TOCTOU window between hashing a pathname and the OS opening that pathname for exec (and similarly between config hashing and the guest opening its config). Eliminating that completely would require snapshotting/copying the artifacts or launching/reading from already-open descriptors. Given #385's explicit “stable enough to distinguish different builds” wording, hashing immediately before launch/read via the exact resolved paths is a reasonable v1 boundary, not a blocker.

One tiny non-blocking test improvement: the provenance vertical test could assert params.Config == configPath in addition to checking that it is absolute, mirroring the exact-path assertion already made for Exec.

I don't see another correctness blocker. PR #396 looks merge-ready from my review. With #395 and this PR in place, the remaining External Strategies v1 milestone work appears to have the intended equivalence + provenance/operator gates covered.

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.