feat(platform): support static API-key/bearer-token auth for MCP servers - #13683
feat(platform): support static API-key/bearer-token auth for MCP servers#13683Abhi1992002 wants to merge 14 commits into
Conversation
Some MCP servers (e.g. DataFast) authenticate with a static bearer token issued in the vendor's dashboard rather than a full OAuth2 flow. The MCP tool block only advertised `oauth2` credentials, and the builder dialog used a manually-entered token for tool discovery but never persisted it — so the placed block ran with no credentials and 401'd at runtime. - MCPToolBlock now advertises both `oauth2` and `api_key` credential types and accepts either at runtime (token pulled from access_token or api_key). - The /mcp/token endpoint stores the token as a first-class APIKeyCredentials instead of masquerading it as an OAuth2 token; lookup, cleanup and get_host handle both types so existing OAuth-typed tokens keep working. - The builder MCP dialog offers proactive API-key entry and persists the token via /mcp/token, attaching the credential to the block node. - Frontend credential classifier matches api_key MCP credentials by host.
…o-fill _credential_is_for_mcp_server only matched OAuth2Credentials, so MCP tokens now stored as APIKeyCredentials would be reported missing when copilot auto-fills a graph's required credentials. Match both credential types. Also improve the builder MCP dialog: a rejected manual token now shows a clear "authentication failed — check your token" message instead of bouncing the user into an OAuth flow that will fail again.
|
/review |
|
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:
WalkthroughMCP credential handling now supports OAuth2 and API-key bearer credentials across backend storage, lookup, tool execution, copilot utilities, and frontend discovery. Manual tokens are persisted as API-key credentials and matched by normalized MCP server URL. ChangesMCP credential abstraction
Backend credential storage
Frontend manual-token flow
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant MCPToolDialog
participant mcp_store_token
participant CredentialsStore
participant MCPToolBlock
MCPToolDialog->>mcp_store_token: submit bearer token and server URL
mcp_store_token->>CredentialsStore: store APIKeyCredentials
CredentialsStore-->>mcp_store_token: return credential metadata
MCPToolDialog->>MCPToolBlock: confirm tool with credential metadata
MCPToolBlock->>CredentialsStore: resolve MCP credential
CredentialsStore-->>MCPToolBlock: return API-key or OAuth2 credential
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
autogpt_platform/frontend/src/hooks/useCredentials.ts (1)
30-36: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winNormalize MCP URLs before matching credentials.
mcp_store_tokenpersists a normalized URL, while the block discriminator can retain a trailing slash. Exact comparison then hides a valid saved credential forhttps://server/mcp/. Normalize both values before comparing and add a trailing-slash test.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@autogpt_platform/frontend/src/hooks/useCredentials.ts` around lines 30 - 36, Update the MCP credential matching logic in the credentials hook to normalize both c.host and discriminatorValue before comparison, removing trailing slashes so equivalent URLs match. Preserve the existing null guard and credential collection behavior, and add a test covering a discriminator URL with a trailing slash matching the normalized saved URL.autogpt_platform/frontend/src/app/(platform)/build/components/MCPToolDialog.tsx (1)
143-164: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUse
unknownfor the caught error.Replace
catch (e: any)withcatch (e: unknown)and narrow before readingstatus,message, ordetail;anydisables type checking and conflicts with the frontend guideline to avoidany.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@autogpt_platform/frontend/src/app/`(platform)/build/components/MCPToolDialog.tsx around lines 143 - 164, Update the catch block in the MCP tool connection flow to use catch (e: unknown) instead of any. Narrow e before accessing status, message, or detail, while preserving the existing 401/403 authentication handling and fallback error message behavior.Source: Coding guidelines
🧹 Nitpick comments (1)
autogpt_platform/frontend/src/app/(platform)/build/components/MCPToolDialog.tsx (1)
340-345: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the manual dark-mode overrides.
These controls use
dark:classes even though the design system owns dark-mode styling. Use semantic design-system styling instead.As per coding guidelines, “No
dark:Tailwind classes — the design system handles dark mode.”Also applies to: 355-363
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@autogpt_platform/frontend/src/app/`(platform)/build/components/MCPToolDialog.tsx around lines 340 - 345, Remove the dark: Tailwind classes from the manual token toggle button and the related controls near it, including dark:text-gray-400 and dark:hover:text-gray-300. Preserve the existing semantic text, underline, hover, and layout styling while relying on the design system for dark-mode behavior.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@autogpt_platform/backend/backend/api/features/mcp/routes.py`:
- Around line 369-377: Make MCP credential replacement atomic in routes.py at
lines 369-377 and 443-473: update both OAuth and static-token flows to use the
same store-level atomic replace/create operation, passing the server-matching
criteria and new credential together. Remove the separate read/delete or
ID-collection steps so concurrent submissions cannot retain duplicate
credentials.
In `@autogpt_platform/backend/backend/blocks/mcp/helpers.py`:
- Around line 163-177: Update the credential selection loop around
_mcp_credential_expiry so expired credentials cannot outrank valid non-expiring
credentials. Rank matching credentials by not-expired status first, then use the
existing iteration-order recency tiebreaker; preserve the subsequent
refresh_if_needed handling for the selected OAuth2Credentials.
---
Outside diff comments:
In
`@autogpt_platform/frontend/src/app/`(platform)/build/components/MCPToolDialog.tsx:
- Around line 143-164: Update the catch block in the MCP tool connection flow to
use catch (e: unknown) instead of any. Narrow e before accessing status,
message, or detail, while preserving the existing 401/403 authentication
handling and fallback error message behavior.
In `@autogpt_platform/frontend/src/hooks/useCredentials.ts`:
- Around line 30-36: Update the MCP credential matching logic in the credentials
hook to normalize both c.host and discriminatorValue before comparison, removing
trailing slashes so equivalent URLs match. Preserve the existing null guard and
credential collection behavior, and add a test covering a discriminator URL with
a trailing slash matching the normalized saved URL.
---
Nitpick comments:
In
`@autogpt_platform/frontend/src/app/`(platform)/build/components/MCPToolDialog.tsx:
- Around line 340-345: Remove the dark: Tailwind classes from the manual token
toggle button and the related controls near it, including dark:text-gray-400 and
dark:hover:text-gray-300. Preserve the existing semantic text, underline, hover,
and layout styling while relying on the design system for dark-mode behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 46c793da-9a65-4d8b-886d-4c79fa2b85f4
📒 Files selected for processing (14)
autogpt_platform/backend/backend/api/features/integrations/router.pyautogpt_platform/backend/backend/api/features/mcp/routes.pyautogpt_platform/backend/backend/api/features/mcp/test_routes.pyautogpt_platform/backend/backend/blocks/mcp/block.pyautogpt_platform/backend/backend/blocks/mcp/helpers.pyautogpt_platform/backend/backend/blocks/mcp/test_helpers.pyautogpt_platform/backend/backend/blocks/mcp/test_mcp.pyautogpt_platform/backend/backend/copilot/tools/run_mcp_tool.pyautogpt_platform/backend/backend/copilot/tools/utils.pyautogpt_platform/backend/backend/copilot/tools/utils_test.pyautogpt_platform/frontend/src/app/(platform)/build/components/MCPToolDialog.tsxautogpt_platform/frontend/src/app/(platform)/build/components/__tests__/MCPToolDialog.test.tsxautogpt_platform/frontend/src/hooks/__tests__/classifyCredentials.test.tsautogpt_platform/frontend/src/hooks/useCredentials.ts
Codecov Report❌ Patch coverage is ❌ Your patch check has failed because the patch coverage (66.66%) is below the target coverage (70.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## dev #13683 +/- ##
==========================================
+ Coverage 76.64% 76.73% +0.08%
==========================================
Files 2715 2715
Lines 207923 208100 +177
Branches 19947 19953 +6
==========================================
+ Hits 159370 159680 +310
+ Misses 44153 43996 -157
- Partials 4400 4424 +24
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
…r msg - CredentialsGroupedView matchesDiscriminatorValues now filters api_key MCP credentials by host (like classifyCredentials); previously an api_key cred for one server matched every server, risking wrong auto-assignment + 401. - auto_lookup ranking prefers non-expiring credentials so a valid static bearer token is not shadowed by a stale row that once had an expiry. - MCPToolDialog shows a clear "saving your API token failed" message on a non-2xx /token response instead of throwing the raw body.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@autogpt_platform/frontend/src/components/contextual/CredentialsInput/components/CredentialsGroupedView/__tests__/helpers.test.ts`:
- Around line 8-10: Rename the apiKeyCred helper to APIKeyCred and update every
call site in helpers.test.ts to use the capitalized acronym, preserving its
implementation and behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a03ee85b-0483-4cfe-b964-1bf022b87663
📒 Files selected for processing (5)
autogpt_platform/backend/backend/blocks/mcp/helpers.pyautogpt_platform/frontend/src/app/(platform)/build/components/MCPToolDialog.tsxautogpt_platform/frontend/src/app/(platform)/build/components/__tests__/MCPToolDialog.test.tsxautogpt_platform/frontend/src/components/contextual/CredentialsInput/components/CredentialsGroupedView/__tests__/helpers.test.tsautogpt_platform/frontend/src/components/contextual/CredentialsInput/components/CredentialsGroupedView/helpers.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- autogpt_platform/frontend/src/app/(platform)/build/components/tests/MCPToolDialog.test.tsx
- autogpt_platform/backend/backend/blocks/mcp/helpers.py
- autogpt_platform/frontend/src/app/(platform)/build/components/MCPToolDialog.tsx
📜 Review details
⏰ Context from checks skipped due to timeout. (18)
- GitHub Check: integration_test
- GitHub Check: lint
- GitHub Check: check API types
- GitHub Check: Seer Code Review
- GitHub Check: lint
- GitHub Check: types
- GitHub Check: test (3.12)
- GitHub Check: type-check (3.13)
- GitHub Check: test (3.13)
- GitHub Check: type-check (3.12)
- GitHub Check: type-check (3.11)
- GitHub Check: test (3.11)
- GitHub Check: lint
- GitHub Check: end-to-end tests
- GitHub Check: Analyze (python)
- GitHub Check: Check PR Status
- GitHub Check: check-docs-sync
- GitHub Check: Analyze (typescript)
🧰 Additional context used
📓 Path-based instructions (13)
autogpt_platform/frontend/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
autogpt_platform/frontend/**/*.{ts,tsx,js,jsx}: Use Node.js 21+ with pnpm package manager for frontend development
Always run 'pnpm format' for formatting and linting code in frontend developmentFormat frontend code using
pnpm format
autogpt_platform/frontend/**/*.{ts,tsx,js,jsx}: Fully capitalize acronyms in symbols, e.g.graphID,useBackendAPI
No linter suppressors (//@ts-ignore``,// eslint-disable) — fix the actual issue
Files:
autogpt_platform/frontend/src/components/contextual/CredentialsInput/components/CredentialsGroupedView/__tests__/helpers.test.tsautogpt_platform/frontend/src/components/contextual/CredentialsInput/components/CredentialsGroupedView/helpers.ts
autogpt_platform/frontend/**/*.{tsx,ts}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
autogpt_platform/frontend/**/*.{tsx,ts}: Use function declarations for components and handlers (not arrow functions) in React components
Only use arrow functions for small inline lambdas (map, filter, etc.) in React components
Use PascalCase for component names and camelCase with 'use' prefix for hook names in React
Use Tailwind CSS utilities only for styling in frontend components
Use design system components from 'src/components/' (atoms, molecules, organisms) in frontend development
Never use 'src/components/legacy/' in frontend code
Only use Phosphor Icons (@phosphor-icons/react) for icons in frontend components
Use generated API hooks from '@/app/api/__generated__/endpoints/' instead of deprecated 'BackendAPI' or 'src/lib/autogpt-server-api/'
Use React Query for server state (via generated hooks) in frontend development
Default to client components ('use client') in Next.js; only use server components for SEO or extreme TTFB needs
Use '' component for rendering errors in frontend UI; use toast notifications for mutation errors; use 'Sentry.captureException()' for manual exceptions
Separate render logic from data/behavior in React components; keep comments minimal (code should be self-documenting)
Files:
autogpt_platform/frontend/src/components/contextual/CredentialsInput/components/CredentialsGroupedView/__tests__/helpers.test.tsautogpt_platform/frontend/src/components/contextual/CredentialsInput/components/CredentialsGroupedView/helpers.ts
autogpt_platform/frontend/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
autogpt_platform/frontend/**/*.{ts,tsx}: No barrel files or 'index.ts' re-exports in frontend code
Regenerate API hooks with 'pnpm generate:api' after backend OpenAPI spec changes in frontend development
autogpt_platform/frontend/**/*.{ts,tsx}: Use function declarations (not arrow functions) for components/handlers
Noanytypes unless the value genuinely can be anything
Keep render functions and hooks under ~50 lines; extract named helpers or sub-components when they grow longer
Files:
autogpt_platform/frontend/src/components/contextual/CredentialsInput/components/CredentialsGroupedView/__tests__/helpers.test.tsautogpt_platform/frontend/src/components/contextual/CredentialsInput/components/CredentialsGroupedView/helpers.ts
autogpt_platform/frontend/src/components/**/*.{tsx,ts}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Structure React components as: ComponentName/ComponentName.tsx + useComponentName.ts + helpers.ts (exception: small 3-4 line components can be inline; render-only components can be direct files)
Files:
autogpt_platform/frontend/src/components/contextual/CredentialsInput/components/CredentialsGroupedView/__tests__/helpers.test.tsautogpt_platform/frontend/src/components/contextual/CredentialsInput/components/CredentialsGroupedView/helpers.ts
autogpt_platform/frontend/src/components/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Structure components as
ComponentName/ComponentName.tsx+useComponentName.ts+helpers.ts, use design system components fromsrc/components/(atoms, molecules, organisms), and never usesrc/components/__legacy__/*
Files:
autogpt_platform/frontend/src/components/contextual/CredentialsInput/components/CredentialsGroupedView/__tests__/helpers.test.tsautogpt_platform/frontend/src/components/contextual/CredentialsInput/components/CredentialsGroupedView/helpers.ts
autogpt_platform/frontend/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
autogpt_platform/frontend/src/**/*.{ts,tsx}: Use generated API hooks from@/app/api/__generated__/endpoints/following the patternuse{Method}{Version}{OperationName}, and regenerate withpnpm generate:api
Separate render logic from business logic using component.tsx + useComponent.ts + helpers.ts pattern, colocate state when possible and avoid creating large components, use sub-components in local/componentsfolder
Use function declarations for components and handlers, use arrow functions only for callbacks
Do not useuseCallbackoruseMemounless asked to optimise a given function
autogpt_platform/frontend/src/**/*.{ts,tsx}: Keep files under ~200 lines; extract sub-components or hooks into their own files when a file grows beyond this
Use generated API hooks from@/app/api/__generated__/endpoints/with patternuse{Method}{Version}{OperationName}
Always import the-Icon-suffixed alias from@phosphor-icons/react(e.g.TrashIcon,PlusIcon,SquareIcon) — bare exports are deprecated
Do not useuseCallbackoruseMemounless asked to optimize a given function
Never usesrc/components/__legacy__/*— use design system components fromsrc/components/
Files:
autogpt_platform/frontend/src/components/contextual/CredentialsInput/components/CredentialsGroupedView/__tests__/helpers.test.tsautogpt_platform/frontend/src/components/contextual/CredentialsInput/components/CredentialsGroupedView/helpers.ts
autogpt_platform/frontend/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
No barrel files or
index.tsre-exports in the frontend
Files:
autogpt_platform/frontend/src/components/contextual/CredentialsInput/components/CredentialsGroupedView/__tests__/helpers.test.tsautogpt_platform/frontend/src/components/contextual/CredentialsInput/components/CredentialsGroupedView/helpers.ts
autogpt_platform/frontend/src/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
Do not type hook returns, let Typescript infer as much as possible
autogpt_platform/frontend/src/**/*.ts: Extract component logic into custom hooks grouped by concern, not by component, with each hook in its own.tsfile
Do not type hook returns; let TypeScript infer as much as possible
Files:
autogpt_platform/frontend/src/components/contextual/CredentialsInput/components/CredentialsGroupedView/__tests__/helpers.test.tsautogpt_platform/frontend/src/components/contextual/CredentialsInput/components/CredentialsGroupedView/helpers.ts
autogpt_platform/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Never type with
any, if no types available useunknown
Files:
autogpt_platform/frontend/src/components/contextual/CredentialsInput/components/CredentialsGroupedView/__tests__/helpers.test.tsautogpt_platform/frontend/src/components/contextual/CredentialsInput/components/CredentialsGroupedView/helpers.ts
autogpt_platform/frontend/**/*.{test,spec}.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
autogpt_platform/frontend/**/*.{test,spec}.{ts,tsx}: Use Vitest + RTL + MSW for integration tests as the primary testing approach (~90%, page-level), use Playwright for E2E critical flows, and use Storybook for design system components
Run frontend integration tests withpnpm test:unit(Vitest + RTL + MSW)
Files:
autogpt_platform/frontend/src/components/contextual/CredentialsInput/components/CredentialsGroupedView/__tests__/helpers.test.ts
autogpt_platform/frontend/src/**/components/**/*.{ts,tsx}
📄 CodeRabbit inference engine (autogpt_platform/frontend/AGENTS.md)
Structure components as
ComponentName/ComponentName.tsx+useComponentName.ts+helpers.ts
Files:
autogpt_platform/frontend/src/components/contextual/CredentialsInput/components/CredentialsGroupedView/__tests__/helpers.test.tsautogpt_platform/frontend/src/components/contextual/CredentialsInput/components/CredentialsGroupedView/helpers.ts
autogpt_platform/frontend/src/**/__tests__/**/*.{test,spec}.{ts,tsx}
📄 CodeRabbit inference engine (autogpt_platform/frontend/AGENTS.md)
Use Orval-generated MSW handlers from
@/app/api/__generated__/endpoints/{tag}/{tag}.msw.tsfor API mocking
Files:
autogpt_platform/frontend/src/components/contextual/CredentialsInput/components/CredentialsGroupedView/__tests__/helpers.test.ts
autogpt_platform/frontend/src/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (autogpt_platform/frontend/AGENTS.md)
Avoid index and barrel files
Files:
autogpt_platform/frontend/src/components/contextual/CredentialsInput/components/CredentialsGroupedView/__tests__/helpers.test.tsautogpt_platform/frontend/src/components/contextual/CredentialsInput/components/CredentialsGroupedView/helpers.ts
🧠 Learnings (4)
📚 Learning: 2026-04-01T18:54:16.035Z
Learnt from: Bentlybro
Repo: Significant-Gravitas/AutoGPT PR: 12633
File: autogpt_platform/frontend/src/app/(platform)/library/components/AgentFilterMenu/AgentFilterMenu.tsx:3-10
Timestamp: 2026-04-01T18:54:16.035Z
Learning: In the frontend, the legacy Select component at `@/components/__legacy__/ui/select` is an intentional, codebase-wide visual-consistency pattern. During code reviews, do not flag or block PRs merely for continuing to use this legacy Select. If a migration to the newer design-system Select is desired, bundle it into a single dedicated cleanup/migration PR that updates all Select usages together (e.g., avoid piecemeal replacements).
Applied to files:
autogpt_platform/frontend/src/components/contextual/CredentialsInput/components/CredentialsGroupedView/__tests__/helpers.test.tsautogpt_platform/frontend/src/components/contextual/CredentialsInput/components/CredentialsGroupedView/helpers.ts
📚 Learning: 2026-04-07T09:24:16.582Z
Learnt from: 0ubbe
Repo: Significant-Gravitas/AutoGPT PR: 12686
File: autogpt_platform/frontend/src/app/(no-navbar)/onboarding/steps/__tests__/PainPointsStep.test.tsx:1-19
Timestamp: 2026-04-07T09:24:16.582Z
Learning: In Significant-Gravitas/AutoGPT’s `autogpt_platform/frontend` (Vite + `vitejs/plugin-react` with the automatic JSX transform), do not flag usages of React types/components (e.g., `React.ReactNode`) in `.ts`/`.tsx` files as missing `React` imports. Since the React namespace is made available by the project’s TS/Vite setup, an explicit `import React from 'react'` or `import type { ReactNode } ...` is not required; only treat it as missing if typechecking (e.g., `pnpm types`) would actually fail.
Applied to files:
autogpt_platform/frontend/src/components/contextual/CredentialsInput/components/CredentialsGroupedView/__tests__/helpers.test.tsautogpt_platform/frontend/src/components/contextual/CredentialsInput/components/CredentialsGroupedView/helpers.ts
📚 Learning: 2026-04-02T05:43:49.128Z
Learnt from: 0ubbe
Repo: Significant-Gravitas/AutoGPT PR: 12640
File: autogpt_platform/frontend/src/app/(no-navbar)/onboarding/steps/WelcomeStep.tsx:13-13
Timestamp: 2026-04-02T05:43:49.128Z
Learning: Do not flag `import { Question } from "phosphor-icons/react"` as an invalid import. `Question` is a valid named export from `phosphor-icons/react` (as reflected in the package’s generated `.d.ts` files and re-exports via `dist/index.d.ts`), so it should be treated as a supported named export during code reviews.
Applied to files:
autogpt_platform/frontend/src/components/contextual/CredentialsInput/components/CredentialsGroupedView/__tests__/helpers.test.tsautogpt_platform/frontend/src/components/contextual/CredentialsInput/components/CredentialsGroupedView/helpers.ts
📚 Learning: 2026-04-20T20:07:22.981Z
Learnt from: ntindle
Repo: Significant-Gravitas/AutoGPT PR: 11235
File: autogpt_platform/frontend/src/app/(platform)/admin/diagnostics/__tests__/ExecutionsTable.test.tsx:27-76
Timestamp: 2026-04-20T20:07:22.981Z
Learning: In this codebase, Orval-generated API modules under `src/app/api/__generated__/` are not committed to git and must be generated via `pnpm generate:api` (requires a running backend). In integration tests, it’s acceptable—and expected—to stub generated hooks/modules by mocking them with `vi.mock("`@/app/api/__generated__/endpoints/`{tag}/{tag}")`. Do not treat `vi.mock` of these generated hook modules as a violation of the MSW handler guideline, since the corresponding MSW handlers cannot be imported at test time when generated files are absent.
Applied to files:
autogpt_platform/frontend/src/components/contextual/CredentialsInput/components/CredentialsGroupedView/__tests__/helpers.test.ts
🔇 Additional comments (2)
autogpt_platform/frontend/src/components/contextual/CredentialsInput/components/CredentialsGroupedView/__tests__/helpers.test.ts (1)
1-6: LGTM!Also applies to: 12-47
autogpt_platform/frontend/src/components/contextual/CredentialsInput/components/CredentialsGroupedView/helpers.ts (1)
31-37: LGTM!
There was a problem hiding this comment.
📋 Automated Review — PR #13683
PR #13683 — feat(platform): support static API-key/bearer-token auth for MCP servers
Author: Abhi1992002 | Files: 14
🎯 Verdict: REQUEST_CHANGES
PR Description Quality
✅ Has Why (placed MCP blocks ran with no credential → 401) + What (first-class api_key/bearer-token credential) + How (proactive dialog entry → /mcp/token persist → attach to node). One gap: the end-to-end manual test in the PR checklist is left unchecked — the exact "no 401 at runtime" journey the PR exists to fix is unconfirmed by the author.
What This PR Does
Previously, static bearer/API-key tokens for MCP servers were shoehorned into the OAuth2 credential model and effectively discarded, so a placed MCP block ran with no usable credential and failed with a 401. This PR makes static tokens a first-class APIKeyCredentials type: the builder dialog now surfaces "Use an API key / bearer token instead" up front, persists the token via /api/v2/mcp/token on successful discovery, and attaches the returned credential to the node. Shared helpers (mcp_auth_token, is_mcp_credential_for_server) centralize token extraction and cross-server matching, with dual-direction cleanup preserving backward compatibility for legacy OAuth2-masqueraded tokens.
Specialist Findings
🛡️ Security ✅ — Traced every credential path: SSRF validation (validate_url_host) runs before any lookup/outbound call, both endpoints require get_user_id, to_meta_response never serializes api_key/access_token, and cross-server scoping compares the full normalized URL (verified by ...rejects_other_server). Two low secret-hygiene notes only.
🟡 normalize_mcp_url isn't case-insensitive → stale token can survive cleanup (helpers.py:29).
🏗️ Architecture ✅ — Storing static tokens as APIKeyCredentials instead of masquerading as OAuth2 is the correct model fix; helper extraction is DRY and introduces no circular deps. Sound migration-free backward-compat strategy.
🟠 Mixed-type selection ranks by raw expiry magnitude, so an expired OAuth2 row (timestamp > 0) can outrank a valid non-expiring api_key (expiry 0) for the same server (helpers.py:172).
⚡ Performance ✅ — No regressions. New code is O(n) in a user's MCP credential count (single/low-double digits realistically). Sequential per-credential delete loop (routes.py:470) and per-execution full-credential scan (helpers.py:161) are pre-existing and bounded.
🧪 Testing mockOAuthLogin not-called checks — not slop). But the riskiest new logic — the api_key-vs-oauth2 branching inside auto_lookup_mcp_credential (OAuth-only refresh guard + expiry selection) — is never directly tested because the function is mocked in every caller.
🟠 No unit test pins the refresh guard or mixed-type "best" selection (helpers.py:173,176).
📖 Quality ✅ — Readability A. Accurate docstrings, good naming, duplication removed (hand-rolled CredentialsMetaResponse → to_meta_response). Minor: discovery and token-persist share one try/catch, so a persist failure is misattributed as an invalid-token error (MCPToolDialog.tsx:121).
📦 Product type="button", no role="alert" on the error).
📬 Discussion api_key host-classification in useCredentials.ts but missed the sibling classifier CredentialsGroupedView/helpers.ts:32, which still only filters oauth2 MCP creds. A CodeRabbit Major (TOCTOU on credential replace) is also unanswered.
🔴 Second classifier not updated for api_key (CredentialsGroupedView/helpers.ts:32).
🔎 QA ✅ — Verified live: /mcp/token persists a first-class api_key credential (type:"api_key", host:"https://api.github.com/mcp"), replace/cleanup leaves exactly one row, the builder shows the new proactive API-key entry, and negatives return 401/422/400 (incl. SSRF block of 169.254.169.254). Could not reach a real bearer-auth MCP server (no sandbox DNS), so live block runtime and persist-on-200-discovery weren't exercised — both covered by added unit tests.
🔴 Blockers
api_keyMCP creds match every server in the grouped credential picker (autogpt_platform/frontend/src/components/contextual/CredentialsInput/components/CredentialsGroupedView/helpers.ts:32) —matchesDiscriminatorValuesonly host-filters MCP credentials whentype === "oauth2"; anapi_keyMCP credential falls through toreturn true, matching any server URL. This is the exact wrong-credential/401 bug this PR fixed inuseCredentials.ts, left in place in the sibling classifier. Reachable path: a user with twoapi_keyMCP creds for different servers is offered the wrong one when configuring a block. Since this PR introduced theapi_keyMCP credential type, closing this parity gap belongs in this PR. (Flagged by: discussion — Sentry HIGH)
🟠 Should Fix
- Add a direct unit test for
auto_lookup_mcp_credentialbranching (autogpt_platform/backend/backend/blocks/mcp/helpers.py:173,176) — the OAuth-only refresh guard and mixed-type "best" selection are this PR's backward-compat promise and are only ever exercised through mocks. Pin them intest_helpers.py(the cred factory fixtures already exist). (Flagged by: testing) - Validity-aware credential ranking (
autogpt_platform/backend/backend/blocks/mcp/helpers.py:172) — rank not-expired before expired instead of by raw expiry magnitude, so an expired OAuth2 row can't shadow a valid non-expiring api_key when cleanup leaves two rows. (Flagged by: architect, discussion — 2 specialists) - Clear stale error on auth-mode toggle (
autogpt_platform/frontend/src/app/(platform)/build/components/MCPToolDialog.tsx:340,355) — both toggles shouldsetError(null)so a prior "Authentication failed" message doesn't linger over the other flow. (Flagged by: product) - Separate the token-persist try/catch from discovery (
autogpt_platform/frontend/src/app/(platform)/build/components/MCPToolDialog.tsx:121) — a persistence failure is currently reported as an invalid-token error even though discovery already validated the token. (Flagged by: quality, product — 2 specialists)
🟡 Nice to Have
- Persist token on block-confirm rather than on discovery (
MCPToolDialog.tsx:124) — avoids orphaned credentials if the user cancels after discovery. Encrypted + user-scoped + cleaned on re-store, so low risk. (Flagged by: security, performance, product — 3 specialists) - Case-insensitive
normalize_mcp_url(backend/blocks/mcp/helpers.py:29) — lowercase scheme+host (keep path/query) so differently-cased URLs don't leave a stale live token behind. (Flagged by: security) - Batch the cleanup deletes (
backend/api/features/mcp/routes.py:470) —asyncio.gatheroverold_cred_idsinstead of sequential awaits. Bounded and pre-existing. (Flagged by: performance)
🔵 Nits
catch (e: any)(MCPToolDialog.tsx:143) — violates the "neverany" guideline; pre-existing context line, cheap cleanup while here.- Add
type="button"androle="alert"(MCPToolDialog.tsx:340,355,377) — consistency withMCPToolCardand screen-reader announcement of auth failures.
QA Screenshots
Human Review Needed
YES — This change alters how credentials/secrets are stored and how a credential is matched to a server (the security/trust boundary), and the Blocker involves a credential-to-server matching gap; a maintainer should confirm the classifier parity fix before merge.
Risk Assessment
Merge risk: MEDIUM | Rollback: EASY (additive credential type + isolated frontend dialog/classifier changes; no schema migration).
CI Status
GitHub CI (per PR discussion): lint, types, CodeQL, e2e, integration green; test (3.11/3.12/3.13) still running at review time — live status UNVERIFIED from this harness.
Local harness: ✅ frontend lint, ✅ frontend typecheck, ✅ frontend build; ❌ backend poetry run lint and ❌ frontend test:unit failed locally — GitHub reported the lint suite green on this head, so the backend-lint failure is treated as environment skew, not a code defect. The local test:unit failure could not be reconciled against a confirmed green GitHub run and should be checked against live CI before merge.
UI Testing — Variant Results
✅ local: MCP static API-key/bearer-token auth works end-to-end: /token persists a first-class api_key credential with correct host shape, replace/cleanup works, the builder shows the new proactive API-key entry, and negative cases return 401/422/400 correctly.
✅ hosted: MCP static bearer-token auth works end-to-end: /mcp/token persists api_key credentials with correct normalization/cleanup, and the builder dialog's proactive token entry and invalid-token error path behave correctly; all negative tests pass.
…a11y on MCP dialog - Add direct unit tests for auto_lookup_mcp_credential: mixed-type "best" selection (non-expiring api_key beats stale OAuth), OAuth-only refresh guard, and no-match → None. Previously only exercised via mocks. - MCP dialog: clear any stale error when toggling between token and OAuth entry, add type="button" to the toggles, and role="alert" on the error.
|
Thanks — addressing the review. Most items were already fixed in the same commit the review ran against ( 🔴 Blocker — 🟠 Should-fix 1 — direct test for 🟠 Should-fix 2 — validity-aware ranking: fixed in 🟠 Should-fix 3 — clear stale error on auth-mode toggle: fixed. Both toggles now 🟠 Should-fix 4 — persist-failure misattributed as invalid-token: fixed in 🔵 Nits — a11y: added Deferred (with rationale, noted in the CodeRabbit thread): the TOCTOU/atomic credential-replace is pre-existing (create-before-delete already avoids data loss; duplicates are tolerated by |
|
/review |
There was a problem hiding this comment.
📋 Automated Review — PR #13683
PR #13683 — feat(platform): support static API-key/bearer-token auth for MCP servers
Author: Abhi1992002 | Files: 16
🎯 Verdict: APPROVE
PR Description Quality
✅ Has Why + What + How — the description explains the 401-at-execution bug (builder discarded manually-entered tokens), the fix (persist as a first-class api_key credential), and the backend/frontend mechanics. One gap: the PR checklist's manual end-to-end item is left unchecked, though QA has since exercised that exact flow (see below). Worth noting the /token response contract change (type now api_key, scopes now null) in the description for downstream awareness.
What This PR Does
Previously, when a user pasted a static API key / bearer token for an MCP server in the builder, the token was thrown away and the placed block ran with no credential → 401 at execution. This PR makes the manual token a first-class APIKeyCredentials (replacing an OAuth2Credentials-as-bearer-token masquerade), persists it via POST /mcp/token, and auto-attaches it to the node. Shared helpers (mcp_auth_token, is_mcp_credential_for_server) centralize token extraction and server-matching for both credential shapes, and legacy OAuth2-masqueraded rows keep working with no DB migration.
Specialist Findings
🛡️ Security ✅ — SSRF guards (validate_url_host) on /discover-tools, /oauth/login, and /token including metadata-derived URLs; credentials strictly user-scoped and exact-host matched; tokens stored as SecretStr and not logged. Overall risk LOW.
🟡 Stale/revoked static token can outrank a fresh OAuth token if best-effort cleanup fails (helpers.py:64) — fail-degraded, not a privilege issue.
🏗️ Architecture ✅ — Replacing the OAuth2-masquerade hack with a real APIKeyCredentials type is genuine debt reduction; three duplicated match blocks collapse to one TypeGuard predicate; backward compat achieved purely read-side with no migration.
🟡 mcp/routes.py imports to_meta_response from sibling integrations/router.py (routes.py:16) — feature-to-feature coupling; consider relocating to a neutral module.
⚡ Performance ✅ — No new DB round trips on the runtime path; auto_lookup_mcp_credential stays single get_creds_by_provider call + O(n) in-memory filter over one user's (small, bounded) credential set. The extra /token POST is confined to the one-time interactive builder flow.
🧪 Testing ✅ — Strong (~90%), with meaningful negative cases (wrong token → no OAuth bounce, public server → no attach, /token 500 → no block, wrong-server cred untouched) and assertions on the actual token source (api_key vs access_token). The prior "OAuth2-only refresh guard untested" finding is ✅ Addressed — test_helpers.py now asserts it via assert_not_called().
🟠 Documented "most-recently-created wins" tiebreaker for multiple non-expiring creds (helpers.py:189) is uncovered; switching >= to > would regress silently.
📖 Quality ✅ — Readability grade A; descriptive names, comments explain why (ranking rationale, backward-compat intent), to_meta_response dedup is clean. Verified APIKeyCredentials.expires_at exists (model.py:361) so _mcp_credential_rank is safe.
🔵 MCP-credential predicate duplicated in useCredentials.ts:32 and CredentialsGroupedView/helpers.ts:35.
📦 Product ✅ — Feature matches its description, backward compatible, well tested. Terminology consistent, accessibility solid (role="alert" on errors, keyboard submit, real <button> toggles).
🟠 A /token persist failure after successful discovery throws away the discovered tools and strands the user on the URL step (MCPToolDialog.tsx:129) — a valid, working token becomes unusable on a transient save error.
📬 Discussion 28d2394 with regression test). Two items need a glance before merge: the CHANGES_REQUESTED bot decision is stale (predates 4 fix commits; re-review was queued but never posted), and CodeRabbit's TOCTOU concern was thread-resolved without an evident code change.
🟠 Non-atomic read→create→delete of MCP credentials (routes.py:443) — concurrent submits could leave duplicate tokens; confirm fixed or acknowledge as won't-fix.
🔎 QA ✅ — Full end-to-end verification against the running stack. Token stored as {"type":"api_key",...} (not OAuth2 masquerade), trailing-slash normalized, re-store cleanup deletes old cred while leaving other servers untouched, and the builder dialog persists + auto-attaches the credential to the placed block (MCP: mcp.deepwiki.com (API Key) selected). Negative cases all hold: no-auth → 401, blank → 422, missing URL → 422, private IP → 400 (SSRF blocked).
🟠 Should Fix
- Persist failure discards a successful discovery (
MCPToolDialog.tsx:129) — On a non-200 fromPOST /mcp/token, the code throws beforesetStep("tool"), dropping the already-fetched tool list even though the token is valid. Keep the discovered tools visible and retry just the save, or let the user proceed and retry persistence. (Flagged by: product, security — 2 specialists) - Tiebreaker for multiple non-expiring credentials untested (
helpers.py:189) — The docstring calls the "most-recently-created wins">=behavior load-bearing (for the failed-cleanup / duplicate-token case), but no test asserts it. Add anauto_lookup_mcp_credentialtest with two non-expiringapi_keycreds for the same server. (Flagged by: testing) - Confirm MCP credential replacement race (
routes.py:443) — CodeRabbit's TOCTOU finding (non-atomic read→create→delete; concurrent submits can leave duplicate tokens) was thread-resolved with no evident code change. Confirm it was fixed, make the replace atomic, or acknowledge as won't-fix with rationale. (Flagged by: discussion)
🟡 Nice to Have
- Ranking tiebreaker on recency/validity (
helpers.py:64) — a revoked non-expiring static token can outrank a live OAuth token if best-effort cleanup fails; add a recency tiebreaker or document that single-cred-per-server cleanup is the safety invariant. (security, architect) - Relocate
to_meta_responseto a neutral credentials module to avoid feature-to-feature router coupling (routes.py:16). (architect) - Route-level test for the
api_keydiscover-tools path (routes.py) — currently only the OAuth2 branch is exercised at the route level. (testing) - Defer credential persistence until block is added (
MCPToolDialog.tsx:124) — token is stored server-side on discovery, before commit; closing the dialog mid-flow leaves an orphaned credential (self-heals on next connect). (security, product)
🔵 Nits
- Reword persist-failure copy (
MCPToolDialog.tsx:130) — "Connected, but saving your API token failed" says Connected when nothing was added; prefer "We reached the server, but couldn't save your token." (product) catch (e: any)(MCPToolDialog.tsx:143) — violates the repo's no-anyguideline; useunknownand narrow. Pre-existing, untouched by this PR. (discussion, quality)- Shared
isMcpCredentialhelper — dedupe the MCP predicate acrossuseCredentials.ts:32andCredentialsGroupedView/helpers.ts:35. (quality)
QA Screenshots
Human Review Needed
YES — This PR changes how MCP credentials/secrets are stored and handled (new api_key credential type, token persistence, cleanup semantics), which sits on the credential-storage boundary per the review policy. The security specialist and QA both cleared it, so this is a confirmation ask rather than an unresolved concern.
Risk Assessment
Merge risk: LOW | Rollback: EASY — read-side matching change, no DB migration; reverting restores prior behavior cleanly.
CI Status
Local harness: ✅ frontend lint, ✅ backend lint, ✅ frontend typecheck, ✅ frontend build. test:unit failed in the sandbox (459s) — this suite runs green on the repo's GitHub CI (all codecov flags reported green per the discussion review), so the local failure is treated as environment skew, not a defect, per policy.
GitHub CI: PARTIALLY VERIFIED via discussion review — ~40/44 checks green (lint, types, e2e, CodeQL, Snyk, codecov); backend test (3.11/3.12/3.13) and integration_test were still pending at review time and the size label check fails (size/xl, non-functional). Confirm the backend matrix goes green before merge.
UI Testing — Variant Results
✅ local: End-to-end verification confirms MCP static API-key/bearer-token auth works: the builder dialog persists the token as a first-class api_key credential and attaches it to the placed block, cleanup/host-matching/negative guards all hold.
✅ hosted: MCP static bearer-token auth works end-to-end: /token persists a first-class api_key credential with correct shape and per-server cleanup, negative/SSRF guards hold, and the builder dialog exposes the new proactive token flow correctly.
Superseded by a newer automated review.
- Add tests: discover-tools with a stored api_key credential, auto_lookup recency tiebreaker among equal-rank creds, and TypeGuard rejection of a non-OAuth2/non-APIKey (host-scoped) MCP credential. - MCPToolDialog: type the discovery catch as unknown (drop `any`) and reword the token-persist-failure message so it no longer says "Connected".
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
!deploy |
|
🚀 Deploying PR #13683 to development environment... |
|
✅ Preview environment is live (all services healthy)
Push more commits, then comment |
Five defects made an MCP block unusable end to end. Frontend: - MCPToolCard rendered a "Show details" <button> inside the card's own <button>, which is invalid HTML and threw a hydration error on the tool discovery step. The card is now a div with role="button" and keyboard handling. - Adding an MCP block stamped `credentials_optional: true` on the node. That flag means "skip this node if credentials are missing" to the executor, so a graph with only an MCP block completed instantly with zero nodes run. The block already declares a schema default for `credentials`, so the flag was redundant as well as harmful. CredentialField now only lets the toggle relax a credential that the schema actually requires. - The credentials picker compared the node's raw `server_url` against the normalized URL the backend stores MCP credentials under, so the saved credential never appeared. Added `normalizeMCPUrl` (mirror of the backend's `normalize_mcp_url`), used both when matching and when the dialog emits the node's `server_url`. - Creating an API key from the picker never tagged it with `metadata.mcp_server_url`, so it came back with `host: null`, was filtered out of the list, and the selection made on create was immediately cleared again. Backend: - MCPToolBlock.run fell back to looking up a stored credential by server URL whenever none was injected. The executor nulls the field both when the user picks "None (skip this credential)" and when nothing was ever configured, so the fallback silently overrode an explicit choice. It also masked the picker bug above. Removed from the block; the copilot and discovery callers keep their own lookups, where no user selection exists to override.
|
!undeploy |
|
🗑️ Undeploying PR #13683 from development environment... |
|
🧹 Preview Environment Cleaned Up All resources for PR #13683 have been removed:
Cleanup completed successfully. |
| # (skip this credential)" or the server needs no auth. Silently | ||
| # substituting a stored token would override that explicit choice — | ||
| # the two cases are indistinguishable by the time they reach `run`. | ||
| auth_token = mcp_auth_token(credentials) if credentials else None |
There was a problem hiding this comment.
🤖 🔴 Blocker: Dropping the auto_lookup_mcp_credential fallback silently breaks MCP nodes that don't carry an explicit credential ID. credentials has schema default {}, and executor/manager.py:282-300 turns an empty or stale value into input_data["credentials"] = None and runs the block anyway (ValueError → "running without (field has default)"). So credentials is None in run() is not the same as "user chose None" — it is also "never set" and "ID points at a deleted row".
Failure: a user reconnects an existing server via /mcp/token or the OAuth callback → both delete the old credential and create a replacement with a new ID, while the placed node still references the deleted one → every subsequent run sends no Authorization header and fails with 401. That is exactly the bug this PR sets out to fix. Same for graphs saved before the picker populated credentials; there is no backfill of node.input_default["credentials"].
(flagged by: Codex + Claude)
| // persist it so the block can authenticate at runtime — discovery only | ||
| // uses the token transiently and would otherwise be discarded, leaving | ||
| // the placed block with no credentials and failing on the next run. | ||
| if (authToken) { |
There was a problem hiding this comment.
🤖 🔴 Blocker: One server's secret can be sent to — and stored for — a different server. On success the dialog calls setShowManualToken(false) (L145) but never clears manualToken, and handleDiscoverTools (L179) keeps passing it.
Failure: connect to server A with token T_A → "Back" (L418, resets only step/selectedTool) → type server B's URL. The token field is hidden and the button reads "Discover Tools", so nothing signals a token is still loaded — but authToken is truthy, so T_A goes out in B's discovery request and this new block persists it via /mcp/token as B's credential.
Clearing manualToken alongside setShowManualToken(false) (and on "Back") fixes it. (flagged by: Claude)
| throw new Error( | ||
| "Discovered the server, but saving your API token failed. Please try again.", | ||
| ); | ||
| setCredentials({ |
There was a problem hiding this comment.
🤖 🔴 Blocker: The stored credential never reaches CredentialsProvidersContext. The OAuth path deliberately goes through mcpProvider.mcpOAuthCallback — the code comments it as "updates cache" (L211) — but this path calls the generated endpoint directly, so nothing upserts the new credential and no query invalidation fires.
Failure: the backend's mcp_store_token deletes the previous credential for that server and returns a new ID. The node is set to the new ID, but useCredentialsInput still sees the old, now-deleted credential in the cached list and the new ID missing → it clears the selection and re-selects the deleted credential. The node is saved pointing at a nonexistent credential and 401s until a full page refresh.
(flagged by: Codex + Claude)
| isinstance(old, OAuth2Credentials) | ||
| and (old.metadata or {}).get("mcp_server_url") == meta["server_url"] | ||
| ): | ||
| if is_mcp_credential_for_server(old, normalize_mcp_url(meta["server_url"])): |
There was a problem hiding this comment.
🤖 🟠 Should Fix: In mcp_oauth_callback this cleanup loop runs before await creds_manager.create(...) (L385), and broadening it to api_key means it now destroys a working static bearer token before the replacement exists.
Failure: a user with a working API-key credential switches that server to OAuth; the exchange succeeds, the key is deleted, then create fails → the user is left with no credential at all and must re-obtain the token. mcp_store_token already gets this right (create first, delete old IDs after) — worth mirroring that ordering here.
Note this is distinct from the atomicity thread above: the "create-before-cleanup ordering avoids credential loss" reasoning holds for /token, but this path is delete-before-create. (flagged by: Codex)
| if isinstance(cred, APIKeyCredentials) | ||
| else cred.access_token_expires_at | ||
| ) | ||
| return expiry if expiry is not None else sys.maxsize |
There was a problem hiding this comment.
🤖 🟠 Should Fix: Follow-up on the existing sys.maxsize thread — the reason it was triaged low ("only if best-effort cleanup fails") no longer holds after this PR.
By making the block advertise api_key, MCP credentials can now also be created through the generic POST /{provider}/credentials (api/features/integrations/router.py:444-466) via the API-key modal — and that endpoint does no per-server cleanup at all. So an OAuth credential and a static key for the same server coexist through a completely normal user flow, not a failure path.
Failure: user OAuths into https://mcp.acme.com/mcp, later pastes a personal token through the block's API-key modal. auto_lookup_mcp_credential then permanently prefers the static key, so /discover-tools and CoPilot's run_mcp_tool authenticate with it even after the token is revoked — and reconnecting via OAuth cannot fix it. (flagged by: Claude)
| server_url = ( | ||
| credential.metadata.get("mcp_server_url") | ||
| if isinstance(credential, OAuth2Credentials) | ||
| if isinstance(credential, (OAuth2Credentials, APIKeyCredentials)) |
There was a problem hiding this comment.
🤖 🟡 Nice to Have: This predicate now covers api_key credentials but still compares the raw stored mcp_server_url against raw requirements.discriminator_values — no normalize_mcp_url, unlike the new is_mcp_credential_for_server and the frontend's classifyCredentials, which normalize both sides.
Failure: the node's server_url is https://mcp.acme.com/mcp/ (trailing slash) while the credential was stored normalized without it → match_user_credentials_to_graph reports the MCP credential as missing and CoPilot refuses to run an agent the user has already connected. (flagged by: Claude)
| // match by server URL — otherwise a token for one server would be treated | ||
| // as valid for every server. | ||
| if ( | ||
| (credential.type === "oauth2" || credential.type === "api_key") && |
There was a problem hiding this comment.
🤖 🟡 Nice to Have: matchesDiscriminatorValues compares discriminatorValues.includes(credential.host) verbatim (L40), while classifyCredentials (useCredentials.ts:39) now normalizes both sides with normalizeMCPUrl. The two MCP matchers disagree.
Failure: a node whose server_url carries a trailing slash matches in the builder picker but not in the grouped/run-dialog auto-assign, so the run dialog reports no credential for a server that has one. (flagged by: Claude)
| }, | ||
| }); | ||
|
|
||
| const mcpServerUrl = |
There was a problem hiding this comment.
🤖 🟡 Nice to Have: When the provider is mcp but discriminatorValue is empty (node's server_url not yet filled in), mcpServerUrl is "" and the metadata spread is silently dropped.
Failure: the modal creates an MCP credential with host: null, which can never be matched by the picker or by the backend's is_mcp_credential_for_server. The user sees the credential as selected, yet the block 401s at run time with no indication why. Worth blocking submission (or surfacing an error) instead of creating an unusable credential. (flagged by: Claude)
| title: string; | ||
| api_key: string; | ||
| expires_at?: number; | ||
| metadata?: Record<string, any>; |
There was a problem hiding this comment.
🤖 🟡 Nice to Have: Record<string, any> violates the repo rule "Never type with any, if not types available use unknown" (frontend/AGENTS.md). Record<string, unknown> compiles for both call sites, which only ever write { mcp_server_url: string }. (flagged by: Claude)
| async def test_run_with_api_key_credentials(self): | ||
| """Verify the block accepts a static API-key / bearer token and pulls | ||
| the token from ``api_key`` (not ``access_token``).""" | ||
| from pydantic import SecretStr |
There was a problem hiding this comment.
🤖 🟡 Nice to Have: Function-local imports violate backend/AGENTS.md ("Top-level imports only — no local/inner imports"), and these two are redundant: this same PR already added SecretStr and APIKeyCredentials at module scope (L9, L13). Same pattern in test_helpers.py:168 and copilot/tools/utils_test.py:37-40. (flagged by: Claude)









Why / What / How
Why: Some MCP servers (e.g. DataFast) authenticate with a static bearer token / API key issued in the vendor's own dashboard, not a full OAuth2 authorize/token exchange. The MCP tool block only advertised
oauth2credentials, and — critically — the graph builder's "Connect to MCP Server" dialog used a manually-entered token only for tool discovery and never persisted it. The placed block therefore ran with no credentials and failed with a 401 at execution time. (Linear: REQ-115)What: Make static API-key / bearer-token a first-class credential type for MCP server connections, entered via the builder's secure credential dialog, and stored as a proper
api_keycredential the block uses at runtime.How:
MCPToolBlocknow advertises bothoauth2andapi_keycredential types and accepts either at runtime — the bearer token is pulled fromaccess_token(OAuth2) orapi_key(API key) via a single sharedmcp_auth_token()helper.POST /api/v2/mcp/tokennow stores the token as a first-classAPIKeyCredentials(typeapi_key) instead of masquerading it as anOAuth2Credentials.get_hostnow match bothoauth2andapi_keyMCP credentials by server URL, so tokens already stored under the old (OAuth2-masquerade) shape keep working./mcp/token, and attaches the returned credential to the block node.api_keyMCP credentials by host, same as OAuth2.The copilot (
run_mcp_tool+MCPSetupCard) bearer-token flow already worked; this PR reuses the same/tokenendpoint and brings the builder to parity.Changes 🏗️
Backend
blocks/mcp/block.py— widenMCPCredentialstoLiteral["oauth2", "api_key"];run()acceptsOAuth2Credentials | APIKeyCredentials; token extracted viamcp_auth_token().blocks/mcp/helpers.py— addmcp_auth_token(),is_mcp_credential_for_server(),MCPCredentialunion;auto_lookup_mcp_credential()matches both credential types (OAuth2-onlyrefresh_if_neededstill guarded).api/features/mcp/routes.py—/tokenstoresAPIKeyCredentials; discovery token extraction and old-credential cleanup (both/tokenand OAuth callback) cover both types.api/features/integrations/router.py—get_host()returns the MCP server URL forAPIKeyCredentialstoo.copilot/tools/run_mcp_tool.py— token extraction via the shared helper.Frontend
build/components/MCPToolDialog.tsx— proactive API-key entry; persist the token via/mcp/tokenon successful discovery and attach the credential to the node.hooks/useCredentials.ts—classifyCredentialsmatchesapi_keyMCP credentials by host.Tests — backend: helper token extraction + both-type matching, block
run()withAPIKeyCredentials,/tokenstoresapi_key+ cleans up legacy OAuth2 and api_key rows. Frontend: dialog persists+attaches the token (and does not for public servers), classifier matches api_key MCP creds.Checklist 📋
For code changes:
poetry run pytest backend/blocks/mcp backend/api/features/mcp/test_routes.py backend/copilot/tools/test_run_mcp_tool.py— all passpnpm test:unitforMCPToolDialog.test.tsxandclassifyCredentials.test.ts— all passpoetry run format,poetry run lint,pnpm lint,pnpm types— clean