refactor(opencode): retire MCP promise facades - #1438
Conversation
github-actions
Bot
added
the
harness
Astro-Han
added
task
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughRemoves module-level Promise façade exports from ChangesRemove MCP Promise façades and introduce AuthRunner
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed. For unrecoverable errors, disable the tool in CodeRabbit configuration. 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
🤖 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/opencode/src/mcp/oauth-provider.ts`:
- Around line 127-137: The expiresAt calculation in the updateTokens call uses a
truthy check on tokens.expires_in, which treats 0 as falsy and sets expiresAt to
undefined, losing the semantic meaning of an immediately-expiring token. Replace
the truthy check with an explicit null or undefined check so that when
tokens.expires_in is 0 (or any number including 0), it is properly calculated
and included in the expiresAt value, while only falling back to undefined when
expires_in is actually null or undefined.
In `@packages/opencode/test/effect/legacy-boundaries.test.ts`:
- Around line 225-251: The current facade detection in the test file for
mcp/index.ts and mcp/auth.ts only checks for the literal async keyword pattern
(export const ... = async), but a facade can evade this by returning a Promise
without using async, such as with arrow functions or function calls that return
Promises. To harden the detection, expand the check to identify any export that
returns or resolves to a Promise, not just those using the async keyword. This
may involve checking for Promise return types, Promise-returning function calls,
or other patterns that indicate an export provides Promise-based functionality
beyond just the async keyword syntax.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 5257bf4b-b0c7-45a7-a968-cb931e18e3de
📒 Files selected for processing (8)
packages/opencode/src/mcp/auth.tspackages/opencode/src/mcp/index.tspackages/opencode/src/mcp/oauth-provider.tspackages/opencode/test/effect/legacy-boundaries.test.tspackages/opencode/test/mcp/headers.test.tspackages/opencode/test/mcp/lifecycle.test.tspackages/opencode/test/mcp/oauth-auto-connect.test.tspackages/opencode/test/mcp/oauth-browser.test.ts
💤 Files with no reviewable changes (1)
- packages/opencode/src/mcp/auth.ts
…-auth-facade-retire # Conflicts: # packages/opencode/test/effect/legacy-boundaries.test.ts
1 participant
Summary
Retires the MCP and McpAuth Promise facade exports and keeps callers on the Effect service boundary.
Why
Related to #936.
The MCP services were still carrying per-service
makeRuntime(Service, defaultLayer)bridges and exported async facade functions after the sharedAppRuntimegraph became the production runtime. This removes that compatibility surface without changing MCP catalog, OAuth, auth storage, or connection semantics.Related Issue
Related to #936
Human Review Status
Pending
Review Focus
Please focus on the OAuth provider boundary and the MCP test helper conversions. The production MCP service now injects its existing
McpAuth.ServiceintoMcpOAuthProvider; standalone provider usage falls back toAppRuntimewithout reintroducing exported service facades.Risk Notes
No visible UI or copy changed, so the UI/manual screenshot checklist item is not applicable. No platform, packaging, updater, signing, shell, or permissions surface changed. No docs, release notes, dependencies, credentials, deletion behavior, generated content, or local file output changed.
Fresh-eye result: no P0/P1 findings. One P2 simplification was found and fixed by using the already-injected MCP auth service inside the production provider path instead of bouncing every OAuth callback through the full AppRuntime.
How To Verify
Screenshots or Recordings
Not applicable. No visible UI changes.
Checklist
bug,enhancement,task,documentation. Type labels are author-added; the labeler bot does NOT assign them. Add the label in the GitHub UI, then tick this.app,ui,platform,harness,ci. The labeler bot assigns these on PR open based on changed paths. Confirm the bot's choice (or override if wrong), then tick this.P0,P1,P2,P3. The priority-triage bot suggests one on PR open. Confirm or override, then tick this.Pending,Approved by @<reviewer>, orNot required: <reason>(default isPending; "not required" is restricted to bot-authored low-risk PRs).dev, and my PR title and commit messages use Conventional Commits in English.Summary by CodeRabbit