LCORE-1371: MCP OAuth Integration Tests#1218
Conversation
…points to verify 401 responses with WWW-Authenticate when MCP OAuth is required. Add mock fixtures for Llama Stack client interactions in each test file.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughAdds integration tests and fixtures verifying that the /query (v2), /streaming_query, and /tools endpoints propagate MCP OAuth HTTP 401 responses, both with and without a WWW-Authenticate header. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
tests/integration/endpoints/test_streaming_query_integration.py (1)
76-80:⚠️ Potential issue | 🟠 MajorSame potentially incorrect patch target as in
test_query_integration.py.
"utils.responses.get_mcp_tools"patches the definition site. Ifapp.endpoints.streaming_queryimports the function directly (from utils.responses import get_mcp_tools), the mock won't intercept calls from that module. Verify and, if needed, patch"app.endpoints.streaming_query.get_mcp_tools"instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/endpoints/test_streaming_query_integration.py` around lines 76 - 80, The mock currently patches "utils.responses.get_mcp_tools" which may not intercept calls if streaming_query imports get_mcp_tools directly; update the test to patch the symbol used by the code under test by replacing the target with "app.endpoints.streaming_query.get_mcp_tools" (or confirm the import style in app.endpoints.streaming_query and patch that module's get_mcp_tools) so the AsyncMock side_effect oauth_401 is applied where streaming_query calls get_mcp_tools.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@tests/integration/endpoints/test_streaming_query_integration.py`:
- Around line 76-80: The mock currently patches "utils.responses.get_mcp_tools"
which may not intercept calls if streaming_query imports get_mcp_tools directly;
update the test to patch the symbol used by the code under test by replacing the
target with "app.endpoints.streaming_query.get_mcp_tools" (or confirm the import
style in app.endpoints.streaming_query and patch that module's get_mcp_tools) so
the AsyncMock side_effect oauth_401 is applied where streaming_query calls
get_mcp_tools.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
tests/integration/endpoints/test_query_integration.pytests/integration/endpoints/test_streaming_query_integration.pytests/integration/endpoints/test_tools_integration.py
tests/integration/endpoints/test_streaming_query_integration.py
Outdated
Show resolved
Hide resolved
…es out in query, streaming_query, and tools endpoints. Each test verifies that a 401 status is returned without a WWW-Authenticate header upon timeout.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/integration/endpoints/test_query_integration.py (1)
293-344: Test approach inconsistency vs.test_tools_integration.pyfor the timeout scenario.
test_query_v2_endpoint_returns_401_when_oauth_probe_times_outmocksget_mcp_toolsdirectly to raise a 401, skipping the realprobe_mcp_oauth_and_raise_401/ aiohttp path entirely. The analogous test intest_tools_integration.py(test_tools_endpoint_returns_401_when_oauth_probe_times_out) exercises the real probe by mockingaiohttp.ClientSessionto raiseTimeoutError. This gap means the query-side timeout path through the probe function itself is untested.Consider adopting the same deeper integration strategy used in
test_tools_integration.pyif the intent is to validate the full timeout propagation path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/endpoints/test_query_integration.py` around lines 293 - 344, The test test_query_v2_endpoint_returns_401_when_oauth_probe_times_out currently mocks get_mcp_tools directly; instead simulate the real timeout path by letting get_mcp_tools call probe_mcp_oauth_and_raise_401 and cause a TimeoutError in the HTTP layer (like the pattern in test_tools_integration.py). Replace the direct mock of get_mcp_tools with a mock that patches aiohttp.ClientSession (or the underlying session.request) to raise asyncio.TimeoutError during the probe so probe_mcp_oauth_and_raise_401 raises the same 401-without-WWW-Authenticate, then assert query_endpoint_handler propagates that HTTPException; reference functions: test_query_v2_endpoint_returns_401_when_oauth_probe_times_out, get_mcp_tools, probe_mcp_oauth_and_raise_401, and aiohttp.ClientSession.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/integration/endpoints/test_tools_integration.py`:
- Line 123: The long line creating the patch call exceeds Black's line length;
break the call into multiple lines using implicit continuation with parentheses
so Black can reformat it. Locate the mocker.patch invocation that targets
"utils.mcp_oauth_probe.aiohttp.ClientSession" and rewrite it across lines (e.g.,
put the target string on its own line and the return_value=mock_session_cm on
the next line inside parentheses) so the call remains the same but fits within
the formatter's column limit.
---
Nitpick comments:
In `@tests/integration/endpoints/test_query_integration.py`:
- Around line 293-344: The test
test_query_v2_endpoint_returns_401_when_oauth_probe_times_out currently mocks
get_mcp_tools directly; instead simulate the real timeout path by letting
get_mcp_tools call probe_mcp_oauth_and_raise_401 and cause a TimeoutError in the
HTTP layer (like the pattern in test_tools_integration.py). Replace the direct
mock of get_mcp_tools with a mock that patches aiohttp.ClientSession (or the
underlying session.request) to raise asyncio.TimeoutError during the probe so
probe_mcp_oauth_and_raise_401 raises the same 401-without-WWW-Authenticate, then
assert query_endpoint_handler propagates that HTTPException; reference
functions: test_query_v2_endpoint_returns_401_when_oauth_probe_times_out,
get_mcp_tools, probe_mcp_oauth_and_raise_401, and aiohttp.ClientSession.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
tests/integration/endpoints/test_query_integration.pytests/integration/endpoints/test_streaming_query_integration.pytests/integration/endpoints/test_tools_integration.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/integration/endpoints/test_streaming_query_integration.py
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/integration/endpoints/test_tools_integration.py (1)
48-48: Prefer@pytest.mark.usefixturesover discarding the fixture value.
_ = test_configis used solely to activate the fixture. The more idiomatic pytest pattern is to annotate the test with@pytest.mark.usefixtures("test_config")and remove the parameter from the signature, which makes the intent clearer.Same applies to line 102.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/endpoints/test_tools_integration.py` at line 48, Replace the pattern of accepting and discarding the fixture value by using pytest's usefixtures marker: remove the test_config parameter from the test function signatures where you only do `_ = test_config` and instead annotate those tests with `@pytest.mark.usefixtures`("test_config") (apply to both occurrences around lines with `_ = test_config`, e.g., the tests in tests/integration/endpoints/test_tools_integration.py that currently accept test_config as a parameter).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/integration/endpoints/test_tools_integration.py`:
- Line 48: Replace the pattern of accepting and discarding the fixture value by
using pytest's usefixtures marker: remove the test_config parameter from the
test function signatures where you only do `_ = test_config` and instead
annotate those tests with `@pytest.mark.usefixtures`("test_config") (apply to both
occurrences around lines with `_ = test_config`, e.g., the tests in
tests/integration/endpoints/test_tools_integration.py that currently accept
test_config as a parameter).
|
Hey @radofuchs do you think that you could review this? |
Description
Created integration tests for the MCP OAuth flow on the following endpoints:
/tools/query/streaming_queryType of change
Tools used to create PR
Identify any AI code assistants used in this PR (for transparency and review context)
Related Tickets & Documents
Checklist before requesting a review
Testing
Summary by CodeRabbit