Infra-MCP v2 with tools for container deployment - #201
Conversation
WalkthroughReplaces the Task MCP server with a new infra-mcp FastMCP server and tooling; adds infra-mcp package, container/tag/category/dashboard/icon discovery tools and collections; introduces DockerOptions and a --quiet flow in labctl; removes legacy scripts; updates MCP, Claude settings, docs, and lint/task config. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev as Developer
participant VSCode as VSCode MCP Client
participant InfraMCP as infra-mcp Server
participant TagFinder as ContainerTagFinder
participant Registry as Docker/Registry
Dev->>VSCode: Request most-specific tag for image
VSCode->>InfraMCP: get-most-specific-container-tag(image)
InfraMCP->>TagFinder: resolve tags & specificity
TagFinder->>Registry: HTTP fetch tags/manifests
Registry-->>TagFinder: tags/manifests
TagFinder-->>InfraMCP: selected tag
InfraMCP-->>VSCode: return tag
VSCode-->>Dev: display result
sequenceDiagram
autonumber
actor Dev as Developer
participant VSCode as VSCode MCP Client
participant InfraMCP as infra-mcp Server
participant CTools as Container Operation Tools
participant Labctl as scripts/labctl.py
participant Docker as Docker Engine
Dev->>VSCode: Invoke container operation (e.g., pull)
VSCode->>InfraMCP: MCP call (operation, service)
InfraMCP->>CTools: execute_container_operation(op, service)
CTools->>Labctl: run `python -m scripts.labctl service <op> <category>/<app> [--quiet]`
Labctl->>Docker: docker-compose/docker commands (honoring DockerOptions.quiet)
Docker-->>Labctl: stdout/stderr
Labctl-->>CTools: result text
CTools-->>InfraMCP: response
InfraMCP-->>VSCode: return output
VSCode-->>Dev: show output
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (3 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 |
There was a problem hiding this comment.
Actionable comments posted: 25
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
scripts/infra-mcp/start-server.sh (1)
1-23: ShellCheck hardening and resilienceTighten for ShellCheck and better failure handling: add -u/pipefail, guard cd, annotate source, and verify uv presence before use.
Apply this diff:
#!/usr/bin/env bash -set -e +set -euo pipefail # Change to script directory -cd "$(dirname "$0")" +cd "$(dirname "$0")" || { echo "Failed to change to script directory" >&2; exit 1; } # Check if virtual environment exists, create if it doesn't if [ ! -d ".venv" ]; then echo "Creating virtual environment..." - uv venv + if ! command -v uv >/dev/null 2>&1; then + echo "Error: 'uv' is required (https://docs.astral.sh/uv/) but not found in PATH." >&2 + exit 1 + fi + uv venv fi # Activate virtual environment +# shellcheck source=/dev/null source .venv/bin/activate # Install or update dependencies echo "Installing dependencies..." uv sync # Start the MCP server echo "Starting Infra MCP server..." -fastmcp run server.py --transport http --host 127.0.0.1 --port 9876 +exec fastmcp run server.py --transport http --host 127.0.0.1 --port 9876scripts/infra-mcp/pyproject.toml (1)
13-15: Add missing main() entrypoint or update pyproject mappingscripts/infra-mcp/pyproject.toml declares infra-mcp = "server:main" (scripts/infra-mcp/pyproject.toml:14) but scripts/infra-mcp/server.py has no def main(); either add a main() (e.g., call mcp.run() there) or update pyproject to point to an existing callable.
scripts/labctl.py (1)
32-33: Add 'update' or align docsDocs reference a docker:update task (AGENTS.md:39; docker/README.md:38; docker/guidelines.md:196) but scripts/labctl.py does not support it — ALLOWED_STATES lacks "update" (scripts/labctl.py:32) and docker_command has no "update" case. Either add "update" to ALLOWED_STATES and implement the intended update+restart behavior in docker_command/CLI, or remove/update the docs/tasks that advertise docker:update.
scripts/infra-mcp/tools/get_container_tags.py (1)
1-480: Fix Ruff errors in scripts/infra-mcp/tools/get_container_tags.pyRuff reported 12 issues — actions required:
- ARG002: unused
limitparameter in get_docker_hub_tags (line 32) and get_registry_tags (line 99) — remove or use the param.- S113:
requests.getcalls without timeout (lines 43, 66, 103, 115) — add a timeout or use a Session with a default timeout.- BLE001: bare
except Exception(lines 90, 154, 176) — catch specific exceptions (e.g., ValueError, TypeError) or handle parse failures explicitly.- TRY300:
returninsidetry(lines 94, 158) — move the return into anelseblock or restructure the try/except.- RUF059: unpacked unused variable
image_namein list_same_hash_tags (line 340) — prefix with_or remove.scripts/infra-mcp/server.py (1)
1-226: Fix Ruff errors in scripts//*.py (linting currently fails)**ruff reported 117 issues in scripts/ (11 auto-fixable with --fix, 33 additional hidden fixes with --unsafe-fixes). Run ruff --fix on scripts/, then address remaining issues; prioritize security flags (S603/S604 subprocess/shell=True), requests without timeouts (S113), bare Exception catches (BLE001), redundant logging.exception args (TRY401), implicit Optional annotations (RUF013), and unused-arg warnings (ARG001/ARG002).
🧹 Nitpick comments (16)
scripts/infra-mcp/tools/collections/__init__.py (1)
1-3: Collapse to a one‑line docstring (Ruff D200) and tweak phrasingOne-line docstrings should be on a single line; also using imperative mood helps avoid D401 in stricter configs.
-""" -Tool collections for Infra MCP server. -""" +"""Provide tool collections for the Infra MCP server."""docker/guidelines.md (5)
171-179: Clarify PUID/PGID usage and align with examples/templatesGood addition. Add brief guidance that many images (e.g., LSIO) expect PUID/PGID via env vars, while others prefer the Compose
user: ${PUID}:${PGID}directive. Ensure the template example shows PUID/PGID so readers wire it correctly.
235-241: Include PUID/PGID in the service template environmentAdd PUID/PGID to the template so new services inherit correct ownership patterns.
Apply this diff:
environment: TZ: ${TIMEZONE} + PUID: ${PUID} # Host user ID for file ownership (if image supports it) + PGID: ${PGID} # Host group ID for file ownership (if image supports it) # Service-specific variables
184-190: Add a note about host directory ownership and permissionsOwnership mismatches are a common pitfall. Add a short post-snippet note to preempt bind-mount permission issues.
Apply this diff:
- ./service-name/config:/config # Local configuration files, committed to the repository+Tip: Ensure host directories exist and are owned by the intended IDs:
+bash +sudo mkdir -p ${DOCKER_VOLUMES}/service-name ./service-name/config +sudo chown -R ${PUID}:${PGID} ${DOCKER_VOLUMES}/service-name ./service-name/config +
+
121-127: Quote Traefik rule label to avoid YAML parsing edge casesQuoting the rule value is safer and more portable across tooling.
Apply this diff:
- traefik.http.routers.service-name.rule: Host(`service.${MYDOMAIN}`) + traefik.http.routers.service-name.rule: "Host(`service.${MYDOMAIN}`)"
11-16: Use consistent service key naming in examplesTop section uses
main-servicewhile the template usesservice. Pick one for consistency to reduce confusion when copy/pasting.Also applies to: 231-236
GEMINI.md (2)
1-3: Doc title/content mismatch; likely copy-paste from AGENTS.mdThis file starts with "AGENTS.md" and references Claude. For GEMINI.md, adjust title and intro to be Gemini-specific or dedupe by linking to AGENTS.md.
Apply this diff to fix the title and intro:
-AGENTS.md - -This file provides guidance to Claude Code (claude.ai/code) when working with code in this repository. +# Gemini Code + +This file provides guidance to Gemini Code when working with code in this repository.
5-210: Duplicate content with CLAUDE.md; centralize to avoid driftThe body duplicates CLAUDE.md. Prefer a single source (AGENTS.md) and keep GEMINI.md/CLAUDE.md as thin pointers, or tailor content per agent.
scripts/infra-mcp/tools/collections/container_tools.py (2)
82-92: Docstring return mismatchFunction returns None; update docstring accordingly.
Apply this diff:
- Returns: - The number of tools added + Returns: + None
104-116: Consider passing --quiet to labctl to reduce noiseIf labctl supports a global --quiet, include it in cmd to make tool outputs cleaner.
scripts/infra-mcp/tools/collections/task_tools.py (2)
6-13: Import additions for path resolutionApply this diff:
import logging import re import subprocess from collections.abc import Callable +import os +import shutil
95-105: Docstring return mismatchFunction returns None; update docstring accordingly.
Apply this diff:
- Returns: - The number of tools added + Returns: + NoneCLAUDE.md (2)
1-3: Doc title/content mismatch with file nameStarts with "AGENTS.md". Make this Claude-specific or replace with a pointer to AGENTS.md.
Apply this diff:
-AGENTS.md +# Claude Code
5-210: Duplicated content with GEMINI.md; consider single sourceAvoid maintaining identical long-form content in multiple files. Centralize in AGENTS.md and link.
scripts/labctl.py (1)
132-134: Use absolute docker path to satisfy Ruff S607 and improve robustnessResolve docker via shutil.which and reuse the resolved path.
Apply this diff:
-def docker(cmd: list[str], env=None, stdin=None, stdout=None, stderr=None) -> None: - subprocess.run(["docker"] + cmd, env=env, stdin=stdin, stdout=stdout, stderr=stderr, check=True) +def docker(cmd: list[str], env=None, stdin=None, stdout=None, stderr=None) -> None: + subprocess.run([DOCKER_EXE] + cmd, env=env, stdin=stdin, stdout=stdout, stderr=stderr, check=True)Add this definition near the top (after logger setup):
import shutil DOCKER_EXE = shutil.which("docker") or "docker"scripts/infra-mcp/server.py (1)
1-31: Nit: server description mentions only task runner; update docstring to reflect infra tools.The top-level docstring still refers to task runner only. Consider updating to include dashboard and container tooling.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
scripts/infra-mcp/uv.lockis excluded by!**/*.lockscripts/task-mcp/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (23)
.claude/commands/implement-container-deployment.md(1 hunks).claude/commands/plan-container-deployment.md(2 hunks).claude/settings.json(1 hunks).vscode/mcp.json(1 hunks)AGENTS.md(1 hunks)CLAUDE.md(0 hunks)CLAUDE.md(1 hunks)GEMINI.md(1 hunks)docker/guidelines.md(1 hunks)scripts/get-container-tags.py(0 hunks)scripts/infra-mcp/README.md(2 hunks)scripts/infra-mcp/pyproject.toml(1 hunks)scripts/infra-mcp/server.py(1 hunks)scripts/infra-mcp/start-server.sh(1 hunks)scripts/infra-mcp/tools/__init__.py(1 hunks)scripts/infra-mcp/tools/collections/__init__.py(1 hunks)scripts/infra-mcp/tools/collections/container_tools.py(1 hunks)scripts/infra-mcp/tools/collections/task_tools.py(1 hunks)scripts/infra-mcp/tools/get_app_icon.py(1 hunks)scripts/infra-mcp/tools/get_container_tags.py(1 hunks)scripts/infra-mcp/tools/get_dashboard_groups.py(1 hunks)scripts/labctl.py(10 hunks)scripts/task-mcp/server.py(0 hunks)
💤 Files with no reviewable changes (2)
- scripts/get-container-tags.py
- scripts/task-mcp/server.py
🧰 Additional context used
📓 Path-based instructions (2)
scripts/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Python code must pass Ruff linting
Files:
scripts/infra-mcp/tools/__init__.pyscripts/infra-mcp/tools/collections/__init__.pyscripts/infra-mcp/tools/get_app_icon.pyscripts/infra-mcp/tools/collections/container_tools.pyscripts/labctl.pyscripts/infra-mcp/tools/get_dashboard_groups.pyscripts/infra-mcp/tools/collections/task_tools.pyscripts/infra-mcp/server.pyscripts/infra-mcp/tools/get_container_tags.py
**/*.sh
📄 CodeRabbit inference engine (CLAUDE.md)
Shell scripts must pass ShellCheck
Files:
scripts/infra-mcp/start-server.sh
🧠 Learnings (1)
📚 Learning: 2025-08-31T08:38:34.585Z
Learnt from: CR
PR: bubacoder/infra#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-31T08:38:34.585Z
Learning: Manage Docker services using the scripts/labctl.py tool (operations: up, down, restart, recreate, update, pull, config)
Applied to files:
.claude/commands/implement-container-deployment.mdscripts/labctl.py
🧬 Code graph analysis (7)
scripts/infra-mcp/tools/get_app_icon.py (1)
scripts/task-mcp/tools/find_app_icon.py (3)
main(150-170)AppIconFinder(12-123)test_icon_finder(126-147)
scripts/infra-mcp/tools/collections/container_tools.py (1)
scripts/task-mcp/server.py (1)
control_container_service(124-161)
.vscode/mcp.json (1)
scripts/task-mcp/server.py (1)
control_container_service(124-161)
scripts/labctl.py (1)
scripts/task-mcp/server.py (1)
control_container_service(124-161)
scripts/infra-mcp/tools/get_dashboard_groups.py (1)
scripts/infra-mcp/server.py (2)
get_git_root(33-46)get_dashboard_groups(75-87)
scripts/infra-mcp/tools/collections/task_tools.py (1)
scripts/task-mcp/server.py (4)
get_task_list(48-81)execute_task(84-104)create_task_function(107-120)task_fn(117-118)
scripts/infra-mcp/server.py (6)
scripts/infra-mcp/tools/collections/container_tools.py (1)
add_container_operation_tools(82-117)scripts/infra-mcp/tools/collections/task_tools.py (1)
add_task_tools(95-121)scripts/infra-mcp/tools/get_app_icon.py (2)
get_app_icon(26-49)AppIconFinder(12-123)scripts/infra-mcp/tools/get_container_tags.py (4)
ContainerTagFinder(12-432)get_image_tags(280-297)list_same_hash_tags(329-383)get_most_specific_tag(385-432)scripts/infra-mcp/tools/get_dashboard_groups.py (3)
get_dashboard_groups(34-56)DashboardGroupFinder(22-56)get_git_root(10-19)scripts/task-mcp/server.py (1)
get_task_list(48-81)
🪛 Ruff (0.13.1)
scripts/infra-mcp/tools/collections/container_tools.py
45-45: subprocess call: check for execution of untrusted input
(S603)
51-51: Consider moving this statement to an else block
(TRY300)
scripts/infra-mcp/tools/get_dashboard_groups.py
15-15: Starting a process with a partial executable path
(S607)
75-75: Do not catch blind exception: Exception
(BLE001)
scripts/infra-mcp/tools/collections/task_tools.py
27-27: subprocess call: check for execution of untrusted input
(S603)
28-28: Starting a process with a partial executable path
(S607)
48-48: Consider moving this statement to an else block
(TRY300)
50-50: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
67-67: subprocess call: check for execution of untrusted input
(S603)
68-68: Starting a process with a partial executable path
(S607)
74-74: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
scripts/infra-mcp/server.py
37-37: Starting a process with a partial executable path
(S607)
43-43: Avoid specifying long messages outside the exception class
(TRY003)
45-45: Avoid specifying long messages outside the exception class
(TRY003)
50-50: Unused function argument: request
(ARG001)
70-70: Redundant exception object included in logging.exception call
(TRY401)
86-86: Redundant exception object included in logging.exception call
(TRY401)
118-118: Redundant exception object included in logging.exception call
(TRY401)
123-123: PEP 484 prohibits implicit Optional
Convert to Optional[T]
(RUF013)
153-153: Redundant exception object included in logging.exception call
(TRY401)
158-158: PEP 484 prohibits implicit Optional
Convert to Optional[T]
(RUF013)
203-203: Redundant exception object included in logging.exception call
(TRY401)
216-216: Do not catch blind exception: Exception
(BLE001)
217-217: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
scripts/infra-mcp/tools/get_container_tags.py
32-32: Unused method argument: limit
(ARG002)
43-43: Probable use of requests call without timeout
(S113)
66-66: Probable use of requests call without timeout
(S113)
90-90: Do not catch blind exception: Exception
(BLE001)
94-94: Consider moving this statement to an else block
(TRY300)
99-99: Unused method argument: limit
(ARG002)
103-103: Probable use of requests call without timeout
(S113)
115-115: Probable use of requests call without timeout
(S113)
154-154: Do not catch blind exception: Exception
(BLE001)
158-158: Consider moving this statement to an else block
(TRY300)
176-176: Do not catch blind exception: Exception
(BLE001)
340-340: Unpacked variable image_name is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
🪛 markdownlint-cli2 (0.18.1)
AGENTS.md
174-174: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🪛 Biome (2.1.2)
.claude/settings.json
[error] 20-20: Expected a property but instead found '// 10 minutes'.
Expected a property here.
(parse)
[error] 21-21: expected , but instead found "BASH_MAX_TIMEOUT_MS"
Remove "BASH_MAX_TIMEOUT_MS"
(parse)
[error] 21-21: expected , but instead found // 30 minutes
Remove // 30 minutes
(parse)
⏰ 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 (8)
scripts/infra-mcp/tools/__init__.py (1)
1-3: LGTMMinimal, clear docstring. No issues for Ruff.
scripts/infra-mcp/tools/get_app_icon.py (1)
154-154: LGTMProg name updated to match filename. Tool behavior unchanged and remains sound.
scripts/infra-mcp/README.md (1)
23-23: Path update LGTMUsing scripts/infra-mcp matches the refactor and server placement.
.claude/commands/implement-container-deployment.md (1)
26-26: Reflects new quiet mode correctlyThe --quiet flag matches the new CLI behavior in labctl.
.vscode/mcp.json (1)
22-22: MCP server path updated correctlySwapping to scripts/infra-mcp is consistent with the rename/migration.
.claude/commands/plan-container-deployment.md (1)
18-21: Tooling path updates LGTMSwitching to scripts/infra-mcp tools is aligned with the new flow.
scripts/labctl.py (1)
143-151: Verify flags: 'docker compose build/pull --quiet'
Confirm target Docker/Compose versions accept --quiet for both build and pull; otherwise use -q or add a version-aware fallback. Local checks: rundocker compose build --helpanddocker compose pull --help.
File: scripts/labctl.py (lines 143-151)scripts/infra-mcp/tools/get_container_tags.py (1)
385-432: Return type already explicit; no change needed. LGTM overall.Please verify rate-limits and error paths against your registries. Optionally add retries/backoff if needed.
8bc99ce to
06e102f
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
ruff.toml (2)
35-39: Align known-first-party with actual package names (ruff.toml:35-39)
srcandtoolsdon’t map to any top-level packages (onlycollectionswas found); updateknown-first-partyto list your real import roots.
3-3: Align ruff target-version to minimum supported Python (py310).
Updateruff.tomlline 3 fromtarget-version = "py313"to
target-version = "py310"to match
requires-python = ">=3.10"inscripts/infra-mcp/pyproject.toml.docs/web/update-docs.py (1)
283-294: Fix logic: function returns None when no labels found; pipeline crash.Returning inside the service loop (else: return {}) skips other services and missing a final default return yields None, causing AttributeError at metadata.setdefault.
- if homepage_icon or homepage_description or homepage_name: - return { - "name": homepage_name, - "description": homepage_description, - "icon": homepage_icon, - } - else: - return {} + if homepage_icon or homepage_description or homepage_name: + return { + "name": homepage_name, + "description": homepage_description, + "icon": homepage_icon, + } + # No matching labels found in any service + return {}This should resolve the CI error: AttributeError: 'NoneType' object has no attribute 'setdefault'.
🧹 Nitpick comments (26)
.claude/commands/fix-lint-issue.md (5)
5-5: Clarify parameter name and support multiple codes/descriptions.Rename to ISSUE_QUERY and document comma-separated support; ensures grep-friendly queries and multi-code runs.
-ISSUE_ID: $ARGUMENTS +ISSUE_QUERY: $ARGUMENTS # supports comma-separated codes (e.g., S607,S603) or text substrings
9-11: Make filtering actionable with concrete commands (ruff/eslint/golangci-lint + generic grep).Add exact commands so this is executable, not aspirational.
-1. Run "task lint" to identify all lint issues -2. Filter for issues matching "ISSUE_ID" (can be a specific error code like S607 or a description like "subprocess") +1. Run "task lint" from repo root to list all issues (capture output for filtering) + - Example: task lint | tee lint.out +2. Filter for issues matching "ISSUE_QUERY": + - Generic grep: rg -n "$ISSUE_QUERY" lint.out + - Ruff (Python): ruff check --select "$ISSUE_QUERY" --output-format full . + - Multiple codes: ruff check --select "$(echo "$ISSUE_QUERY" | tr , ' ')" . + - ESLint (JS/TS): eslint . --max-warnings=0 | rg -n "$ISSUE_QUERY" + - Single rule only: eslint . --rule "$ISSUE_QUERY:error" + - golangci-lint (Go): golangci-lint run ./... | rg -n "$ISSUE_QUERY" + - ShellCheck (shell): shellcheck -S style -f gcc $(fd -e sh -e bash) | rg -n "$ISSUE_QUERY" + - Hadolint (Dockerfiles): hadolint $(fd -g 'Dockerfile*') | rg -n "$ISSUE_QUERY"
13-16: Standardize fix report format (file path + fenced code + unified diff).Ensures consistent, scannable documentation for each fix.
-4. For each fixed issue: - - Explain the problem and why it's a concern - - Show the original code - - Explain your fix and its benefits +4. For each fixed issue (repeat per file/occurrence): + - File and location: path:line(s) + - Why it matters: brief risk/impact + - Before (code): use fenced block with language + ```lang + <original snippet> + ``` + - After (code): fenced block + ```lang + <fixed snippet> + ``` + - Diff (optional but preferred): + ```diff + - <before line> + + <after line> + ``` + - Benefits: what improved (security/perf/readability/compliance)
16-17: Strengthen verification: assert zero exit, rerun targeted check, run tests.Avoid regressions and confirm the specific rule is clean.
-5. After implementing fixes, run "task lint" again to verify the issues are resolved +5. After implementing fixes: + - Re-run the full lint: task lint (expect success/zero exit) + - Re-run targeted check for ISSUE_QUERY (expect no matches) + - Run tests to catch functional regressions: task test (or equivalent)
19-23: Expand Notes: suppression policy, config changes over inline ignores, and security guidance.Codifies guardrails and consistent handling across tools.
Notes: - Focus only on issues matching the specified ISSUE_ID - Apply consistent fixes across similar issues - For Python security issues (S-prefixed codes), consult Python security best practices - For other issues, follow the appropriate linting tool's recommendations + - Prefer automated fixes where safe: + - Ruff: ruff check --select "$(echo "$ISSUE_QUERY" | tr , ' ')" --fix . + - ESLint: eslint . --rule "$ISSUE_QUERY:error" --fix + - Suppress only when necessary and with justification: + - Prefer central config (e.g., ruff.toml, .eslintrc, golangci.yml) over inline ignores + - If inline is required (e.g., # noqa: S607), add a brief reason and scope it narrowly + - If a rule is noisy or not applicable, propose a config change with rationale in the PR + - For security rules (Ruff S*/Bandit): avoid unsafe patterns (shell=True, eval/exec, weak crypto, hardcoded secrets); prefer subprocess list args with timeouts, safe loaders (yaml.safe_load), etc. + - Validate that fixes don’t alter behavior without tests; add/adjust tests when neededscripts/github-star-repo.py (5)
36-36: Good addition—set separate connect/read timeouts.Use a tuple to avoid long connect hangs while keeping a generous read window.
- response = requests.put(api_url, headers=headers, timeout=30) + response = requests.put(api_url, headers=headers, timeout=(5, 30))
21-24: Harden repo URL parsing (.git suffix, extra path segments).Avoid split failures and handle common URL forms.
- owner, repo_name = repo_path.split('/') + parts = [p for p in repo_path.split('/') if p] + if len(parts) < 2: + raise ValueError("Invalid GitHub repository URL (expected https://github.com/<owner>/<repo>)") + owner, repo_name = parts[0], parts[1] + if repo_name.endswith(".git"): + repo_name = repo_name[:-4]
28-35: Fail fast when GITHUB_API_TOKEN is missing; include API version header.Clear feedback saves time, and the version header aligns with GitHub’s guidance.
- github_token = os.environ.get("GITHUB_API_TOKEN") + github_token = os.environ.get("GITHUB_API_TOKEN") + if not github_token: + print("GITHUB_API_TOKEN is not set; skipping star operation.") + return @@ - headers = { - "Authorization": f"Bearer {github_token}", - "Accept": "application/vnd.github+json" - } + headers = { + "Authorization": f"token {github_token}", + "Accept": "application/vnd.github+json", + "X-GitHub-Api-Version": "2022-11-28", + }
39-44: Treat idempotent “already starred” as success and clarify 401.Prevents false negatives on re-runs and improves error messaging.
- if response.status_code == 204: + if response.status_code in (204, 304): print(f"Successfully starred repository: {repo_url}") - else: + elif response.status_code == 401: + print(f"Failed to star repository: {repo_url}") + print("Unauthorized (401): check GITHUB_API_TOKEN presence/permissions.") + else: print(f"Failed to star repository: {repo_url}") print(f"Response status code: {response.status_code}") print(f"Response content: {response.text}")
50-52: Only show prompt in interactive TTY.Prevents noisy output when piped.
- print("Enter a GitHub repository URL (or press Enter to exit):") + if sys.stdin.isatty(): + print("Enter a GitHub repository URL (or press Enter to exit):")Add at the top of the file:
import sysruff.toml (3)
14-14: Add per-file ignores for tests to avoid Bandit S101 churn.Bandit flags
assert(S101), which is idiomatic in tests. Add a per-file ignore for tests.[lint] select = [ "E", # pycodestyle - PEP 8 style guide errors and conventions @@ "FAST", # FastAPI - FastAPI-specific best practices ] +[lint.per-file-ignores] +# Allow asserts in tests +"tests/**" = ["S101"]
18-18: TRY can be noisy; consider preemptive targeted ignores only if it blocks adoption.TRY003/TRY200/TRY201 can be high-noise in legacy code. If rollout gets blocked, selectively ignore them instead of disabling TRY entirely.
Example (only if needed):
[lint.extend-ignore] # Avoid blanket ignores; start narrow if these cause excessive churn: codes = ["TRY003", "TRY200", "TRY201"]
29-33: Broaden script exclusions to future-proof third‑party/vendor scripts.Current exclude lists specific files. Consider excluding the entire scripts folder to avoid linting vendored utilities.
exclude = [ - "scripts/git-filter-repo.py", - "scripts/test-colors.py", + "scripts/**", ]scripts/infra-mcp/tools/get_app_icon.py (1)
118-124: Add GET fallback for /favicon.ico when HEAD is blocked (403/405).Some origins disallow HEAD on favicon. Mirror the earlier CDN fallback to improve hit rate.
- if favicon_response.status_code == 200: - return default_favicon - else: - return None + if favicon_response.status_code == 200: + return default_favicon + # Fallback to GET if HEAD is blocked + if favicon_response.status_code in (403, 405): + with requests.get(default_favicon, headers=self.headers, timeout=5) as probe: + if probe.ok: + return default_favicon + return Nonescripts/infra-mcp/tools/get_container_tags.py (5)
9-10: Introduce a shared timeout constant for HTTP calls.Centralize timeouts and reuse across requests.
import requests +TIMEOUT = (5, 15) # (connect, read) seconds
70-85: Cap collection by limit inside the loop.Stop collecting once limit is reached to avoid unnecessary work.
tag_data.append({ 'name': tag['name'], 'last_updated': tag.get('last_updated'), 'size': tag.get('full_size', 0), 'digest': arch_digest }) + if len(tag_data) >= limit: + break
79-85: Same cap in pagination loop.tag_data.append({ 'name': tag['name'], 'last_updated': tag.get('last_updated'), 'size': tag.get('full_size', 0), 'digest': arch_digest }) + if len(tag_data) >= limit: + break
173-179: Narrow caught exception in datetime formatter (BLE001).Avoid blanket Exception.
- except Exception: + except (TypeError, ValueError): return datetime_str
341-343: Prefix unused variable to satisfy Ruff (RUF059).image_name is unused; mark it as intentionally unused.
- all_tags, _, image_name, _ = self.get_image_tags(args, limit=1000) + all_tags, _, _image_name, _ = self.get_image_tags(args, limit=1000)scripts/infra-mcp/README.md (1)
7-10: Update features to reflect new Infra MCP tools, not only Task toolsREADME still focuses on Task-driven dynamic tools. Please add the newly added infra tools (e.g., service-ops via labctl, get-dashboard-groups, get-app-icon, list-container-tags, get-most-specific-container-tag) to avoid confusing users.
Apply this diff to expand the Features section:
-## Features - -- Dynamically generates FastMCP tools from the output of `task --list-all` -- Each task becomes a callable tool in the MCP server -- Uses UV for dependency management +## Features + +- Dynamically generates FastMCP tools from the output of `task --list-all` (each task becomes a callable tool) +- Infrastructure tools: + - Service operations via labctl: `service-pull`, `service-up`, `service-down`, `service-restart`, `service-recreate`, `service-config` + - Discovery helpers: `get-dashboard-groups`, `get-app-icon`, `list-container-tags`, `list-same-hash-container-tags`, `get-most-specific-container-tag` +- Uses UV for dependency managementscripts/infra-mcp/start-server.sh (1)
4-22: Make script ShellCheck-clean and resilient (cd guard, missing uv, source directive)Harden the startup script and silence common ShellCheck warnings (SC2164, SC1091).
Apply this diff:
-#!/usr/bin/env bash -set -e +#!/usr/bin/env bash +set -Eeuo pipefail + +# Abort if script directory change fails -cd "$(dirname "$0")" +cd "$(dirname "$0")" || exit 1 @@ -# Check if virtual environment exists, create if it doesn't +# Check if UV is installed +if ! command -v uv >/dev/null 2>&1; then + echo "uv is not installed. See https://docs.astral.sh/uv/ (e.g.,: pipx install uv)" + exit 1 +fi + +# Check if virtual environment exists, create if it doesn't if [ ! -d ".venv" ]; then echo "Creating virtual environment..." uv venv fi @@ -# Activate virtual environment -source .venv/bin/activate +# Activate virtual environment +# shellcheck source=/dev/null +source .venv/bin/activate @@ -echo "Starting Infra MCP server..." +echo "Starting Infra MCP server..." fastmcp run server.py --transport http --host 127.0.0.1 --port 9876scripts/infra-mcp/tools/collections/container_tools.py (2)
45-51: Add timeout and mark subprocess as safe post-validationHelps avoid hanging calls and placates Ruff S603 after input checks.
Apply this diff:
- result = subprocess.run( + result = subprocess.run( # noqa: S603 cmd, capture_output=True, text=True, - check=True + check=True, + timeout=600 )
83-96: Fix docstring: function returns None, not a countThe docstring claims a return value but the function returns None.
Apply this diff:
-def add_container_operation_tools(mcp_server: FastMCP, repository_root_path: str) -> None: +def add_container_operation_tools(mcp_server: FastMCP, repository_root_path: str) -> None: @@ - Returns: - The number of tools added + Returns: + Nonescripts/infra-mcp/server.py (1)
210-214: Use parameterized loggingAvoid f-strings in logging to defer formatting.
Apply this diff:
- logger.info(f"Repository root path: {repository_root_path}") + logger.info("Repository root path: %s", repository_root_path)scripts/labctl.py (2)
205-210: Add encoding and guard None when loading YAMLPrevent surprises on non-UTF8 systems and empty files.
Apply this diff:
- with open(config_file) as file: - config = yaml.safe_load(file) - return config + with open(config_file, encoding="utf-8") as file: + config = yaml.safe_load(file) or {} + return config
132-134: Optional: resolve docker path once and reuse (Ruff S607 hardening)Consider resolving the docker executable via shutil.which and erroring early; also pass explicit types to subprocess.run. This improves robustness.
Here’s a possible rewrite outside the changed hunk:
import shutil _DOCKER = shutil.which("docker") if _DOCKER is None: raise RuntimeError("docker not found on PATH") def docker(cmd: list[str], env=None, stdin=None, stdout=None, stderr=None) -> None: subprocess.run([_DOCKER, *cmd], env=env, stdin=stdin, stdout=stdout, stderr=stderr, check=True)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
scripts/infra-mcp/uv.lockis excluded by!**/*.lockscripts/task-mcp/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (23)
.claude/commands/fix-lint-issue.md(1 hunks).claude/commands/plan-container-deployment.md(2 hunks).claude/settings.json(1 hunks).vscode/mcp.json(1 hunks)AGENTS.md(1 hunks)docker/guidelines.md(1 hunks)docs/web/update-docs.py(2 hunks)ruff.toml(1 hunks)scripts/get-container-tags.py(0 hunks)scripts/github-star-repo.py(1 hunks)scripts/infra-mcp/README.md(2 hunks)scripts/infra-mcp/pyproject.toml(1 hunks)scripts/infra-mcp/server.py(1 hunks)scripts/infra-mcp/start-server.sh(1 hunks)scripts/infra-mcp/tools/__init__.py(1 hunks)scripts/infra-mcp/tools/collections/__init__.py(1 hunks)scripts/infra-mcp/tools/collections/container_tools.py(1 hunks)scripts/infra-mcp/tools/collections/task_tools.py(1 hunks)scripts/infra-mcp/tools/get_app_icon.py(3 hunks)scripts/infra-mcp/tools/get_container_tags.py(1 hunks)scripts/infra-mcp/tools/get_dashboard_groups.py(1 hunks)scripts/labctl.py(11 hunks)scripts/task-mcp/server.py(0 hunks)
💤 Files with no reviewable changes (2)
- scripts/task-mcp/server.py
- scripts/get-container-tags.py
✅ Files skipped from review due to trivial changes (1)
- scripts/infra-mcp/tools/init.py
🚧 Files skipped from review as they are similar to previous changes (3)
- scripts/infra-mcp/tools/collections/init.py
- AGENTS.md
- docker/guidelines.md
🧰 Additional context used
📓 Path-based instructions (2)
scripts/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Python code must pass Ruff linting
Files:
scripts/infra-mcp/tools/get_dashboard_groups.pyscripts/infra-mcp/tools/get_app_icon.pyscripts/github-star-repo.pyscripts/labctl.pyscripts/infra-mcp/tools/get_container_tags.pyscripts/infra-mcp/server.pyscripts/infra-mcp/tools/collections/container_tools.pyscripts/infra-mcp/tools/collections/task_tools.py
**/*.sh
📄 CodeRabbit inference engine (CLAUDE.md)
Shell scripts must pass ShellCheck
Files:
scripts/infra-mcp/start-server.sh
🧠 Learnings (1)
📚 Learning: 2025-08-31T08:38:34.585Z
Learnt from: CR
PR: bubacoder/infra#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-31T08:38:34.585Z
Learning: Manage Docker services using the scripts/labctl.py tool (operations: up, down, restart, recreate, update, pull, config)
Applied to files:
scripts/labctl.py
🧬 Code graph analysis (5)
scripts/infra-mcp/tools/get_dashboard_groups.py (1)
scripts/infra-mcp/server.py (2)
get_git_root(34-47)get_dashboard_groups(76-88)
scripts/infra-mcp/tools/get_app_icon.py (1)
scripts/task-mcp/tools/find_app_icon.py (7)
AppIconFinder(12-123)main(150-170)_find_dashboard_icon(51-72)test_icon_finder(126-147)_find_favicon_url(74-123)get_app_icon(26-49)__init__(18-24)
scripts/infra-mcp/tools/get_container_tags.py (3)
scripts/infra-mcp/tools/get_app_icon.py (1)
main(154-174)scripts/infra-mcp/tools/get_dashboard_groups.py (1)
main(59-77)scripts/labctl.py (1)
main(291-322)
scripts/infra-mcp/server.py (6)
scripts/infra-mcp/tools/collections/container_tools.py (1)
add_container_operation_tools(83-118)scripts/infra-mcp/tools/collections/task_tools.py (1)
add_task_tools(95-121)scripts/infra-mcp/tools/get_app_icon.py (2)
get_app_icon(26-49)AppIconFinder(12-127)scripts/infra-mcp/tools/get_container_tags.py (4)
ContainerTagFinder(12-434)get_image_tags(282-299)list_same_hash_tags(331-385)get_most_specific_tag(387-434)scripts/infra-mcp/tools/get_dashboard_groups.py (3)
get_dashboard_groups(34-56)DashboardGroupFinder(22-56)get_git_root(10-19)scripts/task-mcp/server.py (2)
control_container_service(124-161)get_task_list(48-81)
scripts/infra-mcp/tools/collections/task_tools.py (1)
scripts/task-mcp/server.py (4)
get_task_list(48-81)execute_task(84-104)create_task_function(107-120)task_fn(117-118)
🪛 Ruff (0.13.1)
scripts/infra-mcp/tools/get_dashboard_groups.py
15-15: Starting a process with a partial executable path
(S607)
75-75: Do not catch blind exception: Exception
(BLE001)
scripts/infra-mcp/tools/get_container_tags.py
32-32: Unused method argument: limit
(ARG002)
90-90: Do not catch blind exception: Exception
(BLE001)
100-100: Unused method argument: limit
(ARG002)
155-155: Do not catch blind exception: Exception
(BLE001)
178-178: Do not catch blind exception: Exception
(BLE001)
342-342: Unpacked variable image_name is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
scripts/infra-mcp/server.py
38-38: Starting a process with a partial executable path
(S607)
44-44: Avoid specifying long messages outside the exception class
(TRY003)
46-46: Avoid specifying long messages outside the exception class
(TRY003)
124-124: PEP 484 prohibits implicit Optional
Convert to Optional[T]
(RUF013)
159-159: PEP 484 prohibits implicit Optional
Convert to Optional[T]
(RUF013)
scripts/infra-mcp/tools/collections/container_tools.py
45-45: subprocess call: check for execution of untrusted input
(S603)
scripts/infra-mcp/tools/collections/task_tools.py
27-27: subprocess call: check for execution of untrusted input
(S603)
28-28: Starting a process with a partial executable path
(S607)
67-67: subprocess call: check for execution of untrusted input
(S603)
68-68: Starting a process with a partial executable path
(S607)
🪛 GitHub Actions: Documentation site
docs/web/update-docs.py
[error] 310-310: AttributeError: 'NoneType' object has no attribute 'setdefault' when calling metadata.setdefault(...) in process_docker_compose_file (update-docs.py:310). This occurs during the Docker build RUN step that executes /repo/docs/web/update-docs.py.
⏰ 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 (31)
.claude/commands/fix-lint-issue.md (1)
9-9: Confirm ‘task lint’ entrypoint exists
The root Taskfile.yaml defines a lint target that runs pre-commit on all files; no changes needed.scripts/github-star-repo.py (1)
32-35: Choose the correct Authorization header scheme
Classic PATs requireAuthorization: token <PAT>(https://developer.github.com/changes/2/?utm_source=openai); fine-grained PATs and GitHub App installation tokens requireAuthorization: Bearer <TOKEN>(https://docs.github.com/en/enterprise-cloud%40latest/rest/orgs/personal-access-tokens?utm_source=openai, https://docs.github.com/en/enterprise-server%403.17/apps/creating-github-apps/authenticating-with-a-github-app/authenticating-as-a-github-app-installation?utm_source=openai). Ensure your header matches the token you supply.ruff.toml (1)
8-21: Good addition: enable security and exception-handling linting.Adding flake8-bandit (S) and tryceratops (TRY) is a solid move for catching security issues and brittle exception patterns.
scripts/infra-mcp/tools/get_app_icon.py (1)
158-158: Parser program name update looks good.Renaming prog to "get_app_icon.py" aligns with the new tool path.
.claude/commands/plan-container-deployment.md (3)
18-21: Tooling path updates look correct.The uv run invocations align with the new infra-mcp tooling and CLI flags.
24-26: Flow change LGTM.Deferring YAML persistence and focusing on markdown output is clearly stated.
37-39: Template fields updated appropriately.Icon source and Dashboard Group references match the new tools. Typo from a prior review (“matching”) is correct here.
.claude/settings.json (2)
7-14: Expanded permissions align with new infra-mcp usage.Allow-list entries for hub.docker.com, tree, uv, and scripts paths look consistent.
18-22: JSON is valid; env timeouts configured.Inline comments removed as previously flagged; Biome/JSON parsers should pass.
scripts/infra-mcp/pyproject.toml (1)
9-10: beautifulsoup4 constraint likely invalid (4.13 doesn’t exist on PyPI).Pin to a known current release to avoid resolver failures.
- "beautifulsoup4>=4.13,<5", + "beautifulsoup4>=4.12.3,<5",scripts/infra-mcp/tools/collections/task_tools.py (2)
65-75: Validate task name; resolve binary; improve error reporting (Ruff S603/S607).Defend against invalid names and use resolved binary path.
- logger.info(f"Executing task: {task_name}") + # Allow alphanumerics, colon, dash, underscore (Taskfile namespaces) + if not re.match(r'^[A-Za-z0-9:_-]+$', task_name): + return f"Invalid task name: {task_name!r}" + logger.info("Executing task: %s", task_name) try: - return subprocess.run( - ["task", task_name, "--dir", repository_root_path], + task_bin = shutil.which("task") + if not task_bin: + return "Error: 'task' binary not found in PATH." + repo_path = os.path.abspath(repository_root_path) + return subprocess.run( # noqa: S603 + [task_bin, task_name, "--dir", repo_path], capture_output=True, text=True, check=True ).stdout.strip() - except subprocess.CalledProcessError as e: - logger.exception(f"Error executing task {task_name}") - return f"Error executing task {task_name}: {e.stderr}" + except subprocess.CalledProcessError as e: + logger.exception("Error executing task %s", task_name) + return f"Error executing task {task_name}: {e.stderr or str(e)}"
27-33: Resolve task binary and use absolute paths; annotate subprocess for Ruff (S603/S607).Harden the call and satisfy linting.
- result = subprocess.run( - ["task", "--list-all", "--dir", repository_root_path], + task_bin = shutil.which("task") + if not task_bin: + logger.error("The 'task' binary was not found in PATH.") + return [] + repo_path = os.path.abspath(repository_root_path) + result = subprocess.run( # noqa: S603 + [task_bin, "--list-all", "--dir", repo_path], capture_output=True, text=True, check=True )scripts/infra-mcp/tools/get_container_tags.py (2)
100-164: Use limit, shared timeout, and narrower exceptions for registry queries (ARG002, TRY300, S113).Honor the limit, reuse TIMEOUT, narrow exception in _httpdate, and return outside the try.
- def get_registry_tags(self, registry_url: str, image_name: str, limit: int = 10, architecture: str = "linux/amd64") -> list[dict[str, Any]]: + def get_registry_tags(self, registry_url: str, image_name: str, limit: int = 10, architecture: str = "linux/amd64") -> list[dict[str, Any]]: @@ - url: str = f"{registry_url}/v2/{image_name}/tags/list" - try: - response = requests.get(url, timeout=30) + url: str = f"{registry_url}/v2/{image_name}/tags/list" + tag_data: list[dict[str, Any]] = [] + try: + response = requests.get(url, timeout=TIMEOUT) response.raise_for_status() data = response.json() tags: list[str] = data.get('tags', []) # For Docker Registry API v2, we need to make additional requests to get manifest and timestamps - tag_data: list[dict[str, Any]] = [] - for tag in tags[:100]: # Limit the number of additional requests + for tag in tags[: max(1, min(int(limit), 1000))]: manifest_url = f"{registry_url}/v2/{image_name}/manifests/{tag}" try: # Try to get the manifest to extract creation time headers = {'Accept': 'application/vnd.docker.distribution.manifest.v2+json'} - manifest_response = requests.get(manifest_url, headers=headers, timeout=30) + manifest_response = requests.get(manifest_url, headers=headers, timeout=TIMEOUT) manifest_response.raise_for_status() @@ - def _httpdate(dt_str): + def _httpdate(dt_str): try: return parsedate_to_datetime(dt_str) - except Exception: + except (TypeError, ValueError): return datetime.min tag_data.sort(key=lambda x: _httpdate(x['last_updated']) if x['last_updated'] else datetime.min, reverse=True) - except requests.exceptions.RequestException as e: - print(f"Error querying registry: {e}", file=sys.stderr) - return [] - else: - return tag_data + except requests.exceptions.RequestException as e: + print(f"Error querying registry: {e}", file=sys.stderr) + tag_data = [] + return tag_data
32-99: Honor limit, reduce page size, narrow exceptions, and move return outside try (ARG002, TRY300).Currently limit is unused and returns occur in an else block. Use limit to constrain page size/pagination, narrow catch in _iso, and return after try/except.
- def get_docker_hub_tags(self, image_name: str, limit: int = 10, architecture: str = "linux/amd64") -> list[dict[str, Any]]: + def get_docker_hub_tags(self, image_name: str, limit: int = 10, architecture: str = "linux/amd64") -> list[dict[str, Any]]: @@ - url: str = f"https://hub.docker.com/v2/repositories/{namespace}/{repo}/tags?page_size=100" - try: - response = requests.get(url, timeout=30) + page_size = max(1, min(100, int(limit))) + url: str = f"https://hub.docker.com/v2/repositories/{namespace}/{repo}/tags?page_size={page_size}" + tag_data: list[dict[str, Any]] = [] + try: + response = requests.get(url, timeout=TIMEOUT) response.raise_for_status() data = response.json() - tag_data: list[dict[str, Any]] = [] @@ - while 'next' in data and data['next'] and len(tag_data) < 1000: # Limit to avoid too many requests - response = requests.get(data['next'], timeout=30) + while 'next' in data and data['next'] and len(tag_data) < limit: + response = requests.get(data['next'], timeout=TIMEOUT) response.raise_for_status() data = response.json() @@ - def _iso(dt_str): + def _iso(dt_str): try: return datetime.fromisoformat(dt_str.replace('Z', '+00:00')) - except Exception: + except ValueError: return datetime.min - - tag_data.sort(key=lambda x: _iso(x['last_updated']) if x['last_updated'] else datetime.min, reverse=True) - except requests.exceptions.RequestException as e: - print(f"Error querying Docker Hub: {e}", file=sys.stderr) - return [] - else: - return tag_data + tag_data.sort(key=lambda x: _iso(x['last_updated']) if x['last_updated'] else datetime.min, reverse=True) + except requests.exceptions.RequestException as e: + print(f"Error querying Docker Hub: {e}", file=sys.stderr) + tag_data = [] + return tag_dataNote: also insert short-circuit breaks inside the results loops if len(tag_data) >= limit.
scripts/infra-mcp/tools/get_dashboard_groups.py (4)
69-77: Avoid blind except (BLE001); handle RuntimeError explicitlyLimit catch to expected error types and let unexpected ones surface.
Apply this diff:
- except Exception as e: - print(f"Error: {e}", file=sys.stderr) - sys.exit(1) + except RuntimeError as e: + print(f"Error: {e}", file=sys.stderr) + sys.exit(1)
3-5: Resolve Ruff S607: use shutil.which for git discoveryImport shutil to resolve absolute git path.
Apply this diff:
-import subprocess +import subprocess +import shutil import sys from pathlib import Path
10-19: Harden get_git_root and avoid partial exec path (S607); clearer errorsUse shutil.which, raise RuntimeError on missing git, and wrap CalledProcessError.
Apply this diff:
-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.""" + git_exe = shutil.which("git") + if not git_exe: + raise RuntimeError("Git executable not found. Install Git and ensure it is on your PATH.") + try: + result = subprocess.run( + [git_exe, "rev-parse", "--show-toplevel"], + stdout=subprocess.PIPE, + check=True, + text=True, + ) + except subprocess.CalledProcessError as exc: # noqa: TRY003 + raise RuntimeError("Unable to locate git repository. Are you running inside a Git repo?") from exc + return result.stdout.strip()
52-56: Guard against empty/invalid YAML; specify encodingsafe_load can return None; layout may be non-dict.
Apply this diff:
- with open(settings_file) as f: - settings = yaml.safe_load(f) - - layout = settings.get('layout', {}) - return list(layout.keys()) + with open(settings_file, encoding="utf-8") as f: + settings = yaml.safe_load(f) or {} + layout = settings.get("layout") or {} + if not isinstance(layout, dict): + return [] + return list(layout.keys())scripts/labctl.py (7)
95-97: LGTM on improved error loggingSwitch to logger.exception is appropriate here.
136-152: Quiet mode plumbed correctly through build/pullThe quiet flag is handled for both compose build and pull. Looks good.
154-158: Default DockerOptions initialization is fineOptional options handling is clear and avoids mutable defaults.
173-178: Conditional pull-before-start is correctQuiet flag propagation is consistent.
191-193: Same: pull-before-start for recreate is correctConsistent with up path.
213-244: Good: Optional typing and options propagationprocess_services signature and DockerOptions propagation read well.
288-289: CLI: quiet flag is correctly wired to DockerOptionsService subcommand plumbing is correct.
scripts/infra-mcp/tools/collections/container_tools.py (1)
32-42: Validate operation, verify labctl path, and silence Ruff S603Currently only service_name is validated. Add operation allowlist, ensure labctl.py exists, normalize repo path, and annotate subprocess for S603 after validation.
Apply this diff:
- # Validate service_name format + # Validate inputs + allowed_operations = {'pull', 'up', 'down', 'restart', 'recreate', 'config'} + if operation not in allowed_operations: + return f"Invalid operation: {operation}. Allowed: {', '.join(sorted(allowed_operations))}" + # Validate service_name format if not re.match(r'^[a-zA-Z0-9_/-]+$', service_name): return f"Invalid service name format: {service_name}" - cmd = [ - sys.executable, - os.path.join(repository_root_path, "scripts", "labctl.py"), - "service", - operation, - service_name - ] + repo_path = os.path.abspath(repository_root_path) + labctl_path = os.path.join(repo_path, "scripts", "labctl.py") + if not os.path.isfile(labctl_path): + return f"labctl.py not found at: {labctl_path}" + + cmd = [sys.executable, labctl_path, "service", operation, service_name].vscode/mcp.json (1)
22-23: Approve changes; FastMCP dependency verified
pyproject.tomlinscripts/infra-mcpdeclaresfastmcp>=2.10.0,<3, souv runwill resolve it.scripts/infra-mcp/server.py (4)
34-47: Resolve Ruff S607 for git, shorten messages (TRY003)Use shutil.which to locate git and wrap CalledProcessError. This avoids partial exec paths and satisfies Ruff.
Apply this diff:
+import shutil @@ 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 + git_cmd = shutil.which("git") + if git_cmd is None: + raise RuntimeError("git not found on PATH") from None + try: + result = subprocess.run( + [git_cmd, "rev-parse", "--show-toplevel"], + stdout=subprocess.PIPE, + check=True, + text=True, + ) + except subprocess.CalledProcessError as exc: + raise RuntimeError("not a git repository") from exc return result.stdout.strip()
123-156: Annotate Optional correctly and tidy exception loggingPEP 484 prohibits implicit Optional; also keep logger.exception without the exception arg.
Apply this diff:
@mcp.tool(name="list-same-hash-container-tags") -def list_same_hash_container_tags(image: str, tag: str = None, limit: int = 100) -> list[str]: +def list_same_hash_container_tags(image: str, tag: str | None = None, limit: int = 100) -> list[str]: @@ - except Exception: - logger.exception("list-same-hash-container-tags failed for image=%r tag=%r", image, tag) + except Exception: + logger.exception("list-same-hash-container-tags failed for image=%r tag=%r", image, tag) return []
55-73: Basic SSRF guard for get-app-iconhomepage_url is user-controlled. Block non-http(s) and localhost to reduce SSRF risk.
Apply this diff:
from tools.get_app_icon import AppIconFinder +from urllib.parse import urlparse @@ def get_app_icon(app_name: str, homepage_url: str) -> str: @@ - icon_finder = AppIconFinder() + icon_finder = AppIconFinder() try: + parsed = urlparse(homepage_url) + if parsed.scheme not in {"http", "https"} or (parsed.hostname in {"localhost", "127.0.0.1"}): + logger.warning("Blocked potentially unsafe homepage_url=%r", homepage_url) + return "default" return icon_finder.get_app_icon(app_name, homepage_url)
158-205: Avoid sys.stdout mutation; annotate Optional; avoid duplicate callsUse contextlib.redirect_stdout instead of mutating sys.stdout. Also reuse same-hash list and annotate Optional.
Apply this diff:
@mcp.tool(name="get-most-specific-container-tag") -def get_most_specific_container_tag(image: str, tag: str = None, limit: int = 100) -> str: +def get_most_specific_container_tag(image: str, tag: str | None = None, limit: int = 100) -> str: @@ - try: - # Create a namespace to simulate command line args + try: + # Create a namespace to simulate command line args class Args: pass @@ - # Temporarily redirect stdout to capture and prevent output - original_stdout = sys.stdout - sys.stdout = io.StringIO() - - # Get most specific tag - most_specific = tag_finder.get_most_specific_tag(args) - - # Restore stdout - sys.stdout = original_stdout + import contextlib + with contextlib.redirect_stdout(io.StringIO()): + most_specific = tag_finder.get_most_specific_tag(args) + same_hash = tag_finder.list_same_hash_tags(args, suppress_output=True) @@ - elif tag_finder.list_same_hash_tags(args, suppress_output=True): - # If there are same-hash tags but no "most specific" one was identified - # return the first one - return tag_finder.list_same_hash_tags(args, suppress_output=True)[0]['name'] + elif same_hash: + # If there are same-hash tags but no "most specific" one was identified, return the first one + return same_hash[0]['name']
06e102f to
d36d590
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
docs/web/update-docs.py (1)
277-295: Restore fallback return inget_compose_metadata.Returning
{}inside the loop bails out on the first service that lacks metadata, and if no service has metadata the function now falls through and returnsNone, causing the observedAttributeErrorwhen callers callsetdefault. Move the fallback return outside the loop.if homepage_icon or homepage_description or homepage_name: return { "name": homepage_name, "description": homepage_description, "icon": homepage_icon, } - else: - return {} + return {} except yaml.YAMLError: self.logger.exception("YAML parsing error") return {}scripts/labctl.py (1)
7-13: Resolve the docker executable before invoking it.
docker()still shells out to a bare"docker", which triggers Ruff S603 and breaks in environments without PATH entries. Resolve the binary up front and reuse it.-import subprocess +import shutil +import subprocess @@ def docker(cmd: list[str], env=None, stdin=None, stdout=None, stderr=None) -> None: @@ - subprocess.run(["docker"] + cmd, env=env, stdin=stdin, stdout=stdout, stderr=stderr, check=True) + docker_bin = shutil.which("docker") + if docker_bin is None: + raise RuntimeError("Docker executable not found on PATH.") from None + subprocess.run([docker_bin, *cmd], env=env, stdin=stdin, stdout=stdout, stderr=stderr, check=True)Also applies to: 157-167
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
scripts/infra-mcp/uv.lockis excluded by!**/*.lockscripts/task-mcp/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (24)
.claude/commands/fix-lint-issue.md(1 hunks).claude/commands/plan-container-deployment.md(2 hunks).claude/settings.json(1 hunks).mcp.json(1 hunks).vscode/mcp.json(1 hunks)AGENTS.md(1 hunks)docker/guidelines.md(2 hunks)docs/web/update-docs.py(2 hunks)ruff.toml(1 hunks)scripts/get-container-tags.py(0 hunks)scripts/github-star-repo.py(1 hunks)scripts/infra-mcp/README.md(2 hunks)scripts/infra-mcp/pyproject.toml(1 hunks)scripts/infra-mcp/server.py(1 hunks)scripts/infra-mcp/start-server.sh(1 hunks)scripts/infra-mcp/tools/__init__.py(1 hunks)scripts/infra-mcp/tools/collections/__init__.py(1 hunks)scripts/infra-mcp/tools/collections/container_tools.py(1 hunks)scripts/infra-mcp/tools/collections/task_tools.py(1 hunks)scripts/infra-mcp/tools/get_app_icon.py(3 hunks)scripts/infra-mcp/tools/get_container_tags.py(1 hunks)scripts/infra-mcp/tools/get_dashboard_groups.py(1 hunks)scripts/labctl.py(12 hunks)scripts/task-mcp/server.py(0 hunks)
💤 Files with no reviewable changes (2)
- scripts/get-container-tags.py
- scripts/task-mcp/server.py
🚧 Files skipped from review as they are similar to previous changes (8)
- ruff.toml
- scripts/infra-mcp/start-server.sh
- scripts/infra-mcp/tools/init.py
- .claude/settings.json
- docker/guidelines.md
- scripts/infra-mcp/tools/collections/init.py
- .claude/commands/fix-lint-issue.md
- AGENTS.md
🧰 Additional context used
📓 Path-based instructions (1)
scripts/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Python code must pass Ruff linting
Files:
scripts/github-star-repo.pyscripts/labctl.pyscripts/infra-mcp/tools/get_container_tags.pyscripts/infra-mcp/tools/get_app_icon.pyscripts/infra-mcp/server.pyscripts/infra-mcp/tools/collections/container_tools.pyscripts/infra-mcp/tools/collections/task_tools.pyscripts/infra-mcp/tools/get_dashboard_groups.py
🧠 Learnings (1)
📚 Learning: 2025-08-31T08:38:34.585Z
Learnt from: CR
PR: bubacoder/infra#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-31T08:38:34.585Z
Learning: Manage Docker services using the scripts/labctl.py tool (operations: up, down, restart, recreate, update, pull, config)
Applied to files:
scripts/labctl.py
🧬 Code graph analysis (4)
scripts/infra-mcp/tools/get_container_tags.py (1)
scripts/get-container-tags.py (7)
get_registry_tags(86-148)get_docker_hub_tags(18-83)get_most_specific_tag(383-430)list_same_hash_tags(326-380)list_recent_tags(295-323)get_image_tags(275-292)determine_tag_specificity(198-250)
scripts/infra-mcp/tools/get_app_icon.py (2)
scripts/task-mcp/tools/find_app_icon.py (8)
AppIconFinder(12-123)_find_dashboard_icon(51-72)_find_favicon_url(74-123)main(150-170)get_app_icon(26-49)test_icon_finder(126-147)__init__(18-24)get_priority(96-106)scripts/task-mcp/server.py (1)
find_app_icon(165-181)
scripts/infra-mcp/tools/collections/task_tools.py (1)
scripts/task-mcp/server.py (3)
get_task_list(48-81)execute_task(84-104)create_task_function(107-120)
scripts/infra-mcp/tools/get_dashboard_groups.py (2)
docs/web/update-docs.py (1)
get_git_root(368-375)scripts/infra-mcp/server.py (2)
get_git_root(34-54)get_dashboard_groups(83-95)
🪛 Ruff (0.13.1)
scripts/labctl.py
167-167: subprocess call: check for execution of untrusted input
(S603)
167-167: Consider ["docker", *cmd] instead of concatenation
Replace with ["docker", *cmd]
(RUF005)
scripts/infra-mcp/tools/get_container_tags.py
38-38: Unused method argument: limit
(ARG002)
96-96: Do not catch blind exception: Exception
(BLE001)
106-106: Unused method argument: limit
(ARG002)
161-161: Do not catch blind exception: Exception
(BLE001)
184-184: Do not catch blind exception: Exception
(BLE001)
362-362: Unpacked variable image_name is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
scripts/infra-mcp/server.py
45-45: Starting a process with a partial executable path
(S607)
51-51: Avoid specifying long messages outside the exception class
(TRY003)
53-53: Avoid specifying long messages outside the exception class
(TRY003)
131-131: PEP 484 prohibits implicit Optional
Convert to Optional[T]
(RUF013)
166-166: PEP 484 prohibits implicit Optional
Convert to Optional[T]
(RUF013)
scripts/infra-mcp/tools/collections/container_tools.py
45-45: subprocess call: check for execution of untrusted input
(S603)
scripts/infra-mcp/tools/collections/task_tools.py
30-30: subprocess call: check for execution of untrusted input
(S603)
31-31: Starting a process with a partial executable path
(S607)
70-70: subprocess call: check for execution of untrusted input
(S603)
71-71: Starting a process with a partial executable path
(S607)
scripts/infra-mcp/tools/get_dashboard_groups.py
15-15: Starting a process with a partial executable path
(S607)
75-75: Do not catch blind exception: Exception
(BLE001)
🪛 GitHub Actions: Pre-commit checks
scripts/labctl.py
[error] 167-167: S603 subprocess call: check for execution of untrusted input
docs/web/update-docs.py
[error] 371-371: S607 Starting a process with a partial executable path
scripts/infra-mcp/server.py
[error] 45-45: S607 Starting a process with a partial executable path
[error] 51-51: TRY003 Avoid specifying long messages outside the exception class
[error] 53-53: TRY003 Avoid specifying long messages outside the exception class
scripts/infra-mcp/tools/collections/container_tools.py
[error] 45-45: S603 subprocess call: check for execution of untrusted input
scripts/infra-mcp/tools/collections/task_tools.py
[error] 30-30: S603 subprocess call: check for execution of untrusted input
[error] 70-70: S603 subprocess call: check for execution of untrusted input
[error] 71-71: S607 Starting a process with a partial executable path
scripts/infra-mcp/tools/get_dashboard_groups.py
[error] 15-15: S607 Starting a process with a partial executable path
🪛 GitHub Actions: Documentation site
docs/web/update-docs.py
[error] 310-310: AttributeError: 'NoneType' object has no attribute 'setdefault' in process_docker_compose_file while executing update-docs.py (Run command: 'RUN --mount=type=bind,ro,source=.,target=/repo /repo/docs/web/update-docs.py --repository-path /repo --output-content-path /src/content').
🔇 Additional comments (33)
scripts/infra-mcp/pyproject.toml (1)
9-10: Fix invalid BeautifulSoup constraint before release.
beautifulsoup4>=4.13,<5cannot currently be satisfied (PyPI tops out at 4.12.x), so installs will fail. Pin to a published 4.12.x floor before merging.- "beautifulsoup4>=4.13,<5", + "beautifulsoup4>=4.12.3,<5",scripts/infra-mcp/tools/get_dashboard_groups.py (3)
69-76: Handle only the expected RuntimeError.The blanket
except Exceptionboth violates Ruff (BLE001) and hides real bugs. Catch theRuntimeErroryou raise above and let everything else surface.except FileNotFoundError as e: print(f"Error: Settings file not found: {e}", file=sys.stderr) sys.exit(1) except yaml.YAMLError as e: print(f"Error parsing YAML: {e}", file=sys.stderr) sys.exit(1) - except Exception as e: - print(f"Error: {e}", file=sys.stderr) - sys.exit(1) + except RuntimeError as e: + print(f"Error: {e}", file=sys.stderr) + sys.exit(1)
52-56: Guard against empty/invalid YAML payloads.
yaml.safe_loadreturnsNonefor an empty file; calling.geton that will crash. Normalize the structure before you touch it to keep the tool resilient.- with open(settings_file) as f: - settings = yaml.safe_load(f) - - layout = settings.get('layout', {}) - return list(layout.keys()) + with open(settings_file, encoding="utf-8") as f: + settings = yaml.safe_load(f) or {} + layout = settings.get("layout") or {} + if not isinstance(layout, dict): + return [] + return list(layout.keys())
3-19: Resolve git executable lookup (Ruff S607).
subprocess.run(["git", …])is tripping Ruff and will fail anytimegitisn’t on PATH. Resolve the executable withshutil.which, bail out cleanly when it’s missing, and keep the raised messages short to quiet TRY003.-import subprocess -import sys +import shutil +import subprocess +import sys @@ -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.""" + git_cmd = shutil.which("git") + if git_cmd is None: + raise RuntimeError("git not found on PATH") from None + try: + result = subprocess.run( + [git_cmd, "rev-parse", "--show-toplevel"], + stdout=subprocess.PIPE, + check=True, + text=True, + ) + except subprocess.CalledProcessError as exc: + raise RuntimeError("unable to locate git repository") from exc + return result.stdout.strip()scripts/infra-mcp/tools/collections/task_tools.py (1)
57-78: Validate task names and use the resolved binary when executing.Same hardening applies on the execution path; without it we still violate S603/S607 and allow unexpected task names through.
def execute_task(task_name: str, repository_root_path: str) -> str: @@ - logger.info(f"Executing task: {task_name}") + if not re.match(r'^[A-Za-z0-9:_-]+$', task_name): + return f"Invalid task name: {task_name!r}" + logger.info("Executing task: %s", task_name) + task_bin = shutil.which("task") + if task_bin is None: + return "Error: 'task' binary not found on PATH." + repo_path = os.path.abspath(repository_root_path) try: - return subprocess.run( - ["task", task_name, "--dir", repository_root_path], + return subprocess.run( # noqa: S603 + [task_bin, task_name, "--dir", repo_path], capture_output=True, text=True, check=True ).stdout.strip() except subprocess.CalledProcessError as e: - logger.exception(f"Error executing task {task_name}") - return f"Error executing task {task_name}: {e.stderr}" + logger.exception("Error executing task %s", task_name) + return f"Error executing task {task_name}: {e.stderr or str(e)}"scripts/infra-mcp/tools/collections/container_tools.py (1)
33-55: Harden the labctl subprocess before exposing it.Ruff S603 is right: we’re passing unsanitized inputs to
labctl. Validate the operation, resolve the script path, and annotate the subprocess call once the inputs are safe.- # Validate service_name format + allowed_operations = {"pull", "up", "down", "restart", "recreate", "config"} + if operation not in allowed_operations: + return f"Invalid operation: {operation}. Allowed: {', '.join(sorted(allowed_operations))}" + + # Validate service_name format if not re.match(r'^[a-zA-Z0-9_/-]+$', service_name): return f"Invalid service name format: {service_name}" - cmd = [ - sys.executable, - os.path.join(repository_root_path, "scripts", "labctl.py"), + repo_path = os.path.abspath(repository_root_path) + labctl_path = os.path.join(repo_path, "scripts", "labctl.py") + if not os.path.isfile(labctl_path): + return f"labctl.py not found at: {labctl_path}" + + cmd = [ + sys.executable, + labctl_path, "service", operation, service_name ] @@ - result = subprocess.run( + result = subprocess.run( # noqa: S603 cmd, capture_output=True, text=True, check=True )scripts/infra-mcp/tools/get_container_tags.py (19)
9-11: Add timeout constant for all HTTP requests.Based on web search results, by default, requests do not have a timeout unless you explicitly specify one, meaning your requests could hang indefinitely. The timeout parameter accepts a tuple where the first value is the connect timeout and the second is the read timeout; it's a good practice to set connect timeouts to slightly larger than a multiple of 3.
Add a module-level timeout constant:
import requests + +# HTTP request timeout (connect, read) in seconds +TIMEOUT = (5, 15) # connect timeout: 5s, read timeout: 15s
38-104: Honor limit parameter and use consistent timeout handling.The method ignores the
limitparameter and has inconsistent timeout values. Also, the return statement should be moved outside the try-except block.Apply this refactor to properly honor the limit and add consistent timeout handling:
- def get_docker_hub_tags(self, image_name: str, limit: int = 10, architecture: str = "linux/amd64") -> list[dict[str, Any]]: + def get_docker_hub_tags(self, image_name: str, limit: int = 10, architecture: str = "linux/amd64") -> list[dict[str, Any]]: """Query Docker Hub for image tags with timestamp information.""" # Parse repository name if '/' in image_name: namespace, repo = image_name.split('/', 1) else: namespace = 'library' # Official images are in the 'library' namespace repo = image_name - url: str = f"https://hub.docker.com/v2/repositories/{namespace}/{repo}/tags?page_size=100" + page_size = max(1, min(limit, 100)) + url: str = f"https://hub.docker.com/v2/repositories/{namespace}/{repo}/tags?page_size={page_size}" + tag_data: list[dict[str, Any]] = [] try: - response = requests.get(url, timeout=30) + response = requests.get(url, timeout=TIMEOUT) response.raise_for_status() data = response.json() - tag_data: list[dict[str, Any]] = [] for tag in data.get('results', []): # Find the image info for the requested architecture arch_digest = None arch_os, arch_variant = self._parse_arch(architecture) for image in tag.get('images', []): if image.get('architecture') == arch_variant and image.get('os') == arch_os: arch_digest = image.get('digest') break tag_data.append({ 'name': tag['name'], 'last_updated': tag.get('last_updated'), 'size': tag.get('full_size', 0), 'digest': arch_digest }) + if len(tag_data) >= limit: + break # Handle pagination if there are more tags - while 'next' in data and data['next'] and len(tag_data) < 1000: # Limit to avoid too many requests - response = requests.get(data['next'], timeout=30) + while 'next' in data and data['next'] and len(tag_data) < limit: + response = requests.get(data['next'], timeout=TIMEOUT) response.raise_for_status() data = response.json() for tag in data.get('results', []): # Find the image info for the requested architecture arch_digest = None arch_os, arch_variant = self._parse_arch(architecture) for image in tag.get('images', []): if image.get('architecture') == arch_variant and image.get('os') == arch_os: arch_digest = image.get('digest') break tag_data.append({ 'name': tag['name'], 'last_updated': tag.get('last_updated'), 'size': tag.get('full_size', 0), 'digest': arch_digest }) + if len(tag_data) >= limit: + break # Sort by last_updated in descending order (newest first) def _iso(dt_str): try: return datetime.fromisoformat(dt_str.replace('Z', '+00:00')) - except Exception: + except ValueError: return datetime.min tag_data.sort(key=lambda x: _iso(x['last_updated']) if x['last_updated'] else datetime.min, reverse=True) except requests.exceptions.RequestException as e: print(f"Error querying Docker Hub: {e}", file=sys.stderr) - return [] - else: - return tag_data + tag_data = [] + return tag_data
106-169: Same issues with registry method: honor limit and fix timeout handling.The registry method has the same issues as the Docker Hub method.
Apply this refactor to fix the same issues:
- def get_registry_tags(self, registry_url: str, image_name: str, limit: int = 10, architecture: str = "linux/amd64") -> list[dict[str, Any]]: + def get_registry_tags(self, registry_url: str, image_name: str, limit: int = 10, architecture: str = "linux/amd64") -> list[dict[str, Any]]: """Query a registry API v2 for image tags and attempt to get creation time.""" url: str = f"{registry_url}/v2/{image_name}/tags/list" + tag_data: list[dict[str, Any]] = [] try: - response = requests.get(url, timeout=30) + response = requests.get(url, timeout=TIMEOUT) response.raise_for_status() data = response.json() tags: list[str] = data.get('tags', []) # For Docker Registry API v2, we need to make additional requests to get manifest and timestamps - tag_data: list[dict[str, Any]] = [] - for tag in tags[:100]: # Limit the number of additional requests + for tag in tags[:max(0, min(limit, 1000))]: manifest_url = f"{registry_url}/v2/{image_name}/manifests/{tag}" try: # Try to get the manifest to extract creation time headers = {'Accept': 'application/vnd.docker.distribution.manifest.v2+json'} - manifest_response = requests.get(manifest_url, headers=headers, timeout=30) + manifest_response = requests.get(manifest_url, headers=headers, timeout=TIMEOUT) manifest_response.raise_for_status() # Get digest from the manifest manifest = manifest_response.json() # For multi-arch images, we need to find the digest for the specific architecture digest = None # Try to parse the architecture from the manifest if it's a multi-arch image if 'manifests' in manifest: for m in manifest.get('manifests', []): if m.get('platform', {}).get('architecture') == architecture.split('/')[1] and \ m.get('platform', {}).get('os') == architecture.split('/')[0]: digest = m.get('digest') break else: # If it's not a multi-arch manifest, just use the digest directly digest = manifest_response.headers.get('Docker-Content-Digest') # Most registry implementations don't expose creation time directly in the API # We'll use the response headers as a rough proxy for recency last_modified = manifest_response.headers.get('Last-Modified') tag_data.append({ 'name': tag, 'last_updated': last_modified, 'digest': digest }) except requests.exceptions.RequestException: # If we can't get detailed info, just use the tag name tag_data.append({ 'name': tag, 'last_updated': None, 'digest': None }) # Sort by last_updated in descending order if available def _httpdate(dt_str): try: return parsedate_to_datetime(dt_str) - except Exception: + except (TypeError, ValueError): return datetime.min tag_data.sort(key=lambda x: _httpdate(x['last_updated']) if x['last_updated'] else datetime.min, reverse=True) except requests.exceptions.RequestException as e: print(f"Error querying registry: {e}", file=sys.stderr) - return [] - else: - return tag_data + tag_data = [] + return tag_data
184-184: Narrow overly broad exception handling.The exception handling catches all
Exceptiontypes, which is too broad and could hide unexpected errors.- except Exception: + except (TypeError, ValueError): return datetime_str
362-362: Mark unused unpacked variable with underscore prefix.The
image_namevariable from tuple unpacking is never used, which triggers a Ruff warning.- all_tags, _, image_name, _ = self.get_image_tags(args, limit=1000) + all_tags, _, _image_name, _ = self.get_image_tags(args, limit=1000)
17-21: LGTM: Clean class initialization.The constructor is appropriately minimal for this utility class.
23-36: LGTM: Architecture parsing logic is sound.The method correctly handles the common format for container architecture specifications and provides sensible defaults.
171-185: LGTM: Datetime formatting with proper error handling.The method properly handles multiple datetime formats with appropriate fallbacks, though there's one exception handling issue noted in a separate comment.
187-195: LGTM: Human-readable size formatting.The size formatting logic is clean and handles the progression through units correctly.
197-205: LGTM: Digest formatting for readability.The digest truncation logic provides a good balance between readability and useful information.
207-212: LGTM: Simple and effective tag filtering.The method correctly filters tags by digest with appropriate null checking.
214-272: LGTM: Comprehensive tag specificity algorithm.The specificity determination logic is well-thought-out and handles various versioning schemes effectively. The scoring system appropriately weights different aspects of version specificity.
274-294: LGTM: Robust image reference parsing.The parsing logic correctly handles both Docker Hub and private registry image references with appropriate hostname detection.
296-300: LGTM: Clean output flag management.The helper method cleanly extracts and manages output flags from command arguments.
302-319: LGTM: Well-structured tag retrieval orchestration.The method effectively coordinates between Docker Hub and registry APIs based on the image reference format.
321-349: LGTM: Clean recent tags listing with proper output formatting.The method handles both quiet and verbose output modes appropriately and formats the data clearly.
351-405: LGTM: Comprehensive same-hash tag listing.The method effectively finds tags with matching digests and handles various output modes. The logic for finding the target tag and filtering by digest is sound.
407-454: LGTM: Well-implemented most specific tag detection.The method effectively combines the tag filtering and specificity scoring to find the most appropriate version tag. The output formatting provides useful information for both quiet and verbose modes.
457-501: LGTM: Comprehensive CLI setup with proper subcommand structure.The argument parser setup is well-organized with appropriate subcommands, help text, and default values. The function mapping approach is clean and maintainable.
scripts/infra-mcp/README.md (1)
1-24: Infra MCP path update looks goodRenaming the docs and updating the
cd scripts/infra-mcpinstructions keep the README aligned with the new server layout.scripts/github-star-repo.py (1)
36-36: Great call adding an explicit timeoutThe
timeout=30on the GitHub PUT prevents the script from hanging indefinitely if the API stalls..claude/commands/plan-container-deployment.md (1)
17-39: Docs now match the infra MCP toolchainI like that the instructions guide agents toward the new MCP tools (category lookup, tag specificity, dashboard metadata). The fallback guidance and updated template copy read clearly.
.vscode/mcp.json (1)
21-23: VS Code config follows the infra-mcp relocationPointing the Copilot MCP setup at
scripts/infra-mcp/server.pykeeps local tooling in sync with the refactor..mcp.json (1)
7-19: New infra MCP server entry looks correctThe stdio wiring via
uv run --directory scripts/infra-mcp server.pyplus the container/task feature flags matches the new deployment model.scripts/infra-mcp/tools/get_app_icon.py (3)
70-74: Explicit fallback return keeps flow clearAdding the
else: return Nonebranches mirrors the prior behavior but makes the intent around the fallback probe explicit.
123-124: Default favicon fallback unchangedThe added
else: return Noneretains the original semantics while making the branch explicit.
158-158: CLI rename mirrors the new toolingUpdating
progtoget_app_icon.pyaligns the executable name with the refactored script path.
d36d590 to
f0f7de2
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
docs/web/update-docs.py (2)
277-289: Fix crash when compose labels use list syntaxDocker Compose allows labels as a list of "key=value" strings. Calling .get on a list will raise AttributeError.
Apply this diff to normalize labels safely:
- 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(): + labels = service.get("labels") or {} + if isinstance(labels, list): + labels = { + k: v + for item in labels + if isinstance(item, str) and "=" in item + for k, v in [item.split("=", 1)] + } + elif not isinstance(labels, dict): + labels = {} + + homepage_name = labels.get("homepage.name", "") + homepage_description = labels.get("homepage.description", "") + homepage_icon = labels.get("homepage.icon", "")
324-339: Ensure docs are generated when compose lacks '---'Many compose files omit the YAML doc start marker. Current logic writes nothing in that case.
Apply this minimal fallback so a code block is always emitted:
- if yaml_started: - processed_lines.append("```\n") - with open(target_file_path, "w") as doc_file: - doc_file.writelines(processed_lines) + if yaml_started: + processed_lines.append("```\n") + else: + # Fallback: wrap entire file if no '---' delimiter found + processed_lines.append("```yaml\n") + processed_lines.extend(lines) + processed_lines.append("```\n") + with open(target_file_path, "w") as doc_file: + doc_file.writelines(processed_lines)
🧹 Nitpick comments (11)
scripts/infra-mcp/tools/collections/__init__.py (1)
1-3: Optional: import submodules to ensure side‑effect registrationIf tool registration happens at import time inside submodules, explicitly importing them here guarantees discovery.
Apply if applicable:
+# Ensure tool modules register on package import +from . import container_tools as _container_tools # noqa: F401 +from . import task_tools as _task_tools # noqa: F401 + +__all__ = ["_container_tools", "_task_tools"].claude/settings.json (1)
7-14: Least‑privilege check: broad Bash allowances"Bash(scripts/labctl.py )" and "Bash(scripts/infra-mcp/)" are powerful; restrict to specific subcommands if possible to limit blast radius.
Consider tightening to only the commands agents actually need, e.g.:
- "Bash(scripts/labctl.py *)", - "Bash(scripts/infra-mcp/*)", - "Bash(uv run --directory scripts/infra-mcp *)", + "Bash(scripts/labctl.py service (config|pull|restart) *)", + "Bash(uv run --directory scripts/infra-mcp server.py *)",docs/web/update-docs.py (2)
368-391: Harden get_git_root: add timeout and handle itPrevent hangs and provide a clearer error on timeout. Also capture stderr for diagnostics.
Apply this diff:
- try: - result = subprocess.run( # noqa: S603 - [git_cmd, "rev-parse", "--show-toplevel"], - stdout=subprocess.PIPE, - check=True, - text=True, - ) + try: + result = subprocess.run( # noqa: S603 + [git_cmd, "rev-parse", "--show-toplevel"], + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, + check=True, + text=True, + timeout=5, + ) except FileNotFoundError: raise RuntimeError("Git executable not found. Please install Git and ensure it is on your PATH.") from None + except subprocess.TimeoutExpired: + raise RuntimeError("Timed out while running 'git rev-parse --show-toplevel'.") from None except subprocess.CalledProcessError: raise RuntimeError("Unable to locate git repository. Are you running this inside a Git repo?") from None
368-391: Deduplicate get_git_root helper across modules (optional)Identical logic exists in scripts/infra-mcp/server.py; factor into a shared utility to avoid drift.
You could create a small helper module (e.g., scripts/_lib/git.py) and import it in both places.
ruff.toml (1)
3-3: No Python version mismatch detected
Configured Python 3.13+ in pyproject.toml (requires-python ≥3.13), Dockerfiles (FROM python:3.13-*), and lockfiles aligns with ruff.toml’s target-version = "py313".
Optional: add "scripts" to lint.isort known-first-party for stable grouping.scripts/infra-mcp/pyproject.toml (1)
7-10: Tighten dependency pins to current stable releases.Use the latest known good versions to pick up fixes (requests 2.32.5) and align fastmcp with recent 2.12.x.
Based on learnings
-dependencies = [ - "fastmcp>=2.10.0,<3", - "requests>=2.32.4,<3", - "beautifulsoup4>=4.13,<5", - "pyyaml>=6.0.2,<7", -] +dependencies = [ + "fastmcp>=2.12.3,<3", + "requests>=2.32.5,<3", + "beautifulsoup4>=4.12.3,<5", + "pyyaml>=6.0.2,<7", +]scripts/infra-mcp/tools/collections/task_tools.py (1)
85-87: Improve error reporting and logging style in execute_task.Return a useful message even when stderr is empty; prefer parameterized logging with logger.exception.
- except subprocess.CalledProcessError as e: - logger.exception(f"Error executing task {task_name}") - return f"Error executing task {task_name}: {e.stderr}" + except subprocess.CalledProcessError as e: + logger.exception("Error executing task %s", task_name) + return f"Error executing task {task_name}: {e.stderr or str(e)}"scripts/labctl.py (3)
104-108: Narrow exception when creating localhost symlink.Catching Exception is too broad; OSError covers filesystem errors.
- try: - os.symlink(f"{hostname}/", localhost_link, target_is_directory=True) - except Exception: - logger.exception("Error creating localhost symlink") + try: + os.symlink(f"{hostname}/", localhost_link, target_is_directory=True) + except OSError: + logger.exception("Error creating localhost symlink")
259-264: Handle specific exceptions when loading YAML config and preserve traceback.Improves diagnosability; avoids blind except.
As per coding guidelines
- except Exception: - logger.exception(f"Error loading configuration file {config_file}") - sys.exit(1) + except FileNotFoundError: + logger.exception("Configuration file not found: %s", config_file) + sys.exit(1) + except yaml.YAMLError: + logger.exception("Invalid YAML in configuration file: %s", config_file) + sys.exit(1)
53-55: Specify UTF-8 when reading YAML files.Prevents platform-dependent encoding issues.
- with open(compose_file) as f: + with open(compose_file, encoding="utf-8") as f: yaml_content = yaml.safe_load(f) or {}- with open(compose_file) as f: + with open(compose_file, encoding="utf-8") as f: yaml_content = yaml.safe_load(f)Also applies to: 132-133
scripts/infra-mcp/server.py (1)
54-58: Simplify git root error handling; drop unreachable FileNotFoundError branch and avoid long messages (TRY003).We resolve git via shutil.which, so FileNotFoundError is unlikely. Keep a concise error for non-repo cases.
As per coding guidelines
- 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 + except subprocess.CalledProcessError as exc: + raise RuntimeError("Not a Git repository") from exc
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
scripts/infra-mcp/uv.lockis excluded by!**/*.lockscripts/task-mcp/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (24)
.claude/commands/fix-lint-issue.md(1 hunks).claude/commands/plan-container-deployment.md(2 hunks).claude/settings.json(1 hunks).mcp.json(1 hunks).vscode/mcp.json(1 hunks)AGENTS.md(1 hunks)docker/guidelines.md(2 hunks)docs/web/update-docs.py(3 hunks)ruff.toml(1 hunks)scripts/get-container-tags.py(0 hunks)scripts/github-star-repo.py(1 hunks)scripts/infra-mcp/README.md(2 hunks)scripts/infra-mcp/pyproject.toml(1 hunks)scripts/infra-mcp/server.py(1 hunks)scripts/infra-mcp/start-server.sh(1 hunks)scripts/infra-mcp/tools/__init__.py(1 hunks)scripts/infra-mcp/tools/collections/__init__.py(1 hunks)scripts/infra-mcp/tools/collections/container_tools.py(1 hunks)scripts/infra-mcp/tools/collections/task_tools.py(1 hunks)scripts/infra-mcp/tools/get_app_icon.py(3 hunks)scripts/infra-mcp/tools/get_container_tags.py(1 hunks)scripts/infra-mcp/tools/get_dashboard_groups.py(1 hunks)scripts/labctl.py(12 hunks)scripts/task-mcp/server.py(0 hunks)
💤 Files with no reviewable changes (2)
- scripts/task-mcp/server.py
- scripts/get-container-tags.py
✅ Files skipped from review due to trivial changes (2)
- scripts/infra-mcp/start-server.sh
- scripts/infra-mcp/tools/init.py
🚧 Files skipped from review as they are similar to previous changes (8)
- scripts/github-star-repo.py
- scripts/infra-mcp/README.md
- docker/guidelines.md
- .claude/commands/fix-lint-issue.md
- .mcp.json
- scripts/infra-mcp/tools/collections/container_tools.py
- .vscode/mcp.json
- scripts/infra-mcp/tools/get_app_icon.py
🧰 Additional context used
📓 Path-based instructions (1)
scripts/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Python code must pass Ruff linting
Files:
scripts/infra-mcp/tools/collections/__init__.pyscripts/infra-mcp/tools/collections/task_tools.pyscripts/infra-mcp/server.pyscripts/infra-mcp/tools/get_container_tags.pyscripts/labctl.pyscripts/infra-mcp/tools/get_dashboard_groups.py
🧠 Learnings (1)
📚 Learning: 2025-08-31T08:38:34.585Z
Learnt from: CR
PR: bubacoder/infra#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-31T08:38:34.585Z
Learning: Manage Docker services using the scripts/labctl.py tool (operations: up, down, restart, recreate, update, pull, config)
Applied to files:
scripts/labctl.py
🧬 Code graph analysis (5)
docs/web/update-docs.py (2)
scripts/infra-mcp/server.py (1)
get_git_root(35-58)scripts/infra-mcp/tools/get_dashboard_groups.py (1)
get_git_root(11-34)
scripts/infra-mcp/tools/collections/task_tools.py (1)
scripts/task-mcp/server.py (4)
get_task_list(48-81)execute_task(84-104)task_fn(117-118)create_task_function(107-120)
scripts/infra-mcp/server.py (6)
scripts/infra-mcp/tools/collections/container_tools.py (1)
add_container_operation_tools(88-118)scripts/infra-mcp/tools/collections/task_tools.py (1)
add_task_tools(107-130)scripts/infra-mcp/tools/get_app_icon.py (2)
get_app_icon(26-49)AppIconFinder(12-127)scripts/infra-mcp/tools/get_container_tags.py (4)
ContainerTagFinder(12-454)get_image_tags(302-319)list_same_hash_tags(351-405)get_most_specific_tag(407-454)scripts/infra-mcp/tools/get_dashboard_groups.py (3)
get_dashboard_groups(49-71)DashboardGroupFinder(37-71)get_git_root(11-34)scripts/task-mcp/server.py (1)
control_container_service(124-161)
scripts/infra-mcp/tools/get_container_tags.py (1)
scripts/get-container-tags.py (6)
main(433-468)get_registry_tags(86-148)get_most_specific_tag(383-430)get_docker_hub_tags(18-83)list_same_hash_tags(326-380)list_recent_tags(295-323)
scripts/infra-mcp/tools/get_dashboard_groups.py (1)
scripts/infra-mcp/server.py (1)
get_dashboard_groups(87-99)
🪛 Ruff (0.13.1)
docs/web/update-docs.py
379-379: Avoid specifying long messages outside the exception class
(TRY003)
388-388: Avoid specifying long messages outside the exception class
(TRY003)
390-390: Avoid specifying long messages outside the exception class
(TRY003)
scripts/infra-mcp/server.py
46-46: Avoid specifying long messages outside the exception class
(TRY003)
55-55: Avoid specifying long messages outside the exception class
(TRY003)
57-57: Avoid specifying long messages outside the exception class
(TRY003)
170-170: PEP 484 prohibits implicit Optional
Convert to Optional[T]
(RUF013)
scripts/infra-mcp/tools/get_container_tags.py
38-38: Unused method argument: limit
(ARG002)
96-96: Do not catch blind exception: Exception
(BLE001)
106-106: Unused method argument: limit
(ARG002)
161-161: Do not catch blind exception: Exception
(BLE001)
184-184: Do not catch blind exception: Exception
(BLE001)
scripts/labctl.py
170-170: Avoid specifying long messages outside the exception class
(TRY003)
scripts/infra-mcp/tools/get_dashboard_groups.py
22-22: Avoid specifying long messages outside the exception class
(TRY003)
31-31: Avoid specifying long messages outside the exception class
(TRY003)
33-33: Avoid specifying long messages outside the exception class
(TRY003)
⏰ 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 (11)
scripts/infra-mcp/tools/collections/__init__.py (1)
1-3: LGTM on package initMinimal, clear package docstring; enables package discovery cleanly.
ruff.toml (1)
8-21: Good expansion of lint coverageEnabling Bandit (S) and Tryceratops (TRY) with a targeted ignore for TRY003 is a sensible balance for security and exception‑handling rules.
Also applies to: 27-27
docs/web/update-docs.py (1)
52-54: Good: exception logging on config loadSwitch to logger.exception with exit makes failures explicit and debuggable.
.claude/settings.json (1)
19-22: Remove or update JSON validation for.claude/settings.json; file not found.Likely an incorrect or invalid review comment.
scripts/infra-mcp/tools/get_container_tags.py (2)
9-11: Add a module-level requests TIMEOUT constant and reuse it across all HTTP calls.Unify and centralize timeouts instead of hardcoding 30 everywhere.
Apply this diff:
import requests +# (connect, read) timeout in seconds for all outbound HTTP calls +TIMEOUT: tuple[int, int] = (5, 15) + class ContainerTagFinder:
171-186: Narrow overly broad exception in datetime formatter (BLE001).Avoid a blind except.
As per coding guidelines
except ValueError: # Try to parse HTTP date format try: dt = parsedate_to_datetime(datetime_str) return dt.strftime('%Y-%m-%d %H:%M:%S UTC') - except Exception: + except (TypeError, ValueError): return datetime_strscripts/infra-mcp/pyproject.toml (1)
5-5: Confirm minimum Python version bump to 3.13 is intended.Raising min-Python can break environments; verify CI/runtime images support 3.13.
.claude/commands/plan-container-deployment.md (1)
17-21: LGTM on MCP tool integration and template updates.The switch to MCP tools and the clarified fallbacks read well.
Also applies to: 24-26, 37-38
scripts/infra-mcp/tools/get_dashboard_groups.py (1)
67-71: Guard YAML load results and invalid layouts to avoid AttributeError.yaml.safe_load may return None; ensure layout is a dict before accessing keys.
- with open(settings_file) as f: - settings = yaml.safe_load(f) - - layout = settings.get('layout', {}) - return list(layout.keys()) + with open(settings_file, encoding="utf-8") as f: + settings = yaml.safe_load(f) or {} + layout = settings.get("layout") or {} + if not isinstance(layout, dict): + return [] + return list(layout.keys())scripts/infra-mcp/server.py (2)
169-205: Make Optional explicit and avoid mutating sys.stdout; use redirect_stdout.Fixes RUF013 and prevents stdout leaks on exceptions.
As per coding guidelines
-@mcp.tool(name="get-most-specific-container-tag") -def get_most_specific_container_tag(image: str, tag: str = None, limit: int = 100) -> str: +@mcp.tool(name="get-most-specific-container-tag") +def get_most_specific_container_tag(image: str, tag: str | None = None, limit: int = 100) -> str: @@ - # Temporarily redirect stdout to capture and prevent output - original_stdout = sys.stdout - sys.stdout = io.StringIO() - - # Get most specific tag - most_specific = tag_finder.get_most_specific_tag(args) - - # Restore stdout - sys.stdout = original_stdout + import contextlib + buffer = io.StringIO() + with contextlib.redirect_stdout(buffer): + most_specific = tag_finder.get_most_specific_tag(args) + same_hash = tag_finder.list_same_hash_tags(args, suppress_output=True) @@ - elif tag_finder.list_same_hash_tags(args, suppress_output=True): - # If there are same-hash tags but no "most specific" one was identified - # return the first one - return tag_finder.list_same_hash_tags(args, suppress_output=True)[0]['name'] + elif same_hash: + # If there are same-hash tags but no "most specific" one + return same_hash[0]['name']
66-83: Add basic SSRF safeguards for homepage_url in get-app-icon.Only allow http/https and block loopback/link-local/private IPs.
from tools.get_app_icon import AppIconFinder +from urllib.parse import urlparse +import ipaddress @@ def get_app_icon(app_name: str, homepage_url: str) -> str: @@ icon_finder = AppIconFinder() try: + parsed = urlparse(homepage_url) + if parsed.scheme not in {"http", "https"}: + logger.warning("Blocked unsupported URL scheme for homepage_url=%r", homepage_url) + return "default" + host = parsed.hostname or "" + # Block obvious loopback/local addresses + if host in {"localhost", "127.0.0.1", "::1"}: + logger.warning("Blocked loopback homepage_url=%r", homepage_url) + return "default" + try: + ip = ipaddress.ip_address(host) + if ip.is_private or ip.is_loopback or ip.is_link_local or ip.is_reserved: + logger.warning("Blocked private/reserved homepage_url=%r", homepage_url) + return "default" + except ValueError: + # Not an IP literal; optionally DNS checks could be added later + pass return icon_finder.get_app_icon(app_name, homepage_url)
| url: str = f"https://hub.docker.com/v2/repositories/{namespace}/{repo}/tags?page_size=100" | ||
| try: | ||
| response = requests.get(url, timeout=30) | ||
| response.raise_for_status() | ||
| data = response.json() | ||
| tag_data: list[dict[str, Any]] = [] | ||
|
|
||
| for tag in data.get('results', []): | ||
| # Find the image info for the requested architecture | ||
| arch_digest = None | ||
| arch_os, arch_variant = self._parse_arch(architecture) | ||
| for image in tag.get('images', []): | ||
| if image.get('architecture') == arch_variant and image.get('os') == arch_os: | ||
| arch_digest = image.get('digest') | ||
| break | ||
|
|
||
| tag_data.append({ | ||
| 'name': tag['name'], | ||
| 'last_updated': tag.get('last_updated'), | ||
| 'size': tag.get('full_size', 0), | ||
| 'digest': arch_digest | ||
| }) | ||
|
|
||
| # Handle pagination if there are more tags | ||
| while 'next' in data and data['next'] and len(tag_data) < 1000: # Limit to avoid too many requests | ||
| response = requests.get(data['next'], timeout=30) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Honor limit, reduce page size, and stop pagination when the limit is reached; use the TIMEOUT constant.
Fixes Ruff ARG002 (unused arg) and avoids excessive fetch.
As per coding guidelines
- url: str = f"https://hub.docker.com/v2/repositories/{namespace}/{repo}/tags?page_size=100"
+ page_size = max(1, min(int(limit), 100))
+ url: str = f"https://hub.docker.com/v2/repositories/{namespace}/{repo}/tags?page_size={page_size}"
try:
- response = requests.get(url, timeout=30)
+ response = requests.get(url, timeout=TIMEOUT)
response.raise_for_status()
data = response.json()
tag_data: list[dict[str, Any]] = []
for tag in data.get('results', []):
@@
tag_data.append({
'name': tag['name'],
'last_updated': tag.get('last_updated'),
'size': tag.get('full_size', 0),
'digest': arch_digest
})
+ if len(tag_data) >= limit:
+ break
# Handle pagination if there are more tags
- while 'next' in data and data['next'] and len(tag_data) < 1000: # Limit to avoid too many requests
- response = requests.get(data['next'], timeout=30)
+ while 'next' in data and data['next'] and len(tag_data) < limit:
+ response = requests.get(data['next'], timeout=TIMEOUT)
response.raise_for_status()
data = response.json()
for tag in data.get('results', []):
@@
tag_data.append({
'name': tag['name'],
'last_updated': tag.get('last_updated'),
'size': tag.get('full_size', 0),
'digest': arch_digest
})
+ if len(tag_data) >= limit:
+ breakAlso applies to: 76-91
| def get_registry_tags(self, registry_url: str, image_name: str, limit: int = 10, architecture: str = "linux/amd64") -> list[dict[str, Any]]: | ||
| """Query a registry API v2 for image tags and attempt to get creation time.""" | ||
| url: str = f"{registry_url}/v2/{image_name}/tags/list" | ||
| try: | ||
| response = requests.get(url, timeout=30) | ||
| response.raise_for_status() | ||
| data = response.json() | ||
| tags: list[str] = data.get('tags', []) | ||
|
|
||
| # For Docker Registry API v2, we need to make additional requests to get manifest and timestamps | ||
| tag_data: list[dict[str, Any]] = [] | ||
| for tag in tags[:100]: # Limit the number of additional requests |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Registry fetch: respect limit, reuse parsed arch, use TIMEOUT, and narrow date parsing exception.
Prevents over-fetch and addresses ARG002 and BLE001.
As per coding guidelines
- try:
- response = requests.get(url, timeout=30)
+ try:
+ response = requests.get(url, timeout=TIMEOUT)
response.raise_for_status()
data = response.json()
tags: list[str] = data.get('tags', [])
# For Docker Registry API v2, we need to make additional requests to get manifest and timestamps
tag_data: list[dict[str, Any]] = []
- for tag in tags[:100]: # Limit the number of additional requests
+ # Cap per-call fan-out and honor limit
+ for tag in tags[: max(0, min(limit, 1000))]:
manifest_url = f"{registry_url}/v2/{image_name}/manifests/{tag}"
try:
# Try to get the manifest to extract creation time
headers = {'Accept': 'application/vnd.docker.distribution.manifest.v2+json'}
- manifest_response = requests.get(manifest_url, headers=headers, timeout=30)
+ manifest_response = requests.get(manifest_url, headers=headers, timeout=TIMEOUT)
manifest_response.raise_for_status()
# Get digest from the manifest
manifest = manifest_response.json()
# For multi-arch images, we need to find the digest for the specific architecture
- digest = None
+ digest = None
+ arch_os, arch_variant = self._parse_arch(architecture)
# Try to parse the architecture from the manifest if it's a multi-arch image
if 'manifests' in manifest:
for m in manifest.get('manifests', []):
- if m.get('platform', {}).get('architecture') == architecture.split('/')[1] and \
- m.get('platform', {}).get('os') == architecture.split('/')[0]:
+ if m.get('platform', {}).get('architecture') == arch_variant and \
+ m.get('platform', {}).get('os') == arch_os:
digest = m.get('digest')
break
else:
# If it's not a multi-arch manifest, just use the digest directly
digest = manifest_response.headers.get('Docker-Content-Digest')
@@
- def _httpdate(dt_str):
+ def _httpdate(dt_str):
try:
return parsedate_to_datetime(dt_str)
- except Exception:
+ except (TypeError, ValueError):
return datetime.min
tag_data.sort(key=lambda x: _httpdate(x['last_updated']) if x['last_updated'] else datetime.min, reverse=True)Also applies to: 129-139, 141-165
🧰 Tools
🪛 Ruff (0.13.1)
106-106: Unused method argument: limit
(ARG002)
🤖 Prompt for AI Agents
In scripts/infra-mcp/tools/get_container_tags.py around lines 106 to 117 (and
similarly apply changes to 129-139 and 141-165), the tag-fetch loop currently
ignores the passed limit, reparses architecture per-tag, uses a hardcoded
timeout literal, and catches broad exceptions when parsing dates; update the
code to slice the tags list using the provided limit (e.g., tags[:limit]), parse
the architecture once before the loop and reuse that value when filtering,
replace the literal timeout with a module-level TIMEOUT constant, and narrow the
exception handling to the specific date parsing exception (e.g., catch
ValueError or dateutil.parser.ParserError) instead of a bare except so only
parsing errors are suppressed.
f0f7de2 to
d6d6157
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (17)
scripts/infra-mcp/utils/git.py (1)
22-27: Optional: capture and surface stderr in error messages (debuggability).If you want richer diagnostics, include result.stderr in the CalledProcessError path (behind a debug flag) or log it before raising.
scripts/infra-mcp/pyproject.toml (1)
8-8: Optional: bump requests floor to latest 2.32.xNot required, but moving to >=2.32.5 picks up fixes without API changes.
Apply:
- "requests>=2.32.4,<3", + "requests>=2.32.5,<3",Based on learnings.
.claude/settings.json (1)
7-12: Scope Bash permission to least privilege"Bash(scripts/labctl.py *)" grants broad execution. Consider whitelisting specific subcommands you need (e.g., Bash("scripts/labctl.py service *"), Bash("scripts/labctl.py config apply *")) to reduce blast radius.
scripts/labctl.py (3)
84-108: Avoid bare except; narrow to OS errors for symlinkCatching Exception hides unrelated failures; OS errors are expected here.
- try: - os.symlink(f"{hostname}/", localhost_link, target_is_directory=True) - except Exception: - logger.exception("Error creating localhost symlink") + try: + os.symlink(f"{hostname}/", localhost_link, target_is_directory=True) + except OSError: + logger.exception("Error creating localhost symlink")
158-171: Prefer FileNotFoundError for missing docker; addresses Ruff TRY003More precise exception and shorter message improves linting and clarity.
- docker_bin = shutil.which("docker") - if docker_bin is None: - raise RuntimeError("Docker executable not found on PATH.") from None + docker_bin = shutil.which("docker") + if docker_bin is None: + raise FileNotFoundError("docker not found on PATH") # noqa: TRY003
256-265: Avoid bare except when loading YAMLNarrow to (OSError, yaml.YAMLError) to satisfy Ruff and reduce noise.
- except Exception: - logger.exception(f"Error loading configuration file {config_file}") + except (OSError, yaml.YAMLError): + logger.exception("Error loading configuration file %s", config_file)scripts/infra-mcp/start-server.sh (1)
21-22: Harden script for ShellCheck and robustnessAdd strict flags, guard cd, and silence SC1091 for virtualenv sourcing to satisfy ShellCheck.
#!/usr/bin/env bash set -euo pipefail # Change to script directory cd "$(dirname "$0")" || exit 1 # Check if virtual environment exists, create if it doesn't if [ ! -d ".venv" ]; then echo "Creating virtual environment..." uv venv fi # Activate virtual environment # shellcheck disable=SC1091 source .venv/bin/activate # Install or update dependencies echo "Installing dependencies..." uv sync # Start the MCP server echo "Starting Infra MCP server..." fastmcp run server.py --transport http --host 127.0.0.1 --port 9876scripts/infra-mcp/tools/get_app_icon.py (2)
118-124: Follow redirects and GET fallback for /favicon.icoSome servers 30x favicon.ico or disallow HEAD. Follow redirects and fallback to GET for robustness.
- default_favicon = urljoin(homepage_url, '/favicon.ico') - favicon_response = requests.head(default_favicon, headers=self.headers, timeout=5) - if favicon_response.status_code == 200: - return default_favicon - else: - return None + default_favicon = urljoin(homepage_url, '/favicon.ico') + favicon_response = requests.head(default_favicon, headers=self.headers, timeout=5, allow_redirects=True) + if favicon_response.status_code == 200: + return default_favicon + # Fallback for sites that block HEAD + try: + get_resp = requests.get(default_favicon, headers=self.headers, timeout=5) + if get_resp.ok: + return default_favicon + except requests.RequestException: + pass + return None
70-74: Redundant else branches are acceptable but can be simplifiedNo functional issue; consider dropping else returns for brevity if desired.
docs/web/update-docs.py (2)
368-391: Shorten RuntimeError messages to satisfy Ruff TRY003Current raise messages are overly long. Prefer concise messages and let context/traceback carry details.
As per coding guidelines
def get_git_root() -> str: @@ - if git_cmd is None: - raise RuntimeError("Git not found on PATH") from None + if git_cmd is None: + raise RuntimeError("Git not found on PATH") from None @@ - 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 + except FileNotFoundError: + raise RuntimeError("Git executable not found") from None + except subprocess.CalledProcessError: + raise RuntimeError("Not a Git repository") from None
368-392: Avoid duplicating get_git_root; reuse infra-mcp/utils/git.pyThis implementation duplicates scripts/infra-mcp/utils/git.py. Import and reuse to reduce drift.
Proposed change (outside this range): replace local function with
from scripts.infra-mcp.utils.git import get_git_root(adjust sys.path if needed for docs build pipeline).scripts/infra-mcp/tools/collections/task_tools.py (2)
29-40: Resolve repo path before subprocess; keep Ruff happy (S603 context retained)Use an absolute path for --dir to avoid ambiguity and improve logs.
As per coding guidelines
- try: - result = subprocess.run( # noqa: S603 - [task_bin, "--list-all", "--dir", repository_root_path], + try: + import os # at top-level import if missing + repo_path = os.path.abspath(repository_root_path) + result = subprocess.run( # noqa: S603 + [task_bin, "--list-all", "--dir", repo_path], capture_output=True, text=True, check=True )If
osisn’t imported yet, addimport osat the top.
73-87: Validate task_name and improve error result robustness
- Add basic validation for task_name chars to avoid accidental misuse.
- Use e.stderr or str(e) to avoid "None" in error string.
- logger.info(f"Executing task: {task_name}") + # Names typically include letters, digits, dashes, underscores, and colons + if not re.match(r'^[A-Za-z0-9:_-]+$', task_name): + return f"Invalid task name: {task_name!r}" + logger.info(f"Executing task: {task_name}") @@ - except subprocess.CalledProcessError as e: - logger.exception(f"Error executing task {task_name}") - return f"Error executing task {task_name}: {e.stderr}" + except subprocess.CalledProcessError as e: + logger.exception("Error executing task %s", task_name) + return f"Error executing task {task_name}: {e.stderr or str(e)}"scripts/infra-mcp/tools/get_container_categories.py (2)
62-73: Drop broad except; let specific errors surfaceCatching Exception and wrapping in RuntimeError hides useful context and trips BLE001. There are no expected runtime errors beyond FileNotFoundError from _check_docker_dir_exists and OSErrors during traversal.
As per coding guidelines
- try: - # Ensure docker directory exists - self._check_docker_dir_exists() + # Ensure docker directory exists + self._check_docker_dir_exists() @@ - return sorted(categories) - except Exception as e: - raise RuntimeError(f"Error finding container categories: {str(e)}") from None + return sorted(categories)
66-70: Use Path.as_posix() instead of manual backslash replacementSimplifies cross-platform path string formatting.
- rel_path_str = str(rel_path).replace('\\', '/') + rel_path_str = rel_path.as_posix()scripts/infra-mcp/server.py (1)
40-58: SSRF hardening for get-app-iconhomepage_url is user-controlled. Add a basic allowlist for scheme and block localhost.
@mcp.tool(name="get-app-icon") def get_app_icon(app_name: str, homepage_url: str) -> str: @@ - icon_finder = AppIconFinder() + from urllib.parse import urlparse + icon_finder = AppIconFinder() + parsed = urlparse(homepage_url) + if parsed.scheme not in {"http", "https"} or (parsed.hostname in {"localhost", "127.0.0.1"}): + logger.warning("Blocked potentially unsafe homepage_url=%r", homepage_url) + return "default"If you want stricter private-IP checks, I can add RFC1918 filtering.
scripts/infra-mcp/tools/get_container_tags.py (1)
171-186: Narrow exception in datetime formatterCatching Exception is too broad; handle only expected parse errors.
As per coding guidelines
- except ValueError: + except ValueError: # Try to parse HTTP date format try: dt = parsedate_to_datetime(datetime_str) return dt.strftime('%Y-%m-%d %H:%M:%S UTC') - except Exception: + except (TypeError, ValueError): return datetime_str
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
scripts/infra-mcp/uv.lockis excluded by!**/*.lockscripts/task-mcp/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (32)
.claude/commands/fix-lint-issue.md(1 hunks).claude/commands/plan-container-deployment.md(2 hunks).claude/settings.json(1 hunks).mcp.json(1 hunks).vscode/mcp.json(1 hunks)AGENTS.md(1 hunks)CLAUDE.md(0 hunks)CLAUDE.md(1 hunks)GEMINI.md(1 hunks)Taskfile.yaml(1 hunks)docker/guidelines.md(2 hunks)docs/web/update-docs.py(3 hunks)ruff.toml(2 hunks)scripts/get-container-tags.py(0 hunks)scripts/git-reorder-fixup.py(0 hunks)scripts/github-star-repo.py(1 hunks)scripts/infra-mcp/README.md(2 hunks)scripts/infra-mcp/pyproject.toml(1 hunks)scripts/infra-mcp/server.py(1 hunks)scripts/infra-mcp/start-server.sh(1 hunks)scripts/infra-mcp/tools/__init__.py(1 hunks)scripts/infra-mcp/tools/collections/__init__.py(1 hunks)scripts/infra-mcp/tools/collections/container_tools.py(1 hunks)scripts/infra-mcp/tools/collections/task_tools.py(1 hunks)scripts/infra-mcp/tools/get_app_icon.py(3 hunks)scripts/infra-mcp/tools/get_container_categories.py(1 hunks)scripts/infra-mcp/tools/get_container_tags.py(1 hunks)scripts/infra-mcp/tools/get_dashboard_groups.py(1 hunks)scripts/infra-mcp/utils/__init__.py(1 hunks)scripts/infra-mcp/utils/git.py(1 hunks)scripts/labctl.py(12 hunks)scripts/task-mcp/server.py(0 hunks)
💤 Files with no reviewable changes (3)
- scripts/task-mcp/server.py
- scripts/get-container-tags.py
- scripts/git-reorder-fixup.py
✅ Files skipped from review due to trivial changes (4)
- scripts/infra-mcp/tools/init.py
- CLAUDE.md
- scripts/infra-mcp/utils/init.py
- GEMINI.md
🚧 Files skipped from review as they are similar to previous changes (7)
- .claude/commands/fix-lint-issue.md
- scripts/infra-mcp/tools/collections/container_tools.py
- AGENTS.md
- ruff.toml
- .claude/commands/plan-container-deployment.md
- scripts/infra-mcp/tools/collections/init.py
- docker/guidelines.md
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{yml,yaml}
📄 CodeRabbit inference engine (CLAUDE.md)
All YAML files must be linted and valid
Files:
Taskfile.yaml
scripts/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Python code must pass Ruff linting
Files:
scripts/infra-mcp/tools/get_app_icon.pyscripts/github-star-repo.pyscripts/infra-mcp/tools/get_container_categories.pyscripts/infra-mcp/tools/collections/task_tools.pyscripts/infra-mcp/tools/get_container_tags.pyscripts/infra-mcp/tools/get_dashboard_groups.pyscripts/infra-mcp/utils/git.pyscripts/infra-mcp/server.pyscripts/labctl.py
**/*.sh
📄 CodeRabbit inference engine (CLAUDE.md)
Shell scripts must pass ShellCheck
Files:
scripts/infra-mcp/start-server.sh
🧠 Learnings (1)
📚 Learning: 2025-08-31T08:38:34.585Z
Learnt from: CR
PR: bubacoder/infra#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-31T08:38:34.585Z
Learning: Manage Docker services using the scripts/labctl.py tool (operations: up, down, restart, recreate, update, pull, config)
Applied to files:
scripts/labctl.py
🧬 Code graph analysis (7)
scripts/infra-mcp/tools/get_app_icon.py (2)
scripts/task-mcp/tools/find_app_icon.py (8)
AppIconFinder(12-123)_find_dashboard_icon(51-72)_find_favicon_url(74-123)get_app_icon(26-49)main(150-170)test_icon_finder(126-147)get_priority(96-106)__init__(18-24)scripts/task-mcp/server.py (1)
find_app_icon(165-181)
scripts/infra-mcp/tools/get_container_categories.py (2)
scripts/infra-mcp/utils/git.py (1)
get_git_root(9-32)scripts/infra-mcp/server.py (1)
get_container_categories(77-90)
scripts/infra-mcp/tools/collections/task_tools.py (1)
scripts/task-mcp/server.py (4)
get_task_list(48-81)execute_task(84-104)task_fn(117-118)create_task_function(107-120)
scripts/infra-mcp/tools/get_container_tags.py (1)
scripts/get-container-tags.py (8)
main(433-468)get_registry_tags(86-148)get_docker_hub_tags(18-83)get_most_specific_tag(383-430)list_same_hash_tags(326-380)list_recent_tags(295-323)get_image_tags(275-292)determine_tag_specificity(198-250)
scripts/infra-mcp/tools/get_dashboard_groups.py (2)
scripts/infra-mcp/utils/git.py (1)
get_git_root(9-32)scripts/infra-mcp/server.py (1)
get_dashboard_groups(61-73)
scripts/infra-mcp/utils/git.py (1)
scripts/task-mcp/server.py (1)
get_git_root(32-45)
scripts/infra-mcp/server.py (7)
scripts/infra-mcp/tools/collections/container_tools.py (1)
add_container_operation_tools(105-128)scripts/infra-mcp/tools/collections/task_tools.py (1)
add_task_tools(107-130)scripts/infra-mcp/tools/get_app_icon.py (3)
get_app_icon(26-49)AppIconFinder(12-127)main(154-174)scripts/infra-mcp/tools/get_container_categories.py (3)
get_container_categories(38-74)ContainerCategoryFinder(16-74)main(77-92)scripts/infra-mcp/tools/get_container_tags.py (5)
ContainerTagFinder(12-454)get_image_tags(302-319)list_same_hash_tags(351-405)get_most_specific_tag(407-454)main(457-497)scripts/infra-mcp/tools/get_dashboard_groups.py (3)
get_dashboard_groups(30-47)DashboardGroupFinder(18-47)main(50-68)scripts/infra-mcp/utils/git.py (1)
get_git_root(9-32)
🪛 Ruff (0.13.1)
scripts/infra-mcp/tools/get_container_categories.py
36-36: Avoid specifying long messages outside the exception class
(TRY003)
73-73: Do not catch blind exception: Exception
(BLE001)
74-74: Avoid specifying long messages outside the exception class
(TRY003)
74-74: Use explicit conversion flag
Replace with conversion flag
(RUF010)
scripts/infra-mcp/tools/get_container_tags.py
38-38: Unused method argument: limit
(ARG002)
96-96: Do not catch blind exception: Exception
(BLE001)
106-106: Unused method argument: limit
(ARG002)
161-161: Do not catch blind exception: Exception
(BLE001)
184-184: Do not catch blind exception: Exception
(BLE001)
docs/web/update-docs.py
379-379: Avoid specifying long messages outside the exception class
(TRY003)
388-388: Avoid specifying long messages outside the exception class
(TRY003)
390-390: Avoid specifying long messages outside the exception class
(TRY003)
scripts/infra-mcp/utils/git.py
20-20: Avoid specifying long messages outside the exception class
(TRY003)
29-29: Avoid specifying long messages outside the exception class
(TRY003)
31-31: Avoid specifying long messages outside the exception class
(TRY003)
scripts/infra-mcp/server.py
161-161: PEP 484 prohibits implicit Optional
Convert to Optional[T]
(RUF013)
scripts/labctl.py
170-170: Avoid specifying long messages outside the exception class
(TRY003)
⏰ 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: check
- GitHub Check: build
🔇 Additional comments (20)
scripts/github-star-repo.py (1)
36-36: Timeout addition improves reliabilitySetting an explicit timeout on the GitHub request prevents the script from hanging indefinitely on slow or unresponsive endpoints. Looks good to me.
.mcp.json (2)
7-19: Infra MCP server registration looks good.Valid JSON, stdio wiring via uv run with --directory looks consistent, and env flags align with the PR intent.
9-15: Paths verified; ensureuvis on PATH.
server.pyandpyproject.tomlexist under scripts/infra-mcp and the args match; confirm theuvbinary (e.g. uvicorn) is installed and accessible in the target environment.scripts/infra-mcp/pyproject.toml (3)
14-14: Entry point looks good; ensure server.main existsIf server.py defines main() at module scope, this will work.
9-10: Fix bs4 pin; 4.13 likely doesn’t exist"beautifulsoup4>=4.13,<5" can break resolution. Pin to a known release in 4.12.x.
Apply:
- "beautifulsoup4>=4.13,<5", + "beautifulsoup4>=4.12.3,<5",
5-5: Python ≥3.13 floor matches CI
Workflows pinpython-version: '3.13', sorequires-python = ">=3.13"aligns with the tested environment. No change required unless you intend to support Python 3.11/3.12..claude/settings.json (1)
16-20: JSON now valid; env defaults OKGood cleanup of comments and addition of sane timeouts.
scripts/labctl.py (3)
200-212: LGTM: options plumbed and networks ensured pre-runGood cohesion: DockerOptions defaulting and network creation before compose.
267-276: CLI quiet/pull plumbed end-to-endClear docstrings and consistent propagation. Looks good.
174-198: Remove fallback: ‘docker compose pull’ supports --quiet
Per official Docker Compose docs, pull accepts -q/--quiet; no version check or fallback needed.scripts/infra-mcp/README.md (2)
1-1: Rename looks consistentTitle and paths align with infra-mcp. Good.
23-23: Path update to infra-mcp is correctThe setup steps will land in the right directory.
.vscode/mcp.json (1)
22-22: Confirm uv availability in VS Code environmentThis server relies on uv. Ensure it’s installed and on PATH in the VS Code session.
scripts/infra-mcp/tools/get_app_icon.py (1)
158-158: CLI prog rename is correctMatches file rename and usage.
Taskfile.yaml (1)
42-47: SKIP hook IDs match .pre-commit-config.yaml All declared IDs (renovate-config-validator, terraform_fmt, terraform_docs, terraform_validate, terraform_tflint, ansible-lint, kics-scan) are present; no changes needed.scripts/infra-mcp/tools/get_container_tags.py (3)
9-10: Define a shared timeout constant for all HTTP callsCentralizes network timeouts and avoids magic numbers.
As per coding guidelines
-import requests +import requests +TIMEOUT = (5, 15) # (connect, read) seconds
47-53: Honor limit, reduce page size, and use TIMEOUT; narrow exceptionCurrent Docker Hub fetch ignores limit and uses large fixed page sizes; also catches broad Exception in ISO parser.
As per coding guidelines
- url: str = f"https://hub.docker.com/v2/repositories/{namespace}/{repo}/tags?page_size=100" + page_size = max(1, min(100, int(limit))) + url: str = f"https://hub.docker.com/v2/repositories/{namespace}/{repo}/tags?page_size={page_size}" try: - response = requests.get(url, timeout=30) + response = requests.get(url, timeout=TIMEOUT) @@ - while 'next' in data and data['next'] and len(tag_data) < 1000: # Limit to avoid too many requests - response = requests.get(data['next'], timeout=30) + while 'next' in data and data['next'] and len(tag_data) < limit: + response = requests.get(data['next'], timeout=TIMEOUT) @@ - for tag in data.get('results', []): + for tag in data.get('results', []): @@ tag_data.append({ 'name': tag['name'], 'last_updated': tag.get('last_updated'), 'size': tag.get('full_size', 0), 'digest': arch_digest }) + if len(tag_data) >= limit: + break @@ - try: - return datetime.fromisoformat(dt_str.replace('Z', '+00:00')) - except Exception: + try: + return datetime.fromisoformat(dt_str.replace('Z', '+00:00')) + except ValueError: return datetime.minAlso applies to: 70-76, 92-104
106-123: Registry: respect limit, use TIMEOUT, and narrow date parsing exceptionsAvoid excessive fan-out and broad exceptions.
As per coding guidelines
- try: - response = requests.get(url, timeout=30) + try: + response = requests.get(url, timeout=TIMEOUT) @@ - for tag in tags[:100]: # Limit the number of additional requests + for tag in tags[: max(1, min(limit, 1000))]: @@ - manifest_response = requests.get(manifest_url, headers=headers, timeout=30) + manifest_response = requests.get(manifest_url, headers=headers, timeout=TIMEOUT) @@ - def _httpdate(dt_str): + def _httpdate(dt_str): try: return parsedate_to_datetime(dt_str) - except Exception: + except (TypeError, ValueError): return datetime.minAlso applies to: 157-165
scripts/infra-mcp/tools/get_dashboard_groups.py (1)
43-47: Guard against empty or invalid YAML (avoids AttributeError)yaml.safe_load can return None; layout may not be a dict. Add safe defaults.
As per coding guidelines
- with open(settings_file) as f: - settings = yaml.safe_load(f) - - layout = settings.get('layout', {}) - return list(layout.keys()) + with open(settings_file, encoding="utf-8") as f: + settings = yaml.safe_load(f) or {} + layout = settings.get('layout') or {} + if not isinstance(layout, dict): + return [] + return list(layout.keys())scripts/infra-mcp/server.py (1)
160-207: Fix stdout redirection leak; annotate Optional; avoid duplicate callsOverwriting sys.stdout without guaranteed restoration risks muting the process on exceptions. Use contextlib.redirect_stdout. Also make tag annotation explicit and avoid calling list_same_hash_tags twice.
As per coding guidelines
-@mcp.tool(name="get-most-specific-container-tag") -def get_most_specific_container_tag(image: str, tag: str = None, limit: int = 100) -> str: +@mcp.tool(name="get-most-specific-container-tag") +def get_most_specific_container_tag(image: str, tag: str | None = None, limit: int = 100) -> str: @@ - try: + try: # Create a namespace to simulate command line args class Args: pass @@ - # Temporarily redirect stdout to capture and prevent output - original_stdout = sys.stdout - sys.stdout = io.StringIO() - - # Get most specific tag - most_specific = tag_finder.get_most_specific_tag(args) - - # Restore stdout - sys.stdout = original_stdout + import contextlib + buffer = io.StringIO() + with contextlib.redirect_stdout(buffer): + most_specific = tag_finder.get_most_specific_tag(args) + same_hash = tag_finder.list_same_hash_tags(args, suppress_output=True) @@ - elif tag_finder.list_same_hash_tags(args, suppress_output=True): - # If there are same-hash tags but no "most specific" one was identified - # return the first one - return tag_finder.list_same_hash_tags(args, suppress_output=True)[0]['name'] + elif same_hash: + # If there are same-hash tags but no "most specific" one, return the first one + return same_hash[0]['name']Add import near top if not present:
+import contextlib
d6d6157 to
ce47501
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (9)
scripts/infra-mcp/pyproject.toml (1)
5-5: Confirm Python floor version is intentional.requires-python ">=3.13" narrows install base. If not required, consider >=3.10 to match FastMCP guidance.
scripts/infra-mcp/tools/collections/task_tools.py (2)
35-41: Add deterministic decoding and absolute path for --dir.Include encoding="utf-8" and pass an absolute repo path for clarity.
- result = subprocess.run( # noqa: S603 - [task_bin, "--list-all", "--dir", repository_root_path], + import os + repo_path = os.path.abspath(repository_root_path) + result = subprocess.run( # noqa: S603 + [task_bin, "--list-all", "--dir", repo_path], capture_output=True, - text=True, + text=True, + encoding="utf-8", check=True )
74-87: Harden execute path; logging format; deterministic decoding.
- Use absolute path for --dir.
- Prefer logger.exception with format args.
- Add encoding.
- task_bin = shutil.which("task") + task_bin = shutil.which("task") @@ - try: - return subprocess.run( # noqa: S603 - [task_bin, task_name, "--dir", repository_root_path], + try: + import os + repo_path = os.path.abspath(repository_root_path) + return subprocess.run( # noqa: S603 + [task_bin, task_name, "--dir", repo_path], capture_output=True, - text=True, + text=True, + encoding="utf-8", check=True ).stdout.strip() except subprocess.CalledProcessError as e: - logger.exception(f"Error executing task {task_name}") - return f"Error executing task {task_name}: {e.stderr}" + logger.exception("Error executing task %s", task_name) + return f"Error executing task {task_name}: {e.stderr or str(e)}"scripts/labctl.py (1)
262-264: Use logging format args for exceptions.Minor polish; avoids f-string in logger.exception messages.
- except Exception: - logger.exception(f"Error loading configuration file {config_file}") + except Exception: + logger.exception("Error loading configuration file %s", config_file)scripts/infra-mcp/tools/collections/container_tools.py (2)
58-76: Validate labctl path and resolve repository path before subprocess.Harden inputs and improve error reporting; keeps S603 suppressed with justification.
- cmd = [ - sys.executable, # Use the current Python interpreter - os.path.join(repository_root_path, "scripts", "labctl.py"), - "service", - operation, - service_name - ] + repo_path = os.path.abspath(repository_root_path) + labctl_path = os.path.join(repo_path, "scripts", "labctl.py") + if not os.path.isfile(labctl_path): + return f"labctl.py not found at: {labctl_path}" + cmd = [sys.executable, labctl_path, "service", operation, service_name] @@ - result = subprocess.run( # noqa: S603 + result = subprocess.run( # noqa: S603 cmd, capture_output=True, text=True, check=True )
20-34: Optional: add return type annotations for clarity.Public helpers benefit from explicit types in this module.
-def get_container_operations(): +def get_container_operations() -> list[dict[str, str]]:scripts/infra-mcp/server.py (1)
126-159: Minor: avoid shadowing parameter name in comprehensionUse a different loop variable than “tag” for readability.
- return [tag['name'] for tag in same_hash_tags] if same_hash_tags else [] + return [t['name'] for t in same_hash_tags] if same_hash_tags else []scripts/infra-mcp/tools/get_container_tags.py (2)
9-10: Define and reuse a requests TIMEOUT constantUnify timeouts and avoid literals per S113 best practices.
import requests +TIMEOUT = (5, 15) # (connect, read) secondsBased on learnings
171-186: Narrow inner exception in datetime formatterAvoid bare Exception; restrict to (TypeError, ValueError).
except (TypeError, ValueError): # Try to parse HTTP date format try: dt = parsedate_to_datetime(datetime_str) return dt.strftime('%Y-%m-%d %H:%M:%S UTC') - except Exception: + except (TypeError, ValueError): return datetime_strAs per coding guidelines
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
scripts/infra-mcp/uv.lockis excluded by!**/*.lockscripts/task-mcp/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (22)
.claude/commands/plan-container-deployment.md(2 hunks).claude/settings.json(1 hunks).mcp.json(1 hunks).vscode/mcp.json(1 hunks)docker/guidelines.md(2 hunks)scripts/get-container-tags.py(0 hunks)scripts/infra-mcp/README.md(2 hunks)scripts/infra-mcp/pyproject.toml(1 hunks)scripts/infra-mcp/server.py(1 hunks)scripts/infra-mcp/start-server.sh(1 hunks)scripts/infra-mcp/tools/__init__.py(1 hunks)scripts/infra-mcp/tools/collections/__init__.py(1 hunks)scripts/infra-mcp/tools/collections/container_tools.py(1 hunks)scripts/infra-mcp/tools/collections/task_tools.py(1 hunks)scripts/infra-mcp/tools/get_app_icon.py(3 hunks)scripts/infra-mcp/tools/get_container_categories.py(1 hunks)scripts/infra-mcp/tools/get_container_tags.py(1 hunks)scripts/infra-mcp/tools/get_dashboard_groups.py(1 hunks)scripts/infra-mcp/utils/__init__.py(1 hunks)scripts/infra-mcp/utils/git.py(1 hunks)scripts/labctl.py(12 hunks)scripts/task-mcp/server.py(0 hunks)
💤 Files with no reviewable changes (2)
- scripts/task-mcp/server.py
- scripts/get-container-tags.py
✅ Files skipped from review due to trivial changes (1)
- scripts/infra-mcp/tools/collections/init.py
🚧 Files skipped from review as they are similar to previous changes (8)
- .vscode/mcp.json
- scripts/infra-mcp/utils/init.py
- .claude/settings.json
- scripts/infra-mcp/start-server.sh
- scripts/infra-mcp/README.md
- scripts/infra-mcp/tools/init.py
- scripts/infra-mcp/tools/get_app_icon.py
- scripts/infra-mcp/tools/get_dashboard_groups.py
🧰 Additional context used
📓 Path-based instructions (1)
scripts/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Python code must pass Ruff linting
Files:
scripts/infra-mcp/utils/git.pyscripts/infra-mcp/tools/collections/task_tools.pyscripts/labctl.pyscripts/infra-mcp/server.pyscripts/infra-mcp/tools/get_container_tags.pyscripts/infra-mcp/tools/collections/container_tools.pyscripts/infra-mcp/tools/get_container_categories.py
🧠 Learnings (2)
📚 Learning: 2025-08-31T08:38:34.585Z
Learnt from: CR
PR: bubacoder/infra#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-31T08:38:34.585Z
Learning: Add new services to host configuration at config/docker/<hostname>/services.yaml
Applied to files:
docker/guidelines.md
📚 Learning: 2025-08-31T08:38:34.585Z
Learnt from: CR
PR: bubacoder/infra#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-31T08:38:34.585Z
Learning: Manage Docker services using the scripts/labctl.py tool (operations: up, down, restart, recreate, update, pull, config)
Applied to files:
scripts/labctl.py
🧬 Code graph analysis (7)
scripts/infra-mcp/utils/git.py (2)
scripts/task-mcp/server.py (1)
get_git_root(32-45)docs/web/update-docs.py (1)
get_git_root(368-375)
scripts/infra-mcp/tools/collections/task_tools.py (1)
scripts/task-mcp/server.py (4)
get_task_list(48-81)execute_task(84-104)task_fn(117-118)create_task_function(107-120)
scripts/labctl.py (1)
scripts/task-mcp/server.py (1)
control_container_service(124-161)
scripts/infra-mcp/server.py (7)
scripts/infra-mcp/tools/collections/container_tools.py (1)
add_container_operation_tools(105-128)scripts/infra-mcp/tools/collections/task_tools.py (1)
add_task_tools(107-131)scripts/infra-mcp/tools/get_app_icon.py (2)
get_app_icon(26-49)AppIconFinder(12-127)scripts/infra-mcp/tools/get_container_categories.py (2)
get_container_categories(38-74)ContainerCategoryFinder(16-74)scripts/infra-mcp/tools/get_container_tags.py (4)
ContainerTagFinder(12-454)get_image_tags(302-319)list_same_hash_tags(351-405)get_most_specific_tag(407-454)scripts/infra-mcp/tools/get_dashboard_groups.py (2)
get_dashboard_groups(30-50)DashboardGroupFinder(18-50)scripts/infra-mcp/utils/git.py (1)
get_git_root(9-32)
scripts/infra-mcp/tools/get_container_tags.py (1)
scripts/get-container-tags.py (10)
main(433-468)get_registry_tags(86-148)list_recent_tags(295-323)get_docker_hub_tags(18-83)get_most_specific_tag(383-430)list_same_hash_tags(326-380)get_image_tags(275-292)parse_image_reference(253-265)determine_tag_specificity(198-250)format_digest(179-187)
scripts/infra-mcp/tools/collections/container_tools.py (1)
scripts/task-mcp/server.py (1)
control_container_service(124-161)
scripts/infra-mcp/tools/get_container_categories.py (2)
scripts/infra-mcp/utils/git.py (1)
get_git_root(9-32)scripts/infra-mcp/server.py (1)
get_container_categories(78-91)
🪛 Ruff (0.13.1)
scripts/infra-mcp/utils/git.py
20-20: Avoid specifying long messages outside the exception class
(TRY003)
29-29: Avoid specifying long messages outside the exception class
(TRY003)
31-31: Avoid specifying long messages outside the exception class
(TRY003)
scripts/labctl.py
170-170: Avoid specifying long messages outside the exception class
(TRY003)
scripts/infra-mcp/server.py
226-226: Unused noqa directive (unused: BLE001)
Remove unused noqa directive
(RUF100)
scripts/infra-mcp/tools/get_container_tags.py
38-38: Unused method argument: limit
(ARG002)
106-106: Unused method argument: limit
(ARG002)
161-161: Do not catch blind exception: Exception
(BLE001)
184-184: Do not catch blind exception: Exception
(BLE001)
scripts/infra-mcp/tools/get_container_categories.py
36-36: Avoid specifying long messages outside the exception class
(TRY003)
73-73: Do not catch blind exception: Exception
(BLE001)
74-74: Avoid specifying long messages outside the exception class
(TRY003)
74-74: Use explicit conversion flag
Replace with conversion flag
(RUF010)
⏰ 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 (11)
.claude/commands/plan-container-deployment.md (3)
17-21: LGTM: updated to Infra MCP tools.
24-26: LGTM: doc flow clarified and consistent.
37-38: LGTM: template fields align with new tools.scripts/infra-mcp/tools/collections/task_tools.py (1)
117-131: LGTM: preserves namespaced task execution; sanitized tool names.scripts/infra-mcp/utils/git.py (1)
18-32: Fix Ruff TRY003; introduce typed exceptions and deterministic decoding.Define small exception classes, capture stderr, and set encoding="utf-8". Replace inline messages to satisfy Ruff and improve diagnostics. This also aligns with prior feedback.
Apply:
@@ -import shutil -import subprocess +import shutil +import subprocess @@ -def get_git_root() -> str: +class GitError(RuntimeError): + """Base class for git-related errors.""" + + +class GitNotFoundError(GitError): + """Raised when Git is not available on PATH.""" + + def __init__(self) -> None: + super().__init__("Git not found on PATH") + + +class NotAGitRepositoryError(GitError): + """Raised when not inside a Git repository.""" + + def __init__(self) -> None: + super().__init__("Not a Git repository") + + +def get_git_root() -> str: @@ - Raises: - RuntimeError: If git executable is not found or not in a git repository. + Raises: + GitError: If git executable is not found or not in a git repository. @@ - if git_cmd is None: - raise RuntimeError("Git not found on PATH") from None + if git_cmd is None: + raise GitNotFoundError() from None try: result = subprocess.run( # noqa: S603 [git_cmd, "rev-parse", "--show-toplevel"], stdout=subprocess.PIPE, + stderr=subprocess.PIPE, check=True, - text=True, + text=True, + encoding="utf-8", ) except FileNotFoundError: - raise RuntimeError("Git executable not found. Please install Git and ensure it is on your PATH.") from None + raise GitNotFoundError() from None except subprocess.CalledProcessError: - raise RuntimeError("Unable to locate git repository. Are you running this inside a Git repo?") from None + raise NotAGitRepositoryError() from None return result.stdout.strip()As per coding guidelines
.mcp.json (1)
7-19: LGTM; ensure the ‘uv’ command is available at runtime.scripts/infra-mcp/pyproject.toml (1)
9-10: Pin beautifulsoup4 to an existing release; consider latest requests.b4.13 likely doesn’t exist; use a known current 4.12.3. Optionally bump requests to 2.32.5.
- "beautifulsoup4>=4.13,<5", + "beautifulsoup4>=4.12.3,<5",Optional:
- "requests>=2.32.4,<3", + "requests>=2.32.5,<3",scripts/infra-mcp/server.py (2)
36-38: Health check handler looks goodProperly marks unused Request param and returns plain text OK.
41-58: Add SSRF guard for homepage_url before fetching iconsValidate scheme/host and block localhost/private IPs to prevent SSRF in get-app-icon.
Apply this diff:
def get_app_icon(app_name: str, homepage_url: str) -> str: @@ - icon_finder = AppIconFinder() + icon_finder = AppIconFinder() try: + # Basic SSRF guard: only http/https, require host, and block loopback/private ranges + parsed = urlparse(homepage_url) + if parsed.scheme not in {"http", "https"} or not parsed.hostname: + logger.warning("Blocked potentially unsafe homepage_url=%r (invalid scheme/host)", homepage_url) + return "default" + try: + ip = ipaddress.ip_address(parsed.hostname) + if ip.is_loopback or ip.is_private or ip.is_link_local or ip.is_reserved: + logger.warning("Blocked potentially unsafe homepage_url=%r (private/loopback IP)", homepage_url) + return "default" + except ValueError: + # Not an IP; still block common local hostnames + if parsed.hostname.lower() in {"localhost"}: + logger.warning("Blocked potentially unsafe homepage_url=%r (localhost)", homepage_url) + return "default" return icon_finder.get_app_icon(app_name, homepage_url) except Exception: logger.exception("find-app-icon failed for app_name=%r homepage_url=%r", app_name, homepage_url) return "default"Also add these imports near the top of the file:
from urllib.parse import urlparse import ipaddressscripts/infra-mcp/tools/get_container_tags.py (2)
321-350: CLI printing path and formatting helpers look soundOutput respects quiet flag; helpers are used consistently.
106-170: Respect limit, use TIMEOUT, parse arch once, and narrow date parsing in registry callsLimit currently unused (ARG002), timeout literals present, arch parsing repeated, and broad exception in date parsing.
Apply this diff:
def get_registry_tags(self, registry_url: str, image_name: str, limit: int = 10, architecture: str = "linux/amd64") -> list[dict[str, Any]]: """Query a registry API v2 for image tags and attempt to get creation time.""" url: str = f"{registry_url}/v2/{image_name}/tags/list" - try: - response = requests.get(url, timeout=30) + tag_data: list[dict[str, Any]] = [] + arch_os, arch_variant = self._parse_arch(architecture) + try: + response = requests.get(url, timeout=TIMEOUT) response.raise_for_status() data = response.json() tags: list[str] = data.get('tags', []) # For Docker Registry API v2, we need to make additional requests to get manifest and timestamps - tag_data: list[dict[str, Any]] = [] - for tag in tags[:100]: # Limit the number of additional requests + for tag in tags[: max(1, min(int(limit), 1000))]: manifest_url = f"{registry_url}/v2/{image_name}/manifests/{tag}" try: # Try to get the manifest to extract creation time headers = {'Accept': 'application/vnd.docker.distribution.manifest.v2+json'} - manifest_response = requests.get(manifest_url, headers=headers, timeout=30) + manifest_response = requests.get(manifest_url, headers=headers, timeout=TIMEOUT) manifest_response.raise_for_status() @@ - digest = None + digest = None # Try to parse the architecture from the manifest if it's a multi-arch image if 'manifests' in manifest: for m in manifest.get('manifests', []): - if m.get('platform', {}).get('architecture') == architecture.split('/')[1] and \ - m.get('platform', {}).get('os') == architecture.split('/')[0]: + if m.get('platform', {}).get('architecture') == arch_variant and \ + m.get('platform', {}).get('os') == arch_os: digest = m.get('digest') break else: # If it's not a multi-arch manifest, just use the digest directly digest = manifest_response.headers.get('Docker-Content-Digest') @@ - def _httpdate(dt_str): + def _httpdate(dt_str): try: return parsedate_to_datetime(dt_str) - except Exception: + except (TypeError, ValueError): return datetime.min tag_data.sort(key=lambda x: _httpdate(x['last_updated']) if x['last_updated'] else datetime.min, reverse=True) - except requests.exceptions.RequestException as e: - print(f"Error querying registry: {e}", file=sys.stderr) - return [] - else: - return tag_data + except requests.exceptions.RequestException as e: + print(f"Error querying registry: {e}", file=sys.stderr) + return [] + else: + return tag_dataAs per coding guidelines
| try: | ||
| # Ensure docker directory exists | ||
| self._check_docker_dir_exists() | ||
|
|
||
| # Walk through the docker directory recursively | ||
| for root, _dirs, files in os.walk(self.docker_path): | ||
| root_path = Path(root) | ||
|
|
||
| # Check for README.md | ||
| readme_path = root_path.joinpath("README.md") | ||
| has_readme = readme_path.exists() and readme_path.is_file() | ||
|
|
||
| # Check for any yaml files in the current directory | ||
| has_yaml = any(file.lower().endswith(('.yaml', '.yml')) for file in files) | ||
|
|
||
| # Only include directories with both README.md and at least one yaml file | ||
| if has_readme and has_yaml: | ||
| # Get the path relative to the docker directory | ||
| rel_path = root_path.relative_to(self.docker_path) | ||
| # Convert to string and use forward slashes for consistency | ||
| rel_path_str = str(rel_path).replace('\\', '/') | ||
| # Add the directory to categories if it's not the root docker directory | ||
| if rel_path_str != '.': | ||
| categories.append(rel_path_str) | ||
|
|
||
| return sorted(categories) | ||
| except Exception as e: | ||
| raise RuntimeError(f"Error finding container categories: {str(e)}") from None | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Avoid broad except (BLE001) and message strings (TRY003/RUF010).
Remove the broad try/except and let specific errors bubble; main() already handles them.
- try:
- # Ensure docker directory exists
- self._check_docker_dir_exists()
-
- # Walk through the docker directory recursively
- for root, _dirs, files in os.walk(self.docker_path):
- root_path = Path(root)
-
- # Check for README.md
- readme_path = root_path.joinpath("README.md")
- has_readme = readme_path.exists() and readme_path.is_file()
-
- # Check for any yaml files in the current directory
- has_yaml = any(file.lower().endswith(('.yaml', '.yml')) for file in files)
-
- # Only include directories with both README.md and at least one yaml file
- if has_readme and has_yaml:
- # Get the path relative to the docker directory
- rel_path = root_path.relative_to(self.docker_path)
- # Convert to string and use forward slashes for consistency
- rel_path_str = str(rel_path).replace('\\', '/')
- # Add the directory to categories if it's not the root docker directory
- if rel_path_str != '.':
- categories.append(rel_path_str)
-
- return sorted(categories)
- except Exception as e:
- raise RuntimeError(f"Error finding container categories: {str(e)}") from None
+ # Ensure docker directory exists
+ self._check_docker_dir_exists()
+ # Walk through the docker directory recursively
+ for root, _dirs, files in os.walk(self.docker_path):
+ root_path = Path(root)
+ has_readme = (root_path / "README.md").is_file()
+ has_yaml = any(file.lower().endswith(('.yaml', '.yml')) for file in files)
+ if has_readme and has_yaml:
+ rel_path = Path(root).relative_to(self.docker_path)
+ rel_path_str = str(rel_path).replace('\\', '/')
+ if rel_path_str != '.':
+ categories.append(rel_path_str)
+ return sorted(categories)📝 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.
| try: | |
| # Ensure docker directory exists | |
| self._check_docker_dir_exists() | |
| # Walk through the docker directory recursively | |
| for root, _dirs, files in os.walk(self.docker_path): | |
| root_path = Path(root) | |
| # Check for README.md | |
| readme_path = root_path.joinpath("README.md") | |
| has_readme = readme_path.exists() and readme_path.is_file() | |
| # Check for any yaml files in the current directory | |
| has_yaml = any(file.lower().endswith(('.yaml', '.yml')) for file in files) | |
| # Only include directories with both README.md and at least one yaml file | |
| if has_readme and has_yaml: | |
| # Get the path relative to the docker directory | |
| rel_path = root_path.relative_to(self.docker_path) | |
| # Convert to string and use forward slashes for consistency | |
| rel_path_str = str(rel_path).replace('\\', '/') | |
| # Add the directory to categories if it's not the root docker directory | |
| if rel_path_str != '.': | |
| categories.append(rel_path_str) | |
| return sorted(categories) | |
| except Exception as e: | |
| raise RuntimeError(f"Error finding container categories: {str(e)}") from None | |
| # Ensure docker directory exists | |
| self._check_docker_dir_exists() | |
| # Walk through the docker directory recursively | |
| for root, _dirs, files in os.walk(self.docker_path): | |
| root_path = Path(root) | |
| has_readme = (root_path / "README.md").is_file() | |
| has_yaml = any(file.lower().endswith(('.yaml', '.yml')) for file in files) | |
| if has_readme and has_yaml: | |
| rel_path = Path(root).relative_to(self.docker_path) | |
| rel_path_str = str(rel_path).replace('\\', '/') | |
| if rel_path_str != '.': | |
| categories.append(rel_path_str) | |
| return sorted(categories) |
🧰 Tools
🪛 Ruff (0.13.1)
73-73: Do not catch blind exception: Exception
(BLE001)
74-74: Avoid specifying long messages outside the exception class
(TRY003)
74-74: Use explicit conversion flag
Replace with conversion flag
(RUF010)
🤖 Prompt for AI Agents
In scripts/infra-mcp/tools/get_container_categories.py around lines 47 to 75,
remove the broad try/except that catches Exception and re-raises a RuntimeError
with a string (which hides original traceback); instead, let specific errors
bubble up to main(): delete the try: and except: blocks, unindent the enclosed
code so _check_docker_dir_exists(), the os.walk loop, the categories
accumulation and the return sorted(categories) execute normally, and do not
replace them with a generic catch — if you want to handle only particular cases,
catch and re-raise specific exception types with preserved chaining.
| def get_docker_hub_tags(self, image_name: str, limit: int = 10, architecture: str = "linux/amd64") -> list[dict[str, Any]]: | ||
| """Query Docker Hub for image tags with timestamp information.""" | ||
| # Parse repository name | ||
| if '/' in image_name: | ||
| namespace, repo = image_name.split('/', 1) | ||
| else: | ||
| namespace = 'library' # Official images are in the 'library' namespace | ||
| repo = image_name | ||
|
|
||
| url: str = f"https://hub.docker.com/v2/repositories/{namespace}/{repo}/tags?page_size=100" | ||
| try: | ||
| response = requests.get(url, timeout=30) | ||
| response.raise_for_status() | ||
| data = response.json() | ||
| tag_data: list[dict[str, Any]] = [] | ||
|
|
||
| for tag in data.get('results', []): | ||
| # Find the image info for the requested architecture | ||
| arch_digest = None | ||
| arch_os, arch_variant = self._parse_arch(architecture) | ||
| for image in tag.get('images', []): | ||
| if image.get('architecture') == arch_variant and image.get('os') == arch_os: | ||
| arch_digest = image.get('digest') | ||
| break | ||
|
|
||
| tag_data.append({ | ||
| 'name': tag['name'], | ||
| 'last_updated': tag.get('last_updated'), | ||
| 'size': tag.get('full_size', 0), | ||
| 'digest': arch_digest | ||
| }) | ||
|
|
||
| # Handle pagination if there are more tags | ||
| while 'next' in data and data['next'] and len(tag_data) < 1000: # Limit to avoid too many requests | ||
| response = requests.get(data['next'], timeout=30) | ||
| response.raise_for_status() | ||
| data = response.json() | ||
|
|
||
| for tag in data.get('results', []): | ||
| # Find the image info for the requested architecture | ||
| arch_digest = None | ||
| arch_os, arch_variant = self._parse_arch(architecture) | ||
| for image in tag.get('images', []): | ||
| if image.get('architecture') == arch_variant and image.get('os') == arch_os: | ||
| arch_digest = image.get('digest') | ||
| break | ||
|
|
||
| tag_data.append({ | ||
| 'name': tag['name'], | ||
| 'last_updated': tag.get('last_updated'), | ||
| 'size': tag.get('full_size', 0), | ||
| 'digest': arch_digest | ||
| }) | ||
|
|
||
| # Sort by last_updated in descending order (newest first) | ||
| def _iso(dt_str): | ||
| try: | ||
| return datetime.fromisoformat(dt_str.replace('Z', '+00:00')) | ||
| except ValueError: | ||
| return datetime.min | ||
|
|
||
| tag_data.sort(key=lambda x: _iso(x['last_updated']) if x['last_updated'] else datetime.min, reverse=True) | ||
| except requests.exceptions.RequestException as e: | ||
| print(f"Error querying Docker Hub: {e}", file=sys.stderr) | ||
| return [] | ||
| else: | ||
| return tag_data | ||
|
|
There was a problem hiding this comment.
Honor limit, reduce page size, reuse parsed arch, and use TIMEOUT for Docker Hub calls
Currently limit is unused (ARG002), page_size fixed at 100, arch parsing repeats, and timeout literals are used.
Apply this diff:
def get_docker_hub_tags(self, image_name: str, limit: int = 10, architecture: str = "linux/amd64") -> list[dict[str, Any]]:
"""Query Docker Hub for image tags with timestamp information."""
@@
- url: str = f"https://hub.docker.com/v2/repositories/{namespace}/{repo}/tags?page_size=100"
- try:
- response = requests.get(url, timeout=30)
+ page_size = max(1, min(100, int(limit)))
+ url: str = f"https://hub.docker.com/v2/repositories/{namespace}/{repo}/tags?page_size={page_size}"
+ tag_data: list[dict[str, Any]] = []
+ arch_os, arch_variant = self._parse_arch(architecture)
+ try:
+ response = requests.get(url, timeout=TIMEOUT)
response.raise_for_status()
data = response.json()
- tag_data: list[dict[str, Any]] = []
for tag in data.get('results', []):
# Find the image info for the requested architecture
arch_digest = None
- arch_os, arch_variant = self._parse_arch(architecture)
for image in tag.get('images', []):
if image.get('architecture') == arch_variant and image.get('os') == arch_os:
arch_digest = image.get('digest')
break
tag_data.append({
'name': tag['name'],
'last_updated': tag.get('last_updated'),
'size': tag.get('full_size', 0),
'digest': arch_digest
})
+ if len(tag_data) >= limit:
+ break
# Handle pagination if there are more tags
- while 'next' in data and data['next'] and len(tag_data) < 1000: # Limit to avoid too many requests
- response = requests.get(data['next'], timeout=30)
+ while 'next' in data and data['next'] and len(tag_data) < limit:
+ response = requests.get(data['next'], timeout=TIMEOUT)
response.raise_for_status()
data = response.json()
for tag in data.get('results', []):
# Find the image info for the requested architecture
arch_digest = None
- arch_os, arch_variant = self._parse_arch(architecture)
for image in tag.get('images', []):
if image.get('architecture') == arch_variant and image.get('os') == arch_os:
arch_digest = image.get('digest')
break
tag_data.append({
'name': tag['name'],
'last_updated': tag.get('last_updated'),
'size': tag.get('full_size', 0),
'digest': arch_digest
})
+ if len(tag_data) >= limit:
+ break
@@
- except requests.exceptions.RequestException as e:
- print(f"Error querying Docker Hub: {e}", file=sys.stderr)
- return []
- else:
- return tag_data
+ except requests.exceptions.RequestException as e:
+ print(f"Error querying Docker Hub: {e}", file=sys.stderr)
+ return []
+ else:
+ return tag_dataAs per coding guidelines
📝 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.
| def get_docker_hub_tags(self, image_name: str, limit: int = 10, architecture: str = "linux/amd64") -> list[dict[str, Any]]: | |
| """Query Docker Hub for image tags with timestamp information.""" | |
| # Parse repository name | |
| if '/' in image_name: | |
| namespace, repo = image_name.split('/', 1) | |
| else: | |
| namespace = 'library' # Official images are in the 'library' namespace | |
| repo = image_name | |
| url: str = f"https://hub.docker.com/v2/repositories/{namespace}/{repo}/tags?page_size=100" | |
| try: | |
| response = requests.get(url, timeout=30) | |
| response.raise_for_status() | |
| data = response.json() | |
| tag_data: list[dict[str, Any]] = [] | |
| for tag in data.get('results', []): | |
| # Find the image info for the requested architecture | |
| arch_digest = None | |
| arch_os, arch_variant = self._parse_arch(architecture) | |
| for image in tag.get('images', []): | |
| if image.get('architecture') == arch_variant and image.get('os') == arch_os: | |
| arch_digest = image.get('digest') | |
| break | |
| tag_data.append({ | |
| 'name': tag['name'], | |
| 'last_updated': tag.get('last_updated'), | |
| 'size': tag.get('full_size', 0), | |
| 'digest': arch_digest | |
| }) | |
| # Handle pagination if there are more tags | |
| while 'next' in data and data['next'] and len(tag_data) < 1000: # Limit to avoid too many requests | |
| response = requests.get(data['next'], timeout=30) | |
| response.raise_for_status() | |
| data = response.json() | |
| for tag in data.get('results', []): | |
| # Find the image info for the requested architecture | |
| arch_digest = None | |
| arch_os, arch_variant = self._parse_arch(architecture) | |
| for image in tag.get('images', []): | |
| if image.get('architecture') == arch_variant and image.get('os') == arch_os: | |
| arch_digest = image.get('digest') | |
| break | |
| tag_data.append({ | |
| 'name': tag['name'], | |
| 'last_updated': tag.get('last_updated'), | |
| 'size': tag.get('full_size', 0), | |
| 'digest': arch_digest | |
| }) | |
| # Sort by last_updated in descending order (newest first) | |
| def _iso(dt_str): | |
| try: | |
| return datetime.fromisoformat(dt_str.replace('Z', '+00:00')) | |
| except ValueError: | |
| return datetime.min | |
| tag_data.sort(key=lambda x: _iso(x['last_updated']) if x['last_updated'] else datetime.min, reverse=True) | |
| except requests.exceptions.RequestException as e: | |
| print(f"Error querying Docker Hub: {e}", file=sys.stderr) | |
| return [] | |
| else: | |
| return tag_data | |
| def get_docker_hub_tags( | |
| self, | |
| image_name: str, | |
| limit: int = 10, | |
| architecture: str = "linux/amd64", | |
| ) -> list[dict[str, Any]]: | |
| """Query Docker Hub for image tags with timestamp information.""" | |
| # Parse repository name | |
| if '/' in image_name: | |
| namespace, repo = image_name.split('/', 1) | |
| else: | |
| namespace = 'library' # Official images are in the 'library' namespace | |
| repo = image_name | |
| page_size = max(1, min(100, int(limit))) | |
| url: str = ( | |
| f"https://hub.docker.com/v2/repositories/" | |
| f"{namespace}/{repo}/tags?page_size={page_size}" | |
| ) | |
| tag_data: list[dict[str, Any]] = [] | |
| arch_os, arch_variant = self._parse_arch(architecture) | |
| try: | |
| response = requests.get(url, timeout=TIMEOUT) | |
| response.raise_for_status() | |
| data = response.json() | |
| for tag in data.get("results", []): | |
| # Find the image info for the requested architecture | |
| arch_digest = None | |
| for image in tag.get("images", []): | |
| if ( | |
| image.get("architecture") == arch_variant | |
| and image.get("os") == arch_os | |
| ): | |
| arch_digest = image.get("digest") | |
| break | |
| tag_data.append({ | |
| "name": tag["name"], | |
| "last_updated": tag.get("last_updated"), | |
| "size": tag.get("full_size", 0), | |
| "digest": arch_digest, | |
| }) | |
| if len(tag_data) >= limit: | |
| break | |
| # Handle pagination if there are more tags | |
| while data.get("next") and len(tag_data) < limit: | |
| response = requests.get(data["next"], timeout=TIMEOUT) | |
| response.raise_for_status() | |
| data = response.json() | |
| for tag in data.get("results", []): | |
| arch_digest = None | |
| for image in tag.get("images", []): | |
| if ( | |
| image.get("architecture") == arch_variant | |
| and image.get("os") == arch_os | |
| ): | |
| arch_digest = image.get("digest") | |
| break | |
| tag_data.append({ | |
| "name": tag["name"], | |
| "last_updated": tag.get("last_updated"), | |
| "size": tag.get("full_size", 0), | |
| "digest": arch_digest, | |
| }) | |
| if len(tag_data) >= limit: | |
| break | |
| # Sort by last_updated in descending order (newest first) | |
| def _iso(dt_str): | |
| try: | |
| return datetime.fromisoformat(dt_str.replace("Z", "+00:00")) | |
| except ValueError: | |
| return datetime.min | |
| tag_data.sort( | |
| key=lambda x: _iso(x["last_updated"]) if x["last_updated"] else datetime.min, | |
| reverse=True, | |
| ) | |
| except requests.exceptions.RequestException as e: | |
| print(f"Error querying Docker Hub: {e}", file=sys.stderr) | |
| return [] | |
| else: | |
| return tag_data |
🧰 Tools
🪛 Ruff (0.13.1)
38-38: Unused method argument: limit
(ARG002)
🤖 Prompt for AI Agents
In scripts/infra-mcp/tools/get_container_tags.py around lines 38 to 105, the
get_docker_hub_tags function ignores the limit parameter, hardcodes
page_size=100, reparses architecture for every tag, and uses literal timeouts;
fix by: 1) add or use a module-level TIMEOUT constant (replace timeout=30 with
TIMEOUT), 2) compute arch_os and arch_variant once before looping and reuse
them, 3) set a smaller page_size (e.g., min(50, limit) or configurable) in the
initial URL and subsequent page requests, 4) honor the limit by stopping when
len(tag_data) >= limit and trimming the returned list to limit, and 5) ensure
pagination requests also use the same page_size and TIMEOUT values.
ce47501 to
8e10340
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
scripts/infra-mcp/uv.lockis excluded by!**/*.lockscripts/task-mcp/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (22)
.claude/commands/plan-container-deployment.md(2 hunks).claude/settings.json(1 hunks).mcp.json(1 hunks).vscode/mcp.json(1 hunks)docker/guidelines.md(2 hunks)scripts/get-container-tags.py(0 hunks)scripts/infra-mcp/README.md(2 hunks)scripts/infra-mcp/pyproject.toml(1 hunks)scripts/infra-mcp/server.py(1 hunks)scripts/infra-mcp/start-server.sh(1 hunks)scripts/infra-mcp/tools/__init__.py(1 hunks)scripts/infra-mcp/tools/collections/__init__.py(1 hunks)scripts/infra-mcp/tools/collections/container_tools.py(1 hunks)scripts/infra-mcp/tools/collections/task_tools.py(1 hunks)scripts/infra-mcp/tools/get_app_icon.py(3 hunks)scripts/infra-mcp/tools/get_container_categories.py(1 hunks)scripts/infra-mcp/tools/get_container_tags.py(1 hunks)scripts/infra-mcp/tools/get_dashboard_groups.py(1 hunks)scripts/infra-mcp/utils/__init__.py(1 hunks)scripts/infra-mcp/utils/git.py(1 hunks)scripts/labctl.py(12 hunks)scripts/task-mcp/server.py(0 hunks)
💤 Files with no reviewable changes (2)
- scripts/get-container-tags.py
- scripts/task-mcp/server.py
🚧 Files skipped from review as they are similar to previous changes (8)
- scripts/infra-mcp/tools/collections/init.py
- scripts/infra-mcp/tools/init.py
- .mcp.json
- scripts/infra-mcp/tools/get_dashboard_groups.py
- .claude/settings.json
- scripts/infra-mcp/start-server.sh
- scripts/infra-mcp/utils/init.py
- .claude/commands/plan-container-deployment.md
🧰 Additional context used
📓 Path-based instructions (1)
scripts/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Python code must pass Ruff linting
Files:
scripts/infra-mcp/tools/collections/task_tools.pyscripts/labctl.pyscripts/infra-mcp/tools/get_app_icon.pyscripts/infra-mcp/tools/collections/container_tools.pyscripts/infra-mcp/utils/git.pyscripts/infra-mcp/tools/get_container_tags.pyscripts/infra-mcp/server.pyscripts/infra-mcp/tools/get_container_categories.py
🧠 Learnings (2)
📚 Learning: 2025-08-31T08:38:34.585Z
Learnt from: CR
PR: bubacoder/infra#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-31T08:38:34.585Z
Learning: Add new services to host configuration at config/docker/<hostname>/services.yaml
Applied to files:
docker/guidelines.md
📚 Learning: 2025-08-31T08:38:34.585Z
Learnt from: CR
PR: bubacoder/infra#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-31T08:38:34.585Z
Learning: Manage Docker services using the scripts/labctl.py tool (operations: up, down, restart, recreate, update, pull, config)
Applied to files:
scripts/labctl.py
🧬 Code graph analysis (7)
scripts/infra-mcp/tools/collections/task_tools.py (1)
scripts/task-mcp/server.py (4)
get_task_list(48-81)execute_task(84-104)task_fn(117-118)create_task_function(107-120)
scripts/infra-mcp/tools/get_app_icon.py (2)
scripts/task-mcp/tools/find_app_icon.py (8)
AppIconFinder(12-123)_find_dashboard_icon(51-72)get_app_icon(26-49)main(150-170)_find_favicon_url(74-123)test_icon_finder(126-147)__init__(18-24)get_priority(96-106)scripts/task-mcp/server.py (1)
find_app_icon(165-181)
scripts/infra-mcp/tools/collections/container_tools.py (1)
scripts/task-mcp/server.py (1)
control_container_service(124-161)
scripts/infra-mcp/utils/git.py (2)
scripts/task-mcp/server.py (1)
get_git_root(32-45)docs/web/update-docs.py (1)
get_git_root(368-375)
scripts/infra-mcp/tools/get_container_tags.py (1)
scripts/get-container-tags.py (10)
main(433-468)get_registry_tags(86-148)list_recent_tags(295-323)get_docker_hub_tags(18-83)get_most_specific_tag(383-430)list_same_hash_tags(326-380)get_image_tags(275-292)parse_image_reference(253-265)determine_tag_specificity(198-250)format_digest(179-187)
scripts/infra-mcp/server.py (7)
scripts/infra-mcp/tools/collections/container_tools.py (1)
add_container_operation_tools(105-128)scripts/infra-mcp/tools/collections/task_tools.py (1)
add_task_tools(107-131)scripts/infra-mcp/tools/get_app_icon.py (2)
get_app_icon(26-49)AppIconFinder(12-127)scripts/infra-mcp/tools/get_container_categories.py (2)
get_container_categories(38-74)ContainerCategoryFinder(16-74)scripts/infra-mcp/tools/get_container_tags.py (4)
ContainerTagFinder(12-454)get_image_tags(302-319)list_same_hash_tags(351-405)get_most_specific_tag(407-454)scripts/infra-mcp/tools/get_dashboard_groups.py (2)
get_dashboard_groups(30-50)DashboardGroupFinder(18-50)scripts/infra-mcp/utils/git.py (1)
get_git_root(9-32)
scripts/infra-mcp/tools/get_container_categories.py (2)
scripts/infra-mcp/utils/git.py (1)
get_git_root(9-32)scripts/infra-mcp/server.py (1)
get_container_categories(84-97)
🪛 Ruff (0.13.1)
scripts/labctl.py
170-170: Avoid specifying long messages outside the exception class
(TRY003)
scripts/infra-mcp/utils/git.py
20-20: Avoid specifying long messages outside the exception class
(TRY003)
29-29: Avoid specifying long messages outside the exception class
(TRY003)
31-31: Avoid specifying long messages outside the exception class
(TRY003)
scripts/infra-mcp/tools/get_container_tags.py
38-38: Unused method argument: limit
(ARG002)
106-106: Unused method argument: limit
(ARG002)
161-161: Do not catch blind exception: Exception
(BLE001)
184-184: Do not catch blind exception: Exception
(BLE001)
scripts/infra-mcp/server.py
232-232: Unused noqa directive (unused: BLE001)
Remove unused noqa directive
(RUF100)
scripts/infra-mcp/tools/get_container_categories.py
36-36: Avoid specifying long messages outside the exception class
(TRY003)
73-73: Do not catch blind exception: Exception
(BLE001)
74-74: Avoid specifying long messages outside the exception class
(TRY003)
74-74: Use explicit conversion flag
Replace with conversion flag
(RUF010)
🔇 Additional comments (8)
scripts/infra-mcp/utils/git.py (1)
19-31: Define dedicated Git exceptions to satisfy RuffLine 19 and line 29 trigger Ruff TRY003 because the long error strings live inline on
RuntimeError.scripts/**/*.pymust pass Ruff per coding guidelines, so this will keep failing CI until the messages move into exception classes. While you’re touching it, please capturestderrand lock inencodingso subprocess decoding is deterministic. Suggested fix:+class GitError(RuntimeError): + """Base class for git-related errors.""" + + +class GitNotFoundError(GitError): + """Raised when Git is not available on PATH.""" + + def __init__(self) -> None: + super().__init__("Git not found on PATH.") + + +class NotAGitRepositoryError(GitError): + """Raised when executed outside a Git repository.""" + + def __init__(self) -> None: + super().__init__("Not a Git repository.") + + def get_git_root() -> str: @@ - Raises: - RuntimeError: If git executable is not found or not in a git repository. + Raises: + GitNotFoundError: If git executable is not found. + NotAGitRepositoryError: If not run inside a git repository. @@ - if git_cmd is None: - raise RuntimeError("Git not found on PATH") from None + if git_cmd is None: + raise GitNotFoundError() from None try: result = subprocess.run( # noqa: S603 [git_cmd, "rev-parse", "--show-toplevel"], stdout=subprocess.PIPE, + stderr=subprocess.PIPE, check=True, - text=True, + text=True, + encoding="utf-8", ) except FileNotFoundError: - raise RuntimeError("Git executable not found. Please install Git and ensure it is on your PATH.") from None + raise GitNotFoundError() from None except subprocess.CalledProcessError as exc: - raise RuntimeError("Unable to locate git repository. Are you running this inside a Git repo?") from None + raise NotAGitRepositoryError() from excAs per coding guidelines.
scripts/infra-mcp/pyproject.toml (1)
9-10: Fix invalidbeautifulsoup4constraintLine 9 pins
beautifulsoup4>=4.13, but 4.13 hasn’t been published on PyPI, so resolution will fail. Please drop to the latest known release:- "beautifulsoup4>=4.13,<5", + "beautifulsoup4>=4.12.3,<5",This keeps the <5 guard while letting installers succeed. Based on static analysis hints.
scripts/infra-mcp/tools/get_container_tags.py (3)
181-185: Narrow the fallback exception in_format_datetimeWe’re still catching a blanket
Exceptionon Line 185, which hides unrelated bugs and triggers Ruff BLE001. Please only catch the parsing failures we expect.- except Exception: + except (TypeError, ValueError): return datetime_str
47-91: Honor thelimitparameter for Docker Hub paginationLine 38 declares
limit, yet Lines 47-91 ignore it and always walk up to 1000 items withpage_size=100. This reintroduces the over-fetch problem we called out earlier (Ruff still flags ARG002) and can hammer Docker Hub plus slow our MCP tool. Please cap the request size and stop oncelimititems are collected.- url: str = f"https://hub.docker.com/v2/repositories/{namespace}/{repo}/tags?page_size=100" - try: - response = requests.get(url, timeout=30) - response.raise_for_status() - data = response.json() - tag_data: list[dict[str, Any]] = [] - - for tag in data.get('results', []): - # Find the image info for the requested architecture - arch_digest = None - arch_os, arch_variant = self._parse_arch(architecture) - for image in tag.get('images', []): - if image.get('architecture') == arch_variant and image.get('os') == arch_os: - arch_digest = image.get('digest') - break - - tag_data.append({ - 'name': tag['name'], - 'last_updated': tag.get('last_updated'), - 'size': tag.get('full_size', 0), - 'digest': arch_digest - }) - - # Handle pagination if there are more tags - while 'next' in data and data['next'] and len(tag_data) < 1000: # Limit to avoid too many requests - response = requests.get(data['next'], timeout=30) - response.raise_for_status() - data = response.json() - - for tag in data.get('results', []): - # Find the image info for the requested architecture - arch_digest = None - arch_os, arch_variant = self._parse_arch(architecture) - for image in tag.get('images', []): - if image.get('architecture') == arch_variant and image.get('os') == arch_os: - arch_digest = image.get('digest') - break - - tag_data.append({ - 'name': tag['name'], - 'last_updated': tag.get('last_updated'), - 'size': tag.get('full_size', 0), - 'digest': arch_digest - }) + normalized_limit = max(1, int(limit)) + page_size = min(normalized_limit, 100) + arch_os, arch_variant = self._parse_arch(architecture) + next_url: str | None = f"https://hub.docker.com/v2/repositories/{namespace}/{repo}/tags?page_size={page_size}" + tag_data: list[dict[str, Any]] = [] + try: + while next_url and len(tag_data) < normalized_limit: + response = requests.get(next_url, timeout=30) + response.raise_for_status() + data = response.json() + for tag in data.get('results', []): + arch_digest = None + for image in tag.get('images', []): + if image.get('architecture') == arch_variant and image.get('os') == arch_os: + arch_digest = image.get('digest') + break + tag_data.append( + { + 'name': tag['name'], + 'last_updated': tag.get('last_updated'), + 'size': tag.get('full_size', 0), + 'digest': arch_digest, + } + ) + if len(tag_data) >= normalized_limit: + break + next_url = data.get('next')
117-155: Respectlimitin registry manifest lookupsSimilar to the Docker Hub path, Lines 117-155 still slice
tags[:100]and never short-circuit whenlimitis reached. Calls fromget_image_tagsnow over-fetch manifests and re-trigger Ruff ARG002. Please reuse the parsed architecture, bound the slice bylimit, and break as soon as we have enough entries.- tag_data: list[dict[str, Any]] = [] - for tag in tags[:100]: # Limit the number of additional requests + tag_data: list[dict[str, Any]] = [] + normalized_limit = max(1, int(limit)) + arch_os, arch_variant = self._parse_arch(architecture) + for tag in tags[: min(normalized_limit, 1000)]: manifest_url = f"{registry_url}/v2/{image_name}/manifests/{tag}" try: @@ - # For multi-arch images, we need to find the digest for the specific architecture - digest = None - # Try to parse the architecture from the manifest if it's a multi-arch image + digest = None if 'manifests' in manifest: for m in manifest.get('manifests', []): - if m.get('platform', {}).get('architecture') == architecture.split('/')[1] and \ - m.get('platform', {}).get('os') == architecture.split('/')[0]: + platform = m.get('platform', {}) + if platform.get('architecture') == arch_variant and platform.get('os') == arch_os: digest = m.get('digest') break @@ - tag_data.append({ - 'name': tag, - 'last_updated': last_modified, - 'digest': digest - }) + tag_data.append( + { + 'name': tag, + 'last_updated': last_modified, + 'digest': digest, + } + ) + if len(tag_data) >= normalized_limit: + break except requests.exceptions.RequestException: # If we can't get detailed info, just use the tag name - tag_data.append({ - 'name': tag, - 'last_updated': None, - 'digest': None - }) + tag_data.append( + { + 'name': tag, + 'last_updated': None, + 'digest': None, + } + ) + if len(tag_data) >= normalized_limit: + break + tag_data = tag_data[:normalized_limit]scripts/infra-mcp/tools/get_container_categories.py (1)
35-74: Replace broad exceptions so Ruff passes and errors remain actionableLines 35-74 still raise formatted
FileNotFoundErrormessages and wrap the entire walker inexcept Exception, both of which Ruff forbids (TRY003, BLE001). This also hides the original stack trace. Please introduce a typed exception and let real errors propagate.+class DockerDirNotFoundError(FileNotFoundError): + """Raised when the docker directory is missing.""" + + def __init__(self, path: Path) -> None: + super().__init__(f"Docker directory not found: {path}") + @@ - if not self.docker_path.exists() or not self.docker_path.is_dir(): - raise FileNotFoundError(f"Docker directory not found: {self.docker_path}") + if not self.docker_path.exists() or not self.docker_path.is_dir(): + raise DockerDirNotFoundError(self.docker_path) @@ - try: - # Ensure docker directory exists - self._check_docker_dir_exists() - - # Walk through the docker directory recursively - for root, _dirs, files in os.walk(self.docker_path): + # Ensure docker directory exists + self._check_docker_dir_exists() + + # Walk through the docker directory recursively + for root, _dirs, files in os.walk(self.docker_path): @@ - return sorted(categories) - except Exception as e: - raise RuntimeError(f"Error finding container categories: {str(e)}") from None + return sorted(categories)scripts/labctl.py (1)
168-198: Define a dedicated exception for missingdockerbinaryRaising
RuntimeError("Docker executable not found on PATH.")on Line 170 continues to violate Ruff TRY003 (per repository guidelines forscripts/**/*.py) and fails the lint gate we discussed earlier. Please introduce a typed exception and raise that instead.+class DockerNotFoundError(RuntimeError): + """Raised when the Docker executable is unavailable.""" + + def __init__(self) -> None: + super().__init__("Docker executable not found on PATH.") + @@ - if docker_bin is None: - raise RuntimeError("Docker executable not found on PATH.") from None + if docker_bin is None: + raise DockerNotFoundError() from Nonescripts/infra-mcp/server.py (1)
213-234: Remove unusednoqato satisfy Ruff.Ruff now flags the
# noqa: BLE001as unused (RUF100), so this file won’t pass the required lint gate forscripts/**/*.py. Please drop the directive so the guideline is satisfied.-except Exception: # noqa: BLE001 +except Exception: logger.exception("Failed to initialize server") sys.exit(1)As per coding guidelines
8e10340 to
0b17fdc
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
scripts/infra-mcp/tools/get_container_categories.py (1)
47-74: Drop the blanket exception wrapper inget_container_categoriesCatching
Exceptionand re-raising a freshRuntimeErrorhides the original stack and defeats the specificFileNotFoundErrorbranch inmain(). Let the real exception bubble (or catch only the cases you expect) so diagnostics stay intact and Ruff’s BLE001/TRY003 complaints go away.Apply this diff to remove the blanket wrap:
@@ - categories = [] - - try: - # Ensure docker directory exists - self._check_docker_dir_exists() - - # Walk through the docker directory recursively - for root, _dirs, files in os.walk(self.docker_path): - root_path = Path(root) - - # Check for README.md - readme_path = root_path.joinpath("README.md") - has_readme = readme_path.exists() and readme_path.is_file() - - # Check for any yaml files in the current directory - has_yaml = any(file.lower().endswith(('.yaml', '.yml')) for file in files) - - # Only include directories with both README.md and at least one yaml file - if has_readme and has_yaml: - # Get the path relative to the docker directory - rel_path = root_path.relative_to(self.docker_path) - # Convert to string and use forward slashes for consistency - rel_path_str = str(rel_path).replace('\\', '/') - # Add the directory to categories if it's not the root docker directory - if rel_path_str != '.': - categories.append(rel_path_str) - - return sorted(categories) - except Exception as e: - raise RuntimeError(f"Error finding container categories: {str(e)}") from None + categories = [] + self._check_docker_dir_exists() + + for root, _dirs, files in os.walk(self.docker_path): + root_path = Path(root) + has_readme = (root_path / "README.md").is_file() + has_yaml = any(file.lower().endswith(('.yaml', '.yml')) for file in files) + if has_readme and has_yaml: + rel_path_str = str(Path(root).relative_to(self.docker_path)).replace('\\', '/') + if rel_path_str != '.': + categories.append(rel_path_str) + + return sorted(categories)scripts/infra-mcp/server.py (1)
212-212: Use parameterized logging instead of f-strings.Replace f-string with parameterized logging for better performance and consistency.
- logger.info(f"Repository root path: {repository_root_path}") + logger.info("Repository root path: %s", repository_root_path)scripts/infra-mcp/tools/get_container_tags.py (1)
161-161: Narrow broad exception catching.Replace broad
Exceptioncatches with specific exception types for better error handling.def _httpdate(dt_str): try: return parsedate_to_datetime(dt_str) - except Exception: + except (TypeError, ValueError): return datetime.min # In _format_datetime method: try: dt = parsedate_to_datetime(datetime_str) return dt.strftime('%Y-%m-%d %H:%M:%S UTC') - except Exception: + except (TypeError, ValueError): return datetime_strAlso applies to: 184-184
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
scripts/infra-mcp/uv.lockis excluded by!**/*.lockscripts/task-mcp/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (23)
.claude/commands/plan-container-deployment.md(2 hunks).claude/settings.json(1 hunks).mcp.json(1 hunks).vscode/mcp.json(1 hunks)docker/guidelines.md(2 hunks)scripts/get-container-tags.py(0 hunks)scripts/infra-mcp/README.md(2 hunks)scripts/infra-mcp/pyproject.toml(1 hunks)scripts/infra-mcp/server.py(1 hunks)scripts/infra-mcp/start-server.sh(1 hunks)scripts/infra-mcp/tools/__init__.py(1 hunks)scripts/infra-mcp/tools/collections/__init__.py(1 hunks)scripts/infra-mcp/tools/collections/container_tools.py(1 hunks)scripts/infra-mcp/tools/collections/task_tools.py(1 hunks)scripts/infra-mcp/tools/get_app_icon.py(3 hunks)scripts/infra-mcp/tools/get_container_categories.py(1 hunks)scripts/infra-mcp/tools/get_container_tags.py(1 hunks)scripts/infra-mcp/tools/get_dashboard_groups.py(1 hunks)scripts/infra-mcp/utils/__init__.py(1 hunks)scripts/infra-mcp/utils/git.py(1 hunks)scripts/infra-mcp/utils/security.py(1 hunks)scripts/labctl.py(12 hunks)scripts/task-mcp/server.py(0 hunks)
💤 Files with no reviewable changes (2)
- scripts/task-mcp/server.py
- scripts/get-container-tags.py
🚧 Files skipped from review as they are similar to previous changes (5)
- scripts/infra-mcp/utils/init.py
- .claude/commands/plan-container-deployment.md
- scripts/infra-mcp/tools/init.py
- scripts/infra-mcp/pyproject.toml
- .vscode/mcp.json
🧰 Additional context used
📓 Path-based instructions (2)
scripts/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Python code must pass Ruff linting
Files:
scripts/infra-mcp/utils/security.pyscripts/infra-mcp/tools/get_container_categories.pyscripts/infra-mcp/tools/get_dashboard_groups.pyscripts/labctl.pyscripts/infra-mcp/utils/git.pyscripts/infra-mcp/tools/get_app_icon.pyscripts/infra-mcp/tools/collections/task_tools.pyscripts/infra-mcp/server.pyscripts/infra-mcp/tools/collections/__init__.pyscripts/infra-mcp/tools/get_container_tags.pyscripts/infra-mcp/tools/collections/container_tools.py
**/*.sh
📄 CodeRabbit inference engine (CLAUDE.md)
Shell scripts must pass ShellCheck
Files:
scripts/infra-mcp/start-server.sh
🧠 Learnings (2)
📚 Learning: 2025-08-31T08:38:34.585Z
Learnt from: CR
PR: bubacoder/infra#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-31T08:38:34.585Z
Learning: Manage Docker services using the scripts/labctl.py tool (operations: up, down, restart, recreate, update, pull, config)
Applied to files:
scripts/labctl.py
📚 Learning: 2025-08-31T08:38:34.585Z
Learnt from: CR
PR: bubacoder/infra#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-31T08:38:34.585Z
Learning: Add new services to host configuration at config/docker/<hostname>/services.yaml
Applied to files:
docker/guidelines.md
🧬 Code graph analysis (8)
scripts/infra-mcp/tools/get_container_categories.py (2)
scripts/infra-mcp/utils/git.py (1)
get_git_root(9-32)scripts/infra-mcp/server.py (1)
get_container_categories(82-95)
scripts/infra-mcp/tools/get_dashboard_groups.py (2)
scripts/infra-mcp/utils/git.py (1)
get_git_root(9-32)scripts/infra-mcp/server.py (1)
get_dashboard_groups(66-78)
scripts/labctl.py (1)
scripts/task-mcp/server.py (1)
control_container_service(124-161)
scripts/infra-mcp/utils/git.py (2)
scripts/task-mcp/server.py (1)
get_git_root(32-45)docs/web/update-docs.py (1)
get_git_root(368-375)
scripts/infra-mcp/tools/get_app_icon.py (2)
scripts/task-mcp/tools/find_app_icon.py (7)
AppIconFinder(12-123)_find_dashboard_icon(51-72)_find_favicon_url(74-123)get_app_icon(26-49)test_icon_finder(126-147)get_priority(96-106)__init__(18-24)scripts/task-mcp/server.py (1)
find_app_icon(165-181)
scripts/infra-mcp/tools/collections/task_tools.py (1)
scripts/task-mcp/server.py (4)
get_task_list(48-81)execute_task(84-104)create_task_function(107-120)task_fn(117-118)
scripts/infra-mcp/server.py (8)
scripts/infra-mcp/tools/collections/container_tools.py (1)
add_container_operation_tools(105-128)scripts/infra-mcp/tools/collections/task_tools.py (1)
add_task_tools(107-131)scripts/infra-mcp/tools/get_app_icon.py (2)
get_app_icon(26-49)AppIconFinder(12-127)scripts/infra-mcp/tools/get_container_categories.py (2)
get_container_categories(38-74)ContainerCategoryFinder(16-74)scripts/infra-mcp/tools/get_container_tags.py (4)
ContainerTagFinder(12-454)get_image_tags(302-319)list_same_hash_tags(351-405)get_most_specific_tag(407-454)scripts/infra-mcp/tools/get_dashboard_groups.py (2)
get_dashboard_groups(30-50)DashboardGroupFinder(18-50)scripts/infra-mcp/utils/git.py (1)
get_git_root(9-32)scripts/infra-mcp/utils/security.py (1)
validate_url_for_ssrf(12-53)
scripts/infra-mcp/tools/get_container_tags.py (1)
scripts/get-container-tags.py (10)
main(433-468)get_registry_tags(86-148)get_docker_hub_tags(18-83)list_recent_tags(295-323)get_most_specific_tag(383-430)list_same_hash_tags(326-380)get_image_tags(275-292)parse_image_reference(253-265)determine_tag_specificity(198-250)x(78-78)
🪛 Ruff (0.13.1)
scripts/infra-mcp/tools/get_container_categories.py
36-36: Avoid specifying long messages outside the exception class
(TRY003)
73-73: Do not catch blind exception: Exception
(BLE001)
74-74: Avoid specifying long messages outside the exception class
(TRY003)
74-74: Use explicit conversion flag
Replace with conversion flag
(RUF010)
scripts/labctl.py
170-170: Avoid specifying long messages outside the exception class
(TRY003)
scripts/infra-mcp/utils/git.py
20-20: Avoid specifying long messages outside the exception class
(TRY003)
29-29: Avoid specifying long messages outside the exception class
(TRY003)
31-31: Avoid specifying long messages outside the exception class
(TRY003)
scripts/infra-mcp/server.py
230-230: Unused noqa directive (unused: BLE001)
Remove unused noqa directive
(RUF100)
scripts/infra-mcp/tools/get_container_tags.py
38-38: Unused method argument: limit
(ARG002)
106-106: Unused method argument: limit
(ARG002)
161-161: Do not catch blind exception: Exception
(BLE001)
184-184: Do not catch blind exception: Exception
(BLE001)
⏰ 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 (22)
docker/guidelines.md (2)
173-177: Good call documenting PUID/PGID in the shared env list.
This explicitly surfaces the variables Compose needs and closes the gap that previously caused runtime failures when they were missing.
237-243: Template now enforces the required PUID/PGID placeholders.
Baking these into the service boilerplate keeps new stacks aligned with the host-permissions expectations and averts the missing-variable errors we saw before.scripts/infra-mcp/utils/git.py (1)
18-31: Address Ruff TRY003 violations and deterministic subprocess decoding.Ruff is still flagging TRY003 here because the long error strings are raised inline, and we’re also leaving subprocess decoding dependent on locale. This will break the required
scripts/**/*.pyRuff lint step and make stack traces less structured. Please move the messages into dedicated exception classes and setencoding="utf-8"(plusstderr=PIPE) so the output is stable.Apply this diff:
@@ -import shutil -import subprocess +import shutil +import subprocess + + +class GitError(RuntimeError): + """Base class for git-related errors.""" + + +class GitNotFoundError(GitError): + """Raised when Git is not available on PATH.""" + + def __init__(self) -> None: + super().__init__("Git not found on PATH") + + +class NotAGitRepositoryError(GitError): + """Raised when the current directory is outside a Git repository.""" + + def __init__(self) -> None: + super().__init__("Unable to locate git repository. Are you running this inside a Git repo?") @@ - if git_cmd is None: - raise RuntimeError("Git not found on PATH") from None + if git_cmd is None: + raise GitNotFoundError() from None try: result = subprocess.run( # noqa: S603 [git_cmd, "rev-parse", "--show-toplevel"], stdout=subprocess.PIPE, + stderr=subprocess.PIPE, check=True, - text=True, + text=True, + encoding="utf-8", ) except FileNotFoundError: - raise RuntimeError("Git executable not found. Please install Git and ensure it is on your PATH.") from None + raise GitNotFoundError() from None except subprocess.CalledProcessError: - raise RuntimeError("Unable to locate git repository. Are you running this inside a Git repo?") from None + raise NotAGitRepositoryError() from NoneAs per coding guidelines
scripts/labctl.py (5)
24-29: LGTM! Clean dataclass design.The
DockerOptionsdataclass provides a clean way to centralize and propagate Docker operation configuration across the toolchain.
49-69: LGTM! Robust network extraction with comprehensive parsing.The function handles multiple Docker Compose external network formats correctly, including boolean external flags, name overrides, and nested configurations.
174-198: LGTM! Proper quiet mode support in docker_pull.The function correctly accepts and propagates the quiet flag to both build and pull commands.
200-254: LGTM! Well-structured docker_command with proper options handling.The function creates networks before executing commands and properly propagates DockerOptions through all operation modes. The match statement provides clear handling of different actions.
168-171: Duplicate - Fix Ruff TRY003: raise a typed exception for missing Docker.This was already flagged in previous reviews. Define
DockerNotFoundErrorand use it here to avoid long messages outside exception classes.+class DockerNotFoundError(RuntimeError): + """Raised when Docker executable is not found on PATH.""" + def docker(cmd: list[str], env=None, stdin=None, stdout=None, stderr=None) -> None: """Execute a docker command with the given arguments. Args: cmd: List of command arguments to pass to docker env: Environment variables for the subprocess stdin: Standard input for the subprocess stdout: Standard output for the subprocess stderr: Standard error for the subprocess """ docker_bin = shutil.which("docker") if docker_bin is None: - raise RuntimeError("Docker executable not found on PATH.") from None + raise DockerNotFoundError() from None subprocess.run([docker_bin, *cmd], env=env, stdin=stdin, stdout=stdout, stderr=stderr, check=True) # noqa: S603scripts/infra-mcp/tools/collections/container_tools.py (4)
20-35: LGTM! Well-structured operation definitions.The operation list provides clear descriptions and covers all necessary Docker Compose operations.
79-102: LGTM! Clean function factory pattern.The closure creates operation-specific functions effectively for use with the MCP framework.
105-128: LGTM! Consistent MCP tool registration.The function follows the same pattern as other tool collections in the codebase and provides appropriate logging.
37-77: Duplicate - Validate operation and add path verification.This issue was already identified in previous reviews. The subprocess call needs input validation and path checking.
scripts/infra-mcp/server.py (5)
31-40: LGTM! Clean FastMCP server initialization.The server setup with health endpoint and proper naming follows MCP best practices.
42-63: LGTM! Proper SSRF protection in get_app_icon.The function correctly uses the security validation from
utils.securitybefore making external requests, which addresses SSRF concerns comprehensively.
65-95: LGTM! Clean tool implementations with proper error handling.All the tool functions follow a consistent pattern with try-catch blocks, proper logging, and safe default returns.
192-204: LGTM! Proper stdout redirection and variable reuse.The function correctly uses
contextlib.redirect_stdoutto prevent output leakage and captures the same-hash tags once for reuse, addressing previous performance concerns.
230-230: Remove unused noqa directive.The
# noqa: BLE001comment is not needed as the linter no longer flags this line.-except Exception: # noqa: BLE001 +except Exception: logger.exception("Failed to initialize server") sys.exit(1)scripts/infra-mcp/tools/get_container_tags.py (5)
12-22: LGTM! Clean class structure.The
ContainerTagFinderclass provides a well-organized interface for container tag operations.
214-272: LGTM! Well-implemented tag specificity algorithm.The
_determine_tag_specificitymethod provides a sophisticated scoring system for identifying the most useful version tags, with appropriate handling of semantic versioning patterns.
9-10: Add timeout constant for HTTP requests.Define a module-level timeout constant to use across all HTTP calls, addressing security concerns about hanging requests.
import requests + +# Timeout for HTTP requests (connect, read) in seconds +TIMEOUT = (5, 15)Based on learnings
38-105: Honor limit parameter and use TIMEOUT constant.The method currently ignores the
limitparameter and uses hardcoded timeouts, which can lead to performance issues and excessive API calls.def get_docker_hub_tags(self, image_name: str, limit: int = 10, architecture: str = "linux/amd64") -> list[dict[str, Any]]: """Query Docker Hub for image tags with timestamp information.""" # Parse repository name if '/' in image_name: namespace, repo = image_name.split('/', 1) else: namespace = 'library' # Official images are in the 'library' namespace repo = image_name - url: str = f"https://hub.docker.com/v2/repositories/{namespace}/{repo}/tags?page_size=100" + page_size = max(1, min(100, int(limit))) + url: str = f"https://hub.docker.com/v2/repositories/{namespace}/{repo}/tags?page_size={page_size}" + tag_data: list[dict[str, Any]] = [] + arch_os, arch_variant = self._parse_arch(architecture) try: - response = requests.get(url, timeout=30) + response = requests.get(url, timeout=TIMEOUT) response.raise_for_status() data = response.json() - tag_data: list[dict[str, Any]] = [] for tag in data.get('results', []): # Find the image info for the requested architecture arch_digest = None - arch_os, arch_variant = self._parse_arch(architecture) for image in tag.get('images', []): if image.get('architecture') == arch_variant and image.get('os') == arch_os: arch_digest = image.get('digest') break tag_data.append({ 'name': tag['name'], 'last_updated': tag.get('last_updated'), 'size': tag.get('full_size', 0), 'digest': arch_digest }) + if len(tag_data) >= limit: + break # Handle pagination if there are more tags - while 'next' in data and data['next'] and len(tag_data) < 1000: # Limit to avoid too many requests - response = requests.get(data['next'], timeout=30) + while 'next' in data and data['next'] and len(tag_data) < limit: + response = requests.get(data['next'], timeout=TIMEOUT) response.raise_for_status() data = response.json() for tag in data.get('results', []): # Find the image info for the requested architecture arch_digest = None - arch_os, arch_variant = self._parse_arch(architecture) for image in tag.get('images', []): if image.get('architecture') == arch_variant and image.get('os') == arch_os: arch_digest = image.get('digest') break tag_data.append({ 'name': tag['name'], 'last_updated': tag.get('last_updated'), 'size': tag.get('full_size', 0), 'digest': arch_digest }) + if len(tag_data) >= limit: + break
106-169: Honor limit parameter and improve architecture parsing for registry tags.Similar issues as Docker Hub method - unused limit parameter, hardcoded timeout, and repeated architecture parsing.
def get_registry_tags(self, registry_url: str, image_name: str, limit: int = 10, architecture: str = "linux/amd64") -> list[dict[str, Any]]: """Query a registry API v2 for image tags and attempt to get creation time.""" url: str = f"{registry_url}/v2/{image_name}/tags/list" + tag_data: list[dict[str, Any]] = [] + arch_os, arch_variant = self._parse_arch(architecture) try: - response = requests.get(url, timeout=30) + response = requests.get(url, timeout=TIMEOUT) response.raise_for_status() data = response.json() tags: list[str] = data.get('tags', []) # For Docker Registry API v2, we need to make additional requests to get manifest and timestamps - tag_data: list[dict[str, Any]] = [] - for tag in tags[:100]: # Limit the number of additional requests + for tag in tags[:max(0, min(limit, 1000))]: manifest_url = f"{registry_url}/v2/{image_name}/manifests/{tag}" try: # Try to get the manifest to extract creation time headers = {'Accept': 'application/vnd.docker.distribution.manifest.v2+json'} - manifest_response = requests.get(manifest_url, headers=headers, timeout=30) + manifest_response = requests.get(manifest_url, headers=headers, timeout=TIMEOUT) manifest_response.raise_for_status() # Get digest from the manifest manifest = manifest_response.json() # For multi-arch images, we need to find the digest for the specific architecture digest = None # Try to parse the architecture from the manifest if it's a multi-arch image if 'manifests' in manifest: for m in manifest.get('manifests', []): - if m.get('platform', {}).get('architecture') == architecture.split('/')[1] and \ - m.get('platform', {}).get('os') == architecture.split('/')[0]: + if m.get('platform', {}).get('architecture') == arch_variant and \ + m.get('platform', {}).get('os') == arch_os: digest = m.get('digest') break else: # If it's not a multi-arch manifest, just use the digest directly digest = manifest_response.headers.get('Docker-Content-Digest')
0b17fdc to
78411c6
Compare
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)
scripts/infra-mcp/utils/security.py (1)
64-76: Add explicit success returnIf every check passes we fall off the end of the function and implicitly return
None, which downstream code will treat as falsy and block all safe URLs. Add an explicitreturn Trueafter the loop.for resolved_ip in resolved_ips: if ( resolved_ip.is_loopback or resolved_ip.is_private or resolved_ip.is_link_local or resolved_ip.is_reserved or resolved_ip.is_multicast or resolved_ip.is_unspecified ): logger.warning("Blocked potentially unsafe URL=%r (private/reserved IP)", url) return False + + return True
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
scripts/infra-mcp/uv.lockis excluded by!**/*.lockscripts/task-mcp/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (29)
.claude/commands/fix-lint-issue.md(1 hunks).claude/commands/plan-container-deployment.md(2 hunks).claude/settings.json(1 hunks).mcp.json(1 hunks).vscode/mcp.json(1 hunks)AGENTS.md(1 hunks)Taskfile.yaml(1 hunks)docker/guidelines.md(2 hunks)docs/web/update-docs.py(3 hunks)scripts/get-container-tags.py(0 hunks)scripts/git-reorder-fixup.py(0 hunks)scripts/github-star-repo.py(1 hunks)scripts/infra-mcp/README.md(2 hunks)scripts/infra-mcp/pyproject.toml(1 hunks)scripts/infra-mcp/server.py(1 hunks)scripts/infra-mcp/start-server.sh(1 hunks)scripts/infra-mcp/tools/__init__.py(1 hunks)scripts/infra-mcp/tools/collections/__init__.py(1 hunks)scripts/infra-mcp/tools/collections/container_tools.py(1 hunks)scripts/infra-mcp/tools/collections/task_tools.py(1 hunks)scripts/infra-mcp/tools/get_app_icon.py(3 hunks)scripts/infra-mcp/tools/get_container_categories.py(1 hunks)scripts/infra-mcp/tools/get_container_tags.py(1 hunks)scripts/infra-mcp/tools/get_dashboard_groups.py(1 hunks)scripts/infra-mcp/utils/__init__.py(1 hunks)scripts/infra-mcp/utils/git.py(1 hunks)scripts/infra-mcp/utils/security.py(1 hunks)scripts/labctl.py(12 hunks)scripts/task-mcp/server.py(0 hunks)
💤 Files with no reviewable changes (3)
- scripts/task-mcp/server.py
- scripts/git-reorder-fixup.py
- scripts/get-container-tags.py
✅ Files skipped from review due to trivial changes (1)
- scripts/infra-mcp/tools/collections/init.py
🚧 Files skipped from review as they are similar to previous changes (8)
- .mcp.json
- scripts/github-star-repo.py
- scripts/infra-mcp/tools/init.py
- scripts/infra-mcp/utils/init.py
- .claude/settings.json
- .claude/commands/fix-lint-issue.md
- scripts/infra-mcp/start-server.sh
- Taskfile.yaml
🧰 Additional context used
📓 Path-based instructions (1)
scripts/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Python code must pass Ruff linting
Files:
scripts/infra-mcp/tools/get_app_icon.pyscripts/infra-mcp/tools/get_dashboard_groups.pyscripts/infra-mcp/tools/collections/task_tools.pyscripts/labctl.pyscripts/infra-mcp/utils/security.pyscripts/infra-mcp/tools/get_container_tags.pyscripts/infra-mcp/tools/collections/container_tools.pyscripts/infra-mcp/server.pyscripts/infra-mcp/tools/get_container_categories.pyscripts/infra-mcp/utils/git.py
🧠 Learnings (2)
📚 Learning: 2025-08-31T08:38:34.585Z
Learnt from: CR
PR: bubacoder/infra#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-31T08:38:34.585Z
Learning: Manage Docker services using the scripts/labctl.py tool (operations: up, down, restart, recreate, update, pull, config)
Applied to files:
scripts/labctl.py
📚 Learning: 2025-08-31T08:38:34.585Z
Learnt from: CR
PR: bubacoder/infra#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-31T08:38:34.585Z
Learning: Add new services to host configuration at config/docker/<hostname>/services.yaml
Applied to files:
docker/guidelines.md
🧬 Code graph analysis (8)
scripts/infra-mcp/tools/get_app_icon.py (2)
scripts/task-mcp/tools/find_app_icon.py (8)
AppIconFinder(12-123)_find_dashboard_icon(51-72)get_app_icon(26-49)main(150-170)_find_favicon_url(74-123)test_icon_finder(126-147)__init__(18-24)get_priority(96-106)scripts/task-mcp/server.py (1)
find_app_icon(165-181)
scripts/infra-mcp/tools/get_dashboard_groups.py (2)
scripts/infra-mcp/utils/git.py (1)
get_git_root(9-32)scripts/infra-mcp/server.py (1)
get_dashboard_groups(66-78)
scripts/infra-mcp/tools/collections/task_tools.py (1)
scripts/task-mcp/server.py (4)
get_task_list(48-81)execute_task(84-104)task_fn(117-118)create_task_function(107-120)
scripts/infra-mcp/tools/get_container_tags.py (3)
scripts/infra-mcp/tools/get_app_icon.py (1)
main(154-174)scripts/infra-mcp/tools/get_container_categories.py (1)
main(77-92)scripts/infra-mcp/tools/get_dashboard_groups.py (1)
main(53-71)
scripts/infra-mcp/tools/collections/container_tools.py (1)
scripts/task-mcp/server.py (1)
control_container_service(124-161)
scripts/infra-mcp/server.py (8)
scripts/infra-mcp/tools/collections/container_tools.py (1)
add_container_operation_tools(105-128)scripts/infra-mcp/tools/collections/task_tools.py (1)
add_task_tools(107-131)scripts/infra-mcp/tools/get_app_icon.py (2)
get_app_icon(26-49)AppIconFinder(12-127)scripts/infra-mcp/tools/get_container_categories.py (2)
get_container_categories(38-74)ContainerCategoryFinder(16-74)scripts/infra-mcp/tools/get_container_tags.py (4)
ContainerTagFinder(12-454)get_image_tags(302-319)list_same_hash_tags(351-405)get_most_specific_tag(407-454)scripts/infra-mcp/tools/get_dashboard_groups.py (2)
get_dashboard_groups(30-50)DashboardGroupFinder(18-50)scripts/infra-mcp/utils/git.py (1)
get_git_root(9-32)scripts/infra-mcp/utils/security.py (1)
validate_url_for_ssrf(13-75)
scripts/infra-mcp/tools/get_container_categories.py (2)
scripts/infra-mcp/utils/git.py (1)
get_git_root(9-32)scripts/infra-mcp/server.py (1)
get_container_categories(82-95)
scripts/infra-mcp/utils/git.py (1)
scripts/task-mcp/server.py (1)
get_git_root(32-45)
🪛 Ruff (0.13.1)
scripts/labctl.py
170-170: Avoid specifying long messages outside the exception class
(TRY003)
docs/web/update-docs.py
379-379: Avoid specifying long messages outside the exception class
(TRY003)
388-388: Avoid specifying long messages outside the exception class
(TRY003)
390-390: Avoid specifying long messages outside the exception class
(TRY003)
scripts/infra-mcp/tools/get_container_tags.py
38-38: Unused method argument: limit
(ARG002)
106-106: Unused method argument: limit
(ARG002)
161-161: Do not catch blind exception: Exception
(BLE001)
184-184: Do not catch blind exception: Exception
(BLE001)
scripts/infra-mcp/server.py
230-230: Unused noqa directive (unused: BLE001)
Remove unused noqa directive
(RUF100)
scripts/infra-mcp/tools/get_container_categories.py
36-36: Avoid specifying long messages outside the exception class
(TRY003)
73-73: Do not catch blind exception: Exception
(BLE001)
74-74: Avoid specifying long messages outside the exception class
(TRY003)
74-74: Use explicit conversion flag
Replace with conversion flag
(RUF010)
scripts/infra-mcp/utils/git.py
20-20: Avoid specifying long messages outside the exception class
(TRY003)
29-29: Avoid specifying long messages outside the exception class
(TRY003)
31-31: Avoid specifying long messages outside the exception class
(TRY003)
⏰ 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 (13)
AGENTS.md (1)
1-1: Fix heading to match filename.The document title still says “CLAUDE.md,” which conflicts with the file name AGENTS.md (same issue flagged in the earlier review). Please update the H1 to
# AGENTS.md.docker/guidelines.md (2)
173-177: Documenting host UID/GID resolves the compose preflight gap.Adding
PUIDandPGIDto the “common environment variables” list closes the loop for anyone following the templates—compose no longer fails on undefined IDs. Nice catch.
232-243: Template now mirrors the real-world PUID/PGID expectation.Including
PUID/PGIDright in the service scaffold keeps new definitions aligned with how the images expect ownership to be managed. Good improvement.scripts/infra-mcp/tools/get_app_icon.py (1)
67-74: Explicit fallback keeps flow predictableThanks for making the fallback explicit; it keeps the unhappy path obvious and prevents any reuse of stale responses if the GET probe fails.
scripts/infra-mcp/utils/git.py (1)
20-31: Fix Ruff TRY003 violations in git helperRuff still flags these inline
RuntimeErrormessages (TRY003). Becausescripts/**/*.pymust pass Ruff, this will block the merge. Please move the messages into small exception classes and raise those instead so Ruff is satisfied (and capture stderr deterministically). Example fix:+class GitError(RuntimeError): + """Base class for git-related errors.""" + + +class GitNotFoundError(GitError): + """Raised when Git is missing from PATH.""" + + def __init__(self) -> None: + super().__init__("Git not found on PATH") + + +class GitExecutableNotFoundError(GitError): + """Raised when the Git executable cannot be invoked.""" + + def __init__(self) -> None: + super().__init__("Git executable not found. Please install Git and ensure it is on your PATH.") + + +class NotAGitRepositoryError(GitError): + """Raised when not inside a Git repository.""" + + def __init__(self) -> None: + super().__init__("Unable to locate git repository. Are you running this inside a Git repo?") + + def get_git_root() -> str: @@ - if git_cmd is None: - raise RuntimeError("Git not found on PATH") from None + if git_cmd is None: + raise GitNotFoundError() from None @@ - result = subprocess.run( # noqa: S603 + result = subprocess.run( # noqa: S603 [git_cmd, "rev-parse", "--show-toplevel"], stdout=subprocess.PIPE, + stderr=subprocess.PIPE, check=True, - text=True, + text=True, + encoding="utf-8", ) except FileNotFoundError: - raise RuntimeError("Git executable not found. Please install Git and ensure it is on your PATH.") from None + raise GitExecutableNotFoundError() from None except subprocess.CalledProcessError: - raise RuntimeError("Unable to locate git repository. Are you running this inside a Git repo?") from None + raise NotAGitRepositoryError() from NoneAs per coding guidelines
scripts/labctl.py (1)
24-29: Introduce a typed exception to satisfy Ruff TRY003This block still raises
RuntimeError("Docker executable not found on PATH.")(Line 170). Ruff rule TRY003 (enforced forscripts/**/*.pyper coding guidelines) flags that message. The prior review already requested introducing a dedicated exception type; doing so remains necessary to keep the linter green. Please define a smallDockerNotFoundErrorclass and raise it here.@dataclass class DockerOptions: """Configuration options for Docker operations.""" pull_before_start: bool = False quiet: bool = False +class DockerNotFoundError(RuntimeError): + """Raised when the Docker executable is not available on PATH.""" + + def docker(cmd: list[str], env=None, stdin=None, stdout=None, stderr=None) -> None: @@ - if docker_bin is None: - raise RuntimeError("Docker executable not found on PATH.") from None + if docker_bin is None: + raise DockerNotFoundError() from NoneAs per coding guidelines
Also applies to: 168-171
scripts/infra-mcp/pyproject.toml (1)
5-5: Clarify the Python 3.13 floorPython 3.13 only went GA on October 7 2024 and the branch is still receiving frequent maintenance drops (latest 3.13.7 landed August 14 2025), so please confirm all deployment and CI environments are already pinned to ≥3.13 before we raise the floor—otherwise installs on the still-common 3.12/3.11 runtimes will break.(python.org)
scripts/infra-mcp/tools/collections/task_tools.py (1)
117-129: Prevent sanitized tool-name collisionsReplacing
:with--still collides whenever the Taskfile already contains a literal--(e.g.,deploy--prod) alongside a namespaced entry such asdeploy:prod; the later registration silently overwrites the earlier one, making a task unreachable through MCP. Please keep the execution name unchanged, generate a unique sanitized slug for the MCP tool (trackseennames and append a deterministic suffix or hash on collision), and keeptitleset to the human-readable original. A sketch:- for task_info in tasks: - task_name = task_info["name"] - tool_name = task_name.replace(":", "--") + seen_tool_names: set[str] = set() + for task_info in tasks: + task_name = task_info["name"] + base_tool_name = task_name.replace(":", "--") + tool_name = base_tool_name + suffix = 1 + while tool_name in seen_tool_names: + tool_name = f"{base_tool_name}--{suffix}" + suffix += 1 + seen_tool_names.add(tool_name) description = task_info["description"] task_fn = create_task_function(task_name, repository_root_path) tool = Tool.from_function( fn=task_fn, name=tool_name, - title=tool_name, + title=task_name, description=description )scripts/infra-mcp/tools/get_container_categories.py (2)
28-36: Define a dedicated DockerDirNotFoundError to satisfy Ruff TRY003Line [35] raises
FileNotFoundErrorwith a custom message, which triggers Ruff TRY003 forscripts/**/*.pyunder our coding guidelines. Please introduce a smallDockerDirNotFoundError(FileNotFoundError)next to this class and raise that instead so the descriptive message lives with the exception definition and the linter passes.+class DockerDirNotFoundError(FileNotFoundError): + """Raised when the docker directory is missing.""" + + def __init__(self, path: Path) -> None: + super().__init__(f"Docker directory not found: {path}") @@ - if not self.docker_path.exists() or not self.docker_path.is_dir(): - raise FileNotFoundError(f"Docker directory not found: {self.docker_path}") + if not self.docker_path.exists() or not self.docker_path.is_dir(): + raise DockerDirNotFoundError(self.docker_path)As per coding guidelines
47-74: Let specific errors bubble instead of re-wrapping every exceptionLines [47-74] wrap the entire method in
try/except Exception, convert everything into aRuntimeError, and even strip the traceback withfrom None. That violates Ruff BLE001/RUF010, hides useful diagnostics, and means callers can no longer catch the original exception type. Please drop this blanket catch (or catch only the concrete cases you need) and return the sorted list directly. For example:- try: - # Ensure docker directory exists - self._check_docker_dir_exists() - ... - return sorted(categories) - except Exception as e: - raise RuntimeError(f"Error finding container categories: {str(e)}") from None + self._check_docker_dir_exists() + ... + return sorted(categories)If you really need to add context for a narrow failure mode, catch that specific exception and re-raise it with
raiseto preserve the traceback. As per coding guidelinesscripts/infra-mcp/tools/get_container_tags.py (3)
184-185: Narrow the exception in_format_datetimeLine [184] still uses a bare
except Exception, which Ruff BLE001 rejects for scripts. Please catch the specific parsing errors instead:- except (TypeError, ValueError): - try: - dt = parsedate_to_datetime(datetime_str) - return dt.strftime('%Y-%m-%d %H:%M:%S UTC') - except Exception: - return datetime_str + except (TypeError, ValueError): + try: + dt = parsedate_to_datetime(datetime_str) + return dt.strftime('%Y-%m-%d %H:%M:%S UTC') + except (TypeError, ValueError): + return datetime_strThis keeps the graceful fallback without suppressing unrelated bugs.
47-104: Honor thelimitparameter when paging Docker Hub tagsLine [38] advertises a
limit, but the implementation always pulls up to 1,000 items (len(tag_data) < 1000) and never breaks early. That defeats the limit coming from the CLI/server, wastes network calls, and violates Ruff ARG002. Please respect the caller’s limit, reuse the parsed architecture once, and stop paginating once the limit is reached:- url: str = f"https://hub.docker.com/v2/repositories/{namespace}/{repo}/tags?page_size=100" - try: - response = requests.get(url, timeout=30) - response.raise_for_status() - data = response.json() - tag_data: list[dict[str, Any]] = [] - - for tag in data.get('results', []): + page_size = max(1, min(limit, 100)) if limit is not None else 100 + next_url = f"https://hub.docker.com/v2/repositories/{namespace}/{repo}/tags?page_size={page_size}" + tag_data: list[dict[str, Any]] = [] + arch_os, arch_variant = self._parse_arch(architecture) + try: + while next_url and (limit is None or len(tag_data) < limit): + response = requests.get(next_url, timeout=30) + response.raise_for_status() + data = response.json() + for tag in data.get('results', []): ... - tag_data.append({ + tag_data.append({ 'name': tag['name'], 'last_updated': tag.get('last_updated'), 'size': tag.get('full_size', 0), 'digest': arch_digest }) - - # Handle pagination if there are more tags - while 'next' in data and data['next'] and len(tag_data) < 1000: # Limit to avoid too many requests - response = requests.get(data['next'], timeout=30) - response.raise_for_status() - data = response.json() - - for tag in data.get('results', []): - ... - tag_data.append({ - ... - }) + if limit is not None and len(tag_data) >= limit: + break + next_url = data.get('next')
129-166: Registry path: avoid IndexError and respect the caller’slimitLines [133-134] call
architecture.split('/')twice, which crashes withIndexErrorif someone passes a single-token architecture (e.g.--architecture amd64). At the same time, the method ignoreslimitand iterates over a fixedtags[:100]. Please parse the architecture once via_parse_arch, slice using the supplied limit, and break pagination when the limit is hit:- tag_data: list[dict[str, Any]] = [] - for tag in tags[:100]: # Limit the number of additional requests + tag_data: list[dict[str, Any]] = [] + arch_os, arch_variant = self._parse_arch(architecture) + for tag in tags[: max(0, min(limit, 100))]: manifest_url = f"{registry_url}/v2/{image_name}/manifests/{tag}" try: headers = {'Accept': 'application/vnd.docker.distribution.manifest.v2+json'} manifest_response = requests.get(manifest_url, headers=headers, timeout=30) manifest_response.raise_for_status() @@ - if 'manifests' in manifest: - for m in manifest.get('manifests', []): - if m.get('platform', {}).get('architecture') == architecture.split('/')[1] and \ - m.get('platform', {}).get('os') == architecture.split('/')[0]: + if 'manifests' in manifest: + for m in manifest.get('manifests', []): + if m.get('platform', {}).get('architecture') == arch_variant and \ + m.get('platform', {}).get('os') == arch_os: digest = m.get('digest') break @@ - def _httpdate(dt_str): + def _httpdate(dt_str): try: return parsedate_to_datetime(dt_str) - except Exception: + except (TypeError, ValueError): return datetime.minAlso stop the loop after appending
limitentries to avoid hammering the registry unnecessarily.
78411c6 to
1cec378
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (9)
scripts/infra-mcp/tools/get_dashboard_groups.py (2)
52-52: Return groups in a stable orderSort keys for deterministic output across runs.
- return list(layout.keys()) + return sorted(layout.keys())
55-58: Docstring doesn’t match behaviorNo CLI args are parsed; adjust the docstring for accuracy.
- """ - Process command line arguments and run the application. - """ + """ + Run the application: read homepage settings and print dashboard groups. + """scripts/labctl.py (5)
49-69: Deduplicate discovered external networksIf multiple entries collapse to the same effective name, we’ll try to create the same network repeatedly. Return a de‑duplicated list while preserving order.
- return networks + # De-duplicate while preserving order + return list(dict.fromkeys(networks))
84-107: Tighten exception scope and make symlink update atomicCatching broad Exception hides unrelated issues. Also consider atomic replacement to avoid partial state on failures.
- try: - os.symlink(f"{hostname}/", localhost_link, target_is_directory=True) - except Exception: - logger.exception("Error creating localhost symlink") + try: + tmp_link = docker_config_dir / f".localhost.tmp.{os.getpid()}" + os.symlink(f"{hostname}/", tmp_link, target_is_directory=True) + os.replace(tmp_link, localhost_link) + except OSError: + logger.exception("Error creating localhost symlink")
219-221: Avoid creating networks for non-up actionsCreating external networks before pull/down/config is unnecessary work.
- # Ensure all required networks exist before executing any Docker Compose command - create_service_networks(compose_file) + # Ensure external networks exist only when (re)starting containers + if action in {"up", "recreate"}: + create_service_networks(compose_file)
262-264: Consider narrowing the exception when loading YAMLCatching Exception masks interrupts and unrelated errors; handle FileNotFoundError | yaml.YAMLError | OSError explicitly.
- except Exception: + except (FileNotFoundError, yaml.YAMLError, OSError): logger.exception(f"Error loading configuration file {config_file}") sys.exit(1)
267-276: Prefer named args when constructing DockerOptionsAvoids brittle coupling to dataclass field order.
- docker_command(host_config_dir, docker_stacks_dir / category, name, state, DockerOptions(pull_before_start, quiet)) + docker_command(host_config_dir, docker_stacks_dir / category, name, state, DockerOptions(pull_before_start=pull_before_start, quiet=quiet)) @@ - docker_command(get_host_config_dir(), docker_stacks_dir / category_path, service_name, args.operation, DockerOptions(args.pull_before_start, args.quiet)) + docker_command(get_host_config_dir(), docker_stacks_dir / category_path, service_name, args.operation, DockerOptions(pull_before_start=args.pull_before_start, quiet=args.quiet))Also applies to: 305-305, 350-350
scripts/infra-mcp/utils/security.py (2)
23-27: Optional: enforce a port policy.Consider rejecting sensitive ports (e.g., 22, 2375, 3389, 5900) or allowing only {80, 443} via a parameter (allowed_ports: set[int] | None).
If helpful, I can draft a minimal allowlist implementation wired into this function.
12-22: Usage note: re‑validate after redirects and at connect time.This guard runs pre‑request. Ensure the HTTP client disables auto‑redirects or re‑applies this check to each Location target to prevent DNS‑rebinding/redirect SSRF.
I can provide a requests.Session example that handles redirects manually and re‑checks each hop.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
scripts/infra-mcp/uv.lockis excluded by!**/*.lockscripts/task-mcp/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (34)
.claude/commands/fix-lint-issue.md(1 hunks).claude/commands/implement-container-deployment.md(1 hunks).claude/commands/plan-container-deployment.md(3 hunks).claude/settings.json(1 hunks).mcp.json(1 hunks).vscode/mcp.json(1 hunks)AGENTS.md(1 hunks)CLAUDE.md(0 hunks)CLAUDE.md(1 hunks)GEMINI.md(1 hunks)Taskfile.yaml(1 hunks)docker/guidelines.md(2 hunks)docs/web/update-docs.py(3 hunks)ruff.toml(2 hunks)scripts/get-container-tags.py(0 hunks)scripts/git-reorder-fixup.py(0 hunks)scripts/github-star-repo.py(1 hunks)scripts/infra-mcp/README.md(2 hunks)scripts/infra-mcp/pyproject.toml(1 hunks)scripts/infra-mcp/server.py(1 hunks)scripts/infra-mcp/start-server.sh(1 hunks)scripts/infra-mcp/tools/__init__.py(1 hunks)scripts/infra-mcp/tools/collections/__init__.py(1 hunks)scripts/infra-mcp/tools/collections/container_tools.py(1 hunks)scripts/infra-mcp/tools/collections/task_tools.py(1 hunks)scripts/infra-mcp/tools/get_app_icon.py(3 hunks)scripts/infra-mcp/tools/get_container_categories.py(1 hunks)scripts/infra-mcp/tools/get_container_tags.py(1 hunks)scripts/infra-mcp/tools/get_dashboard_groups.py(1 hunks)scripts/infra-mcp/utils/__init__.py(1 hunks)scripts/infra-mcp/utils/git.py(1 hunks)scripts/infra-mcp/utils/security.py(1 hunks)scripts/labctl.py(12 hunks)scripts/task-mcp/server.py(0 hunks)
💤 Files with no reviewable changes (3)
- scripts/git-reorder-fixup.py
- scripts/task-mcp/server.py
- scripts/get-container-tags.py
✅ Files skipped from review due to trivial changes (2)
- CLAUDE.md
- GEMINI.md
🚧 Files skipped from review as they are similar to previous changes (12)
- scripts/github-star-repo.py
- .claude/commands/fix-lint-issue.md
- scripts/infra-mcp/tools/get_app_icon.py
- .claude/settings.json
- scripts/infra-mcp/utils/init.py
- ruff.toml
- scripts/infra-mcp/tools/collections/container_tools.py
- scripts/infra-mcp/README.md
- .claude/commands/implement-container-deployment.md
- scripts/infra-mcp/tools/collections/task_tools.py
- CLAUDE.md
- .vscode/mcp.json
🧰 Additional context used
📓 Path-based instructions (3)
**/*.sh
📄 CodeRabbit inference engine (CLAUDE.md)
Shell scripts must pass ShellCheck
Files:
scripts/infra-mcp/start-server.sh
**/*.{yml,yaml}
📄 CodeRabbit inference engine (CLAUDE.md)
All YAML files must be linted and valid
Files:
Taskfile.yaml
scripts/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Python code must pass Ruff linting
Files:
scripts/infra-mcp/tools/__init__.pyscripts/infra-mcp/tools/get_dashboard_groups.pyscripts/infra-mcp/tools/collections/__init__.pyscripts/infra-mcp/utils/security.pyscripts/labctl.pyscripts/infra-mcp/tools/get_container_categories.pyscripts/infra-mcp/tools/get_container_tags.pyscripts/infra-mcp/server.pyscripts/infra-mcp/utils/git.py
🧠 Learnings (2)
📚 Learning: 2025-08-31T08:38:34.585Z
Learnt from: CR
PR: bubacoder/infra#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-31T08:38:34.585Z
Learning: Add new services to host configuration at config/docker/<hostname>/services.yaml
Applied to files:
docker/guidelines.md
📚 Learning: 2025-08-31T08:38:34.585Z
Learnt from: CR
PR: bubacoder/infra#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-31T08:38:34.585Z
Learning: Manage Docker services using the scripts/labctl.py tool (operations: up, down, restart, recreate, update, pull, config)
Applied to files:
scripts/labctl.py
🧬 Code graph analysis (5)
scripts/infra-mcp/tools/get_dashboard_groups.py (1)
scripts/infra-mcp/utils/git.py (1)
get_git_root(9-32)
scripts/infra-mcp/tools/get_container_categories.py (2)
scripts/infra-mcp/utils/git.py (1)
get_git_root(9-32)scripts/infra-mcp/server.py (1)
get_container_categories(82-95)
scripts/infra-mcp/tools/get_container_tags.py (1)
scripts/get-container-tags.py (8)
main(433-468)get_registry_tags(86-148)get_docker_hub_tags(18-83)get_most_specific_tag(383-430)list_recent_tags(295-323)list_same_hash_tags(326-380)get_image_tags(275-292)parse_image_reference(253-265)
scripts/infra-mcp/server.py (8)
scripts/infra-mcp/tools/collections/container_tools.py (1)
add_container_operation_tools(105-128)scripts/infra-mcp/tools/collections/task_tools.py (1)
add_task_tools(107-131)scripts/infra-mcp/tools/get_app_icon.py (2)
get_app_icon(26-49)AppIconFinder(12-127)scripts/infra-mcp/tools/get_container_categories.py (2)
get_container_categories(38-74)ContainerCategoryFinder(16-74)scripts/infra-mcp/tools/get_container_tags.py (3)
get_image_tags(302-319)list_same_hash_tags(351-405)get_most_specific_tag(407-454)scripts/infra-mcp/tools/get_dashboard_groups.py (2)
get_dashboard_groups(30-52)DashboardGroupFinder(18-52)scripts/infra-mcp/utils/git.py (1)
get_git_root(9-32)scripts/infra-mcp/utils/security.py (1)
validate_url_for_ssrf(13-77)
scripts/infra-mcp/utils/git.py (1)
scripts/task-mcp/server.py (1)
get_git_root(32-45)
🪛 Ruff (0.13.1)
docs/web/update-docs.py
379-379: Avoid specifying long messages outside the exception class
(TRY003)
388-388: Avoid specifying long messages outside the exception class
(TRY003)
390-390: Avoid specifying long messages outside the exception class
(TRY003)
scripts/infra-mcp/utils/security.py
13-13: Unused noqa directive (non-enabled: PLR0911)
Remove unused noqa directive
(RUF100)
scripts/labctl.py
170-170: Avoid specifying long messages outside the exception class
(TRY003)
scripts/infra-mcp/tools/get_container_categories.py
36-36: Avoid specifying long messages outside the exception class
(TRY003)
73-73: Do not catch blind exception: Exception
(BLE001)
74-74: Avoid specifying long messages outside the exception class
(TRY003)
74-74: Use explicit conversion flag
Replace with conversion flag
(RUF010)
scripts/infra-mcp/tools/get_container_tags.py
38-38: Unused method argument: limit
(ARG002)
106-106: Unused method argument: limit
(ARG002)
161-161: Do not catch blind exception: Exception
(BLE001)
184-184: Do not catch blind exception: Exception
(BLE001)
scripts/infra-mcp/server.py
230-230: Unused noqa directive (unused: BLE001)
Remove unused noqa directive
(RUF100)
scripts/infra-mcp/utils/git.py
20-20: Avoid specifying long messages outside the exception class
(TRY003)
29-29: Avoid specifying long messages outside the exception class
(TRY003)
31-31: Avoid specifying long messages outside the exception class
(TRY003)
⏰ 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 (17)
scripts/infra-mcp/pyproject.toml (1)
9-10: Fix the beautifulsoup4 version range
beautifulsoup4>=4.13will currently break installs: as of September 28, 2025 the latest release is 4.12.3, so the resolver can’t satisfy this lower bound. Adjust the constraint to target a real release (e.g. 4.12.3) while keeping the<5upper cap.- "beautifulsoup4>=4.13,<5", + "beautifulsoup4>=4.12.3,<5",scripts/infra-mcp/utils/git.py (1)
18-31: Fix Ruff TRY003 by moving error messages into exception classes.Inline
RuntimeErrormessages keep tripping Ruff TRY003, so this script still fails the required lint gate. Please move the messages into small exception classes and raise those instead.@@ -import shutil -import subprocess +import shutil +import subprocess + + +class GitError(RuntimeError): + """Base class for git-related errors.""" + + +class GitNotFoundError(GitError): + """Raised when Git is missing from PATH.""" + + def __init__(self) -> None: + super().__init__("Git not found on PATH") + + +class GitExecutableNotFoundError(GitError): + """Raised when the Git executable cannot be invoked.""" + + def __init__(self) -> None: + super().__init__("Git executable not found. Please install Git and ensure it is on your PATH.") + + +class NotAGitRepositoryError(GitError): + """Raised when not inside a Git repository.""" + + def __init__(self) -> None: + super().__init__("Unable to locate git repository. Are you running this inside a Git repo?") @@ - git_cmd = shutil.which("git") - if git_cmd is None: - raise RuntimeError("Git not found on PATH") from None + git_cmd = shutil.which("git") + if git_cmd is None: + raise GitNotFoundError() from None try: result = subprocess.run( # noqa: S603 [git_cmd, "rev-parse", "--show-toplevel"], stdout=subprocess.PIPE, + stderr=subprocess.PIPE, check=True, - text=True, + text=True, + encoding="utf-8", ) except FileNotFoundError: - raise RuntimeError("Git executable not found. Please install Git and ensure it is on your PATH.") from None + raise GitExecutableNotFoundError() from None except subprocess.CalledProcessError: - raise RuntimeError("Unable to locate git repository. Are you running this inside a Git repo?") from None + raise NotAGitRepositoryError() from NoneAs per coding guidelines
docs/web/update-docs.py (1)
368-390: Mirror the Ruff TRY003 fix forget_git_root.This script still raises the same inline
RuntimeErrorstrings, so Ruff TRY003 continues to fail. Please move the messages into dedicated exception classes and raise those instead.@@ -import shutil -import subprocess +import shutil +import subprocess @@ +class GitError(RuntimeError): + """Base class for git-related errors.""" + + +class GitNotFoundError(GitError): + """Raised when Git is missing from PATH.""" + + def __init__(self) -> None: + super().__init__("Git not found on PATH") + + +class GitExecutableNotFoundError(GitError): + """Raised when the Git executable cannot be invoked.""" + + def __init__(self) -> None: + super().__init__("Git executable not found. Please install Git and ensure it is on your PATH.") + + +class NotAGitRepositoryError(GitError): + """Raised when not inside a Git repository.""" + + def __init__(self) -> None: + super().__init__("Unable to locate git repository. Are you running this inside a Git repo?") + + def get_git_root() -> str: @@ - git_cmd = shutil.which("git") - if git_cmd is None: - raise RuntimeError("Git not found on PATH") from None + git_cmd = shutil.which("git") + if git_cmd is None: + raise GitNotFoundError() from None try: result = subprocess.run( # noqa: S603 [git_cmd, "rev-parse", "--show-toplevel"], stdout=subprocess.PIPE, + stderr=subprocess.PIPE, check=True, - text=True, + text=True, + encoding="utf-8", ) except FileNotFoundError: - raise RuntimeError("Git executable not found. Please install Git and ensure it is on your PATH.") from None + raise GitExecutableNotFoundError() from None except subprocess.CalledProcessError: - raise RuntimeError("Unable to locate git repository. Are you running this inside a Git repo?") from None + raise NotAGitRepositoryError() from NoneAs per coding guidelines
scripts/infra-mcp/tools/get_container_categories.py (2)
28-37: Raise a typed exception instead of inline message (TRY003); improves clarity and satisfies RuffDefine a small custom error type and use it here.
+class DockerDirNotFoundError(FileNotFoundError): + """Raised when the docker directory is missing.""" + def __init__(self, path: Path) -> None: + super().__init__(f"Docker directory not found: {path}") @@ - if not self.docker_path.exists() or not self.docker_path.is_dir(): - raise FileNotFoundError(f"Docker directory not found: {self.docker_path}") + if not self.docker_path.exists() or not self.docker_path.is_dir(): + raise DockerDirNotFoundError(self.docker_path)
47-75: Avoid broad except (BLE001) and message re-wrapping (TRY003/RUF010); let specific errors bubbleRemove the generic try/except and return directly. main() already handles FileNotFoundError/RuntimeError.
- try: - # Ensure docker directory exists - self._check_docker_dir_exists() - - # Walk through the docker directory recursively - for root, _dirs, files in os.walk(self.docker_path): - root_path = Path(root) - - # Check for README.md - readme_path = root_path.joinpath("README.md") - has_readme = readme_path.exists() and readme_path.is_file() - - # Check for any yaml files in the current directory - has_yaml = any(file.lower().endswith(('.yaml', '.yml')) for file in files) - - # Only include directories with both README.md and at least one yaml file - if has_readme and has_yaml: - # Get the path relative to the docker directory - rel_path = root_path.relative_to(self.docker_path) - # Convert to string and use forward slashes for consistency - rel_path_str = str(rel_path).replace('\\', '/') - # Add the directory to categories if it's not the root docker directory - if rel_path_str != '.': - categories.append(rel_path_str) - - return sorted(categories) - except Exception as e: - raise RuntimeError(f"Error finding container categories: {str(e)}") from None + # Ensure docker directory exists + self._check_docker_dir_exists() + # Walk through the docker directory recursively + for root, _dirs, files in os.walk(self.docker_path): + root_path = Path(root) + has_readme = (root_path / "README.md").is_file() + has_yaml = any(file.lower().endswith(('.yaml', '.yml')) for file in files) + if has_readme and has_yaml: + rel_path = root_path.relative_to(self.docker_path) + rel_path_str = str(rel_path).replace('\\', '/') + if rel_path_str != '.': + categories.append(rel_path_str) + return sorted(categories)scripts/infra-mcp/server.py (1)
209-232: Remove unused noqa and use parameterized loggingCleans Ruff RUF100 and standardizes logging style.
- repository_root_path = get_git_root() - logger.info(f"Repository root path: {repository_root_path}") + repository_root_path = get_git_root() + logger.info("Repository root path: %s", repository_root_path) @@ -except Exception: # noqa: BLE001 +except Exception: logger.exception("Failed to initialize server") sys.exit(1)scripts/infra-mcp/tools/get_container_tags.py (1)
171-186: Narrow broad exception in datetime formatter (BLE001)Catches only expected parse failures.
except (TypeError, ValueError): # Try to parse HTTP date format try: dt = parsedate_to_datetime(datetime_str) return dt.strftime('%Y-%m-%d %H:%M:%S UTC') - except Exception: + except (TypeError, ValueError): return datetime_strscripts/labctl.py (9)
24-29: Nice: centralized DockerOptionsClean way to thread pull/quiet through the flow.
111-121: Docstring/readability LGTM
124-131: Docstring/readability LGTM
174-183: Signature/doc updates LGTMQuiet mode plumbing is clear and consistent.
189-193: Compose quiet flags: supported; consider global --progress=quiet or graceful fallbackDocs show:
- docker compose build supports -q/--quiet. (docs.docker.com)
- docker compose pull supports -q/--quiet. (docs.docker.com)
So the earlier advice to drop --quiet for build looks out-of-date. Keep quiet, or alternatively prefer the global flag to consistently suppress progress (also with Bake): docker compose --progress quiet …. (docs.docker.com)
Option A (prefer global progress for consistency):
- build_cmd = ["compose", "-f", compose_file, *env_file_args, "build", "--pull"] - if quiet: - build_cmd.append("--quiet") + build_cmd = ["compose", "-f", compose_file, *env_file_args, "build", "--pull"] + if quiet: + build_cmd = ["compose", "--progress", "quiet", "-f", compose_file, *env_file_args, "build", "--pull"] @@ - pull_cmd = ["compose", "-f", compose_file, *env_file_args, "pull"] - if quiet: - pull_cmd.append("--quiet") + pull_cmd = ["compose", "-f", compose_file, *env_file_args, "pull"] + if quiet: + pull_cmd = ["compose", "--progress", "quiet", "-f", compose_file, *env_file_args, "pull", "--quiet"]Option B (graceful fallback for older Compose):
- docker(build_cmd, env=env) + try: + docker(build_cmd, env=env) + except subprocess.CalledProcessError as e: + if quiet and "--quiet" in build_cmd and e.returncode in (1, 125): + logger.info("compose build --quiet not supported; retrying without quiet") + docker([arg for arg in build_cmd if arg != "--quiet"], env=env) + else: + raiseRun this to confirm your CI/runtime Compose supports quiet:
#!/bin/bash set -euo pipefail docker compose version docker compose build --help | grep -E -- '--quiet|-q' || true docker compose pull --help | grep -E -- '--quiet|-q' || trueAlso applies to: 194-197
200-212: Good: options object replacing multiple paramsThis simplifies call sites and future expansion.
227-247: Pull-before-start flow LGTMThe sequencing for up/recreate with optional pull is correct.
366-366: CLI quiet flags LGTMFlags are wired through to DockerOptions correctly.
Also applies to: 373-373
158-171: Fix Ruff TRY003 and improve typing for docker()
- Raise a typed exception to satisfy TRY003 and convey intent.
- Broaden the type of cmd to accept PathLike since compose_file is a Path in call sites.
As per coding guidelines
+class DockerNotFoundError(RuntimeError): + """Raised when Docker executable is not found on PATH.""" + def __init__(self) -> None: + super().__init__("Docker executable not found on PATH.") + -from dataclasses import dataclass +from dataclasses import dataclass +from typing import Mapping, Sequence @@ -def docker(cmd: list[str], env=None, stdin=None, stdout=None, stderr=None) -> None: +def docker(cmd: Sequence[str | os.PathLike[str]], env: Mapping[str, str] | None = None, stdin=None, stdout=None, stderr=None) -> None: @@ - docker_bin = shutil.which("docker") - if docker_bin is None: - raise RuntimeError("Docker executable not found on PATH.") from None + docker_bin = shutil.which("docker") + if docker_bin is None: + raise DockerNotFoundError() from Nonescripts/infra-mcp/utils/security.py (1)
13-13: Remove unusednoqadirective-def validate_url_for_ssrf(url: str) -> bool: # noqa: PLR0911 +def validate_url_for_ssrf(url: str) -> bool:Verified Ruff flags RUF100 for this unused directive; removing it satisfies the linter.
| def get_registry_tags(self, registry_url: str, image_name: str, limit: int = 10, architecture: str = "linux/amd64") -> list[dict[str, Any]]: | ||
| """Query a registry API v2 for image tags and attempt to get creation time.""" | ||
| url: str = f"{registry_url}/v2/{image_name}/tags/list" | ||
| try: | ||
| response = requests.get(url, timeout=30) | ||
| response.raise_for_status() | ||
| data = response.json() | ||
| tags: list[str] = data.get('tags', []) | ||
|
|
||
| # For Docker Registry API v2, we need to make additional requests to get manifest and timestamps | ||
| tag_data: list[dict[str, Any]] = [] | ||
| for tag in tags[:100]: # Limit the number of additional requests | ||
| manifest_url = f"{registry_url}/v2/{image_name}/manifests/{tag}" | ||
| try: | ||
| # Try to get the manifest to extract creation time | ||
| headers = {'Accept': 'application/vnd.docker.distribution.manifest.v2+json'} | ||
| manifest_response = requests.get(manifest_url, headers=headers, timeout=30) | ||
| manifest_response.raise_for_status() |
There was a problem hiding this comment.
Registry path: respect limit, reuse parsed arch, and use TIMEOUT; narrow date parsing
Prevents over-fetch and addresses ARG002.
- try:
- response = requests.get(url, timeout=30)
+ try:
+ response = requests.get(url, timeout=TIMEOUT)
response.raise_for_status()
data = response.json()
tags: list[str] = data.get('tags', [])
# For Docker Registry API v2, we need to make additional requests to get manifest and timestamps
tag_data: list[dict[str, Any]] = []
- for tag in tags[:100]: # Limit the number of additional requests
+ arch_os, arch_variant = self._parse_arch(architecture)
+ for tag in tags[: max(0, min(limit, 1000))]:
manifest_url = f"{registry_url}/v2/{image_name}/manifests/{tag}"
try:
# Try to get the manifest to extract creation time
headers = {'Accept': 'application/vnd.docker.distribution.manifest.v2+json'}
- manifest_response = requests.get(manifest_url, headers=headers, timeout=30)
+ manifest_response = requests.get(manifest_url, headers=headers, timeout=TIMEOUT)
manifest_response.raise_for_status()
@@
- if 'manifests' in manifest:
+ if 'manifests' in manifest:
for m in manifest.get('manifests', []):
- if m.get('platform', {}).get('architecture') == architecture.split('/')[1] and \
- m.get('platform', {}).get('os') == architecture.split('/')[0]:
+ if m.get('platform', {}).get('architecture') == arch_variant and \
+ m.get('platform', {}).get('os') == arch_os:
digest = m.get('digest')
break
@@
- def _httpdate(dt_str):
+ def _httpdate(dt_str):
try:
return parsedate_to_datetime(dt_str)
- except Exception:
+ except (TypeError, ValueError):
return datetime.minAlso applies to: 128-136, 157-170
🧰 Tools
🪛 Ruff (0.13.1)
106-106: Unused method argument: limit
(ARG002)
1cec378 to
34171b5
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
scripts/infra-mcp/uv.lockis excluded by!**/*.lockscripts/task-mcp/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (24)
.claude/commands/implement-container-deployment.md(1 hunks).claude/commands/plan-container-deployment.md(3 hunks).claude/settings.json(1 hunks).mcp.json(1 hunks).vscode/mcp.json(1 hunks)docker/guidelines.md(2 hunks)scripts/get-container-tags.py(0 hunks)scripts/infra-mcp/README.md(2 hunks)scripts/infra-mcp/pyproject.toml(1 hunks)scripts/infra-mcp/server.py(1 hunks)scripts/infra-mcp/start-server.sh(1 hunks)scripts/infra-mcp/tools/__init__.py(1 hunks)scripts/infra-mcp/tools/collections/__init__.py(1 hunks)scripts/infra-mcp/tools/collections/container_tools.py(1 hunks)scripts/infra-mcp/tools/collections/task_tools.py(1 hunks)scripts/infra-mcp/tools/get_app_icon.py(3 hunks)scripts/infra-mcp/tools/get_container_categories.py(1 hunks)scripts/infra-mcp/tools/get_container_tags.py(1 hunks)scripts/infra-mcp/tools/get_dashboard_groups.py(1 hunks)scripts/infra-mcp/utils/__init__.py(1 hunks)scripts/infra-mcp/utils/git.py(1 hunks)scripts/infra-mcp/utils/security.py(1 hunks)scripts/labctl.py(12 hunks)scripts/task-mcp/server.py(0 hunks)
💤 Files with no reviewable changes (2)
- scripts/task-mcp/server.py
- scripts/get-container-tags.py
✅ Files skipped from review due to trivial changes (1)
- scripts/infra-mcp/start-server.sh
🚧 Files skipped from review as they are similar to previous changes (7)
- scripts/infra-mcp/utils/init.py
- .vscode/mcp.json
- scripts/infra-mcp/README.md
- docker/guidelines.md
- .mcp.json
- scripts/infra-mcp/tools/init.py
- scripts/infra-mcp/pyproject.toml
🧰 Additional context used
📓 Path-based instructions (1)
scripts/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Python code must pass Ruff linting
Files:
scripts/infra-mcp/utils/security.pyscripts/infra-mcp/server.pyscripts/infra-mcp/tools/get_app_icon.pyscripts/infra-mcp/tools/collections/task_tools.pyscripts/infra-mcp/tools/get_container_tags.pyscripts/infra-mcp/tools/collections/container_tools.pyscripts/infra-mcp/tools/get_dashboard_groups.pyscripts/infra-mcp/tools/collections/__init__.pyscripts/infra-mcp/utils/git.pyscripts/labctl.pyscripts/infra-mcp/tools/get_container_categories.py
🧠 Learnings (1)
📚 Learning: 2025-08-31T08:38:34.585Z
Learnt from: CR
PR: bubacoder/infra#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-31T08:38:34.585Z
Learning: Manage Docker services using the scripts/labctl.py tool (operations: up, down, restart, recreate, update, pull, config)
Applied to files:
scripts/labctl.py.claude/commands/implement-container-deployment.md
🧬 Code graph analysis (6)
scripts/infra-mcp/server.py (8)
scripts/infra-mcp/tools/collections/container_tools.py (1)
add_container_operation_tools(105-128)scripts/infra-mcp/tools/collections/task_tools.py (1)
add_task_tools(107-131)scripts/infra-mcp/tools/get_app_icon.py (2)
get_app_icon(26-49)AppIconFinder(12-127)scripts/infra-mcp/tools/get_container_categories.py (2)
get_container_categories(38-74)ContainerCategoryFinder(16-74)scripts/infra-mcp/tools/get_container_tags.py (3)
get_image_tags(302-319)list_same_hash_tags(351-405)get_most_specific_tag(407-454)scripts/infra-mcp/tools/get_dashboard_groups.py (2)
get_dashboard_groups(30-52)DashboardGroupFinder(18-52)scripts/infra-mcp/utils/git.py (1)
get_git_root(9-32)scripts/infra-mcp/utils/security.py (1)
validate_url_for_ssrf(13-77)
scripts/infra-mcp/tools/get_app_icon.py (1)
scripts/task-mcp/tools/find_app_icon.py (7)
AppIconFinder(12-123)_find_dashboard_icon(51-72)main(150-170)get_app_icon(26-49)_find_favicon_url(74-123)test_icon_finder(126-147)__init__(18-24)
scripts/infra-mcp/tools/collections/task_tools.py (1)
scripts/task-mcp/server.py (4)
get_task_list(48-81)execute_task(84-104)task_fn(117-118)create_task_function(107-120)
scripts/infra-mcp/tools/get_container_tags.py (1)
scripts/get-container-tags.py (9)
main(433-468)get_registry_tags(86-148)get_docker_hub_tags(18-83)get_most_specific_tag(383-430)list_recent_tags(295-323)list_same_hash_tags(326-380)get_image_tags(275-292)parse_image_reference(253-265)x(78-78)
scripts/infra-mcp/tools/get_dashboard_groups.py (1)
scripts/infra-mcp/utils/git.py (1)
get_git_root(9-32)
scripts/infra-mcp/tools/get_container_categories.py (2)
scripts/infra-mcp/utils/git.py (1)
get_git_root(9-32)scripts/infra-mcp/server.py (1)
get_container_categories(82-95)
🪛 Ruff (0.13.1)
scripts/infra-mcp/utils/security.py
13-13: Unused noqa directive (non-enabled: PLR0911)
Remove unused noqa directive
(RUF100)
scripts/infra-mcp/server.py
230-230: Unused noqa directive (unused: BLE001)
Remove unused noqa directive
(RUF100)
scripts/infra-mcp/tools/get_container_tags.py
38-38: Unused method argument: limit
(ARG002)
106-106: Unused method argument: limit
(ARG002)
161-161: Do not catch blind exception: Exception
(BLE001)
184-184: Do not catch blind exception: Exception
(BLE001)
scripts/infra-mcp/utils/git.py
20-20: Avoid specifying long messages outside the exception class
(TRY003)
29-29: Avoid specifying long messages outside the exception class
(TRY003)
31-31: Avoid specifying long messages outside the exception class
(TRY003)
scripts/labctl.py
171-171: Avoid specifying long messages outside the exception class
(TRY003)
scripts/infra-mcp/tools/get_container_categories.py
36-36: Avoid specifying long messages outside the exception class
(TRY003)
73-73: Do not catch blind exception: Exception
(BLE001)
74-74: Avoid specifying long messages outside the exception class
(TRY003)
74-74: Use explicit conversion flag
Replace with conversion flag
(RUF010)
⏰ 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 (12)
scripts/infra-mcp/utils/security.py (2)
25-75: Stop logging full URLs; scrub the messagesEvery warning here prints the raw URL, which can leak query secrets or credentials. Prior review already requested we log only safe components.
- logger.warning("Blocked potentially unsafe URL=%r (invalid scheme)", url) + logger.warning("Blocked URL (invalid scheme): scheme=%r", parsed.scheme) @@ - logger.warning("Blocked potentially unsafe URL=%r (missing hostname)", url) + logger.warning("Blocked URL (missing hostname)") @@ - if hostname in {"localhost", "127.0.0.1"}: - logger.warning("Blocked potentially unsafe URL=%r (localhost)", url) + if hostname in {"localhost", "127.0.0.1"}: + logger.warning("Blocked URL (localhost): host=%r", hostname) @@ - except socket.gaierror: - logger.warning("Blocked potentially unsafe URL=%r (unresolvable host)", url) + except socket.gaierror: + logger.warning("Blocked URL (unresolvable host): host=%r", hostname) return False @@ - if not resolved_ips: - logger.warning("Blocked potentially unsafe URL=%r (unresolvable host)", url) + if not resolved_ips: + logger.warning("Blocked URL (unresolvable host): host=%r", hostname) return False @@ - logger.warning("Blocked potentially unsafe URL=%r (private/reserved IP)", url) + logger.warning("Blocked URL (non-global IP): host=%r ip=%s", hostname, resolved_ip) return False
64-75: Harden the IP checks to block IPv4-mapped IPv6 and other non-global ranges
is_private/is_reservedmiss networks like::ffff:127.0.0.1, CGNAT (100.64.0.0/10), RFC 2544 ranges, etc., so attackers can still reach internal targets. Normalize IPv4-mapped IPv6 and reject anything that isn’tis_global, as requested earlier.- for resolved_ip in resolved_ips: - if ( - resolved_ip.is_loopback - or resolved_ip.is_private - or resolved_ip.is_link_local - or resolved_ip.is_reserved - or resolved_ip.is_multicast - or resolved_ip.is_unspecified - ): - logger.warning("Blocked URL (non-global IP): host=%r ip=%s", hostname, resolved_ip) - return False + for resolved_ip in resolved_ips: + ip_to_check = ( + resolved_ip.ipv4_mapped + if isinstance(resolved_ip, ipaddress.IPv6Address) and resolved_ip.ipv4_mapped + else resolved_ip + ) + if not ip_to_check.is_global: + logger.warning("Blocked URL (non-global IP): host=%r ip=%s", hostname, ip_to_check) + return Falsescripts/labctl.py (1)
190-198: Remove--quietfromdocker compose buildQuiet mode appends
--quietto the compose build command, butdocker compose build --quietisn’t supported (Docker exits with status 125), so any service with a build directive fails outright. Please drop the flag when constructingbuild_cmd; keep quiet mode only forcompose pull, which supports it.- build_cmd = ["compose", "-f", compose_file, *env_file_args, "build", "--pull"] - if quiet: - build_cmd.append("--quiet") - docker(build_cmd, env=env) + build_cmd = ["compose", "-f", compose_file, *env_file_args, "build", "--pull"] + docker(build_cmd, env=env)scripts/infra-mcp/tools/collections/task_tools.py (1)
6-129: Keep MCP tool names unique after sanitization.Replacing colons with double hyphens still collides when the Taskfile already exposes a
deploy--prodtask alongsidedeploy:prod; the latter silently overwrites the former, leaving one task unreachable. Please deduplicate the generated names (e.g., append a deterministic hash suffix on collision) while continuing to execute the original task name.-import logging -import re -import shutil -import subprocess +import hashlib +import logging +import re +import shutil +import subprocess @@ - for task_info in tasks: - task_name = task_info["name"] - tool_name = task_name.replace(":", "--") + seen_tool_names: set[str] = set() + for task_info in tasks: + task_name = task_info["name"] + tool_name = task_name.replace(":", "--") + if tool_name in seen_tool_names: + digest = hashlib.sha256(task_name.encode("utf-8")).hexdigest()[:8] + tool_name = f"{tool_name}-{digest}" + seen_tool_names.add(tool_name) description = task_info["description"] task_fn = create_task_function(task_name, repository_root_path)scripts/infra-mcp/tools/get_container_categories.py (2)
47-74: Drop the blanketexcept Exceptionwrapper.Catching
Exceptionand re-raising a freshRuntimeErrorboth obscures the underlying failure and violates Ruff BLE001/TRY003 for scripts/. Let the specific errors surface;main()already handlesFileNotFoundError, and unexpected exceptions should propagate with their original stack.As per coding guidelines
- try: - # Ensure docker directory exists - self._check_docker_dir_exists() - - # Walk through the docker directory recursively - for root, _dirs, files in os.walk(self.docker_path): - root_path = Path(root) - - # Check for README.md - readme_path = root_path.joinpath("README.md") - has_readme = readme_path.exists() and readme_path.is_file() - - # Check for any yaml files in the current directory - has_yaml = any(file.lower().endswith(('.yaml', '.yml')) for file in files) - - # Only include directories with both README.md and at least one yaml file - if has_readme and has_yaml: - # Get the path relative to the docker directory - rel_path = root_path.relative_to(self.docker_path) - # Convert to string and use forward slashes for consistency - rel_path_str = str(rel_path).replace('\\', '/') - # Add the directory to categories if it's not the root docker directory - if rel_path_str != '.': - categories.append(rel_path_str) - - return sorted(categories) - except Exception as e: - raise RuntimeError(f"Error finding container categories: {str(e)}") from None + # Ensure docker directory exists + self._check_docker_dir_exists() + + # Walk through the docker directory recursively + for root, _dirs, files in os.walk(self.docker_path): + root_path = Path(root) + + # Check for README.md + readme_path = root_path.joinpath("README.md") + has_readme = readme_path.exists() and readme_path.is_file() + + # Check for any yaml files in the current directory + has_yaml = any(file.lower().endswith(('.yaml', '.yml')) for file in files) + + # Only include directories with both README.md and at least one yaml file + if has_readme and has_yaml: + # Get the path relative to the docker directory + rel_path = root_path.relative_to(self.docker_path) + # Convert to string and use forward slashes for consistency + rel_path_str = str(rel_path).replace('\\', '/') + # Add the directory to categories if it's not the root docker directory + if rel_path_str != '.': + categories.append(rel_path_str) + + return sorted(categories)
28-36: Define a typed exception for missing docker directory.The inline message in this
FileNotFoundErrortriggers Ruff TRY003 on scripts/, so the lint gate will fail. Please move the message into a small dedicated exception class and raise that instead.As per coding guidelines
+class DockerDirNotFoundError(FileNotFoundError): + """Raised when the docker directory is missing.""" + + def __init__(self, path: Path) -> None: + super().__init__(f"Docker directory not found: {path}") @@ - if not self.docker_path.exists() or not self.docker_path.is_dir(): - raise FileNotFoundError(f"Docker directory not found: {self.docker_path}") + if not self.docker_path.exists() or not self.docker_path.is_dir(): + raise DockerDirNotFoundError(self.docker_path)scripts/infra-mcp/utils/git.py (1)
18-31: Replace inline RuntimeErrors with typed git exceptions.Ruff still flags these inline error strings (TRY003), and the subprocess call relies on locale-dependent defaults for decoding. Please introduce lightweight typed exceptions and opt into deterministic UTF-8 decoding so the script meets linting and error-handling expectations.
As per coding guidelines
+class GitError(RuntimeError): + """Base class for git-related errors.""" + + +class GitNotFoundError(GitError): + """Raised when Git is not available on PATH.""" + + def __init__(self) -> None: + super().__init__("Git not found on PATH") + + +class NotAGitRepositoryError(GitError): + """Raised when the current directory is not inside a Git repository.""" + + def __init__(self) -> None: + super().__init__("Unable to locate git repository. Are you running this inside a Git repo?") + + def get_git_root() -> str: @@ - git_cmd = shutil.which("git") - if git_cmd is None: - raise RuntimeError("Git not found on PATH") from None + git_cmd = shutil.which("git") + if git_cmd is None: + raise GitNotFoundError() from None try: result = subprocess.run( # noqa: S603 [git_cmd, "rev-parse", "--show-toplevel"], stdout=subprocess.PIPE, + stderr=subprocess.PIPE, check=True, - text=True, + encoding="utf-8", ) 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 + raise GitNotFoundError() from None + except subprocess.CalledProcessError as exc: + raise NotAGitRepositoryError() from excscripts/infra-mcp/server.py (1)
229-232: Drop the unusednoqa
except Exception: # noqa: BLE001no longer suppresses anything, so Ruff flags it with RUF100 (“unusednoqadirective”). Removing the tag is enough to get lint passing again.(docs.astral.sh)-except Exception: # noqa: BLE001 +except Exception: logger.exception("Failed to initialize server") sys.exit(1)scripts/infra-mcp/tools/collections/container_tools.py (1)
58-75: Handle missinglabctl.pyso the tool doesn’t crashIf the repo root is wrong or
labctl.pywas removed,subprocess.runraisesFileNotFoundErrorand the MCP tool bubbles an exception back to the client instead of returning a helpful message. Guarding the path up front (and catching the error) keeps the tool responsive even when the executable disappears.(codecalamity.com)@@ - cmd = [ - sys.executable, - os.path.join(repository_root_path, "scripts", "labctl.py"), + repo_path = os.path.abspath(repository_root_path) + labctl_path = os.path.join(repo_path, "scripts", "labctl.py") + if not os.path.isfile(labctl_path): + return f"labctl.py not found at: {labctl_path}" + + cmd = [ + sys.executable, + labctl_path, "service", operation, service_name ] @@ - except subprocess.CalledProcessError as e: + except FileNotFoundError: + return f"Executable not found while running: {' '.join(cmd)}" + except subprocess.CalledProcessError as e: return f"Error running operation: {e.stderr or str(e)}"scripts/infra-mcp/tools/get_container_tags.py (3)
171-185: Narrow the inner exception handlerCatching bare
Exceptionreintroduces Ruff BLE001 and makes real parsing bugs hard to spot. Restricting this to the specific date-parsing failures keeps the guard in place without swallowing unrelated errors.- except (TypeError, ValueError): + except (TypeError, ValueError): # Try to parse HTTP date format try: dt = parsedate_to_datetime(datetime_str) return dt.strftime('%Y-%m-%d %H:%M:%S UTC') - except Exception: + except (TypeError, ValueError): return datetime_str
47-104: Honorlimitwhen paging Docker Hub
limitis ignored today, so Ruff reports ARG002 and the code will happily fetch up to 1000 tags even when the caller only asked for a handful. Respecting the limit (and the API’spage_size<=100cap) keeps lint happy and avoids unnecessary requests.(stackoverflow.com)- url: str = f"https://hub.docker.com/v2/repositories/{namespace}/{repo}/tags?page_size=100" - try: - response = requests.get(url, timeout=30) - response.raise_for_status() - data = response.json() - tag_data: list[dict[str, Any]] = [] - - for tag in data.get('results', []): + limit = max(1, limit) + page_size = min(limit, 100) + url: str | None = f"https://hub.docker.com/v2/repositories/{namespace}/{repo}/tags?page_size={page_size}" + tag_data: list[dict[str, Any]] = [] + arch_os, arch_variant = self._parse_arch(architecture) + try: + while url and len(tag_data) < limit: + response = requests.get(url, timeout=30) + response.raise_for_status() + data = response.json() + + for tag in data.get('results', []): # Find the image info for the requested architecture arch_digest = None - arch_os, arch_variant = self._parse_arch(architecture) for image in tag.get('images', []): if image.get('architecture') == arch_variant and image.get('os') == arch_os: arch_digest = image.get('digest') break tag_data.append({ 'name': tag['name'], 'last_updated': tag.get('last_updated'), 'size': tag.get('full_size', 0), 'digest': arch_digest }) - # Handle pagination if there are more tags - while 'next' in data and data['next'] and len(tag_data) < 1000: # Limit to avoid too many requests - response = requests.get(data['next'], timeout=30) - response.raise_for_status() - data = response.json() - - for tag in data.get('results', []): - # Find the image info for the requested architecture - arch_digest = None - arch_os, arch_variant = self._parse_arch(architecture) - for image in tag.get('images', []): - if image.get('architecture') == arch_variant and image.get('os') == arch_os: - arch_digest = image.get('digest') - break - - tag_data.append({ - 'name': tag['name'], - 'last_updated': tag.get('last_updated'), - 'size': tag.get('full_size', 0), - 'digest': arch_digest - }) + if len(tag_data) >= limit: + break + + url = data.get('next') if len(tag_data) < limit else None # Sort by last_updated in descending order (newest first) def _iso(dt_str): try: return datetime.fromisoformat(dt_str.replace('Z', '+00:00')) except ValueError: return datetime.min tag_data.sort(key=lambda x: _iso(x['last_updated']) if x['last_updated'] else datetime.min, reverse=True) - except requests.exceptions.RequestException as e: - print(f"Error querying Docker Hub: {e}", file=sys.stderr) - return [] - else: - return tag_data + except requests.exceptions.RequestException as e: + print(f"Error querying Docker Hub: {e}", file=sys.stderr) + return [] + return tag_data
106-169: Apply the caller’s limit to registry v2 lookups
get_registry_tagsalso ignoreslimit, so it fires up to 100 extra manifest requests regardless of what the caller asked for. Counting requests toward the limit (and reusing the parsed architecture) fixes Ruff’s unused-argument warning and trims I/O.- url: str = f"{registry_url}/v2/{image_name}/tags/list" - try: - response = requests.get(url, timeout=30) + limit = max(1, limit) + url: str = f"{registry_url}/v2/{image_name}/tags/list" + try: + response = requests.get(url, timeout=30) response.raise_for_status() data = response.json() tags: list[str] = data.get('tags', []) # For Docker Registry API v2, we need to make additional requests to get manifest and timestamps tag_data: list[dict[str, Any]] = [] - for tag in tags[:100]: # Limit the number of additional requests + arch_os, arch_variant = self._parse_arch(architecture) + for tag in tags[:min(limit, 1000)]: # limit defensive fan-out manifest_url = f"{registry_url}/v2/{image_name}/manifests/{tag}" try: # Try to get the manifest to extract creation time headers = {'Accept': 'application/vnd.docker.distribution.manifest.v2+json'} manifest_response = requests.get(manifest_url, headers=headers, timeout=30) @@ tag_data.append({ 'name': tag, 'last_updated': last_modified, 'digest': digest }) + if len(tag_data) >= limit: + break # Sort by last_updated in descending order if available def _httpdate(dt_str): try: return parsedate_to_datetime(dt_str) except (TypeError, ValueError): return datetime.min tag_data.sort(key=lambda x: _httpdate(x['last_updated']) if x['last_updated'] else datetime.min, reverse=True) - except requests.exceptions.RequestException as e: - print(f"Error querying registry: {e}", file=sys.stderr) - return [] - else: - return tag_data + except requests.exceptions.RequestException as e: + print(f"Error querying registry: {e}", file=sys.stderr) + return [] + return tag_data
Summary by CodeRabbit