Skip to content

LCORE-1462: Fix skipping "client" MCP Auth Type in /tools#1382

Merged
tisnik merged 1 commit intolightspeed-core:mainfrom
jrobertboos:lcore-1462
Mar 24, 2026
Merged

LCORE-1462: Fix skipping "client" MCP Auth Type in /tools#1382
tisnik merged 1 commit intolightspeed-core:mainfrom
jrobertboos:lcore-1462

Conversation

@jrobertboos
Copy link
Copy Markdown
Contributor

@jrobertboos jrobertboos commented Mar 23, 2026

Description

Type of change

  • Refactor
  • New feature
  • Bug fix
  • CVE fix
  • Optimization
  • Documentation Update
  • Configuration Update
  • Bump-up service version
  • Bump-up dependent library
  • Bump-up library or tool used for development (does not change the final image)
  • CI configuration change
  • Konflux configuration change
  • Unit tests improvement
  • Integration tests improvement
  • End to end tests improvement
  • Benchmarks improvement

Tools used to create PR

Identify any AI code assistants used in this PR (for transparency and review context)

  • Assisted-by: (e.g., Claude, CodeRabbit, Ollama, etc., N/A if not used)
  • Generated by: (e.g., tool name and version; N/A if not used)

Related Tickets & Documents

  • Related Issue #
  • Closes #

Checklist before requesting a review

  • I have performed a self-review of my code.
  • PR has passed all pre-merge test jobs.
  • If it is a core feature, I have added thorough tests.

Testing

  • Please provide detailed steps to perform tests related to this code change.
  • How were the fix/results from this change verified? Please provide relevant screenshots or results.

Summary by CodeRabbit

  • Bug Fixes

    • Improved validation of authorization headers for MCP servers before attempting to fetch tools; servers with missing required authentication are now skipped with appropriate logging.
  • Tests

    • Enabled test scenario for MCP client-provided authentication handling.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Mar 23, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: e434d191-85ed-4e9d-9b1a-de7142d73f00

📥 Commits

Reviewing files that changed from the base of the PR and between 41dfe2a and a3f5e0e.

📒 Files selected for processing (2)
  • src/app/endpoints/tools.py
  • tests/e2e/features/mcp.feature
💤 Files with no reviewable changes (1)
  • tests/e2e/features/mcp.feature

Walkthrough

The pull request adds pre-validation of MCP server authorization headers before attempting to fetch tools from each server. The tools endpoint now checks for unresolved required auth headers per toolgroup and skips servers with incomplete authentication. Related test coverage is enabled by removing a skip marker.

Changes

Cohort / File(s) Summary
Authorization Header Validation
src/app/endpoints/tools.py
Added find_unresolved_auth_headers import and logic to validate MCP server authorization headers before calling the tools API. Moved mcp_server lookup earlier in the loop and refactored header resolution to occur prior to validation checks; servers with unresolved required headers are now skipped with a warning log. Adjusted server_source metadata assignment to use pre-resolved mcp_server value.
Test Scenario Enablement
tests/e2e/features/mcp.feature
Removed @skip marker from the "Check if tools endpoint succeeds by skipping when MCP client-provided auth token is omitted" scenario to allow it to execute during test runs.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly identifies the issue (LCORE-1462) and the specific fix applied: handling the 'client' MCP Auth Type in the /tools endpoint. The changes confirm this fix by adding authorization header validation and removing a skip marker from the client-auth test scenario.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@jrobertboos jrobertboos marked this pull request as ready for review March 24, 2026 15:05
Copy link
Copy Markdown
Contributor

@tisnik tisnik left a comment

Choose a reason for hiding this comment

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

LGTM

@tisnik tisnik merged commit 8d614a6 into lightspeed-core:main Mar 24, 2026
24 checks passed
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.

2 participants