Replace Flake8 with Ruff, move long-running pre-commit checks to end - #187
Conversation
WalkthroughReplaces Flake8 with Ruff and updates pre-commit hooks and VS Code recommendations; adds ruff.toml and lint task variants. Refactors docker/labctl.py to Path-based APIs, rewrites docs/web/update-docs.py into a DocsProcessor, enhances task-mcp server/tools, and applies various script modernizations and minor IO/typing cleanups. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Main as update-docs.py (CLI)
participant Proc as DocsProcessor
participant FS as Filesystem
participant YAML as YAML Parser
User->>Main: run update-docs.py [--verbose]
Main->>Proc: initialize(repository_path, output_content_path, verbose)
Proc->>FS: delete_directory_content(output_path)
Proc->>FS: create_directory(output_path)
Proc->>YAML: load_config(update-docs-config.yaml)
Proc->>Proc: process_docker_directory(docker/, target/)
loop per compose file
Proc->>FS: read compose file
Proc->>YAML: parse compose
Proc->>Proc: extract compose metadata
Proc->>FS: write generated markdown/index
end
Proc->>Proc: process_markdown_locations()
loop per markdown file
Proc->>FS: read file
Proc->>Proc: extract_relative_links()
Proc->>Proc: update_relative_link()*
Proc->>FS: write processed file with frontmatter
end
Proc-->>User: completed
sequenceDiagram
autonumber
actor User
participant CLI as labctl CLI
participant Loader as load_services_config
participant Proc as process_services
participant Cmd as docker_command
participant FS as Filesystem
participant Docker as Docker Engine
User->>CLI: cmd_config_apply(config_path)
CLI->>Loader: load_services_config(config_path)
Loader-->>CLI: config (dict)
CLI->>Proc: process_services(host_config_dir: Path, config)
loop per category/service
Proc->>Cmd: docker_command(host_config_dir, stack_dir: Path, service, action)
Cmd->>FS: get_compose_file(stack_dir, service)
alt compose has build directive
Cmd->>Docker: build/bake
else
Cmd->>Docker: compose pull/up/down with --env-file args
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
.pre-commit-config.yaml (2)
41-61: Avoid per-hook Ruff excludes; keep them in ruff.tomlPre-commit excludes can drift from
ruff.toml. Prefer a single source of truth.Apply:
- - id: ruff-check - args: ["--fix"] - exclude: | - (?x)^( - scripts/git-filter-repo.py| - scripts/test-colors.py - )$ + - id: ruff-check + args: ["--fix"] # Formatter - https://docs.astral.sh/ruff/formatter/ - - id: ruff-format - exclude: | - (?x)^( - scripts/git-filter-repo.py| - scripts/test-colors.py - )$ + - id: ruff-formatEnsure the same excludes are set in
ruff.toml(see my other comment).
93-106: Ansible-lint not always_run: aligns with “long runners last” goalExplicitly disabling
always_runis fine; the customentryis environment-coupled—consider migrating toadditional_dependencieslater.Taskfile.yaml (2)
38-41: Use “staged” (not “stashed”) and add a precondition for pre-commit.Also surface diffs on failure for better UX.
lint: - desc: Run linting tools on stashed files (via pre-commit) - cmds: - - pre-commit run + desc: Run linting tools on staged files (via pre-commit) + preconditions: + - sh: "command -v pre-commit >/dev/null 2>&1" + msg: "pre-commit is not installed (try: pipx install pre-commit)" + cmds: + - pre-commit run --show-diff-on-failure
42-45: LGTM; consider same precondition and consistent flags for lint-all.Keeps output readable and failure-focused.
lint-all: desc: Run linting tools on all files (via pre-commit) - cmds: - - pre-commit run --all-files + preconditions: + - sh: "command -v pre-commit >/dev/null 2>&1" + msg: "pre-commit is not installed (try: pipx install pre-commit)" + cmds: + - PRE_COMMIT_COLOR=always pre-commit run --all-files --show-diff-on-failure
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
.flake8(0 hunks).pre-commit-config.yaml(2 hunks).vscode/extensions.json(1 hunks)CLAUDE.md(1 hunks)README.md(1 hunks)Taskfile.yaml(1 hunks)ruff.toml(1 hunks)
💤 Files with no reviewable changes (1)
- .flake8
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{sh,Dockerfile,yml,yaml,tf}
📄 CodeRabbit inference engine (CLAUDE.md)
Follow the pre-commit rules defined in .pre-commit-config.yaml, which includes linting for shell scripts, Dockerfiles, YAML files, Ansible playbooks, and Terraform configurations.
Files:
Taskfile.yaml
🧠 Learnings (4)
📚 Learning: 2025-08-06T20:29:29.605Z
Learnt from: CR
PR: bubacoder/infra#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-06T20:29:29.605Z
Learning: Use Pre-commit for code quality and security checks.
Applied to files:
README.mdTaskfile.yaml.pre-commit-config.yaml
📚 Learning: 2025-08-06T20:29:29.605Z
Learnt from: CR
PR: bubacoder/infra#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-06T20:29:29.605Z
Learning: Use GitHub Actions for CI/CD workflows, including pre-commit checks, building devcontainer, and deploying documentation site.
Applied to files:
README.md
📚 Learning: 2025-08-06T20:29:29.605Z
Learnt from: CR
PR: bubacoder/infra#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-06T20:29:29.605Z
Learning: Applies to **/*.{sh,Dockerfile,yml,yaml,tf} : Follow the pre-commit rules defined in .pre-commit-config.yaml, which includes linting for shell scripts, Dockerfiles, YAML files, Ansible playbooks, and Terraform configurations.
Applied to files:
README.mdTaskfile.yaml.pre-commit-config.yaml
📚 Learning: 2025-08-06T20:29:29.605Z
Learnt from: CR
PR: bubacoder/infra#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-06T20:29:29.605Z
Learning: Use Task (taskfile.dev) as the task runner/build tool for development and building tasks.
Applied to files:
README.mdCLAUDE.md
⏰ 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: build
- GitHub Check: check
🔇 Additional comments (7)
ruff.toml (1)
1-1: Good addition of a dedicated Ruff configClear, minimal starting point; header link helps discovery.
.vscode/extensions.json (1)
11-11: No leftover Flake8 references foundThe case-insensitive ripgrep search for “flake8”, “ms-python.flake8”, or “.flake8” across the codebase (excluding
dist/andbuild/) returned no matches, confirming that the migration to the Ruff extension is complete..pre-commit-config.yaml (3)
34-40: ShellCheck addition: good coverage for sh scriptsNice inclusion;
.shellcheckrcnote is helpful.
107-119: KICS moved to the end and not always_run: matches PR objectiveAlso good to cap severities during initial rollout.
69-76: Verify Nodeenv Version SupportThe attempt to list available Node.js versions via
python -m nodeenv --listin your CI image returned an empty list, indicating that nodeenv may not be installed correctly or that the command isn’t yielding any version data. Before relying onlanguage_version: "22.17.1", please manually confirm the following in your CI environment:
- Ensure
nodeenvis installed (pip show nodeenvorpip listshould list it).- Run
nodeenv --list(orpython -m nodeenv --list) directly in the CI shell to see which Node.js versions are actually supported.- If
22.17.1is not available, either:
- Pin to a known-supported patch version (e.g.
22.7.0),- Use a major-only pin (
"22"),- Or switch to
language_version: "system"until your Ubuntu image is upgraded.README.md (1)
77-77: Docs updated totask lint-all: good clarityMatches the repo-wide shift to Ruff/pre-commit on all files.
CLAUDE.md (1)
22-24: Docs: switch tolint-allacknowledgedConsistent with README and task changes.
0098dbe to
738472f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.pre-commit-config.yaml (1)
96-108: KICS still runs full scans on any change; gate it to push/manual.With
pass_filenames: false, KICS will scan the repo whenever the hook runs. To keep local iterations fast, run it onpushandmanualonly, and keep it last.- repo: https://github.com/Checkmarx/kics rev: v2.1.13 hooks: - id: kics-scan # TODO solve the high severity issues first args: ["--exclude-severities", "info,low,medium"] pass_filenames: false - always_run: false + stages: [push, manual]If you want it in CI only, we can also add a top-level
ci:block to skip this hook locally and run it in workflows.
♻️ Duplicate comments (1)
.pre-commit-config.yaml (1)
51-57: Avoid pinning Hadolint to a beta tag.Prefer a stable release to reduce churn; prior review already called this out.
Run to find the latest stable (non-prerelease) tag and patch the config:
#!/bin/bash set -euo pipefail latest="$(gh release list -R hadolint/hadolint --limit 20 | awk '!/Prerelease/{print $1; exit}')" echo "Latest stable hadolint: $latest" sed -n '1,120p' .pre-commit-config.yaml | nl -ba | sed -n '1,120p' # Show a suggested patch printf -- "--- a/.pre-commit-config.yaml\n+++ b/.pre-commit-config.yaml\n@@\n- rev: v2.13.0-beta\n+ rev: %s\n" "$latest"
🧹 Nitpick comments (5)
scripts/task-mcp/server.py (1)
17-17: Import path update looks good; consider package-izing for robustness.Absolute local import works when running the script directly, but can break if this module is imported from elsewhere or if another installed package named
toolsshadows it. Optional: convertscripts/task-mcp/into a package (__init__.py) and renametask-mcp→task_mcp, then use a package-relative import.- from tools.find_app_icon import AppIconFinder + from tools.find_app_icon import AppIconFinder # keep for nowOutside this diff, to package-ize:
- Rename directory:
scripts/task-mcp→scripts/task_mcp- Create
scripts/task_mcp/__init__.py- Then you can do:
from .tools.find_app_icon import AppIconFinderruff.toml (1)
21-24: Ignore list is fine; consider revisiting once formatter is enabled.
E501becomes redundant ifruff-formatis adopted later..pre-commit-config.yaml (2)
58-65: Node version pin for Renovate: considersystemto avoid nodeenv friction.Since you noted nodeenv constraints, an alternative is to rely on a system Node until the base image is upgraded.
- language_version: "22.17.1" + language_version: "system"
90-90:always_run: falseis redundant for ansible-lint.Default is false; you can drop it for brevity.
- always_run: falseTaskfile.yaml (1)
38-41: “stashed files” → “staged files”.
pre-commit runoperates on staged/changed files, not “stashed” files.- desc: Run linting tools on stashed files (via pre-commit) + desc: Run linting tools on staged files (via pre-commit)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (8)
.flake8(0 hunks).pre-commit-config.yaml(2 hunks).vscode/extensions.json(1 hunks)CLAUDE.md(1 hunks)README.md(1 hunks)Taskfile.yaml(1 hunks)ruff.toml(1 hunks)scripts/task-mcp/server.py(1 hunks)
💤 Files with no reviewable changes (1)
- .flake8
🚧 Files skipped from review as they are similar to previous changes (3)
- .vscode/extensions.json
- CLAUDE.md
- README.md
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{sh,Dockerfile,yml,yaml,tf}
📄 CodeRabbit inference engine (CLAUDE.md)
Follow the pre-commit rules defined in .pre-commit-config.yaml, which includes linting for shell scripts, Dockerfiles, YAML files, Ansible playbooks, and Terraform configurations.
Files:
Taskfile.yaml
🧠 Learnings (3)
📓 Common learnings
Learnt from: CR
PR: bubacoder/infra#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-06T20:29:29.605Z
Learning: Use Pre-commit for code quality and security checks.
📚 Learning: 2025-08-06T20:29:29.605Z
Learnt from: CR
PR: bubacoder/infra#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-06T20:29:29.605Z
Learning: Use Pre-commit for code quality and security checks.
Applied to files:
Taskfile.yaml.pre-commit-config.yaml
📚 Learning: 2025-08-06T20:29:29.605Z
Learnt from: CR
PR: bubacoder/infra#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-06T20:29:29.605Z
Learning: Applies to **/*.{sh,Dockerfile,yml,yaml,tf} : Follow the pre-commit rules defined in .pre-commit-config.yaml, which includes linting for shell scripts, Dockerfiles, YAML files, Ansible playbooks, and Terraform configurations.
Applied to files:
Taskfile.yaml.pre-commit-config.yaml
🧬 Code graph analysis (1)
scripts/task-mcp/server.py (1)
scripts/task-mcp/tools/find_app_icon.py (1)
AppIconFinder(8-112)
⏰ 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). (1)
- GitHub Check: check
🔇 Additional comments (7)
ruff.toml (4)
3-5: Target version and line length: LGTM.Matches the project’s shift to Ruff and long-line policy.
6-19: Rule selection is reasonable.Good, broad coverage roughly equivalent to Flake8 + common plugins.
26-31: Centralized excludes: LGTM.Keeps drift out of hook-local config.
38-43: Format settings are fine even ifruff-formatis not enabled yet.No action needed now.
.pre-commit-config.yaml (2)
34-40: ShellCheck hook: LGTM.Pinned and documented; good addition.
41-50: Ruff linter hook: LGTM.
ruff-check --fixis the right default; leaving formatter commented is fine.Taskfile.yaml (1)
42-45: lint-all task: LGTM.Clear separation between targeted and full-repo runs.
738472f to
6f808af
Compare
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 (8)
scripts/proxy-request-log.py (1)
44-52: Early return disables proxying; also use HTTPSConnection when needed.
The return prevents any upstream request; HTTPS targets will fail via HTTPConnection.Apply:
- print(f"\nSending request to: https://{target_host}:{target_port}{parsed_url.path}") - return + print(f"\nSending request to: {parsed_url.scheme}://{target_host}:{target_port}{parsed_url.path}") @@ - conn = http.client.HTTPConnection(target_host, target_port) + Connection = http.client.HTTPSConnection if parsed_url.scheme == "https" else http.client.HTTPConnection + conn = Connection(target_host, target_port) + try: @@ - target_response = conn.getresponse() + target_response = conn.getresponse() + finally: + # ensure the socket is closed even on exceptions + try: + conn.close() + except Exception: + passdocker/labctl.py (1)
34-53: Fix hostname case mismatch and make symlink creation robust (relative, cross-platform).
get_host_config_dir()lowercases the hostname, butcreate_localhost_link()uses the raw hostname. On case-sensitive filesystems this breaks the symlink target. Also preferPath.symlink_to()and create a relative link to keep the tree relocatable.Apply:
def create_localhost_link(docker_config_dir: Path) -> None: - """Create 'localhost' symlink in the parent directory.""" - hostname = socket.gethostname() - localhost_link = docker_config_dir / "localhost" - hostname_dir = docker_config_dir / hostname + """Create 'localhost' symlink in the parent directory.""" + hostname = socket.gethostname().lower() + localhost_link = docker_config_dir / "localhost" + hostname_dir = docker_config_dir / hostname @@ - if hostname_dir.exists() and hostname_dir.is_dir(): + if hostname_dir.exists() and hostname_dir.is_dir(): # Create or update the localhost symlink if localhost_link.exists(): if localhost_link.is_symlink(): localhost_link.unlink() else: logger.error(f"Error: {localhost_link} exists but is not a symlink. Cannot create link.") return - - try: - os.symlink(f"{hostname}/", localhost_link, target_is_directory=True) + try: + # Relative symlink keeps repo relocatable + localhost_link.symlink_to(hostname, target_is_directory=True) except Exception as e: logger.error(f"Error creating localhost symlink: {e}") + else: + logger.warning(f"Host config directory not found: {hostname_dir} — skipping localhost symlink.")docs/web/update-docs.py (6)
29-41: Always attach a console handler; make verbosity change level, not handler presence.Without
--verbose, no handler is attached and INFO logs won’t show.- # Set up logging - self.logger = logging.getLogger(__name__) - self.logger.setLevel(logging.INFO) - - if verbose: - # Create console handler with a higher log level - ch = logging.StreamHandler() - ch.setLevel(logging.DEBUG) - formatter = logging.Formatter(' %(levelname)s: %(message)s') - ch.setFormatter(formatter) - self.logger.addHandler(ch) - self.logger.setLevel(logging.DEBUG) + # Set up logging + self.logger = logging.getLogger(__name__) + level = logging.DEBUG if verbose else logging.INFO + self.logger.setLevel(level) + ch = logging.StreamHandler() + ch.setLevel(level) + ch.setFormatter(logging.Formatter(' %(levelname)s: %(message)s')) + self.logger.addHandler(ch)
126-158: Normalize path separators in rewritten links for Markdown portability.
os.path.relpathyields backslashes on Windows; Markdown links should use/.- new_relative_link = os.path.relpath( + new_relative_link = os.path.relpath( self.output_content_path / new_target, target_path.parent - ) + ).replace(os.sep, "/") @@ - new_relative_link = os.path.relpath( + new_relative_link = os.path.relpath( self.output_content_path / new_target, target_path.parent - ) + ).replace(os.sep, "/")
200-228: Read/write Markdown with UTF-8; add title frontmatter when no H1 is present.- with open(source_file_path) as readme_file: + with open(source_file_path, encoding="utf-8") as readme_file: content = readme_file.read() @@ - with open(target_file_path, "w") as readme_file: - readme_file.write(processed_content) + # Ensure a title even if no '# ' header exists + if not title_found: + inferred = source_file_path.stem.replace("-", " ").title() + processed_content = f"---\ntitle: \"{inferred}\"\n" + (f"weight: {weight}\n" if weight != 0 else "") + "---\n" + processed_content + + with open(target_file_path, "w", encoding="utf-8") as readme_file: + readme_file.write(processed_content)
194-199: Guard against accidental deletion outside the repo when wiping output directory.Add a safety check before
rmtree.def delete_directory_content(self, content_path): """Delete all content in a directory and recreate the directory.""" - if content_path.exists() and content_path.is_dir(): + if content_path.exists() and content_path.is_dir(): + # Safety: ensure content_path is inside the repository + repo_root = self.repository_path.resolve() + resolved = content_path.resolve() + if not str(resolved).startswith(str(repo_root)): + self.logger.error(f"Refusing to delete outside repo: {resolved}") + sys.exit(1) shutil.rmtree(content_path) self.create_directory(content_path)
270-295: Handle labels as list or dict; use UTF-8 when reading compose.Compose allows
labelsto be either a mapping or a list ofkey=valuestrings.- try: - with open(file_path) as stream: + try: + with open(file_path, encoding="utf-8") as stream: compose_dict = yaml.safe_load(stream) if compose_dict is None: return {} @@ - for service in services.values(): - labels = service.get("labels", {}) - homepage_name = labels.get("homepage.name", "") - homepage_description = labels.get("homepage.description", "") - homepage_icon = labels.get("homepage.icon", "") + for service in services.values(): + raw_labels = service.get("labels", {}) or {} + if isinstance(raw_labels, dict): + label_map = raw_labels + elif isinstance(raw_labels, list): + label_map = {} + for item in raw_labels: + if isinstance(item, str) and "=" in item: + k, v = item.split("=", 1) + label_map[k.strip()] = v.strip() + else: + label_map = {} + homepage_name = label_map.get("homepage.name", "") + homepage_description = label_map.get("homepage.description", "") + homepage_icon = label_map.get("homepage.icon", "")
312-339: Ensure compose docs are always written; current logic writes nothing if no '---' is present.Many compose files omit the YAML document separator. Write the entire file as a fenced code block unconditionally.
- with open(source_file_path) as compose_file: - lines = compose_file.readlines() - - yaml_started = False - processed_lines = ["---\n", f"title: \"{metadata['name']}\"\n"] - if 'description' in metadata: - processed_lines.append(f"description: \"{metadata['description']}\"\n") - if 'icon' in metadata: - processed_lines.append("params:\n") - processed_lines.append(f" icon: \"{self.get_icon_url(metadata['icon'])}\"\n") - processed_lines.append("---\n") - - for line in lines: - if yaml_started: - processed_lines.append(line) - elif line.startswith("# "): - processed_lines.append(line[2:]) - elif line.startswith("#"): - processed_lines.append(line[1:]) - elif line.strip() == "---": - yaml_started = True - processed_lines.append("```yaml\n") - - if yaml_started: - processed_lines.append("```\n") - with open(target_file_path, "w") as doc_file: - doc_file.writelines(processed_lines) + with open(source_file_path, encoding="utf-8") as compose_file: + compose_text = compose_file.read() + + processed_lines = ["---\n", f"title: \"{metadata['name']}\"\n"] + if 'description' in metadata and metadata['description']: + processed_lines.append(f"description: \"{metadata['description']}\"\n") + if 'icon' in metadata and metadata['icon']: + processed_lines.append("params:\n") + processed_lines.append(f" icon: \"{self.get_icon_url(metadata['icon'])}\"\n") + processed_lines.append("---\n") + processed_lines.append("```yaml\n") + processed_lines.append(compose_text) + if not compose_text.endswith("\n"): + processed_lines.append("\n") + processed_lines.append("```\n") + with open(target_file_path, "w", encoding="utf-8") as doc_file: + doc_file.writelines(processed_lines)
♻️ Duplicate comments (1)
.pre-commit-config.yaml (1)
52-58: Avoid pinning Hadolint to a beta tag.
Prefer a stable release to reduce break risk; pin to latest stable.Run to find the latest stable:
#!/bin/bash gh release list -R hadolint/hadolint -L 20 | awk '/^v[0-9].*Latest/ || ($1 ~ /^v[0-9]/ && $3 != "Pre-release") {print $1, $3}' | head -n5
🧹 Nitpick comments (9)
scripts/proxy-request-log.py (2)
38-43: Redact sensitive headers and cap body logging.
Authorization/Cookie may leak secrets; also avoid dumping huge/binary bodies.Example:
- print("\nHeaders:") - print(headers) + print("\nHeaders:") + redacted = {k: ("<redacted>" if k.lower() in {"authorization","cookie","set-cookie"} else v) + for k, v in dict(self.headers).items()} + print(redacted) @@ - print(post_data) + if post_data and len(post_data) > 4096: + print(post_data[:4096] + b"... <truncated>") + else: + print(post_data)
66-68: Use a threaded server to avoid blocking.
Single-threaded TCPServer will serialize requests.Apply:
-with socketserver.TCPServer(("", PORT), Handler) as httpd: +with socketserver.ThreadingTCPServer(("", PORT), Handler) as httpd:Taskfile.yaml (1)
42-46: Rename to lint-staged; description is misleading.
pre-commit operates on staged/changed files, not “stashed files.”Apply:
- lint-stashed: - desc: Run linting tools on stashed files (via pre-commit) + lint-staged: + desc: Run linting tools on staged files (via pre-commit) cmds: - pre-commit runscripts/github-extract-links.py (1)
8-20: Precompile regex and align character classes with GitHub rules.
Usernames allow [A-Za-z0-9-]; repo names allow [A-Za-z0-9._-]. Tighten the regex and compile once.Apply:
-def extract_github_links(directory: str) -> list[str]: - github_links: set[str] = set() - for root, _dirs, files in os.walk(directory): +GH_RE = re.compile(r"https://github\.com/([A-Za-z0-9-]+/[A-Za-z0-9._-]+)") + +def extract_github_links(directory: str) -> list[str]: + github_links: set[str] = set() + for root, _dirs, files in os.walk(directory): @@ - with open(file_path) as f: + with open(file_path, encoding="utf-8", errors="ignore") as f: content = f.read() - # Usernames for user accounts on GitHub can only contain alphanumeric characters and dashes ( - ). - links = re.findall(r"https://github.com/([\w.\-\_]+/[\w.\-\_]+)", content) + # Usernames allow alnum and dashes; repo names allow alnum, dot, underscore, dash. + links = GH_RE.findall(content)docker/labctl.py (2)
60-69: Use explicit UTF-8 when reading compose files; defensive defaults.def has_build_directive(compose_file: Path) -> bool: """Check if the service uses a build directive.""" - with open(compose_file) as f: - yaml_content = yaml.safe_load(f) + with open(compose_file, encoding="utf-8") as f: + yaml_content = yaml.safe_load(f) or {} if yaml_content and 'services' in yaml_content: for service_config in yaml_content['services'].values(): if 'build' in service_config: return True return False
134-143: Explicit UTF-8 for config read; prefer raising over exit for testability.def load_services_config(config_file: str) -> dict: """Load services configuration from YAML file.""" try: - with open(config_file) as file: + with open(config_file, encoding="utf-8") as file: config = yaml.safe_load(file) return config except Exception as e: logger.error(f"Error loading configuration file {config_file}: {e}") sys.exit(1)docs/web/update-docs.py (3)
49-55: Open YAML config with UTF-8 to avoid locale issues.- with open(yaml_path) as yaml_file: + with open(yaml_path, encoding="utf-8") as yaml_file: data = yaml.safe_load(yaml_file)
173-188: Optional: avoid rewriting links inside fenced code blocks.Current regex rewrites links even in code examples. If that’s undesirable, we can strip code blocks prior to replacement.
237-243: Deterministic directory processing order.- for file in os.listdir(source_dir): + for file in sorted(os.listdir(source_dir)):
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (15)
.flake8(0 hunks).pre-commit-config.yaml(2 hunks).vscode/extensions.json(1 hunks)CLAUDE.md(1 hunks)Taskfile.yaml(1 hunks)docker/labctl.py(9 hunks)docs/web/update-docs.py(7 hunks)ruff.toml(1 hunks)scripts/create-example-env.py(1 hunks)scripts/git-reorder-fixup.py(2 hunks)scripts/github-extract-links.py(3 hunks)scripts/github-star-repo.py(1 hunks)scripts/proxy-request-log.py(2 hunks)scripts/task-mcp/server.py(3 hunks)scripts/task-mcp/tools/find_app_icon.py(1 hunks)
💤 Files with no reviewable changes (1)
- .flake8
✅ Files skipped from review due to trivial changes (4)
- scripts/create-example-env.py
- scripts/github-star-repo.py
- scripts/task-mcp/tools/find_app_icon.py
- .vscode/extensions.json
🚧 Files skipped from review as they are similar to previous changes (3)
- CLAUDE.md
- ruff.toml
- scripts/task-mcp/server.py
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{sh,Dockerfile,yml,yaml,tf}
📄 CodeRabbit inference engine (CLAUDE.md)
Follow the pre-commit rules defined in .pre-commit-config.yaml, which includes linting for shell scripts, Dockerfiles, YAML files, Ansible playbooks, and Terraform configurations.
Files:
Taskfile.yaml
🧠 Learnings (2)
📚 Learning: 2025-08-06T20:29:29.605Z
Learnt from: CR
PR: bubacoder/infra#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-06T20:29:29.605Z
Learning: Applies to **/*.{sh,Dockerfile,yml,yaml,tf} : Follow the pre-commit rules defined in .pre-commit-config.yaml, which includes linting for shell scripts, Dockerfiles, YAML files, Ansible playbooks, and Terraform configurations.
Applied to files:
Taskfile.yaml.pre-commit-config.yaml
📚 Learning: 2025-08-06T20:29:29.605Z
Learnt from: CR
PR: bubacoder/infra#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-06T20:29:29.605Z
Learning: Use Pre-commit for code quality and security checks.
Applied to files:
Taskfile.yaml.pre-commit-config.yaml
⏰ 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: build
- GitHub Check: check
🔇 Additional comments (19)
scripts/proxy-request-log.py (1)
3-3: Import placement/change is fine.
Explicit import of http.client is correct and resolves usage below.Taskfile.yaml (1)
38-38: Clarified lint description: good.
Accurately signals “all files”; matches pre-commit usage.scripts/github-extract-links.py (1)
23-31: trim_git_ending: LGTM.
Function is clear and type-hinted; no issues..pre-commit-config.yaml (5)
34-40: ShellCheck hook: good addition.
Covers .sh and shebang’d scripts; aligns with repo guidelines.
41-51: Ruff adoption looks correct.
Using ruff-check with --fix matches the PR objective; formatter can be enabled later.
59-66: Renovate validator: OK.
Pinned node language_version workaround is reasonable on older nodeenv.
91-91: ansible-lint not always_run: good.
Improves local iteration; still runs on matching paths.
97-109: KICS moved to bottom with reduced severities: good.
Matches PR goal to push slower scans later and focus on highs.docker/labctl.py (8)
71-85: Env-file resolution looks good.Absolute, normalized paths and existence checks are correct; ordering supports precedence layering.
116-133: LGTM on action dispatch.
updatecorrectly performs pull/build beforeup; match/case is clear.
178-183: Consistent lowercasing with hostname.
get_host_config_dir()lowercases the hostname. After applying thecreate_localhost_link()fix, both will align. No further action.
195-202: Good: localhost link creation moved near config load.Ensures symlink is refreshed before env-file resolution and service processing.
204-208: Image prune on update: confirm intended retention policy.
until=504h(21 days) is reasonable, but validate with ops that this won't evict needed images on slower hosts.
210-228: CLI service path parsing is clear and flexible.Nicely handles category/subcategory/name and passes resolved paths.
104-114: No changes needed: COMPOSE_BAKE=true is an official toggle for “docker compose build” to use Buildx Bake, and passing --pull correctly forces pulling base images.
154-176: New services schema confirmed – no legacy mappings detected. The exampleconfig-example/docker/myhost/services.yamluses the list-of-single-key format, and noservices.yamlunderconfig/dockeruses the old mapping style.docs/web/update-docs.py (3)
368-376: LGTM: git root resolution with text mode.
386-391: CLI wiring looks solid.Reasonable defaults for repo root and output content path; verbose flag plumbs through.
341-354: Remove theREADME.mdgate around Docker-compose processing or clarify its intentIn docs/web/update-docs.py at lines 341–354, the loop only runs
process_docker_compose_filein folders that contain a README.md, skipping many stack definitions underdocker/…(e.g.docker/ai/litellm/config.yaml,docker/dashboard/homepage/config/*.yaml,docker/monitoring/prometheus/prometheus.yaml, …). If you intend to restrict processing to dirs with README.md, please document that rationale; otherwise move the YML/YAML handling outside theif "README.md" in filesblock so all compose files are rendered.
6f808af to
afcddc7
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (9)
.pre-commit-config.yaml (1)
97-109: KICS: gate to pre-push and serialize; whole-repo scan on pushGiven pass_filenames: false, run only on push and always (security gate).
- id: kics-scan # TODO solve the high severity issues first args: ["--exclude-severities", "info,low,medium"] pass_filenames: false - always_run: false + always_run: true + stages: [push] + require_serial: truescripts/task-mcp/server.py (5)
32-45: Harden get_git_root with a timeoutPrevents indefinite hangs if git blocks (e.g., credential prompts).
result = subprocess.run( ["git", "rev-parse", "--show-toplevel"], stdout=subprocess.PIPE, check=True, - text=True, + text=True, + timeout=DEFAULT_TIMEOUT, )
48-82: Make task discovery reliable: disable colors, use cwd, strip ANSI, handle missing CLICurrent parsing may break with colored output; also unhandled FileNotFoundError. Add timeout.
-def get_task_list() -> list[dict[str, str]]: +def get_task_list() -> list[dict[str, str]]: @@ logger.debug("Getting task list") try: result = subprocess.run( - ["task", "--list-all", "--dir", repository_root_path], - capture_output=True, - text=True, - check=True + ["task", "--list-all"], + capture_output=True, + text=True, + check=True, + cwd=repository_root_path, + env=NO_COLOR_ENV, + timeout=DEFAULT_TIMEOUT, ) - tasks = [] - # Parse output lines - for line in result.stdout.splitlines(): - # Match lines like "* task_name: task description" - match = re.match(r'^\*\s+(.+?):\s+(.+)$', line.strip()) - if match: - task_name = match.group(1).strip() - description = match.group(2).strip() - tasks.append({ - "name": task_name, - "description": description - }) + tasks: list[dict[str, str]] = [] + task_line_re = re.compile(r'^\*\s+([^:]+):\s+(.*)$') + for raw_line in result.stdout.splitlines(): + line = ANSI_ESCAPE_RE.sub("", raw_line).strip() + match = task_line_re.match(line) + if not match: + continue + task_name = match.group(1).strip() + description = match.group(2).strip() + tasks.append({"name": task_name, "description": description}) @@ - except subprocess.CalledProcessError as e: - logger.error(f"Error getting task list: {e}") + except FileNotFoundError: + logger.error("`task` CLI not found. Install from https://taskfile.dev/#/installation.") + return [] + except subprocess.CalledProcessError as e: + logger.error(f"Error getting task list (rc={e.returncode}): {e.stderr or e.stdout or e}") return []
84-105: Add timeout/cwd/env to task execution and return actionable error detailAvoids hangs and improves diagnostics. Keeps security by not using a shell.
logger.info(f"Executing task: {task_name}") try: - return subprocess.run( - ["task", task_name, "--dir", repository_root_path], - capture_output=True, - text=True, - check=True - ).stdout.strip() + return subprocess.run( + ["task", task_name], + capture_output=True, + text=True, + check=True, + cwd=repository_root_path, + env=NO_COLOR_ENV, + timeout=DEFAULT_TIMEOUT, + ).stdout.strip() except subprocess.CalledProcessError as e: - logger.error(f"Error executing task {task_name}: {e}") - return f"Error executing task {task_name}: {e.stderr}" + logger.error(f"Error executing task {task_name} (rc={e.returncode}): {e.stderr or e.stdout or e}") + return f"Error executing task {task_name} (rc={e.returncode}): {e.stderr or e.stdout or str(e)}"
123-162: Stabilize labctl invocation: pathlib, cwd, timeoutEnsure relative paths inside labctl work; add timeout to avoid hangs.
cmd = [ sys.executable, - os.path.join(repository_root_path, "docker", "labctl.py"), + str(Path(repository_root_path) / "docker" / "labctl.py"), "service", operation, service_name ] try: result = subprocess.run( cmd, capture_output=True, text=True, - check=True + check=True, + cwd=repository_root_path, + timeout=DEFAULT_TIMEOUT, ) return result.stdout or "(No output)" except subprocess.CalledProcessError as e: return f"Error running operation: {e.stderr or str(e)}"
202-209: Sanitize tool names to be API-friendly and stableTasks may contain spaces or punctuation; sanitize for MCP tool IDs while keeping human title.
for task_info in tasks: task_name = task_info["name"] description = task_info["description"] task_fn = create_task_function(task_name) - tool = Tool.from_function(fn=task_fn, name=task_name, title=task_name, description=description) + safe_name = re.sub(r'[^a-zA-Z0-9_-]+', '-', task_name).strip('-').lower() + tool = Tool.from_function(fn=task_fn, name=safe_name, title=task_name, description=description) mcp.add_tool(tool)docker/labctl.py (1)
36-53: Normalize hostname casing and create a portable relative symlink.
get_host_config_dir()lower-cases the hostname, butcreate_localhost_link()uses the raw hostname. On systems where the config dir is lower-cased, the symlink won’t be created. Also, prefer a relative target to keep the link portable if the repo path changes.Apply:
- hostname = socket.gethostname() + hostname = socket.gethostname().lower() @@ - try: - os.symlink(f"{hostname}/", localhost_link, target_is_directory=True) + try: + # relative symlink for portability + target = os.path.relpath(hostname_dir, start=docker_config_dir) + os.symlink(target, localhost_link, target_is_directory=True)docs/web/update-docs.py (2)
277-289: Handle labels given as a list (Compose supports list and dict forms).Currently assumes a dict; when labels are a list of "key=value" strings, metadata extraction fails.
- for service in services.values(): - labels = service.get("labels", {}) - homepage_name = labels.get("homepage.name", "") - homepage_description = labels.get("homepage.description", "") - homepage_icon = labels.get("homepage.icon", "") + for service in services.values(): + raw_labels = service.get("labels", {}) + label_map: dict[str, str] = {} + if isinstance(raw_labels, dict): + label_map = raw_labels + elif isinstance(raw_labels, list): + for item in raw_labels: + if isinstance(item, str) and "=" in item: + k, v = item.split("=", 1) + label_map[k.strip()] = v.strip() + homepage_name = label_map.get("homepage.name", "") + homepage_description = label_map.get("homepage.description", "") + homepage_icon = label_map.get("homepage.icon", "")
312-338: Compose doc generation can skip writing output unless a '---' marker exists.Most compose files don’t start with '---'. The current logic writes only if
yaml_startedis True, so many files will produce no docs. Start a code block at the first non-comment line and always write the file.- with open(source_file_path) as compose_file: + with open(source_file_path, encoding="utf-8") as compose_file: lines = compose_file.readlines() - yaml_started = False - processed_lines = ["---\n", f"title: \"{metadata['name']}\"\n"] + yaml_started = False + in_comment_header = True + processed_lines = ["---\n", f"title: \"{metadata['name']}\"\n"] if 'description' in metadata: processed_lines.append(f"description: \"{metadata['description']}\"\n") if 'icon' in metadata: processed_lines.append("params:\n") processed_lines.append(f" icon: \"{self.get_icon_url(metadata['icon'])}\"\n") processed_lines.append("---\n") - for line in lines: - if yaml_started: - processed_lines.append(line) - elif line.startswith("# "): - processed_lines.append(line[2:]) - elif line.startswith("#"): - processed_lines.append(line[1:]) - elif line.strip() == "---": - yaml_started = True - processed_lines.append("```yaml\n") + for line in lines: + if in_comment_header and line.lstrip().startswith("#"): + # Strip one leading '#' and a single following space if present + stripped = line.lstrip()[1:] + if stripped.startswith(" "): + stripped = stripped[1:] + processed_lines.append(stripped) + continue + in_comment_header = False + if not yaml_started: + processed_lines.append("```yaml\n") + yaml_started = True + processed_lines.append(line) - if yaml_started: - processed_lines.append("```\n") - with open(target_file_path, "w") as doc_file: - doc_file.writelines(processed_lines) + if not yaml_started: + # Empty compose file; still emit an empty code block for consistency + processed_lines.append("```yaml\n") + yaml_started = True + processed_lines.append("```\n") + with open(target_file_path, "w", encoding="utf-8") as doc_file: + doc_file.writelines(processed_lines)
♻️ Duplicate comments (5)
.pre-commit-config.yaml (1)
52-58: Pin Hadolint to a stable release (drop beta tag)Prefer a non-prerelease to avoid unexpected breaking changes. Prior review already flagged this; proposing the same fix.
- repo: https://github.com/hadolint/hadolint - rev: v2.13.0-beta + rev: v2.12.0 hooks: - id: hadolint-docker exclude: "\\.dockerignore$"Run to verify the latest stable and adjust if newer exists:
#!/bin/bash set -euo pipefail # Show latest stable (non-prerelease) hadolint tag gh release list -R hadolint/hadolint -L 20 | awk '!/Prerelease/ && /^v[0-9]/{print $1; exit}' # Show current pinned revs for key repos rg -nP 'repo:\s+https://github.com/(astral-sh/ruff-pre-commit|pre-commit/pre-commit-hooks|ansible-community/ansible-lint|antonbabenko/pre-commit-terraform|Checkmarx/kics|hadolint/hadolint)' -n -C1 .pre-commit-config.yamlscripts/git-reorder-fixup.py (3)
9-12: Resolved: imports and editor handling look good.os/shlex imports are correct and alphabetized; sets up later use cleanly.
16-16: Resolved: respects $VISUAL/$EDITOR with sensible fallback.Good call not to use $GIT_SEQUENCE_EDITOR here (avoids recursion).
32-44: Always convert to fixup; match originals by normalized message; keep header comments first.Currently the line becomes “fixup …” only when a matching original is found; otherwise it stays “pick …” and is inserted at index 0 (above the header). Convert unconditionally, compare normalized messages for exact matches (reduces substring collisions), and insert after the comment header if no match.
- for fixup_line in fixup_lines: - line_to_add = fixup_line - original_commit_message = get_original_commit_message(line_to_add) + for fixup_line in fixup_lines: + # Always convert to "fixup" regardless of the current action token. + parts = fixup_line.split(maxsplit=2) + line_to_add = ('fixup ' + ' '.join(parts[1:])) if len(parts) >= 2 else fixup_line + original_commit_message = get_original_commit_message(line_to_add) original_commit_found = False - for i, reordered_line in enumerate(reordered_lines): - if original_commit_message in reordered_line: - line_to_add = line_to_add.replace('pick', 'fixup', 1) - reordered_lines.insert(i + 1, line_to_add) - original_commit_found = True - break + for i, reordered_line in enumerate(reordered_lines): + if reordered_line.startswith('#'): + continue + parts = reordered_line.split() + if len(parts) >= 3: + candidate_message = ' '.join(parts[2:]).replace('[FIXUP]', '').replace('[F]', '').strip() + if candidate_message == original_commit_message: + reordered_lines.insert(i + 1, line_to_add) + original_commit_found = True + break if not original_commit_found: - reordered_lines.insert(0, line_to_add) + # Insert right after the header comment block. + header_end = next((idx for idx, l in enumerate(reordered_lines) if not l.startswith('#')), len(reordered_lines)) + reordered_lines.insert(header_end, line_to_add)docker/labctl.py (1)
55-58: Support both .yaml and .yml compose filenames.This still hard-codes “.yaml” and will miss stacks using “.yml”. Return the first existing candidate and fall back to “.yaml” only for error messaging consistency.
def get_compose_file(stack_dir: Path, service_name: str) -> Path: - """Get the yaml file path for a service.""" - return stack_dir / f"{service_name}.yaml" + """Get the compose file path for a service (.yaml or .yml). + + Prefers .yaml, then .yml. Returns the first existing Path; if none exist, + returns the .yaml Path (so callers can use it in error messages). + """ + candidates = [ + stack_dir / f"{service_name}.yaml", + stack_dir / f"{service_name}.yml", + ] + for c in candidates: + if c.exists(): + return c + return candidates[0]
🧹 Nitpick comments (19)
.pre-commit-config.yaml (4)
34-40: ShellCheck: follow sourced files to reduce false positivesAdd -x so shellcheck can resolve files referenced via "source" or ".".
- id: shellcheck + args: ["-x"]
41-51: Ruff: stage formatter for pre-push to keep commits fastKeep ruff-check on commit; run ruff-format on push to avoid churn.
# Linter - https://docs.astral.sh/ruff/linter/ - id: ruff-check args: ["--fix"] - # TODO Enable after pendling MRs are merged - # Formatter - https://docs.astral.sh/ruff/formatter/ - # - id: ruff-format + # Formatter - https://docs.astral.sh/ruff/formatter/ + - id: ruff-format + stages: [push]
59-66: Scope Renovate validator to its config files and run on pushAvoid Node env bootstrapping on every commit.
- repo: https://github.com/renovatebot/pre-commit-hooks rev: 41.1.4 hooks: - id: renovate-config-validator # TODO Old nodeenv (v0.13.4) does not support "lts" - remove after Ubuntu upgrade language_version: "22.17.1" + stages: [push] + files: ^(renovate\.(json|json5|config\.(js|ts))|\.github/renovate\.(json|json5))$
89-96: Ansible-lint: good call on not always running; consider pre-push stageKeeps commit latency low while retaining guardrails.
- id: ansible-lint files: ^ansible/ always_run: false + stages: [push] entry: > env ANSIBLE_ROLES_PATH=~/.ansible/roles:/usr/share/ansible/roles:/etc/ansible/roles:ansible/roles env ANSIBLE_COLLECTIONS_PATH=/usr/lib/python3/dist-packages:/usr/share/ansible/collections:/etc/ansible/collections:/opt/pipx/venvs/ansible-core/lib/python3.12/site-packages/ansible_collections:~/.ansible/collections:collections python3 -m ansiblelint --force-colorscripts/git-reorder-fixup.py (3)
18-18: Specify UTF-8 when reading/writing the rebase-todo file.Prevents locale-dependent bugs with non-ASCII commit messages.
- with open(file_path) as file: + with open(file_path, encoding="utf-8") as file: @@ - with open(file_path, 'w') as file: + with open(file_path, 'w', encoding="utf-8") as file:Also applies to: 45-45
27-31: Make fixup marker detection case-insensitive (optional).Catches “[fixup]”/“[f]” variants without false positives.
- elif '[FIXUP]' in line or '[F]' in line: + elif any(m in line.lower() for m in ("[fixup]", "[f]")): fixup_lines.append(line)
48-48: Resolved: editor command is split safely.Minor portability nit: on Windows, shlex POSIX rules can misparse unquoted paths with spaces. Consider os.name == 'nt' handling if Windows support is needed.
scripts/task-mcp/server.py (3)
165-182: Reuse the shared AppIconFinder instanceAvoids repeated construction and makes it easy to add caching later.
- icon_finder = AppIconFinder() try: - return icon_finder.get_app_icon(app_name, homepage_url) + return ICON_FINDER.get_app_icon(app_name, homepage_url)
191-200: Fail-soft note when no tasks discoveredHelps operators notice a misconfigured environment without crashing the server.
# Get the list of available tasks tasks = get_task_list() + if not tasks: + logger.warning("No tasks discovered. The MCP server will start without dynamic tools.")
213-216: Optional: allow host/port/transport via env for ops flexibilityKeeps defaults but lets operators override without editing code.
if __name__ == "__main__": # Start the server - mcp.run() + transport = os.getenv("MCP_TRANSPORT") # e.g., "http" + host = os.getenv("MCP_HOST") + port = int(os.getenv("MCP_PORT")) if os.getenv("MCP_PORT") else None + path = os.getenv("MCP_PATH") + if any([transport, host, port, path]): + mcp.run(transport=transport or "stdio", host=host, port=port, path=path) + else: + mcp.run()docker/labctl.py (3)
92-133: Pass strings to subprocess and avoid repeating Path-to-str conversions.Some Python versions accept Path-like args, but typing says list[str]; make it explicit and DRY.
def docker_command(host_config_dir: Path, stack_dir: Path, service_name: str, action: str) -> None: @@ - compose_file = get_compose_file(stack_dir, service_name) + compose_file = get_compose_file(stack_dir, service_name) + compose_file_str = str(compose_file) @@ - docker(["compose", "-f", compose_file, *env_file_args, "build", "--pull"], env=env) + docker(["compose", "-f", compose_file_str, *env_file_args, "build", "--pull"], env=env) else: - docker(["compose", "-f", compose_file, *env_file_args, "pull"]) + docker(["compose", "-f", compose_file_str, *env_file_args, "pull"]) @@ - docker(["compose", "-f", compose_file, *env_file_args, "up", "--detach"]) + docker(["compose", "-f", compose_file_str, *env_file_args, "up", "--detach"]) @@ - docker(["compose", "-f", compose_file, *env_file_args, "down"]) + docker(["compose", "-f", compose_file_str, *env_file_args, "down"]) @@ - docker(["compose", "-f", compose_file, *env_file_args, "restart"]) + docker(["compose", "-f", compose_file_str, *env_file_args, "restart"]) @@ - docker(["compose", "-f", compose_file, *env_file_args, "up", "--detach", "--force-recreate"]) + docker(["compose", "-f", compose_file_str, *env_file_args, "up", "--detach", "--force-recreate"])
71-85: De-duplicate env files to avoid repeated --env-file flags.If common and host-specific paths resolve to the same file (e.g., via symlinks), dedupe for cleaner CLI invocations.
args = [] + seen: set[Path] = set() for file in env_paths: absolute_path = file.resolve() - if absolute_path.is_file(): + if absolute_path.is_file() and absolute_path not in seen: args.extend(["--env-file", str(absolute_path)]) + seen.add(absolute_path)
134-142: Specify UTF-8 when reading YAML config.Prevents platform-dependent decoding issues.
- with open(config_file) as file: + with open(config_file, encoding="utf-8") as file:docs/web/update-docs.py (6)
49-55: Open YAML with explicit UTF-8.Avoids locale surprises.
- with open(yaml_path) as yaml_file: + with open(yaml_path, encoding="utf-8") as yaml_file:
67-79: Exclude image links and reduce false positives in link extraction.The regex currently matches images (
) and everything, then filters later. Tighten the pattern to skip images up front.- link_pattern = re.compile(r'\[([^\]]+)\]\(([^\)]+)\)') + # negative lookbehind to exclude images, capture any non-`)` URL + link_pattern = re.compile(r'(?<!!)\[([^\]]+)\]\(([^)]+)\)')
138-145: Use POSIX separators for markdown links.
os.path.relpathmay produce backslashes on Windows. Convert to POSIX for markdown.- new_relative_link = os.path.relpath( - self.output_content_path / new_target, - target_path.parent - ) + new_relative_link = os.path.relpath( + self.output_content_path / new_target, + target_path.parent + ).replace("\\", "/")
200-228: Read/write markdown with UTF-8.Ensure consistent encoding for docs.
- with open(source_file_path) as readme_file: + with open(source_file_path, encoding="utf-8") as readme_file: @@ - with open(target_file_path, "w") as readme_file: + with open(target_file_path, "w", encoding="utf-8") as readme_file:
236-243: Prefer Path joins over string concatenation.Using strings risks subtle path issues; keep everything Path-native.
- source_dir = self.repository_path / source_path - for file in os.listdir(source_dir): + source_dir = self.repository_path / source_path + for file in os.listdir(source_dir): if file.endswith(".md"): - target_filename = "_index.md" if file == "README.md" else file - self.process_markdown_file(source_path + "/" + file, target_name + "/" + target_filename, weight) + target_filename = "_index.md" if file == "README.md" else file + src = str((Path(source_path) / file).as_posix()) + dst = str((Path(target_name) / target_filename).as_posix()) + self.process_markdown_file(src, dst, weight)
368-376: Harden get_git_root() error handling (reuse existing pattern in scripts/task-mcp/server.py).If git is unavailable or not a repo, this currently crashes with a raw CalledProcessError. Mirror the robust handling used elsewhere.
-def get_git_root(): - """Get the git repository root directory.""" - return subprocess.run( - ["git", "rev-parse", "--show-toplevel"], - stdout=subprocess.PIPE, - check=True, - text=True, - ).stdout.strip() +def get_git_root() -> str: + """Get the git repository root directory.""" + try: + result = subprocess.run( + ["git", "rev-parse", "--show-toplevel"], + stdout=subprocess.PIPE, + check=True, + text=True, + ) + except FileNotFoundError: + raise RuntimeError("Git executable not found. Please install Git and ensure it is on your PATH.") from None + except subprocess.CalledProcessError: + raise RuntimeError("Unable to locate git repository. Are you running this inside a Git repo?") from None + return result.stdout.strip()
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (15)
.flake8(0 hunks).pre-commit-config.yaml(2 hunks).vscode/extensions.json(1 hunks)CLAUDE.md(1 hunks)Taskfile.yaml(1 hunks)docker/labctl.py(9 hunks)docs/web/update-docs.py(7 hunks)ruff.toml(1 hunks)scripts/create-example-env.py(1 hunks)scripts/git-reorder-fixup.py(2 hunks)scripts/github-extract-links.py(3 hunks)scripts/github-star-repo.py(1 hunks)scripts/proxy-request-log.py(2 hunks)scripts/task-mcp/server.py(3 hunks)scripts/task-mcp/tools/find_app_icon.py(1 hunks)
💤 Files with no reviewable changes (1)
- .flake8
🚧 Files skipped from review as they are similar to previous changes (9)
- scripts/github-star-repo.py
- scripts/create-example-env.py
- Taskfile.yaml
- CLAUDE.md
- .vscode/extensions.json
- scripts/task-mcp/tools/find_app_icon.py
- scripts/proxy-request-log.py
- ruff.toml
- scripts/github-extract-links.py
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-08-06T20:29:29.605Z
Learnt from: CR
PR: bubacoder/infra#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-06T20:29:29.605Z
Learning: Use Pre-commit for code quality and security checks.
Applied to files:
.pre-commit-config.yaml
📚 Learning: 2025-08-06T20:29:29.605Z
Learnt from: CR
PR: bubacoder/infra#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-06T20:29:29.605Z
Learning: Applies to **/*.{sh,Dockerfile,yml,yaml,tf} : Follow the pre-commit rules defined in .pre-commit-config.yaml, which includes linting for shell scripts, Dockerfiles, YAML files, Ansible playbooks, and Terraform configurations.
Applied to files:
.pre-commit-config.yaml
📚 Learning: 2025-08-06T20:29:29.605Z
Learnt from: CR
PR: bubacoder/infra#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-06T20:29:29.605Z
Learning: Applies to docker/**/*.{yml,yaml} : Docker Compose files for services should be placed under the docker/ directory, organized by service type (e.g., security, media, storage, monitoring).
Applied to files:
docker/labctl.py
🧬 Code graph analysis (2)
docs/web/update-docs.py (1)
scripts/task-mcp/server.py (1)
get_git_root(32-45)
scripts/task-mcp/server.py (1)
scripts/task-mcp/tools/find_app_icon.py (1)
AppIconFinder(9-113)
⏰ 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: build
- GitHub Check: check
🔇 Additional comments (3)
scripts/task-mcp/server.py (2)
107-121: Good closure to bind loop variableThe factory avoids the classic late-binding bug when registering multiple tools. LGTM.
184-186: Health check endpoint looks goodSimple and sufficient for liveness.
docker/labctl.py (1)
145-176: Confirm intended semantics for 'update'.Current flow for 'update' performs both pull (with bake if build present) and then 'up --detach'. If ‘update’ was meant to only refresh images without redeploy, consider separating behaviors or documenting this.
| import logging | ||
| import os | ||
| import re | ||
| import subprocess | ||
| import sys | ||
| import os | ||
| from typing import Callable, Dict, List | ||
| from collections.abc import Callable | ||
|
|
||
| from fastmcp import FastMCP | ||
| from fastmcp.tools import Tool | ||
| from starlette.requests import Request | ||
| from starlette.responses import PlainTextResponse | ||
| from find_app_icon import AppIconFinder | ||
|
|
||
| from tools.find_app_icon import AppIconFinder | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add shared timeout, color-stripping, and reuse AppIconFinder instance
Centralize subprocess defaults and make parsing robust against ANSI output; also avoid re-instantiating AppIconFinder on every call.
import logging
+from pathlib import Path
import os
import re
import subprocess
import sys
from collections.abc import Callable
from fastmcp import FastMCP
from fastmcp.tools import Tool
from starlette.requests import Request
from starlette.responses import PlainTextResponse
from tools.find_app_icon import AppIconFinder
+
+# Subprocess defaults and robust parsing
+DEFAULT_TIMEOUT = int(os.getenv("TASK_MCP_TIMEOUT", "120"))
+NO_COLOR_ENV = {**os.environ, "NO_COLOR": "1"}
+ANSI_ESCAPE_RE = re.compile(r'\x1B\[[0-?]*[ -/]*[@-~]')
+
+# Reuse a single instance (lighter and easier to cache later if needed)
+ICON_FINDER = AppIconFinder()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import logging | |
| import os | |
| import re | |
| import subprocess | |
| import sys | |
| import os | |
| from typing import Callable, Dict, List | |
| from collections.abc import Callable | |
| from fastmcp import FastMCP | |
| from fastmcp.tools import Tool | |
| from starlette.requests import Request | |
| from starlette.responses import PlainTextResponse | |
| from find_app_icon import AppIconFinder | |
| from tools.find_app_icon import AppIconFinder | |
| import logging | |
| from pathlib import Path | |
| import os | |
| import re | |
| import subprocess | |
| import sys | |
| from collections.abc import Callable | |
| from fastmcp import FastMCP | |
| from fastmcp.tools import Tool | |
| from starlette.requests import Request | |
| from starlette.responses import PlainTextResponse | |
| from tools.find_app_icon import AppIconFinder | |
| # Subprocess defaults and robust parsing | |
| DEFAULT_TIMEOUT = int(os.getenv("TASK_MCP_TIMEOUT", "120")) | |
| NO_COLOR_ENV = {**os.environ, "NO_COLOR": "1"} | |
| ANSI_ESCAPE_RE = re.compile(r'\x1B\[[0-?]*[ -/]*[@-~]') | |
| # Reuse a single instance (lighter and easier to cache later if needed) | |
| ICON_FINDER = AppIconFinder() |
🤖 Prompt for AI Agents
In scripts/task-mcp/server.py around lines 7-20, centralize subprocess defaults
(e.g., timeout, capture_output/text) into a shared dict used for all
subprocess.run/call invocations, instantiate AppIconFinder once at module scope
and reuse that instance instead of creating it per-request, and strip ANSI color
codes from any subprocess output before parsing (use a compiled regex to remove
\x1b[...] sequences) so parsing is robust; also ensure subprocess calls handle
timeouts and non-zero return codes gracefully by using the shared kwargs and
checking returncode or catching subprocess.TimeoutExpired.
Summary by CodeRabbit
New Features
Refactor
Chores