Skip to content

Conversation

hongkongkiwi
Copy link

@hongkongkiwi hongkongkiwi commented Sep 23, 2025

Summary

This PR adds three new command-line options to the MCP server for better control over permissions and environment access:

  • --disable-deployment: Disables deployment-related tools only
  • --readonly: Runs in full read-only mode, disabling all write operations
  • --env-only <environments>: Restricts the MCP server to specific environments (comma-separated list)

Changes

New Features

  • --env-only flag: Allows fine-grained environment control with comma-separated list of allowed environments (dev, staging, prod, preview)
  • --readonly flag: Disables all write operations including deployments, task triggering, project creation, and run cancellation
  • --disable-deployment flag: Disables only deployment-related tools while keeping other write operations available

Key Implementation Details

  1. Environment Control: The --env-only flag deprecates --dev-only (kept for backward compatibility) and allows specifying exactly which environments the MCP server can access
  2. Proper Hierarchy: --readonly overrides --disable-deployment since deployments are write operations
  3. Mutual Exclusivity: --dev-only and --env-only are mutually exclusive with proper error handling
  4. Environment-specific Deployments: Deployments work for any allowed environment (not requiring access to all environments)
  5. Tool Categorization: Tools are properly categorized into read-only, write, and deployment categories

Code Quality Improvements

  • Added environment name validation for --env-only flag
  • Simplified environment validation logic by removing redundant checks
  • Standardized error messages across all tools for consistency
  • Added getAllowedEnvironments() helper method for better error messaging

Testing

  • Tested --env-only with single environment
  • Tested --env-only with multiple environments
  • Tested --readonly mode disables write operations
  • Tested --disable-deployment only disables deployments
  • Verified backward compatibility with --dev-only
  • Verified mutual exclusivity error handling
  • Tested invalid environment name validation

Breaking Changes

None. The --dev-only flag is maintained for backward compatibility.

Documentation

Command help text has been updated with clear descriptions of each option and their relationships.

Copy link

changeset-bot bot commented Sep 23, 2025

⚠️ No Changeset found

Latest commit: 04e673f

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Copy link
Contributor

coderabbitai bot commented Sep 23, 2025

Walkthrough

Adds three MCP options across the CLI and context: --env-only (string/CSV), --disable-deployment (boolean), and --readonly (boolean). install-mcp and mcp commands expose the flags, deprecate --dev-only, enforce mutual exclusivity between --dev-only and --env-only (error if both provided), parse/validate env-only values (legacy --dev-only maps to envOnly=['dev'] if used alone), and treat env-only as taking precedence. McpContext gains envOnly, disableDeployment, readonly, plus isEnvironmentAllowed and getAllowedEnvironments. resolveMcpServerConfig and install-mcp argument construction include the new flags. Tools are refactored to use isEnvironmentAllowed, standardize denial messages, and register read/write/deployment tools conditionally based on readonly and disableDeployment.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Pre-merge checks and finishing touches

❌ Failed checks (2 warnings)
Check name Status Explanation Resolution
Description Check ⚠️ Warning The PR description is detailed and documents summary, implementation details, testing, breaking changes, and documentation updates, but it does not follow the repository's required template: it is missing a "Closes #" header, the ✓ Checklist section, and the explicit "Changelog" and "Screenshots" sections required by the template. Because the repository template is specified as required, these omissions mean the description does not fully adhere to the expected format. The substantive content is good, but the missing template sections must be added to pass the repository's description requirements. Update the PR description to exactly follow the repository template by adding a "Closes #" line if applicable, filling in the ✅ Checklist (confirm contributing guide, title convention, and tests), and adding the "Changelog" and "Screenshots" sections (or explicitly noting none). Keep the existing Summary, Changes, and Testing content but ensure the template headings and required items are present before merging.
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (1 passed)
Check name Status Explanation
Title Check ✅ Passed The title concisely and accurately describes the primary change: adding three CLI options (disable-deployment, readonly, env-only) to the MCP server. It matches the changes in the diff and is specific without extra noise or vague terms. A teammate scanning history will understand the main intent from the title alone.
✨ Finishing touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Tip

