[TexasCoding]

Wave 2-B of audit umbrella. 6 P1 polish items + passphrase support.

Changes

• Polish bundle: HTTP transport DX (http2 extra, per-request headers, Retry-After date form, trailing-slash signing, RecordingTransport headers, extra_headers docs) #220 P1 HTTP transport DX bundle: http2 install extra + h2 import check, per-request extra_headers plumbing, Retry-After HTTP-date parsing, RecordingTransport response-header scrubbing (Set-Cookie / Authorization / X-Kalshi-*-id|key|account|user), trailing-slash signing canonicalization at the transport boundary, docs/configuration.md extra_headers override claim corrected.
• Auth: support passphrase-protected PEMs (currently forces plaintext key on disk) #217 KalshiAuth.from_pem / from_key_path / from_env / try_from_env accept optional password (str/bytes/callable). KALSHI_PRIVATE_KEY_PASSPHRASE env var. Users no longer need to write plaintext keys to disk.

Closes #217, Closes #220
Refs #224

[claude]

Code Review — PR #233

Wave 2-B is solid overall: the changes are surgical, well-tested, and address real gaps. A few issues worth addressing before merge, ranging from a meaningful semantic bug to minor style nits.

---

Overview

Six targeted improvements bundled together:

1. HTTP/2 fail-fast — h2 availability checked at KalshiConfig construction instead of silently deferring to the first request.
2. Per-request extra_headers — layered on top of config-level headers; auth headers always win.
3. Retry-After HTTP-date parsing — RFC 7231 HTTP-date format now parsed via email.utils.parsedate_to_datetime, not silently ignored.
4. Response-header scrubbing — RecordingTransport drops Set-Cookie, Authorization, and X-Kalshi-* identity headers from fixtures by default.
5. Trailing-slash canonicalization — strip before both signing and the httpx call.
6. Passphrase support — KalshiAuth loaders accept password= (str/bytes/callable) and KALSHI_PRIVATE_KEY_PASSPHRASE.

---

Issues

Medium — documentation contradicts implementation

kalshi/testing/__init__.py (line 469) and _recorder.py docstring:

pass response_header_filter= to extend the deny-list

The implementation does the opposite — a user-supplied filter (callable or iterable) fully replaces the default. The test even documents this explicitly:

# … and the default Set-Cookie scrubbing did NOT apply (user took over).
assert headers.get("set-cookie") == "should-be-kept-when-filter-is-overridden"

This is fine as a design choice, but the word "extend" is wrong and will mislead anyone who wants to add a header to the deny-list while keeping the defaults. Either:

• Change "extend" → "replace" / "override" in the warning, or
• Add a helper for composition: _default_response_header_drop(name, value) or my_predicate(name, value)

The current UX footgun: a user passing response_header_filter=["x-my-internal-id"] silently stops scrubbing Set-Cookie and Authorization.

Medium — async transport has no test coverage for the new features

AsyncTransport.request received identical extra_headers and trailing-slash changes but all tests (TestExtraHeadersPerRequest, TestTrailingSlashCanonicalization) only exercise SyncTransport. Same for AsyncRecordingTransport — only the sync RecordingTransport is exercised in TestResponseHeaderFilter.

Minimum additions needed:

• One async extra_headers test mirroring test_extra_headers_per_request_overrides_config
• One async extra_headers cannot-forge-auth-headers test
• AsyncRecordingTransport default-filter smoke test

---

Minor / Style

Internal ticket references in source code (_base_client.py lines 182–226, _fixtures.py line 489):

# P1.6: canonicalize trailing slash BEFORE both signing and httpx call
# P1.5 default response-header scrubbing:

Per the project's own CLAUDE.md: "Don't reference the current task, fix, or callers… those belong in the PR description and rot as the codebase evolves." P1.1/P1.5/P1.6 mean nothing to a future reader. The explanatory text after the label is valuable — keep that, drop the label.

test_from_key_path_with_passphrase_succeeds (tests/test_auth.py):

