fix: spec drift — Klear Bearer auth (breaking) + cfbenchmarks WS + account/perps additions (closes #443)

[TexasCoding]

Spec drift reconciliation (closes #443)

Re-vendored the upstream specs and reconciled the hand-written SDK + contract-drift suites. The vendored specs had drifted again since the issue was filed (2026-06-08), so this PR resolves the current upstream state, which is broader than the issue's summary:

Spec	Issue checksum	Current upstream	New surface
openapi.yaml	baf3bf09…	760df935…	+ /account/api_usage_level/upgrade, AccountApiLimits.grants
asyncapi.yaml	8ee211e1…	25e85ecd…	+ cfbenchmarks_value channel
perps_openapi.yaml	4e133710…	4336b33d…	+ /account/limits/perps, notional/leverage fields
perps_asyncapi.yaml	1dd27956…	1dd27956… ✓	ticker notional fields
perps_scm_openapi.yaml	d8c0d9c4…	d8c0d9c4… ✓	log_in removed → Bearer auth

⚠️ Breaking change — Klear (SCM) auth

Upstream removed POST /log_in and switched the Klear API from cookie-session to a static Bearer token (Authorization: Bearer <admin_user_id>:<access_token>).

• KlearClient / AsyncKlearClient now require admin_user_id + access_token at construction (or from_env() → KALSHI_KLEAR_ADMIN_USER_ID / KALSHI_KLEAR_ACCESS_TOKEN).
• KlearAuth(admin_user_id, access_token) is now a Bearer-credential holder; the header is injected per-request by the Klear resource base.
• Removed: KlearClient.login(), KlearClient.is_authenticated, client.auth, LogInRequest, LogInResponse, AuthResource.

Because this breaks a public surface, the version is bumped 3.3.0 → 4.0.0 (semver major). Happy to re-target the release if you'd rather batch it.

Added (additive)

• cfbenchmarks_value WS channel — subscribe_cfbenchmarks_value(index_ids=[...]) + CFBenchmarks* models + dispatch/param/update_subscription plumbing + error-code notes 23–25.
• AccountResource.upgrade() — POST /account/api_usage_level/upgrade.
• AccountApiLimits.grants + new ApiUsageLevelGrant model.
• MarginAccountResource.api_limits() — GET /account/limits/perps (reuses AccountApiLimits).
• Perps notional/leverage fields on MarginMarket, MarginMarketCandlestick, and the margin ticker WS payload.
• Subaccount prose updated 1‑32 → 1‑63 (doc-only; no validator change).

Verification

• ruff check . ✅ · mypy kalshi/ (strict) ✅ — no issues in 157 files
• Full suite: 4221 passed, 316 skipped. The 6 originally-red drift tests are green.
• ⚠️ Note: 4 pre-existing test_from_env_* failures (client/async_client) appear only in the full run due to env-var test-pollution — they pass in isolation and were already failing on main before this PR. Left out of scope; happy to file a separate issue.

🤖 Generated with Claude Code

[claude]

Code Review — PR #444: Spec drift reconciliation (v4.0.0)

Overall this is a solid, well-structured PR. The Klear auth migration is clean, the new models are consistent with existing patterns, and test coverage is broad. A few things worth addressing before merge.

---

Issues

1. upgrade() bypasses the Pydantic body convention (convention violation)

File: kalshi/resources/account.py

self._post("/account/api_usage_level/upgrade", json={}, extra_headers=extra_headers)

CLAUDE.md says: "Every POST/PUT/DELETE-with-body method builds a request model internally and serializes via model.model_dump(exclude_none=True, by_alias=True, mode="json"). Don't build inline dict bodies in resource methods."

Sending json={} directly bypasses this. If the OpenAPI spec truly defines no request body for this endpoint, the right call is self._post("/account/api_usage_level/upgrade", extra_headers=extra_headers) with no json kwarg at all. If the server requires an explicit Content-Type: application/json with an empty body, a zero-field Pydantic model (with extra="forbid") makes the intent explicit and lets the contract drift test catch phantom fields:

class UpgradeRequest(BaseModel):
    model_config = ConfigDict(extra="forbid")
    # No fields — server expects an empty JSON object body

The test comment even flags this: # Bodyless POST: SDK sends json={} so httpx pins the Content-Type header — "bodyless" and "sends json={}" are contradictory. If there really is a body, use the Pydantic pattern; if there isn't, don't send json={}.

2. Missing contract documentation for the upgrade endpoint

File: tests/_contract_support.py

The new MethodEndpointEntry for POST /account/api_usage_level/upgrade has no request_body_schema. CLAUDE.md: "POST/PUT/DELETE entries must set request_body_schema."

If the spec has no body schema (and the json={} above is unintentional), the right fix is to also not send a body, so this becomes moot. If the body is intentional and there's no spec schema, add a comment or an EXCLUSIONS entry with a reason string explaining why it's absent. The body drift test cannot validate this endpoint either way — that gap should be explicit.

3. DollarDecimal for a non-dollar index value

File: kalshi/ws/models/cfbenchmarks.py

class CFBenchmarksAvgData(BaseModel):
    value: DollarDecimal  # e.g. 65000.12345678 (BRTI)

DollarDecimal is named and documented as a financial dollar amount type. CF Benchmarks index values like BRTI are not prices in the Kalshi sense — they're reference rates from a third-party publisher and can be 8 dp BTC prices. Functionally it works (it's just Decimal under the hood), but it reads as a misnomer. Consider using Annotated[Decimal, ...] directly or a MultiplierDecimal-style alias, especially since there's no _dollars field name mapping here.

---

Minor / Low-priority

4. CFBenchmarksValuePayload.data — intentional, but worth a pointer

The data: str field (raw CF Benchmarks JSON frame as a string) is correctly left unparsed since the schema is upstream-defined and may vary per index. The docstring explains this. Could add a one-liner pointing callers to json.loads(msg.data) to save them the guess-work.

5. Pre-existing test_from_env_* env-var pollution

PR notes 4 pre-existing test_from_env_* failures that pass in isolation but fail in the full suite. These are not introduced by this PR, but since env-var pollution is an isolation failure (likely missing monkeypatch teardown), it's worth filing a separate issue and tracking it rather than leaving "they pass in isolation" as a long-term comment. The note in the PR description covers it — just flagging it should be a filed issue, not a known caveat.

---

What looks good

• KlearAuth repr correctly redacts access_token=<redacted> and raises on empty credentials at construction.
• Auth migration is complete — LogInRequest/LogInResponse/AuthResource removed from models, resources, exports, and contract map. No dead references found.
• New models (ApiUsageLevelGrant, CFBenchmarks*) follow the extra="allow" / AliasChoices conventions expected for response/WS models.
• NullableList for grants is tested with null → [] coercion — the behavior is validated, not just hoped for.
• All exports updated across kalshi/__init__.py, kalshi/ws/models/__init__.py, kalshi/perps/klear/__init__.py.
• Async parity — every new sync method has an async counterpart with matching behavior and test coverage.
• WS dispatch updated — cfbenchmarks_value and cfbenchmarks_value_indexlist channels registered in MESSAGE_MODELS and validated via test_all_channel_types_have_models.

---

Summary

Two things need resolution before merge:

1. Decide whether upgrade() sends an empty JSON body or no body, and align the implementation, test comment, and contract entry accordingly.
2. Either add a Pydantic model for the upgrade body or explicitly document the absence of request_body_schema via an EXCLUSIONS entry.

The DollarDecimal naming for the CFBenchmarks value field is low-urgency but worth addressing in a follow-up.

[TexasCoding]

Thanks for the review — addressed in 6a01a2d. Point-by-point:

1 & 2 — upgrade() body / contract entry. The endpoint has no requestBody in the spec, so the "build a Pydantic body model" convention (which is scoped to POST/PUT/DELETE-with-body) doesn't apply here. The json={} is the codebase's established no-body pattern — subaccounts.create does exactly this, with the same rationale (httpx omits Content-Type for a bodyless POST and demo rejects it with invalid_content_type), and is likewise registered with no request_body_schema. Dropping json={} would break against demo; an empty extra="forbid" model would be inconsistent with every other no-body endpoint. So I kept the behavior but made the intent explicit: the resource comment now references the subaccounts.create precedent, the "contradictory" test comment is reworded, and the MethodEndpointEntry notes why there's no body schema.

3 — DollarDecimal on the CFBenchmarks value. Kept, with a clarifying docstring. The tracked indices (BRTI, ETHUSD_RTI) are USD-denominated reference rates, so DollarDecimal is semantically fair, and it's the codebase's canonical "exact Decimal from string, no float drift" type. The three Decimal aliases are byte-identical implementations differing only in intent, so adding a 4th for one field felt like over-engineering — open to a dedicated alias in a follow-up if you'd prefer.

4 — json.loads(msg.data) pointer. Added to the CFBenchmarksValuePayload docstring.

5 — env-var pollution. Filed as #445 with the confirmed root cause: tests/integration/conftest.py uses a session-scoped monkeypatch.setenv("KALSHI_DEMO", "true") (undoes only at session end), which leaks into later unit from_env tests that don't delenv("KALSHI_DEMO"). Pre-existing on main; deterministic repro and suggested fixes are in the issue. Left out of this PR to keep it scoped to the spec drift.

[claude]

Code Review

Good overall — the breaking change is well-scoped, the additive surface follows existing patterns, and the PR description is thorough. A few items worth addressing before merge.

---

Breaking change — Klear Bearer auth ✅

The migration is clean:

• KlearAuth(admin_user_id, access_token) validates non-empty at construction time — prevents silent null tokens.
• __repr__/__str__ redact the access token — good security hygiene.
• _with_auth() in _base.py injects the Authorization header per-request and the _require_session() guards are gone — correct pattern for header-based auth.
• from_env() raises a helpful ValueError if either env var is missing.

One concern: _with_auth() silently overwrites an Authorization key if one is already in extra_headers. This is unlikely to matter in practice since callers shouldn't be passing their own auth headers here, but a defensive guard (or at minimum a comment) would future-proof it:

def _with_auth(self, extra_headers: dict[str, str] | None) -> dict[str, str]:
    merged = dict(extra_headers) if extra_headers else {}
    # Caller-supplied Authorization is intentionally replaced by Bearer token
    merged["Authorization"] = self._klear_auth.authorization_header()
    return merged

---

AccountApiLimits.grants field — possible missing default

grants: NullableList[ApiUsageLevelGrant] has no default value. If the server returns a response that omits the grants key (e.g. older API version or a partial response), Pydantic will raise ValidationError. Given that AccountApiLimits already uses "extra": "allow", this could happen silently in prod. The pattern elsewhere is:

grants: NullableList[ApiUsageLevelGrant] = Field(default_factory=list)

Worth checking whether the OpenAPI spec marks this field as required or optional.

---

New WebSocket channel — cfbenchmarks_value ✅

Models are well-structured:

• DollarDecimal for value in CFBenchmarksAvgData — correct.
• last_60s_windowed_average_15min: CFBenchmarksAvgData | None = None — correctly optional (only present near quarter-hour close; the docstring explains this).
• CFBenchmarksIndexListMessage.id: int | None = None — optional with a clear comment explaining why.
• Both message types registered in CHANNEL_MESSAGE_MODELS in dispatch.py.

---

New endpoints ✅

• AccountResource.upgrade() — json={} to force Content-Type: application/json on a body-less POST is a reasonable workaround; the comment explains it. Returns None for 201 — correct.
• MarginAccountResource.api_limits() — Reusing AccountApiLimits from the prediction API is a good choice; reduces model duplication.

---

Perps notional/leverage fields ✅

leverage_estimates, volume_notional_value, volume_24h_notional_value, and open_interest_notional_value all follow the existing AliasChoices("field_dollars", "field") pattern correctly. MarginMarketCandlestick makes these non-nullable (appropriate for historical records), while MarginMarket and MarginTickerPayload make them nullable.

---

Test coverage — needs verification

The diff doesn't include any changes under tests/. Before merging, please confirm:

1. KlearClient — constructor raises on empty/None credentials; from_env() raises with missing vars; bearer header is injected correctly.
2. AccountResource.upgrade() — happy path (201 → None) + error path (403 when eligibility not met).
3. MarginAccountResource.api_limits() — response deserialization including grants field.
4. CFBenchmarksValueMessage / CFBenchmarksIndexListMessage — round-trip deserialization, especially the optional last_60s_windowed_average_15min field.
5. Drift tests — METHOD_ENDPOINT_MAP and BODY_MODEL_MAP updated for upgrade and api_limits; the PR description says the 6 previously-red drift tests are green, which is good.

---

Minor

• The 4 pre-existing test_from_env_* failures are a real concern even if out of scope here — env-var test pollution at the suite level suggests those tests aren't properly isolated. Suggest filing a follow-up issue.
• pyproject.toml version bump to 4.0.0 is appropriate for the breaking Klear change.
• CHANGELOG entry is clear and comprehensive.

---

Summary: The implementation is solid and follows SDK conventions throughout. The two items I'd want resolved before shipping are (1) confirming grants has an appropriate default or verifying the spec marks it required, and (2) confirming test coverage for the new surface. Everything else is either correct or a minor style suggestion.

[TexasCoding]

Follow-up on the second review + the env-flake:

grants requiredness (verified). The perps & predictions GetAccountApiLimitsResponse schemas both list grants in their required array, so it's modeled required as NullableList[ApiUsageLevelGrant] — a missing key raises (surfacing drift), null coerces to [] (Kalshi's empty-as-null convention). TestSpecDrift::test_required_drift[AccountApiLimits] (a hard-fail) enforces the requiredness, and the null→[] path is covered by test_account.py::TestAsyncAccountLimits::test_returns_limits and test_margin_account.py::TestApiLimits::test_null_grants_coerced.