👮 Agentic pre-merge checks are now available in preview!

Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.

  • Built-in checks – Quickly apply ready-made checks to enforce title conventions, require pull request descriptions that follow templates, validate linked issues for compliance, and more.
  • Custom agentic checks – Define your own rules using CodeRabbit’s advanced agentic capabilities to enforce organization-specific policies and workflows. For example, you can instruct CodeRabbit’s agent to verify that API documentation is updated whenever API schema files are modified in a PR. Note: Upto 5 custom checks are currently allowed during the preview period. Pricing for this feature will be announced in a few weeks.

Please see the documentation for more information.

Example:

reviews:
  pre_merge_checks:
    custom_checks:
      - name: "Undocumented Breaking Changes"
        mode: "warning"
        instructions: |
          Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).

Please share your feedback with us on this Discord post.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
packages/cli-v3/src/commands/install-mcp.ts (1)

280-313: Manual config misses new flags and can emit both devOnly + envOnly.

Unsupported clients’ instructions don’t include --env-only/--disable-deployment/--readonly and may add --dev-only even when --env-only is set.

Apply this diff:

   const args = [`trigger.dev@${options.tag}`, "mcp"];
 
   if (options.logFile) {
     args.push("--log-file", options.logFile);
   }
 
   if (options.apiUrl) {
     args.push("--api-url", options.apiUrl);
   }
 
-  if (options.devOnly) {
-    args.push("--dev-only");
-  }
+  if (options.envOnly) {
+    args.push("--env-only", options.envOnly);
+  } else if (options.devOnly) {
+    args.push("--dev-only");
+  }
 
   if (options.projectRef) {
     args.push("--project-ref", options.projectRef);
   }
 
   if (options.logLevel && options.logLevel !== "log") {
     args.push("--log-level", options.logLevel);
   }
+
+  if (options.disableDeployment) {
+    args.push("--disable-deployment");
+  }
+
+  if (options.readonly) {
+    args.push("--readonly");
+  }
🧹 Nitpick comments (3)
packages/cli-v3/src/mcp/context.ts (1)

191-206: Normalize env names and tighten types for safer checks.

Trim/lower-case both input and configured envs to avoid case/whitespace mismatches. Optionally define a literal type for environments.

Apply this diff to normalize values:

   public isEnvironmentAllowed(environment: string): boolean {
-    // If envOnly is specified, use that (devOnly is already converted to envOnly)
-    if (this.options.envOnly && this.options.envOnly.length > 0) {
-      return this.options.envOnly.includes(environment);
-    }
- 
-    // If no restrictions, all environments are allowed
-    return true;
+    const env = environment.trim().toLowerCase();
+    const allowed = this.options.envOnly?.map((e) => e.trim().toLowerCase());
+    if (allowed && allowed.length > 0) {
+      return allowed.includes(env);
+    }
+    return true; // no restrictions
   }
 
   public getAllowedEnvironments(): string {
-    if (this.options.envOnly && this.options.envOnly.length > 0) {
-      return this.options.envOnly.join(", ");
-    }
-    return "all environments";
+    const allowed = this.options.envOnly?.map((e) => e.trim().toLowerCase());
+    if (allowed && allowed.length > 0) {
+      return allowed.join(", ");
+    }
+    return "all environments";
   }

Optionally, add this near the top and use it in McpContextOptions.envOnly and isEnvironmentAllowed(environment):

export type AllowedEnvironment = "dev" | "staging" | "prod" | "preview";
packages/cli-v3/src/commands/mcp.ts (1)

124-147: Harden env parsing: lower-case, drop empties, and de-duplicate.

Prevents confusing errors like an empty env entry and accepts case-insensitive input.

Apply this diff:

-  // Parse envOnly into an array if provided
+  // Parse envOnly into an array if provided
   let envOnly: string[] | undefined;
   if (options.envOnly) {
-    envOnly = options.envOnly.split(',').map(env => env.trim());
+    envOnly = Array.from(
+      new Set(
+        options.envOnly
+          .split(",")
+          .map((env) => env.trim().toLowerCase())
+          .filter(Boolean)
+      )
+    );
 
-    // Validate environment names
-    const validEnvironments = ['dev', 'staging', 'prod', 'preview'];
+    // Validate environment names
+    const validEnvironments = ["dev", "staging", "prod", "preview"] as const;
     const invalidEnvs = envOnly.filter(env => !validEnvironments.includes(env));
     if (invalidEnvs.length > 0) {
-      logger.error(`Error: Invalid environment(s): ${invalidEnvs.join(', ')}`);
-      logger.error(`Valid environments are: ${validEnvironments.join(', ')}`);
+      logger.error(`Error: Invalid environment(s): ${invalidEnvs.join(", ")}`);
+      logger.error(`Valid environments are: ${validEnvironments.join(", ")}`);
       process.exit(1);
     }
   } else if (options.devOnly) {
     // For backward compatibility, convert devOnly to envOnly
-    envOnly = ['dev'];
+    envOnly = ["dev"];
   }
