Skip to content

Fix the defects the agents page turned up - #20

Merged
AlexeyShalaev merged 5 commits into
masterfrom
fix/agents-page-findings
Sep 6, 2026
Merged

Fix the defects the agents page turned up#20
AlexeyShalaev merged 5 commits into
masterfrom
fix/agents-page-findings

Conversation

@AlexeyShalaev

Copy link
Copy Markdown
Member

Five defects that surfaced while writing docs/agents.md against the source. Each one
reproduced first; the order below is the order they were fixed — the ones that change what
a caller gets first, the reader-only ones last.

1. The retry method gate could only permit, never refuse

Wrong. The gate was info.method not in config.methods and not info.idempotent — an OR
of two permissions, so neither half could say no. Two documented behaviours were dead:

  • idempotent=False on a GET was a silent no-op. GET is in retry.methods, so the
    call site's veto never reached the decision, while the per-call guide said "False works
    in the other direction: it forbids retrying a normally-idempotent method".
  • RetryConfig.methods could not narrow at all. Every adapter derives
    RequestInfo.idempotent from IDEMPOTENT_METHODS, so a DELETE arrives with
    idempotent=True and was retried even after the operator took DELETE out of methods
    — which docs/agents.md rule 9 states as the way to stop retrying a method.

How I know. Against master, with the values the shipped adapters actually put in
RequestInfo:

default config, DELETE (adapter-derived idempotent=True): retry
methods={'GET'},   DELETE (adapter-derived idempotent=True): retry
methods={'GET'},   GET    (call site says idempotent=False): retry
default config,    GET    (call site says idempotent=False): retry

Only the first line is correct. Two independent docs claim narrowing on two different axes
and one expression breaks both, so I read this as the code drifting, not the docs.

Changed. RequestInfo.idempotent restates the method's RFC default unless the call site
overrode it, so a flag that disagrees with IDEMPOTENT_METHODS is the call site talking
and decides on its own; a flag that only restates the default leaves the decision with the
operator's retry.methods. POST + idempotent=True and a widened methods list behave
exactly as before, and the refusal still counts as retry_skipped{reason="method"}.

Tests: four policy cases in tests/unit/core/policy/test_retry.py (three of them fail on
master) plus a parity pair that drives a vetoed GET through all five adapters against
the real origin and asserts one request and one method skip.

Not breaking as an API, but it does change behaviour where a caller asked for something
and was ignored: an explicit idempotent=False, or a customised retry.methods (note that
a list like {"GET", "POST"} now really means only those two). A default config with no
per-call flags is unaffected. That is the whole point of the change, so it is a fix: and
not a feat!: — worth a line in the release notes all the same.

2. report.native_overrides was always empty

Wrong. compile_plan has accepted native_overrides since the first release, and both
docs/guide/native-options.md and docs/guide/capabilities.md promise the report lists the
accepted passthrough per slot — but no adapter ever passed it.

How I know. Building a requests client with
NativeOptions.of(session={"trust_env": False}): the option is applied
(session.trust_env is False) and handle.report.native_overrides == {}.

Changed. A shared accepted_overrides helper next to validate_native; every adapter
now hands its validated passthrough to compile_plan. Shape: {slot: (key, ...)}, keys
sorted, slots the caller left empty omitted. Covered per adapter family and in
tests/unit/core/test_capabilities.py. Not breaking — a field that was always {} starts
carrying what it always advertised.

3. conn_metrics had no consumer

Wrong. conn_metrics is on both normalizer protocols and implemented by every adapter —
aiohttp does real work for it through TraceConfig — but neither engine called it, so
Attempt.conn was always None and nothing surfaced the timings, while
aiohttp/capabilities.py declares CONN_METRICS: native.

How I know. No conn_metrics call anywhere in core/engine/, no .conn read in the
emitter, and no raise/read of the value outside the aiohttp trace tests.

Changed. Both engines fill Attempt.conn from the normalizer, and the emitter hangs
what the adapter saw on the call span: http.connection.dns_duration,
...connect_duration, ...tls_duration, ...pool_wait_duration, ...reused and
network.protocol.version. A phase the adapter cannot observe stays off the span rather
than being reported as a zero, and on a retried call the last attempt that saw them wins.

The metric families are a frozen contract and are untouched — that is why the span, not a
new histogram, is the surface. ClientTelemetry.attempt_end now takes the observation
first, like call_end; it is called only by the two engines.

Covered in both engine suites, in the emitter suite, and end to end in
tests/integration/adapters/test_aiohttp.py: two calls to the same origin, the first
showing a connect duration and reused: False, the second reused: True and no connect
duration. Not breaking.

4. NotReplayableError is exported but never raised

Not a code defect — the docstring was the defect. The engine deliberately ends a call
with the response it already has plus retry_skipped{reason="non_replayable"}; that has
always been pinned by test__non_replayable_body__skip_sentinel_instead_of_retry. Removing
the export would break except clauses that compile today, and turning the refusal into an
exception would be a much worse change than the one it fixes.

