Skip to content

test(bedrock): accept the router kwarg in the knowledge base search fake - #39420

Merged
mateo-berri merged 2 commits into
litellm_internal_stagingfrom
litellm_bedrock_kb_fake_router_kwarg
Sep 2, 2026
Merged

mateo-berri merged 2 commits into
litellm_internal_stagingfrom
litellm_bedrock_kb_fake_router_kwarg

Conversation

@mateo-berri

@mateo-berri mateo-berri commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

TLDR

Problem this solves:

How it solves it:

  • The fake now accepts router=None, matching the real handler
  • Test-only change, no product code touched

User Flow

Before: any PR that merges staging inherits a red CircleCI logging_testing job, because test_bedrock_kb_request_body_has_transformed_filters dies with TypeError: got an unexpected keyword argument 'router' before it asserts anything

After: the same job runs that test through its assertions and goes green, so PRs merging staging stop inheriting the red job

Relevant issues

None

Linear ticket

Resolves LIT-6766

Pre-Submission checklist

Please complete all items before asking a LiteLLM maintainer to review your PR

  • I have added meaningful tests
  • The handful of test files covering my change pass locally, e.g. uv run pytest tests/test_litellm/<your_test_file>.py -v. Leave the suites (make test-unit-*, make test-unit) to CI: it finishes in ~15 minutes where a laptop takes an hour or more
  • My PR passes all required CI/CD checks (e.g., lint, schema.d.ts sync check, etc.)
  • My PR's scope is as isolated as possible; it only solves 1 specific problem
  • I have received a Greptile Confidence Score of at least 4/5 before requesting a maintainer review (Greptile reviews automatically once the PR is opened; only comment @greptileai to re-request a review after pushing changes)

Delays in PR merge?

If you're seeing a delay in your PR being merged, ping the LiteLLM Team on Slack (#pr-review).

Screenshots / Proof of Fix

This change has no end-user surface, so the proof is the failing test itself: the same pytest command at the merge base and at the tip, run from a bootstrapped worktree, plus the CircleCI logging_testing job on this PR

Before (719b671)

  1. uv run --no-sync pytest tests/logging_callback_tests/test_bedrock_knowledgebase_hook.py::test_bedrock_kb_request_body_has_transformed_filters -q -p no:cacheprovider
  2. Observed output:
TypeError: test_bedrock_kb_request_body_has_transformed_filters.<locals>.fake_async_vector_store_search_handler() got an unexpected keyword argument 'router'
=========================== short test summary info ============================
FAILED tests/logging_callback_tests/test_bedrock_knowledgebase_hook.py::test_bedrock_kb_request_body_has_transformed_filters
1 failed in 32.69s

After (8e26d13)

  1. uv run --no-sync pytest tests/logging_callback_tests/test_bedrock_knowledgebase_hook.py::test_bedrock_kb_request_body_has_transformed_filters -q -p no:cacheprovider
  2. Observed output:
.                                                                        [100%]
1 passed in 7.12s
  1. uv run --no-sync pytest tests/logging_callback_tests/test_bedrock_knowledgebase_hook.py -q -p no:cacheprovider (the whole file)
  2. Observed output (the other 7 failures are local-environment only: the machine's AWS login credential provider needs botocore[crt], and the mock-OpenAI cases fail the same way at the merge base, where the whole file reads 8 failed, 4 passed, 1 warning in 25.51s; the only delta between the two runs is this PR's test):
FAILED tests/logging_callback_tests/test_bedrock_knowledgebase_hook.py::test_e2e_bedrock_knowledgebase_retrieval_with_completion
FAILED tests/logging_callback_tests/test_bedrock_knowledgebase_hook.py::test_e2e_bedrock_knowledgebase_retrieval_with_llm_api_call
FAILED tests/logging_callback_tests/test_bedrock_knowledgebase_hook.py::test_e2e_bedrock_knowledgebase_retrieval_with_llm_api_call_streaming
FAILED tests/logging_callback_tests/test_bedrock_knowledgebase_hook.py::test_e2e_bedrock_knowledgebase_retrieval_with_llm_api_call_with_tools_and_filters
FAILED tests/logging_callback_tests/test_bedrock_knowledgebase_hook.py::test_openai_with_knowledge_base_mock_openai
FAILED tests/logging_callback_tests/test_bedrock_knowledgebase_hook.py::test_openai_with_mixed_tool_call_mock_openai
FAILED tests/logging_callback_tests/test_bedrock_knowledgebase_hook.py::test_openai_with_vector_store_ids_in_tool_call_mock_openai
7 failed, 5 passed, 1 warning in 13.54s
  1. CircleCI logging_testing on this PR: green at b6b5b31ce4 (the same fix before the router parameter got its annotation); at this tip: green at 8e26d13fa0. The three red non-required jobs in that workflow (local_testing_part1, local_testing_part2, litellm_router_testing) each fail on a single live-OpenAI timeout test (test_openai_embedding_timeouts, test_timeout_streaming, test_router_timeout) with an httpx connection error, the known timeout flakes, unrelated to this file

Type

✅ Test

Caveats (if any)

Low

  • No end-user-visible behavior changes (test-only), so the proof is the pytest Before/After plus the CircleCI job rather than a live proxy run
  • The fake ignores router; the test asserts the Bedrock request body, not routing
  • Only the new router parameter carries a type annotation; the fake's other twelve parameters stay untyped as they were, since the file sits outside the basedpyright gate
  • /live-pr-risk: no product code changes and the fake's only dependent is the patch.object in the same test, so there is no proxy path to A/B; CHECKED at 8e26d13: CircleCI logging_testing green, no untested dependent path, no breaking change
  • captured_request_body: dict = {} predates this PR and stays untouched

Final Attestation

  • The tests check the right things, including the edge cases, and regressions in the respective real-world customer use-cases are not possible after this PR

@mateo-berri

Copy link
Copy Markdown
Contributor Author

bugbot run

@devin-ai-integration devin-ai-integration Bot added the risk:low Small, isolated, low-risk change label Sep 2, 2026
@devin-ai-integration

Copy link
Copy Markdown
Contributor

PR #39420 labeled risk:low (1 test file, +1/-0, test-only). No enterprise label, so Linear routing was skipped.

@cursor cursor Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

Reviewed by Cursor Bugbot for commit b6b5b31. Configure here.

@greptile-apps

greptile-apps Bot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This test-only change updates the Bedrock knowledge-base search fake to match the real handler’s optional router parameter.

  • Adds a litellm.Router | None annotation and None default.
  • Prevents the patched fake from rejecting the handler’s router keyword argument.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains.

Important Files Changed

Filename Overview
tests/logging_callback_tests/test_bedrock_knowledgebase_hook.py The fake handler now accepts and types the optional router argument, resolving the prior thread without changing test assertions or production behavior.

Reviews (2): Last reviewed commit: "test(bedrock): type the router parameter..." | Re-trigger Greptile

Comment thread tests/logging_callback_tests/test_bedrock_knowledgebase_hook.py Outdated
@codecov

codecov Bot commented Sep 2, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@mateo-berri

Copy link
Copy Markdown
Contributor Author

@greptileai

@mateo-berri

Copy link
Copy Markdown
Contributor Author

bugbot run

@cursor cursor Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

Reviewed by Cursor Bugbot for commit 8e26d13. Configure here.

@mateo-berri
mateo-berri merged commit 3c6b070 into litellm_internal_staging Sep 2, 2026
106 of 125 checks passed
@mateo-berri
mateo-berri deleted the litellm_bedrock_kb_fake_router_kwarg branch September 2, 2026 23:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

risk:low Small, isolated, low-risk change run-ci

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants