docs: pre-release audit sweep — accuracy, gaps, and staleness fixes (#179)#188
Conversation
…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`.
Code Review — PR #188: Pre-release docs audit sweepPure 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
Issues to Address1.
Suggest updating the CLAUDE.md API Reference line to match: 2. The admonition block ends without a blank line before the next heading: MkDocs may render Observations (no action required)README import path change is valid "85 endpoints" → "98 operations" terminology shift "Findings not actioned" table WebSocket async-only clarification SummaryTwo items worth fixing before merge:
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.
|
Round 1 addressed in 252ce06. 1. 2. Extra fix you missed: 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
|
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 ✅
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., [Minor] ROADMAP "Open trackers" section — verbose tombstone prose The section header
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 Positive Observations
SummaryOne real bug (CHANGELOG title), one housekeeping nit (ROADMAP prose). All factual corrections verified against the code. The |
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.
|
Round 2 addressed in 701b0a2. Item 2 (ROADMAP tombstone) — actioned. Trimmed the four-line "why these closed" history down to Item 1 (CHANGELOG Existing CHANGELOG entries use issue numbers, not PR numbers:
Verified via
|
Code Review — PR #188: Pre-release docs audit sweepOverall: Clean, well-scoped docs-only PR. I verified all technical claims against the actual code before writing this review. What this doesFixes documentation accuracy across nine files: wrong endpoint path in Accuracy — all claims verified ✓
IssuesMinor: PR description says "Pure-docs change, no code touched" but
Observations
"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 ROADMAP cleanup is justified. Confirmed Suggestion (non-blocking)In Verdict: Approve. The substantive accuracy fixes are well-targeted and code-verified. New doc sections backfill real gaps. No functional changes. |
23dd55c to
b4c10e4
Compare
|
Round 3 (approve verdict) — non-blocking suggestion actioned in b4c10e4. Lifecycle async parity — added an "Pure-docs change, no code touched" stat-line nit — fair. The original commit's
|
Code Review — PR #188: Pre-release docs audit sweepOverviewPure 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 SourceI ground-truthed all material claims against the actual code before writing this review:
IssuesMinor: CLAUDE.md absent from the PR description's The diff includes a Minor: stash WARNING dedup description is accurate but slightly abstract
Code Quality and Style
What Was FixedAll three material correctness bugs are solid fixes:
SummaryThis 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 Approve with the note about the PR description stat block. |
Code Review — PR #188: Pre-release docs audit sweepOverviewPure 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)
The most substantive bug fix — wrong endpoint path and broken field access in Issues / SuggestionsMinor (non-blocking):
What's done well
SummaryApproval-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.
|
Rounds 4 + 5 (both approve verdicts) — non-blocking suggestions actioned in 0b4ad59. Round 4 — stash WARNING dedup wording (verifier alignment) Aligned Round 5 — Good catch. Replaced the
The coercion behavior is what callers actually need to know; the type-system mechanism stays in Round 5 —
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.
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. |
Code Review — PR #188: Pre-release docs audit sweepThis is a pure-docs PR (no code touched), so the review focuses on accuracy, consistency, and clarity. OverviewA 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
Minor issues1. Redundancy in After the change, "async-only" appears twice within two adjacent bullets: 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 2. The note says:
But the PR body says the field is typed 3. The new example uses: print(point.last_queried_ts, point.market_ticker, point.event_ticker)The PR doesn't show a code change to Documentation quality
SummaryApprove with minor nits. The redundancy in |
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:
AuditTopLevelDocsREADME.md,CHANGELOG.md,ROADMAP.md,AGENTS.mdAuditFoundationMkdocsdocs/index.md,getting-started.md,concepts.md,authentication.md,configuration.md,environment-variables.mdAuditCoreMkdocsdocs/types.md,request-models.md,dataframes.md,errors.md,retries.md,pagination.md,testing.md,migration.md,reference.md,RELEASING.mdAuditResourcesMkdocsdocs/resources/*.md(~19 files)AuditWebsocketDocsdocs/websockets.md+ recent #175/#176/#177 cross-checkAuditMkdocsConfigAndConsistencymkdocs.yml+ cross-file link/nav consistencyEach 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)#45and#53are both closed (verified viagh issue view);#106was a PR (not an issue) whose remaining sub-items all shipped post-v2.2.0.MessageQueuemaxlenvia WSMessageQueue: adddeque(maxlen=maxsize+1)as defense-in-depth ceiling #173,_coerce_decimalvia Consolidate_to_decimal_dollarsand_to_decimal_fp— byte-identical helpers #174, WS UX foot-guns via WS foot-gun: callback-only client silently no-ops when nosubscribe_*is called #175/WS: data frames arriving during reconnect+resubscribe are dropped instead of stashed #176/WS:run_forever()lacks graceful shutdown / signal-driven stop path #177, CONTRACT_MAP completeness via test(contracts): map remaining 42 REST sub-models + promote completeness gate (#171) #181) removed.MessageQueue: adddeque(maxlen=maxsize+1)as defense-in-depth ceiling #173–Offload RSA-PSS signing to a thread executor — currently inline on the event loop #178 + Integration tests failing on demo API #183.docs/resources/multivariate.mdlookup_historyhits/lookup_history. Actual code (kalshi/resources/multivariate.py:233) uses/lookupwith alookback_secondsquery param.hist.lookupson the return value, butlookup_historyreturnslist[LookupPoint]directly. Rewritten to iterate the list and print realLookupPointfields.docs/index.mdspecs/openapi.yaml.AsyncKalshiClient.ws).docs/websockets.mdstash_maxlenbound, per-(sid, cycle)WARNING dedup, and the per-sub-failure drop case. The CHANGELOG WS: data frames arriving during reconnect+resubscribe are dropped instead of stashed #176 entry had claimed docs were updated; they weren't.docs/authentication.mdKalshiAuth.sign_request_async()(Offload RSA-PSS signing to a thread executor — currently inline on the event loop #178), the dedicated 2-workerThreadPoolExecutorlifecycle, why it's separate from asyncio's default pool, and theclose()chain throughKalshiClient.docs/configuration.mdKalshiClient.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.mdEvent.product_metadata: dict[str, Any] | None,EventMetadata.market_details: NullableList[MarketMetadata]. Cross-references theEXCLUSIONSmap /server_omits_despite_required.README.mdfrom kalshi.ws import KalshiWebSocket(matchesdocs/websockets.mdstyle) instead of the deeperkalshi.ws.clientpath.subscribe_*methods; the remaining two (control_frames,root) ride the genericsubscribe(channel)escape hatch.Findings not actioned
docs/environment-variables.mdprecedence wordingdocs/configuration.mdretry_base_delay/retry_max_delaytabledocs/types.mdMoneyAmountdocs/resources/orders.mdbatch_cancelpositional vs kwargmkdocs.ymlnot_in_navdocs/RELEASING.mduvsetup stepuvinstalledVerification
mkdocs build --strictclean (zero warnings on internal links or missing pages)ruff check .cleanmypy kalshi/strict cleangit diff --statAfter this lands, the only post-v2.2.0 work in
Unreleasedis the WS reliability + auth polish batch (#173–#178, #183) plus this docs sweep — ready to cut a release.