The class docstring read like an error you catch. It now says the engine never raises it,
and the retry gate list in the guide says the veto is a counter, never an exception — so
nobody writes an except NotReplayableError that can never fire. docs/agents.md already
had this right and is unchanged.

5. DEFAULT_SENSITIVE_HEADERS pointed into a private module

Wrong. The constant is a root export with no config knob behind it (the knob was dropped
in 0.2.0; headers never reach logs or spans by design), so its entire purpose is the recipe
in the masking guide — and the other half of that recipe, redact_headers, was only
importable from clientwright.core.telemetry.redaction.

Changed. redact_headers is now a root export; the masking guide and the agents page use
it. Additive, so feat: rather than fix:. redact_url stays where it is: its companion
constant DEFAULT_SENSITIVE_QUERY_PARAMS does have a config knob
(ObservabilityConfig.sensitive_query_params), so it is not orphaned in the same way — say
the word and I will export it too for symmetry.

Checks

make check                 # ruff, ruff format, mypy (97 files), 3/3 import-linter contracts
make test-unit             # 634 passed, 216 deselected
make test-integration      # 174 passed, 676 deselected
make test                  # 808 passed, 42 skipped — coverage 99.59% (threshold 97%)

uv sync --frozen --all-extras throughout; uv.lock is unchanged.

One thing I did not touch

docs/adapters/aiohttp.md says the body read duration is reported as
http_client_body_duration_seconds "measured via the trace hooks", while
docs/agents.md says that metric is httpx-family only and the aiohttp adapter never calls
wrap_stream. One of the two pages is wrong about the aiohttp body metric; it is outside
this round's findings, so I left it alone.

``conn_metrics`` sits on both normalizer protocols and every adapter implements
it - aiohttp does real work for it through ``TraceConfig`` - but neither engine
ever called it. ``Attempt.conn`` was therefore always ``None`` and aiohttp's
``conn_metrics: native`` declaration bought a caller nothing.

Both engines now fill ``Attempt.conn`` from the normalizer, and the emitter hangs
what the adapter saw on the call span as ``http.connection.*`` plus
``network.protocol.version``. A phase the adapter cannot observe stays off the
span instead of being reported as a zero. The metric families are a frozen
contract and are untouched; ``ClientTelemetry.attempt_end`` takes the
observation first, like ``call_end``.
…ough

``compile_plan`` has taken ``native_overrides`` since the first release and both
the native-options and capabilities guides promise the report lists what was
accepted per slot, but no adapter ever passed it, so
``handle.report.native_overrides`` was always ``{}``.

Every adapter now hands its validated passthrough to ``compile_plan`` through a
shared ``accepted_overrides`` helper: ``{slot: (key, ...)}``, keys sorted, slots
the caller left empty omitted.
``DEFAULT_SENSITIVE_HEADERS`` is a root export with no config knob behind it -
headers never reach a log line or a span - so its whole purpose is the recipe in
the masking guide. The other half of that recipe, ``redact_headers``, lived at
``clientwright.core.telemetry.redaction`` and was not exported, which made the
public constant point into a private module.

``redact_headers`` is now a root export; the guide and the agents page use it.
The gate was ``method not in retry.methods and not info.idempotent``: an OR of
two permissions, so neither half could ever say no.

* ``idempotent=False`` on a GET was a silent no-op - the method was still in
  ``retry.methods``, so the call site's veto never reached the decision, though
  the per-call guide promises it does.
* ``RetryConfig.methods`` could not narrow at all. Every adapter derives
  ``RequestInfo.idempotent`` from ``IDEMPOTENT_METHODS``, so a DELETE arrives
  with ``idempotent=True`` and was retried even after the operator took DELETE
  out of ``methods`` - which the agents page states as the way to stop it.

``RequestInfo.idempotent`` restates the method's RFC default unless the call site
overrode it, so a flag that disagrees with ``IDEMPOTENT_METHODS`` is the call
site talking and decides; one that only restates the default leaves the decision
with ``retry.methods``. POST + ``idempotent=True`` and a widened ``methods`` list
keep working exactly as before.

Behaviour changes only where a caller asked for it and was ignored: an explicit
``idempotent=False``, or a customised ``retry.methods``. A default config is
unaffected. Refusals still count as ``retry_skipped{reason="method"}``.
The class docstring read like an error you catch. It is not raised anywhere: a
non-replayable body ends the call with the response it already has plus a
``retry_skipped{reason="non_replayable"}`` counter, which
``test__non_replayable_body__skip_sentinel_instead_of_retry`` has always pinned.
The docstring and the retry gate list now say so, so nobody writes an
``except NotReplayableError`` that can never fire.
@AlexeyShalaev
AlexeyShalaev merged commit 504b022 into master Sep 6, 2026
8 checks passed
@AlexeyShalaev
AlexeyShalaev deleted the fix/agents-page-findings branch September 6, 2026 18:46
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.

1 participant