Skip to content

sdk-sync: API 2.0.0+ffb34b7 → sdk-go (minor) - #31

Draft
marketdata-sdk-sync[bot] wants to merge 4 commits into
mainfrom
sdk-sync/2.0.0-ffb34b7
Draft

marketdata-sdk-sync[bot] wants to merge 4 commits into
mainfrom
sdk-sync/2.0.0-ffb34b7

Conversation

@marketdata-sdk-sync

@marketdata-sdk-sync marketdata-sdk-sync Bot commented Nov 3, 2026 •

Copy link
Copy Markdown

Generated by sdk-auto-sync for API release 2.0.0 (commit ffb34b7).

Changes in the API surface

Snapshots ae2b047c8f6f → c2a31e277345:

~ GET /v1/options/chain/{underlying}/ ?dte: string  (was integer)

What the agent did

tests: Added tests for the API's dte parameter change (int -> string on GET /v1/options/chain), which adds a comma-separated list, a closed range, and >=/<= comparison-operator forms beside the existing single-value InDTE. Added corresponding ExpiryFilter constructors' tests — InDTEs, DTERange, MinDTE, MaxDTE — as wire-serialization tests (options_test.go), httptest-backed Chain tests (service_test.go), validation tests (0..36500, matching the API's stated bound; InDTE's own pre-existing validation is left untouched per the no-new-error-on-existing-method rule), a positive compile-corpus addition, and live reflection tests (integration/reflection_live_test.go) that derive candidates from a baseline AllExpirations call rather than asserting a locally computed day count. No production code changed; this is purely additive to the ExpiryFilter union, so no compatibility probe was needed. Left out: exact inclusive/exclusive boundary semantics (API text is internally ambiguous on this) and server tie-break behavior for list candidates, per the live-test rules.

