feat: add Claude Code marketplace plugin and env var tool filtering#623
Open
mickey-mikey wants to merge 39 commits intotaylorwilsdon:mainfrom
Open
feat: add Claude Code marketplace plugin and env var tool filtering#623mickey-mikey wants to merge 39 commits intotaylorwilsdon:mainfrom
mickey-mikey wants to merge 39 commits intotaylorwilsdon:mainfrom
Conversation
Add marketplace deployment manifests and WORKSPACE_MCP_TOOLS/ WORKSPACE_MCP_TOOL_TIER env var fallbacks so plugin users can restrict tool loading via ~/.claude/settings.json without needing CLI args. - .claude-plugin/plugin.json and marketplace.json - Env var fallbacks in main.py for --tools and --tool-tier args Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds Claude plugin manifests for Changes
Sequence Diagram(s)sequenceDiagram
participant Env as Environment (WORKSPACE_MCP_*)
participant CLI as main.py (CLI)
participant Server as MCP Server
participant HTTP as HTTP endpoint (uvicorn)
participant STDIO as STDIO loop
Env->>CLI: Provide WORKSPACE_MCP_* vars (tools, tool_tier, read_only, permissions, transport, http_port)
CLI->>CLI: Parse args, apply env fallbacks, validate against SERVICE_MODULES / VALID_SERVICES
CLI->>Server: Build service import mapping and runtime config
alt transport == "streamable-http"
CLI->>Server: Start server with streamable-http transport
Server->>HTTP: Serve streamable HTTP endpoints
else stdio transport
CLI->>Server: Prepare stdio runtime
alt WORKSPACE_MCP_HTTP_PORT set and port available
par
Server->>STDIO: run_stdio_async()
and
CLI->>HTTP: Start uvicorn on /mcp
end
else
Server->>STDIO: run() (stdio-only)
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
20 tasks
Use plugin framework's userConfig to prompt users for OAuth client ID and secret, passing them as env vars to the MCP server. Sensitive secret is stored in system keychain. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Without required: true, the plugin framework doesn't prompt for values on enable. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Plugin users configure tools, tool_tier, read_only, and permissions at install time via userConfig prompts. Values pass to main.py as WORKSPACE_MCP_* env vars, read as fallbacks when CLI args aren't set. - plugin.json: 6 userConfig fields (OAuth creds + 4 filtering options) - main.py: env var fallback block after argparse for all 4 filtering options - Boolean read_only uses type: boolean for toggle UI - Empty optional values are ignored (falsy/not-in-valid-set checks) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…stituted Optional fields are skipped at install and can't be substituted in env vars. Making them required ensures the user is prompted and empty values still work (main.py ignores empty env vars). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
![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)
main.py (2)
214-217: Invalid service names are silently ignored.If a user misconfigures
WORKSPACE_MCP_TOOLSwith a typo (e.g.,"gmail,calender,drive"), the invalid entry is silently dropped. Consider logging a debug/warning message for discoverability.🔍 Optional: Add logging for invalid services
if args.tools is None: _env_tools = os.getenv("WORKSPACE_MCP_TOOLS") if _env_tools: - _parsed = [t.strip() for t in _env_tools.split(",") if t.strip() in _VALID_SERVICES] + _raw = [t.strip() for t in _env_tools.split(",") if t.strip()] + _invalid = [t for t in _raw if t not in _VALID_SERVICES] + if _invalid: + logger.debug("Ignoring invalid services from WORKSPACE_MCP_TOOLS: %s", _invalid) + _parsed = [t for t in _raw if t in _VALID_SERVICES] if _parsed: args.tools = _parsed🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@main.py` around lines 214 - 217, The parsing silently drops invalid service names from WORKSPACE_MCP_TOOLS: when splitting _env_tools into _parsed against _VALID_SERVICES and assigning to args.tools, any invalid tokens should be surfaced; modify the block around _env_tools/_parsed/_VALID_SERVICES to compute the invalid_items (tokens present in _env_tools but not in _VALID_SERVICES) and emit a logger.warning or logger.debug with those invalid names and a short hint about valid values, then continue to set args.tools = _parsed as before; reference the variables _env_tools, _parsed, _VALID_SERVICES, and args.tools to locate the change.
208-211: Consider extracting_VALID_SERVICESto avoid duplication.This set duplicates the
choiceslist from the--toolsargument on lines 161-174. If the list of valid services changes, both locations must be updated.♻️ Suggested refactor
Extract the services list to a module-level constant:
+# Valid service names for tool filtering +VALID_SERVICES = frozenset({ + "gmail", "drive", "calendar", "docs", "sheets", "chat", + "forms", "slides", "tasks", "contacts", "search", "appscript", +}) + def main(): ... parser.add_argument( "--tools", nargs="*", - choices=[ - "gmail", "drive", "calendar", ... - ], + choices=sorted(VALID_SERVICES), ... ) ... # Env var fallbacks - _VALID_SERVICES = { - "gmail", "drive", "calendar", "docs", "sheets", "chat", - "forms", "slides", "tasks", "contacts", "search", "appscript", - } if args.tools is None: _env_tools = os.getenv("WORKSPACE_MCP_TOOLS") if _env_tools: - _parsed = [t.strip() for t in _env_tools.split(",") if t.strip() in _VALID_SERVICES] + _parsed = [t.strip() for t in _env_tools.split(",") if t.strip() in VALID_SERVICES]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@main.py` around lines 208 - 211, Extract the duplicated services set into one module-level constant (e.g., SERVICES or AVAILABLE_SERVICES) and use that single symbol both for _VALID_SERVICES and for the choices passed to the --tools argparse argument; update references in the file so _VALID_SERVICES is assigned from that constant and the argparse --tools choices parameter uses the same constant (convert to list/tuple as needed) to eliminate duplication and ensure a single source of truth.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.claude-plugin/plugin.json:
- Around line 23-50: The four filtering properties tools, tool_tier, read_only,
and permissions in the plugin JSON are incorrectly marked "required": true;
update each property (tools, tool_tier, read_only, permissions) to set
"required": false to match their "Leave empty for..." descriptions and other
Claude plugin conventions so these fields are treated as optional.
In `@main.py`:
- Around line 218-221: Tool tier environment variable comparison is
case-sensitive causing values like "CORE" to be ignored; normalize _env_tier
(e.g., call .lower() or .casefold()) before checking membership in
{"core","extended","complete"} and assign the normalized value to args.tool_tier
so it behaves consistently with the read_only normalization used elsewhere (see
args.tool_tier and WORKSPACE_MCP_TOOL_TIER references).
---
Nitpick comments:
In `@main.py`:
- Around line 214-217: The parsing silently drops invalid service names from
WORKSPACE_MCP_TOOLS: when splitting _env_tools into _parsed against
_VALID_SERVICES and assigning to args.tools, any invalid tokens should be
surfaced; modify the block around _env_tools/_parsed/_VALID_SERVICES to compute
the invalid_items (tokens present in _env_tools but not in _VALID_SERVICES) and
emit a logger.warning or logger.debug with those invalid names and a short hint
about valid values, then continue to set args.tools = _parsed as before;
reference the variables _env_tools, _parsed, _VALID_SERVICES, and args.tools to
locate the change.
- Around line 208-211: Extract the duplicated services set into one module-level
constant (e.g., SERVICES or AVAILABLE_SERVICES) and use that single symbol both
for _VALID_SERVICES and for the choices passed to the --tools argparse argument;
update references in the file so _VALID_SERVICES is assigned from that constant
and the argparse --tools choices parameter uses the same constant (convert to
list/tuple as needed) to eliminate duplication and ensure a single source of
truth.
🪄 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: cacb7d9a-08be-46b8-b8ff-f160540a97df
📒 Files selected for processing (3)
.claude-plugin/marketplace.json.claude-plugin/plugin.jsonmain.py
Comment on lines
+23
to
+50
| "tools": { | ||
| "title": "Services to enable", | ||
| "description": "Comma-separated list: gmail, drive, calendar, docs, sheets, chat, forms, slides, tasks, contacts, search, appscript. Leave empty for all.", | ||
| "type": "string", | ||
| "sensitive": false, | ||
| "required": true | ||
| }, | ||
| "tool_tier": { | ||
| "title": "Tool tier", | ||
| "description": "core, extended, or complete. Leave empty for all tools.", | ||
| "type": "string", | ||
| "sensitive": false, | ||
| "required": true | ||
| }, | ||
| "read_only": { | ||
| "title": "Read-only mode", | ||
| "description": "true to restrict to read-only scopes. Leave empty for full access.", | ||
| "type": "string", | ||
| "sensitive": false, | ||
| "required": true | ||
| }, | ||
| "permissions": { | ||
| "title": "Per-service permissions", | ||
| "description": "Space-separated service:level pairs. Example: gmail:send drive:readonly. Gmail levels: readonly, organize, drafts, send, full. Other services: readonly, full. Mutually exclusive with read-only and tools. Leave empty for full access.", | ||
| "type": "string", | ||
| "sensitive": false, | ||
| "required": true | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Claude plugin userConfig required field empty string behavior
💡 Result:
In Claude Code plugins, the userConfig field in the .claude-plugin/plugin.json manifest declares user-configurable values that Claude Code prompts the user to provide when enabling the plugin. Each entry is an object with a description (prompt text) and optional sensitive flag (true for secrets like tokens). No sources explicitly document behavior for required fields or empty strings in userConfig. The schema shows simple string fields without type: "string", minLength, or required indicators, suggesting fields are optional or accept empty strings. Similar Claude Code validation issues (e.g., agents field expecting arrays, tool schemas needing required: []) imply strict Zod/JSON Schema enforcement. If a field were required and empty, it would likely fail validation (error like "Expected string, received undefined" or minLength violation), preventing plugin enablement. However, since userConfig prompts users interactively, empty input may be accepted as "" unless validated otherwise. No bugs report empty string failures specifically for userConfig. To test: Define "myfield": {"description": "Enter value", "sensitive": false}, enable plugin, submit empty—likely succeeds with empty config value, as no evidence of rejection.
Citations:
- 1: https://docs.claude.com/en/docs/claude-code/plugins-reference
- 2: https://code.claude.com/docs/en/plugins-reference
- 3: https://docs.anthropic.com/en/docs/claude-code/plugins
- 4: https://claudelint.com/api/schemas
- 5: http://docs.openclaw.ai/plugins/manifest
- 6: [DOCS]: Agents field as string in plugin manifest causes installation failure anthropics/claude-code#21598
- 7: Claude API rejects tool schemas: missing 'required' field causes 'JSON schema is invalid' error anomalyco/opencode#13618
- 8: [Bug] Plugin TUI validation rejects agents array while CLI install succeeds anthropics/claude-code#13927
- 9: [Bug] Plugin manifest validation rejects unrecognized keys after installation anthropics/claude-code#31384
- 10: Claude Code plugin marketplace fails to load - listAvailablePlugins validation error anthropics/claude-code#33068
🏁 Script executed:
# Check if there are any other plugin.json files or documentation about required fields
find . -type f -name "plugin.json" -o -name "*plugin*" -type f | head -20Repository: taylorwilsdon/google_workspace_mcp
Length of output: 106
🏁 Script executed:
# Search for any documentation or comments about required fields in plugin configuration
rg -i "required" --type json | head -20Repository: taylorwilsdon/google_workspace_mcp
Length of output: 538
🏁 Script executed:
# Check the full structure of the plugin.json file to understand the context better
cat -n .claude-plugin/plugin.jsonRepository: taylorwilsdon/google_workspace_mcp
Length of output: 3029
Change "required": false for optional filtering fields to resolve semantic inconsistency.
The filtering fields (tools, tool_tier, read_only, permissions) are marked "required": true but have descriptions saying "Leave empty for...". This creates a semantic contradiction—optional fields should use "required": false, as shown in other Claude plugin configurations.
Update lines 23–50 to change "required": true to "required": false for these four fields to clarify that they are optional.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.claude-plugin/plugin.json around lines 23 - 50, The four filtering
properties tools, tool_tier, read_only, and permissions in the plugin JSON are
incorrectly marked "required": true; update each property (tools, tool_tier,
read_only, permissions) to set "required": false to match their "Leave empty
for..." descriptions and other Claude plugin conventions so these fields are
treated as optional.
Contributor
Author
There was a problem hiding this comment.
Intentional. When required: false, the plugin framework skips the field at install time and then fails at server startup with "Missing required user configuration value" because ${user_config.*} substitution has no value to inject. Making them required: true with "Leave empty for..." descriptions is the current workaround — users just press Enter to accept the empty default, and main.py treats empty strings as no-ops.
Contributor
There was a problem hiding this comment.
@mickey-mikey, thanks for the detailed explanation — that's a non-obvious but completely valid workaround. The framework's behavior of silently skipping required: false fields and then failing on unresolved ${user_config.*} substitutions at startup makes the required: true + "press Enter for empty" pattern the only practical option here. My original comment was incorrect given this constraint. The code is correct as-is.
✏️ Learnings added
Learnt from: mickey-mikey
Repo: taylorwilsdon/google_workspace_mcp PR: 623
File: .claude-plugin/plugin.json:23-50
Timestamp: 2026-03-28T06:03:32.583Z
Learning: In `.claude-plugin/plugin.json` for Claude Code plugins (google_workspace_mcp repository), optional filtering fields (`tools`, `tool_tier`, `read_only`, `permissions`) are intentionally marked `"required": true` even though their descriptions say "Leave empty for...". This is because the Claude plugin framework skips `required: false` fields at install time, causing `${user_config.*}` variable substitution to fail at server startup with "Missing required user configuration value". The workaround is to mark all fields `required: true` so users are prompted (and can press Enter to submit an empty string), while `main.py` treats empty strings as no-ops. This is a known limitation of the Claude plugin framework's userConfig substitution behavior.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
- Case-insensitive parsing for WORKSPACE_MCP_TOOLS and WORKSPACE_MCP_TOOL_TIER - Log warning for invalid service names in WORKSPACE_MCP_TOOLS - Extract VALID_SERVICES constant shared between argparse and env var fallbacks Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
main.py (1)
49-52: Consider derivingVALID_SERVICESfromtool_importsto prevent drift.If a new service is added to
tool_importsbut not toVALID_SERVICES, users won't be able to specify it via--toolsor env vars. Consider deriving the frozenset from the dict keys:♻️ Optional refactor to prevent drift
Move
VALID_SERVICESdefinition aftertool_importsand derive it:# After tool_imports dict definition around line 328 VALID_SERVICES: frozenset[str] = frozenset(tool_imports.keys())This would require restructuring since
tool_importsis currently insidemain(). Alternatively, extract the service names list to a single location that both reference.Also applies to: 315-328
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@main.py` around lines 49 - 52, VALID_SERVICES is a hardcoded frozenset that can drift from the actual tools defined in tool_imports; move the authoritative list to one place by deriving VALID_SERVICES from tool_imports (or extracting a single SERVICE_NAMES list used by both) so that VALID_SERVICES = frozenset(tool_imports.keys()) (ensure you relocate the VALID_SERVICES definition to after tool_imports or lift tool_imports out of main() so both can reference the same symbol), then update any uses that reference VALID_SERVICES to the new derived constant.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@main.py`:
- Around line 49-52: VALID_SERVICES is a hardcoded frozenset that can drift from
the actual tools defined in tool_imports; move the authoritative list to one
place by deriving VALID_SERVICES from tool_imports (or extracting a single
SERVICE_NAMES list used by both) so that VALID_SERVICES =
frozenset(tool_imports.keys()) (ensure you relocate the VALID_SERVICES
definition to after tool_imports or lift tool_imports out of main() so both can
reference the same symbol), then update any uses that reference VALID_SERVICES
to the new derived constant.
Prevents drift between VALID_SERVICES, argparse choices, and tool_imports. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
Author
|
@coderabbitai resume |
Contributor
✅ Actions performedReviews resumed. |
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Allows plugin users to choose stdio or streamable-http at install time. streamable-http enables workspace-cli access to the running server. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
main.py (3)
319-332: Implicit order coupling betweenSERVICE_NAMESand lambda list.The
dict(zip(...))pattern requires the lambda list order to matchSERVICE_NAMESexactly. While this works correctly now, future edits to either location could silently mismatch services with modules. Consider adding a brief comment or keeping the explicit dict for clarity.💡 Alternative: Retain explicit dict for safety
If readability and maintenance safety are priorities:
tool_imports = { "gmail": lambda: import_module("gmail.gmail_tools"), "drive": lambda: import_module("gdrive.drive_tools"), # ... etc }Or add a comment documenting the coupling:
# NOTE: Lambda order must match SERVICE_NAMES order exactly tool_imports = dict(zip(SERVICE_NAMES, [ lambda: import_module("gmail.gmail_tools"), # ... ]))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@main.py` around lines 319 - 332, The tool_imports dict uses dict(zip(SERVICE_NAMES, [...])) which couples the lambda list order to SERVICE_NAMES and can silently mismatch; change tool_imports to an explicit mapping keyed by service name (e.g. keys like "gmail", "drive") mapping to the corresponding lambda that calls import_module, or at minimum add a clear comment above tool_imports noting that the lambda list order must exactly match SERVICE_NAMES to prevent future maintenance errors; update references to SERVICE_NAMES/tool_imports accordingly.
221-224: Consider adding.strip()for consistency with other env var handling.Other env vars (like
WORKSPACE_MCP_TOOL_TIERat line 214) strip whitespace before checking. While.split()handles leading/trailing whitespace implicitly, adding.strip()before the truthiness check would make the intent clearer and be consistent with the pattern used elsewhere.♻️ Optional consistency fix
if args.permissions is None: - _env_perms = os.getenv("WORKSPACE_MCP_PERMISSIONS") - if _env_perms: + _env_perms = os.getenv("WORKSPACE_MCP_PERMISSIONS", "").strip() + if _env_perms: args.permissions = _env_perms.split()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@main.py` around lines 221 - 224, The env var handling for WORKSPACE_MCP_PERMISSIONS should mirror the pattern used elsewhere by stripping whitespace first: when reading os.getenv("WORKSPACE_MCP_PERMISSIONS") into _env_perms, call _env_perms = _env_perms.strip() (or check truthiness on _env_perms.strip()) before using .split(), and then assign args.permissions = _env_perms.split(); update the block around args.permissions and the _env_perms variable to perform the .strip() so intent and behavior match other vars like WORKSPACE_MCP_TOOL_TIER.
225-227: Transport env var can override explicit--transport stdioCLI argument.The condition
args.transport == "stdio"doesn't distinguish between the argparse default and an explicit CLI argument. If a user runs--transport stdioexplicitly,WORKSPACE_MCP_TRANSPORT=streamable-httpwill still override it. Per PR objectives, CLI arguments should take precedence over env vars.One fix is to check if
--transportwas explicitly provided by inspectingsys.argvor using a sentinel default:🔧 Proposed fix using sentinel default
parser.add_argument( "--transport", choices=["stdio", "streamable-http"], - default="stdio", + default=None, help="Transport mode: stdio (default) or streamable-http", )Then update the fallback logic:
- _env_transport = os.getenv("WORKSPACE_MCP_TRANSPORT", "").strip().lower() - if _env_transport in {"stdio", "streamable-http"} and args.transport == "stdio": - args.transport = _env_transport + if args.transport is None: + _env_transport = os.getenv("WORKSPACE_MCP_TRANSPORT", "").strip().lower() + args.transport = _env_transport if _env_transport in {"stdio", "streamable-http"} else "stdio"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@main.py` around lines 225 - 227, The current logic allows the WORKSPACE_MCP_TRANSPORT env var to override an explicit CLI `--transport stdio` because it only checks args.transport value; change this so env var only applies when the CLI did not explicitly set --transport. Use either inspect sys.argv for the presence of "--transport" or switch args to use a sentinel default (e.g., transport=None) and update argument parsing, then modify the block using _env_transport and args.transport so it only assigns args.transport = _env_transport when the CLI did not explicitly provide --transport; reference symbols: _env_transport, WORKSPACE_MCP_TRANSPORT, args.transport, and sys.argv or the sentinel default in argparse.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@main.py`:
- Around line 319-332: The tool_imports dict uses dict(zip(SERVICE_NAMES,
[...])) which couples the lambda list order to SERVICE_NAMES and can silently
mismatch; change tool_imports to an explicit mapping keyed by service name (e.g.
keys like "gmail", "drive") mapping to the corresponding lambda that calls
import_module, or at minimum add a clear comment above tool_imports noting that
the lambda list order must exactly match SERVICE_NAMES to prevent future
maintenance errors; update references to SERVICE_NAMES/tool_imports accordingly.
- Around line 221-224: The env var handling for WORKSPACE_MCP_PERMISSIONS should
mirror the pattern used elsewhere by stripping whitespace first: when reading
os.getenv("WORKSPACE_MCP_PERMISSIONS") into _env_perms, call _env_perms =
_env_perms.strip() (or check truthiness on _env_perms.strip()) before using
.split(), and then assign args.permissions = _env_perms.split(); update the
block around args.permissions and the _env_perms variable to perform the
.strip() so intent and behavior match other vars like WORKSPACE_MCP_TOOL_TIER.
- Around line 225-227: The current logic allows the WORKSPACE_MCP_TRANSPORT env
var to override an explicit CLI `--transport stdio` because it only checks
args.transport value; change this so env var only applies when the CLI did not
explicitly set --transport. Use either inspect sys.argv for the presence of
"--transport" or switch args to use a sentinel default (e.g., transport=None)
and update argument parsing, then modify the block using _env_transport and
args.transport so it only assigns args.transport = _env_transport when the CLI
did not explicitly provide --transport; reference symbols: _env_transport,
WORKSPACE_MCP_TRANSPORT, args.transport, and sys.argv or the sentinel default in
argparse.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7250c096-cba9-4dde-bc2c-21bce75c98c9
📒 Files selected for processing (2)
.claude-plugin/plugin.jsonmain.py
🚧 Files skipped from review as they are similar to previous changes (1)
- .claude-plugin/plugin.json
- Use sentinel default (None) for --transport so CLI args take precedence over WORKSPACE_MCP_TRANSPORT env var - Add .strip() to WORKSPACE_MCP_PERMISSIONS for consistency - Add order coupling comment to tool_imports dict Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
main.py (1)
49-54: Collapse the service registry to one source of truth.The note at Line 319 is documenting a fragile invariant rather than enforcing it.
SERVICE_NAMES/VALID_SERVICESnow drive CLI and env validation, while the actual importer registry lives in a separate ordered list. If one side changes without the other, the process can accept a service name that has no matching importer. Defining the importer map once and derivingSERVICE_NAMES/VALID_SERVICESfrom it would remove that maintenance trap.Also applies to: 167-170, 318-333
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@main.py` around lines 49 - 54, Replace the two separate registries by making the importer registry the single source of truth: locate the importer registry (the ordered list/mapper of service name -> importer) and keep that as the canonical structure, then derive SERVICE_NAMES (tuple/sequence) and VALID_SERVICES (frozenset) programmatically from its keys; update any CLI/env validation code to reference VALID_SERVICES and any code expecting SERVICE_NAMES to use the derived tuple so names cannot drift from the importer map. Ensure the importer registry retains its ordering (use an ordered dict or list of tuples if order is important) and remove the hardcoded SERVICE_NAMES/VALID_SERVICES definitions so only the importer map is edited when adding/removing services.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@main.py`:
- Around line 202-227: The current env-var parsing can silently widen access
when non-empty malformed values are provided; update the checks for
WORKSPACE_MCP_TOOLS, WORKSPACE_MCP_TOOL_TIER, WORKSPACE_MCP_READ_ONLY and
WORKSPACE_MCP_TRANSPORT so that non-empty but invalid values cause a fail-closed
behavior (log an error and exit) instead of falling back to permissive defaults:
for args.tools, if _env_tools is non-empty and _parsed is empty while _raw
contains values, call logger.error and exit(1) rather than leaving args.tools as
None; for args.tool_tier, if _env_tier is non-empty and not in
{"core","extended","complete"}, error and exit; for args.read_only, if _env_ro
is non-empty and not in {"true","1","yes","false","0","no"} treat it as invalid,
log and exit; for args.transport, if _env_transport is non-empty and not in
{"stdio","streamable-http"} log and exit — keep the empty-string/no-op behavior
intact so only non-empty malformed values trigger the fail-closed path.
---
Nitpick comments:
In `@main.py`:
- Around line 49-54: Replace the two separate registries by making the importer
registry the single source of truth: locate the importer registry (the ordered
list/mapper of service name -> importer) and keep that as the canonical
structure, then derive SERVICE_NAMES (tuple/sequence) and VALID_SERVICES
(frozenset) programmatically from its keys; update any CLI/env validation code
to reference VALID_SERVICES and any code expecting SERVICE_NAMES to use the
derived tuple so names cannot drift from the importer map. Ensure the importer
registry retains its ordering (use an ordered dict or list of tuples if order is
important) and remove the hardcoded SERVICE_NAMES/VALID_SERVICES definitions so
only the importer map is edited when adding/removing services.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Plugin now starts server in HTTP mode and connects via URL, enabling both Claude Code and workspace-cli to use the same server instance. - mcpServers uses command+url pattern (start process, connect via HTTP) - Hardcode --transport streamable-http in args - Add OAUTHLIB_INSECURE_TRANSPORT=1 for localhost - Remove transport from userConfig (always HTTP now) - Fix CLI precedence: --transport uses sentinel default (None) - Add .strip() to WORKSPACE_MCP_PERMISSIONS for consistency Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Claude Code requires "type": "http" to connect via streamable-http URL instead of defaulting to stdio. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
In stdio mode, the server now also starts an HTTP listener on port 8001 so workspace-cli can connect to the same process. Both transports share the same tools, permissions, and auth state. - Plugin uses standard stdio command config (all plugins do this) - Background uvicorn serves http_app() on port+1 via asyncio.gather - If port is busy, HTTP silently skips (stdio still works) - Reverted plugin.json to stdio (removed type:http experiment) - Fix CLI precedence: --transport uses sentinel default - Add .strip() to WORKSPACE_MCP_PERMISSIONS for consistency Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
HTTP listener only starts when WORKSPACE_MCP_HTTP_PORT is set. Plugin.json sets it to 8001; CLI users are unaffected. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Non-empty but invalid env vars now error and exit instead of silently falling back to permissive defaults (security: prevents access widening) - Replace dual SERVICE_NAMES/tool_imports registries with single SERVICE_MODULES dict as source of truth - Derive SERVICE_NAMES, VALID_SERVICES, and tool_imports from it Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
Author
|
@coderabbitai full review |
Contributor
✅ Actions performedFull review triggered. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@main.py`:
- Line 567: The code converts _cli_port to int directly (http_port =
int(_cli_port)), which will raise an unhandled ValueError for non-numeric
WORKSPACE_MCP_HTTP_PORT; update the parsing to validate _cli_port before
conversion (e.g., check str.isdigit() or use a try/except around int(_cli_port))
and raise or log a clear, user-friendly error indicating invalid
WORKSPACE_MCP_HTTP_PORT and expected numeric port, ensuring the variable names
http_port and _cli_port are used in the fix and that failure path exits or falls
back safely.
🪄 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: e6b15414-5096-4b72-bac8-383666c29881
📒 Files selected for processing (3)
.claude-plugin/marketplace.json.claude-plugin/plugin.jsonmain.py
Non-numeric values now fail closed with a clear error instead of an unhandled ValueError traceback. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
Author
|
@coderabbitai resume |
Contributor
✅ Actions performedReviews resumed. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
main.py (1)
573-573: Add a return annotation to_run_dual.
async def _run_dual() -> None:is enough here.As per coding guidelines, "Use mandatory type hints (mypy strict), f-strings (avoid % formatting), and async/await; limit time.sleep."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@main.py` at line 573, Add an explicit return type annotation to the async helper by changing the signature of _run_dual to async def _run_dual() -> None:, and ensure the function does not return any value (replace any bare returns with return None or remove them) so it satisfies mypy strict typing and async/await conventions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@main.py`:
- Around line 573-592: The stdio/http run loop hangs because asyncio.gather
waits for both coroutines but uvicorn.Server.serve() keeps running after
server.run_stdio_async() completes; modify _run_dual to start the HTTP serve as
a background task (create_task on http_srv.serve()), await the stdio task
returned by server.run_stdio_async(), then signal the HTTP server to stop (set
http_srv.should_exit = True or call its graceful shutdown) and await the HTTP
task to finish before exiting; references: _run_dual, server.run_stdio_async,
uvicorn.Server(config) -> http_srv, http_srv.serve, and
asyncio.create_task/await.
- Around line 217-248: Before applying environment fallbacks, record which
mutually exclusive options were explicitly provided on the CLI (e.g., detect
presence of "--permissions", "--tools", "--read-only", "--tool-tier" in sys.argv
or via argparse 'parser.parse_known_args' metadata) and use that snapshot to
skip any env-branch that would conflict; specifically, if args.permissions was
supplied on the CLI, do not apply WORKSPACE_MCP_TOOLS or WORKSPACE_MCP_READ_ONLY
fallbacks to set args.tools or args.read_only, and if
args.tools/args.read_only/args.tool_tier were supplied on the CLI, do not apply
WORKSPACE_MCP_PERMISSIONS; update the branches that set args.tools,
args.tool_tier, args.read_only, and args.permissions to first check the recorded
CLI-supplied set and return early when a conflict would overwrite a CLI-provided
value.
---
Nitpick comments:
In `@main.py`:
- Line 573: Add an explicit return type annotation to the async helper by
changing the signature of _run_dual to async def _run_dual() -> None:, and
ensure the function does not return any value (replace any bare returns with
return None or remove them) so it satisfies mypy strict typing and async/await
conventions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
… shutdown - Don't apply WORKSPACE_MCP_READ_ONLY env var when --permissions was passed on CLI, and vice versa, preventing conflicting mode errors. - Replace asyncio.gather with create_task for HTTP server so the process exits when stdio completes instead of hanging. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
main.py (2)
693-706: HTTP task exceptions silently dropped; consider storing and checking the task.The HTTP server task created at line 698 is never awaited or stored. If
http_srv.serve()raises an exception during operation, it will be silently ignored (Python logs "Task exception was never retrieved" to stderr). Additionally, settingshould_exit = Truedoesn't guarantee the server has fully shut down before the function returns.🔧 Proposed fix to handle HTTP task lifecycle
http_srv = None + http_task = None if http_available: app = server.http_app(path="/mcp") config = uvicorn.Config(app, host=host, port=http_port, log_level="warning") http_srv = uvicorn.Server(config) - asyncio.create_task(http_srv.serve()) + http_task = asyncio.create_task(http_srv.serve()) safe_print(f" workspace-cli endpoint: http://{host}:{http_port}/mcp") await server.run_stdio_async() if http_srv: http_srv.should_exit = True + if http_task: + try: + await asyncio.wait_for(http_task, timeout=5.0) + except asyncio.TimeoutError: + http_task.cancel() + except Exception: + pass # HTTP errors non-critical in stdio mode🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@main.py` around lines 693 - 706, The HTTP server task created with asyncio.create_task(http_srv.serve()) in _run_dual is never stored or awaited, so exceptions from http_srv.serve() are lost and shutdown may not complete; fix by assigning the task to a variable (e.g., http_task), attach exception handling (await http_task or add a done callback to log/propagate exceptions), and on shutdown set http_srv.should_exit = True and then await http_task (or call await http_srv.wait_for_exit() if available) after await server.run_stdio_async() to ensure the server has fully stopped; update references to http_srv, http_task, server.http_app, uvicorn.Server, and _run_dual accordingly.
263-265:_cli_has_toolscaptured but never used; env permissions fallback missing guard.
_cli_has_toolsis captured at line 263 but never referenced. TheWORKSPACE_MCP_PERMISSIONSfallback at line 295 only checksnot _cli_has_read_only, so running--tools gmailwithWORKSPACE_MCP_PERMISSIONS=drive:fullwould apply the env permissions, then fail at line 317 with a confusing error implying the user combined the flags.Consider also guarding against
_cli_has_toolsfor consistency with how_cli_has_permissionsguardsWORKSPACE_MCP_READ_ONLY:🔧 Proposed fix
- if args.permissions is None and not _cli_has_read_only: + if args.permissions is None and not _cli_has_read_only and not _cli_has_tools: _env_perms = os.getenv("WORKSPACE_MCP_PERMISSIONS", "").strip() if _env_perms: args.permissions = _env_perms.split()Also applies to: 295-298
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@main.py` around lines 263 - 265, The code currently captures _cli_has_tools but never uses it, and the WORKSPACE_MCP_PERMISSIONS env fallback is only guarded by _cli_has_read_only; update the guard logic so env fallbacks are only applied when the corresponding CLI flags were not provided: use _cli_has_permissions to gate applying WORKSPACE_MCP_PERMISSIONS and use _cli_has_read_only to gate applying WORKSPACE_MCP_READ_ONLY, and also ensure _cli_has_tools is considered (i.e., do not apply env permissions if the user passed --tools) or remove _cli_has_tools if you decide it’s unnecessary; adjust the conditional(s) around reading WORKSPACE_MCP_PERMISSIONS/WORKSPACE_MCP_READ_ONLY to reference _cli_has_permissions and _cli_has_tools/_cli_has_read_only appropriately.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@main.py`:
- Around line 693-706: The HTTP server task created with
asyncio.create_task(http_srv.serve()) in _run_dual is never stored or awaited,
so exceptions from http_srv.serve() are lost and shutdown may not complete; fix
by assigning the task to a variable (e.g., http_task), attach exception handling
(await http_task or add a done callback to log/propagate exceptions), and on
shutdown set http_srv.should_exit = True and then await http_task (or call await
http_srv.wait_for_exit() if available) after await server.run_stdio_async() to
ensure the server has fully stopped; update references to http_srv, http_task,
server.http_app, uvicorn.Server, and _run_dual accordingly.
- Around line 263-265: The code currently captures _cli_has_tools but never uses
it, and the WORKSPACE_MCP_PERMISSIONS env fallback is only guarded by
_cli_has_read_only; update the guard logic so env fallbacks are only applied
when the corresponding CLI flags were not provided: use _cli_has_permissions to
gate applying WORKSPACE_MCP_PERMISSIONS and use _cli_has_read_only to gate
applying WORKSPACE_MCP_READ_ONLY, and also ensure _cli_has_tools is considered
(i.e., do not apply env permissions if the user passed --tools) or remove
_cli_has_tools if you decide it’s unnecessary; adjust the conditional(s) around
reading WORKSPACE_MCP_PERMISSIONS/WORKSPACE_MCP_READ_ONLY to reference
_cli_has_permissions and _cli_has_tools/_cli_has_read_only appropriately.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…r task - Add `_cli_has_tools` check to WORKSPACE_MCP_PERMISSIONS env fallback so explicit --tools on the CLI prevents env permissions from being applied - Store asyncio task for http_srv.serve() and await it with a 5s timeout after signaling should_exit, preventing silent exception loss and ensuring clean shutdown Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
main.py (1)
683-714: Consider wrapping the entire coroutine in try/finally for robust cleanup.If
server.run_stdio_async()raises an exception, the HTTP server cleanup code (lines 704-712) won't execute. While the outerexcept Exceptionat line 724 does callcleanup_oauth_callback_server()and thensys.exit(1), thehttp_taskis technically orphaned until process exit.This is unlikely to cause issues in practice (process exits anyway), but a try/finally would make the cleanup more robust and explicit.
♻️ Proposed refactor for explicit cleanup
async def _run_dual() -> None: """Run stdio and HTTP transports concurrently.""" http_available = True try: with socket.socket(socket.AF_INET, socket.SOCK_STREAM) as s: s.bind((host, http_port)) except OSError: logger.warning("Port %d in use, workspace-cli HTTP endpoint unavailable", http_port) http_available = False http_srv = None http_task = None if http_available: app = server.http_app(path="/mcp") config = uvicorn.Config(app, host=host, port=http_port, log_level="warning") http_srv = uvicorn.Server(config) http_task = asyncio.create_task(http_srv.serve()) safe_print(f" workspace-cli endpoint: http://{host}:{http_port}/mcp") - await server.run_stdio_async() - - if http_srv: - http_srv.should_exit = True - if http_task: - try: - await asyncio.wait_for(http_task, timeout=5.0) - except (asyncio.TimeoutError, asyncio.CancelledError): - http_task.cancel() - except Exception: - pass # HTTP errors non-critical in stdio mode + try: + await server.run_stdio_async() + finally: + if http_srv: + http_srv.should_exit = True + if http_task: + try: + await asyncio.wait_for(http_task, timeout=5.0) + except (asyncio.TimeoutError, asyncio.CancelledError): + http_task.cancel() + except Exception: + pass # HTTP errors non-critical in stdio mode🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@main.py` around lines 683 - 714, Wrap the core of async def _run_dual() in a try/finally so that regardless of exceptions from server.run_stdio_async() you always run the HTTP shutdown/cleanup: in the finally block set http_srv.should_exit (if http_srv), await/wait_for http_task with a short timeout and cancel if needed, and swallow expected asyncio.TimeoutError/CancelledError; ensure http_task.cancel() is called and awaited where appropriate so the http_task is not orphaned. Target symbols: _run_dual, server.run_stdio_async, http_srv, http_task (and reuse the existing exception-handling logic currently after server.run_stdio_async inside the finally).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@main.py`:
- Around line 683-714: Wrap the core of async def _run_dual() in a try/finally
so that regardless of exceptions from server.run_stdio_async() you always run
the HTTP shutdown/cleanup: in the finally block set http_srv.should_exit (if
http_srv), await/wait_for http_task with a short timeout and cancel if needed,
and swallow expected asyncio.TimeoutError/CancelledError; ensure
http_task.cancel() is called and awaited where appropriate so the http_task is
not orphaned. Target symbols: _run_dual, server.run_stdio_async, http_srv,
http_task (and reuse the existing exception-handling logic currently after
server.run_stdio_async inside the finally).
If run_stdio_async() raises, the HTTP server shutdown and task await now still execute via the finally block, preventing the sidecar from being orphaned on unexpected exits. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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
2 participants
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.
Description
Add Claude Code marketplace plugin manifests (
.claude-plugin/) andWORKSPACE_MCP_*env var fallbacks in main.py so plugin users can configure OAuth credentials and tool filtering at install time viauserConfigprompts.userConfig fields (all prompted at install)
google_oauth_client_idgoogle_oauth_client_secrettoolstool_tierread_onlypermissionsSee #589 for the companion skill PR.
Type of Change
Testing
Test plan
/plugin marketplace add mickey-mikey/google_workspace_mcp#feat/plugin-marketplace/plugin install google-workspace-mcp@google-workspace-mcpstart_google_authgmail,drivefor tools loads only Gmail + Drivecorefor tool_tier loads only core tiertruefor read_only enables read-only modegmail:send drive:readonlyfor permissionsChecklist
Additional Notes
All
userConfigfields are markedrequired: trueintentionally — whenrequired: false, the plugin framework skips the field at install and${user_config.*}substitution fails at server startup. Users press Enter on optional fields to accept the empty default; main.py treats empty strings as no-ops.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Chores