packages/cli-v3/src/commands/install-mcp.ts (1)

211-216: Optional: pre‑validate envOnly during install to fail fast.

Mirror the server’s validation (dev, staging, prod, preview) and throw OutroCommandError early to avoid writing broken configs.

I can add a small parser/validator here that normalizes case, drops empties, and validates against a shared VALID_ENVIRONMENTS constant. Want a patch?

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 49728b5 and e181829.

📒 Files selected for processing (7)
  • packages/cli-v3/src/commands/install-mcp.ts (4 hunks)
  • packages/cli-v3/src/commands/mcp.ts (3 hunks)
  • packages/cli-v3/src/mcp/context.ts (2 hunks)
  • packages/cli-v3/src/mcp/tools.ts (1 hunks)
  • packages/cli-v3/src/mcp/tools/deploys.ts (2 hunks)
  • packages/cli-v3/src/mcp/tools/runs.ts (4 hunks)
  • packages/cli-v3/src/mcp/tools/tasks.ts (2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

**/*.{ts,tsx}: Always prefer using isomorphic code like fetch, ReadableStream, etc. instead of Node.js specific code
For TypeScript, we usually use types over interfaces
Avoid enums
No default exports, use function declarations

Files:

  • packages/cli-v3/src/mcp/tools/tasks.ts
  • packages/cli-v3/src/mcp/context.ts
  • packages/cli-v3/src/mcp/tools/deploys.ts
  • packages/cli-v3/src/mcp/tools/runs.ts
  • packages/cli-v3/src/commands/mcp.ts
  • packages/cli-v3/src/mcp/tools.ts
  • packages/cli-v3/src/commands/install-mcp.ts
🧠 Learnings (1)
📚 Learning: 2025-08-18T10:07:17.368Z
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-08-18T10:07:17.368Z
Learning: Applies to **/trigger/**/*.{ts,tsx,js,jsx} : Use triggerAndWait() only from within a task context (not from generic app code) and handle result.ok or use unwrap() with error handling

Applied to files:

  • packages/cli-v3/src/mcp/tools/tasks.ts
🧬 Code graph analysis (6)
packages/cli-v3/src/mcp/tools/tasks.ts (1)
packages/cli-v3/src/mcp/utils.ts (1)
  • respondWithError (5-15)
packages/cli-v3/src/mcp/tools/deploys.ts (1)
packages/cli-v3/src/mcp/utils.ts (1)
  • respondWithError (5-15)
packages/cli-v3/src/mcp/tools/runs.ts (2)
packages/core/src/v3/taskContext/index.ts (1)
  • ctx (26-28)
packages/cli-v3/src/mcp/utils.ts (1)
  • respondWithError (5-15)
packages/cli-v3/src/commands/mcp.ts (1)
packages/cli-v3/src/mcp/context.ts (2)
  • logger (36-38)
  • McpContext (27-207)
packages/cli-v3/src/mcp/tools.ts (6)
packages/cli-v3/src/mcp/tools/docs.ts (1)
  • searchDocsTool (6-20)
packages/cli-v3/src/mcp/tools/orgs.ts (4)
  • listOrgsTool (10-36)
  • listProjectsTool (38-104)
  • createProjectInOrgTool (106-134)
  • initializeProjectTool (136-221)
packages/cli-v3/src/mcp/tools/tasks.ts (2)
  • getCurrentWorker (6-83)
  • triggerTaskTool (85-158)
packages/cli-v3/src/mcp/tools/runs.ts (4)
  • listRunsTool (157-205)
  • getRunDetailsTool (7-62)
  • waitForRunToCompleteTool (64-115)
  • cancelRunTool (117-155)
packages/cli-v3/src/mcp/tools/previewBranches.ts (1)
  • listPreviewBranchesTool (6-35)
packages/cli-v3/src/mcp/tools/deploys.ts (2)
  • listDeploysTool (114-146)
  • deployTool (13-112)
packages/cli-v3/src/commands/install-mcp.ts (1)
packages/cli-v3/src/cli/common.ts (1)
  • OutroCommandError (35-35)
🔇 Additional comments (15)
packages/cli-v3/src/mcp/context.ts (1)

22-24: Options added align with PR goals.

envOnly, disableDeployment, and readonly extensions look good.

packages/cli-v3/src/mcp/tools/tasks.ts (2)

14-17: Environment gate added — looks correct.

Consistent with new ctx API and message format.


93-96: Environment gate added — looks correct.

Blocks write when env not allowed; registration handles readonly.

packages/cli-v3/src/mcp/tools.ts (1)

47-58: Dynamic registration honors readonly/disableDeployment.

Tool grouping and precedence (readonly > disableDeployment) are correct.

packages/cli-v3/src/mcp/tools/runs.ts (4)

15-19: Env gate added — get_run_details.

Good early return with standardized message.


72-76: Env gate added — wait_for_run_to_complete.

Consistent with ctx API.


125-129: Env gate added — cancel_run.

Correctly blocks unauthorized environments.


165-169: Env gate added — list_runs.

Aligned with other tools.

packages/cli-v3/src/mcp/tools/deploys.ts (2)

21-26: Env gate added — deploy.

Clear denial message; complements registration-time disablement.


122-126: Env gate added — list_deploys.

Consistent with read access policy.

packages/cli-v3/src/commands/mcp.ts (3)

23-25: Schema additions look good.

New options are typed and defaulted appropriately.


40-53: CLI flags and deprecation messaging read well.

Descriptions clearly explain relationships (readonly overrides disable-deployment).


153-157: McpContextOptions contains the required fields — no changes needed.
packages/cli-v3/src/mcp/context.ts declares: devOnly?: boolean; envOnly?: string[]; disableDeployment?: boolean; readonly?: boolean.

packages/cli-v3/src/commands/install-mcp.ts (2)

117-120: Schema additions LGTM.

New options are optional and consistent with CLI flags.


143-155: New flags’ help text is clear.

Deprecation guidance for --dev-only is explicit.

…MCP server

Add three new options to the MCP server for better control:
- --disable-deployment: Disables deployment-related tools
- --readonly: Run in read-only mode, disabling all write operations (deployments, task triggering, project creation, run cancellation)
- --env-only: Restrict the MCP server to specific environments (dev, staging, prod, preview)

Key features:
- The --env-only flag allows fine-grained environment control and deprecates --dev-only (kept for backward compatibility)
- The --readonly flag overrides --disable-deployment since deployments are write operations
- Deployments are allowed to any environment that the MCP server has access to
- --dev-only and --env-only are mutually exclusive with proper error handling
- listDeploysTool correctly categorized as read-only operation

Signed-off-by: Andy <andy+github@savage.hk>
- Add environment name validation for --env-only flag
- Simplify environment validation logic (remove redundant devOnly check)
- Standardize error messages across all tools
- Add getAllowedEnvironments() helper method for consistent messaging

Signed-off-by: Andy <andy+github@savage.hk>
- Fix manual config for unsupported clients to include all new flags
- Improve environment parsing to handle edge cases (empty strings, duplicates)
- Normalize environment names (lowercase, trim) for consistent matching
- Ensure envOnly takes precedence over devOnly in all places

Signed-off-by: Andy <andy+github@savage.hk>
@hongkongkiwi hongkongkiwi force-pushed the feat/mcp-server-disable-readonly-options branch from c152f3f to 28c8cdd Compare September 23, 2025 08:01
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (1)
packages/cli-v3/src/mcp/context.ts (1)

191-203: Defensively normalize envOnly values inside the check

Avoid relying on upstream normalization; trim/lowercase envOnly here before includes.

-  public isEnvironmentAllowed(environment: string): boolean {
-    // Normalize the environment name for comparison
-    const normalizedEnv = environment.trim().toLowerCase();
-
-    // If envOnly is specified, use that (devOnly is already converted to envOnly)
-    if (this.options.envOnly && this.options.envOnly.length > 0) {
-      // Note: envOnly is already normalized to lowercase in mcp.ts
-      return this.options.envOnly.includes(normalizedEnv);
-    }
-
-    // If no restrictions, all environments are allowed
-    return true;
-  }
+  public isEnvironmentAllowed(environment: string): boolean {
+    const normalizedEnv = environment.trim().toLowerCase();
+    const allowed =
+      this.options.envOnly
+        ?.map((e) => e.trim().toLowerCase())
+        .filter((e) => e.length > 0) ?? [];
+    if (allowed.length > 0) {
+      return allowed.includes(normalizedEnv);
+    }
+    return true;
+  }
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 73e5b67 and c152f3f.

📒 Files selected for processing (7)
  • packages/cli-v3/src/commands/install-mcp.ts (5 hunks)
  • packages/cli-v3/src/commands/mcp.ts (3 hunks)
  • packages/cli-v3/src/mcp/context.ts (2 hunks)
  • packages/cli-v3/src/mcp/tools.ts (1 hunks)
  • packages/cli-v3/src/mcp/tools/deploys.ts (2 hunks)
  • packages/cli-v3/src/mcp/tools/runs.ts (4 hunks)
  • packages/cli-v3/src/mcp/tools/tasks.ts (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
  • packages/cli-v3/src/mcp/tools/tasks.ts
  • packages/cli-v3/src/commands/mcp.ts
  • packages/cli-v3/src/mcp/tools/deploys.ts
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

**/*.{ts,tsx}: Always prefer using isomorphic code like fetch, ReadableStream, etc. instead of Node.js specific code
For TypeScript, we usually use types over interfaces
Avoid enums
No default exports, use function declarations

Files:

  • packages/cli-v3/src/mcp/tools.ts
  • packages/cli-v3/src/mcp/tools/runs.ts
  • packages/cli-v3/src/mcp/context.ts
  • packages/cli-v3/src/commands/install-mcp.ts
🧬 Code graph analysis (3)
packages/cli-v3/src/mcp/tools.ts (5)
packages/cli-v3/src/mcp/tools/docs.ts (1)
  • searchDocsTool (6-20)
packages/cli-v3/src/mcp/tools/orgs.ts (4)
  • listOrgsTool (10-36)
  • listProjectsTool (38-104)
  • createProjectInOrgTool (106-134)
  • initializeProjectTool (136-221)
packages/cli-v3/src/mcp/tools/tasks.ts (2)
  • getCurrentWorker (6-83)
  • triggerTaskTool (85-158)
packages/cli-v3/src/mcp/tools/runs.ts (1)
  • cancelRunTool (117-155)
packages/cli-v3/src/mcp/tools/deploys.ts (2)
  • listDeploysTool (114-146)
  • deployTool (13-112)
packages/cli-v3/src/mcp/tools/runs.ts (2)
packages/core/src/v3/taskContext/index.ts (1)
  • ctx (26-28)
packages/cli-v3/src/mcp/utils.ts (1)
  • respondWithError (5-15)
packages/cli-v3/src/commands/install-mcp.ts (1)
packages/cli-v3/src/cli/common.ts (1)
  • OutroCommandError (35-35)
🔇 Additional comments (13)
packages/cli-v3/src/mcp/tools/runs.ts (1)

15-18: Unified env gating and messaging: LGTM

Consistent use of ctx.isEnvironmentAllowed(...) and standardized denial text looks good.

Also applies to: 72-76, 125-129, 165-168

packages/cli-v3/src/mcp/tools.ts (4)

21-32: Gate preview-branches registration behind envOnly allowance

Only register listPreviewBranchesTool if preview is allowed (or unrestricted) to avoid exposing a tool that cannot be used under env restrictions.

Apply these diffs:

@@
   const readOnlyTools = [
     searchDocsTool,
     listOrgsTool,
     listProjectsTool,
     getCurrentWorker,
     listRunsTool,
     getRunDetailsTool,
     waitForRunToCompleteTool,
-    listPreviewBranchesTool,
     listDeploysTool, // This is a read operation, not a write
   ];
@@
-  // Add deployment tools if not disabled and not in readonly mode
+  // Add deployment tools if not disabled and not in readonly mode
   if (!context.options.disableDeployment && !context.options.readonly) {
     tools = [...tools, ...deploymentTools];
   }
+
+  // Register preview branches only if preview env is allowed (or no restriction)
+  if (!context.options.envOnly || context.options.envOnly.includes("preview")) {
+    tools = [...tools, listPreviewBranchesTool];
+  }

Also applies to: 54-58


34-41: Tool grouping: LGTM

Correctly categorizes write tools for readonly gating.


43-46: Deployment tools separation: LGTM

Clean separation enables independent disablement.


47-58: Conditional assembly: LGTM

Readonly and disableDeployment checks are correct.

packages/cli-v3/src/mcp/context.ts (2)

22-25: Options surface: LGTM

Public options added without breaking existing callers.


205-210: Allowed envs messaging: LGTM

Simple, user-friendly fallback.

packages/cli-v3/src/commands/install-mcp.ts (6)

117-120: Schema extensions: LGTM

New flags captured in zod schema; backward compatible.


143-155: CLI options and help text: LGTM

Clear descriptions; deprecation note is helpful.


211-216: Mutual exclusivity validation: LGTM

Explicit guard prevents conflicting inputs.


218-221: Skip devOnly prompt when envOnly is provided

Otherwise both flags can be set, leading to conflicting args later.

-  const devOnly = await resolveDevOnly(opts);
+  const devOnly = opts.envOnly ? false : await resolveDevOnly(opts);
   opts.devOnly = devOnly;

290-304: Manual config args precedence: LGTM

envOnly correctly takes precedence over devOnly; readonly/disableDeployment appended.


516-527: Ensure only one of --env-only or --dev-only is emitted

Currently both can be added; enforce precedence to avoid invalid configurations.

-  if (options.devOnly) {
-    args.push("--dev-only");
-  }
-
-  if (options.envOnly) {
-    args.push("--env-only", options.envOnly);
-  }
+  if (options.envOnly) {
+    args.push("--env-only", options.envOnly);
+  } else if (options.devOnly) {
+    args.push("--dev-only");
+  }

- Skip devOnly prompt when envOnly is already provided
- Ensure only one of --env-only or --dev-only is emitted in config
- Addresses CodeRabbit review feedback
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (5)
packages/cli-v3/src/mcp/context.ts (2)

22-25: Type the environments and make options invariant

envOnly looks good, but consider tightening and normalizing at the boundary to avoid drift if other callers construct McpContext directly.

You can:

  • Define a shared Environment union type to match allowed values.
  • Normalize/dedupe envOnly in the constructor to enforce invariant.

Example outside this range:

// e.g. in a shared constants module
export const VALID_ENVIRONMENTS = ["dev", "staging", "prod", "preview"] as const;
export type Environment = typeof VALID_ENVIRONMENTS[number];

// here
export type McpContextOptions = {
  // ...
  envOnly?: Environment[];
  disableDeployment?: boolean;
  readonly?: boolean;
};

And in the constructor (outside this range):

if (options.envOnly) {
  options.envOnly = Array.from(
    new Set(options.envOnly.map((e) => e.trim().toLowerCase() as Environment))
  );
}

191-210: LGTM; small defensive nit

isEnvironmentAllowed()/getAllowedEnvironments() are clean. If you don’t normalize in the constructor, consider normalizing envOnly here for resilience against future callers.

packages/cli-v3/src/commands/mcp.ts (2)

130-155: Handle empty --env-only value and centralize valid env list

  • If a user passes an empty string (e.g., --env-only ""), envOnly becomes [] and silently disables restriction. Better to error.
  • VALID_ENVIRONMENTS is duplicated here and in install-mcp.ts (and likely tools). Consider a shared constant.

Apply this within this block to reject empty lists:

   if (options.envOnly) {
     // Parse, normalize, and deduplicate environments
     envOnly = Array.from(
       new Set(
         options.envOnly
           .split(',')
           .map(env => env.trim().toLowerCase())
           .filter(Boolean) // Remove empty strings
       )
     );
 
+    if (envOnly.length === 0) {
+      logger.error("Error: --env-only was provided but no environments were parsed.");
+      logger.error("Provide a comma-separated list, e.g. --env-only dev,staging");
+      process.exit(1);
+    }
+
     // Validate environment names
     const validEnvironments = ['dev', 'staging', 'prod', 'preview'];
     const invalidEnvs = envOnly.filter(env => !validEnvironments.includes(env));
     if (invalidEnvs.length > 0) {
       logger.error(`Error: Invalid environment(s): ${invalidEnvs.join(', ')}`);
       logger.error(`Valid environments are: ${validEnvironments.join(', ')}`);
       process.exit(1);
     }

161-165: Passing devOnly is redundant

Since devOnly is always converted to envOnly above, passing devOnly through the context isn’t used. Consider dropping to reduce confusion.

packages/cli-v3/src/commands/install-mcp.ts (1)

211-216: Validate env-only values here to avoid writing broken configs

Currently invalid env names are only rejected by the runtime mcp command. Validate early to fail fast during install and avoid emitting unusable configs. Also reject empty lists.

Apply after this check:

   if (opts.devOnly && opts.envOnly) {
     throw new OutroCommandError(
       "--dev-only and --env-only are mutually exclusive. Please use only one. Consider using --env-only dev instead of --dev-only."
     );
   }
 
+  // Validate envOnly early (consistent with mcp.ts)
+  if (opts.envOnly) {
+    const parsed = Array.from(
+      new Set(
+        opts.envOnly
+          .split(",")
+          .map((e) => e.trim().toLowerCase())
+          .filter(Boolean)
+      )
+    );
+    if (parsed.length === 0) {
+      throw new OutroCommandError(
+        "--env-only was provided but no environments were parsed. Example: --env-only dev,staging"
+      );
+    }
+    const valid = ["dev", "staging", "prod", "preview"];
+    const invalid = parsed.filter((e) => !valid.includes(e));
+    if (invalid.length > 0) {
+      throw new OutroCommandError(
+        `Invalid environment(s): ${invalid.join(", ")}. Valid environments are: ${valid.join(", ")}`
+      );
+    }
+    // Normalize for downstream config emission
+    opts.envOnly = parsed.join(",");
+  }

Also consider extracting VALID_ENVIRONMENTS into a shared constants module to DRY with mcp.ts.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between c152f3f and 180e506.

📒 Files selected for processing (7)
  • packages/cli-v3/src/commands/install-mcp.ts (5 hunks)
  • packages/cli-v3/src/commands/mcp.ts (3 hunks)
  • packages/cli-v3/src/mcp/context.ts (2 hunks)
  • packages/cli-v3/src/mcp/tools.ts (1 hunks)
  • packages/cli-v3/src/mcp/tools/deploys.ts (2 hunks)
  • packages/cli-v3/src/mcp/tools/runs.ts (4 hunks)
  • packages/cli-v3/src/mcp/tools/tasks.ts (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
  • packages/cli-v3/src/mcp/tools.ts
  • packages/cli-v3/src/mcp/tools/tasks.ts
  • packages/cli-v3/src/mcp/tools/deploys.ts
  • packages/cli-v3/src/mcp/tools/runs.ts
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

**/*.{ts,tsx}: Always prefer using isomorphic code like fetch, ReadableStream, etc. instead of Node.js specific code
For TypeScript, we usually use types over interfaces
Avoid enums
No default exports, use function declarations

Files:

  • packages/cli-v3/src/commands/mcp.ts
  • packages/cli-v3/src/commands/install-mcp.ts
  • packages/cli-v3/src/mcp/context.ts
🧬 Code graph analysis (2)
packages/cli-v3/src/commands/mcp.ts (1)
packages/cli-v3/src/mcp/context.ts (2)
  • logger (36-38)
  • McpContext (27-211)
packages/cli-v3/src/commands/install-mcp.ts (1)
packages/cli-v3/src/cli/common.ts (1)
  • OutroCommandError (35-35)
🔇 Additional comments (8)
packages/cli-v3/src/commands/mcp.ts (3)

23-26: Schema additions look correct

envOnly/disableDeployment/readonly added with sensible defaults.


39-53: CLI help text reads well

Clear deprecation note for --dev-only and precise messaging for new flags.


124-129: Good mutual‑exclusivity guard

Early exit with clear error prevents ambiguous state.

packages/cli-v3/src/commands/install-mcp.ts (5)

117-120: Schema additions look good

New flags added as optional align with CLI surface.


143-155: Help text and deprecation messaging are clear

Consistent with mcp.ts.


218-221: Addresses prior regression: gated devOnly prompt when envOnly is set

This fixes the mutual‑exclusivity prompt issue noted earlier. Good catch.


291-305: Args emission precedence is correct

--env-only takes precedence; only one of the two flags will be emitted. Includes disable-deployment and readonly flags. LGTM.


513-527: Config args emission mirrors manual path

Same precedence and flags as handleUnsupportedClientOnly. Consistency is good.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant