feat: publish typed MCP setup contract (DVR-212) - #217
Draft
xsvfat wants to merge 22 commits into
Draft
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughThe PR adds a typed MCP setup contract and generated manifest. It exports both artifacts, centralizes surface registration, updates setup documentation, and validates generation, packaging, runtime surfaces, authentication, and publish status. ChangesSetup contract and publication
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant SetupManifest
participant BrowserlessMcpPackage
participant MCPServer
Client->>SetupManifest: Read setup and authentication configuration
SetupManifest->>BrowserlessMcpPackage: Resolve published setup exports
BrowserlessMcpPackage->>MCPServer: Launch stdio or connect to streamable HTTP
Client->>MCPServer: Authenticate with OAuth or bearer headers
MCPServer-->>Client: Expose configured MCP surfaces
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/tools/register.ts (1)
28-28: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe no-op couples
browserless_skillavailability tobrowserless_agentwithout a compile-time guard.
registerAgentToolsregisters both surfaces. If a future change sets a differentsurfacevalue forbrowserless_skillandbrowserless_agentinMCP_SURFACE_REGISTRY, the generated inventory and the runtime surface diverge, and the type system stays silent. Both entries are'both'today, so there is no current defect.Consider asserting the invariant next to the no-op so the failure is loud.
♻️ Optional: assert the shared surface value
- browserless_skill: () => {}, // registered together with browserless_agent + // Registered together with browserless_agent; guard the shared availability + // so a registry edit cannot split them silently. + browserless_skill: () => { + const agent = MCP_SURFACE_REGISTRY.find( + (e) => e.id === 'browserless_agent', + ); + const skill = MCP_SURFACE_REGISTRY.find( + (e) => e.id === 'browserless_skill', + ); + if (agent?.surface !== skill?.surface) { + throw new Error( + 'browserless_skill must share browserless_agent availability.', + ); + } + },🤖 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 `@src/tools/register.ts` at line 28, Add a compile-time assertion beside the browserless_skill no-op in registerAgentTools that verifies browserless_skill and browserless_agent have the same surface value in MCP_SURFACE_REGISTRY, while preserving the existing registration behavior.src/setup-contract.ts (1)
48-48: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider deriving the endpoint from the runtime base URL and narrowing the exported literals.
HOSTED_URLduplicatesconfig.mcpBaseUrl.test/setup-contract.spec.ts(lines 110-122) only proves parity whenMCP_BASE_URLis unset, so a deployment that setsMCP_BASE_URLproduces a contract that no longer matches the served endpoint.The returned object is also not
as const, soendpoint.transport,auth.methods[].id, andclients[].setupKindwiden tostringinBrowserlessMcpSetupContract. Consumers in Account and Docs then lose discriminated-union checking on a contract that exists to be typed.♻️ Optional: narrow the published literals
- endpoint: { - url: HOSTED_URL, - transport: 'streamable-http', - }, + endpoint: { + url: HOSTED_URL, + transport: 'streamable-http' as const, + },Also applies to: 65-84
🤖 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 `@src/setup-contract.ts` at line 48, Replace the hard-coded HOSTED_URL in the setup contract with the runtime config.mcpBaseUrl so the published endpoint matches deployments that override MCP_BASE_URL. Mark the returned BrowserlessMcpSetupContract object as const to preserve literal types for endpoint.transport, auth.methods[].id, and clients[].setupKind.
🤖 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 `@README.md`:
- Around line 25-28: Centralize the Browserless setup skill URL in the
createSetupContract contract, then generate the links in README.md (lines
25-28), install.md (lines 3-4), and llms-install.md (lines 3-4) from that
contract value; update the affected test to use the same source, or switch all
consumers to an unversioned alias so future skill releases require no manual
documentation changes.
In `@scripts/ci/notify-slack.sh`:
- Line 19: Update the PUBLISH_STATUS initialization in the notification script
to require an explicitly provided status, matching the required handling used
for TAG and RELEASE_URL, rather than defaulting to success; also update the
corresponding literal assertion in setup-contract.spec.ts to reflect the new
initialization.
---
Nitpick comments:
In `@src/setup-contract.ts`:
- Line 48: Replace the hard-coded HOSTED_URL in the setup contract with the
runtime config.mcpBaseUrl so the published endpoint matches deployments that
override MCP_BASE_URL. Mark the returned BrowserlessMcpSetupContract object as
const to preserve literal types for endpoint.transport, auth.methods[].id, and
clients[].setupKind.
In `@src/tools/register.ts`:
- Line 28: Add a compile-time assertion beside the browserless_skill no-op in
registerAgentTools that verifies browserless_skill and browserless_agent have
the same surface value in MCP_SURFACE_REGISTRY, while preserving the existing
registration 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
Run ID: f135512b-7cf1-42d5-9c4c-ed5dfc751bdb
📒 Files selected for processing (12)
.github/workflows/npm-publish.ymlREADME.mdinstall.mdllms-install.mdpackage.jsonscripts/ci/notify-slack.shscripts/ci/verify-package-tarball.shscripts/generate-setup-contract.mjssetup/browserless-mcp-setup.jsonsrc/setup-contract.tssrc/tools/register.tstest/setup-contract.spec.ts
xsvfat
force-pushed
the
feat/browserless-setup-for-agents-plan
branch
from
August 3, 2026 21:01
7e52a80 to
d7aa923
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
scripts/generate-setup-contract.mjs (1)
1-56: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd Node globals if
scriptsenters the lint scope.
eslint.config.jsdoes not declareglobals.nodeforscripts/**/*.mjs. Directly linting this file reports nineno-undeferrors. The currentnpm run lintcommand excludesscripts, so update the lint scope and config together.🤖 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 `@scripts/generate-setup-contract.mjs` around lines 1 - 56, Update eslint.config.js to include Node globals for scripts/**/*.mjs, then expand the npm run lint target so the scripts directory is linted alongside the existing sources. Ensure scripts/generate-setup-contract.mjs passes lint without no-undef errors for Node globals such as process, URL, and Buffer.
🤖 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.
Nitpick comments:
In `@scripts/generate-setup-contract.mjs`:
- Around line 1-56: Update eslint.config.js to include Node globals for
scripts/**/*.mjs, then expand the npm run lint target so the scripts directory
is linted alongside the existing sources. Ensure
scripts/generate-setup-contract.mjs passes lint without no-undef errors for Node
globals such as process, URL, and Buffer.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8035d849-07ee-430e-bba1-bf6ae4d95e61
📒 Files selected for processing (12)
.github/workflows/npm-publish.ymlREADME.mdinstall.mdllms-install.mdpackage.jsonscripts/ci/notify-slack.shscripts/ci/verify-package-tarball.shscripts/generate-setup-contract.mjssetup/browserless-mcp-setup.jsonsrc/setup-contract.tssrc/tools/register.tstest/setup-contract.spec.ts
🚧 Files skipped from review as they are similar to previous changes (9)
- setup/browserless-mcp-setup.json
- llms-install.md
- package.json
- install.md
- scripts/ci/verify-package-tarball.sh
- .github/workflows/npm-publish.yml
- src/tools/register.ts
- src/setup-contract.ts
- scripts/ci/notify-slack.sh
Contributor
Author
|
Automated PR watch summary:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Related issues
DVR-212
Changes
Test plan
npm testpasses locally — 570 passed after rebasing onto currentmain.npm run lintpasses locally.npm run coveragethresholds still met — no separate coverage gate was run.Additional verification: typed/JSON imports, tarball allowlist, shell syntax, Account snapshot byte parity, and hosted OAuth tool execution passed.
Checklist
🤖 Built with the /implement-plan skill from pasted-text.txt
Summary by CodeRabbit
New Features
Bug Fixes
Documentation