diff --git a/architecture/errors.md b/architecture/errors.md index 56bcac4..73b325d 100644 --- a/architecture/errors.md +++ b/architecture/errors.md @@ -14,7 +14,7 @@ The error-mapping table (what `httpx2` exception maps to which `httpware` except `TimeoutError` inherits from both `httpware.ClientError` and `builtins.TimeoutError` so `except builtins.TimeoutError` (the form `asyncio.wait_for` uses) also catches httpware-raised timeouts. -`DecodeError` covers the case where `response_model=` is set, the HTTP call itself succeeded, but the active `ResponseDecoder` raised. The wrap happens at the seam in `Client.send` / `AsyncClient.send` — `except Exception` translates any decoder-side failure into `DecodeError(response=..., model=..., original=...)` with `raise ... from exc` chaining. The `original` attribute exposes the underlying library exception (e.g., `pydantic.ValidationError`, `msgspec.ValidationError`); `__cause__` carries the same reference. +`DecodeError` covers the case where `response_model=` is set, the HTTP call itself succeeded, but the active `ResponseDecoder` raised. The wrap happens in `_BoundDecoder.decode` (`decoders/_resolver.py`) — `except Exception` translates any decoder-side failure into `DecodeError(response=..., model=..., original=...)` with `raise ... from exc` chaining. The `original` attribute exposes the underlying library exception (e.g., `pydantic.ValidationError`, `msgspec.ValidationError`); `__cause__` carries the same reference. The "no `__init__` override" rule scopes only to `StatusError` subclasses. Non-status `ClientError` subclasses — `DecodeError`, `MissingDecoderError`, `BulkheadFullError`, `RetryBudgetExhaustedError`, `CircuitOpenError`, `ResponseTooLargeError` — deliberately define `__init__` with keyword-only fields. diff --git a/docs/testing.md b/docs/testing.md index 79bf40d..e922238 100644 --- a/docs/testing.md +++ b/docs/testing.md @@ -107,7 +107,7 @@ For middleware with state-keeping (counters, circuit-breaker state), assert on i ## Why not `respx`? -`httpware` deliberately uses `httpx2.MockTransport` instead of `respx` for its own tests. `MockTransport` is the public test seam in `httpx` — supported by the maintainers, stable across versions, lives in the public API surface. `respx` patches private internals and has historically broken across `httpx` major versions. Stick with `MockTransport` unless you have a specific reason not to. +`httpware` deliberately uses `httpx2.MockTransport` instead of `respx` for its own tests. `MockTransport` is the public test seam in `httpx2` — supported by the maintainers, stable across versions, lives in the public API surface. `respx` patches private internals and has historically broken across `httpx2` major versions. Stick with `MockTransport` unless you have a specific reason not to. ## See also diff --git a/planning/audits/2026-07-13-docs-comments-audit.md b/planning/audits/2026-07-13-docs-comments-audit.md new file mode 100644 index 0000000..cf53f0e --- /dev/null +++ b/planning/audits/2026-07-13-docs-comments-audit.md @@ -0,0 +1,112 @@ +# httpware docs & comments audit — 2026-07-13 + +**Status:** complete +**Scope:** source-level docstrings/comments (`src/httpware/**/*.py`, 23 files) and +the living documentation surface (`README.md`, `docs/**`, `architecture/**`). +`planning/` was excluded — it is a kept historical record, not living truth, +per the planning convention. This is the first audit to cover source comments +and `architecture/` directly; the 2026-06-13 docs audit covered only the +user-facing `docs/`/README surface and used `architecture/` purely as a +reference. +**Method:** two parallel first-pass agents (one over `src/`, on a cheap model; +one over docs/architecture, on the standard model), each returning a compact +candidate list. Every candidate was then independently re-verified against the +real source before being written up here — several early candidates turned out +correct on inspection and are recorded under Verified correct rather than as +findings. + +## Summary + +- Stale/inconsistent comments or docstrings: **2** (both in `src/`) +- Cross-doc contradictions: **4** (all in `docs/`/`architecture/`) +- Duplication / compaction candidates: **3** + +**Verdict on your compaction/dedup question:** worth doing, but narrowly — the +`src/` comment surface is already lean (the first-pass sweep found zero +comments that just restate obvious code; every comment/docstring earns its +keep). The dedup opportunity is entirely on the docs side, and it's +concentrated: `ResponseTooLargeError`'s behavior is spelled out near-verbatim +in **three** places (D1) and the "why not respx" paragraph in two (D2, tangled +up with the `httpx`/`httpx2` slip in I4) — both good candidates for "full +account in one place, cross-reference from the rest," the same pattern +`architecture/` already uses elsewhere. The `CircuitBreaker` overlap (D3) is a +lower-priority, mostly-intentional tutorial-vs-reference depth split. + +## Findings + +### Stale/inconsistent (source) + +**C1 — `errors.py:184` — `BulkheadFullError` docstring says "AsyncBulkhead" but the error is shared by both bulkheads.** +Docstring: `"Raised when acquire_timeout elapses before an AsyncBulkhead slot becomes available."` +Both `AsyncBulkhead.acquire` (`bulkhead.py:133`) and sync `Bulkhead.acquire` +(`bulkhead.py:175`) raise it via the shared `_emit_bulkhead_rejected` helper +(`bulkhead.py:50-68`) — it is not async-specific. +*Fix:* drop "Async" — `"Raised when acquire_timeout elapses before a bulkhead slot becomes available."` +**Resolved** (`2026-07-13.07`). + +**C2 — `client.py:1037` vs `client.py:2009` — `stream()` docstrings disagree on whether the middleware-bypass is version-scoped.** +`AsyncClient.stream()`: `"Bypasses the middleware chain (...) for v1 — see architecture/client.md for the contract."` +`Client.stream()`: `"Bypasses the middleware chain (...) — matches AsyncClient.stream() behavior."` (no "for v1"). +`architecture/client.md:23` states the bypass as a permanent design choice ("Both bypass the middleware chain by design"), not a v1-only caveat — so the async docstring's "for v1" doesn't match the truth home either, and the two client docstrings say different things about the same shared behavior. +*Fix:* drop "for v1" from the async docstring (or, if the bypass genuinely is meant to be revisited post-v1, say so in `architecture/client.md` too and mirror the caveat into the sync docstring). +**Resolved** (`2026-07-13.07`) — "for v1" dropped; both client docstrings now agree with `architecture/client.md`. + +### Cross-doc contradictions + +**I1 — `architecture/errors.md:17` misstates where `DecodeError` wrapping happens.** +`errors.md`: `"The wrap happens at the seam in Client.send / AsyncClient.send — except Exception translates any decoder-side failure into DecodeError(...)."` +Verified against source: the `try/except Exception: raise DecodeError(...)` lives in `_BoundDecoder.decode` (`decoders/_resolver.py:38-43`), called from `client.py` as `bound.decode(response)` — not inline in `send`. `architecture/decoders.md:12` already states this correctly ("Any exception is wrapped by `_BoundDecoder.decode`"). The two truth-home files contradict each other on the same fact. +*Fix:* correct `errors.md:17` to point at `_BoundDecoder.decode` (`decoders/_resolver.py`), matching `decoders.md`. +**Resolved** (`2026-07-13.07`). + +**I2 — `docs/dev/contributing.md:28` vs `architecture/conventions.md:24-25` — contradictory docstring requirement.** +`contributing.md`: `"Module docstrings are required; per-method docstrings only when types alone are insufficient."` (conditional) +`conventions.md`: `"Module / class / public-method docstrings are required ..."` (unconditional) +*Fix:* pick one policy and make both files say it — recommend keeping `conventions.md`'s unconditional wording since it's the capability truth home, and updating `contributing.md` to match. + +**I3 — `docs/dev/contributing.md:32-34` understates what CI machine-checks, vs `architecture/overview.md:9`.** +`contributing.md`: `"The CI lint pass (...) catches what the linters can see (e.g. print() via ruff T201); the rest are enforced in code review."` — reads as "only `print()` is machine-checked." +`overview.md`: documents two more checks contributing.md omits — `PGH003` (blanket `# type: ignore`) is machine-checked, and `SLF001` partially checks the `httpx2._` ban (attribute access, not import). +This is a fresh drift, not a re-flag of the 2026-06-13 audit's I1 (that finding was about a since-removed "CI grep gates" phrase, already fixed) — `overview.md`'s finer breakdown was apparently added after `contributing.md`'s wording was last touched. +*Fix:* replace `contributing.md`'s "the rest are enforced in code review" with the same three-tier breakdown `overview.md` uses (machine-checked / partially-checked / review-only), or have it link to `overview.md` instead of restating. + +**I4 — `docs/testing.md:110` says `httpx` where it means `httpx2`.** +`"MockTransport is the public test seam in httpx — supported by the maintainers, stable across versions ... respx patches private internals and has historically broken across httpx major versions."` +Every other reference in the same file (line 3) and in `architecture/testing.md:4` correctly says `httpx2` — a real, separate PyPI package (`httpx2>=2.0.0,<3.0`, maintained by Pydantic Services Inc.), not an alias for the original `httpx`. This line reads as a leftover phrase from before the `httpx2` rename, and as written it inaccurately implies `respx`'s breakage history is about `httpx2` specifically. +*Fix:* change both `httpx` occurrences on that line to `httpx2` (verify against `respx`'s actual `httpx2` support status if this section is touched — it may be that `respx` doesn't support `httpx2` at all, which is a stronger reason to use `MockTransport` than "it breaks across versions"). +**Resolved** (`2026-07-13.07`) — both occurrences corrected to `httpx2`; the underlying "does respx support httpx2 at all" question is left as-is, not part of this mechanical fix. + +### Duplication / compaction candidates + +**D1 — `ResponseTooLargeError` behavior is spelled out near-verbatim in three files.** +`docs/errors.md:193-204`, `architecture/errors.md:23`, and `architecture/client.md:36` all restate the same handful of facts (status-agnostic, counts decoded bytes, fires from the non-streaming terminal and `stream()`'s error pre-read but not user-driven iteration, the `"declared"`/`"streamed"` reason split, "neither StatusError, NetworkError, nor TimeoutError — not retried, doesn't count toward the circuit breaker") in matching or near-matching phrasing. `docs/errors.md` has the fullest account. +*Suggest:* keep the full account in `docs/errors.md` (or `architecture/errors.md`, whichever is meant to be canonical for this fact), compress the other two to a one-line cross-reference. + +**D2 — "why not respx" is duplicated between `docs/testing.md:108-110` and `architecture/testing.md:4`.** +Same argument, ~3 near-identical sentences in each. Bundle this cleanup with the I4 fix (same lines) — reconcile the `httpx`/`httpx2` wording and de-duplicate the reasoning in the same edit, keeping the fuller version in one file. + +**D3 — `CircuitBreaker` states/failure-classification/rate-mode overlap between `docs/resilience.md:158-212` and `architecture/resilience.md:17,21`.** Lower priority: this is mostly a legitimate tutorial-vs-compressed-reference depth split, not verbatim duplication, but several exact clauses ("4xx including 429 count as successes," the `window_seconds=30.0`/`minimum_calls=20` defaults) are copied rather than merely covering the same ground. Worth a light pass if `resilience.md` is next revised for another reason — not worth a dedicated change on its own. + +## Verified correct (negative results) + +- **`src/` comment/docstring redundancy:** zero comments found that merely restate what well-named code already makes obvious — every inline comment explains a non-obvious constraint or invariant (e.g. wire-body header stripping, coverage pragmas, semaphore behavior). +- **README ↔ `docs/index.md` duplication (regression check):** the ~70% prose duplication fixed in change `2026-06-14.01` has **not** crept back. The only verbatim overlap remaining is short, non-prose boilerplate (the Pre-1.0 status line, the "Part of `modern-python`" footer) — not the capability-description duplication the prior fix targeted. The install-extras blocks differ in *coverage* (README documents the `[all]` extra, `docs/index.md` doesn't) rather than in duplicated content. +- Terminology elsewhere (`AsyncMiddleware`/`Middleware`, `AsyncNext`/`Next`, "terminal", "Seam A/B/C", phase-decorator names) is used consistently across every file checked — I4 is an isolated slip, not a pattern. + +## Spawned changes + +- **`2026-07-13.07-docs-comments-audit-fixes`** (lightweight) — fixes C1, C2, + I1, I4. Verified against source; `just lint-ci`, `mkdocs build --strict`, + and `just test` (780 passed, 100% coverage) all clean. + +## Deferred / next steps + +- **Needs your call:** I3's fix direction (restate the three-tier breakdown in + `contributing.md`, or replace it with a link to `overview.md`) and I2's + policy choice (unconditional vs conditional method-docstring requirement) — + both are wording contradictions where either side could be "the fix," + not a clear code-vs-doc mismatch. Not yet scheduled. +- **Compaction:** D1 (`ResponseTooLargeError` triplication) and D2 (bundled + with I4's `httpx`/`httpx2` fix — the "why not respx" duplication itself + wasn't touched by `2026-07-13.07`, only the terminology slip within it) are + still open. D3 is a defer-until-touched item, not worth its own change. diff --git a/planning/changes/2026-07-13.07-docs-comments-audit-fixes.md b/planning/changes/2026-07-13.07-docs-comments-audit-fixes.md new file mode 100644 index 0000000..78bcd6b --- /dev/null +++ b/planning/changes/2026-07-13.07-docs-comments-audit-fixes.md @@ -0,0 +1,70 @@ +--- +summary: Fixed 4 verified stale/contradictory comments and docs from the docs-and-comments audit — BulkheadFullError docstring, stream() version caveat, DecodeError wrap-site claim, testing.md httpx/httpx2 slip. +--- + +# Change: Fix mechanical findings from the docs-and-comments audit + +**Lane:** lightweight — touches 4 files (above the usual ≤2 guard, but per the +`2026-06-13.04` precedent, the file-count guard proxies *code* risk; these are +mechanical corrections whose design thinking already lives in the audit). No +new file, no public-API change. Spec is the audit, not a `design.md`. + +Spec: [`planning/audits/2026-07-13-docs-comments-audit.md`](../../audits/2026-07-13-docs-comments-audit.md) +(findings C1, C2, I1, I4). + +## Goal + +Correct the four verified stale/contradictory comments and docs so source +docstrings match the code they annotate and the `architecture/` truth home is +internally consistent. Excludes I2 and I3 (wording contradictions needing a +maintainer policy call, not a code-vs-doc mismatch) and D1/D2 (compaction — +separate scope decision). + +## Approach + +Each edit is pinned to a finding verified against source in the audit: + +- **C1** `src/httpware/errors.py` — `BulkheadFullError` docstring says + "AsyncBulkhead slot"; the error is raised by both `Bulkhead.acquire` and + `AsyncBulkhead.acquire` via the shared `_emit_bulkhead_rejected` helper. + Drop "Async" from the docstring. +- **C2** `src/httpware/client.py` — `AsyncClient.stream()`'s docstring says + the middleware bypass is "for v1"; `Client.stream()`'s docstring has no such + caveat, and `architecture/client.md:23` states the bypass as permanent + ("by design"). Drop "for v1" from the async docstring so both client + docstrings agree with the truth home. +- **I1** `architecture/errors.md` — states the `DecodeError` wrap happens + "at the seam in `Client.send` / `AsyncClient.send`"; the actual + `try/except Exception: raise DecodeError(...)` lives in + `_BoundDecoder.decode` (`decoders/_resolver.py:38-43`), matching what + `architecture/decoders.md:12` already says. Rewrite the sentence to point at + `_BoundDecoder.decode`. +- **I4** `docs/testing.md` — the "why not respx" paragraph says + `"MockTransport is the public test seam in httpx"` and `"breaks across + httpx major versions"`; every other reference in the file and in + `architecture/testing.md:4` says `httpx2`, which is a distinct package + (`httpx2>=2.0.0,<3.0`), not an alias for `httpx`. Change both `httpx` + occurrences on that line to `httpx2`. + +No `architecture/` promotion needed beyond I1 itself — these are corrections +that bring stale prose back into line with the current truth home, not +behavior changes. + +## Files + +- `src/httpware/errors.py` — C1 `BulkheadFullError` docstring +- `src/httpware/client.py` — C2 `AsyncClient.stream()` docstring +- `architecture/errors.md` — I1 `DecodeError` wrap-site correction +- `docs/testing.md` — I4 `httpx` → `httpx2` + +## Verification + +- [x] Each edit matches its cited source line (C1↔`bulkhead.py:133,175`, + C2↔`architecture/client.md:23`, I1↔`decoders/_resolver.py:38-43`, + I4↔`architecture/testing.md:4` + `pyproject.toml:36`). +- [x] `mkdocs build --strict` succeeds (no broken links/refs introduced). +- [x] `just lint-ci` — clean. +- [x] `just test` — 780 passed, 100% coverage. +- [x] Final read-through — no residual "AsyncBulkhead slot" / "for v1" / + "Client.send / AsyncClient.send" wrap claim / bare `httpx` on the + respx line remains. diff --git a/src/httpware/client.py b/src/httpware/client.py index 35e876b..19ea0cd 100644 --- a/src/httpware/client.py +++ b/src/httpware/client.py @@ -1035,7 +1035,7 @@ async def stream( # noqa: PLR0913 — mirrors httpx2 per-method signatures; kwa is closed when the context exits. Bypasses the middleware chain (no AsyncRetry, no AsyncBulkhead, no user-installed - middleware) for v1 — see architecture/client.md for the contract. + middleware) — see architecture/client.md for the contract. Auto-raises StatusError subclasses on 4xx/5xx (NotFoundError, ServiceUnavailableError, etc.) — consistent with client.get()/post()/etc. diff --git a/src/httpware/errors.py b/src/httpware/errors.py index 81dd82f..6e0ae4a 100644 --- a/src/httpware/errors.py +++ b/src/httpware/errors.py @@ -181,7 +181,7 @@ def __init__( class BulkheadFullError(_KeywordReduceMixin, ClientError): - """Raised when ``acquire_timeout`` elapses before an AsyncBulkhead slot becomes available. + """Raised when ``acquire_timeout`` elapses before a bulkhead slot becomes available. Carries the configured caps for caller logging/alerting. """