Skip to content

Review fixes and cassette-backed provider tests - #1

Merged
egafni merged 7 commits into
mainfrom
review-fixes
Aug 9, 2026
Merged

egafni merged 7 commits into
mainfrom
review-fixes

Conversation

@egafni

@egafni egafni commented Aug 9, 2026

Copy link
Copy Markdown
Contributor

Fixes from a review of the repo, then a migration of the provider tests from json_cache to HTTP cassettes.

Correctness

  • Score was computed over an unnormalized distribution. score is an expected value, so it only means anything over weights summing to 1. An LLM distribution summing to 0.2 produced score=0.1 on a 0..1 scale. It is now computed over an explicitly normalized copy while the reported probabilities stay untouched when normalize_probabilities=False.
  • Token usage under-reported retried calls. Only the successful attempt's usage was counted, so tokens burned by an attempt that later failed were never billed. A single RunUsage is now threaded through every attempt. Because request counts accumulate across attempts, malformed-structure retries are counted from a per-attempt snapshot rather than the run total.
  • Two prompt defects, both found by recording real traffic. The answers field was described as "keyed by question ID", so gpt-4o-mini invented an ID and nested every answer under it. Discrete score questions carried no criteria at all, so the model guessed its own scale and returned 5 on a 0-4 score.

Tests

json_cache memoized system_one's return value, so on a cache hit none of the client ran — the test compared a stored blob against a literal in the file beside it. VCR (vcrpy via pytest-recording) replays at the HTTP layer instead, so prompt building, output parsing, normalization, confidence, and usage aggregation all execute for real. It caught both prompt bugs above on the first recording run.

  • Expected responses stay inline in the test as readable JSON, now the single source of truth instead of being duplicated in json_cache.json.
  • latency is dropped before comparison rather than pinned to a value no later run can reproduce (REGENERATE_JSON_CACHE=1 previously failed by construction).
  • Cassettes are JSON, matched on request body because every call posts to the same endpoint. On vcrpy's default matchers they would all replay the first interaction.
  • Request credentials filtered; response headers reduced to a content-type allowlist, since providers return organization and workspace IDs, request IDs, cookies, and per-account rate-limit quotas.
  • --block-network is on by default, so a missing cassette fails instead of silently making a live call.

Coverage went from 30 to 46 tests: timeout mapping, TypeSafeUnknownError status passthrough, retry exhaustion, the async retry path, invalid question collections, and a test asserting tokens from failed attempts are billed.

Also

  • Ruff lint families enabled (the existing # noqa: B008 / BLE001 referenced rules that were never on), testpaths set so pytest stops collecting from .venv, distribution renamed to match the imported module, real description.
  • .venv console scripts were shebanged to a stale OpenTypeSafe2 path from the directory rename, so uv run pytest failed outright. Rebuilt.
  • Comment on why OpenTypeSafeClient.__init__ skips super().__init__(), and the invariant that creates.

Test plan

  • uv run pytest -> 46 passed.
  • With OPENAI_API_KEY, ANTHROPIC_API_KEY, and TYPESAFE_API_KEY unset and network blocked -> 46 passed.
  • uv run pytest tests/test_live_apis.py --record-mode=rewrite re-records against the live APIs; verified end to end.
  • Cassettes scanned for the real key values, sk-/Bearer patterns, and identifying headers -> clean.

Notes for review

  • 011551f hardens JsonCache and 24e7595 then deletes it. That is the order the work was requested in; squash if you would rather not carry it.
  • Two fidelity gaps this surfaced, not addressed here: the TypeSafe API returns model: "speed_v10_mango_loco" rather than the requested speed_latest alias, and its ScoreAnswer carries a legend mapping each score level to its criterion, which OpenTypeSafeClient never emits.
  • Still open from the review: _resolve_pydantic_ai_model only knows OpenAI and Anthropic prefixes, and _TOKEN_ERROR_MARKERS substring matching is fragile.

Erik Gafni added 6 commits August 8, 2026 18:00
The score is an expected value, so it is now computed over an explicitly
normalized distribution while the reported probabilities stay untouched when
normalize_probabilities is disabled. Previously an LLM distribution summing to
0.2 produced a score of 0.1 on a 0..1 scale.

A single RunUsage is threaded through every PydanticAI attempt so tokens spent
on attempts that later failed are still billed. Request counts now accumulate
across attempts, so malformed-structure retries are counted from a per-attempt
snapshot rather than the run total.

system_one and system_one_async share an _Evaluation dataclass instead of
duplicating ~35 lines each.
Misses are computed outside the lock so concurrent calls do not serialize on
the network, with a re-check under the lock so the first stored value wins.
The file is written via a temp file and replace so an interrupted run cannot
truncate it. The docstring example no longer looks like a runnable doctest.
test_provider_errors grows from 2 to 10 rows covering timeout mapping, auth on
403, and TypeSafeUnknownError status_code passthrough. Adds retry exhaustion,
async retry coverage, and a test asserting tokens from failed attempts are
billed. Invalid question collections are now checked.

The live-API test no longer constructs TypeSafeClient at import time without a
key, so a fully cached run works with no credentials.
Enables the rule families the existing noqa comments already referenced, stops
pytest collecting from .venv, and renames the distribution to match the
imported module.
Recording real provider traffic surfaced two prompt defects that a cached
response could not.

The answers field was described as "keyed by question ID", so gpt-4o-mini
invented an ID and nested every answer under it, failing validation. The
description now fixes the property names instead.

Discrete score questions carried no criteria at all, leaving the model to guess
its own scale: it returned 5 on a 0-4 score. The criteria are now listed as a
score legend in discrete mode only, since probabilities mode already carries
them as per-label descriptions and naming an integer there made models return a
score instead of a distribution.
json_cache memoized system_one's return value, so on a cache hit none of the
client ran and the test only compared a stored blob to a literal beside it. VCR
replays at the HTTP layer instead, so prompt building, output parsing,
normalization, confidence, and usage aggregation all execute against recorded
provider traffic. It caught two real prompt bugs immediately.

Expected responses stay inline in the test as readable JSON, now the single
source of truth rather than being duplicated in json_cache.json. Latency is
dropped before comparison instead of pinned to an unreproducible value.

Cassettes are JSON, matched on request body because every call posts to the
same endpoint. Request credentials are filtered and response headers reduced to
an allowlist, since providers return organization IDs, request IDs, cookies,
and per-account rate-limit quotas. Placeholder keys are set at conftest import
because provider SDKs demand one before any HTTP happens, so replay needs no
credentials. --block-network is on by default so a missing cassette fails
rather than silently making a live call.
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