-
Notifications
You must be signed in to change notification settings - Fork 4
[auth] Implement CIMD conformance test #37
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
db29715 to
2ec3e36
Compare
2ec3e36 to
a1d6d5d
Compare
commit: |
| import { Client } from '@modelcontextprotocol/sdk/client/index.js'; | ||
| import { StreamableHTTPClientTransport } from '@modelcontextprotocol/sdk/client/streamableHttp.js'; | ||
| import { withOAuthRetry } from './helpers/withOAuthRetry.js'; | ||
| import { runAsCli } from './helpers/cliRunner.js'; | ||
| import { logger } from './helpers/logger.js'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: .js extension is not needed with tsx
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed all these 👍
| const oauthFetch = withOAuthRetry( | ||
| 'test-auth-client-no-cimd', | ||
| new URL(serverUrl) | ||
| // Missing: handle401, clientMetadataUrl | ||
| )(fetch); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as a user am I expected to understand what this does? Like is this a learning moment for me. If so I think this method could be clearer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yea the readability is a bit busted here for now. we need to upstream some things to the withOauth middleware in typescript and then refactor. for now i think this will be a little gross.
mattzcarey
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. Couple of comments for readability
Address review feedback: expanded comments to better explain what CIMD is, why this client is non-compliant, and what behavior it's testing. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
tsx doesn't require .js extensions for TypeScript imports. Keeps .js for @modelcontextprotocol/sdk imports as those are external packages. Also improves auth-test-no-cimd.ts documentation for clarity. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
Waiting on typescript-sdk PR #1127 for CIMD support. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
- Integrate new discovery-metadata.ts from main (replaces basic-dcr/basic-metadata) - Keep CIMD scenario from this branch - Apply .js extension removal to discovery-metadata imports 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
partially addresses #34
depends on typescript sdk change here:
modelcontextprotocol/typescript-sdk#1127