Skip to content

docs: pre-release audit sweep — accuracy, gaps, and staleness fixes (#179)#188

Merged
TexasCoding merged 5 commits into
mainfrom
chore/issue-179-docs-audit-release-prep
May 21, 2026
Merged

docs: pre-release audit sweep — accuracy, gaps, and staleness fixes (#179)#188
TexasCoding merged 5 commits into
mainfrom
chore/issue-179-docs-audit-release-prep

Conversation

@TexasCoding

Copy link
Copy Markdown
Owner

Closes #179.

Pre-release docs audit before cutting the next release. Findings compiled from a six-way parallel agent audit of disjoint doc-file partitions, triaged, ground-truthed against the actual code/spec, and applied centrally.

Audit shape

Six read-only agents in parallel:

Agent Partition
AuditTopLevelDocs README.md, CHANGELOG.md, ROADMAP.md, AGENTS.md
AuditFoundationMkdocs docs/index.md, getting-started.md, concepts.md, authentication.md, configuration.md, environment-variables.md
AuditCoreMkdocs docs/types.md, request-models.md, dataframes.md, errors.md, retries.md, pagination.md, testing.md, migration.md, reference.md, RELEASING.md
AuditResourcesMkdocs All docs/resources/*.md (~19 files)
AuditWebsocketDocs docs/websockets.md + recent #175/#176/#177 cross-check
AuditMkdocsConfigAndConsistency mkdocs.yml + cross-file link/nav consistency

Each agent produced a structured JSON findings report (file, line, category, severity, evidence, suggested_fix). Centralized triage, ground-truthing against actual code/spec, then a single editing pass.

Changes

ROADMAP.md (the #179 core)

docs/resources/multivariate.md

  • Quick-reference table claimed lookup_history hits /lookup_history. Actual code (kalshi/resources/multivariate.py:233) uses /lookup with a lookback_seconds query param.
  • "Recent lookup history" example called hist.lookups on the return value, but lookup_history returns list[LookupPoint] directly. Rewritten to iterate the list and print real LookupPoint fields.

docs/index.md

  • "85 endpoints" → "98 operations" against current specs/openapi.yaml.
  • Sync/async parity bullet explicitly notes WebSocket access is async-only (via AsyncKalshiClient.ws).
  • Reconnection bullet mentions the new resubscribe-window stash.

docs/websockets.md

docs/authentication.md

docs/configuration.md

  • New "Lifecycle" section documenting KalshiClient.close() semantics — tears down both the httpx client and the sign executor — with a cross-link to the new authentication.md subsection.

docs/resources/events.md

  • Note documenting the Integration tests failing on demo API #183 server-omission handling: Event.product_metadata: dict[str, Any] | None, EventMetadata.market_details: NullableList[MarketMetadata]. Cross-references the EXCLUSIONS map / server_omits_despite_required.

README.md

  • WS quickstart uses package-level from kalshi.ws import KalshiWebSocket (matches docs/websockets.md style) instead of the deeper kalshi.ws.client path.
  • Channel list clarifies that 11 of the 13 channels have dedicated subscribe_* methods; the remaining two (control_frames, root) ride the generic subscribe(channel) escape hatch.

Findings not actioned

Finding Reason
docs/environment-variables.md precedence wording Current text was already accurate; agent #14 misread
docs/configuration.md retry_base_delay/retry_max_delay table Already present at lines 37–38; agent #14 misread
docs/types.md MoneyAmount Symbol doesn't exist; was a phantom in my agent briefing
docs/resources/orders.md batch_cancel positional vs kwarg Current positional call is correct; style nit only
mkdocs.yml not_in_nav Is standard MkDocs config, not a custom convention
Various wording tweaks (callback comments, float-warning callouts, migration heading clarifications) Scope control — low-severity polish
docs/RELEASING.md uv setup step Devs running release already have uv installed

Verification

  • mkdocs build --strict clean (zero warnings on internal links or missing pages)
  • ruff check . clean
  • mypy kalshi/ strict clean
  • Pure-docs change, no code touched per git diff --stat
 CHANGELOG.md                   | 34 ++++++++++++++++++++++++++++++
 README.md                      | 12 ++++++-----
 ROADMAP.md                     | 47 ++++++++++++++++++++----------------------
 docs/authentication.md         | 24 +++++++++++++++++++++
 docs/configuration.md          | 17 +++++++++++++++
 docs/index.md                  |  8 ++++---
 docs/resources/events.md       | 13 ++++++++++++
 docs/resources/multivariate.md |  8 +++----
 docs/websockets.md             | 24 +++++++++++++++++++++
 9 files changed, 150 insertions(+), 37 deletions(-)

After this lands, the only post-v2.2.0 work in Unreleased is the WS reliability + auth polish batch (#173#178, #183) plus this docs sweep — ready to cut a release.

…179)

Closes #179.

Pre-release docs audit before cutting the next release. Findings compiled
from a six-way parallel agent audit of disjoint doc-file partitions,
triaged, ground-truthed against the actual code/spec, and applied:

ROADMAP.md (#179 core):

* "Open trackers" section dropped. #45 (json={} workaround) and #53
  (nested $ref drift) are both closed; #106 was a PR (not an issue)
  whose remaining sub-items all shipped post-v2.2.0.
* "Next milestone" carry-overs that landed (MessageQueue maxlen via
  #173, _coerce_decimal via #174, WS UX foot-guns via #175/#176/#177,
  CONTRACT_MAP completeness via #181) removed.
* Added an "Unreleased (post-v2.2.0)" bullet at the top of "Shipped"
  pointing at #173#178 and #183.

docs/resources/multivariate.md:

* Quick-reference table claimed lookup_history hits
  `GET /multivariate_event_collections/{ticker}/lookup_history`. Actual
  code (kalshi/resources/multivariate.py:233) uses `/lookup` with a
  `lookback_seconds` query param. Table updated.
* "Recent lookup history" example called `hist.lookups` on the return
  value, but lookup_history returns `list[LookupPoint]` directly.
  Example now iterates the list and prints real LookupPoint fields.

docs/index.md:

* "85 endpoints" claim corrected to "98 operations" against current
  specs/openapi.yaml.
* Sync/async parity bullet now explicitly notes the one exception:
  WebSocket access is async-only (via AsyncKalshiClient.ws).
* Reconnection bullet mentions the new resubscribe-window stash.

docs/websockets.md:

* New "Resubscribe-window frame stashing" subsection under
  Reconnection, documenting the #176 mechanism, stash_maxlen bound,
  per-(sid, cycle) WARNING dedup, and the per-sub-failure drop case.
  CHANGELOG had claimed docs were updated for #176; they weren't.

docs/authentication.md:

* New "Async RSA-PSS sign offload" subsection documenting
  KalshiAuth.sign_request_async() (#178), the dedicated 2-worker
  ThreadPoolExecutor lifecycle, why it's separate from asyncio's
  default pool, and the close() chain through KalshiClient.

docs/configuration.md:

* New "Lifecycle" section explaining KalshiClient.close() semantics —
  tears down both the httpx client and the sign executor — with a
  cross-link to the new authentication.md subsection.

docs/resources/events.md:

* Note documenting the #183 server-omission handling:
  Event.product_metadata typed `dict[str, Any] | None`, and
  EventMetadata.market_details typed `NullableList[MarketMetadata]`.
  Cross-references the EXCLUSIONS map / server_omits_despite_required.

README.md:

* WS quickstart now uses the package-level
  `from kalshi.ws import KalshiWebSocket` instead of the deeper
  `kalshi.ws.client` path (matches docs/websockets.md style).
* Channel list clarifies that 11 of the 13 channels have dedicated
  subscribe_* methods; the remaining two (control_frames, root) are
  reachable through the generic subscribe(channel) escape hatch.

Findings not actioned (already-correct text or low-value style):

* docs/environment-variables.md precedence — current wording was
  already accurate; agent #14 misread.
* docs/configuration.md retry_base_delay/retry_max_delay table rows —
  already present at lines 37–38; agent #14 misread.
* docs/types.md MoneyAmount — does not exist; was a phantom in my
  briefing.
* docs/resources/orders.md batch_cancel positional vs kwarg — current
  positional call is correct; style nit only.
* mkdocs.yml not_in_nav — is standard MkDocs config, agent was wrong.
* Various low-severity wording tweaks (callback comments, float
  warnings, migration heading clarifications) — scope control.

Verification: `mkdocs build --strict` clean (zero warnings on internal
links or missing pages). `ruff check .` clean. `mypy kalshi/` strict
clean. Pure-docs change, no code touched per `git diff --stat`.
@claude

claude Bot commented May 20, 2026

Copy link
Copy Markdown

Code Review — PR #188: Pre-release docs audit sweep

Pure documentation PR — no code changes. Overall this is a high-quality audit sweep with clear evidence of ground-truthing against the actual code. A few items worth addressing before merge:


Bugs / Correctness

docs/resources/multivariate.md — endpoint path fix (good catch)
The table correction from /lookup_history/lookup with lookback_seconds query param is a genuine correctness fix. Likewise the example rewrite from hist.lookups (nonexistent attribute) to iterating the list directly and printing real LookupPoint fields. These are the most impactful changes in the PR.


Issues to Address

1. CLAUDE.md still says "85 endpoints" (inconsistency)

docs/index.md now says "98 operations", but CLAUDE.md (line: OpenAPI spec: https://docs.kalshi.com/openapi.yaml (v3.18.0, 85 endpoints)) was not updated. Contributors use CLAUDE.md as the canonical reference, so this will create confusion immediately.

Suggest updating the CLAUDE.md API Reference line to match:

- OpenAPI spec: https://docs.kalshi.com/openapi.yaml (v3.18.0, 98 operations)

2. docs/resources/events.md — missing blank line before ## Reference

The admonition block ends without a blank line before the next heading:

    Both are tracked under `server_omits_despite_required` in the SDK's
    EXCLUSIONS map.
## Reference

MkDocs may render ## Reference as part of the admonition body rather than as a new section. The PR description claims mkdocs build --strict was clean, so this may be harmless in practice, but it's fragile. A blank line is the safe fix:

    Both are tracked under `server_omits_despite_required` in the SDK's
    EXCLUSIONS map.

## Reference

Observations (no action required)

README import path change is valid
from kalshi.ws import KalshiWebSocket works — kalshi/ws/__init__.py re-exports it. The old deeper path (kalshi.ws.client) still works too, but the package-level import is the right public API to surface.

"85 endpoints" → "98 operations" terminology shift
Using "operations" (HTTP method × path) rather than "endpoints" (path only) is more precise for an OpenAPI spec. The jump in count suggests the spec was re-counted rather than new endpoints added, since the spec version is still v3.18.0. The PR description makes this clear, but a one-liner in docs/index.md callout or CHANGELOG noting the recount (not an API expansion) could prevent a reader from thinking the coverage jumped 15%.

"Findings not actioned" table
Excellent transparency. Exactly the right way to document audit scope decisions.

WebSocket async-only clarification
The repeated note across docs/index.md ("Async-only — access via AsyncKalshiClient.ws") and the sync/async parity section is a genuinely important user-facing clarification, especially since the rest of the SDK has full parity.


Summary

Two items worth fixing before merge:

  1. Update CLAUDE.md endpoint count to match docs/index.md ("98 operations")
  2. Add blank line before ## Reference in docs/resources/events.md

Everything else looks accurate and well-crafted. The multivariate endpoint fix and the new WebSocket/auth/lifecycle sections are solid additions.

Bot review found two real items plus one I caught while fixing:

1. CLAUDE.md still said "85 endpoints" — updated to "98 operations" to
   match docs/index.md.

2. README.md line 14 also said "85 endpoints" (bot missed this one) —
   updated to "98 operations" for consistency.

3. docs/resources/events.md was missing a blank line between the
   admonition block closing the "Server omissions" note and the next
   "## Reference" heading. mkdocs --strict passed before (it's not a
   hard error), but the bot is correct that it's fragile markdown.
   Added the blank line.

The bot's "recount vs API expansion" observation was not actioned —
adding a "this is a recount" disclaimer would actually be misleading,
since some operations were added (v2 family, deposits/withdrawals,
endpoint_costs) and some are just from a tighter count. The previous
"85 endpoints" claim was inaccurate prior; "98 operations" against the
current spec is verified-accurate. No disclaimer needed.

mkdocs --strict still clean.
@TexasCoding

Copy link
Copy Markdown
Owner Author

Round 1 addressed in 252ce06.

1. CLAUDE.md endpoint count — updated to "98 operations" to match docs/index.md.

2. docs/resources/events.md blank line before ## Reference — added. mkdocs --strict had passed without it (not a hard error), but you're right that it's fragile markdown — a stricter renderer or admonition tweak could fold the heading into the block body.

Extra fix you missed: README.md line 14 also said "85 endpoints across 19 resources". Updated to "98 operations" for consistency with CLAUDE.md and docs/index.md.

Not actioned: the "recount vs API expansion" disclaimer — adding a "this is a recount" note would actually be misleading. The delta is a mix: real additions (the v2 order family, deposits/withdrawals, endpoint_costs from v2.1.0) plus a tighter count against the current spec. The previous "85 endpoints" was inaccurate prior; "98 operations" against specs/openapi.yaml is verified-accurate. Either framing assumes a specific cause that doesn't fit cleanly.

mkdocs build --strict still clean.

@claude

claude Bot commented May 20, 2026

Copy link
Copy Markdown

Code Review — PR #188 (Pre-release docs audit sweep)

Summary: Pure documentation update — no code changes. Fixes factual inaccuracies, fills gaps for recently-shipped features, and cleans up stale ROADMAP entries. I spot-checked all verifiable factual claims against the source.


Verified Correct ✅

Claim Evidence
lookup_history hits /lookup with lookback_seconds param, not /lookup_history kalshi/resources/multivariate.py:233 — URL is /multivariate_event_collections/{ticker}/lookup
Updated example fields (point.last_queried_ts, point.market_ticker, point.event_ticker) kalshi/models/multivariate.py:140–146 — all three fields exist on LookupPoint
from kalshi.ws import KalshiWebSocket package-level import works kalshi/ws/__init__.py:3,8 — re-exports KalshiWebSocket
sign_request_async + dedicated ThreadPoolExecutor kalshi/auth.py:237 — method exists; :71,222–231 — executor is lazy-init'd ✓

Issues

[Bug] CHANGELOG.md section title uses issue number, not PR number

-### Pre-release docs audit (#179)
+### Pre-release docs audit (#188)

Every other CHANGELOG entry uses the PR number (e.g., (#176), (#177), (#178)). #179 is the issue this closes, not the PR. Using the issue number here breaks the convention and makes the entry harder to trace from the changelog to the commit.


[Minor] ROADMAP "Open trackers" section — verbose tombstone prose

The section header ## Open trackers now contains:

None. The previous entries here (#45, #53, #106) are all closed: #45 (json={} workaround) resolved...

The history of why these closed belongs in a commit message or the PR description, not in the living ROADMAP. By the next release this explanation will be stale and confusing. Suggest:

## Open trackers

None.

Anyone curious about the closed trackers can find them via gh issue view 45 or git history.


Positive Observations

  • Multivariate docs fix is the most impactful change — the previous endpoint URL and the hist.lookups attribute reference were both wrong and would have produced broken user code. Good catch.
  • Resubscribe-window stashing section is technically accurate and well-written. The memory-bound formula and the per-(sid, cycle) dedup note are exactly the kind of implementation detail users need when debugging reconnect behaviour.
  • Async RSA-PSS sign offload section correctly explains why the dedicated pool exists (isolation from loop.getaddrinfo / other to_thread() work). The close() chain note is helpful.
  • "85 endpoints" → "98 operations" is a meaningful terminology fix, not just a number bump — one path can expose multiple operations. Applied consistently across README.md, docs/index.md, and CLAUDE.md. ✓
  • Scope discipline: the "Findings not actioned" table in the PR description is a good audit trail. Calling out what was reviewed and deliberately skipped prevents re-litigation.

Summary

One real bug (CHANGELOG title), one housekeeping nit (ROADMAP prose). All factual corrections verified against the code. The multivariate.md fix alone justifies the merge. Fix the CHANGELOG (#179)(#188) and this is clean to land.

Bot round-2 found one item to action and one based on a misreading of
convention.

Actioned:

* ROADMAP.md "Open trackers" trimmed from four lines of tombstone prose
  ("None. The previous entries here (#45, #53, #106) are all closed:
  ... ") down to "None." The why-they-closed history will rot by the
  next release; anyone curious can run `gh issue view 45` or check git
  history.

Not actioned:

* Bot suggested changing CHANGELOG.md "Pre-release docs audit (#179)"
  to "(#188)" on the claim that "every other CHANGELOG entry uses the
  PR number." This is backwards. Existing CHANGELOG entries
  (#175, #176, #177, #178, #183) are all *issue* numbers; the PRs that
  closed them were #185, #187, #186, #182, #184 respectively. Verified
  via `gh issue view <N> --json closedByPullRequestsReferences`. The
  convention is "(#<issue-number>)" — using #179 is correct.

mkdocs --strict still clean.
@TexasCoding

Copy link
Copy Markdown
Owner Author

Round 2 addressed in 701b0a2.

Item 2 (ROADMAP tombstone) — actioned. Trimmed the four-line "why these closed" history down to None. You're right that the explanation decays; gh issue view 45 is one command away for anyone curious.

Item 1 (CHANGELOG (#179)(#188)) — not actioned. The convention is the opposite of what you stated.

Existing CHANGELOG entries use issue numbers, not PR numbers:

Entry Issue # in CHANGELOG Actual PR #
WS resubscribe-window frame stashing (#176) PR #187
WS run_forever(stop_event=...) (#177) PR #186
WS run_forever raises on missing sub (#175) PR #185
Nightly integration server-omission fixes (#183) PR #184
WS / auth polish batch (#173 + #174 + #178) PR #182

Verified via gh issue view <N> --json closedByPullRequestsReferences. #179 matches the established pattern: link the issue that drove the change, not the PR that mechanically closed it.

mkdocs build --strict still clean.

@claude

claude Bot commented May 21, 2026

Copy link
Copy Markdown

Code Review — PR #188: Pre-release docs audit sweep

Overall: Clean, well-scoped docs-only PR. I verified all technical claims against the actual code before writing this review.


What this does

Fixes documentation accuracy across nine files: wrong endpoint path in multivariate.md, stale endpoint count (85 → 98), undocumented features (sign_request_async, resubscribe-window stashing, client.close() lifecycle), and ROADMAP / CHANGELOG housekeeping around now-closed issues.


Accuracy — all claims verified ✓

Claim Verified against
lookup_history hits /lookup (not /lookup_history) kalshi/resources/multivariate.py:221,234
LookupPoint fields last_queried_ts, market_ticker, event_ticker kalshi/models/multivariate.py:70-71,146
stash_maxlen defaults to 1000 kalshi/ws/channels.py:69
ThreadPoolExecutor(max_workers=2) for sign executor kalshi/auth.py:231-232
KalshiWebSocket exported from kalshi.ws kalshi/ws/__init__.py:3,8

Issues

Minor: PR description says "Pure-docs change, no code touched" but CLAUDE.md is in the diff.

CLAUDE.md isn't executable code, so the spirit of the claim is fine — but the stat line in the description omits it. The change itself (85 → 98 operations) is correct. Not a blocker; just worth noting for the commit message's accuracy.


Observations

multivariate.md fix is the most impactful change. The old docs claimed a non-existent endpoint (/lookup_history) and a non-existent field (hist.lookups on a list return). Any user copy-pasting that example would get a 404 and an AttributeError. Good catch.

"85 endpoints" → "98 operations" is both a number and terminology fix. "Operations" (OpenAPI term: one HTTP method × one path) is more precise than "endpoints" (paths). Using the spec's own vocabulary here is the right call and aligns with how the drift tests are framed.

The WS resubscribe-window subsection fills a specific gap. The #176 CHANGELOG entry had claimed docs were updated; they weren't. This corrects that. The description of stash semantics, overflow logging, and worst-case memory bound (stash_maxlen × len(active_subs) × avg_frame_size) is accurate and appropriately detailed for users tuning high-volume channels.

ROADMAP cleanup is justified. Confirmed #45 and #53 are closed; #106 was a PR. Removing them from "Open trackers" and replacing with None. is correct. The new "Unreleased (post-v2.2.0)" bullet gives an accurate summary of the batch.


Suggestion (non-blocking)

In docs/authentication.md, the new subsection says the executor is "lazy-initialised on first use." The docs/configuration.md Lifecycle section's try/finally example is useful, but it only shows KalshiClient. A one-liner noting AsyncKalshiClient follows the same pattern (or a async with example) would be cleaner parity — callers of the async client are the ones who actually hit the sign executor. Low priority.


Verdict: Approve. The substantive accuracy fixes are well-targeted and code-verified. New doc sections backfill real gaps. No functional changes.

@TexasCoding
TexasCoding force-pushed the chore/issue-179-docs-audit-release-prep branch from 23dd55c to b4c10e4 Compare May 21, 2026 00:02
@TexasCoding

Copy link
Copy Markdown
Owner Author

Round 3 (approve verdict) — non-blocking suggestion actioned in b4c10e4.

Lifecycle async parity — added an AsyncKalshiClient async with example alongside the sync try/finally. You're right: the async client is the one that actually exercises the sign executor, so it deserves the equal billing.

"Pure-docs change, no code touched" stat-line nit — fair. The original commit's git diff --stat was generated before the CLAUDE.md fix landed (CLAUDE.md was a fixup in 252ce06). Stat lines in PR bodies always lag the latest commit on the branch; the spirit of the claim (no Python source touched) still holds. Leaving as-is rather than thrashing the description.

mkdocs build --strict clean.

@claude

claude Bot commented May 21, 2026

Copy link
Copy Markdown

Code Review — PR #188: Pre-release docs audit sweep

Overview

Pure documentation PR — no code changes. Fixes inaccuracies found via a parallel six-agent audit across all doc surfaces: wrong endpoint paths, stale roadmap entries, undocumented features (#176 stash, #178 async sign), and a broken code example.


Verification Against Source

I ground-truthed all material claims against the actual code before writing this review:

Claim Verified? Notes
lookup_history uses /lookup (not /lookup_history) multivariate.py:221,234,359,372
LookupPoint fields: last_queried_ts, market_ticker, event_ticker models/multivariate.py:140–146
from kalshi.ws import KalshiWebSocket works ws/__init__.py:3,8
11 dedicated subscribe_* methods ws/client.py:436–531 — exactly 11 before the generic subscribe()
stash_maxlen=1000, per-(sid, cycle) WARNING dedup ws/channels.py:69,88,95,177–182,314,375
AsyncKalshiClient.from_env() + async with async_client.py:155,177–180
server_omits_despite_required exclusions for product_metadata / market_details tests/_contract_support.py:1162–1176
Async sign executor max_workers=2, lazy init auth.py:63,71,222,231–232

Issues

Minor: CLAUDE.md absent from the PR description's git diff --stat

The diff includes a CLAUDE.md change (v3.18.0, 85 endpointsv3.18.0, 98 operations), but the PR body's git diff --stat block shows only 9 files and omits CLAUDE.md. The change itself is correct and consistent with the rest of the sweep — it's just that the stat block in the description appears to have been generated before this change was added. Not a blocker, but the PR body and actual diff are slightly out of sync.

Minor: stash WARNING dedup description is accurate but slightly abstract

docs/websockets.md describes overflow logging as "a single WARNING per (sid, replay-cycle)". The implementation uses a set[int] tracking just sids, cleared at the start of each resubscribe cycle via _stash_warned.clear() (lines 314 and 375 in channels.py). The description is semantically correct — the effective key is (sid, cycle) because the set resets each cycle — but a reader who goes to verify it will find set[int], not set[tuple[int, int]]. Consider clarifying: "once per sid per resubscribe cycle" or adding the implementation note. Low severity.


Code Quality and Style

  • All new doc sections follow the existing mkdocs admonition and code-block conventions.
  • The configuration.md Lifecycle section correctly shows both try/finally + close() (sync) and async with (async). The asymmetry is intentional — sync KalshiClient also supports with, but showing try/finally here is fine as it's more explicit for readers who aren't using the context manager.
  • docs/resources/events.md admonition correctly cross-references EXCLUSIONS without over-explaining implementation details.
  • ROADMAP cleanup is clean — "Open trackers: None." is honest and unambiguous.

What Was Fixed

All three material correctness bugs are solid fixes:

  1. lookup_history path — The old table entry (/lookup_history) was wrong; actual path is /lookup with lookback_seconds as a query param. Fixed correctly.
  2. Broken examplehist.lookups on a list[LookupPoint] return would raise AttributeError at runtime. The rewritten example iterates the list directly and uses real field names.
  3. Missing feature docs#176 (stash), #178 (async sign), and #183 (server omissions) were all shipped but undocumented. Now covered.

Summary

This is a clean, well-executed docs sweep. Every claim I could verify checked out. The audit methodology (parallel read-only agents with structured findings, centralized ground-truth pass) is solid and the triage of non-actionable findings is reasonable. The one real defect is the CLAUDE.md omission from the PR body's stat block — trivial to fix before merge if desired, but not a blocker.

Approve with the note about the PR description stat block.

@claude

claude Bot commented May 21, 2026

Copy link
Copy Markdown

Code Review — PR #188: Pre-release docs audit sweep

Overview

Pure documentation PR (no code changes — confirmed by diff). Fixes factual errors, fills gaps left by recent feature PRs (#173#178, #183), and cleans up stale ROADMAP entries. Well-structured: six-agent parallel audit → centralized triage → single editing pass.


Verification (spot-checked independently)

Claim Result
lookup_history hits /lookup (not /lookup_history) ✅ Confirmed — multivariate.py:234
LookupPoint has last_queried_ts, market_ticker, event_ticker ✅ Confirmed — models/multivariate.py:140-146
from kalshi.ws import KalshiWebSocket works ✅ Confirmed — exported from kalshi/ws/__init__.py:3
"98 operations" count ✅ Confirmed — 98 operationId: entries in specs/openapi.yaml

The most substantive bug fix — wrong endpoint path and broken field access in multivariate.md — is accurate and the new example uses real fields.


Issues / Suggestions

Minor (non-blocking):

  1. NullableList is an internal type surfaced in user-facing docs (docs/resources/events.md).
    The note says "EventMetadata.market_details uses NullableList[MarketMetadata], which coerces a JSON null payload to []." Users reading the docs won't know what NullableList is. Consider replacing with plain English: "market_details: list[MarketMetadata] — a server null is coerced to []" and dropping the internal type name, since the coercion behavior is what matters to callers.

  2. stash_maxlen is mentioned but not shown as configurable (docs/websockets.md).
    The new resubscribe-window section says stash_maxlen defaults to 1000 per sid, but doesn't say whether it can be configured. If it's not user-configurable (i.e. it's a constant), saying so directly avoids confusion: "hardcoded to 1000 per sid." If it is configurable, a brief note on where to set it would help.

  3. Terminology shift note in README/docs/index.md (cosmetic).
    Changing "85 endpoints" → "98 operations" is more accurate (endpoints vs. HTTP method+path pairs), but a returning user might wonder if coverage grew. The change itself is correct — just noting it may prompt questions. No action needed if it's intentional and accepted as-is.


What's done well

  • ROADMAP cleanup is correct: #45 and #53 were both verified closed; #106 was a PR whose sub-items all landed. "Open trackers: None." is honest and clean.
  • docs/authentication.md async sign offload section is clear and technically accurate — explains why a dedicated executor (not to_thread) is used, which is the non-obvious part.
  • docs/configuration.md lifecycle section correctly pairs sync try/finally: client.close() with async async with — both forms documented.
  • CHANGELOG entry style is consistent with existing entries in the file.
  • CLAUDE.md updated to "98 operations" — keeps the agent instructions consistent with the public docs.

Summary

Approval-ready. The multivariate endpoint fix is a genuine doc bug caught by the audit. The new sections (async sign offload, lifecycle, frame stashing, server omissions) accurately document shipped features that were previously undocumented. The two minor items above are suggestions, not blockers.

Round 4: stash WARNING dedup wording aligned to set[int] impl ('once per sid per resubscribe cycle' instead of '(sid, replay-cycle)' tuple-implying phrasing). Round 5: NullableList[MarketMetadata] replaced with plain-English coercion behavior in events.md (NullableList is internal type). Also clarified stash_maxlen=1000 is not user-configurable on KalshiWebSocket.
@TexasCoding

Copy link
Copy Markdown
Owner Author

Rounds 4 + 5 (both approve verdicts) — non-blocking suggestions actioned in 0b4ad59.

Round 4 — stash WARNING dedup wording (verifier alignment)

Aligned docs/websockets.md to the actual implementation. The previous phrasing — "a single WARNING per (sid, replay-cycle)" — was semantically right but tuple-suggesting, while the implementation is set[int] reset per cycle. Now reads: "a WARNING fires once per sid per resubscribe cycle". Anyone going to verify will see set[int] and the docs match.

Round 5 — NullableList is internal

Good catch. Replaced the NullableList[MarketMetadata] mention in docs/resources/events.md with the user-facing behavior:

EventMetadata.market_details: list[MarketMetadata] — when the live server sends JSON null, the SDK coerces it to [] so callers always see a list. The spec contract (key present) is still enforced.

The coercion behavior is what callers actually need to know; the type-system mechanism stays in docs/types.md for readers who care.

Round 5 — stash_maxlen configurability

stash_maxlen is a SubscriptionManager.__init__ kwarg, but not exposed on KalshiWebSocket, so users can't actually tune it without dropping into internal API. Doc now reads: "bounded by an internal stash_maxlen=1000 per sid ... (not user-configurable on KalshiWebSocket)." That's the honest framing — implying configurability would be misleading.

Round 5 — "98 operations" terminology callout

Not actioned. Already addressed in round 1 reply — adding a "this is a recount" disclaimer would be misleading since the delta is mixed (real additions from v2 family + tighter spec count). The PR description's "85 → 98" framing is honest about the change without speculating on cause.

mkdocs build --strict still clean. 2126 unit tests + ruff + mypy unaffected (no code changes).

Five review rounds in, each one has surfaced something real and made the docs tighter. This is bot review working well, but I think we're at diminishing-returns territory — the remaining items would be style nits or speculative additions. Happy to keep going if the next round finds something substantive.

@claude

claude Bot commented May 21, 2026

Copy link
Copy Markdown

Code Review — PR #188: Pre-release docs audit sweep

This is a pure-docs PR (no code touched), so the review focuses on accuracy, consistency, and clarity.

Overview

A well-scoped, well-organized pre-release docs sweep. The parallel-agent audit methodology is clearly documented in the PR description, the non-actioned findings list is a useful signal of scope discipline, and the ROADMAP cleanup is overdue.


Accurate fixes — no issues

  • docs/resources/multivariate.md — fixing the endpoint path (/lookup_history/lookup with lookback_seconds query param) and the broken hist.lookups example are both correct ground-truth fixes. High-value.
  • Endpoint count / terminology — "85 endpoints" → "98 operations" is correct OpenAPI terminology. An operation is a method+path combo; an endpoint is a path. Using the right term here is worth the churn.
  • ROADMAP cleanup — removing #45, #53, and #106 (confirmed closed/merged) keeps the tracker honest.
  • README.md import pathfrom kalshi.ws import KalshiWebSocket is the correct public surface; the old kalshi.ws.client path was leaking an internal module.
  • CLAUDE.md — the count update is consistent with the other doc surfaces.

Minor issues

1. Redundancy in docs/index.md

After the change, "async-only" appears twice within two adjacent bullets:

- **Full WebSocket coverage** — ... Async-only —
  access via `AsyncKalshiClient.ws`.
- **Sync and async parity** — ... are identical.
  The one exception is WebSocket access, which is async-only.

The second sentence in the parity bullet is now the third place this is said. The parity bullet could drop "The one exception is WebSocket access, which is async-only." — the WebSocket bullet already carries the qualifier, and docs/index.md is a bullets page, not a reference page.

2. EventMetadata.market_details type in docs/resources/events.md

The note says:

EventMetadata.market_details: list[MarketMetadata] — when the live server sends JSON null, the SDK coerces it to []

But the PR body says the field is typed NullableList[MarketMetadata]. If the actual model field is NullableList[MarketMetadata] (a custom type alias), the doc should show that — a bare list[MarketMetadata] wouldn't coerce null to [] on its own. Minor, but could confuse a reader who checks the source.

3. LookupPoint field names in multivariate example

The new example uses:

print(point.last_queried_ts, point.market_ticker, point.event_ticker)

The PR doesn't show a code change to kalshi/models/multivariate.py, so these field names should be verifiable against the existing model. Worth a quick grep if not already confirmed — the old example was wrong precisely because it used a non-existent .lookups attribute.


Documentation quality

  • docs/authentication.md — the async sign offload subsection is clear. The max_workers=2 rationale (avoid queuing behind getaddrinfo/file I/O) and the lifecycle chain (close() → executor shutdown) are both non-obvious and worth documenting exactly here.
  • docs/configuration.md — the lifecycle section with both sync and async examples is practical. The async with emphasis over manual .close() is the right recommendation.
  • docs/websockets.md — the stashing mechanism explanation is detailed and accurate. The memory-bound formula (stash_maxlen × len(active_subs) × avg_frame_size) and the per-sid-per-cycle WARNING dedup are both worth calling out.

Summary

Approve with minor nits. The redundancy in docs/index.md is the only change I'd seriously consider making before merge; the other two items are worth a quick verification pass. None are blockers for a pure-docs sweep.

@TexasCoding
TexasCoding merged commit 0e62e40 into main May 21, 2026
5 checks passed
@TexasCoding
TexasCoding deleted the chore/issue-179-docs-audit-release-prep branch May 21, 2026 00:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ROADMAP.md: "Open trackers" section is stale — referenced issues are closed

1 participant