Skip to content

EQ-381: strategysdk — Go SDK/runtime for out-of-tree strategy authors - #392

Merged
rustyeddy merged 3 commits into
mainfrom
feature/381-strategysdk
Sep 18, 2026
Merged

rustyeddy merged 3 commits into
mainfrom
feature/381-strategysdk

Conversation

@rustyeddy

Copy link
Copy Markdown
Owner

What changed

Adds strategysdk, the guest-side counterpart to adapters/strategy/external.Host (#379) and .Process (#380): a small Go helper/runtime so an out-of-tree Go strategy process can speak Strategy Protocol v1 (protocol/strategy/v1, #377) without hand-writing gRPC plumbing, per ADR-062's own "Go SDK/runtime" section, which had already settled most of this design ahead of implementation.

Why

Issue #381 (depends on #377, merged) asks for this.

Design, per ADR-062

  • strategysdk.Strategy is its own interface, not strategy.Strategy: OnBar returns ([]DescribedIntent, []DescribedSignal, error), never real order.Intent/journal.Record values — a guest never mints a canonical IntentID/EventID/CorrelationID. DescribedIntent's typed constructors (Enter, Exit, AdjustStop, EnterWithStop, TargetExposure) mirror strategy.IntentFactory's own method names.
  • Environment.Clock reflects the host's own injected clock — seeded from SessionStart's start_time and advanced to each BarEvent's own timestamp — never this process's local wall clock.
  • View folds strategy.History's capability into a required method (GetHistoryBars is always available in v1, not negotiated).
  • Depends only on protocol/strategy/v1, order, marketdata, instrument, and num — never strategy, backtest, service, cmd, adapters, broker, execution, risk, pipeline, or chart (mechanically enforced by strategysdk/boundary_test.go, the identical forbidden set strategy/boundary_test.go already uses). The wire-conversion logic is its own independent implementation — not shared code with adapters/strategy/external's own — each side of the v1 boundary owns and tests its own half of the mapping.

Socket discovery and the config/arch_test.go exemption

Serve reads TRADER_STRATEGY_SOCKET (adapters/strategy/external's own ADR-063 launch contract, mirrored as strategysdk.SocketPathEnv rather than imported, since neither v1-boundary package imports the other). Since strategysdk.Serve is itself this process's own composition root — the guest-side equivalent of a cmd/ binary's main(), just packaged as an importable library — config/arch_test.go's exemption list now names strategysdk alongside config/cmd/test, with an inline comment explaining why this is the same exemption, not a relaxation of the rule.

Testing

Unit tests for every conversion, enum, and constructor, including every explicit-failure path. serve_test.go/serve_more_test.go drive strategysdk.ServeConn against the real, production adapters/strategy/external.Host (not a hand-rolled fake), over both bufconn and an actual Unix-domain socket file — proving both halves of the v1 boundary interoperate for real: Describe/Start/OnBar/OnFill round trips, FillHandler capability negotiation, GetHistoryBars, a guest OnBar error reported to the host, a host-initiated graceful SessionEnd, and Handshake rejection producing a clear, typed error (WireError).

examples/strategysdk-minimal is a complete, compiling, minimal strategy binary (a trivial flip-flop enter/exit) with its own README; strategysdk/README.md documents the full author-facing API.

go build/vet/gofmt/golangci-lint clean; go test -race on the package green with 87.7% coverage; full go test ./... green (including the updated config architecture test).

Acceptance criteria

  • external Go strategy can be implemented without importing Trader internals beyond the intended SDK/public types
  • one minimal example binary compiles and serves over Unix socket
  • Describe/Start/OnBar/OnFill path covered by tests
  • protocol mismatch produces a clear error
  • author-facing README/example included

Closes #381

🤖 Generated with Claude Code

Adds strategysdk, the guest-side counterpart to
adapters/strategy/external.Host (#379) and .Process (#380): a small
Go helper/runtime so an out-of-tree Go strategy process can speak
Strategy Protocol v1 (protocol/strategy/v1, #377) without hand-writing
gRPC plumbing, per ADR-062's own "Go SDK/runtime" section, which had
already settled most of this design ahead of implementation.

## Design, per ADR-062

- strategysdk.Strategy is its own interface, not strategy.Strategy:
  OnBar returns ([]DescribedIntent, []DescribedSignal, error), never
  real order.Intent/journal.Record values — a guest never mints a
  canonical IntentID/EventID/CorrelationID (ADR-005/ADR-062's own
  intent-construction-ownership section). DescribedIntent's typed
  constructors (Enter, Exit, AdjustStop, EnterWithStop,
  TargetExposure) mirror strategy.IntentFactory's own method names.
- Environment.Clock reflects the host's own injected clock — seeded
  from SessionStart's start_time and advanced to each BarEvent's own
  timestamp — never this process's local wall clock, giving a guest
  identical clock-observation semantics to an in-process strategy
  under both real and simulated host clocks.
- View folds strategy.History's capability into a required method
  (GetHistoryBars is always available in v1, not negotiated),
  scoped to the guest's own declared requirements.
- Depends only on protocol/strategy/v1, order, marketdata, instrument,
  and num — never on strategy itself, and never on backtest, service,
  cmd, adapters, broker, execution, risk, pipeline, or chart, the
  identical forbidden set strategy/boundary_test.go already enforces
  (mirrored here in strategysdk/boundary_test.go). The wire-conversion
  logic (convert.go, enum.go, interval.go, identity.go, numeric.go) is
  its own independent implementation, not shared code with
  adapters/strategy/external's own — each side of the v1 boundary
  owns and tests its own half of the mapping, per ADR-062.

## Socket discovery and the config/arch_test.go exemption

Serve reads TRADER_STRATEGY_SOCKET (adapters/strategy/external's own
ADR-063 launch contract, mirrored as strategysdk.SocketPathEnv rather
than imported, since neither v1-boundary package imports the other).
Since strategysdk.Serve is itself this process's own composition
root — the guest-side equivalent of a cmd/ binary's main(), just
packaged as an importable library because the real main() lives in an
external author's own repository — config/arch_test.go's exemption
list now names strategysdk alongside config/cmd/test, with an inline
comment explaining why this is the same exemption, not a relaxation
of the rule.

## Implementation

- types.go/intent.go/signal.go/strategy.go: the public SDK surface
  (Descriptor, DataRequirement, BarEvent, FillEvent, AccountSnapshot,
  PositionSnapshot, View, Environment, DescribedIntent + constructors,
  DescribedSignal, Strategy, FillHandler).
- identity.go/numeric.go/enum.go/interval.go/convert.go/errors.go:
  the wire<->sdk conversion layer, including canonical instrument-ID
  validation (reconstructed through the real per-Kind constructor,
  requiring an exact String() round trip — the same discipline
  adapters/strategy/external's own parseInstrumentID uses).
- clock.go: hostClock, satisfying clock.Clock — Now() returns whatever
  the host most recently supplied; NewTimer delegates to clock.Real
  (a guest process is always a genuine external process running in
  real wall-clock time regardless of the host's own simulated clock;
  neither strategy/smatrend nor strategy/emacross calls NewTimer
  today, so no current strategy observes this limitation).
- serve.go: Serve/ServeContext/ServeConn and the guestRun Run-stream
  loop — deliberately simple compared to adapters/strategy/external's
  own actor-model runSession, since there is exactly one strategy, one
  stream, and no concurrent callers on the guest side.

## Testing

Unit tests for every conversion, enum, and constructor, including
every explicit-failure path. serve_test.go/serve_more_test.go drive
strategysdk.ServeConn against the real, production
adapters/strategy/external.Host (not a hand-rolled fake), over both
bufconn and (TestServeContext_RealUnixSocket) an actual Unix-domain
socket file — proving both halves of the v1 boundary interoperate for
real: Describe/Start/OnBar/OnFill round trips, FillHandler capability
negotiation, GetHistoryBars, a guest OnBar error reported to the host,
a host-initiated graceful SessionEnd, and Handshake rejection
producing a clear, typed error (WireError).

examples/strategysdk-minimal is a complete, compiling, minimal
strategy binary (a trivial flip-flop enter/exit) with its own README;
strategysdk/README.md documents the full author-facing API.

go build/vet/gofmt/golangci-lint clean; go test -race on the package
green with 87.7% coverage; full go test ./... green (including the
updated config architecture test).

Closes #381

🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015fVsVuQCgkrhiYaXLxyUF3
Copilot AI lite review requested due to automatic review settings September 18, 2026 20:35

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Unresolved critical and moderate findings remain in the SDK runtime and conversion paths.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Adds the guest-side strategysdk Go runtime for Strategy Protocol v1, enabling out-of-tree strategy authors to connect over Unix sockets without implementing gRPC plumbing.

Changes:

  • Adds SDK interfaces, types, intents, signals, fills, clocks, and conversions.
  • Implements handshake, streaming callbacks, history access, errors, and shutdown handling.
  • Adds tests, documentation, architecture checks, and a minimal example strategy.
File summaries
File Summary Review findings
strategysdk/types.go Public SDK data types and view contracts
strategysdk/strategy.go Strategy and fill-handler interfaces
strategysdk/signal.go Signal construction
strategysdk/signal_test.go Signal tests
strategysdk/serve.go Protocol client runtime Line 92: validate negotiated capabilities, especially CAPABILITY_FILL_HANDLER (critical, 1 vote).
Line 111: use the Run stream context for callbacks and view RPCs (moderate, 1 vote).
Line 334: return an empty successful result for non-positive history counts (moderate, 3 votes).
Line 59: apply a bounded dial or handshake timeout (moderate, 2 votes).
Line 203: make logging configurable or use the default logger instead of discarding records (moderate, 2 votes).
Line 166: treat EOF without SessionEnd as abnormal termination (critical, 2 votes).
Line 354: apply a bounded deadline to history RPCs (moderate, 1 vote).
Line 361: expose RPC and conversion failures instead of collapsing them into ok == false (moderate, 2 votes).
Line 359: bound history counts before converting to int32 (moderate, 1 vote).
strategysdk/serve_test.go End-to-end protocol tests
strategysdk/serve_more_test.go Socket, history, and error tests
strategysdk/README.md Author-facing SDK documentation Line 113: document that history is unavailable during fill callbacks or expose a separate fill view (nit, 1 vote).
strategysdk/numeric.go Numeric wire conversions
strategysdk/numeric_internal_test.go Numeric conversion tests
strategysdk/interval.go Interval conversions
strategysdk/interval_internal_test.go Interval tests
strategysdk/intent.go Described intent constructors
strategysdk/intent_test.go Intent constructor tests
strategysdk/identity.go Instrument ID parsing
strategysdk/identity_internal_test.go Identity tests
strategysdk/errors.go Wire error handling
strategysdk/errors_internal_test.go Error tests
strategysdk/enum.go Enum conversions
strategysdk/enum_internal_test.go Enum conversion tests
strategysdk/doc.go Package architecture documentation
strategysdk/convert.go Protocol/domain conversions Line 109: validate PositionSnapshot average-price invariants (moderate, 2 votes).
Line 223: reject non-positive TargetExposure quantities before sending them over the wire (moderate, 1 vote).
Line 180: reject WarmupBars values exceeding math.MaxInt32 (moderate, 2 votes).
Line 49: run marketdata.Bar.Validate() on received bars (moderate, 1 vote).
strategysdk/convert_internal_test.go Conversion tests
strategysdk/clock.go Host-synchronized clock
strategysdk/clock_internal_test.go Clock tests
strategysdk/boundary_test.go Dependency boundary enforcement
examples/strategysdk-minimal/README.md Minimal example documentation
examples/strategysdk-minimal/main.go Minimal compiling strategy binary
config/arch_test.go SDK environment-access exemption
Review details

Suppressed comments (6)

strategysdk/README.md:117

  • This README says the View passed to OnFill provides HistoryBars and that ok == false only means an undeclared requirement, but handleFillEvent deliberately disables history for fill callbacks because v1 scopes GetHistoryBars to an in-flight BarEvent. Document that restriction (or expose a separate fill view) so strategy authors do not treat every fill-time lookup as a missing requirement.
`View`, passed to `OnBar` (and `OnFill`, below), gives you:

```go
view.Account() AccountSnapshot
view.HistoryBars(inst, interval, n) (bars []marketdata.Bar, ok bool)

strategysdk/convert.go:225

  • order.NewIntent requires a positive quantity for TargetExposure, but this guest-side validation only checks that the pointer is non-nil. A zero quantity is therefore sent across the wire and rejected later by the host's factory, so the guest does not uphold the documented promise that malformed intents fail before crossing the boundary.
	if requireQuantity && d.Quantity == nil {
		return nil, fmt.Errorf("%w: quantity is required for intent kind %v", ErrInvalidWireValue, d.Kind)
	}

strategysdk/convert.go:53

  • This parses each numeric field and checks only Ticks, but never runs marketdata.Bar.Validate(). A malformed peer can therefore send zero time, inconsistent OHLC bounds, or AvgSpread > MaxSpread, and the guest will pass an invalid bar to strategy code even though the canonical Bar type defines those invariants.
	return marketdata.Bar{
		Time:      time.Unix(0, w.GetTimeUnixNanos()).UTC(),
		Open:      open,
		High:      high,
		Low:       low,

strategysdk/serve.go:111

  • guestRun.ctx is the outer ServeContext context, not the Run RPC's context. When the host's callback timeout or caller context tears down the long-lived stream, a guest blocked in Start, OnBar, OnFill, or GetHistoryBars still sees an active g.ctx, so context-aware strategy code can remain stuck after the host has stopped waiting. Use the stream context for the per-session callbacks and view RPCs while retaining the outer context for Handshake.
		ctx:          ctx,

strategysdk/serve.go:360

  • This unary history call uses the whole Serve context without a per-RPC deadline. If the host's history implementation blocks, the guest remains stuck inside OnBar even after the host's callback timer tears down the Run stream, because this loop cannot receive the teardown while it is waiting on GetHistoryBars. ADR-062 assigns unary RPC deadlines to the guest SDK, so apply a bounded context here.
	resp, err := v.client.GetHistoryBars(v.ctx, &v1.GetHistoryBarsRequest{
		SessionId:        v.sessionID,
		CallbackSequence: v.sequence,
		InstrumentId:     instID.String(),
		Interval:         wireIv,
		Count:            int32(n),
	})

strategysdk/serve.go:359

  • n is an int while the protocol count is int32; on 64-bit systems a valid request larger than math.MaxInt32 wraps negative at this cast. The host rejects that request, and the error is then hidden by the false result below, so bound or clamp n before converting it.
		Count:            int32(n),
  • Files reviewed: 29/29 changed files
  • Comments generated: 8
  • Review effort level: Lite

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

Comment thread strategysdk/serve.go
Comment on lines +92 to +98
if !hsResp.GetAccepted() {
if rejectErr := fromWireError(hsResp.GetRejectReason()); rejectErr != nil {
return fmt.Errorf("strategysdk: handshake rejected: %w", rejectErr)
}
return fmt.Errorf("strategysdk: handshake rejected: host protocol version %q, this SDK is %q",
hsResp.GetProtocolVersion(), v1.ProtocolVersion)
}
Comment thread strategysdk/serve.go
Comment on lines +166 to +168
if errors.Is(err, io.EOF) {
return nil
}
Comment thread strategysdk/convert.go
Comment on lines +109 to +113
avgPrice, err := parseOptionalPrice("avg_price", w.GetAvgPrice())
if err != nil {
return PositionSnapshot{}, err
}
return PositionSnapshot{Instrument: instID, Side: side, AvgPrice: avgPrice}, nil
Comment thread strategysdk/convert.go
Comment on lines +173 to +179
if r.WarmupBars < 0 {
return nil, fmt.Errorf("%w: warmup_bars must not be negative", ErrInvalidWireValue)
}
interval, err := toWireInterval(r.Interval)
if err != nil {
return nil, err
}
Comment thread strategysdk/serve.go
Comment on lines +59 to +62
conn, err := grpc.NewClient("unix:"+sockPath, grpc.WithTransportCredentials(insecure.NewCredentials()))
if err != nil {
return fmt.Errorf("strategysdk: dialing %s: %w", sockPath, err)
}
Comment thread strategysdk/serve.go Outdated
env := Environment{
Clock: g.clock,
RunID: g.runID,
Logger: slog.New(slog.DiscardHandler),
Comment thread strategysdk/serve.go Outdated
Comment on lines +334 to +348
if !v.historyOK || n <= 0 {
return nil, false
}

declared := false
for _, r := range v.requirements {
if r.Instrument.Equal(instID) && r.Interval == interval {
declared = true
break
}
}
if !declared {
return nil, false
}

Comment thread strategysdk/serve.go Outdated
Comment on lines +361 to +367
if err != nil {
return nil, false
}

bars, err := fromWireBars(resp.GetBars())
if err != nil {
return nil, false

@rustyeddy rustyeddy left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed PR #392 (EQ-381: strategysdk — Go SDK/runtime for out-of-tree strategy authors). The overall shape is right: a guest-native Strategy interface, host-owned canonical IDs/intents, host-synchronized clock, strict dependency boundary, and real interoperability tests against adapters/strategy/external.Host.

I see several issues to address before merge:

  1. Per-session callbacks are using the wrong context. guestRun.ctx is set to the outer ServeConn context, not the Run stream's context. ADR-062 relies on the host tearing down the Run stream when a callback times out/cancels. If OnBar, OnFill, Start, or a history lookup blocks on g.ctx, that code can remain alive after the host has already terminated the session. Once client.Run(ctx) succeeds, session work should use stream.Context() (or a child derived from it); keep the outer context only for setup/Handshake.

  2. GetHistoryBars violates ADR-062's unary-RPC deadline rule and hides transport failures. ADR-062 explicitly says Handshake and GetHistoryBars deadlines are owned by the guest/SDK. HistoryBars calls the unary RPC with the long-lived session context and no per-RPC timeout. Worse, any RPC or conversion error is returned as (nil, false), but the public contract documents ok == false as “requirement not declared.” A dead host, deadline, malformed response, and undeclared requirement therefore become indistinguishable. This needs a bounded RPC context and an error path that cannot masquerade as normal strategy state. I would strongly prefer changing the SDK method to something like HistoryBars(...) ([]marketdata.Bar, bool, error) (or ([]marketdata.Bar, error) with a typed requirement error) rather than silently swallowing remote failures.

  3. EOF without SessionEnd is treated as normal completion. loop returns nil on bare io.EOF. In v1, normal host shutdown is represented by SessionEnd; a stream that simply disappears should be distinguishable from graceful completion. Otherwise a crashed/aborted host can look like a successful strategy run. Return an error for EOF unless a normal SessionEnd was actually observed.

  4. The guest boundary is not validating all canonical invariants before exposing wire data to strategy code. Examples:

    • fromWireBar parses fields but never calls marketdata.Bar.Validate(), so invalid OHLC/time/spread relationships can reach the strategy.
    • fromWirePositionSnapshot does not enforce the documented flat/non-flat AvgPrice invariant.
    • toWireDataRequirement casts WarmupBars int -> int32 without an upper-bound check.
    • HistoryBars casts n int -> int32 without guarding math.MaxInt32.
    • TargetExposure checks only that Quantity is non-nil, not that it satisfies the same positive-quantity invariant the host will enforce.

    Since the SDK claims malformed values fail at the guest boundary rather than crossing it, these checks belong here.

  5. Handshake capability negotiation is only half-consumed. The guest advertises capabilities, but after an accepted Handshake it never validates the host's returned HandshakeResponse.capabilities. ADR-062 defines negotiation as bilateral and says mismatch should be rejected rather than silently degraded. At minimum verify that every capability the runtime depends on (currently FillHandler when implemented) is present in the accepted response.

A couple of smaller API/docs mismatches fall out of the above:

  • README/types say the same View contract applies to OnFill, but handleFillEvent deliberately disables history for fill callbacks. Document that explicitly or expose a distinct fill view.
  • n <= 0 currently returns ok=false; that again conflicts with ok=false == undeclared requirement. Pick an explicit contract for non-positive counts.

The architecture and test approach are good; these are mostly boundary/lifecycle semantics that are worth making exact before this becomes the public author-facing SDK.

Addresses PR #392 review findings:

- guestRun now runs OnBar/OnFill callback work on stream.Context()
  instead of the outer ServeConn ctx, so cancellation propagates
  correctly once the host tears the session down.
- New Option/WithHandshakeTimeout/WithHistoryBarsTimeout/WithLogger:
  Handshake and GetHistoryBars are both guest-initiated unary RPCs
  (ADR-062 assigns their deadline to the SDK side), and the logger
  passed to Environment is now configurable instead of unconditionally
  discarded.
- View.HistoryBars gains a third return (err error), distinguishing a
  genuine RPC/transport failure from "not declared for this
  instrument/interval" (ok=false, err=nil) — mirrors
  backtest.Scheduler's own historyView contract.
- verifyNegotiatedCapabilities rejects a Handshake whose accepted
  response omits a capability (e.g. CAPABILITY_FILL_HANDLER) the guest
  actually implements, instead of silently running without it.
- loop() now distinguishes a stream that ends via SessionEnd from one
  that just EOFs (host/transport failure) via a sawSessionEnd flag.
- fromWireBar validates the reconstructed Bar's own OHLC/spread
  invariants; fromWirePositionSnapshot enforces the Flat<->AvgPrice=nil
  invariant; toWireDataRequirement bounds WarmupBars to int32;
  toWireDescribedIntent rejects a zero (but non-nil) TargetExposure
  quantity.
- README documents the new HistoryBars signature and that OnFill's
  View never issues a GetHistoryBars RPC (v1 scopes it to BarEvent).
- logging/arch_test.go: strategysdk is now exempt from the
  no-slog.Default rule for the same reason cmd/ already is — Serve is
  the guest process's own composition root.

Tests: new coverage for capability-mismatch rejection, handshake
timeout, history-bars timeout (err vs ok=false), EOF-without-
SessionEnd, WithLogger routing, and the new convert.go invariant
checks. strategysdk coverage 88.7%. Full `go test ./...` and
golangci-lint clean (only the 3 pre-existing unrelated research-runs
errcheck findings remain).

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015fVsVuQCgkrhiYaXLxyUF3
@rustyeddy

Copy link
Copy Markdown
Owner Author

Pushed 37d776e addressing all review findings (Copilot's 8 inline comments + Rusty's 5-point review + the README nit):

  1. Wrong context for session work (Rusty M0-05 Add baseline GitHub Actions CI #1, Copilot serve.go:111)guestRun.ctx is now stream.Context(), set after client.Run(ctx) succeeds. The outer ServeConn context now bounds only Handshake. A host-side callback-timeout teardown of the Run stream now actually cancels a stuck guest OnBar/OnFill/Start/history call.

  2. GetHistoryBars unbounded + swallowed errors (Rusty M0-01 Expand the project README #2, Copilot serve.go:354/359/361)View.HistoryBars is now (bars []marketdata.Bar, ok bool, err error). ok=false, err=nil means "not declared" (unchanged meaning); a non-nil err is a genuine RPC/transport/conversion failure — no longer collapsed into ok=false. The RPC now runs under context.WithTimeout(v.ctx, cfg.historyBarsTimeout) (DefaultHistoryBarsTimeout = 5s, overridable via WithHistoryBarsTimeout). n is bounds-checked against math.MaxInt32 before the int32 cast, returning an error instead of silently wrapping negative. n <= 0 now returns ([]marketdata.Bar{}, true, nil), mirroring backtest.Scheduler's own historyView contract (addresses the smaller n<=0 mismatch both reviews flagged).

  3. EOF without SessionEnd looked like success (Rusty M0-02 Complete the initial ADR records #3, Copilot serve.go:166)guestRun now tracks sawSessionEnd; loop() returns an explicit error on io.EOF unless a real SessionEnd was observed first.

  4. Guest-side invariants not enforced before crossing into strategy code (Rusty M0-03 Add contribution and Definition-of-Done documentation #4, Copilot convert.go:49/109/180/223):

    • fromWireBar now calls marketdata.Bar.Validate() and wraps any failure in ErrInvalidWireValue.
    • fromWirePositionSnapshot now enforces the flat<->AvgPrice==nil invariant both directions.
    • toWireDataRequirement bounds WarmupBars to math.MaxInt32 before the int32 cast.
    • toWireDescribedIntent rejects a zero-but-non-nil TargetExposure quantity, matching order.NewIntent's own positive-quantity requirement, so a malformed intent fails guest-side instead of round-tripping to the host only to be rejected there.
  5. Capability negotiation not verified (Rusty M0-04 Add pull-request and issue templates #5, Copilot serve.go:92) — new verifyNegotiatedCapabilities checks the accepted HandshakeResponse.capabilities against what the guest actually requires (currently CAPABILITY_FILL_HANDLER when the strategy implements FillHandler) and fails the Handshake explicitly on a mismatch, instead of silently running degraded.

  6. Logging unconditionally discarded (Copilot serve.go:203) — new WithLogger option; Environment.Logger now defaults to slog.Default() instead of a hardcoded discard handler. (logging/arch_test.go gained a strategysdk exemption from the no-slog.Default rule, for the same reason cmd/ already has one: Serve is the guest process's own composition root.)

  7. No dial/handshake timeout (Copilot serve.go:59) — new WithHandshakeTimeout option (DefaultHandshakeTimeout = 10s) bounds the Handshake call.

  8. README/View contract mismatch for OnFill (Rusty smaller item, Copilot README.md:117, suppressed comment) — README now states explicitly that HistoryBars during OnFill always returns ok=false, err=nil without an RPC, since v1 scopes GetHistoryBars to an in-flight BarEvent only.

New regression tests cover each of the above: capability-mismatch rejection, handshake timeout, history-bars timeout (distinguishing err from ok=false), EOF-without-SessionEnd, WithLogger routing to Environment.Logger, plus six new convert_internal_test.go cases for the new invariant checks.

Verification: go build ./..., go vet ./..., gofmt -l clean, go test ./strategysdk/... -race (88.7% coverage), full repo go test ./... green, golangci-lint run ./... clean (only the 3 known pre-existing unrelated research-runs/ errcheck findings remain).

@rustyeddy rustyeddy left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rereviewed latest head 37d776e.

The major findings from the previous review are addressed well:

  • session callbacks/history now use stream.Context();
  • Handshake and GetHistoryBars have guest-owned deadlines;
  • HistoryBars now has an explicit error channel instead of collapsing transport failures into ok=false;
  • bare EOF without SessionEnd is abnormal;
  • the wire/domain invariant checks are substantially tightened;
  • capability negotiation is verified;
  • OnFill history semantics are documented;
  • logging is configurable/default-visible.

I found one remaining consistency issue before merge:

Strategy.Describe() is called twice, and the second result becomes the SDK's local history-authorization set.

ServeConn first does:

wireDescriptor, err := toWireDescriptor(strat.Describe())

That exact descriptor is what the host accepts and uses to scope GetHistoryBars. But after Handshake, guestRun is initialized with:

requirements: strat.Describe().Requirements,

If Describe() is stateful, time/config-sensitive, or simply buggy and returns a different value on the second call, the guest-side HistoryBars declaration check no longer reflects the descriptor actually sent in Handshake. It can locally authorize a requirement the host never accepted (causing an RPC error), or reject one the host did accept. ADR-062 explicitly defines the Handshake descriptor as the single declaration used for all later requirement scoping.

Call Describe() once, retain that value (for example descriptor := strat.Describe()), convert that same value for Handshake, and use descriptor.Requirements for guestRun. A regression test with a strategy whose Describe changes across calls would lock this down nicely.

One smaller contract issue: WithLogger(nil) currently overwrites the non-nil default with nil, while Environment.Logger documents “Never nil.” Either make nil mean “use default” or reject/normalize it while applying options.

Other than the double-Describe issue, I don't see a new correctness blocker on this head. The connector currently exposes no commit-status entries for 37d776e, so I’m relying on the PR code/tests and the reported local verification for test status.

Addresses Rusty's re-review of 37d776e on PR #392:

- ServeConn now calls strat.Describe() exactly once, retaining the
  result for both the wire Handshake and guestRun's own local
  history-authorization set. Calling it a second time to build
  guestRun.requirements let a stateful/time-sensitive/buggy Describe
  implementation diverge from the descriptor the host actually
  accepted at Handshake — ADR-062 defines the Handshake descriptor as
  the single declaration all later GetHistoryBars scoping uses.
- WithLogger(nil) no longer overwrites the non-nil default logger,
  honoring Environment.Logger's documented "never nil" contract.

Tests: TestServeConn_DescribeCalledOnceAndReused (a Describe that
returns a different, undeclared requirement on its second call must
not locally authorize it), TestWithLogger_NilDoesNotOverrideDefault.

Verification: go build/vet clean, gofmt clean, strategysdk race tests
89.0% coverage, full `go test ./...` green, golangci-lint clean (only
the 3 known pre-existing research-runs/ errcheck findings).

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015fVsVuQCgkrhiYaXLxyUF3
@rustyeddy

Copy link
Copy Markdown
Owner Author

Pushed b05801c addressing your re-review of 37d776e:

  1. Double Describe() call / potential divergenceServeConn now calls strat.Describe() exactly once and reuses that value for both the wire Handshake (toWireDescriptor(descriptor)) and guestRun.requirements. A stateful/time-sensitive/buggy Describe implementation can no longer locally authorize (or reject) a requirement different from what the host actually accepted at Handshake. New regression test TestServeConn_DescribeCalledOnceAndReused: a guest whose Describe() returns a different, undeclared instrument/interval on its second call is proven not to get local HistoryBars authorization for it.

  2. WithLogger(nil) overwriting the non-nil defaultWithLogger now ignores a nil argument instead of overwriting defaultRunConfig's slog.Default(), honoring Environment.Logger's documented "never nil" contract. New test TestWithLogger_NilDoesNotOverrideDefault.

Verification: go build ./.../go vet ./... clean, gofmt -l clean, go test ./strategysdk/... -race (89.0% coverage), full repo go test ./... green, golangci-lint run ./... clean (only the 3 known pre-existing unrelated research-runs/ errcheck findings).

@rustyeddy rustyeddy left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rereviewed latest head b05801c.

The two remaining findings from the prior pass are fixed correctly:

  • Strategy.Describe() is now called exactly once, and that retained descriptor is used both to build the Handshake payload and to populate guestRun.requirements. That restores ADR-062's single-declaration invariant for later GetHistoryBars scoping. The stateful-Describe regression test is a good guard for this.
  • WithLogger(nil) now leaves the default logger intact, preserving Environment.Logger's documented non-nil contract, with a regression test.

I also rechecked the earlier fixes (stream-context propagation, unary deadlines, explicit HistoryBars errors, abnormal EOF handling, capability verification, wire-value validation, OnFill history semantics, and visible/configurable logging). They remain intact on this head.

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

One tiny non-blocking cleanup: guestRun.sawSessionEnd is effectively unnecessary because the SessionEnd switch case returns immediately, so the later EOF branch can never observe sawSessionEnd == true. That's harmless and not worth holding the PR for; it could simply be removed for clarity.

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

@rustyeddy
rustyeddy merged commit 07642ab into main Sep 18, 2026
1 check passed
@rustyeddy
rustyeddy deleted the feature/381-strategysdk branch September 18, 2026 22:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

external-strategy: Go SDK/runtime for strategy authors

2 participants