Test coverage (it's in the diff). The branch does include the test changes — e.g. tests/test_account.py (upgrade happy/403/auth + grants), tests/perps/test_margin_account.py::TestApiLimits, tests/perps/test_markets.py (notional/leverage), tests/ws/test_models.py::TestCFBenchmarksModels + tests/ws/test_client.py::TestSubscribeCFBenchmarks (round-trip incl. the optional last_60s_windowed_average_15min), tests/ws/test_dispatch.py (channel-type completeness), and the rewritten tests/perps/klear/test_auth.py (KlearAuth empty-cred raise, header injection, redaction, from_env missing-vars raise). The 6 originally-red drift tests are green.

Env-var pollution → fixed here (closes #445). Root-caused and fixed in f273b49: the two non-hermetic from_env tests now delenv("KALSHI_PRIVATE_KEY_PATH") like their siblings, so the integration conftest's session-scoped credential bridge can't collide with them. Full suite is now 4225 passed, 0 failed (was 4221 + 4 flakes).

[claude]

Code Review — PR #444: Spec drift (Klear Bearer auth + cfbenchmarks WS + account/perps additions)

Overall this is a well-executed spec-reconciliation PR. The architecture is sound, the test strategy is correct, and the docs are thorough. A few things worth addressing before merge:

---

🔴 Potential runtime breakage: required fields in MarginMarketCandlestick and MarginTickerPayload

kalshi/perps/models/markets.py — MarginMarketCandlestick:

volume_notional_value: DollarDecimal = Field(...)       # required — no default
open_interest_notional_value: DollarDecimal = Field(...)  # required — no default

kalshi/perps/ws/models/ticker.py — MarginTickerPayload:

volume_notional_value: DollarDecimal = Field(...)        # required
volume_24h_notional_value: DollarDecimal = Field(...)    # required
open_interest_notional_value: DollarDecimal = Field(...) # required

These are required (no None default). If the server does a gradual rollout and any response omits these fields, deserialization silently breaks with a ValidationError. Compare with MarginMarket, where the same three fields are correctly typed DollarDecimal | None = Field(default=None, ...).

The MarginMarket treatment (optional with default) is the safer convention here unless the spec guarantees these are always present — the perps spec does list them under required:, so if the server is already reliably returning them this is fine. But it is worth confirming before merge, since contract tests won't catch a live-server omission.

If there's any doubt, making them optional with None defaults costs nothing and matches the MarginMarket precedent.

---

🟡 _with_auth doesn't cover _delete / _put

kalshi/perps/klear/resources/_base.py:

KlearSyncResource and KlearAsyncResource override _get and _post to inject the Authorization header, but not _delete or _put. The current Klear surface is GET/POST-only so this isn't a live bug, but it's an easy footgun if a new Klear endpoint uses DELETE or PUT in a future spec revision. At minimum, a comment noting the intentional omission would help future contributors.

---

🟡 KlearAuth credential validation accepts whitespace-only strings

kalshi/perps/klear/auth.py:

if not admin_user_id or not access_token:
    raise ValueError(...)

" " (whitespace) is truthy, so it passes validation and produces a malformed header "Bearer : ". Using if not admin_user_id.strip() or not access_token.strip(): would close this gap. Minor, but this is a security credential.

---

🟡 update_subscription index_ids silently drops empty list

kalshi/ws/channels.py:

if index_ids:
    params["index_ids"] = index_ids

Passing index_ids=[] silently omits the key rather than sending the empty list. This is probably intentional (an empty list makes no semantic sense for subscribe/unsubscribe), but it diverges from how market_tickers / market_ids are handled just above (those use if ... is not None). A brief comment or consistent if index_ids is not None + guard inside the subscription handler would make the intent explicit.

---

✅ What's well done

• Breaking-change handling: Semver major bump is correct. from_env() raises a clear ValueError with an actionable message when credentials are missing. The removal of login() / is_authenticated / AuthResource is clean.
• Auth injection design: Overriding _get/_post in the base to inject the header is the right approach — it covers the paginators automatically without touching every leaf method.
• Test coverage: The test_auth.py rewrite covers header injection, paginated endpoints, 401 → KalshiAuthError, from_env reads/missing-creds, repr redaction, and the async path. The old test_unauthenticated_raises_before_http removals are correct — with Bearer auth there's no un-authenticated construction state.
• NullableList[ApiUsageLevelGrant] for grants: Correct coercion of server null → [].
• cfbenchmarks models: data: str (raw upstream frame, unparsed) is the right call. The optional last_60s_windowed_average_15min matches the spec's semantics. DollarDecimal for the 8-dp value field is correct.
• upgrade() empty-body workaround: The json={} pattern for a bodyless POST is already established by subaccounts.create; the inline comment is justified.
• Env-var hermetics in test_client.py / test_async_client.py: The added monkeypatch.delenv("KALSHI_PRIVATE_KEY_PATH", raising=False) calls correctly isolate the tests from ambient CI credentials.
• Subaccount 1-32 → 1-63 propagation: Correctly updated across models, FIX messages, spec, and docs.
• __str__ = __repr__ on KlearAuth: Good consistency with LogInRequest's old pattern.

---

Nit: PERPS_SCM_BODY_MODEL_MAP comment

In tests/test_contracts.py:

PERPS_SCM_BODY_MODEL_MAP: dict[str, str] = {
    # SCM/Klear auth migrated to Bearer header (#443); LogInRequest removed.

The comment is fine but the dict now has no entry before the margin entries. If there's a test that asserts this map is non-empty, it'll still pass since WithdrawSettlementBalanceRequest is present. No action required, just noting it.

---

The two red items (required fields in candlestick/ticker, credential whitespace) are the ones I'd want explicit sign-off on before merge. Everything else is ready.

[TexasCoding]

Thanks — addressed the three minor items in 7e75a5c, and here's the reasoning on the 🔴:

🔴 Required notional fields on candlestick / ticker — keeping required (intentional). The SDK mirrors each schema's own required list, which is the codebase convention:

• MarginMarket.required does not include the notional fields → modeled optional. ✓
• MarginMarketCandlestick.required does include volume_notional_value_dollars + open_interest_notional_value_dollars → modeled required. Making them optional would hard-fail TestPerpsSpecDrift::test_response_drift[MarginMarketCandlestick] (_assert_perps_response_drift fails on "spec requires X but SDK field is optional"), so the only way to relax it is a PERPS_EXCLUSIONS entry of kind server_omits_despite_required citing a real demo+prod observation — which I can't fabricate.
• marginTickerPayload.msg.required includes all three → modeled required, consistent with the payload's pre-existing siblings (volume / volume_24h / open_interest are already required, no default). Making only the new fields optional wouldn't add real resilience — an omitted volume would already break the same parse.

So the asymmetry the review noticed is the spec's, faithfully mirrored. (Your second review also called non-nullable "appropriate for historical records" on the candlestick, which lines up.)

🟡 _with_auth not on _delete/_put. Added a docstring note in _base.py: only _get/_post are overridden because the Klear surface is GET/POST-only, and a future PUT/DELETE endpoint must add its override. I held off on speculative overrides for verbs that don't exist yet (per the repo's simplicity guidance), but the footgun is now documented at the override site.

🟡 KlearAuth whitespace creds. Fixed — now rejects whitespace-only (and None-safe), with new test cases.

🟡 update_subscription empty index_ids. Added a clarifying comment. Note market_tickers/market_ids just above also use a truthy if x: check (not is not None), so index_ids is consistent with them; an empty list is intentionally omitted since the actions require ≥1 id (server error code 24).

Nit (PERPS_SCM_BODY_MODEL_MAP): no map-non-empty assertion exists, and WithdrawSettlementBalanceRequest remains, so no action.

Suite still 4225 passed / 0 failed; ruff + mypy strict clean.

[claude]

Code Review — PR #444: Spec drift reconciliation + Klear Bearer auth

This is a well-executed PR. The breaking change is correctly scoped, the auth migration is sound, test coverage is thorough across the new surfaces, and all major project conventions are followed (forbid models, retry guard, DollarDecimal prices, no-builtin-list shadowing). Below are the actionable findings — one real bug, two test gaps, and a few minor notes.

---

Bug: whitespace-padded credentials survive KlearAuth validation

kalshi/perps/klear/auth.py — KlearAuth.__init__

The validation correctly rejects empty strings and whitespace-only strings (e.g. " "). But a value like " actual_id " (non-empty content with surrounding spaces) passes both guards and is stored verbatim:

if (
    not admin_user_id          # False for "  actual_id  " — non-empty string
    or not admin_user_id.strip()  # False — stripped is "actual_id", truthy
    ...
):

authorization_header() then emits:

Bearer   actual_id  :token

which will fail server-side auth silently. The comment correctly explains the short-circuit ordering (prevents AttributeError on None), but the credential should be stripped before storage:

def __init__(self, admin_user_id: str, access_token: str) -> None:
    admin_user_id = admin_user_id.strip()
    access_token = access_token.strip()
    if not admin_user_id or not access_token:
        raise ValueError(
            "KlearAuth requires a non-empty admin_user_id and access_token."
        )
    self._admin_user_id = admin_user_id
    self._access_token = access_token

This simplifies the guard and eliminates the subtle ordering dependency. The existing tests for "" and " " still pass; add a case for " admin-id " to cover the gap.

---

Test gap: update_subscription(index_ids=...) not exercised

kalshi/ws/channels.py — SubscriptionManager.update_subscription

The new index_ids parameter and the subscribe_indices / unsubscribe_indices action path for the cfbenchmarks_value channel (lines 1519–1540 of the diff) have no unit test. The subscribe-and-receive path is tested, but the post-subscribe management actions are not. Suggested additions to tests/ws/test_client.py:

1. update_subscription with action="subscribe_indices" and index_ids=["BRTI"] — assert that params["index_ids"] is sent.
2. update_subscription with index_ids=[] / index_ids=None — assert index_ids key is absent from the outgoing command.
3. update_subscription with action="indexlist" (no index_ids) — assert the control request is dispatched correctly.

---

Minor: CHANNEL_SEED_PARAMS key presence implies index_ids is mandatory for cfbenchmarks_value, but subscribe_cfbenchmarks_value accepts None

kalshi/ws/channels.py — CHANNEL_SEED_PARAMS

"cfbenchmarks_value": frozenset({"index_ids"}),

This registers index_ids as a seed param, so subscription startup code knows to pass it through. But subscribe_cfbenchmarks_value allows index_ids=None and conditionally omits it:

if index_ids:
    params["index_ids"] = index_ids

The AsyncAPI spec notes that without index_ids, no value events flow until a subscribe_indices action is sent. This behavior is documented in the docstring — good. Just make sure the CHANNEL_SEED_PARAMS entry is consistent with how the seed filtering works in the subscription manager (i.e., the frozenset entry is for filtering, not for validation that the field is present — if this is fine, no action needed).

---

Nit: from_env() env-var validation duplicated across KlearClient and AsyncKlearClient

The from_env() classmethods in both clients are near-identical. If a third constructor param is ever added, there will be two places to update. Not asking for a change now (CLAUDE.md says don't introduce abstractions for single-use), but worth noting if the client surfaces grow further.

---

What's well done

• Bearer header redaction in __repr__ — access_token=<redacted> in both KlearAuth.__repr__ and the client repr is the right call. Error messages don't leak credentials either.
• HTTPS enforcement with explicit allow_unknown_host — the warning logged on non-Kalshi plaintext hosts is a good guardrail.
• Contract test hygiene — log_in correctly removed from METHOD_ENDPOINT_MAP with a comment explaining the removal; new endpoints (upgrade, api_limits/perps) registered correctly.
• All new price fields use DollarDecimal — notional value fields, CF Benchmarks avg data — consistent.
• POST retry guard preserved — test_post_not_retried still verifies call_count == 1 under 503 for POST endpoints.
• Subaccount prose (1-32 → 1-63) updated consistently across all affected files.

---

Summary

Severity	Finding
Bug	KlearAuth stores whitespace-padded credentials verbatim → malformed Bearer header
Medium	update_subscription(index_ids=...) post-subscribe actions untested
Low	Missing test case for " admin-id " credential (padded, non-whitespace-only)
Nit	from_env() duplication between sync/async clients

The Klear auth migration and the additive surfaces (cfbenchmarks WS, account upgrade, perps notional fields) are solid. Fix the whitespace bug and plug the update_subscription test gap before merging.

[TexasCoding]

Both substantive items fixed in 551a67e:

🐞 Padded credentials (bug). Good catch — " id " passed the whitespace-only guard and was stored verbatim. Now strip-then-store, keeping the None-safety via (x or "").strip():

admin_user_id = (admin_user_id or "").strip()
access_token = (access_token or "").strip()
if not admin_user_id or not access_token:
    raise ValueError(...)

Added test_padded_credentials_are_stripped asserting KlearAuth(" admin-id ", " tok ").authorization_header() == "Bearer admin-id:tok". This also covers the "Low" padded-credential test-case ask.

🧪 update_subscription(index_ids=...) test gap. Added three cases to tests/ws/test_channels.py::TestUpdateSubscription: subscribe_indices sends index_ids; indexlist (no ids) omits the key; empty index_ids=[] is dropped.

Nits (no action, agreed): from_env sync/async duplication — left as-is per the repo's no-abstraction-for-single-use guidance, as you noted. The _CHANNEL_PARAMS (cfbenchmarks_value -> {"index_ids"}) entry is a filtering allow-set, not a presence requirement — index_ids stays optional on subscribe, consistent with the docstring's "subscribe first, add indices later" flow.

Suite green; ruff + mypy strict clean.

[claude]

Code Review — PR #444: Spec drift reconciliation + Klear Bearer auth

Overall this is a well-executed, broad reconciliation. The test suite passes at 4221 and the diff is clean. Below are findings sorted by severity.

---

Bugs / Correctness

MarginMarketCandlestick.volume_notional_value and open_interest_notional_value are required (no = None) — these are new required fields on an existing model. Any payload from the server that doesn't yet include them (e.g. a cached response, a spec-lagging test fixture, or a demo environment that's slower to update) will raise a ValidationError at parse time. The parallel optional fields on MarginMarket are correctly | None = None; the candlestick should match:

# kalshi/perps/models/markets.py  (current)
volume_notional_value: DollarDecimal = Field(...)
open_interest_notional_value: DollarDecimal = Field(...)

# safer, consistent with MarginMarket
volume_notional_value: DollarDecimal | None = Field(default=None, ...)
open_interest_notional_value: DollarDecimal | None = Field(default=None, ...)

Same concern applies to the three volume_notional_value / volume_24h_notional_value / open_interest_notional_value fields on MarginTickerPayload — they are added as required. If an older server or demo server omits them the payload will fail to parse entirely, breaking any live ticker subscription.

---

Security

KlearAuth.__init__ accepts None via (admin_user_id or "") but the type hint says str — the strip-then-store comment documents this, but (x or "") silently coerces None to "" before the emptiness check. Since the signature is def __init__(self, admin_user_id: str, ...), mypy won't complain, but a caller who passes None at runtime (e.g. from os.environ.get(...) without a default) would get a ValueError("requires a non-empty...") rather than a TypeError. This is fine as a defensive measure, but the comment is slightly confusing because it describes None-safety that the type contract says shouldn't be needed. Minor, but worth a note.

Authorization header could be overridden by a caller-supplied extra_headers — in _with_auth, extra_headers is merged first and then Authorization is set unconditionally, so a caller cannot accidentally suppress authentication. Good.

---

Code Quality / Conventions

_with_auth is duplicated verbatim between KlearSyncResource and KlearAsyncResource — the method has no async/sync differences. Both classes share _klear_auth: KlearAuth, so a mixin or a simple module-level helper would remove the duplication:

def _build_auth_headers(auth: KlearAuth, extra: dict[str, str] | None) -> dict[str, str]:
    merged = dict(extra) if extra else {}
    merged["Authorization"] = auth.authorization_header()
    return merged

Not blocking, but it's a three-line helper that's copy-pasted exactly.

subscribe_cfbenchmarks_value return type uses a union — AsyncIterator[CFBenchmarksValueMessage | CFBenchmarksIndexListMessage]. The channel spec is clear that both message types arrive on the same subscription, so this is correct. But callers will need isinstance checks; consider documenting the discrimination idiom in the docstring if it isn't already obvious from the type.

AccountResource.upgrade() discards the response body — the spec says 201 Created and the SDK correctly returns None. One nit: the comment says "Returns nothing; inspect the result via :meth:limits" — this is good user guidance, but the :meth:\limits` cross-reference will only render in Sphinx. The rendered docstring in most IDEs will show the raw backtick form. Consider `` :meth:~kalshi.resources.account.AccountResource.limits` `` or just plain prose for an IDE-friendly experience.

---

Test Coverage

MarginMarketCandlestick notional fields: no test for the missing-field parse error — since these are required (see the bug above), there's no test verifying that a payload missing volume_notional_value raises, and no test verifying the opposite (that it's optional). If the intent is "required", add a test; if the intent is "optional", make it optional and add a test for None.

KlearAuth.from_env with only one credential set is not tested — test_from_env_missing_credentials_raises tests with both absent; there's no case for KALSHI_KLEAR_ADMIN_USER_ID set but KALSHI_KLEAR_ACCESS_TOKEN absent (or vice versa). The if not resolved_admin or not resolved_token guard handles this correctly, but the test doesn't cover it.

subscribe_cfbenchmarks_value with ["all"] is not exercised — the spec specifically calls out ["all"] as a special value; a minimal test that the string is passed through unchanged would be worth adding.

---

Minor / Polish

• The integration test file tests/integration/test_perps_klear.py diff wasn't visible in this review — confirm it was updated to use admin_user_id/access_token rather than the old login() call.
• The pre-existing test_from_env_* pollution failures (4 tests, noted in the PR description) are tracked here. The hermetic monkeypatch.delenv("KALSHI_PRIVATE_KEY_PATH") fix in test_client.py / test_async_client.py is a good scoped fix — good hygiene.
• Subaccount range 1→63 is propagated consistently across models, docs, specs, comments, and FIX messages. Thorough.

---

Summary

Area	Status
Klear Bearer auth migration	✅ Clean. KlearAuth, _base.py, client/async_client all consistent.
cfbenchmarks_value WS channel	✅ Models, dispatch, channels, client, tests all wired.
AccountResource.upgrade()	✅ Correct empty-body workaround, good test coverage.
AccountApiLimits.grants + ApiUsageLevelGrant	✅ NullableList coercion tested for both null and non-null.
Perps api_limits() endpoint	✅ Model reuse is sensible, properly tested.
Notional fields on MarginMarketCandlestick / MarginTickerPayload	⚠️ Required fields on an existing model — recommend making them optional (`
Test coverage	✅ Good breadth. Minor gaps: single-missing-credential path in KlearAuth.from_env, candlestick required-vs-optional, ["all"] cfbenchmarks.

The breaking auth change is well-motivated (upstream removed the endpoint), the version bump to 4.0.0 is correct per semver, and the changelog entry is clear. The required-vs-optional concern on candlestick/ticker notional fields is the main thing I'd want resolved before merge.

🤖 Generated with Claude Code

[TexasCoding]

Addressed in 8bb1391.

⚠️ Notional fields required-vs-optional — split, on the merits.

• MarginTickerPayload → now optional (| None = None). You're right that on a live streaming feed a newly-added field omitted mid-rollout would break the whole ticker subscription — a worse failure than a None. The WS payload-drift guard checks name coverage (not required-ness) so this is test-legal, it matches the REST MarginMarket treatment of the same fields, and it fits the SDK's permissive-response-model convention. Added test_ticker_notional_values_optional (parses with all three absent → None).
• MarginMarketCandlestick → stays required. Two reasons it can't follow the ticker: (1) MarginMarketCandlestick.required lists both notional fields, and TestPerpsSpecDrift::test_response_drift hard-fails on "spec requires X but SDK field is optional" — relaxing it would red CI unless I add a PERPS_EXCLUSIONS entry of kind server_omits_despite_required citing a real demo+prod observation, which I can't fabricate; (2) it's a settled historical record, so the rollout-window concern doesn't apply the way it does to a live tick. Added test_notional_value_required_on_candlestick documenting the required intent, plus a code comment explaining the asymmetry vs the ticker.

Code quality — _with_auth duplication. Extracted a module-level _with_klear_auth(auth, extra_headers) helper; both bases delegate to it. (Used twice, so it's not a single-use abstraction.)

Security — KlearAuth None comment. Reworded to be explicit that (x or "") defensively coerces an off-type None (e.g. from os.environ.get) to "" so the empty check rejects empty / whitespace-only / None alike.

Union return idiom. subscribe_cfbenchmarks_value docstring now tells callers to discriminate with isinstance(msg, CFBenchmarksValueMessage) / msg.type before reading msg.msg.

Test gaps filled: ticker-absent (above), candlestick-missing-raises (above), from_env with exactly one credential missing, and subscribe_cfbenchmarks_value(index_ids=["all"]) passthrough.

Minor (no action):

• upgrade() :meth: cross-ref — kept; :meth:/:class: Sphinx refs are the established docstring convention across this codebase, so I left it consistent rather than special-casing one method.
• from_env sync/async duplication — left per the no-single-use-abstraction guidance, as noted.
• Integration test was updated: tests/integration/test_perps_klear.py now constructs KlearClient(admin_user_id=..., access_token=..., demo=True) and the real-api path uses from_env() + margin.settlement_balance() (the old login()/LogInResponse smoke test is gone).

Full suite: 4233 passed, 0 failed; ruff + mypy strict clean.

[claude]

Code Review — PR #444

TL;DR: Well-executed, high-confidence PR. The Klear Bearer migration is clean and complete; all the additive work (cfbenchmarks WS, account/perps additions) follows established conventions. A few minor items worth addressing before merge, but no blockers.

---

Overview

This PR reconciles the SDK against the latest upstream OpenAPI/AsyncAPI specs (as of 2026-06-09). It includes:

• Breaking: Klear (SCM) auth migrated from cookie-session (POST /log_in) to static Bearer token
• Additive: cfbenchmarks_value WebSocket channel, AccountResource.upgrade(), AccountApiLimits.grants, MarginAccountResource.api_limits(), perps notional/leverage fields

---

Code Quality

Klear Bearer auth migration

• The KlearAuth rewrite is clean. Whitespace-stripping of credentials before storage (preventing malformed Bearer id : tok headers) is a nice defensive touch. The (x or "").strip() idiom is slightly unusual but the inline comment explains it.
• _with_klear_auth correctly sets Authorization last so a caller-supplied extra_headers cannot suppress authentication — good security invariant.
• The _base.py override strategy (overriding _get/_post rather than hooking transport) is straightforward, but the docstring note that PUT/DELETE Klear endpoints would need manual additions is essential context — consider promoting it to a # NOTE: comment in the methods themselves, not just the module docstring.
• KlearAuth.__str__ = __repr__ is correct (prevents str() leaking the token through __repr__'s bypass).

AccountResource.upgrade()

• The json={} workaround for httpx's Content-Type omission on bodyless POSTs is well-commented and consistent with the subaccounts.create precedent.
• Minor: the method returns None but the spec shows 201 on success. The test correctly asserts account.upgrade() is None — good.

cfbenchmarks_value WebSocket channel

• CFBenchmarksAvgData.value typed as DollarDecimal is correct — these are USD-denominated reference rates, and the 8-dp string format matches the SDK's DollarDecimal convention.
• The data: str field (raw CF Benchmarks JSON frame left unparsed) is the right call — the field is upstream-defined and index-specific.
• seq: int | None = None on the message envelopes matches the optional treatment on other message types — consistent.

Perps notional fields on MarginTickerPayload

• Modeled as Optional despite the AsyncAPI listing them as required — the inline comment justifying this (live streaming channel, mid-rollout risk) is well-reasoned and follows the same pattern as MarginMarket.

ApiUsageLevelGrant model

• expires_ts: int | None = None with model_config = {"extra": "allow"} — the extra: "allow" is correct for a response-only model.
• All three required fields (exchange_instance, level, source) have no defaults, matching the spec's required block.

---

Potential Issues / Suggestions

1. grants field type in AccountApiLimits

grants: NullableList[ApiUsageLevelGrant]

This has no default, which means it's a required field in Pydantic — validation will fail for any server response that omits grants entirely (as opposed to returning null). The spec marks grants as required, but if there's any chance an older server version or a sub-endpoint omits it, this would raise ValidationError.

The async test correctly exercises the null → [] coercion path. If you're confident the field is always present (just sometimes null), this is fine as-is. If in doubt, grants: NullableList[ApiUsageLevelGrant] = [] would also handle the absent-key case.

2. from_env validation: empty-string env vars pass the not check

In KlearClient.from_env() / AsyncKlearClient.from_env():

if not resolved_admin or not resolved_token:
    raise ValueError(...)

If KALSHI_KLEAR_ADMIN_USER_ID="" is exported (empty string), not resolved_admin is True and ValueError is raised — correct. But the same whitespace issue that prompted the .strip() in KlearAuth.__init__ applies here: KALSHI_KLEAR_ADMIN_USER_ID=" " passes the not check (truthy whitespace string) and then hits KlearAuth.__init__ which strips it and raises ValueError. This is fine end-to-end (the error still surfaces), but the error message from from_env is better than the one from KlearAuth.__init__. Minor: you could .strip() before the check in from_env too.

3. KlearClient construction: empty-string args pass type checking silently

KlearClient(admin_user_id="", access_token="tok") will raise ValueError from KlearAuth.__init__ — good. But the error message ("KlearAuth requires a non-empty admin_user_id and access_token") doesn't mention the client class. Not blocking, but the from_env error messages do name the client class — consider aligning.

4. update_subscription action exhaustiveness — not validated client-side

async def update_subscription(self, client_id: int, action: str, ...) -> None:

action is untyped str. The spec for cfbenchmarks_value defines exactly ["subscribe_indices", "unsubscribe_indices", "indexlist"]. Passing an unsupported action (e.g. "add_markets" on a cfbenchmarks subscription) sends a command that will error server-side. This is consistent with the existing behavior for market channels, so no regression — but worth noting that the comment in the diff refers to "markets: add_markets/delete_markets; cfbenchmarks_value: subscribe_indices/unsubscribe_indices/indexlist" without enforcing it. A Literal type or an assert could catch this, but it's out of scope for this PR.

5. Pre-existing test_from_env_* failures (acknowledged)

The PR notes 4 pre-existing test_from_env_* failures caused by env-var pollution in the full run. The fixes in test_client.py / test_async_client.py (adding monkeypatch.delenv("KALSHI_PRIVATE_KEY_PATH", raising=False)) are the right surgical fix. These look correct — the comment references #445 which should be filed if not already.

---

Test Coverage

• TestKlearAuth — covers authorization_header() format, empty/whitespace rejection, strip behavior, and repr redaction. Complete.
• TestBearerHeaderInjection — verifies the header is present on direct GETs, paginated GETs, and that 401 maps to KalshiAuthError. The async path is covered too.
• TestFromEnv — covers missing both, missing one, and env-provided credentials. Good.
• TestCFBenchmarksModels — covers both message types, the optional quarter-hour field, and the missing-required-field failure case.
• TestSubscribeCFBenchmarks / TestSubscriptionManager channel tests — cover subscribe with/without index_ids, ["all"], and the three update_subscription actions.
• TestAccountUpgrade — covers 201 success, 403 mapped to KalshiAuthError, and auth guard. Good.

Gap: No test for MarginAccountResource.api_limits(). The analogous perps fee_tiers/balance tests exist in tests/perps/test_margin_account.py; this endpoint should follow the same pattern (happy path + auth guard + error path).

---

Security

• Bearer token is never logged, never in exception messages, and repr is redacted. Consistent and correct.
• The KlearConfig HTTPS enforcement message was correctly updated from "session cookie" to "Bearer access token". The security invariant (plaintext → credential exposure warning/error) is preserved.
• No secrets in test constants (TEST_ADMIN_USER_ID = "test-admin" etc.) — fine, they're clearly synthetic.

---

Minor

• specs/perps_openapi.yaml has cosmetic whitespace-only changes (multi-line → single-line descriptions). These are valid re-vendoring artifacts but add noise to the diff without changing behavior.
• The 1-32 → 1-63 subaccount range update is applied consistently across all relevant comments, docstrings, spec descriptions, and FIX message files — thorough.
• CHANGELOG.md entry is clear, versioned correctly, and properly calls out the breaking change.

---

Summary

Category	Assessment
Correctness	✅ No bugs found
Conventions	✅ Follows project patterns throughout
Security	✅ Credential redaction is complete and consistent
Test coverage	⚠️ Missing api_limits() test in test_margin_account.py
Breaking change handling	✅ Semver major bump, CHANGELOG entry, docs updated

One actionable item before merge: add a test for MarginAccountResource.api_limits() (happy path + auth guard). Everything else is either correct or a minor note.

🤖 Generated with Claude Code

[TexasCoding]

Thanks — this one needs no code change; the one "actionable" item is already covered (looks like a partial-diff view, same as a couple earlier passes).

api_limits() test — already present. tests/perps/test_margin_account.py::TestApiLimits (line ~444) covers exactly the requested pattern, mirroring fee_tiers/balance:

• test_happy — 200 with a populated grants entry (asserts usage_tier, read.bucket_capacity, grants[0].exchange_instance, expires_ts is None)
• test_null_grants_coerced — grants: null → []
• test_unauthenticated_raises_before_http — AuthRequiredError, asserts the route wasn't called
• test_async_happy — async path

uv run pytest tests/perps/test_margin_account.py::TestApiLimits → 4 passed.

Notes (no action, with reasons):

1. grants no default. Required is intentional and enforced: grants is in GetAccountApiLimitsResponse.required, and TestSpecDrift::test_required_drift[AccountApiLimits] hard-fails if a spec-required field is optional — so = [] would red CI (same constraint as the candlestick). NullableList still handles null → [], which is tested.
2. from_env whitespace env var. A whitespace value isn't absent — it's malformed, and malformed-credential validation lives in KlearAuth.__init__ (the single chokepoint, which strips + raises a clear ValueError). from_env's guard is just a presence check, so I kept the separation rather than duplicating the strip there.
3. KlearAuth error not naming the client. Intentional — KlearAuth is constructed directly too, so its message is scoped to itself rather than assuming a KlearClient caller.
4. update_subscription action not a Literal. Agreed it's out of scope and consistent with the existing market-channel behavior (untyped action); no regression.
5. #445. Filed and closed by this PR (the delenv fix).
6. specs/perps_openapi.yaml cosmetic whitespace. Re-vendoring artifact — the specs are written verbatim by scripts/sync_spec.py from upstream, not hand-edited, so the multi-line→single-line description reflow comes straight from Kalshi.

Full suite: 4233 passed, 0 failed; ruff + mypy strict clean.