refactor(cloudflare): rename MCP provider service - #274
Conversation
Summary by CodeRabbit
WalkthroughThe Cloudflare provider identifier changes from Sequence Diagram(s)sequenceDiagram
participant Provider
participant RuntimeStore
participant SQLite
Provider->>RuntimeStore: Use cloudflare service identifier
RuntimeStore->>SQLite: Read or migrate persisted records
SQLite-->>RuntimeStore: Return cloudflare service data
RuntimeStore-->>Provider: Return normalized configuration
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches✨ Simplify code
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.
🧹 Nitpick comments (2)
src/server/storage/sqlite-runtime-store.test.ts (1)
580-604: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winShare the migration list and the
runtime_migrationsDDL.This test repeats the migration names already listed at line 52, and it recreates the
runtime_migrationsschema thatrunSqliteMigrationsowns. Both copies drift when a migration is added or the bookkeeping table changes. Extract a module-level constant for the ordered migration names, and derive the legacy list by slicing it. Reuse a single helper for the bookkeeping table DDL.🤖 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/server/storage/sqlite-runtime-store.test.ts` around lines 580 - 604, Extract the ordered migration names into a module-level constant and reuse it throughout the test, deriving the legacy migration list via slicing instead of duplicating names. Extract the runtime_migrations table definition into a shared helper and use that helper in the test setup and runSqliteMigrations-related setup, so the bookkeeping schema has one source of truth.src/server/storage/d1-runtime-store.test.ts (1)
454-478: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRead the migrations directory instead of listing each file.
Every new migration requires another
execblock here. Enumeratemigrations/and apply the files in sorted name order. The test database then stays current automatically.♻️ Proposed refactor
constructor() { - this.database.exec(readFileSync(new URL("../../../migrations/0001_runtime.sql", import.meta.url), "utf8")); - // ... one block per migration ... - this.database.exec( - readFileSync(new URL("../../../migrations/0011_cloudflare_service.sql", import.meta.url), "utf8"), - ); + const migrationsDir = new URL("../../../migrations/", import.meta.url); + for (const name of readdirSync(migrationsDir).filter((file) => file.endsWith(".sql")).sort()) { + this.database.exec(readFileSync(new URL(name, migrationsDir), "utf8")); + } }Add
readdirSyncto the existingnode:fsimport.🤖 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/server/storage/d1-runtime-store.test.ts` around lines 454 - 478, Update the test database constructor to enumerate the migrations directory with readdirSync, sort migration filenames by name, and execute each file in order using the existing database.exec flow. Replace the hardcoded migration list while preserving the current migration URL resolution and file-reading behavior.
🤖 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 `@src/server/storage/d1-runtime-store.test.ts`:
- Around line 454-478: Update the test database constructor to enumerate the
migrations directory with readdirSync, sort migration filenames by name, and
execute each file in order using the existing database.exec flow. Replace the
hardcoded migration list while preserving the current migration URL resolution
and file-reading behavior.
In `@src/server/storage/sqlite-runtime-store.test.ts`:
- Around line 580-604: Extract the ordered migration names into a module-level
constant and reuse it throughout the test, deriving the legacy migration list
via slicing instead of duplicating names. Extract the runtime_migrations table
definition into a shared helper and use that helper in the test setup and
runSqliteMigrations-related setup, so the bookkeeping schema has one source of
truth.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: f1bb0c2a-d3f9-4533-8aae-2ce120518b06
📒 Files selected for processing (8)
migrations/0011_cloudflare_service.sqlsrc/providers/cloudflare/actions.tssrc/providers/cloudflare/definition.tssrc/providers/cloudflare/executors.tssrc/server/storage/d1-runtime-store.test.tssrc/server/storage/d1-runtime-store.tssrc/server/storage/sqlite-runtime-store.test.tssrc/server/storage/sqlite-runtime-store.ts
|
I do not think this rename is justified as currently described. All existing Cloudflare providers use qualified IDs ( The claimed model confusion is also not supported by a reproduction or evaluation, while the rename changes public Action IDs and other external references. Unless the explicit product decision is to make this the default Cloudflare provider, I think it should retain a qualified service ID. I do not support the rename in its current form. |
|
Thanks for raising the namespace concern. One relevant detail may not have been clear in the PR description: Cloudflare describes this unified MCP server as “a token-efficient MCP server for the entire Cloudflare API,” covering around 2,500 endpoints. Its documented supported products include Workers, R2, DNS, KV, D1, Pages, Firewall, Access, and others. So while this provider does not aggregate the typed Action definitions from I agree that this distinction between capability coverage and typed Action aggregation should be made explicit. Does that broader API scope change your view on the |
|
Thanks for clarifying the API coverage. That confirms this provider has broad capabilities, but it does not resolve my naming concern. The unqualified Broad API coverage does not by itself make this the canonical provider. In this case, I therefore still do not support changing the public service ID to |
Summary
cloudflare_mcptocloudflarecloudflare.docs,cloudflare.search, andcloudflare.executeMotivation
The
cloudflare_mcpservice name makes models interpret the Actions as the separately connected Cloudflare MCP server and prefer that tool surface instead of the Cloudflare Provider exposed through Open Connector. The shortercloudflarenamespace makes the Open Connector Actions unambiguous in model-facing catalogs.Compatibility
Stored runtime data is migrated from
cloudflare_mcptocloudflare. Deployment-level policy environment variables are outside runtime storage and operators using explicitcloudflare_mcp.*orcloudflare_mcprules will need to rename those entries.Validation
npm run generate:catalognpm run fix-checknpm test— 69 files, 724 tests passed