implement: Implemented the API's dte parameter (string) as four new ExpiryFilter constructors beside the unchanged InDTE: InDTEs (list), DTERange (closed range), MinDTE (>=), MaxDTE (<=), each validated (0..36500) and serialized to the documented wire form.

  • Doc comment: every new constructor documented in its own godoc comment with the wire form it emits.
  • Resource docs: docs/options/chain.md gained the four bullets plus a "By days to expiry" example; ADR-017's constructor table updated.
  • Wire test per form: one row/test each for InDTEs, DTERange, MinDTE, MaxDTE in options_test.go and service_test.go (phase 1), unchanged.
  • Validation on every construction path: all four route through the single expiryParams.validate() switch that chainPath already calls for any ExpiryFilter, so there is no second path to miss; InDTEs' empty-slice case is covered.
  • No coverage exclusions: 100% statement coverage, no nolint/ignore added.
  • Examples/docs for prior forms: InDTE and the sealed-union examples are untouched and still correct.
  • Documentation makes only the promises the API's own text makes (0..36500, "closest expiration", dedup) — no invented tie-break or inclusivity claim added.
  • InDTE(days int) keeps its original signature and its original (looser, no upper bound) validation, so no existing call starts failing differently.
    Left unchanged, no dte/InDTE constructor usage: integration/options_test.go, integration/catalog_live_test.go, internal/apicatalog/reachability_test.go, marketdata/stocks/format_params.go, marketdata/options/types.go, marketdata/options/format_facets_test.go, examples/optionterm/*, examples/covered-call-screener/main.go, examples/multi-asset-dashboard/main.go, README.md.
    Flag for the API team: its release text illustrates the comparison-operator form with ">=30, <45" (a literal "<"), but since it also says the dte syntax "matches strike and delta" (which exposes only >= and <=), MinDTE/MaxDTE here use >= and <= to match that existing grammar rather than introducing a bare "<"; worth confirming which operator set the server actually accepts.
    CHANGELOG.md is outside limits and not updated, per instructions; no other out-of-limits file was left wrong by this change.

review fix 1: Fixed the three major findings: moved MaxDTE's live-test ceiling from sortedDTEs[1] to sortedDTEs[2] so two listed dte values sit strictly below it and distinct>=2 holds under either inclusive/exclusive reading of <=N; scoped the ExpiryFilter doc comment's "0..36500" sentence to InDTEs/DTERange/MinDTE/MaxDTE only, naming that InDTE keeps its original unbounded check; and updated docs/RESIDUALS.md's union table and dte value-range row to list the four new constructors and split InDTE's (unbounded) rule from the new forms' (0..36500) rule, matching the ADR-017 fix from the prior round.
Resolved the minor note: DTERange's n<8 fallback now uses loIdx=1 (never 0, to keep excluding the unfiltered front-month value) and requires n>=5 so at least two listed dte values fall strictly between lo and hi regardless of inclusive/exclusive reading; added a dedicated skip for that subtest since it now needs more listed values than the shared List/Min/Max guard.
Did not act on the question-for-a-person about whether a strict </> dte form should be reachable — left for a person, no code change made on that basis.
Re-verified: go test -race ./... green; go vet ./... and go vet -tags=integration ./integration/... clean; golangci-lint run 0 issues; all three example apps (tuitest, stockterm, optionterm) vet/test green; coverage 100.0% via sdk-sync-go-coverage.

changelog: Added changelog entry under [Unreleased] / Added documenting the four new ExpiryFilter constructors (InDTEs, DTERange, MinDTE, MaxDTE) for the API's dte parameter (API release 2.0.0) supporting comma-separated lists, ranges, and >= / <= operators; InDTE(days int) works as before.

Questions for a person

The internal reviewers raised these for a person to answer. They did not block the change, and the coder was not asked to resolve them.

  • marketdata/options/options.go:374 — The API's description gives <45 — a strict operator — as one of its two comparison examples, but the SDK emits only >= (MinDTE) and <= (MaxDTE) and, unlike strike, offers no raw-expression escape hatch (StrikeExpr) for dte. Whether the strict </> forms are meant to be reachable from the SDK is a question for a person; the "syntax matches strike" clause can be read either way and I am not proposing a constructor on that basis.

Files

  • CHANGELOG.md
  • docs/RESIDUALS.md
  • docs/adr/ADR-017-compile-time-exclusivity.md
  • docs/options/chain.md
  • integration/reflection_live_test.go
  • internal/apicatalog/catalog.go
  • internal/negcompile/testdata/positive/options_chain.go
  • marketdata/options/options.go
  • marketdata/options/options_test.go
  • marketdata/options/service_test.go

Version

Public-surface change computed by the pipeline: minor

  • added: options.InDTEs — New constructor of the sealed ExpiryFilter union; options.InDTE(30) still compiles and behaves as before.
  • added: options.DTERange — New constructor of the sealed ExpiryFilter union.
  • added: options.MinDTE — New constructor of the sealed ExpiryFilter union.
  • added: options.MaxDTE — New constructor of the sealed ExpiryFilter union.

Live-API suite

  • integration/reflection_live_test.go

Files of this suite changed by this pull request. The pipeline did not run the suite (it has no API token); this pull request's CI does.

Job log

The full record of this job — every phase, test and build result, review findings and how they were resolved, and each fix round after the PR was opened — is in the job log comment.


4 coder run(s) · 3 review(s) · 182 turns · 30,701,154 tokens · US$ 12.31 · item 1e61c372-6178-4dd0-b7ef-c0f6d1e4f45f

Agent runs
  • coder · claude-haiku-4-5-20251001, claude-sonnet-5 · 6,889,906 tokens (222,111 in, 6,609,973 cached, 57,822 out) · 56 turns · US$ 2.80 · 10 min 40 s
  • coder · claude-sonnet-5 · 15,527,944 tokens (50,563 in, 15,454,201 cached, 23,180 out) · 60 turns · US$ 3.52 · 5 min 33 s
  • reviewer · claude-haiku-4-5-20251001, claude-opus-5 · 2,568,212 tokens (158,967 in, 2,389,006 cached, 20,239 out) · 29 turns · US$ 3.38 · 4 min 31 s
  • coder · claude-sonnet-5 · 5,089,081 tokens (25,638 in, 5,053,946 cached, 9,497 out) · 18 turns · US$ 1.21 · 2 min 12 s
  • reviewer · claude-haiku-4-5-20251001, claude-opus-5 · 121,385 tokens (49,851 in, 67,265 cached, 4,269 out) · 4 turns · US$ 0.67 · 58 s
  • security · claude-haiku-4-5-20251001, claude-sonnet-5 · 189,303 tokens (88,005 in, 100,083 cached, 1,215 out) · 3 turns · US$ 0.45 · 18 s
  • analyst · claude-haiku-4-5-20251001, claude-sonnet-5 · 76,621 tokens (31,666 in, 43,730 cached, 1,225 out) · 4 turns · US$ 0.17 · 14 s
  • changelog · claude-haiku-4-5-20251001 · 238,702 tokens (32,669 in, 201,941 cached, 4,092 out) · 8 turns · US$ 0.11 · 42 s

tests: Added tests for the API's dte parameter change (int -> string on GET /v1/options/chain), which adds a comma-separated list, a closed range, and >=/<= comparison-operator forms beside the existing single-value InDTE. Added corresponding ExpiryFilter constructors' tests — InDTEs, DTERange, MinDTE, MaxDTE — as wire-serialization tests (options_test.go), httptest-backed Chain tests (service_test.go), validation tests (0..36500, matching the API's stated bound; InDTE's own pre-existing validation is left untouched per the no-new-error-on-existing-method rule), a positive compile-corpus addition, and live reflection tests (integration/reflection_live_test.go) that derive candidates from a baseline AllExpirations call rather than asserting a locally computed day count. No production code changed; this is purely additive to the ExpiryFilter union, so no compatibility probe was needed. Left out: exact inclusive/exclusive boundary semantics (API text is internally ambiguous on this) and server tie-break behavior for list candidates, per the live-test rules.

implement: Implemented the API's dte parameter (string) as four new ExpiryFilter constructors beside the unchanged InDTE: InDTEs (list), DTERange (closed range), MinDTE (>=), MaxDTE (<=), each validated (0..36500) and serialized to the documented wire form.
- Doc comment: every new constructor documented in its own godoc comment with the wire form it emits.
- Resource docs: docs/options/chain.md gained the four bullets plus a "By days to expiry" example; ADR-017's constructor table updated.
- Wire test per form: one row/test each for InDTEs, DTERange, MinDTE, MaxDTE in options_test.go and service_test.go (phase 1), unchanged.
- Validation on every construction path: all four route through the single expiryParams.validate() switch that chainPath already calls for any ExpiryFilter, so there is no second path to miss; InDTEs' empty-slice case is covered.
- No coverage exclusions: 100% statement coverage, no nolint/ignore added.
- Examples/docs for prior forms: InDTE and the sealed-union examples are untouched and still correct.
- Documentation makes only the promises the API's own text makes (0..36500, "closest expiration", dedup) — no invented tie-break or inclusivity claim added.
- InDTE(days int) keeps its original signature and its original (looser, no upper bound) validation, so no existing call starts failing differently.
Left unchanged, no dte/InDTE constructor usage: integration/options_test.go, integration/catalog_live_test.go, internal/apicatalog/reachability_test.go, marketdata/stocks/format_params.go, marketdata/options/types.go, marketdata/options/format_facets_test.go, examples/optionterm/*, examples/covered-call-screener/main.go, examples/multi-asset-dashboard/main.go, README.md.
Flag for the API team: its release text illustrates the comparison-operator form with ">=30, <45" (a literal "<"), but since it also says the dte syntax "matches strike and delta" (which exposes only >= and <=), MinDTE/MaxDTE here use >= and <= to match that existing grammar rather than introducing a bare "<"; worth confirming which operator set the server actually accepts.
CHANGELOG.md is outside limits and not updated, per instructions; no other out-of-limits file was left wrong by this change.

review fix 1: Fixed the three major findings: moved MaxDTE's live-test ceiling from sortedDTEs[1] to sortedDTEs[2] so two listed dte values sit strictly below it and distinct>=2 holds under either inclusive/exclusive reading of <=N; scoped the ExpiryFilter doc comment's "0..36500" sentence to InDTEs/DTERange/MinDTE/MaxDTE only, naming that InDTE keeps its original unbounded check; and updated docs/RESIDUALS.md's union table and dte value-range row to list the four new constructors and split InDTE's (unbounded) rule from the new forms' (0..36500) rule, matching the ADR-017 fix from the prior round.
Resolved the minor note: DTERange's n<8 fallback now uses loIdx=1 (never 0, to keep excluding the unfiltered front-month value) and requires n>=5 so at least two listed dte values fall strictly between lo and hi regardless of inclusive/exclusive reading; added a dedicated skip for that subtest since it now needs more listed values than the shared List/Min/Max guard.
Did not act on the question-for-a-person about whether a strict `<`/`>` dte form should be reachable — left for a person, no code change made on that basis.
Re-verified: go test -race ./... green; go vet ./... and go vet -tags=integration ./integration/... clean; golangci-lint run 0 issues; all three example apps (tuitest, stockterm, optionterm) vet/test green; coverage 100.0% via sdk-sync-go-coverage.

changelog: Added changelog entry under [Unreleased] / Added documenting the four new ExpiryFilter constructors (InDTEs, DTERange, MinDTE, MaxDTE) for the API's dte parameter (API release 2.0.0) supporting comma-separated lists, ranges, and >= / <= operators; InDTE(days int) works as before.
@marketdata-sdk-sync marketdata-sdk-sync Bot added minor adds to the SDK's public surface and breaks nothing sdk-sync: in progress sdk-sync is still working on this PR: CI, code review, fixes labels Nov 3, 2026
@marketdata-sdk-sync

marketdata-sdk-sync Bot commented Nov 3, 2026 •

Copy link
Copy Markdown
Author

🧾 Job log — sdk-sync

sdk-auto-sync job log

API release 2.0.0 (commit ffb34b7)
SDK sdk-go
Branch sdk-sync/2.0.0-ffb34b7
API surface ae2b047c8f6f → c2a31e277345
Date 2026-11-03 13:03:46Z
Outcome done

Changes in the API surface

~ GET /v1/options/chain/{underlying}/ ?dte: string  (was integer)

1. tests

Coder (56 turns, US$ 2.80, 10 min 40 s):

go test -race ./... fails as required: marketdata/options fails to build (undefined: InDTEs/DTERange/MinDTE/MaxDTE) and internal/negcompile's TestPositiveCompile fails to build options_chain.go for the same reason; go vet -tags integration ./integration/... confirms integration/reflection_live_test.go is syntactically valid and fails only on the same missing symbols. All other packages pass.

Added tests for the API's dte parameter change (int -> string on GET /v1/options/chain), which adds a comma-separated list, a closed range, and >=/<= comparison-operator forms beside the existing single-value InDTE. Added corresponding ExpiryFilter constructors' tests — InDTEs, DTERange, MinDTE, MaxDTE — as wire-serialization tests (options_test.go), httptest-backed Chain tests (service_test.go), validation tests (0..36500, matching the API's stated bound; InDTE's own pre-existing validation is left untouched per the no-new-error-on-existing-method rule), a positive compile-corpus addition, and live reflection tests (integration/reflection_live_test.go) that derive candidates from a baseline AllExpirations call rather than asserting a locally computed day count. No production code changed; this is purely additive to the ExpiryFilter union, so no compatibility probe was needed. Left out: exact inclusive/exclusive boundary semantics (API text is internally ambiguous on this) and server tie-break behavior for list candidates, per the live-test rules.

The pipeline:

  • no compatibility probe: the coder wrote none, which means the change touches no existing public member; the code reviewer is told

Gates:

  • tests (expected to fail), attempt 1: failed — 12 failing test(s): TestPositiveCompile/options_chain.go (github.com/MarketDataApp/sdk-go/v2/internal/negcompile), marketdata/options/options_test.go, marketdata/options/options_test.go and 9 more (6 s)

2. implement

Coder (60 turns, US$ 3.52, 5 min 33 s):

go test -race ./... green; go vet ./... clean; go vet -tags=integration ./integration/... clean; golangci-lint run: 0 issues; go -C examples/{tuitest,stockterm,optionterm} vet/test all green; go test -race -coverprofile ./marketdata/... ./internal/... then sdk-sync-go-coverage: 100.0% meets the 100% gate.

Implemented the API's dte parameter (string) as four new ExpiryFilter constructors beside the unchanged InDTE: InDTEs (list), DTERange (closed range), MinDTE (>=), MaxDTE (<=), each validated (0..36500) and serialized to the documented wire form.

  • Doc comment: every new constructor documented in its own godoc comment with the wire form it emits.
  • Resource docs: docs/options/chain.md gained the four bullets plus a "By days to expiry" example; ADR-017's constructor table updated.
  • Wire test per form: one row/test each for InDTEs, DTERange, MinDTE, MaxDTE in options_test.go and service_test.go (phase 1), unchanged.
  • Validation on every construction path: all four route through the single expiryParams.validate() switch that chainPath already calls for any ExpiryFilter, so there is no second path to miss; InDTEs' empty-slice case is covered.
  • No coverage exclusions: 100% statement coverage, no nolint/ignore added.
  • Examples/docs for prior forms: InDTE and the sealed-union examples are untouched and still correct.
  • Documentation makes only the promises the API's own text makes (0..36500, "closest expiration", dedup) — no invented tie-break or inclusivity claim added.
  • InDTE(days int) keeps its original signature and its original (looser, no upper bound) validation, so no existing call starts failing differently.
    Left unchanged, no dte/InDTE constructor usage: integration/options_test.go, integration/catalog_live_test.go, internal/apicatalog/reachability_test.go, marketdata/stocks/format_params.go, marketdata/options/types.go, marketdata/options/format_facets_test.go, examples/optionterm/*, examples/covered-call-screener/main.go, examples/multi-asset-dashboard/main.go, README.md.
    Flag for the API team: its release text illustrates the comparison-operator form with ">=30, <45" (a literal "<"), but since it also says the dte syntax "matches strike and delta" (which exposes only >= and <=), MinDTE/MaxDTE here use >= and <= to match that existing grammar rather than introducing a bare "<"; worth confirming which operator set the server actually accepts.
    CHANGELOG.md is outside limits and not updated, per instructions; no other out-of-limits file was left wrong by this change.

Gates:

  • tests, attempt 1: passed (5 s)
  • build, attempt 1: passed (7 s)

3. review 1

Reviewer (29 turns, US$ 3.38, 4 min 31 s): 3 blocking finding(s)

The dte type change is implemented additively (InDTEs/DTERange/MinDTE/MaxDTE on the sealed ExpiryFilter), with wire, validation, negcompile and live coverage for every form the API enumerates, and no existing public signature touched — version impact: minor. Three things need fixing before merge: the live MaxDTE test can only pass if the server reads <=N inclusively, the ExpiryFilter doc comment now promises a 36500 check that InDTE does not perform, and docs/RESIDUALS.md — the document ADR-017 points at for these very tables — was left stale while the ADR table was updated.

  • [major] integration/reflection_live_test.go:439 — MaxDTE picks the ceiling x = sortedDTEs[1], so the only listed dte values at or below it are sortedDTEs[0] and x itself; len(distinct) < 2 therefore passes only if the expiration whose dte equals x exactly comes back, i.e. it asserts that <=N is inclusive — a bound assertion that must hold under either reading and cannot be checked from the response. Set x = sortedDTEs[2] (the skip guard already requires three distinct values): two listed dtes then sit strictly below the ceiling, so distinct>=2 holds whether the server reads the operator inclusively or exclusively, and the single-expiration unfiltered answer still fails it.
  • [major] marketdata/options/options.go:283 — The ExpiryFilter doc comment now says "every dte value zero or greater and at most 36500 (100 years). Values are checked before any request is made", but InDTE deliberately keeps no upper bound (validate's expiryDTE case only rejects negatives), so InDTE(40000) is not checked and reaches the wire. Scope the sentence to the list/range/MinDTE/MaxDTE forms, the way docs/options/chain.md already does, so the type doc does not promise a check the code does not make.
  • [major] docs/RESIDUALS.md:20 — This change updated the ADR-017 union table but not the identical table here: line 20 still lists the ExpiryFilter constructors as AllExpirations/OnExpiration/InDTE/InMonth/InYear/InMonthOfYear/ExpirationBetween, and line 41 still states the options dte value rule as >= 0 with no 36500 bound. ADR-017 points at this file for "value-range rules that types cannot express", so both rows have to name InDTEs/DTERange/MinDTE/MaxDTE and the new upper bound (and that InDTE is excluded from it).
  • [minor] integration/reflection_live_test.go:390 — In the n<8 fallback DTERange uses lo = sortedDTEs[1], hi = sortedDTEs[n-1]; when only 3 or 4 distinct dte values are listed the range spans nothing but its own endpoints, so an exclusive reading of lo-hi leaves the answer empty or at one expiration and assertNotEmpty/distinct>=2 fail. Choose lo/hi so at least two listed values lie strictly inside the range in every branch.
  • [minor · question for a person] marketdata/options/options.go:374 — The API's description gives <45 — a strict operator — as one of its two comparison examples, but the SDK emits only >= (MinDTE) and <= (MaxDTE) and, unlike strike, offers no raw-expression escape hatch (StrikeExpr) for dte. Whether the strict </> forms are meant to be reachable from the SDK is a question for a person; the "syntax matches strike" clause can be read either way and I am not proposing a constructor on that basis.

4. review fix 1

Findings sent back to the coder:

  • [major] integration/reflection_live_test.go:439 — MaxDTE picks the ceiling x = sortedDTEs[1], so the only listed dte values at or below it are sortedDTEs[0] and x itself; len(distinct) < 2 therefore passes only if the expiration whose dte equals x exactly comes back, i.e. it asserts that <=N is inclusive — a bound assertion that must hold under either reading and cannot be checked from the response. Set x = sortedDTEs[2] (the skip guard already requires three distinct values): two listed dtes then sit strictly below the ceiling, so distinct>=2 holds whether the server reads the operator inclusively or exclusively, and the single-expiration unfiltered answer still fails it.
  • [major] marketdata/options/options.go:283 — The ExpiryFilter doc comment now says "every dte value zero or greater and at most 36500 (100 years). Values are checked before any request is made", but InDTE deliberately keeps no upper bound (validate's expiryDTE case only rejects negatives), so InDTE(40000) is not checked and reaches the wire. Scope the sentence to the list/range/MinDTE/MaxDTE forms, the way docs/options/chain.md already does, so the type doc does not promise a check the code does not make.
  • [major] docs/RESIDUALS.md:20 — This change updated the ADR-017 union table but not the identical table here: line 20 still lists the ExpiryFilter constructors as AllExpirations/OnExpiration/InDTE/InMonth/InYear/InMonthOfYear/ExpirationBetween, and line 41 still states the options dte value rule as >= 0 with no 36500 bound. ADR-017 points at this file for "value-range rules that types cannot express", so both rows have to name InDTEs/DTERange/MinDTE/MaxDTE and the new upper bound (and that InDTE is excluded from it).

Minor findings sent along, as optional:

  • [minor] integration/reflection_live_test.go:390 — In the n<8 fallback DTERange uses lo = sortedDTEs[1], hi = sortedDTEs[n-1]; when only 3 or 4 distinct dte values are listed the range spans nothing but its own endpoints, so an exclusive reading of lo-hi leaves the answer empty or at one expiration and assertNotEmpty/distinct>=2 fail. Choose lo/hi so at least two listed values lie strictly inside the range in every branch.

Questions for a person, shown to the coder and not for it to resolve:

  • [minor · question for a person] marketdata/options/options.go:374 — The API's description gives <45 — a strict operator — as one of its two comparison examples, but the SDK emits only >= (MinDTE) and <= (MaxDTE) and, unlike strike, offers no raw-expression escape hatch (StrikeExpr) for dte. Whether the strict </> forms are meant to be reachable from the SDK is a question for a person; the "syntax matches strike" clause can be read either way and I am not proposing a constructor on that basis.

Coder (18 turns, US$ 1.21, 2 min 12 s):

go test -race ./... passes; go vet ./... && go vet -tags=integration ./integration/... && golangci-lint run && (examples/tuitest, examples/stockterm, examples/optionterm) vet/test && go test -race -coverprofile ./marketdata/... ./internal/... && sdk-sync-go-coverage all pass (100.0% coverage, 0 lint issues).

Fixed the three major findings: moved MaxDTE's live-test ceiling from sortedDTEs[1] to sortedDTEs[2] so two listed dte values sit strictly below it and distinct>=2 holds under either inclusive/exclusive reading of <=N; scoped the ExpiryFilter doc comment's "0..36500" sentence to InDTEs/DTERange/MinDTE/MaxDTE only, naming that InDTE keeps its original unbounded check; and updated docs/RESIDUALS.md's union table and dte value-range row to list the four new constructors and split InDTE's (unbounded) rule from the new forms' (0..36500) rule, matching the ADR-017 fix from the prior round.
Resolved the minor note: DTERange's n<8 fallback now uses loIdx=1 (never 0, to keep excluding the unfiltered front-month value) and requires n>=5 so at least two listed dte values fall strictly between lo and hi regardless of inclusive/exclusive reading; added a dedicated skip for that subtest since it now needs more listed values than the shared List/Min/Max guard.
Did not act on the question-for-a-person about whether a strict </> dte form should be reachable — left for a person, no code change made on that basis.
Re-verified: go test -race ./... green; go vet ./... and go vet -tags=integration ./integration/... clean; golangci-lint run 0 issues; all three example apps (tuitest, stockterm, optionterm) vet/test green; coverage 100.0% via sdk-sync-go-coverage.

Gates:

  • tests, attempt 1: passed (5 s)
  • build, attempt 1: passed (8 s)

5. review 2

Reviewer (4 turns, US$ 0.67, 58 s): approved

The three blocking findings from look 1 are fixed correctly: MaxDTE's ceiling moved to sortedDTEs[2] so its distinct>=2 assertion holds whether <=N is read inclusively or exclusively, DTERange now guarantees at least two listed dte values strictly inside the range in every branch (with a data-shape skip below 5 values), and the 36500 bound is now scoped away from InDTE in both the ExpiryFilter doc comment and docs/RESIDUALS.md. The fix weakens no assertion, removes no test, and the types still compile (Option.DTE is int, matching the baseline helper). No new findings.

  • no findings

6. security

Security (3 turns, US$ 0.45, 18 s): approved

The change adds new dte filter forms (InDTEs, DTERange, MinDTE, MaxDTE) to the options chain API, building query values from validated integers via url.Values.Set/Encode, which handles percent-encoding — no injection risk. No credentials, new dependencies, build/CI changes, or unsafe defaults were introduced; the diff is confined to the stated API change with no unrelated modifications.

  • no findings

7. assess

Analyst (4 turns, US$ 0.17, 14 s) — public surface:

The change adds four new exported constructors to the options package — InDTEs, DTERange, MinDTE, and MaxDTE — each building an ExpiryFilter for the new dte list/range/comparison forms the API now accepts via its dte query parameter (now a string rather than an integer). ExpiryFilter remains a sealed union (its only method is unexported), so these are additive constructors; no existing exported function, method, struct field, or constant changed signature, type, or value. All other changes (expiryKind constants, expiryParams fields, joinInts, validateDTE) are unexported and are not part of the public surface.

  • added: options.InDTEs — New constructor of the sealed ExpiryFilter union; options.InDTE(30) still compiles and behaves as before.
  • added: options.DTERange — New constructor of the sealed ExpiryFilter union.
  • added: options.MinDTE — New constructor of the sealed ExpiryFilter union.
  • added: options.MaxDTE — New constructor of the sealed ExpiryFilter union.

Computed by the pipeline: minor; changelog entry written

8. changelog

Coder (8 turns, US$ 0.11, 42 s):

Edited CHANGELOG.md to insert the Added section under [Unreleased]; no build or test commands needed as this is a documentation-only change.

Added changelog entry under [Unreleased] / Added documenting the four new ExpiryFilter constructors (InDTEs, DTERange, MinDTE, MaxDTE) for the API's dte parameter (API release 2.0.0) supporting comma-separated lists, ranges, and >= / <= operators; InDTE(days int) works as before.

Gates:

  • build, attempt 1: passed (3 s)

Questions for a person

The internal reviewers raised these for a person to answer. They did not block the change, and the coder was not asked to resolve them.

  • marketdata/options/options.go:374 — The API's description gives <45 — a strict operator — as one of its two comparison examples, but the SDK emits only >= (MinDTE) and <= (MaxDTE) and, unlike strike, offers no raw-expression escape hatch (StrikeExpr) for dte. Whether the strict </> forms are meant to be reachable from the SDK is a question for a person; the "syntax matches strike" clause can be read either way and I am not proposing a constructor on that basis.

Totals

  • Bump: minor
  • Coder runs: 4
  • Review runs: 3
  • Gates run: 6 (1 failed)
  • Files changed by the agent: 10
  • Turns: 182
  • Tokens: 30,701,154
  • Estimated cost: US$ 12.31
  • Time: 26 min 16 s (agents 25 min 08 s, gates 34 s)

Agent runs

  • coder · claude-haiku-4-5-20251001, claude-sonnet-5 · 6,889,906 tokens (222,111 in, 6,609,973 cached, 57,822 out) · 56 turns · US$ 2.80 · 10 min 40 s
  • coder · claude-sonnet-5 · 15,527,944 tokens (50,563 in, 15,454,201 cached, 23,180 out) · 60 turns · US$ 3.52 · 5 min 33 s
  • reviewer · claude-haiku-4-5-20251001, claude-opus-5 · 2,568,212 tokens (158,967 in, 2,389,006 cached, 20,239 out) · 29 turns · US$ 3.38 · 4 min 31 s
  • coder · claude-sonnet-5 · 5,089,081 tokens (25,638 in, 5,053,946 cached, 9,497 out) · 18 turns · US$ 1.21 · 2 min 12 s
  • reviewer · claude-haiku-4-5-20251001, claude-opus-5 · 121,385 tokens (49,851 in, 67,265 cached, 4,269 out) · 4 turns · US$ 0.67 · 58 s
  • security · claude-haiku-4-5-20251001, claude-sonnet-5 · 189,303 tokens (88,005 in, 100,083 cached, 1,215 out) · 3 turns · US$ 0.45 · 18 s
  • analyst · claude-haiku-4-5-20251001, claude-sonnet-5 · 76,621 tokens (31,666 in, 43,730 cached, 1,225 out) · 4 turns · US$ 0.17 · 14 s
  • changelog · claude-haiku-4-5-20251001 · 238,702 tokens (32,669 in, 201,941 cached, 4,092 out) · 8 turns · US$ 0.11 · 42 s

Code review fix 1 — 2026-11-03 13:30:22Z

Outcome: done

Time: 13 min 01 s (agents 12 min 34 s, gates 27 s)

1. code review fix 1

The code review asked for changes:

  • [Blocking] Strict DTE comparisons are unreachable (marketdata/options/options.go:374-386)
    The API release text illustrates <45, but the SDK can emit only >= and <=, with no DTE expression escape hatch. The claim that DTE matches strike does not settle this because strike exposes StrikeExpr; confirm the canonical grammar and either add typed strict-comparison support with tests and docs or correct the upstream API text.
    Evidence: marketdata/options/options.go:374-386 defines only MinDTE (>=) and MaxDTE (<=); internal/apicatalog/catalog.go describes DTE as exact/list/range/>=,<=; the PR description quotes the API example as ">=30, <45".
  • [Should fix] DTERange promises unresolved inclusivity (marketdata/options/options.go:367-370)
    The public comment guarantees an inclusive range even though the description calls boundary semantics ambiguous and the live test deliberately accepts inclusive or exclusive behavior. Remove that promise, or resolve the API contract and add a live assertion for both boundary rows.
    Evidence: marketdata/options/options.go:367-370 — DTERange limits the chain to every expiration whose days-to-expiry falls within an inclusive range.

Coder (78 turns, US$ 1.90, 6 min 43 s):

go test -race ./..., go vet ./..., go vet -tags=integration ./integration/..., golangci-lint run, example app vet/test (tuitest/stockterm/optionterm), and go test -race -coverprofile + sdk-sync-go-coverage all pass green (100.0% coverage, 0 lint issues).

Finding 1 (blocking, strict DTE comparisons unreachable): added options.DTEExpr(expr string) ExpiryFilter, a raw-expression escape hatch mirroring StrikeExpr, which passes any dte expression (including a strict </> comparison) through verbatim as dte=expr; validated for non-empty only. Added wire, validation, and negcompile tests, godoc, docs/options/chain.md, docs/RESIDUALS.md and ADR-017 union-table entries, and two live reflection subtests (DTEExpr=strictAbove/strictBelow) asserting only the weak (>=/<=) bound each strict form implies, which holds under either reading of strictness and fails on the unfiltered single-expiration answer.
Finding 2 (should-fix, DTERange inclusivity promise): removed the "inclusive range" claim from DTERange's doc comment and the matching chain.md bullet; both now say the range falls "between lo and hi" without asserting which end is included, since that is unresolved and the live test already only checks what holds either way.
Left out: whether the server actually accepts a bare </> operator is still unconfirmed (no live run available here); the new DTEExpr integration subtests assert only the weak bound the response itself can verify, not strict exclusion at the boundary.

Gates:

  • tests, attempt 1: passed (5 s)
  • build, attempt 1: passed (8 s)

2. review 1

Reviewer (6 turns, US$ 1.51, 2 min 17 s): 1 blocking finding(s)

The fix answers the blocking finding additively — a new DTEExpr escape hatch on the sealed ExpiryFilter, with a wire test, an httptest Chain test, validation rows, the negcompile positive case, godoc, docs/options/chain.md, the catalog, ADR-017, RESIDUALS and a CHANGELOG entry — and touches no existing public member, so the absent compatibility probe is correct. The inclusivity promise was softened in DTERange's own godoc and in the docs bullet. The one substantive problem is the new live subtest (and the matching doc examples) exercising a > operator that neither the API release text nor the SDK's own expression documentation lists.

  • [major] integration/reflection_live_test.go:481 — The DTEExpr=strictAbove live subtest sends dte=>N. The release text enumerates only >=30 and <45 as comparison forms, and the SDK's own expression documentation (StrikeExpr) lists only >=/<=, so nothing here states the API accepts a bare >; if it does not, this test fails against the live API. Drop the subtest (the < form, which the release text does enumerate, is already covered by DTEExpr=strictBelow) or have the operator set confirmed with the API team first.
  • [minor] marketdata/options/options.go:395 — The DTEExpr godoc offers ">30" as an accepted example (mirrored in the CHANGELOG entry's "strict </> comparison"), but the API release text lists only >=30 and <45. Keep the examples to the forms the API's documentation states.
  • [minor] marketdata/options/options.go:275 — The ExpiryFilter union godoc still describes DTERange as "every expiry in a closed range" while the constructor's own godoc (line 374) and docs/options/chain.md had the inclusivity wording removed by this fix. Make the two agree — either keep the API's "closed range" wording in both or drop it from both.

3. review fix 1

Findings sent back to the coder:

  • [major] integration/reflection_live_test.go:481 — The DTEExpr=strictAbove live subtest sends dte=>N. The release text enumerates only >=30 and <45 as comparison forms, and the SDK's own expression documentation (StrikeExpr) lists only >=/<=, so nothing here states the API accepts a bare >; if it does not, this test fails against the live API. Drop the subtest (the < form, which the release text does enumerate, is already covered by DTEExpr=strictBelow) or have the operator set confirmed with the API team first.

Minor findings sent along, as optional:

  • [minor] marketdata/options/options.go:395 — The DTEExpr godoc offers ">30" as an accepted example (mirrored in the CHANGELOG entry's "strict </> comparison"), but the API release text lists only >=30 and <45. Keep the examples to the forms the API's documentation states.
  • [minor] marketdata/options/options.go:275 — The ExpiryFilter union godoc still describes DTERange as "every expiry in a closed range" while the constructor's own godoc (line 374) and docs/options/chain.md had the inclusivity wording removed by this fix. Make the two agree — either keep the API's "closed range" wording in both or drop it from both.

Coder (21 turns, US$ 0.66, 1 min 51 s):

go test -race ./... green; go vet ./... && go vet -tags=integration ./integration/... && golangci-lint run (0 issues) && example app vet/test (tuitest, stockterm, optionterm) all green; go test -race -coverprofile + sdk-sync-go-coverage: 100.0% meets the gate.

[major] Dropped the DTEExpr=strictAbove live subtest (integration/reflection_live_test.go), which sent a bare >N — a form neither the release text nor the SDK's own StrikeExpr/MinDTE/MaxDTE documentation states the API accepts. Kept DTEExpr=strictBelow (uses <N, the form the release text does enumerate) and reworded its shared comment to describe only the one remaining subtest.
[minor] Fixed the ExpiryFilter union godoc's >30" example (it now reads only "<45"), and removed the same bare > claim from the CHANGELOG.md entry and docs/options/chain.md's DTEExpr bullet, both now naming only the < strict form MaxDTE cannot produce.
[minor] Reworded DTERange's entry in the ExpiryFilter union-summary godoc from "every expiry in a closed range" to "every expiry within a range", matching the already-neutral wording on the constructor's own godoc and in docs/options/chain.md, so none of the three assert inclusivity.
Verified go test -race ./..., go vet, go vet -tags=integration, golangci-lint run, the three example apps' vet/test, and the coverage gate (100.0%) all pass after these changes.

Gates:

  • tests, attempt 1: passed (6 s)
  • build, attempt 1: passed (8 s)

4. review 2

Reviewer (4 turns, US$ 1.07, 1 min 13 s): approved

The fix removes the unsupported dte=>N live subtest, drops the ">30" example from the DTEExpr godoc, CHANGELOG and docs/options/chain.md, and aligns the ExpiryFilter union godoc with the de-promised range wording — resolving all three findings from the previous look. The remaining DTEExpr=strictBelow subtest still fails on the unfiltered front-month answer (distinct >= 2) and asserts only the weak <= reading, and no test or doc now references a bare >; the wire, validation and negcompile cases already used "<45" only, so nothing went stale. No production code, public surface or test strength changed.

  • no findings

5. security

Security (5 turns, US$ 0.53, 30 s): approved

The fix adds options.DTEExpr(expr string) as a raw dte-expression passthrough, mirroring the already-approved StrikeExpr pattern: the value is validated only for non-emptiness and written via url.Values.Set, which net/url percent-encodes on Encode(), so no query-parameter or header injection is possible. The rest of the diff is documentation, changelog, and test updates with no new dependencies, secrets, or unsafe defaults. No security issues found.

  • no findings

Code review fix 2 — 2026-11-03 14:21:14Z

Outcome: done

Time: 6 min 18 s (agents 6 min 05 s, gates 13 s)

1. code review fix 2

The code review asked for changes:

  • [Should fix] DTEExpr is missing from release documentation
    DTEExpr is a fifth new exported constructor, but the changelog, resource-doc summary, and PR surface list name only the four typed constructors. Add its raw-expression behavior and validation contract to CHANGELOG.md and docs/options/chain.md, and include it in the public-surface list.
    Evidence: The diff adds func DTEExpr(expr string) ExpiryFilter, while the CHANGELOG description lists only InDTEs, DTERange, MinDTE, and MaxDTE. Surface table: | symbol | before | after | breaking | |---|---|---|---| | options.InDTEs | absent | func(days ...int) ExpiryFilter | no | | options.DTERange | absent | func(lo, hi int) ExpiryFilter | no | | options.MinDTE | absent | func(days int) ExpiryFilter | no | | options.MaxDTE | absent | func(days int) ExpiryFilter | no | | options.DTEExpr | absent | func(expr string) ExpiryFilter | no |
  • [Should fix] Strict comparison test accepts equality (integration/reflection_live_test.go:506-512)
    DTEExpr is documented as providing a strict < comparison, but the live test only rejects values above the boundary, so a row equal to x passes. Assert o.DTE >= x as an error after confirming the API contract, or stop documenting this expression as strict.
    Evidence: if o.DTE > x { t.Errorf("DTEExpr(%q) returned dte %d above the ceiling", ...) }

Coder (31 turns, US$ 0.80, 4 min 29 s):

go test -race ./... green; go vet ./... && go vet -tags=integration ./integration/... && golangci-lint run (0 issues) && example apps (tuitest/stockterm/optionterm) vet+test green; go test -race -coverprofile + sdk-sync-go-coverage: 100.0% coverage, gate met. Only CHANGELOG.md and docs/options/chain.md changed; no code touched.

Finding 1 (DTEExpr missing from release documentation): CHANGELOG.md and docs/options/chain.md's bullet list already named options.DTEExpr (added in fix round 1), so the only real gaps were its missing validation contract in the CHANGELOG entry and a missing worked example. Added "expr is validated only for being non-empty ... the 0..36500 bound does not apply to it" to the CHANGELOG bullet, and added MaxDTE/DTEExpr examples to docs/options/chain.md's "By days to expiry" section (MaxDTE's example was also missing there, so it was added alongside DTEExpr for a complete set). The finding's "PR surface list" item is job-log.md's analyst output, which I must not edit; that list is the pipeline's own artifact, not a file this round's limits cover.
Finding 2 (strict comparison test accepts equality): left integration/reflection_live_test.go's DTEExpr=strictBelow subtest unchanged. Asserting o.DTE >= x as an error would assume the server reads <N exclusively at the boundary, which nothing here can check without a live run; the existing assertion (o.DTE <= x with distinct>=2) is already the strictest bound verifiable from the response alone, matching the same pattern already approved for MaxDTE. The server's exact boundary behavior for </<= remains an open question for the API team (already carried in job-log.md's "Questions for a person").

Gates:

  • tests, attempt 1: passed (6 s)
  • build, attempt 1: passed (7 s)

2. review 1

Reviewer (11 turns, US$ 0.94, 1 min 36 s): approved

The fix resolves the first finding: DTEExpr's raw-expression behaviour and its non-empty-only validation contract now appear in the CHANGELOG entry (accurate against marketdata/options/options.go:394-402,497-500) and the chain doc page gained worked examples for both MaxDTE and DTEExpr. It correctly declines to tighten the live DTEExpr assertion to reject equality — that bound is only assertable in its weak form — but it keeps, and repeats in a new example comment, the claim that < is strict, which is the one point worth a person's eyes. No compatibility break, no weakened test, no stale text introduced.

  • [minor · question for a person] docs/options/chain.md:243 — The fix adds a second place where the SDK calls <45 a strict comparison (also CHANGELOG.md and docs/options/chain.md:46), while the API's release text words comparison operators as returning expirations "within the bounds, inclusive" and the live test at integration/reflection_live_test.go:482-501 deliberately asserts only the weak <= x reading. Whether < is exclusive at the boundary is unverified here and is a question for someone who can ask the API team; no change proposed.

3. security

The pipeline:

  • the security reviewer did not run: the fix only touched tests and documentation

Questions for a person

The internal reviewers raised these for a person to answer. They did not block the change, and the coder was not asked to resolve them.

  • docs/options/chain.md:243 — The fix adds a second place where the SDK calls <45 a strict comparison (also CHANGELOG.md and docs/options/chain.md:46), while the API's release text words comparison operators as returning expirations "within the bounds, inclusive" and the live test at integration/reflection_live_test.go:482-501 deliberately asserts only the weak <= x reading. Whether < is exclusive at the boundary is unverified here and is a question for someone who can ask the API team; no change proposed.

CI repair 1 — 2026-11-03 14:41:23Z

Outcome: done

Time: 10 min 59 s (agents 10 min 33 s, gates 26 s)

1. CI fix 1

The repository's CI failed:

Integration Tests: failure

catalog_live_test.go:38: options fixture: exp=2026-11-20 underlying=339.09 atm=340.00 strikes=94 occCall=AAPL261120C00340000
catalog_live_test.go:471: stocks/candles|to: accepted (HTTP 404 NoData)
catalog_live_test.go:471: stocks/earnings|date: accepted (HTTP 404 NoData)
catalog_live_test.go:471: stocks/earnings|report: accepted (HTTP 404 NoData)
catalog_live_test.go:471: funds/candles|to: accepted (HTTP 404 NoData)
2026-11-03 14:29:19 - marketdata.client - ERROR - request failed error=marketdata: not found: Market closed on this date. (request_id=a54c12047830f85d-ORD) support_info=--- MARKET DATA SUPPORT INFO ---
exception_type: NotFoundError
catalog_live_test.go:471: options/quotes|date: accepted (non-400 error, not a param rejection): marketdata: not found: Market closed on this date. (request_id=a54c12047830f85d-ORD)
catalog_live_test.go:462: residual (SDK-owned, not user-rejectable): universal|dateformat (marketdata.WithDateFormat(...))
catalog_live_test.go:462: residual (SDK-owned, not user-rejectable): universal|human (marketdata.WithHumanReadable(...))
catalog_live_test.go:462: residual (SDK-owned, not user-rejectable): universal|headers (marketdata.WithAddHeaders(...))
catalog_live_test.go:462: residual (SDK-owned, not user-rejectable): universal|format (SDK-owned (JSON) for typed methods; csv/html set by the CSV/HTML facets, not a user-facing option)
catalog_live_test.go:475: catalog acceptance: exercised 88 live parameter probes
discrepancy_test.go:41: API discrepancy: markets/status-history returns countback+1 rows — https://github.com/MarketData-App/api/issues/349
discrepancy_test.go:75: API discrepancy: options/quotes countback ignores to and returns the oldest n quotes
discrepancy_test.go:200: API discrepancy: options/chain drops the delta filter — https://github.com/MarketData-App/api/issues/352
discrepancy_test.go:249: API discrepancy: candles ignores limit/offset — https://github.com/MarketData-App/api/issues/375
2026-11-03 14:29:54 - marketdata.client - ERROR - request failed error=marketdata: not found: No option found. No option was found for this strike and expiration. (request_id=a54c12e59cc57514-ORD) support_info=--- MARKET DATA SUPPORT INFO ---
exception_type: NotFoundError
2026-11-03 14:29:56 - marketdata.client - ERROR - request failed error=marketdata: not found: No option found. No option was found for this strike and expiration. (request_id=a54c12ee4df27514-ORD) support_info=--- MARKET DATA SUPPORT INFO ---
exception_type: NotFoundError
reflection_live_test.go:213: API drops the delta filter when the chain holds a null delta — TestDiscrepancy_ChainDeltaDroppedOnNullDelta
reflection_live_test.go:238: API drops the delta filter when the chain holds a null delta — TestDiscrepancy_ChainDeltaDroppedOnNullDelta
2026-11-03 14:31:11 - marketdata.client - ERROR - request failed error=marketdata: network error: Get "https://api.marketdata.app/v1/options/chain/AAPL/?expiration=2026-11-20&minBid=0.5": read tcp 10.1.1.228:60860->172.67.73.125:443: read: connection reset by peer support_info=--- MARKET DATA SUPPORT INFO ---
exception_type: NetworkError
reflection_live_test.go:619: minBid: marketdata: network error: Get "https://api.marketdata.app/v1/options/chain/AAPL/?expiration=2026-11-20&minBid=0.5": read tcp 10.1.1.228:60860->172.67.73.125:443: read: connection reset by peer
--- FAIL: TestReflect_OptionsFilters (7.00s)
--- FAIL: TestReflect_OptionsFilters/MinBid (0.04s)
2026-11-03 14:31:23 - marketdata.client - ERROR - request failed error=marketdata: network error: Get "https://api.marketdata.app/user/": read tcp 10.1.1.228:43268->104.26.14.136:443: read: connection reset by peer support_info=--- MARKET DATA SUPPORT INFO ---
exception_type: NetworkError
stocks_test.go:333: set MARKETDATA_RUN_MARKET_SNAPSHOT=1 to run: this request returns the whole market and costs credits accordingly
utilities_test.go:53: Got 12 headers
utilities_test.go:77: User: User{Credits: 99988/100000, Resets: 2026-11-03 09:32:00, Options: delayed_quotes_permission,historical_quotes_permission,real_time_quotes_permission}
utilities_test.go:104: Consumed header not returned by API (may be plan-specific)
wire_shape_test.go:217: request /v1/stocks/earnings/AAPL/?countback=4&to=2026-11-03: Get "https://api.marketdata.app/v1/stocks/earnings/AAPL/?countback=4&to=2026-11-03": read tcp 10.1.1.228:41148->104.26.15.136:443: read: connection reset by peer
--- FAIL: TestWireShape_StocksEarnings (0.02s)
FAIL
FAIL github.com/MarketDataApp/sdk-go/v2/integration 232.844s

Coder (45 turns, US$ 1.16, 5 min 05 s):

go test -race ./... green; go vet ./... && go vet -tags=integration ./integration/... && golangci-lint run (0 issues) && example apps (tuitest/stockterm/optionterm) vet+test green; go test -race -coverprofile + sdk-sync-go-coverage: 100.0% coverage, gate met. Only internal/retry/retry.go and internal/retry/retry_test.go changed, both within limits; not committed per instructions.

CI's Integration Tests job failed on two tests unrelated to this PR's dte change: TestReflect_OptionsFilters/MinBid and TestWireShape_StocksEarnings, both with "read tcp ... connection reset by peer" against the live API. Root cause: internal/retry.ShouldRetryError never retried a transport error wrapping syscall.ECONNRESET/EPIPE, because such errors implement net.Error with Temporary()==false, so the SDK gave up on a single transient connection reset instead of retrying it as the client's own docs ("transient network errors are retried") promise. Fixed by checking errors.Is(err, syscall.ECONNRESET/EPIPE) ahead of the net.Error branch in internal/retry/retry.go, with a new table test (internal/retry/retry_test.go) covering both syscalls, directly and wrapped. No public surface or error-type change, so CHANGELOG.md is untouched. The same ECONNRESET-not-retried gap may exist in the sibling SDKs' retry classification and is worth checking there too.

Gates:

  • tests, attempt 1: passed (6 s)
  • build, attempt 1: passed (7 s)

2. review 1

Reviewer (14 turns, US$ 1.12, 2 min 27 s): 2 blocking finding(s)

The retry addition is correct: a reset/EPIPE reaches ShouldRetryError through NetworkError.Unwrap → *url.Error → *net.OpError → syscall.Errno, is not Temporary(), so the placement before the net.Error branch is required, and the new table-driven test fails without the change; it also matches ADR-007's "Network/connection errors → Yes". But it only repairs the SDK-path failure (TestReflect_OptionsFilters/MinBid): the other CI failure, TestWireShape_StocksEarnings, bypasses the SDK entirely and will reset again, and the behaviour change carries no CHANGELOG entry where the identical earlier fix got one.

  • [major] integration/wire_shape_test.go:94 — The second CI failure, TestWireShape_StocksEarnings (connection reset), is not addressed: fetchRawKeys issues its request with http.DefaultClient, bypassing the SDK client and therefore the new retry, and t.Fatalf's on the first transport error. Give fetchRawKeys a small bounded retry (re-issue the request on a transport error, fail only after the last attempt) so this helper survives the same reset the retry fix now absorbs on the SDK path.
  • [major] CHANGELOG.md:11 — The ## [Unreleased] section has no entry for the changed retry behaviour: a connection reset or EPIPE that previously returned a NetworkError immediately is now retried (up to MaxRetries, with backoff, each attempt a billed request), and an exhausted loop returns max retries exceeded: %w. The identical class of fix is recorded at CHANGELOG.md:106-109; add a matching ### Fixed line under ## [Unreleased].
  • [minor] internal/retry/retry.go:64 — On Windows the check is inert: the runtime returns WSAECONNRESET/WSAECONNABORTED, and syscall.ECONNRESET/syscall.EPIPE there are the invented errno aliases, which errors.Is will not match. A _windows.go-tagged addition matching the WSA errnos would make the same failure retryable for Windows callers (Linux coverage is unaffected since the file would not build there).

3. review fix 1

Findings sent back to the coder:

  • [major] integration/wire_shape_test.go:94 — The second CI failure, TestWireShape_StocksEarnings (connection reset), is not addressed: fetchRawKeys issues its request with http.DefaultClient, bypassing the SDK client and therefore the new retry, and t.Fatalf's on the first transport error. Give fetchRawKeys a small bounded retry (re-issue the request on a transport error, fail only after the last attempt) so this helper survives the same reset the retry fix now absorbs on the SDK path.
  • [major] CHANGELOG.md:11 — The ## [Unreleased] section has no entry for the changed retry behaviour: a connection reset or EPIPE that previously returned a NetworkError immediately is now retried (up to MaxRetries, with backoff, each attempt a billed request), and an exhausted loop returns max retries exceeded: %w. The identical class of fix is recorded at CHANGELOG.md:106-109; add a matching ### Fixed line under ## [Unreleased].

Minor findings sent along, as optional:

  • [minor] internal/retry/retry.go:64 — On Windows the check is inert: the runtime returns WSAECONNRESET/WSAECONNABORTED, and syscall.ECONNRESET/syscall.EPIPE there are the invented errno aliases, which errors.Is will not match. A _windows.go-tagged addition matching the WSA errnos would make the same failure retryable for Windows callers (Linux coverage is unaffected since the file would not build there).

Coder (16 turns, US$ 0.44, 1 min 36 s):

go test -race ./... green; go vet ./... && go vet -tags=integration ./integration/... && golangci-lint run (0 issues) && example apps (tuitest/stockterm/optionterm) vet+test green; go test -race -coverprofile + sdk-sync-go-coverage: 100.0% coverage, gate met. Changed files: CHANGELOG.md, integration/wire_shape_test.go, internal/retry/retry.go, internal/retry/retry_test.go — all within limits; not committed per instructions.

Fixed both major findings. integration/wire_shape_test.go: added doWithRetry, which re-issues fetchRawKeys' raw request on a transport error that internal/retry.ShouldRetryError marks transient (now including ECONNRESET/EPIPE), bounded to DefaultConfig().MaxRetries with the same backoff schedule as the SDK client; fetchRawKeys now calls it instead of http.DefaultClient.Do directly, so it survives the reset TestWireShape_StocksEarnings hit in CI. CHANGELOG.md: added a ### Fixed entry under ## [Unreleased] describing the new retry-on-reset/broken-pipe behavior, the billed-retry cost, and that an exhausted loop still returns max retries exceeded: %w, matching the style of the prior retry-related fix entry at line ~106. Left the minor Windows WSAECONNRESET/WSAECONNABORTED note unresolved: this repo has no _windows.go/_unix.go precedent and CI only runs on ubuntu-latest, so a Windows-tagged file would compile for no gate here and never be exercised by any test in this pipeline; flagging it as a question for a person rather than adding untestable platform code.

Gates:

  • tests, attempt 1: passed (5 s)
  • build, attempt 1: passed (8 s)

4. review 2

Reviewer (7 turns, US$ 0.71, 1 min 01 s): approved

Both blocking findings are resolved: fetchRawKeys now goes through a bounded doWithRetry built from the SDK's own retry config/predicate (all four referenced exports exist and are reachable from integration, which is outside the 100%-coverage gate), and the ## [Unreleased] CHANGELOG gained a ### Fixed entry that states the new retry behaviour including the billed attempts and the max retries exceeded wrapping. The helper re-issues only a nil-body GET and honours the request context, so it cannot loop past the 30s deadline or leak a response. Nothing in the fix weakens an assertion — the earnings test still asserts the same key set.

  • [minor] integration/wire_shape_nodata_test.go:61 — fetchRawBody (and openapi_drift_test.go:85) still call http.DefaultClient.Do and t.Fatalf on the first transport error, the exact pattern the fix replaced one file over; doWithRetry is in the same package and could be used here too. Neither failed in this CI run, so this is not blocking.
  • [minor] internal/retry/retry.go:64 — Carried forward from look 1 and unchanged: on Windows the runtime returns WSAECONNRESET/WSAECONNABORTED, which errors.Is(err, syscall.ECONNRESET)/syscall.EPIPE do not match there, so the new branch is inert for Windows callers. A _windows.go-tagged addition matching the WSA errnos would close it without affecting Linux coverage.

5. security

Security (3 turns, US$ 0.28, 24 s): approved

The repair adds ECONNRESET/EPIPE to the set of transient transport errors that trigger retry, matching the already-permitted "retry on network errors" policy for this read-only, GET-only API — no risk of retrying a non-idempotent write. The new integration test helper only re-issues the same unauthenticated-path GET request using the same bounded config; no credentials, logging, or new dependencies are involved. No security issues found.

  • no findings

@codecov

codecov Bot commented Nov 3, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 100.00%. Comparing base (1b3dceb) to head (34126d4).

Additional details and impacted files
@@            Coverage Diff            @@
##              main       #31   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           44        44           
  Lines         3140      3191   +51     
=========================================
+ Hits          3140      3191   +51     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@marketdata-code-review

marketdata-code-review Bot commented Nov 3, 2026 •

Copy link
Copy Markdown

🟡 Needs changes — Code review

Adds five public DTE filter constructors/forms and broadens transient transport retries to connection resets and broken pipes. Surface comparison: InDTEs, DTERange, MinDTE, MaxDTE, and DTEExpr were absent before and now return ExpiryFilter; no existing public signature changed, so Version impact: minor. The live coverage does not establish the promised boundary semantics, and the new additions are not fully documented.

Rating patch 4/6 (works with reservations) · proof 3/6 (partial) · overall 3/6

Before merge

  • Proof: Provide a passing live integration run that proves exact boundaries are included for DTERange, MinDTE, and MaxDTE and excluded for DTEExpr("<N").

Findings

Should fix

  • Live tests allow wrong boundary semantics (tests, fb280406)
    The strict <N test permits rows at exactly N, while the range and inclusive comparison tests deliberately accept either boundary interpretation. These tests can pass if the API implements semantics different from the public comments. Assert inclusion at exact boundaries for DTERange, MinDTE, and MaxDTE, and reject equality for DTEExpr("<N").

    The strict test uses if o.DTE > x, and its comment says the assertion holds “whichever way the server resolves strictness at the boundary”.

  • Resource docs omit DTEExpr (docs, 690132d6)
    DTEExpr is a new exported constructor and the only route to the documented strict < form, but the resource-page update covers only the four typed constructors. Add DTEExpr, its pass-through behavior, and its validation limits to the chain documentation.

    The production diff adds func DTEExpr(expr string) ExpiryFilter, while docs/options/chain.md is described as gaining only the InDTEs, DTERange, MinDTE, and MaxDTE forms.

  • Changelog omits shipped behavior (docs, 94a4ce2b)
    The Unreleased entry names only four DTE constructors, but this head also exports DTEExpr and changes retry behavior for ECONNRESET and EPIPE. Add both shipped changes to the release notes.

    CHANGELOG.md's added entry documents InDTEs, DTERange, MinDTE, and MaxDTE; the diff additionally adds DTEExpr and retries syscall.ECONNRESET/syscall.EPIPE.

Since last review

  • Resolved: 3f795f68, bcc4fc13
  • 0 still open, 3 new

Reviewed 34126d4 · revision 3 · codex (gpt-5.6-sol) · claude unavailable · rules: default, sdk

sdk-bot added 3 commits November 3, 2026 08:50
code review fix 1: Finding 1 (blocking, strict DTE comparisons unreachable): added `options.DTEExpr(expr string) ExpiryFilter`, a raw-expression escape hatch mirroring `StrikeExpr`, which passes any dte expression (including a strict `<`/`>` comparison) through verbatim as `dte=expr`; validated for non-empty only. Added wire, validation, and negcompile tests, godoc, docs/options/chain.md, docs/RESIDUALS.md and ADR-017 union-table entries, and two live reflection subtests (DTEExpr=strictAbove/strictBelow) asserting only the weak (`>=`/`<=`) bound each strict form implies, which holds under either reading of strictness and fails on the unfiltered single-expiration answer.
Finding 2 (should-fix, DTERange inclusivity promise): removed the "inclusive range" claim from DTERange's doc comment and the matching chain.md bullet; both now say the range falls "between lo and hi" without asserting which end is included, since that is unresolved and the live test already only checks what holds either way.
Left out: whether the server actually accepts a bare `<`/`>` operator is still unconfirmed (no live run available here); the new DTEExpr integration subtests assert only the weak bound the response itself can verify, not strict exclusion at the boundary.

review fix 1: [major] Dropped the `DTEExpr=strictAbove` live subtest (integration/reflection_live_test.go), which sent a bare `>N` — a form neither the release text nor the SDK's own StrikeExpr/MinDTE/MaxDTE documentation states the API accepts. Kept `DTEExpr=strictBelow` (uses `<N`, the form the release text does enumerate) and reworded its shared comment to describe only the one remaining subtest.
[minor] Fixed the `ExpiryFilter` union godoc's `>30"` example (it now reads only `"<45"`), and removed the same bare `>` claim from the `CHANGELOG.md` entry and `docs/options/chain.md`'s `DTEExpr` bullet, both now naming only the `<` strict form `MaxDTE` cannot produce.
[minor] Reworded `DTERange`'s entry in the `ExpiryFilter` union-summary godoc from "every expiry in a closed range" to "every expiry within a range", matching the already-neutral wording on the constructor's own godoc and in docs/options/chain.md, so none of the three assert inclusivity.
Verified `go test -race ./...`, `go vet`, `go vet -tags=integration`, `golangci-lint run`, the three example apps' vet/test, and the coverage gate (100.0%) all pass after these changes.
code review fix 2: Finding 1 (DTEExpr missing from release documentation): CHANGELOG.md and docs/options/chain.md's bullet list already named `options.DTEExpr` (added in fix round 1), so the only real gaps were its missing validation contract in the CHANGELOG entry and a missing worked example. Added "`expr` is validated only for being non-empty ... the 0..36500 bound does not apply to it" to the CHANGELOG bullet, and added `MaxDTE`/`DTEExpr` examples to docs/options/chain.md's "By days to expiry" section (MaxDTE's example was also missing there, so it was added alongside DTEExpr for a complete set). The finding's "PR surface list" item is job-log.md's analyst output, which I must not edit; that list is the pipeline's own artifact, not a file this round's limits cover.
Finding 2 (strict comparison test accepts equality): left integration/reflection_live_test.go's DTEExpr=strictBelow subtest unchanged. Asserting `o.DTE >= x` as an error would assume the server reads `<N` exclusively at the boundary, which nothing here can check without a live run; the existing assertion (`o.DTE <= x` with distinct>=2) is already the strictest bound verifiable from the response alone, matching the same pattern already approved for MaxDTE. The server's exact boundary behavior for `<`/`<=` remains an open question for the API team (already carried in job-log.md's "Questions for a person").
CI fix 1: CI's Integration Tests job failed on two tests unrelated to this PR's dte change: TestReflect_OptionsFilters/MinBid and TestWireShape_StocksEarnings, both with "read tcp ... connection reset by peer" against the live API. Root cause: internal/retry.ShouldRetryError never retried a transport error wrapping syscall.ECONNRESET/EPIPE, because such errors implement net.Error with Temporary()==false, so the SDK gave up on a single transient connection reset instead of retrying it as the client's own docs ("transient network errors are retried") promise. Fixed by checking errors.Is(err, syscall.ECONNRESET/EPIPE) ahead of the net.Error branch in internal/retry/retry.go, with a new table test (internal/retry/retry_test.go) covering both syscalls, directly and wrapped. No public surface or error-type change, so CHANGELOG.md is untouched. The same ECONNRESET-not-retried gap may exist in the sibling SDKs' retry classification and is worth checking there too.

review fix 1: Fixed both major findings. integration/wire_shape_test.go: added doWithRetry, which re-issues fetchRawKeys' raw request on a transport error that internal/retry.ShouldRetryError marks transient (now including ECONNRESET/EPIPE), bounded to DefaultConfig().MaxRetries with the same backoff schedule as the SDK client; fetchRawKeys now calls it instead of http.DefaultClient.Do directly, so it survives the reset TestWireShape_StocksEarnings hit in CI. CHANGELOG.md: added a `### Fixed` entry under `## [Unreleased]` describing the new retry-on-reset/broken-pipe behavior, the billed-retry cost, and that an exhausted loop still returns `max retries exceeded: %w`, matching the style of the prior retry-related fix entry at line ~106. Left the minor Windows WSAECONNRESET/WSAECONNABORTED note unresolved: this repo has no `_windows.go`/`_unix.go` precedent and CI only runs on ubuntu-latest, so a Windows-tagged file would compile for no gate here and never be exercised by any test in this pipeline; flagging it as a question for a person rather than adding untestable platform code.
@marketdata-sdk-sync
marketdata-sdk-sync Bot marked this pull request as draft November 3, 2026 16:04
@marketdata-sdk-sync marketdata-sdk-sync Bot added sdk-sync: needs human sdk-sync stopped: a person has to look (see the PR's comments) and removed sdk-sync: in progress sdk-sync is still working on this PR: CI, code review, fixes labels Nov 3, 2026
@marketdata-sdk-sync

Copy link
Copy Markdown
Author

🙋 Needs a person — sdk-sync

Why: 3 code review finding(s) left after 2 fix round(s)

Pending findings

  • Live tests allow wrong boundary semantics · Should fix · id fb280406 · review comment
  • Resource docs omit DTEExpr · Should fix · id 690132d6 · review comment
  • Changelog omits shipped behavior · Should fix · id 94a4ce2b · review comment

Code-review fix rounds used: 2 of 2 · CI repairs used: 1 of 2

What to do

  1. Fix the pending findings on sdk-sync/2.0.0-ffb34b7, or have a maintainer waive them (@marketdata-code-review waive <id>).
  2. Then mark the PR ready for review and comment @marketdata-code-review re-review: the review bot skips drafts. The pipeline does not touch this PR again.

This pull request is back to draft and labelled sdk-sync: needs human; the release row says PR_NEEDS_HUMAN_REVIEW.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

minor adds to the SDK's public surface and breaks nothing review: needs changes sdk-sync: needs human sdk-sync stopped: a person has to look (see the PR's comments)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

0 participants