Uses tempfile.NamedTemporaryFile(delete=False) + manual os.unlink inside a try/finally. The tmp_path pytest fixture is already available in this test class (it's in the function signature elsewhere). Prefer:

def test_from_key_path_with_passphrase_succeeds(
    self, rsa_private_key: rsa.RSAPrivateKey, tmp_path: Path
) -> None:
    encrypted = self._encrypted_pem(rsa_private_key, b"pw")
    key_file = tmp_path / "key.pem"
    key_file.write_bytes(encrypted)
    auth = KalshiAuth.from_key_path("k", key_file, password="pw")
    assert auth.key_id == "k"

Local imports in test_trailing_slash_path_canonicalized (tests/test_auth.py):

import httpx, import respx, and from kalshi... are placed inside the test function body. They're already available at module level. Move to module scope.

Duplicated comment in async transport (_base_client.py ~line 225):

The sync version has the full three-line comment; the async version has only the single-line form. Either use the same comment or omit both per CLAUDE.md's "no obvious comments" rule.

---

Correctness / Security ✓

• Auth header precedence: {**config_extra, **per_call_extra, **auth_headers} — auth always wins. Critical property, correctly placed.
• parsedate_to_datetime timezone guard (_base_client.py line 159–160): parsedate_to_datetime raises TypeError for tz-naive dates rather than returning tzinfo=None, so the if dt.tzinfo is None guard is technically dead code. Harmless, but slightly misleading.
• Callable passphrase invocation: invoked exactly once, inside from_pem. No double-invocation risk through the from_key_path → from_pem chain. Good.
• h2 check via find_spec: correct — avoids importing the module just to check for it, no startup cost on the http2=False path.
• _DEFAULT_SCRUB_RE regex (?i)^x-kalshi-.*-(id|key|account|user).*$: the trailing .*$ means X-Kalshi-Trace-Account-Hash matches (-account- mid-name). That's the intended behavior based on the tests, but worth confirming this is the desired scope.

---

Test Coverage — Overall

Comprehensive. The trailing-slash test re-derives the signed message and cryptographically verifies the signature against the canonicalized path — that's the right level of rigor for a signing invariant. The HTTP-date Retry-After tests cover future/past/garbage cases cleanly.

The two gaps (async transport tests, "extend vs replace" documentation) are the main things to address before this is merge-ready.

[claude]

Code Review — Wave 2-B Transport/Auth Polish + Passphrase Support

Overall this is a solid, well-structured PR. The implementation is correct, the security model is sound, and the test suite is thorough. A few issues worth addressing before merge.

---

Overview

Six focused improvements: passphrase-protected PEM support, HTTP-date Retry-After parsing, per-request extra_headers, trailing-slash signing canonicalization, h2 fail-fast import check, and RecordingTransport response-header scrubbing.

---

Issues

1. ResponseHeaderFilter string footgun — medium

_coerce_header_filter accepts Iterable[str] but str itself satisfies that type. A caller who passes response_header_filter="authorization" instead of ["authorization"] will silently get a deny-list of single characters ({'a','u','t','h',...}), dropping headers like "a" and keeping "authorization" unchanged. The silence makes this very hard to debug.

kalshi/testing/_fixtures.py:

# Before the callable check, add:
if isinstance(response_header_filter, str):
    raise TypeError(
        "response_header_filter must be a callable or an iterable of header names, "
        f"not a bare string; did you mean [{response_header_filter!r}]?"
    )

2. Vacuous test assertion — minor

test_default_filter_drops_set_cookie asserts:

assert headers.get("x-kalshi-request-id") is None

but X-Kalshi-Request-Id was never added to the stub response's headers in that test, so this is vacuously true. It doesn't exercise the regex path. Either add the header to the stub, or remove the assertion.

3. Task-tracking labels in source code — minor (CLAUDE.md violation)

Several comments embed ticket/task identifiers directly in source:

• _base_client.py: # P1.6: canonicalize trailing slash BEFORE... (appears twice, sync + async)
• _fixtures.py: # P1.5 default response-header scrubbing:...

CLAUDE.md is explicit: "Don't reference the current task, fix, or callers … since those belong in the PR description and rot as the codebase evolves." These should be rewritten as # why comments if the behavior is non-obvious, or removed.

4. Duplicate comment in test — nit

tests/test_mock_transport.py around line 1231 has the same comment on two consecutive lines:

        # Predicate was called for every response header.
        # Predicate was called for every response header (httpx lowercases).

One is clearly a forgotten edit.

5. tempfile + manual os.unlink — nit

test_from_key_path_with_passphrase_succeeds uses NamedTemporaryFile(delete=False) + os.unlink. The tmp_path fixture is already available in the test class context (pytest provides it per-test); using it avoids the manual teardown:

def test_from_key_path_with_passphrase_succeeds(
    self, rsa_private_key: rsa.RSAPrivateKey, tmp_path: Path
) -> None:
    key_file = tmp_path / "encrypted.pem"
    key_file.write_bytes(self._encrypted_pem(rsa_private_key, b"pw"))
    auth = KalshiAuth.from_key_path("k", key_file, password="pw")
    assert auth.key_id == "k"

---

What's working well

• Passphrase API design: The str | bytes | Callable | None type is the right shape. Callable defers the secret fetch until load time, which is the safe pattern for vault integration. The explicit kwarg correctly wins over the env var.
• RFC 7231 HTTP-date parsing: email.utils.parsedate_to_datetime + tzinfo guard + max(0.0, delta) clamp for past dates is correct. The retry_max_delay cap flowing through the same path as numeric Retry-After is good design.
• Header merge order: {**config_extra, **per_call_extra, **auth_headers} — config < per-call < signed auth — is the right security ordering and the test that tries to forge KALSHI-ACCESS-* headers is exactly the right regression guard.
• Trailing-slash test: Verifying the cryptographic signature agrees with the wire path (not just asserting the URL) is a high-quality test.
• h2 fail-fast: importlib.util.find_spec in __post_init__ is the right approach — no side effects, no ImportError on non-h2 systems, immediate feedback at construction rather than at first request.
• RecordingTransport scrubbing: The three-mode API (None → default, callable → predicate, Iterable[str] → deny-list) is clean. The default regex covers the realistic credential leak surface.

---

Required before merge: Issue #1 (string footgun) and Issue #3 (task labels in source).
Issues #2, #4, #5 are polish and can be addressed now or in a follow-up.

[claude]

Code Review — PR #233

Wave 2-B audit polish. Overall this is solid work with good test coverage. I found one substantive bug, one style issue that will likely fail ruff, and a handful of minor observations.

---

Overview

Six P1 polish items plus passphrase support:

• http2=True fail-fast check at KalshiConfig construction
• Per-request extra_headers plumbing in both transports
• RFC 7231 HTTP-date parsing for Retry-After
• RecordingTransport response-header scrubbing
• Trailing-slash signing canonicalization
• KalshiAuth passphrase support (str/bytes/callable/env var)

All six features have corresponding tests and updated docs.

---

Substantive Issue

_coerce_header_filter silent footgun — _fixtures.py:519

ResponseHeaderFilter = Callable[[str, str], bool] | Iterable[str] | None

A plain str is a valid Iterable[str], so response_header_filter="Set-Cookie" would pass the callable() guard, fall through to the iterable branch, and build a deny-list of individual characters ({'S', 'e', 't', '-', …}). Every real header would pass through silently while the user expects Set-Cookie to be scrubbed.

Consider adding a guard at the top of _coerce_header_filter:

if isinstance(response_header_filter, str):
    raise TypeError(
        "response_header_filter must be a callable or an iterable of header names, "
        "not a plain str — did you mean [response_header_filter]?"
    )

mypy may not catch this at call-site because str satisfies Iterable[str].

---

Style Issue (will likely fail ruff)

Missing blank lines before TestHttp2ImportCheck — test_config.py

        assert cfg.ws_json_dumps is json.dumps
class TestHttp2ImportCheck:   # ← no blank lines

PEP 8 / ruff E302 requires two blank lines before a top-level class definition.

---

Minor Observations

bool() around a regex match — _fixtures.py:505

return bool(_DEFAULT_SCRUB_RE.match(name))

re.Match is truthy; bool() is redundant. return _DEFAULT_SCRUB_RE.match(name) is not None reads more clearly, though this won't affect behavior.

---

Unreachable or "/" branch — _base_client.py:185–186 / 349–350

if path != "/" and path.endswith("/"):
    path = path.rstrip("/") or "/"

The or "/" fallback can never be reached: the guard already excludes "/", so path.rstrip("/") on any other trailing-slash path will always yield a non-empty string. Harmless, but a reader might wonder why it's there. Could be dropped.

---

Duplicated test comment — test_mock_transport.py:1230–1231

# Predicate was called for every response header.
# Predicate was called for every response header (httpx lowercases).

The same comment appears twice consecutively. Looks like a copy-paste artifact.

---

What's Well Done

• Auth header priority order — {**config_extra, **per_call_extra, **auth_headers} is exactly right and documented clearly. The "cannot forge auth headers" test is a valuable security regression guard.
• Past-date Retry-After clamped to 0 — correct; the server gave a date in the past and the client should retry immediately rather than raising.
• Callable passphrase invoked exactly once — the test explicitly asserts calls["n"] == 1, which closes the loop on a subtle contract.
• Trailing-slash test verifies the signature — the test reconstructs the signing payload and calls public_key().verify(), so it's actually checking that sign-path and wire-path agree, not just that the URL was stripped.
• HTTP/2 fail-fast at construction — much better DX than an ImportError buried inside the first request.

---

Verdict

One bug fix needed (_coerce_header_filter str guard), one ruff style fix (test_config.py blank lines). Everything else is clean. Happy to approve once those two are addressed.