feat(pi): support pre-registered OAuth client for MCP servers#68
Conversation
Adds support for pre-registered OAuth clients (client_id + client_secret)
on MCP server definitions, alongside the existing bearer/oauth string auth.
- core: widen MCPConfig.auth to accept an MCPOAuthConfig object
(type, clientId, clientSecret, scope, redirectUri, clientName,
clientUri, grantType); buildPiMcpEntry normalizes object auth to the
'oauth' string so other editors stay correct.
- pi: mcp-wiring emits a resolved 'oauth' block into .pi/mcp.json,
resolving ${env:VAR} refs at generation time (pi-mcp-adapter does not
interpolate env inside the oauth block). Adds unit tests.
- cli: generateEnvExample scans auth.clientId/clientSecret so their
env refs land in .env.example.
Bumps: core 0.26.1->0.26.2, cli 0.25.1->0.25.2, pi 1.5.2->1.5.3.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Review limit reached
Next review available in: 30 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis PR introduces ChangesMCP OAuth Auth Support
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant mcpWiring
participant buildEntry
participant buildPiMcpEntry
participant buildOAuthBlock
participant resolveEnvRefs
mcpWiring->>buildEntry: buildEntry(mcp)
buildEntry->>buildPiMcpEntry: buildPiMcpEntry(mcp)
buildEntry->>buildOAuthBlock: buildOAuthBlock(mcp.auth)
buildOAuthBlock->>resolveEnvRefs: resolveEnvRefs(clientId)
buildOAuthBlock->>resolveEnvRefs: resolveEnvRefs(clientSecret)
resolveEnvRefs-->>buildOAuthBlock: resolved value
buildOAuthBlock-->>buildEntry: oauth block or undefined
buildEntry-->>mcpWiring: entry with entry.oauth attached
Possibly related PRs
Suggested reviewers: 🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/pi/src/extensions/mcp-wiring.test.ts (1)
59-93: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider adding a test for missing env var behavior
The tests cover env ref resolution well, but don't test what happens when an env var is unset (the silent empty-string → dropped field chain). Adding a test would document the current behavior and guard against regressions if the behavior is later changed to warn or throw.
🧪 Suggested test case
it("drops fields when env var is not set", () => { delete process.env.NONEXISTENT_VAR; const oauth = buildOAuthBlock({ type: "oauth", clientId: "static-id", clientSecret: "${env:NONEXISTENT_VAR}", }); expect(oauth).toEqual({ clientId: "static-id" }); });🤖 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 `@packages/pi/src/extensions/mcp-wiring.test.ts` around lines 59 - 93, Add a test in buildEntry/buildOAuthBlock coverage for the unset env-var path: verify that when an auth field references "${env:...}" and the environment variable is missing, the resolved value is omitted rather than silently producing an empty string. Update the mcp-wiring.test.ts cases alongside the existing buildEntry tests, and reference buildEntry/buildOAuthBlock behavior directly so the regression is documented if the handling later changes to warn or throw.
🤖 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 `@packages/cli/src/commands/generate.ts`:
- Around line 1173-1182: Broaden MCP env var extraction in generate.ts so
.env.example includes auth variables referenced as both ${env:VAR} and ${VAR}.
Update the logic around mcp.env, mcp.auth, and the values collection to detect
plain ${VAR} placeholders in mcp.auth.clientId and mcp.auth.clientSecret as well
as the existing env: form, then feed both into the same vars/seenInGroup flow.
In `@packages/pi/src/extensions/mcp-wiring.ts`:
- Around line 15-19: The env reference resolver is silently converting missing
variables to empty strings, which lets buildOAuthBlock produce an oauth entry
without required credentials. Update resolveEnvRefs and the OAuth flow in
mcp-wiring.ts so missing ${env:...} or ${...} values are detected before they
are dropped, and either emit a warning or throw/fail fast when
clientId/clientSecret would be missing. Use the existing resolveEnvRefs and
buildOAuthBlock symbols to keep the check close to where OAuth config is
assembled.
---
Nitpick comments:
In `@packages/pi/src/extensions/mcp-wiring.test.ts`:
- Around line 59-93: Add a test in buildEntry/buildOAuthBlock coverage for the
unset env-var path: verify that when an auth field references "${env:...}" and
the environment variable is missing, the resolved value is omitted rather than
silently producing an empty string. Update the mcp-wiring.test.ts cases
alongside the existing buildEntry tests, and reference
buildEntry/buildOAuthBlock behavior directly so the regression is documented if
the handling later changes to warn or throw.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7946ad58-2c14-4ac4-a571-116a5edfb4fb
📒 Files selected for processing (8)
packages/cli/package.jsonpackages/cli/src/commands/generate.tspackages/core/package.jsonpackages/core/src/prompt-builders.tspackages/core/src/types.tspackages/pi/package.jsonpackages/pi/src/extensions/mcp-wiring.test.tspackages/pi/src/extensions/mcp-wiring.ts
- pi: resolveEnvRefs now tracks unset env vars; mcpWiring warns when an
oauth entry references a missing env var instead of silently emitting a
config without clientId/clientSecret. Adds a regression test.
- cli: extractEnvVarsByMcp matches both ${env:VAR} and ${VAR} forms and
collects all refs per value, so .env.example is complete.
- cli: add auth (string | MCPOAuthConfig) to the local MCPConfig type.
1 participant
Summary
Adds support for pre-registered OAuth clients (
client_id+client_secret) on MCP server definitions, alongside the existing"bearer" | "oauth"string auth.Motivation: some MCP servers (e.g. Árvore
backoffice) expose a Dynamic Client Registration endpoint that always returns a fixed, third-party-bound client instead of registering a fresh one. For these, the only way to authenticate is a pre-registered client whose credentials are handed out manually. Until now the hub config'sauthproperty only accepted a string, so there was no way to express this.Changes
MCPConfig.authnow accepts anMCPOAuthConfigobject (type,clientId,clientSecret,scope,redirectUri,clientName,clientUri,grantType) in addition to the"bearer" | "oauth"strings.buildPiMcpEntrynormalizes object auth down to the"oauth"string so non-pi editors (OpenCode, etc.) stay correct and never receive the credentials object.mcp-wiringnow emits a resolvedoauthblock into.pi/mcp.json, resolving${env:VAR}/${VAR}refs at generation time — necessary becausepi-mcp-adapterdoes not interpolate env vars inside theoauthblock (only insideenv). The generated file is gitignored, so the resolved secret never lands in a committed file.buildOAuthBlock/buildEntryhelpers and added unit tests.generateEnvExamplenow scansauth.clientId/auth.clientSecretso their env refs (e.g.BACKOFFICE_MCP_CLIENT_SECRET) show up in the generated.env.example.Usage
Generates in
.pi/mcp.json:What was tested
pnpm lint— clean across core, cli, pi.pnpm test— 28 tests pass, including 6 new tests forbuildOAuthBlock/buildEntry(env-ref resolution, optional field passthrough, string-auth passthrough).pnpm build— all packages build.Version bumps
@arvoretech/hub-core0.26.1 → 0.26.2@arvoretech/hub(cli) 0.25.1 → 0.25.2@arvoretech/hub-pi1.5.2 → 1.5.3Notes / follow-up
This unblocks the hub-config side of pre-registered OAuth. For the Árvore
backofficeserver specifically, the auth server (auth.arvore.com.br) still rejects the pi localhost redirect (http://localhost:19876/callback) withinvalid_redirect_uribecause thebackoffice-mcp-claudeclient's allowlist only contains the Claude callbacks. Adding the localhost redirect URI (or issuing a pi-specific client) is a separate server-side change and is not part of this PR.Summary by CodeRabbit
New Features
Bug Fixes
Tests