feat: bundle comments and file ignore #511
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds optional dependency Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant CLI as Deploy CLI
participant Main as deploy_config/_deploy_with_retry
participant WW as wrangler_wrapper
participant BU as bundle_utils
participant FS as Filesystem
participant Cloud as Deployer
User->>CLI: run deploy [--ignore | --ignore-file]
CLI->>Main: deploy_config(..., ignore_path)
Main->>WW: wrangler_deploy(..., ignore_file=ignore_path)
WW->>FS: read project directory
WW->>BU: create_pathspec_from_gitignore(ignore_file)
BU-->>WW: PathSpec | None
rect rgba(200,230,255,0.25)
WW->>FS: copytree(project -> temp, ignore=adapter)
WW->>BU: should_ignore_by_gitignore(path, names, project_dir, spec)
BU-->>WW: names to skip
WW->>FS: skip or copy entries accordingly
end
WW->>FS: compute workspace fingerprint (apply same ignore set)
WW->>Cloud: upload bundle + metadata
Cloud-->>WW: result
WW-->>Main: return status
Main-->>CLI: report result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
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 |
alienzach
force-pushed
the
09-18-feat_bundle_comments_and_file_ignore
branch
from
0bfa67b to
a7d6259
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR enhances deployment security by adding support for .gitignore patterns and comment cleaning to prevent sensitive information from being included in deployed bundles. The implementation respects gitignore rules during file copying and automatically removes comments from Python and YAML files that might contain secrets.
- Added gitignore pattern support using the
pathspeclibrary for consistent file exclusion - Implemented comment and docstring cleaning for Python and YAML files to prevent secret leakage
- Enhanced
wrangler_deployto apply both gitignore filtering and comment cleaning with user feedback
Reviewed Changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| bundle_utils.py | New module with gitignore parsing, comment cleaning utilities for Python/YAML files |
| wrangler_wrapper.py | Enhanced deploy function to use gitignore patterns and clean comments before bundling |
| test_wrangler_wrapper.py | Comprehensive tests for new bundle utilities and deploy enhancements |
| pyproject.toml | Added pathspec dependency for gitignore pattern matching |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pyproject.toml (1)
79-83: Makepathspeca required dependency or lazy-import it to avoid runtime ImportError
- Problem: src/mcp_agent/cli/cloud/commands/deploy/bundle_utils.py performs a top-level
import pathspecand src/mcp_agent/cli/cloud/commands/deploy/wrangler_wrapper.py imports bundle_utils at module import time; pyproject.toml listspathspeconly under[project.optional-dependencies].cli, so importing these modules without installing[cli]extras will raise ImportError at runtime.- Fix (pick one): move
"pathspec>=0.12.1"from[project.optional-dependencies].cliinto[project].dependenciesin pyproject.toml; OR change bundle_utils.py to lazy-importpathspecinside create_pathspec_from_gitignore (wrap import in try/except ImportError, return None or raise a clear error when gitignore support is requested).- Optional: if keeping it optional, update README to recommend
pip install "mcp-agent[cli]".
🧹 Nitpick comments (3)
src/mcp_agent/cli/cloud/commands/deploy/bundle_utils.py (2)
28-65: Unused helper and weak return typing
create_gitignore_matcherisn’t used. Either use it inwrangler_wrapperor remove it. Also, return type should beCallable[[Path, str], bool]instead ofcallable.Apply this diff for typing:
-from typing import Optional, Set +from typing import Optional, Set, Callable @@ -def create_gitignore_matcher(project_dir: Path) -> callable: +def create_gitignore_matcher(project_dir: Path) -> Callable[[Path, str], bool]:
67-105: Normalize paths for cross‑platform gitignore matchingUse POSIX paths;
PathSpecpatterns are POSIX‑style. This avoids Windows separator mismatches.Apply this diff:
-def should_ignore_by_gitignore(path_str: str, names: list, project_dir: Path, spec: Optional[pathspec.PathSpec]) -> Set[str]: +def should_ignore_by_gitignore(path_str: str, names: list, project_dir: Path, spec: Optional[PathSpecT]) -> Set[str]: @@ - rel_path_str = str(rel_path) + rel_path_str = rel_path.as_posix()tests/cli/commands/test_wrangler_wrapper.py (1)
1060-1093: Avoid monkey‑patching Path.is_dir globallyOverwriting
Path.is_dircan leak across tests under parallel runs. Usemonkeypatchfixture to patchPath.is_dirwithin scope.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
pyproject.toml(1 hunks)src/mcp_agent/cli/cloud/commands/deploy/bundle_utils.py(1 hunks)src/mcp_agent/cli/cloud/commands/deploy/wrangler_wrapper.py(3 hunks)tests/cli/commands/test_wrangler_wrapper.py(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/mcp_agent/cli/cloud/commands/deploy/wrangler_wrapper.py (2)
src/mcp_agent/cli/cloud/commands/deploy/bundle_utils.py (4)
create_pathspec_from_gitignore(10-25)should_ignore_by_gitignore(67-104)clean_yaml_files_in_directory(253-297)clean_python_files_in_directory(300-328)src/mcp_agent/cli/utils/ux.py (1)
print_info(32-49)
tests/cli/commands/test_wrangler_wrapper.py (1)
src/mcp_agent/cli/cloud/commands/deploy/bundle_utils.py (6)
create_pathspec_from_gitignore(10-25)should_ignore_by_gitignore(67-104)clean_yaml_comments(107-126)clean_python_comments(129-250)clean_yaml_files_in_directory(253-297)clean_python_files_in_directory(300-328)
🔇 Additional comments (9)
src/mcp_agent/cli/cloud/commands/deploy/wrangler_wrapper.py (2)
165-168: Good: parse .gitignore once and reuseCreating the PathSpec once avoids repeated parsing during copy. Looks solid.
254-265: Nice touch: include secrets example in workspace hash ignoreAvoids non-functional diffs in fingerprints. LGTM.
src/mcp_agent/cli/cloud/commands/deploy/bundle_utils.py (1)
300-328: Directory cleaners: good error toleranceSkips unreadable files and only writes on change. Nice.
tests/cli/commands/test_wrangler_wrapper.py (6)
7-7: Good: bring in mock_open for pathspec testsImports look appropriate.
928-1023: Great coverage for secrets and example exclusionAsserts both real and example secrets are excluded from temp and original remain intact. LGTM.
1033-1056: Nice: verifies PathSpec parsing and patternsSolid unit test for
.gitignoreparsing behavior.
1095-1156: Python cleaner test hits the right casesValidates docstring and inline comment removal while preserving string
#. Once cleaner is fixed (no extra pass), expectations remain valid.Please re-run this test after applying the
clean_python_commentsfix to ensure no regressions.
1247-1297: Good end‑to‑end for .gitignore honoringConfirms ignored artifacts aren’t copied and included files are processed. Nice.
1300-1356: Useful assertions on cleaning + UX messagingCatches both actual content sanitization and the info logs. 👍
There was a problem hiding this comment.
+1 to @rholinshead's review comments.
In addition, I want to understand what the motivation for this change is. Why exclude things based on gitignore patterns? What if they are needed for the agent to run/function? For example, what if I have gitignored the deployed.secrets.yaml file?
alienzach
force-pushed
the
09-18-feat_bundle_comments_and_file_ignore
branch
from
ca4da7b to
6c94486
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
pyproject.toml(1 hunks)src/mcp_agent/cli/cloud/commands/deploy/bundle_utils.py(1 hunks)src/mcp_agent/cli/cloud/commands/deploy/wrangler_wrapper.py(3 hunks)tests/cli/commands/test_wrangler_wrapper.py(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- pyproject.toml
- src/mcp_agent/cli/cloud/commands/deploy/wrangler_wrapper.py
🧰 Additional context used
🧬 Code graph analysis (1)
tests/cli/commands/test_wrangler_wrapper.py (2)
src/mcp_agent/cli/cloud/commands/deploy/bundle_utils.py (6)
create_pathspec_from_gitignore(10-25)should_ignore_by_gitignore(67-104)clean_yaml_comments(107-126)clean_python_comments(129-250)clean_yaml_files_in_directory(253-297)clean_python_files_in_directory(300-328)src/mcp_agent/cli/cloud/commands/deploy/wrangler_wrapper.py (1)
wrangler_deploy(116-358)
🔇 Additional comments (1)
src/mcp_agent/cli/cloud/commands/deploy/bundle_utils.py (1)
190-251: Stop truncating multi-line Python literals when stripping commentsAfter
ast.unparse, thecleaned_codestring already has comments removed. The extra manual pass starting on Line 193 resetsin_stringfor every physical line. When the unparser emits a multi-line literal (e.g.,return """line1\n# keep\nline3"""), the middle line is processed within_string = False, so the leading#is treated as a comment and the entire line is dropped. This corrupts legitimate string content before deployment.Apply this diff so we rely on
ast.unparse(which preserves literals) and fall back to the original source if the parse fails:- # Compile back to code - cleaned_code = ast.unparse(tree) - - # Now remove inline comments (lines starting with #) - lines = [] - for line in cleaned_code.split('\n'): - # Remove lines that are only comments - stripped = line.lstrip() - if stripped and not stripped.startswith('#'): - # Remove inline comments from the line - # Find # not inside strings - in_string = False - string_char = None - cleaned_line = [] - i = 0 - while i < len(line): - char = line[i] - - # Handle string boundaries - if char in ('"', "'") and (i == 0 or line[i-1] != '\\'): - if not in_string: - in_string = True - string_char = char - elif char == string_char: - in_string = False - - # If we find # outside a string, truncate here - if char == '#' and not in_string: - # Remove trailing whitespace - result = ''.join(cleaned_line).rstrip() - if result: # Only add non-empty lines - lines.append(result) - break - - cleaned_line.append(char) - i += 1 - else: - # No comment found, add the whole line if non-empty - if line.strip(): - lines.append(line.rstrip()) - - return '\n'.join(lines) + return ast.unparse(tree) @@ - except SyntaxError: - # If parsing fails, at least try to remove simple comments - lines = [] - for line in python_content.split('\n'): - stripped = line.lstrip() - if stripped and not stripped.startswith('#'): - # Try to remove inline comments (basic approach) - if '#' in line: - # Find first # not in a string (simple heuristic) - parts = line.split('#') - if parts[0].strip(): - lines.append(parts[0].rstrip()) - else: - lines.append(line.rstrip()) - elif not stripped.startswith('#'): - # Keep empty lines for structure - lines.append('') - return '\n'.join(lines) + except SyntaxError: + # On parse failure, keep the original source to avoid corrupting code + return python_content
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/mcp_agent/cli/cloud/commands/deploy/main.py (1)
299-307: Pass effective ignore path derived from flagsIntegrate new flags: explicit --ignore-file takes precedence; otherwise use .mcpacignore when --ignore is set.
Apply this diff:
app = run_async( _deploy_with_retry( app_id=app_id, api_key=effective_api_key, project_dir=config_dir, mcp_app_client=mcp_app_client, retry_count=retry_count, - ignore=ignore, + ignore_file=( + ignore_file + or (config_dir / ".mcpacignore" if use_ignore else None) + ), ) )
🧹 Nitpick comments (1)
src/mcp_agent/cli/cloud/commands/deploy/bundle_utils.py (1)
61-62: Minor typing nit: prefer Sequence[str] for namesMore accurate and flexible than list for the shutil.copytree callback.
Apply this diff:
-def should_ignore_by_gitignore( - path_str: str, names: list, project_dir: Path, spec: Optional[PathSpecT] +from typing import Sequence + +def should_ignore_by_gitignore( + path_str: str, names: Sequence[str], project_dir: Path, spec: Optional[PathSpecT] ) -> Set[str]:
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
docs/cli-reference.mdx(3 hunks)docs/cloud/overview.mdx(1 hunks)src/mcp_agent/cli/cloud/commands/deploy/bundle_utils.py(1 hunks)src/mcp_agent/cli/cloud/commands/deploy/main.py(4 hunks)src/mcp_agent/cli/cloud/commands/deploy/wrangler_wrapper.py(5 hunks)tests/cli/commands/test_wrangler_wrapper.py(6 hunks)
✅ Files skipped from review due to trivial changes (2)
- docs/cloud/overview.mdx
- docs/cli-reference.mdx
🚧 Files skipped from review as they are similar to previous changes (2)
- src/mcp_agent/cli/cloud/commands/deploy/wrangler_wrapper.py
- tests/cli/commands/test_wrangler_wrapper.py
🔇 Additional comments (1)
src/mcp_agent/cli/cloud/commands/deploy/bundle_utils.py (1)
14-14: Make pathspec optional: lazy import + safe type hints to avoid hard dependencyTop‑level import forces the optional dependency at import time and annotations reference it eagerly. Degrade gracefully when pathspec isn’t installed.
Apply this diff:
@@ -from pathlib import Path -from typing import Optional, Set -import pathspec +from pathlib import Path +from typing import Optional, Set, TYPE_CHECKING, Any + +if TYPE_CHECKING: + import pathspec as _pathspec + PathSpecT = _pathspec.PathSpec +else: + PathSpecT = Any @@ -def create_pathspec_from_gitignore(ignore_file_path: Path) -> Optional[pathspec.PathSpec]: +def create_pathspec_from_gitignore(ignore_file_path: Path) -> Optional[PathSpecT]: @@ - with open(ignore_file_path, "r", encoding="utf-8") as f: - spec = pathspec.PathSpec.from_lines("gitwildmatch", f) + try: + import pathspec as _pathspec # type: ignore + except Exception: + return None + with open(ignore_file_path, "r", encoding="utf-8") as f: + spec = _pathspec.PathSpec.from_lines("gitwildmatch", f) @@ -def should_ignore_by_gitignore( - path_str: str, names: list, project_dir: Path, spec: Optional[pathspec.PathSpec] -) -> Set[str]: +def should_ignore_by_gitignore( + path_str: str, names: list, project_dir: Path, spec: Optional[PathSpecT] +) -> Set[str]:Also applies to: 17-18, 33-36, 41-43
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
src/mcp_agent/cli/cloud/commands/deploy/wrangler_wrapper.py (2)
114-116: Docstring: add missing ignore_file parameterThe Args section doesn’t document ignore_file. Recommend updating to clarify accepted files (e.g., .mcpacignore or .gitignore) and precedence.
Outside-diff snippet for the docstring:
Args: app_id (str): The application ID. api_key (str): User MCP Agent Cloud API key. project_dir (Path): The directory of the project to deploy. ignore_file (Path | None): Optional path to an ignore file (gitignore syntax). If provided, its patterns are applied in addition to default excludes.
165-178: Fallback to project .gitignore when no explicit ignore_file is providedToday we only apply ignore patterns if ignore_file is passed. To “respect .gitignore patterns,” fall back to project_dir/.gitignore when ignore_file is None. Keep current logging and defaults if no file exists.
Apply:
- # Load ignore rules (gitignore syntax) only if an explicit ignore file is provided - ignore_spec = ( - create_pathspec_from_gitignore(ignore_file) if ignore_file else None - ) - if ignore_file: - if ignore_spec is None: - print_warning( - f"Ignore file '{ignore_file}' not found; applying default excludes only" - ) - else: - print_info(f"Using ignore patterns from {ignore_file}") - else: - print_info("No ignore file provided; applying default excludes only") + # Load ignore rules (gitignore syntax). If no explicit file, try project .gitignore + if ignore_file: + ignore_spec = create_pathspec_from_gitignore(ignore_file) + if ignore_spec is None: + print_warning( + f"Ignore file '{ignore_file}' not found; applying default excludes only" + ) + else: + print_info(f"Using ignore patterns from {ignore_file}") + else: + default_ignore = project_dir / ".gitignore" + ignore_spec = create_pathspec_from_gitignore(default_ignore) + if ignore_spec: + print_info(f"Using ignore patterns from {default_ignore}") + else: + print_info("No ignore file provided; applying default excludes only")Additionally, consider using pathspec.GitIgnoreSpec in create_pathspec_from_gitignore to get full gitignore semantics (including negations). This can simplify trailing-slash handling in should_ignore_by_gitignore.
tests/cli/commands/test_wrangler_wrapper.py (3)
924-1020: Add coverage to exclude .env by defaultOnce .env is excluded, add a focused test to assert it’s not copied to temp.
Proposed test:
def test_wrangler_deploy_env_file_exclusion(): with tempfile.TemporaryDirectory() as temp_dir: project_path = Path(temp_dir) (project_path / "main.py").write_text("from mcp_agent_cloud import MCPApp\napp = MCPApp(name='test')") (project_path / ".env").write_text("API_KEY=secret\n") def check_env_excluded(*args, **kwargs): temp_project_dir = Path(kwargs["cwd"]) assert not (temp_project_dir / ".env").exists() return MagicMock(returncode=0) with patch("subprocess.run", side_effect=check_env_excluded): wrangler_deploy("test-app", "test-api-key", project_path)
1025-1049: Consider testing gitignore negations and anchored dir patternsIf create_pathspec_from_gitignore switches to GitIgnoreSpec, add a negation case (e.g., *.log then !keep.log) and a dir pattern case (dir/). This guards semantics users expect from .gitignore.
1052-1085: Use pytest.monkeypatch instead of overriding Path.is_dir globallyDirectly replacing Path.is_dir is risky. Prefer monkeypatch for safer, scoped patching.
Example:
def test_should_ignore_by_gitignore(monkeypatch): import pathspec gitignore_content = "*.log\n*.pyc\nnode_modules/\n" spec = pathspec.PathSpec.from_lines('gitwildmatch', gitignore_content.splitlines()) project_dir = Path("/fake/project") names = ["test.log", "main.py", "node_modules", "config.yaml", "test.pyc"] def fake_is_dir(p: Path) -> bool: return p.name in {"node_modules"} monkeypatch.setattr(Path, "is_dir", fake_is_dir, raising=True) ignored = should_ignore_by_gitignore(str(project_dir), names, project_dir, spec) assert {"test.log", "test.pyc", "node_modules"} <= ignored
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/mcp_agent/cli/cloud/commands/deploy/wrangler_wrapper.py(4 hunks)tests/cli/commands/test_wrangler_wrapper.py(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
tests/cli/commands/test_wrangler_wrapper.py (1)
src/mcp_agent/cli/cloud/commands/deploy/bundle_utils.py (2)
create_pathspec_from_gitignore(17-36)should_ignore_by_gitignore(40-77)
src/mcp_agent/cli/cloud/commands/deploy/wrangler_wrapper.py (2)
src/mcp_agent/cli/cloud/commands/deploy/bundle_utils.py (2)
create_pathspec_from_gitignore(17-36)should_ignore_by_gitignore(40-77)src/mcp_agent/cli/utils/ux.py (2)
print_warning(66-77)print_info(32-49)
🔇 Additional comments (8)
src/mcp_agent/cli/cloud/commands/deploy/wrangler_wrapper.py (5)
21-24: LGTM: imports for ignore utilitiesImports are scoped and minimal. Good separation via bundle_utils.
194-199: Confirm precedence: hardcoded excludes shadow ignore_file re-includesBecause default excludes are applied first, ignore_file negations (e.g., !.somefile) cannot re-include those entries. If you want ignore_file to be authoritative, invert precedence or support a small allowlist override.
233-260: LGTM: clear, stable logging of bundled filesNice touch mapping .mcpac.py back to original names and sorting for stable output.
183-191: Exclude .env to avoid secret leakage (remove whitelist)The current logic explicitly whitelists .env, risking secret exposure. Exclude all dotfiles (including .env) by default.
Apply:
- for name in names: - if (name.startswith(".") and name not in {".env"}) or name in { + for name in names: + if name.startswith(".") or name in { "logs", "__pycache__", "node_modules", "venv", MCP_SECRETS_FILENAME, # Exclude mcp_agent.secrets.yaml only }: ignored.add(name)
114-116: Call sites updated for new ignore_file parameter
Verified all internal callers (CLI wiring and tests) either passignore_fileexplicitly or rely on its defaultNone. No further changes required.tests/cli/commands/test_wrangler_wrapper.py (3)
20-23: LGTM: importing bundle utils for targeted testsGood modularization of tests around the new ignore utilities.
924-1020: LGTM: secrets file exclusion testCovers the live secrets file correctly while allowing the example file.
1087-1137: LGTM: ignore_file respected end-to-endGood scenario coverage verifying ignores aren’t copied and non-ignored files are processed.
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
docs/cli-reference.mdx (1)
277-283: Clarify default ignore-file locationDocs say patterns are evaluated relative to
--config-dir, but--ignorecurrently picks.mcpacignorefrom the current working directory. That’s confusing when--config-dirpoints elsewhere. Please update the docs (or the implementation) so the default file location and evaluation root align.tests/cli/commands/test_deploy_command.py (1)
148-205: Default ignore path should follow config directoryYou only assert the bare
--ignoreforwardsPath(".mcpacignore"). This passes because the testchdirs into the config dir, but that hides a behavioural mismatch: the CLI always uses the caller’s CWD, whilewrangler_deploylater resolves the ignore file againstproject_dir. Add a test where--config-dirdiffers from CWD to document—and ideally rework—the intended behaviour.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
docs/cli-reference.mdx(3 hunks)docs/cloud/overview.mdx(1 hunks)src/mcp_agent/cli/cloud/commands/deploy/bundle_utils.py(1 hunks)src/mcp_agent/cli/cloud/commands/deploy/main.py(4 hunks)tests/cli/commands/test_deploy_command.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/mcp_agent/cli/cloud/commands/deploy/bundle_utils.py
- docs/cloud/overview.mdx
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-07-22T18:59:49.368Z
Learnt from: CR
PR: lastmile-ai/mcp-agent#0
File: examples/usecases/reliable_conversation/CLAUDE.md:0-0
Timestamp: 2025-07-22T18:59:49.368Z
Learning: Applies to examples/usecases/reliable_conversation/examples/reliable_conversation/src/**/*.py : Use mcp-agent's Agent abstraction for ALL LLM interactions, including quality evaluation, to ensure consistent tool access, logging, and error handling.
Applied to files:
docs/cli-reference.mdx
🧬 Code graph analysis (2)
src/mcp_agent/cli/cloud/commands/deploy/main.py (1)
src/mcp_agent/cli/core/utils.py (1)
run_async(12-27)
tests/cli/commands/test_deploy_command.py (2)
src/mcp_agent/cli/mcp_app/api_client.py (2)
get_app_id_by_name(286-308)create_app(134-164)src/mcp_agent/cli/mcp_app/mock_client.py (2)
get_app_id_by_name(36-45)create_app(86-124)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: update_release_draft
- GitHub Check: update_release_draft
🔇 Additional comments (1)
src/mcp_agent/cli/cloud/commands/deploy/main.py (1)
339-365: Propagate resolved ignore pathAfter updating the default above, keep
_deploy_with_retryreceiving an already-resolved ignore path; no code change needed here onceignore_pathis fixed.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
tests/cli/commands/test_deploy_command.py (1)
121-137: Pass config_dir as string to Typer for consistencyClick/Typer expects CLI args as strings. Other tests use str(temp_config_dir); mirror that here to avoid incidental type issues.
Apply this diff:
- temp_config_dir, + str(temp_config_dir),tests/cli/commands/test_wrangler_wrapper.py (1)
1052-1085: Avoid monkeypatching pathlib.Path globally; use pytest monkeypatchDirectly assigning Path.is_dir risks leakage if restoration fails. Prefer the monkeypatch fixture for safer, scoped patching.
Apply this diff to use monkeypatch:
-def test_should_ignore_by_gitignore(): +def test_should_ignore_by_gitignore(monkeypatch): @@ - original_is_dir = Path.is_dir - Path.is_dir = lambda self: self.name in ["node_modules", "temp", "build"] - - try: - ignored = should_ignore_by_gitignore(current_path, names, project_dir, spec) - finally: - # Restore original method - Path.is_dir = original_is_dir + monkeypatch.setattr( + Path, + "is_dir", + lambda self: self.name in ["node_modules", "temp", "build"], + raising=True, + ) + ignored = should_ignore_by_gitignore(current_path, names, project_dir, spec)src/mcp_agent/cli/cloud/commands/deploy/main.py (1)
346-367: Rename _deploy_with_retry param to ignore_file for clarityAligns with wrangler_deploy’s keyword, avoids confusion between boolean flag vs path.
Apply this diff:
-async def _deploy_with_retry( +async def _deploy_with_retry( @@ - ignore: Optional[Path], + ignore_file: Optional[Path], ): @@ - wrangler_deploy( + wrangler_deploy( app_id=app_id, api_key=api_key, project_dir=project_dir, - ignore_file=ignore, + ignore_file=ignore_file, )And update the call site above:
- ignore=ignore_path, + ignore_file=ignore_path,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/mcp_agent/cli/cloud/commands/deploy/main.py(4 hunks)tests/cli/commands/test_deploy_command.py(2 hunks)tests/cli/commands/test_wrangler_wrapper.py(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
tests/cli/commands/test_wrangler_wrapper.py (2)
src/mcp_agent/cli/cloud/commands/deploy/bundle_utils.py (2)
create_pathspec_from_gitignore(17-36)should_ignore_by_gitignore(40-78)src/mcp_agent/cli/cloud/commands/deploy/wrangler_wrapper.py (1)
wrangler_deploy(114-387)
tests/cli/commands/test_deploy_command.py (2)
src/mcp_agent/cli/mcp_app/api_client.py (2)
get_app_id_by_name(286-308)create_app(134-164)src/mcp_agent/cli/mcp_app/mock_client.py (2)
get_app_id_by_name(36-45)create_app(86-124)
src/mcp_agent/cli/cloud/commands/deploy/main.py (1)
src/mcp_agent/cli/core/utils.py (1)
run_async(12-27)
🔇 Additional comments (12)
tests/cli/commands/test_deploy_command.py (5)
77-79: Help text assertions for new flags look goodValidates presence of --ignore and --ignore-file in help output.
138-146: Basic deploy flow assertions are fineExit code and key log messages are asserted; wrangler_deploy is isolated via patch.
148-203: Default ignore file selection behavior is correctly validatedBare --ignore uses config_dir/.mcpacignore and passes its absolute path through.
205-262: Custom --ignore-file propagation test is solidVerifies resolved absolute path is forwarded to bundler.
264-323: Precedence test is correctExplicit --ignore-file overrides the default from --ignore.
tests/cli/commands/test_wrangler_wrapper.py (5)
20-23: Good coverage for ignore utilitiesImporting and testing create_pathspec_from_gitignore and should_ignore_by_gitignore improves confidence in bundling behavior.
925-951: Secrets example file handling coveredEnsures live secrets are excluded and the example file is included and renamed with .mcpac.py.
972-1014: Strong assertions for secrets exclusionClear checks that the live secrets file is not copied or transformed while example and other YAML files are processed.
1025-1040: PathSpec creation test is appropriateMocks file existence and validates typical patterns.
1087-1138: Ignore-file integration test looks correctAffirms Wrangler respects gitignore-like patterns and only copies/renames allowed files.
src/mcp_agent/cli/cloud/commands/deploy/main.py (2)
300-309: Effective ignore path resolution is correctPrecedence of explicit --ignore-file over default .mcpacignore in config_dir is handled cleanly; only passes default if it exists.
107-121: Help text says CWD, but behavior uses config_dirThe default ignore file is resolved against config_dir (correct). Update help to match and consider validating existence for --ignore-file.
Apply this diff:
- ignore: bool = typer.Option( - False, - "--ignore", - help="Use .mcpacignore in CWD to ignore files", - ), + ignore: bool = typer.Option( + False, + "--ignore", + help="Use .mcpacignore in the config directory to ignore files", + ), @@ - ignore_file: Optional[Path] = typer.Option( + ignore_file: Optional[Path] = typer.Option( None, "--ignore-file", - help="Specify custom ignore file (gitignore syntax)", - exists=False, + help="Specify custom ignore file (gitignore syntax).", + exists=True, readable=True, dir_okay=False, file_okay=True, resolve_path=True, ),
There was a problem hiding this comment.
Looks good in general but a couple things:
- from UX perspective, seems weird to have two different
ignoreoptions; I would recommend just the one for overriding ignore file path and otherwise use.mcpacignoreif it exists in the project dir - I don't think we want to tell the user about any 'internal' files included in the deployment since they are implementation details and will just be confusing
Also a comment on the logs dir handling, which may make more sense in a followup
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
tests/cli/commands/test_deploy_command.py (7)
121-135: Pass strings torunner.invokefor consistency.Click/Typer expects string args. Other tests cast to
str(temp_config_dir); do the same here to avoid surprises.Apply this diff:
result = runner.invoke( app, [ "deploy", MOCK_APP_NAME, "--config-dir", - temp_config_dir, + str(temp_config_dir), "--api-url", "http://test-api.com", "--api-key", "test-api-key", "--non-interactive", # Prevent prompting for input ], )
160-163: Make the wrangler capture resilient to signature changes and normalize paths.Capture by position is brittle if the function signature changes; also normalize to resolved Path to avoid type mismatches in assertions.
Apply this diff:
- def _capture_wrangler(app_id, api_key, project_dir, ignore_file=None): - captured["ignore_file"] = ignore_file - return MOCK_APP_ID + def _capture_wrangler(*args, **kwargs): + ignore_file = kwargs.get("ignore_file") or (args[3] if len(args) > 3 else None) + captured["ignore_file"] = Path(ignore_file).resolve() if ignore_file else None + return MOCK_APP_ID
217-220: Repeat resilient capture pattern here.Mirror the robust
*args, **kwargscapture and path normalization to avoid brittle positional assumptions.Apply this diff:
- def _capture_wrangler(app_id, api_key, project_dir, ignore_file=None): - captured["ignore_file"] = ignore_file - return MOCK_APP_ID + def _capture_wrangler(*args, **kwargs): + ignore_file = kwargs.get("ignore_file") or (args[3] if len(args) > 3 else None) + captured["ignore_file"] = Path(ignore_file).resolve() if ignore_file else None + return MOCK_APP_ID
273-276: Repeat resilient capture pattern here as well.Same rationale: future-proof and normalize paths.
Apply this diff:
- def _capture_wrangler(app_id, api_key, project_dir, ignore_file=None): - captured["ignore_file"] = ignore_file - return MOCK_APP_ID + def _capture_wrangler(*args, **kwargs): + ignore_file = kwargs.get("ignore_file") or (args[3] if len(args) > 3 else None) + captured["ignore_file"] = Path(ignore_file).resolve() if ignore_file else None + return MOCK_APP_ID
315-316: Make the assertion robust to string vs Path.If upstream ever passes a string, this still works after normalizing in the capture. Alternatively, coerce at assertion time.
Optionally coerce in the assertion:
- assert captured.get("ignore_file") == custom_ignore.resolve() + assert Path(captured.get("ignore_file")).resolve() == custom_ignore.resolve()
334-337: Repeat resilient capture pattern here too.Keep capture consistent across tests.
Apply this diff:
- def _capture_wrangler(app_id, api_key, project_dir, ignore_file=None): - captured["ignore_file"] = ignore_file - return MOCK_APP_ID + def _capture_wrangler(*args, **kwargs): + ignore_file = kwargs.get("ignore_file") or (args[3] if len(args) > 3 else None) + captured["ignore_file"] = Path(ignore_file).resolve() if ignore_file else None + return MOCK_APP_ID
319-377: Add tests for edge cases: invalid path and both flags set.
- Invalid
--ignore-filepath should produce a clear error and non‑zero exit.- When both
--ignoreand--ignore-fileare provided, verify precedence (custom path wins).I can draft these tests mirroring your existing patterns. Want me to add them?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
docs/cli-reference.mdx(3 hunks)docs/cloud/overview.mdx(1 hunks)src/mcp_agent/cli/cloud/commands/deploy/main.py(4 hunks)tests/cli/commands/test_deploy_command.py(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- docs/cli-reference.mdx
🚧 Files skipped from review as they are similar to previous changes (2)
- src/mcp_agent/cli/cloud/commands/deploy/main.py
- docs/cloud/overview.mdx
🧰 Additional context used
🧬 Code graph analysis (1)
tests/cli/commands/test_deploy_command.py (2)
src/mcp_agent/cli/mcp_app/api_client.py (2)
get_app_id_by_name(286-308)create_app(134-164)src/mcp_agent/cli/mcp_app/mock_client.py (2)
get_app_id_by_name(36-45)create_app(86-124)
🔇 Additional comments (2)
tests/cli/commands/test_deploy_command.py (2)
77-79: Consider asserting presence of--ignoreas well (if supported).Help checks cover
--ignore-fileand.mcpacignore. If the CLI also exposes--ignore(per PR summary), add an assertion for it to prevent regressions. If auto-detection replaced the flag, ignore this.Based on PR objectives
199-201: Assertion stays valid after path normalization.With the proposed capture change,
captured["ignore_file"]is a resolved Path; the equality withdefault_ignore.resolve()remains correct.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/mcp_agent/cli/cloud/commands/deploy/wrangler_wrapper.py (1)
114-137: Update docstring to document ignore_file parameter.The function signature now includes
ignore_file: Path | None = None, but the docstring doesn't describe this parameter.Apply this diff:
def wrangler_deploy( app_id: str, api_key: str, project_dir: Path, ignore_file: Path | None = None ) -> None: """Bundle the MCP Agent using Wrangler. A thin wrapper around the Wrangler CLI to bundle the MCP Agent application code and upload it our internal cf storage. Some key details here: - We copy the user's project to a temporary directory and perform all operations there - Secrets file must be excluded from the bundle - We must add a temporary `wrangler.toml` to the project directory to set python_workers compatibility flag (CLI arg is not sufficient). - Python workers with a `requirements.txt` file cannot be published by Wrangler, so we must rename any `requirements.txt` file to `requirements.txt.mcpac.py` before bundling - Non-python files (e.g. `uv.lock`, `poetry.lock`, `pyproject.toml`) would be excluded by default due to no py extension, so they are renamed with a `.mcpac.py` extension. - We exclude .venv directories from the copy to avoid bundling issues. Args: app_id (str): The application ID. api_key (str): User MCP Agent Cloud API key. project_dir (Path): The directory of the project to deploy. + ignore_file (Path | None): Optional path to a gitignore-style file for excluding files from the bundle. """
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/mcp_agent/cli/cloud/commands/deploy/main.py(4 hunks)src/mcp_agent/cli/cloud/commands/deploy/wrangler_wrapper.py(4 hunks)tests/cli/commands/test_deploy_command.py(3 hunks)tests/cli/commands/test_wrangler_wrapper.py(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/mcp_agent/cli/cloud/commands/deploy/main.py
🧰 Additional context used
🧬 Code graph analysis (3)
tests/cli/commands/test_wrangler_wrapper.py (2)
src/mcp_agent/cli/cloud/commands/deploy/bundle_utils.py (2)
create_pathspec_from_gitignore(17-36)should_ignore_by_gitignore(40-78)src/mcp_agent/cli/cloud/commands/deploy/wrangler_wrapper.py (1)
wrangler_deploy(114-378)
tests/cli/commands/test_deploy_command.py (1)
src/mcp_agent/cli/mcp_app/mock_client.py (2)
get_app_id_by_name(36-45)create_app(86-124)
src/mcp_agent/cli/cloud/commands/deploy/wrangler_wrapper.py (2)
src/mcp_agent/cli/cloud/commands/deploy/bundle_utils.py (2)
create_pathspec_from_gitignore(17-36)should_ignore_by_gitignore(40-78)src/mcp_agent/cli/utils/ux.py (2)
print_warning(66-77)print_info(32-49)
🪛 GitHub Actions: Pull Request Checks
tests/cli/commands/test_wrangler_wrapper.py
[error] 7-7: Ruff check: unused import 'unittest.mock.mock_open' detected (F401).
[error] 22-22: Ruff check: unused import 'create_pathspec_from_gitignore' detected (F401).
🔇 Additional comments (10)
tests/cli/commands/test_deploy_command.py (5)
77-78: LGTM! Help text validation updated correctly.The assertions properly verify that the new
--ignore-fileoption and.mcpacignorereferences appear in the help output.
393-446: LGTM! Auto-detection test validates default ignore behavior.The test correctly verifies that a
.mcpacignorefile in the project directory is automatically detected and passed towrangler_deployvia theignore_fileparameter.
449-503: LGTM! No-ignore test validates fallback behavior.The test correctly verifies that when no
.mcpacignoreexists and no--ignore-fileis provided,Noneis passed towrangler_deploy.
506-562: LGTM! Custom ignore-file test validates CLI option.The test correctly verifies that an explicit
--ignore-filepath is resolved to an absolute path and passed through towrangler_deploy.
565-622: LGTM! Precedence test validates override behavior.The test correctly verifies that an explicit
--ignore-filetakes precedence over the default.mcpacignoredetection, which matches the documented behavior.tests/cli/commands/test_wrangler_wrapper.py (3)
926-1021: LGTM! Secrets exclusion test updated with example file handling.The test now correctly verifies that:
- The live
mcp_agent.secrets.yamlis excluded from the bundle- The
mcp_agent.secrets.yaml.examplefile is included and transformed to.mcpac.py- Other YAML files are processed normally
This aligns with the updated bundling behavior.
1024-1056: Verify Path.is_dir mocking approach.The test monkey-patches
Path.is_dirglobally, which could affect other tests if exceptions occur. Consider using a context manager orpatchto limit the scope.Apply this refactor for safer mocking:
- # Mock Path.is_dir method properly - original_is_dir = Path.is_dir - Path.is_dir = lambda self: self.name in ["node_modules", "temp", "build"] - - try: - ignored = should_ignore_by_gitignore(current_path, names, project_dir, spec) - finally: - # Restore original method - Path.is_dir = original_is_dir + with patch.object(Path, 'is_dir', lambda self: self.name in ["node_modules", "temp", "build"]): + ignored = should_ignore_by_gitignore(current_path, names, project_dir, spec)
1058-1109: LGTM! Ignore-file integration test validates end-to-end behavior.The test correctly verifies that:
- Files matching ignore patterns are excluded from the temp directory
- Files not matching patterns are copied and transformed to
.mcpac.py- The ignore file is passed through to
wrangler_deploysrc/mcp_agent/cli/cloud/commands/deploy/wrangler_wrapper.py (2)
165-177: LGTM! Ignore spec loading and logging are clear.The logic correctly:
- Only loads ignore patterns when an explicit ignore file is provided
- Warns if the ignore file doesn't exist
- Logs the ignore source (file path or default excludes only)
232-250: LGTM! Bundled files logging improves transparency.The new logging:
- Computes and displays the list of original files being bundled
- Excludes internal helpers (wrangler.toml, mcp_deploy_breadcrumb.py)
- Maps
.mcpac.pyfiles back to their original names- Sorts the list for deterministic output
This greatly improves deployment observability.
| def ignore_patterns(path_str, names): | ||
| ignored = set() | ||
|
|
||
| # Keep existing hardcoded exclusions (highest priority) | ||
| for name in names: | ||
| if (name.startswith(".") and name not in {".env"}) or name in { | ||
| "logs", | ||
| "__pycache__", | ||
| "node_modules", | ||
| "venv", | ||
| MCP_SECRETS_FILENAME, | ||
| MCP_SECRETS_FILENAME, # Exclude mcp_agent.secrets.yaml only | ||
| }: | ||
| ignored.add(name) | ||
|
|
||
| # Apply explicit ignore file patterns (if provided) | ||
| spec_ignored = should_ignore_by_gitignore( | ||
| path_str, names, project_dir, ignore_spec | ||
| ) | ||
| ignored.update(spec_ignored) | ||
|
|
||
| return ignored |
There was a problem hiding this comment.
🧩 Analysis chain
Verify .env exclusion for secret safety.
Line 185 whitelists .env by excluding all dotfiles except .env. This could leak secrets commonly stored in .env files.
Based on past review comments, this was flagged before. Should .env be excluded by default? If .env needs to be bundled for specific use cases, consider documenting why.
🏁 Script executed:
#!/bin/bash
# Check if .env is mentioned in documentation or tests as intentionally bundled
rg -n "\.env" docs/ README.md --type md -C2Length of output: 760
🏁 Script executed:
#!/bin/bash
# Check if .env is in .gitignore (standard practice)
if [ -f .gitignore ]; then
echo "=== .gitignore contents related to .env ==="
rg -n "\.env" .gitignore
else
echo ".gitignore not found"
fiLength of output: 52
🏁 Script executed:
#!/bin/bash
# Find and examine the should_ignore_by_gitignore function implementation
echo "=== Searching for should_ignore_by_gitignore function ==="
ast-grep --pattern 'def should_ignore_by_gitignore($$$) {
$$$
}'Length of output: 57
🏁 Script executed:
#!/bin/bash
# Look for any tests or documentation about deployment exclusions
echo "=== Searching for deployment-related tests ==="
rg -n "ignore_patterns|\.env" --type py -g '*test*' -C3Length of output: 14340
🏁 Script executed:
#!/bin/bash
# Check for any other references to .env handling in deployment code
echo "=== Searching for .env in deployment-related code ==="
rg -n "\.env" src/mcp_agent/cli/cloud/commands/deploy/ --type py -C2Length of output: 1517
🏁 Script executed:
#!/bin/bash
# Get the full test context around line 412 to see what's actually being tested
echo "=== Full test context for .env handling ==="
rg -n "def test.*" tests/cli/commands/test_wrangler_wrapper.py -A 50 | rg -A 50 "Check that hidden files"Length of output: 2543
🏁 Script executed:
#!/bin/bash
# Look for any assertion about .env in the test file
echo "=== Searching for .env assertions in wrangler tests ==="
rg -n "\.env" tests/cli/commands/test_wrangler_wrapper.py -B2 -A2Length of output: 257
🏁 Script executed:
#!/bin/bash
# Check if the test fixture creates a .env file
echo "=== Searching for .env in test fixtures and conftest ==="
rg -n "\.env" tests/cli/commands/test_wrangler_wrapper.py tests/cli/commands/conftest.py -B3 -A3Length of output: 763
🏁 Script executed:
#!/bin/bash
# Look for the complex_project_structure fixture definition
echo "=== Finding complex_project_structure fixture ==="
ast-grep --pattern 'def complex_project_structure($$$):
$$$'Length of output: 4461
CRITICAL: Remove .env from deployment whitelist to prevent secret leakage.
Line 185 explicitly whitelists .env files for deployment by excluding all dotfiles except .env. This creates a critical security vulnerability:
.envis gitignored, confirming it contains secrets not meant for version control- README states
.envis for local development and "In production, preferMCP_APP_SETTINGS_PRELOADto avoid writing plaintext secrets to disk" .envcontains API keys likeOPENAI_API_KEYandANTHROPIC_API_KEY
If a developer has a local .env file (common during development), it will be bundled into the Cloudflare Workers deployment, exposing production secrets.
Required fix:
if name.startswith(".") or name in {
"logs",
"__pycache__",
"node_modules",
"venv",
MCP_SECRETS_FILENAME,
}:
ignored.add(name)Remove .env from the exception list—all dotfiles should be excluded from deployment.
🤖 Prompt for AI Agents
In src/mcp_agent/cli/cloud/commands/deploy/wrangler_wrapper.py around lines 180
to 200, the current logic explicitly whitelists “.env” by excluding all dotfiles
except ".env", which risks bundling local secret files into deployments; change
the condition so that all dotfiles are treated as ignored (remove ".env" from
the exception list) and update the surrounding comment to reflect that all
dotfiles are excluded to prevent secret leakage; ensure the ignored.add(name)
branch triggers for any name that starts with "." (no exceptions) while
preserving the other hardcoded exclusions (logs, __pycache__, node_modules,
venv, MCP_SECRETS_FILENAME).
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/mcp_agent/cli/cloud/commands/deploy/wrangler_wrapper.py (1)
114-138: Enhance docstring with gitignore pattern details.The
ignore_fileparameter description is present but could be more detailed. Consider expanding the docstring to explain gitignore pattern syntax and precedence with default excludes.Apply this diff to enhance the docstring:
"""Bundle the MCP Agent using Wrangler. A thin wrapper around the Wrangler CLI to bundle the MCP Agent application code and upload it our internal cf storage. + + The `ignore_file` parameter supports gitignore-style patterns for excluding files + and directories from the bundle. When provided, these patterns are evaluated in + addition to the default hardcoded exclusions (dotfiles, logs, __pycache__, etc.). Some key details here:
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/mcp_agent/cli/cloud/commands/deploy/wrangler_wrapper.py(5 hunks)tests/cli/commands/test_wrangler_wrapper.py(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/cli/commands/test_wrangler_wrapper.py
🧰 Additional context used
🧬 Code graph analysis (1)
src/mcp_agent/cli/cloud/commands/deploy/wrangler_wrapper.py (2)
src/mcp_agent/cli/cloud/commands/deploy/bundle_utils.py (2)
create_pathspec_from_gitignore(17-36)should_ignore_by_gitignore(40-78)src/mcp_agent/cli/utils/ux.py (2)
print_warning(66-77)print_info(32-49)
🔇 Additional comments (4)
src/mcp_agent/cli/cloud/commands/deploy/wrangler_wrapper.py (4)
21-24: LGTM!Clean imports for the new gitignore-style ignore functionality.
166-178: LGTM!The ignore file loading logic correctly handles all cases (not provided, provided but missing, and provided and exists) with appropriate logging.
233-251: LGTM!Excellent observability improvement. The bundled files logging provides clear visibility into what's being deployed, correctly excludes internal helpers, and properly handles renamed files. Sorted output ensures stable, reproducible logs.
181-201: CRITICAL: Remove.envfrom deployment bundle to prevent secret leakage.Line 186 still whitelists
.envfiles for deployment, creating a critical security vulnerability that was flagged in multiple past reviews. If a developer has a local.envfile (common during development), it will be bundled into the Cloudflare Workers deployment, exposing production secrets like API keys.The
.envfile:
- Is gitignored, confirming it contains secrets not meant for version control
- Contains API keys like
OPENAI_API_KEYandANTHROPIC_API_KEY(per README)- Should never be deployed per README: "In production, prefer
MCP_APP_SETTINGS_PRELOAD"Apply this diff to exclude
.envfrom deployments:# Keep existing hardcoded exclusions (highest priority) for name in names: - if (name.startswith(".") and name not in {".env"}) or name in { + if name.startswith(".") or name in { "logs", "__pycache__", "node_modules", "venv", MCP_SECRETS_FILENAME, # Exclude mcp_agent.secrets.yaml only }: ignored.add(name)
| rel_path = full_path.relative_to(project_dir) | ||
| # Normalize to POSIX separators so patterns work cross-platform (Windows too) | ||
| rel_path_str = rel_path.as_posix() | ||
|
|
||
| # Match files exactly; for directories also try with a trailing slash | ||
| # to respect patterns like `build/`. | ||
| if spec.match_file(rel_path_str): | ||
| ignored.add(name) | ||
| elif full_path.is_dir() and spec.match_file(rel_path_str + "/"): | ||
| ignored.add(name) | ||
| except ValueError: | ||
| # If `full_path` is not under `project_dir`, ignore matching is skipped. | ||
| continue |
There was a problem hiding this comment.
The try/except block catches all ValueError exceptions, but it appears the intention is to only handle the specific case where relative_to() fails when a path isn't under the project directory. Consider narrowing the exception handling to immediately follow the relative_to() call:
try:
rel_path = full_path.relative_to(project_dir)
except ValueError:
# Skip paths not under project_dir
continue
# Normalize to POSIX separators so patterns work cross-platform (Windows too)
rel_path_str = rel_path.as_posix()
# ...rest of the matching logic...This would prevent silently ignoring other potential ValueError exceptions from the path manipulation or matching operations, making debugging easier.
| rel_path = full_path.relative_to(project_dir) | |
| # Normalize to POSIX separators so patterns work cross-platform (Windows too) | |
| rel_path_str = rel_path.as_posix() | |
| # Match files exactly; for directories also try with a trailing slash | |
| # to respect patterns like `build/`. | |
| if spec.match_file(rel_path_str): | |
| ignored.add(name) | |
| elif full_path.is_dir() and spec.match_file(rel_path_str + "/"): | |
| ignored.add(name) | |
| except ValueError: | |
| # If `full_path` is not under `project_dir`, ignore matching is skipped. | |
| continue | |
| try: | |
| rel_path = full_path.relative_to(project_dir) | |
| except ValueError: | |
| # If `full_path` is not under `project_dir`, ignore matching is skipped. | |
| continue | |
| # Normalize to POSIX separators so patterns work cross-platform (Windows too) | |
| rel_path_str = rel_path.as_posix() | |
| # Match files exactly; for directories also try with a trailing slash | |
| # to respect patterns like `build/`. | |
| if spec.match_file(rel_path_str): | |
| ignored.add(name) | |
| elif full_path.is_dir() and spec.match_file(rel_path_str + "/"): | |
| ignored.add(name) |
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/mcp_agent/cli/cloud/commands/deploy/wrangler_wrapper.py (1)
189-210: CRITICAL: Remove.envfrom whitelist to prevent secret leakage.Line 195 explicitly whitelists
.envby excluding all dotfiles except.env. This creates a critical security vulnerability:
.envfiles commonly contain secrets (API keys, tokens, passwords)- The project's
.gitignoreexcludes.env, confirming it's sensitive- README states
.envis for local development and "In production, preferMCP_APP_SETTINGS_PRELOADto avoid writing plaintext secrets to disk"- Example
.envcontainsOPENAI_API_KEYandANTHROPIC_API_KEYIf a developer has a local
.envfile during development, it will be bundled and deployed to Cloudflare Workers, exposing production secrets.Past review comments flagged this exact issue, which was supposedly addressed in commit ce313b3, but the whitelist remains in the current code.
Apply this diff to remove the whitelist exception:
# Keep existing hardcoded exclusions (highest priority) for name in names: - if (name.startswith(".") and name not in {".env"}) or name in { + if name.startswith(".") or name in { "logs", "__pycache__", "node_modules", "venv", MCP_SECRETS_FILENAME, # Exclude mcp_agent.secrets.yaml only }: ignored.add(name)
🧹 Nitpick comments (2)
src/mcp_agent/cli/cloud/commands/deploy/main.py (1)
383-383: Parameter naming:ignorevsignore_fileinconsistency.The parameter is named
ignore: Optional[Path]here but calledignore_fileat the call site (line 403) and in the downstreamwrangler_deploysignature. While this works (positional matching), consistent naming improves readability.Consider renaming to match the rest of the chain:
async def _deploy_with_retry( app_id: str, api_key: str, project_dir: Path, mcp_app_client: MCPAppClient, retry_count: int, - ignore: Optional[Path], + ignore_file: Optional[Path], ):And update the call:
wrangler_deploy( app_id=app_id, api_key=api_key, project_dir=project_dir, - ignore_file=ignore, + ignore_file=ignore_file, )src/mcp_agent/cli/cloud/commands/deploy/wrangler_wrapper.py (1)
114-116: Good API extension: ignore_file parameter with sensible default.The signature cleanly extends
wrangler_deploywith an optionalignore_fileparameter (defaulting toNonefor backward compatibility). The docstring mentions the new parameter, though it could be more detailed about precedence and gitignore syntax.Consider expanding the docstring parameter section to mention gitignore syntax and precedence:
Args: app_id (str): The application ID. api_key (str): User MCP Agent Cloud API key. project_dir (Path): The directory of the project to deploy. - ignore_file (Path | None): Optional path to a gitignore-style file for excluding files from the bundle. + ignore_file (Path | None): Optional path to a gitignore-style ignore file. + Patterns are evaluated relative to project_dir. When None, only hardcoded excludes apply.Also applies to: 137-137
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
pyproject.toml(1 hunks)src/mcp_agent/cli/cloud/commands/deploy/main.py(4 hunks)src/mcp_agent/cli/cloud/commands/deploy/wrangler_wrapper.py(5 hunks)tests/cli/commands/test_deploy_command.py(3 hunks)tests/cli/commands/test_wrangler_wrapper.py(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/cli/commands/test_deploy_command.py
- pyproject.toml
🧰 Additional context used
🧬 Code graph analysis (3)
src/mcp_agent/cli/cloud/commands/deploy/wrangler_wrapper.py (2)
src/mcp_agent/cli/cloud/commands/deploy/bundle_utils.py (2)
create_pathspec_from_gitignore(17-36)should_ignore_by_gitignore(40-78)src/mcp_agent/cli/utils/ux.py (2)
print_warning(66-77)print_info(32-49)
tests/cli/commands/test_wrangler_wrapper.py (1)
src/mcp_agent/cli/cloud/commands/deploy/bundle_utils.py (2)
create_pathspec_from_gitignore(17-36)should_ignore_by_gitignore(40-78)
src/mcp_agent/cli/cloud/commands/deploy/main.py (1)
src/mcp_agent/cli/core/utils.py (1)
run_async(12-27)
🔇 Additional comments (11)
src/mcp_agent/cli/cloud/commands/deploy/main.py (1)
119-128: LGTM! Clean parameter definition with appropriate validation.The
ignore_fileoption is well-defined with sensible defaults (exists=False allows non-existent paths, resolve_path=True normalizes the path). The help text clearly describes the behavior.tests/cli/commands/test_wrangler_wrapper.py (7)
10-10: LGTM! Clean import of new dependencies.The imports of
pathspecand bundle utilities are necessary for the new ignore-file tests and match the production code structure.Also applies to: 21-24
944-975: Good coverage: secrets example file handling.The test correctly verifies that
.secrets.yaml.examplefiles are processed as.mcpac.py(unlike live secrets files which are excluded). This ensures example files ship with the bundle for reference without exposing actual secrets.Also applies to: 1014-1014
1024-1061: Strong test: gitignore matching covers files, directories, and extensions.The test exercises the core filtering logic with a realistic PathSpec containing file globs (
*.log,*.pyc) and directory patterns (node_modules/,build/). The mock ofPath.is_direnables testing directory-specific matching without creating actual directories.
1063-1090: Good coverage: PathSpec creation and missing-file fallback.These tests verify the utility correctly parses ignore files into a PathSpec matcher and returns
Nonefor missing files (enabling callers to fall back to default behavior). The assertions confirm the matcher respects gitignore syntax.
1092-1147: Comprehensive: edge cases for directory patterns and nested paths.The tests confirm:
Nonespec returns empty ignore set (no false positives)- Directory patterns with trailing slash match folders
- Nested patterns are evaluated relative to project root
This coverage validates the cross-platform path normalization (POSIX separators) mentioned in the bundle_utils implementation.
1150-1206: Excellent end-to-end test: ignore-file integration.This test creates a realistic project with included and excluded files, supplies a real
.mcpacignore, and verifies the temp bundle contains only the expected files. The assertions prove the ignore spec is correctly wired intocopytree.
1208-1253: Good negative test: missing ignore file warning.The test verifies that when an ignore file is specified but missing, the code:
- Warns the user (via
print_warning)- Falls back to default excludes (no files incorrectly ignored)
The assertions confirm the warning message includes the path and "not found", providing clear feedback.
src/mcp_agent/cli/cloud/commands/deploy/wrangler_wrapper.py (3)
21-24: LGTM! Import of ignore utilities.Clean import of the new bundle utilities module. These functions encapsulate the gitignore-based filtering logic.
175-187: Clear logging of ignore behavior.The logging clearly distinguishes three cases:
- Ignore file provided and loaded successfully
- Ignore file provided but missing (warns and falls back)
- No ignore file provided (applies defaults only)
This provides excellent visibility for debugging bundling issues.
242-260: Excellent: Bundled files list for transparency.Computing and logging the list of bundled original files (excluding internal helpers like
wrangler.tomlandmcp_deploy_breadcrumb.py) provides transparency into what's being deployed. The sorted output ensures deterministic logs for debugging.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/mcp_agent/cli/cloud/commands/deploy/main.py (1)
380-407: Document the newignoreparameter in the docstring.The function signature added an
ignore: Optional[Path]parameter, but the docstring's Args section doesn't document it.Args: app_id: The application ID api_key: API key for authentication project_dir: Directory containing the project files mcp_app_client: MCP App client for API calls retry_count: Number of retry attempts for deployment + ignore: Optional path to ignore file (gitignore syntax) for bundlingsrc/mcp_agent/cli/cloud/commands/deploy/bundle_utils.py (1)
37-37: Remove trailing whitespace.Line 37 has trailing whitespace that should be removed for code style consistency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
docs/cli-reference.mdx(3 hunks)docs/cloud/overview.mdx(1 hunks)src/mcp_agent/cli/cloud/commands/deploy/bundle_utils.py(1 hunks)src/mcp_agent/cli/cloud/commands/deploy/main.py(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- docs/cli-reference.mdx
- docs/cloud/overview.mdx
🧰 Additional context used
🧬 Code graph analysis (1)
src/mcp_agent/cli/cloud/commands/deploy/main.py (1)
src/mcp_agent/cli/core/utils.py (1)
run_async(12-27)
🔇 Additional comments (5)
src/mcp_agent/cli/cloud/commands/deploy/main.py (2)
119-131: LGTM! Clear precedence documentation in help text.The option is properly typed and the help text accurately documents the three-tier precedence order that matches the implementation.
340-348: Verify: Should explicit --ignore-file fail if the file doesn't exist?Currently, if a user provides
--ignore-file missing.txtthat doesn't exist, the code silently falls through to auto-detecting.mcpacignorein config-dir or CWD. This could be confusing behavior.Consider whether an explicitly provided ignore file that doesn't exist should:
- Fail fast with a clear error (user explicitly specified a file), or
- Silently continue with fallback (current behavior)
If option 1 is preferred, add a check after line 343:
if ignore_file is not None: if not ignore_file.exists(): raise CLIError( f"Specified ignore file '{ignore_file}' does not exist.", retriable=False, ) ignore_path = ignore_filesrc/mcp_agent/cli/cloud/commands/deploy/bundle_utils.py (3)
17-36: LGTM! Clean implementation with proper encoding.The function correctly handles non-existent files and uses explicit UTF-8 encoding for cross-platform compatibility.
40-80: LGTM! Correct gitignore semantics with cross-platform support.The implementation correctly:
- Handles None spec gracefully
- Normalizes paths to POSIX format for cross-platform compatibility
- Respects directory-with-trailing-slash patterns
- Safely handles paths outside project_dir
12-14: No changes needed for pathspec import
All usages ofbundle_utilsreside in CLI-only modules (the deploy command and its tests), which already mandate installing the[cli]extras that includepathspec. Keeping the top-level import is acceptable.
Add support for ignore files in deploy bundling
TL;DR
Added support for gitignore-style pattern matching during deployment to exclude files from being bundled.
What changed?
deploycommand:--ignore: Uses.mcpacignorein the current directory--ignore-file <path>: Specifies a custom ignore file pathbundle_utils.pymodule with utilities for:How to use?
Patterns follow gitignore syntax and are evaluated relative to the project directory.
Why make this change?
This change gives users more control over which files are included in their deployment bundles. It allows:
Summary by CodeRabbit