Repository navigation
docs(agents): define fakes and re-record rules; clean up anthropic tests - #942
Abhijeet Prasad (AbhiPrasad) merged 5 commits into
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Abhijeet Prasad (AbhiPrasad)
changed the title
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5e2481511e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "Codex (@codex) review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "Codex (@codex) address that feedback".
| ### Recording traps | ||
|
|
||
| - **Harbor:** `test_atif_import_*` cassettes record Braintrust API traffic, not Harbor traffic. Run `test_harbor` with `BRAINTRUST_API_KEY` unset; `___TEST_API_KEY__` short-circuits login and points at `api.braintrust.ai`, so playback fails with a host mismatch. Do not re-record without a real org. | ||
| - **huggingface_hub:** chat tests pin `provider="cerebras"` and text-generation pins `featherless-ai`, for models HF no longer routes there, so re-recording fails with "not supported by provider". The old cassettes still play back. As of 2026-10-13 the local HF token also returns 402 "no remaining credits", so do not try to re-record huggingface_hub. |
There was a problem hiding this comment.
When refreshing Hugging Face cassettes with a usable token, this blanket instruction can incorrectly prevent a necessary refresh: text generation is not pinned to featherless-ai; py/src/braintrust/integrations/huggingface_hub/test_huggingface_hub.py:43-46 sets TEXT_GEN_PROVIDER = "auto" so the router can select a currently available backend. Treat unsupported routing and exhausted credentials as conditions to verify rather than permanently directing contributors not to re-record.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3844a04c42
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "Codex (@codex) review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "Codex (@codex) address that feedback".
| batch_id = next( | ||
| b.id | ||
| for b in _get_client().messages.batches.list(limit=20).data | ||
| if b.processing_status == "ended" and b.request_counts.succeeded | ||
| ) |
There was a problem hiding this comment.
When a contributor re-records with credentials for a workspace containing unrelated batches, this predicate can select any of the 20 most recent successful jobs, after which VCR writes that batch's complete JSONL prompts and outputs into a checked-in cassette. This risks disclosing unrelated workspace data; on a fresh workspace it can instead raise StopIteration because the batches created earlier in this suite are still in_progress. Fetch results only from a batch known to belong to this test suite.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch, fixed in 168478d.
The test now creates its own two-request batch with _make_batch_requests(), polls until it ends (sleeping only while recording), and asserts the results are exactly req-1 and req-2. The cassette therefore only contains this suite's requests, and it no longer depends on other batches existing, which also removes the StopIteration case.
The existing recording happened to pick a batch from this suite's own test_sync_batches_create_multi_model_metadata, so no unrelated data was committed.
Abhijeet Prasad (AbhiPrasad)
changed the title
Coding-agent sessions on this repo kept writing hand-made stand-ins (for example a plain function posing as an ADK tool) while believing they followed the no-mocks rule. AGENTS.md now says these count as fakes and bans MagicMock/Mock in provider integration tests. Re-record gotchas learned across weekly dependency updates (missing keys, quota errors, retired models, hand-authored cassettes, coupled sessions) only lived in one person's local agent memory. Move them into the repo so every agent sees them: - sdk-vcr-workflows: general "When Recording Fails" rules, including when to restore from git vs. re-record - sdk-dependency-updates: provider-specific list in references/session-mapping.md, linked from the re-record step Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- test_log_message_to_span_includes_stop_reason_and_stop_sequence logs to a real span through memory_logger instead of asserting on a MagicMock's call args. - test_batches_results_produces_span replays recorded results of an already-ended batch instead of patching Batches.results to return a MagicMock. It finds the batch through batches.list(), so re-recording keeps working after a given batch's results expire. The error-path test keeps its narrow unittest.mock.patch side_effect. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- Remove test_anthropic_messages_sync and test_anthropic_messages_create_stream_true. The reasoning-tokens test covers the same create and create(stream=True) paths more strictly on both matrix versions; it now also checks the stream metadata flag. - Remove the _log_message_to_span unit test. Its usage checks duplicate test_extract_anthropic_usage, and stop_sequence is now covered by a real recording: test_anthropic_messages_model_params_inputs prompts the model to stop on END (latest cassette re-recorded). - Run the messages.parse feature-detection test against real clients on SDKs without messages.parse (0.48.0) instead of hand-written fakes. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Keep only the general recording-failure rules in sdk-vcr-workflows. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
test_batches_results_produces_span picked any recently ended batch from batches.list(). With a contributor's key that could write an unrelated batch's prompts and outputs into a checked-in cassette, and on a fresh workspace it raised StopIteration. The test now creates its own two-request batch and polls until it ends (sleeping only while recording), then asserts the results are exactly req-1 and req-2. It uses LATEST_MODEL because the 0.48.0 default, claude-3-haiku-20240307, is retired and its batch requests now error. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
168478d to
f6b7789
Compare
1 participant
Summary
Agent-guidance changes from a review of the last 30 days of coding-agent sessions on this repo (logged in Braintrust,
bt-coding-agent-insights), plus the anthropic test cleanup the new rule calls for.1. Define "fakes" in the mock rule (
AGENTS.md)Agents already had strong anti-mock guidance but still wrote hand-made stand-ins (for example, a plain function posing as an ADK tool) and treated them as fine. Core Rule 6 now says:
MagicMock/Mockis not allowed in provider integration tests (integrations/<provider>/test_*.py). Shared helper unit tests such asintegrations/test_utils.pyare exempt, and narrowmonkeypatcherror injection is still allowed.2. General re-record rules (
sdk-vcr-workflows)A new "When Recording Fails" section covers missing keys, 402/429 quota errors, retired models and upstream drift, hand-authored cassettes, when to restore from git vs. re-record, and traps that silently lose cassettes (skipped sessions, multi-file sessions, shared cassette names, hangs).
3. Anthropic test cleanup (
anthropic/test_anthropic.py)test_batches_results_produces_span[sync/async]Batches.resultsto return aMagicMock. Now: creates its own two-request batch, polls until it ends (sleeping only while recording), and replays the real results. UsesLATEST_MODELbecauseclaude-3-haiku-20240307is retired.test_log_message_to_span_includes_stop_reason_and_stop_sequencetest_extract_anthropic_usagecovers its usage checks.stop_sequenceis now covered by a real recording intest_anthropic_messages_model_params_inputs(latest re-recorded).test_anthropic_messages_sync,test_anthropic_messages_create_stream_truetest_anthropic_messages_reasoning_tokens_metricscovers the same paths more strictly on both versions, and now also checks thestreammetadata flag.test_wrap_anthropic_preserves_messages_parse_feature_detectionmessages.parse) instead of hand-written fake classes; skips on latest.No
MagicMock/Mockremains in the file. The error-path test keeps its narrowpatch(side_effect=...).Validation
nox -s "test_anthropic(<version>)" -- --vcr-record=all -k ...forlatestand0.48.0.CI=1 nox -R -s "test_anthropic(latest)": 53 passed, 1 skipped.CI=1 nox -R -s "test_anthropic(0.48.0)": 36 passed, 18 skipped.api.anthropic.comrequests, with no keys or Braintrust traffic.make check-unused-cassettescurrently skips table-driven sessions such astest_anthropic).pre-commithooks pass.🤖 Generated with Claude Code