Cleanup code on infra-mcp - #285
Merged
Merged
Conversation
Contributor
WalkthroughAdds centralized constants and a dataclass, broadens Path typing and subprocess timeout usage across tools, enhances favicon and container-tag discovery with new helpers and timeouts, introduces env-driven logging and signal-based graceful shutdown in the server, and updates project metadata and dependencies. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
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/tools/collections/task_tools.py (1)
42-52:⚠️ Potential issue | 🟠 MajorMissing
subprocess.TimeoutExpiredhandler inget_task_list.Same issue as in
container_tools.py:TimeoutExpiredis not a subclass ofCalledProcessError. If thetask --list-allcommand hangs beyondTASK_COMMAND_TIMEOUT, the exception propagates unhandled.🐛 Proposed fix
except subprocess.CalledProcessError: logger.exception("Error getting task list") return [] + except subprocess.TimeoutExpired: + logger.error("Timed out getting task list after %s seconds", TASK_COMMAND_TIMEOUT) + return []🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/infra-mcp/tools/collections/task_tools.py` around lines 42 - 52, The get_task_list function currently only catches subprocess.CalledProcessError so a subprocess.TimeoutExpired will escape; update the try/except around the subprocess.run call (where task_bin and TASK_COMMAND_TIMEOUT are used) to also catch subprocess.TimeoutExpired and handle it the same way as CalledProcessError: log the exception via logger.exception (include context like "Error getting task list" or similar) and return an empty list so timeouts don't propagate.
🧹 Nitpick comments (6)
scripts/infra-mcp/tools/get_container_categories.py (1)
73-74:from Nonedrops the original exception chain, hindering diagnostics.
raise RuntimeError(...) from Nonesuppresses the original traceback entirely. If, for example,os.walkorrelative_toraises an unexpected exception, the caller only sees a genericRuntimeErrorwith no chain to the root cause. Usingfrom e(or omittingfrom None) preserves the context.🔧 Proposed fix
- raise RuntimeError(f"Error finding container categories: {str(e)}") from None + raise RuntimeError(f"Error finding container categories: {e}") from e🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/infra-mcp/tools/get_container_categories.py` around lines 73 - 74, The except block currently rethrows a RuntimeError using "from None", which discards the original exception chain; update the handler in the except Exception as e: block to preserve the original traceback by raising the RuntimeError using "from e" (or simply omit the "from" clause) so callers can see the underlying exception that caused the failure when functions like os.walk or Path.relative_to raise errors.scripts/infra-mcp/tools/get_app_icon.py (1)
123-144: Dedicated 5-second timeout for favicon.ico HEAD check is reasonable.Using a shorter timeout than
DEFAULT_REQUEST_TIMEOUTfor a simple existence check at a well-known path is a pragmatic choice. Consider defining this as a constant (e.g.,FAVICON_CHECK_TIMEOUT = 5) for visibility, but this is not blocking.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/infra-mcp/tools/get_app_icon.py` around lines 123 - 144, The _check_default_favicon method uses a hard-coded 5-second timeout; extract that literal into a clearly named module-level constant (e.g., FAVICON_CHECK_TIMEOUT = 5) and replace the literal in requests.head(timeout=...) with the constant; ensure the constant is defined near other module-level settings/imports so it's visible and can be reused or adjusted later.scripts/infra-mcp/tools/get_container_tags.py (2)
219-231: Docker Hub requests still use hardcodedtimeout=30instead ofREGISTRY_REQUEST_TIMEOUT.Lines 221 and 231 use
timeout=30whileget_registry_tags(line 319) and_fetch_manifest_for_tag(line 268) correctly use the imported constant. For consistency, replace the hardcoded values here as well.♻️ Proposed fix
- response = requests.get(url, timeout=30) + response = requests.get(url, timeout=REGISTRY_REQUEST_TIMEOUT) response.raise_for_status() data = response.json() ... while "next" in data and data["next"] and len(tag_data) < 1000: - response = requests.get(data["next"], timeout=30) + response = requests.get(data["next"], timeout=REGISTRY_REQUEST_TIMEOUT)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/infra-mcp/tools/get_container_tags.py` around lines 219 - 231, Replace the hardcoded timeout=30 in the Docker Hub requests within the tag-fetching loop with the imported REGISTRY_REQUEST_TIMEOUT constant: update both requests.get calls in the method that builds tag_data (the block that calls self._create_tag_data_dict and follows the "next" pagination loop) to use timeout=REGISTRY_REQUEST_TIMEOUT so it matches get_registry_tags and _fetch_manifest_for_tag.
11-17:MAX_TAGS_FETCH_LIMITis imported but never used in this file.The constant is imported on line 13 but is not referenced anywhere in the code. Line 230 hardcodes
1000for the Docker Hub pagination limit, which could use this constant. However, line 327 hardcodes100for a different purpose (limiting registry manifest fetches), so the two limits should not be conflated.Either remove the unused import or apply it only to the pagination limit on line 230:
♻️ Option 1: Remove unused import
- from ..utils.constants import MAX_TAGS_FETCH_LIMIT, REGISTRY_REQUEST_TIMEOUT + from ..utils.constants import REGISTRY_REQUEST_TIMEOUT except ImportError: # Fallback for standalone execution REGISTRY_REQUEST_TIMEOUT = 30 - MAX_TAGS_FETCH_LIMIT = 1000♻️ Option 2: Use the constant for pagination limit
- while "next" in data and data["next"] and len(tag_data) < 1000: # Limit to avoid too many requests + while "next" in data and data["next"] and len(tag_data) < MAX_TAGS_FETCH_LIMIT:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/infra-mcp/tools/get_container_tags.py` around lines 11 - 17, The MAX_TAGS_FETCH_LIMIT constant is imported but unused; replace the hardcoded Docker Hub pagination limit of 1000 with MAX_TAGS_FETCH_LIMIT in the Docker Hub pagination loop (the function/section that fetches Docker Hub tags) and keep the separate hardcoded 100 for registry manifest fetches (do not conflate limits), or if you prefer minimal change remove MAX_TAGS_FETCH_LIMIT from the import list so only REGISTRY_REQUEST_TIMEOUT is imported; update the Docker Hub pagination logic to use MAX_TAGS_FETCH_LIMIT when iterating pages (leave the manifest fetch limit unchanged) or delete the unused import if you choose not to apply it.scripts/infra-mcp/utils/models.py (1)
9-27: Defaults are duplicated fromconstants.pyinstead of imported.The
architectureandlimitdefaults duplicate values already defined inconstants.py(DEFAULT_CONTAINER_ARCHITECTUREandDEFAULT_TAG_LIMIT). Import and reference them to keep a single source of truth.Also,
ContainerTagFinder.get_image_tagsaccessesargs.sort(with agetattrfallback). Consider adding asortfield here for completeness, since this dataclass is the structured replacement forargparse.Namespace.♻️ Proposed fix
from dataclasses import dataclass + +from .constants import DEFAULT_CONTAINER_ARCHITECTURE, DEFAULT_TAG_LIMIT `@dataclass` class ContainerTagFinderArgs: ... image: str - architecture: str = "linux/amd64" - limit: int = 10 + architecture: str = DEFAULT_CONTAINER_ARCHITECTURE + limit: int = DEFAULT_TAG_LIMIT quiet: bool = True registry: str | None = None tag: str | None = None + sort: str = "version"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/infra-mcp/utils/models.py` around lines 9 - 27, The dataclass ContainerTagFinderArgs duplicates defaults from constants.py and lacks a sort attribute used by ContainerTagFinder.get_image_tags; update the class to import and use DEFAULT_CONTAINER_ARCHITECTURE and DEFAULT_TAG_LIMIT from constants (replace the literal "linux/amd64" and 10 with those constants) and add an optional sort: str | None = None field so getattr fallback is no longer required; ensure the top of the file imports the two constants and update any type hints/imports accordingly.scripts/infra-mcp/server.py (1)
209-223: Signal handlers and module-level side effects.
handle_shutdowncallssys.exit(0), which raisesSystemExit. This is fine for a top-level server entry point. However, note that signal registration on lines 222-223 happens at import time — ifserver.pyis ever imported in tests or by another module, these handlers will be installed as a side effect. This is consistent with the existing module-levelmcp = FastMCP(...)initialization, so flagging only as a note.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/infra-mcp/server.py` around lines 209 - 223, The module registers signal handlers at import time (signal.signal(...) for handle_shutdown), causing side effects when server.py is imported; move the registration out of module scope by creating and exporting a register_signal_handlers() function (or invoking registration inside an if __name__ == "__main__": block) and call that from the actual process entrypoint; keep handle_shutdown(signum, _frame) unchanged (but if needed perform cleanup before calling sys.exit(0)) and ensure any tests import server.py without triggering signal.signal registration.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/infra-mcp/server.py`:
- Around line 29-34: The code uses getattr(logging, LOG_LEVEL) which will raise
AttributeError for invalid strings; update the LOG_LEVEL handling before calling
logging.basicConfig to validate and safely resolve the level: call
logging.getLevelName(LOG_LEVEL) and if it returns an int use that, otherwise
fall back to logging.INFO (or use logging._nameToLevel.get(LOG_LEVEL.upper(),
logging.INFO)) and pass the resolved integer level into logging.basicConfig;
ensure symbols referenced are LOG_LEVEL, logging.getLevelName (or
logging._nameToLevel), and logging.basicConfig so the change is easy to locate.
In `@scripts/infra-mcp/tools/collections/container_tools.py`:
- Around line 75-86: The try/except around subprocess.run that uses
timeout=TASK_COMMAND_TIMEOUT only catches subprocess.CalledProcessError, so add
an additional except block for subprocess.TimeoutExpired to return a clear
timeout message; update the error handling near the subprocess.run call
(references: cmd, TASK_COMMAND_TIMEOUT, subprocess.run) to catch
subprocess.TimeoutExpired and return something like "Command timed out after
{TASK_COMMAND_TIMEOUT} seconds" (or include cmd) while keeping the existing
CalledProcessError handler and the final else returning result.stdout.
In `@scripts/infra-mcp/tools/collections/task_tools.py`:
- Around line 86-96: The execute_task call currently only catches
subprocess.CalledProcessError and will let subprocess.TimeoutExpired propagate;
add an except subprocess.TimeoutExpired as e: block (referencing
subprocess.TimeoutExpired, execute_task, TASK_COMMAND_TIMEOUT, task_bin,
task_name) that logs the timeout (use logger.exception or logger.error with
context) and returns a clear error string like "Task {task_name} timed out after
{TASK_COMMAND_TIMEOUT}s" including e.stdout/e.stderr if available; ensure this
handler sits alongside the existing CalledProcessError handler to prevent
uncaught timeouts.
- Line 129: The change to normalize tool names uses a single dash instead of the
previous double-dash, which can break external consumers; revert or make it
backward-compatible by restoring the original replacement (use tool_name =
task_name.replace(":", "--")) or add a compatibility/config option and mapping
so both "namespace--task" and "namespace-task" are supported; update the code
that sets tool_name (the task_name→tool_name transformation) accordingly and add
a comment describing the chosen behavior.
In `@scripts/infra-mcp/tools/get_container_categories.py`:
- Around line 8-13: The try/except currently catches ImportError which can mask
errors raised inside utils/git.py; change the except to catch
ModuleNotFoundError instead (or catch ModuleNotFoundError and re-raise any other
ImportError) so only a missing module triggers the sys.path fallback, and keep
the import of get_git_root from utils.git untouched.
- Around line 77-80: The docstring for main() is stale; replace it with a
concise, accurate description of what main() actually does (e.g., parses CLI
args, loads container/category data, and outputs container categories) so the
function header in get_container_categories.py correctly documents its behavior;
update the triple-quoted string for main() to reflect the real steps and
expected output rather than “read homepage settings and print dashboard groups.”
---
Outside diff comments:
In `@scripts/infra-mcp/tools/collections/task_tools.py`:
- Around line 42-52: The get_task_list function currently only catches
subprocess.CalledProcessError so a subprocess.TimeoutExpired will escape; update
the try/except around the subprocess.run call (where task_bin and
TASK_COMMAND_TIMEOUT are used) to also catch subprocess.TimeoutExpired and
handle it the same way as CalledProcessError: log the exception via
logger.exception (include context like "Error getting task list" or similar) and
return an empty list so timeouts don't propagate.
---
Nitpick comments:
In `@scripts/infra-mcp/server.py`:
- Around line 209-223: The module registers signal handlers at import time
(signal.signal(...) for handle_shutdown), causing side effects when server.py is
imported; move the registration out of module scope by creating and exporting a
register_signal_handlers() function (or invoking registration inside an if
__name__ == "__main__": block) and call that from the actual process entrypoint;
keep handle_shutdown(signum, _frame) unchanged (but if needed perform cleanup
before calling sys.exit(0)) and ensure any tests import server.py without
triggering signal.signal registration.
In `@scripts/infra-mcp/tools/get_app_icon.py`:
- Around line 123-144: The _check_default_favicon method uses a hard-coded
5-second timeout; extract that literal into a clearly named module-level
constant (e.g., FAVICON_CHECK_TIMEOUT = 5) and replace the literal in
requests.head(timeout=...) with the constant; ensure the constant is defined
near other module-level settings/imports so it's visible and can be reused or
adjusted later.
In `@scripts/infra-mcp/tools/get_container_categories.py`:
- Around line 73-74: The except block currently rethrows a RuntimeError using
"from None", which discards the original exception chain; update the handler in
the except Exception as e: block to preserve the original traceback by raising
the RuntimeError using "from e" (or simply omit the "from" clause) so callers
can see the underlying exception that caused the failure when functions like
os.walk or Path.relative_to raise errors.
In `@scripts/infra-mcp/tools/get_container_tags.py`:
- Around line 219-231: Replace the hardcoded timeout=30 in the Docker Hub
requests within the tag-fetching loop with the imported REGISTRY_REQUEST_TIMEOUT
constant: update both requests.get calls in the method that builds tag_data (the
block that calls self._create_tag_data_dict and follows the "next" pagination
loop) to use timeout=REGISTRY_REQUEST_TIMEOUT so it matches get_registry_tags
and _fetch_manifest_for_tag.
- Around line 11-17: The MAX_TAGS_FETCH_LIMIT constant is imported but unused;
replace the hardcoded Docker Hub pagination limit of 1000 with
MAX_TAGS_FETCH_LIMIT in the Docker Hub pagination loop (the function/section
that fetches Docker Hub tags) and keep the separate hardcoded 100 for registry
manifest fetches (do not conflate limits), or if you prefer minimal change
remove MAX_TAGS_FETCH_LIMIT from the import list so only
REGISTRY_REQUEST_TIMEOUT is imported; update the Docker Hub pagination logic to
use MAX_TAGS_FETCH_LIMIT when iterating pages (leave the manifest fetch limit
unchanged) or delete the unused import if you choose not to apply it.
In `@scripts/infra-mcp/utils/models.py`:
- Around line 9-27: The dataclass ContainerTagFinderArgs duplicates defaults
from constants.py and lacks a sort attribute used by
ContainerTagFinder.get_image_tags; update the class to import and use
DEFAULT_CONTAINER_ARCHITECTURE and DEFAULT_TAG_LIMIT from constants (replace the
literal "linux/amd64" and 10 with those constants) and add an optional sort: str
| None = None field so getattr fallback is no longer required; ensure the top of
the file imports the two constants and update any type hints/imports
accordingly.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.pre-commit-config.yaml:
- Around line 70-84: The commented-out pre-commit-terraform block (repo:
https://github.com/antonbabenko/pre-commit-terraform) must be either restored or
justified: re-enable the block and ensure hooks id: terraform_fmt,
terraform_docs, terraform_validate, and terraform_tflint (with args
["--args=--fix"] for terraform_tflint and terraform_docs config) are active so
terraform/ files are validated, or if temporarily disabled leave the block
commented but add an inline comment above it explaining the reason, link to a
tracking issue/PR, include expected re-enable date or condition, and the
maintainer responsible; ensure the comment mentions the specific hooks
(terraform_fmt, terraform_docs, terraform_validate, terraform_tflint) so
reviewers understand which checks are suppressed.
bubacoder
force-pushed
the
feature/infra-mcp-cleanup
branch
2 times, most recently
from
February 20, 2026 20:19
2f1b0fa to
de0d3f6
Compare
Contributor
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 (1)
scripts/infra-mcp/server.py (1)
107-113:⚠️ Potential issue | 🟡 MinorReference constant names in docstrings instead of hardcoded values.
The docstrings at lines 113 and 143 hardcode the numeric defaults
(default: 10)and(default: 100)while the function signatures useDEFAULT_TAG_LIMITandDEFAULT_SAME_HASH_LIMIT. If these constants are updated in the future, the docstrings will become outdated and misleading.Use the constant names in docstrings to keep them in sync with the actual defaults:
📝 Proposed fix
- limit: Maximum number of tags to display (default: 10) + limit: Maximum number of tags to display (default: DEFAULT_TAG_LIMIT)- limit: Maximum number of tags to search through (default: 100) + limit: Maximum number of tags to search through (default: DEFAULT_SAME_HASH_LIMIT)Also applies to: 136-144
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/infra-mcp/server.py` around lines 107 - 113, Update the docstrings to reference the constant names instead of hardcoded numeric defaults: replace "(default: 10)" in the docstring of list_container_tags with a reference to DEFAULT_TAG_LIMIT, and replace "(default: 100)" in the docstring of the other function (the one using DEFAULT_SAME_HASH_LIMIT) with a reference to DEFAULT_SAME_HASH_LIMIT; ensure both docstrings mention the constant names (DEFAULT_TAG_LIMIT, DEFAULT_SAME_HASH_LIMIT) so they remain accurate if the constants change and leave the function signatures unchanged.
🧹 Nitpick comments (6)
scripts/infra-mcp/pyproject.toml (1)
11-12: Consider bumping thepyyamllower bound to>=6.0.3.The current
beautifulsoup4>=4.13constraint is valid — 4.13.0 is the first release to include inline type annotations, and the latest stable release is 4.14.3. The constraint is a good fit for the PR's type-safety focus.The latest
pyyamlrelease is 6.0.3 (released Sep 25, 2025), while the lower bound is pinned to6.0.2. There are no known security vulnerabilities in 6.0.3. Bumping to>=6.0.3is a minor alignment with the current patch release, but>=6.0.2already resolves to it in a fresh install.🔧 Optional: align `pyyaml` lower bound with latest patch
- "pyyaml>=6.0.2,<7", + "pyyaml>=6.0.3,<7",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/infra-mcp/pyproject.toml` around lines 11 - 12, Update the pyyaml dependency constraint in pyproject.toml by changing the requirement string "pyyaml>=6.0.2,<7" to use a minimum of 6.0.3; locate the dependency entry for pyyaml in the scripts/infra-mcp pyproject.toml (the line containing "pyyaml>=6.0.2,<7") and bump the lower bound to "pyyaml>=6.0.3,<7" so future installs will prefer the latest 6.0.3 patch.scripts/infra-mcp/tools/get_dashboard_groups.py (1)
28-28: RedundantPath()wrapping onget_git_root()return value.Per
scripts/infra-mcp/utils/git.py,get_git_root()already returns aPath. The outerPath(...)call is a no-op.♻️ Suggested simplification
- self.repo_root = Path(get_git_root()) + self.repo_root = get_git_root()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/infra-mcp/tools/get_dashboard_groups.py` at line 28, The assignment to self.repo_root wraps get_git_root() with Path(...) redundantly since get_git_root() already returns a Path; change the code in the initializer to assign the returned Path directly (replace self.repo_root = Path(get_git_root()) with self.repo_root = get_git_root()), leaving any type hints unchanged and run tests/lint to confirm no other places expect a string.scripts/infra-mcp/tools/get_app_icon.py (2)
85-87: Inconsistent error surfacing between_find_dashboard_iconand_find_favicon_url.
_find_dashboard_iconswallowsRequestExceptionsilently, while_find_favicon_urlprints tostderr. Both are equally production-relevant. Consider logging tostderr(or usinglogging) consistently across both methods.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/infra-mcp/tools/get_app_icon.py` around lines 85 - 87, The two helper functions handle RequestException inconsistently: _find_dashboard_icon currently swallows requests.RequestException silently while _find_favicon_url writes the exception to stderr; make them consistent by capturing the exception in _find_dashboard_icon (except requests.RequestException as e) and either write a concise error message with the exception details to stderr or use the module logger to log the error (matching the style used in _find_favicon_url), ensuring the log includes the function name and the exception instance for diagnostic context.
137-137: Magic numbertimeout=5should be a named constant or accompanied by a comment.All other network calls use
DEFAULT_REQUEST_TIMEOUT; the deliberate choice of 5 s here for a lightweight existence check is non-obvious.♻️ Suggested improvement
+_FAVICON_HEAD_TIMEOUT = 5 # lightweight existence check; intentionally shorter than DEFAULT_REQUEST_TIMEOUT ... favicon_response = requests.head( default_favicon, headers=self.headers, - timeout=5, + timeout=_FAVICON_HEAD_TIMEOUT, allow_redirects=True, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/infra-mcp/tools/get_app_icon.py` at line 137, Replace the magic literal timeout=5 in get_app_icon.py with a named constant (e.g. EXISTENCE_CHECK_TIMEOUT = 5) or use the existing DEFAULT_REQUEST_TIMEOUT if appropriate; update the call that currently passes timeout=5 to reference that constant and add a one-line comment explaining why a shorter 5s timeout is chosen for a lightweight existence check so the intent matches other network calls that use DEFAULT_REQUEST_TIMEOUT.scripts/infra-mcp/server.py (2)
209-223: Signal handlers registered at module level instead of insidemain().
signal.signal(signal.SIGINT, handle_shutdown)andsignal.signal(signal.SIGTERM, handle_shutdown)execute at import time. This silently overrides any signal handling the caller has set up (e.g., pytest's SIGINT handler during testing). Moving the registration intomain()confines the side effect to actual server execution.♻️ Proposed refactor
-# Register signal handlers for graceful shutdown -signal.signal(signal.SIGINT, handle_shutdown) -signal.signal(signal.SIGTERM, handle_shutdown) - try: ... def main() -> None: """Start the MCP server.""" + # Register signal handlers for graceful shutdown + signal.signal(signal.SIGINT, handle_shutdown) + signal.signal(signal.SIGTERM, handle_shutdown) mcp.run()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/infra-mcp/server.py` around lines 209 - 223, The signal handlers are being registered at import time which overrides callers' handlers; move the registrations into the server's entrypoint so they only run during actual execution. Concretely, remove or comment out the module-level calls to signal.signal(SIGINT, handle_shutdown) and signal.signal(SIGTERM, handle_shutdown) and instead add those two registrations inside the main() function (or the function that starts the server) so handle_shutdown remains defined at module scope but is only hooked up when main() runs.
131-131: F-string logging inconsistent with existing%-style lazy formatting.Line 69 uses
%rlazy formatting:logger.exception("... app_name=%r ...", app_name, ...), but the new code on lines 131, 163, and 196 uses eager f-string evaluation:logger.exception(f"..."). For consistency, adopt the lazy formatting style used elsewhere in the file.♻️ Proposed fixes (consistent with line 69)
- logger.exception(f"list-container-tags failed for image={image!r}") + logger.exception("list-container-tags failed for image=%r", image)- logger.exception(f"list-same-hash-container-tags failed for image={image!r} tag={tag!r}") + logger.exception("list-same-hash-container-tags failed for image=%r tag=%r", image, tag)- logger.exception(f"get-most-specific-container-tag failed for image={image!r} tag={tag!r}") + logger.exception("get-most-specific-container-tag failed for image=%r tag=%r", image, tag)Also applies to: 163-163, 196-196
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/infra-mcp/server.py` at line 131, Replace eager f-string logging with the module's existing lazy %-style logging: change calls like logger.exception(f"list-container-tags failed for image={image!r}") to logger.exception("list-container-tags failed for image=%r", image) (and similarly for the other two logger.exception occurrences), i.e., use logger.exception("... %r", var) or "%s" as appropriate and pass the variable as an argument instead of interpolating with an f-string so logging stays consistent and lazy.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/infra-mcp/tools/get_app_icon.py`:
- Around line 12-16: The fallback for standalone execution doesn't catch the
actual exception raised for attempted relative imports; change the except clause
that currently catches ModuleNotFoundError to catch ImportError so the fallback
DEFAULT_REQUEST_TIMEOUT = 10 is applied when the relative import fails (around
the try importing DEFAULT_REQUEST_TIMEOUT from ..utils.constants and the
DEFAULT_REQUEST_TIMEOUT assignment).
In `@scripts/infra-mcp/tools/get_dashboard_groups.py`:
- Around line 9-11: The except block after the relative import in
get_dashboard_groups.py currently catches ModuleNotFoundError but a failed
relative import raises ImportError, so update the handler to catch ImportError
(or both ModuleNotFoundError and ImportError) and then perform the sys.path
adjustment and re-import; specifically modify the try/except around "from
..utils.git import get_git_root" to catch ImportError, append the appropriate
parent directory to sys.path, and then import get_git_root (or retry the import)
so standalone execution (python get_dashboard_groups.py) works.
---
Outside diff comments:
In `@scripts/infra-mcp/server.py`:
- Around line 107-113: Update the docstrings to reference the constant names
instead of hardcoded numeric defaults: replace "(default: 10)" in the docstring
of list_container_tags with a reference to DEFAULT_TAG_LIMIT, and replace
"(default: 100)" in the docstring of the other function (the one using
DEFAULT_SAME_HASH_LIMIT) with a reference to DEFAULT_SAME_HASH_LIMIT; ensure
both docstrings mention the constant names (DEFAULT_TAG_LIMIT,
DEFAULT_SAME_HASH_LIMIT) so they remain accurate if the constants change and
leave the function signatures unchanged.
---
Duplicate comments:
In `@scripts/infra-mcp/server.py`:
- Around line 29-34: LOG_LEVEL can be an invalid string (e.g. "VERBOSE") which
makes getattr(logging, LOG_LEVEL) raise AttributeError at import; change the
logging level lookup in the logging.basicConfig call to use a safe fallback
(e.g. use getattr(logging, LOG_LEVEL, logging.INFO) or use
logging._nameToLevel.get(LOG_LEVEL, logging.INFO)) so an unrecognized LOG_LEVEL
defaults to INFO instead of crashing; update the LOG_LEVEL lookup where
LOG_LEVEL is defined and where logging.basicConfig is called to use this
safe/default lookup.
In `@scripts/infra-mcp/tools/collections/container_tools.py`:
- Around line 75-86: The try/except in the subprocess invocation (in the
function containing this block, e.g., the command-run helper in
container_tools.py) only catches subprocess.CalledProcessError and thus lets
subprocess.TimeoutExpired propagate; update the exception handling to also catch
subprocess.TimeoutExpired (or SubprocessError) and return a clear message such
as "Command timed out" including any available output (e.stdout/e.stderr or
str(e)) so timeouts are handled similarly to non-zero exits; ensure you still
preserve the existing branch that returns result.stdout on success.
---
Nitpick comments:
In `@scripts/infra-mcp/pyproject.toml`:
- Around line 11-12: Update the pyyaml dependency constraint in pyproject.toml
by changing the requirement string "pyyaml>=6.0.2,<7" to use a minimum of 6.0.3;
locate the dependency entry for pyyaml in the scripts/infra-mcp pyproject.toml
(the line containing "pyyaml>=6.0.2,<7") and bump the lower bound to
"pyyaml>=6.0.3,<7" so future installs will prefer the latest 6.0.3 patch.
In `@scripts/infra-mcp/server.py`:
- Around line 209-223: The signal handlers are being registered at import time
which overrides callers' handlers; move the registrations into the server's
entrypoint so they only run during actual execution. Concretely, remove or
comment out the module-level calls to signal.signal(SIGINT, handle_shutdown) and
signal.signal(SIGTERM, handle_shutdown) and instead add those two registrations
inside the main() function (or the function that starts the server) so
handle_shutdown remains defined at module scope but is only hooked up when
main() runs.
- Line 131: Replace eager f-string logging with the module's existing lazy
%-style logging: change calls like logger.exception(f"list-container-tags failed
for image={image!r}") to logger.exception("list-container-tags failed for
image=%r", image) (and similarly for the other two logger.exception
occurrences), i.e., use logger.exception("... %r", var) or "%s" as appropriate
and pass the variable as an argument instead of interpolating with an f-string
so logging stays consistent and lazy.
In `@scripts/infra-mcp/tools/get_app_icon.py`:
- Around line 85-87: The two helper functions handle RequestException
inconsistently: _find_dashboard_icon currently swallows
requests.RequestException silently while _find_favicon_url writes the exception
to stderr; make them consistent by capturing the exception in
_find_dashboard_icon (except requests.RequestException as e) and either write a
concise error message with the exception details to stderr or use the module
logger to log the error (matching the style used in _find_favicon_url), ensuring
the log includes the function name and the exception instance for diagnostic
context.
- Line 137: Replace the magic literal timeout=5 in get_app_icon.py with a named
constant (e.g. EXISTENCE_CHECK_TIMEOUT = 5) or use the existing
DEFAULT_REQUEST_TIMEOUT if appropriate; update the call that currently passes
timeout=5 to reference that constant and add a one-line comment explaining why a
shorter 5s timeout is chosen for a lightweight existence check so the intent
matches other network calls that use DEFAULT_REQUEST_TIMEOUT.
In `@scripts/infra-mcp/tools/get_dashboard_groups.py`:
- Line 28: The assignment to self.repo_root wraps get_git_root() with Path(...)
redundantly since get_git_root() already returns a Path; change the code in the
initializer to assign the returned Path directly (replace self.repo_root =
Path(get_git_root()) with self.repo_root = get_git_root()), leaving any type
hints unchanged and run tests/lint to confirm no other places expect a string.
bubacoder
force-pushed
the
feature/infra-mcp-cleanup
branch
from
February 20, 2026 20:55
de0d3f6 to
c62aabb
Compare
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
scripts/infra-mcp/tools/get_container_tags.py (1)
219-231: 🛠️ Refactor suggestion | 🟠 MajorHard-coded
timeout=30and1000limit should use the imported constants.
REGISTRY_REQUEST_TIMEOUTandMAX_TAGS_FETCH_LIMITare imported (lines 13/16-17) but not used inget_docker_hub_tags. This undermines the purpose of centralizing these values.♻️ Suggested fix
- response = requests.get(url, timeout=30) + response = requests.get(url, timeout=REGISTRY_REQUEST_TIMEOUT) response.raise_for_status() data = response.json() tag_data: list[dict[str, Any]] = [] for tag in data.get("results", []): tag_data.append(self._create_tag_data_dict(tag, architecture)) # 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) < MAX_TAGS_FETCH_LIMIT: + response = requests.get(data["next"], timeout=REGISTRY_REQUEST_TIMEOUT)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/infra-mcp/tools/get_container_tags.py` around lines 219 - 231, Replace the hard-coded timeout and pagination limit in get_docker_hub_tags with the centralized constants: use REGISTRY_REQUEST_TIMEOUT instead of timeout=30 for all requests.get calls (including the initial call and the paginated loop) and use MAX_TAGS_FETCH_LIMIT instead of the literal 1000 when comparing len(tag_data) to stop pagination; keep using self._create_tag_data_dict for per-tag processing and ensure REGISTRY_REQUEST_TIMEOUT and MAX_TAGS_FETCH_LIMIT are referenced (they are already imported) so the function uses the shared configuration values.scripts/infra-mcp/tools/collections/task_tools.py (1)
42-52:⚠️ Potential issue | 🟠 MajorMissing
subprocess.TimeoutExpiredhandler inget_task_list.
timeout=TASK_COMMAND_TIMEOUTis set on thesubprocess.runcall, butTimeoutExpiredis not caught. If the task binary hangs, this exception will propagate unhandled.🐛 Proposed fix
except subprocess.CalledProcessError: logger.exception("Error getting task list") return [] + except subprocess.TimeoutExpired: + logger.error("Task list command timed out after %s seconds", TASK_COMMAND_TIMEOUT) + return []🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/infra-mcp/tools/collections/task_tools.py` around lines 42 - 52, The get_task_list function calls subprocess.run with timeout=TASK_COMMAND_TIMEOUT but only catches subprocess.CalledProcessError; add an except subprocess.TimeoutExpired block to handle timeouts from subprocess.run (the TASK_COMMAND_TIMEOUT case), log the timeout (include the exception details via logger.exception or logger.error with exc_info) and return an empty list like the CalledProcessError handler; ensure you reference subprocess.run, TASK_COMMAND_TIMEOUT, and get_task_list when making the change.
🧹 Nitpick comments (5)
scripts/infra-mcp/pyproject.toml (1)
11-12: Consider bumpingpyyamlminimum to the latest patch (6.0.3).The latest PyYAML version is 6.0.3, which is also the latest non-vulnerable version. Pinning the minimum to
6.0.2instead of6.0.3still resolves correctly at install time, but tightening the floor ensures no one inadvertently installs the older patch.For
beautifulsoup4, the current release series is 4.14.x (latest4.14.2/4.14.3), so>=4.13,<5is valid and will pull in the current stable release without any changes needed.⬆️ Bump pyyaml lower bound to latest patch
- "pyyaml>=6.0.2,<7", + "pyyaml>=6.0.3,<7",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/infra-mcp/pyproject.toml` around lines 11 - 12, Update the pyproject dependency entry for pyyaml: change the minimum version in the requirement string "pyyaml>=6.0.2,<7" to "pyyaml>=6.0.3,<7" so the package floor is bumped to the latest non-vulnerable patch while keeping the upper bound; locate the pyyaml line in pyproject.toml and replace the version specifier accordingly.scripts/infra-mcp/tools/get_app_icon.py (1)
133-138: Hard-codedtimeout=5— inconsistent withDEFAULT_REQUEST_TIMEOUTused elsewhere in this file.Lines 74, 82, and 165 all use
DEFAULT_REQUEST_TIMEOUT, but this HEAD request to/favicon.icouses a hard-coded 5-second timeout.♻️ Suggested fix
favicon_response = requests.head( default_favicon, headers=self.headers, - timeout=5, + timeout=DEFAULT_REQUEST_TIMEOUT, allow_redirects=True, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/infra-mcp/tools/get_app_icon.py` around lines 133 - 138, The HEAD request to default_favicon uses a hard-coded timeout=5; replace that with the shared DEFAULT_REQUEST_TIMEOUT constant to keep timeouts consistent (update the requests.head call in the same try block that references default_favicon to use DEFAULT_REQUEST_TIMEOUT instead of 5); ensure imports/namespace already provide DEFAULT_REQUEST_TIMEOUT as used in other calls in this module (lines that use DEFAULT_REQUEST_TIMEOUT) so the change matches existing usage.scripts/infra-mcp/tools/get_container_categories.py (1)
73-74:from Nonediscards the original exception chain.Using
from Nonehides the original traceback, which can make debugging harder. Considerfrom eto preserve the chain while still wrapping inRuntimeError.♻️ Suggested fix
- raise RuntimeError(f"Error finding container categories: {str(e)}") from None + raise RuntimeError(f"Error finding container categories: {str(e)}") from e🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/infra-mcp/tools/get_container_categories.py` around lines 73 - 74, The except block that wraps exceptions into a RuntimeError currently uses "from None", which suppresses the original traceback; in the except Exception as e: handler replace "from None" with "from e" (or remove the "from ..." clause) so the original exception chain is preserved when raising the RuntimeError in get_container_categories.py (the except block that raises RuntimeError(f"Error finding container categories: {str(e)}")).scripts/infra-mcp/server.py (1)
209-223: Signal-based graceful shutdown looks correct.
handle_shutdownmaps the signal number to a human-readable name and exits cleanly. Registering at module level is appropriate for a server entry point.One minor consideration:
sys.exit(0)inside a signal handler raisesSystemExit. If this module is imported by another module (e.g., tests), the signal registration at lines 222-223 will execute at import time, which could be surprising. Consider guarding withif __name__ == "__main__"or moving registration intomain().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/infra-mcp/server.py` around lines 209 - 223, The signal registration is happening at import time; keep the handle_shutdown function but move the two signal.signal(...) registrations out of module top-level and into the process entry path — either wrap them in an if __name__ == "__main__": block or register inside the existing main() function (or a dedicated setup_signal_handlers() helper) and call it from main(), ensuring handle_shutdown remains available but registrations only occur when the module is run as a program, not when imported.scripts/infra-mcp/tools/get_container_tags.py (1)
326-329: Hard-codedtags[:100]limit — consider using a constant or thelimitparameter.This magic number should either reference a constant from
constants.pyor use thelimitparameter already passed intoget_registry_tags.♻️ Suggested fix
# Limit the number of additional requests - for tag in tags[:100]: + for tag in tags[:limit]:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/infra-mcp/tools/get_container_tags.py` around lines 326 - 329, The loop currently hard-caps additional requests with the magic number tags[:100]; update get_registry_tags to honor the provided limit parameter (or replace 100 with a named constant from constants.py) so the slice becomes tags[:limit] or tags[:TAG_FETCH_LIMIT], and ensure tag fetching still calls _fetch_manifest_for_tag(registry_url, image_name, tag, architecture) and appends to tag_data accordingly; modify any defaulting logic so limit is validated (e.g., positive int) before slicing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@scripts/infra-mcp/tools/collections/task_tools.py`:
- Around line 42-52: The get_task_list function calls subprocess.run with
timeout=TASK_COMMAND_TIMEOUT but only catches subprocess.CalledProcessError; add
an except subprocess.TimeoutExpired block to handle timeouts from subprocess.run
(the TASK_COMMAND_TIMEOUT case), log the timeout (include the exception details
via logger.exception or logger.error with exc_info) and return an empty list
like the CalledProcessError handler; ensure you reference subprocess.run,
TASK_COMMAND_TIMEOUT, and get_task_list when making the change.
In `@scripts/infra-mcp/tools/get_container_tags.py`:
- Around line 219-231: Replace the hard-coded timeout and pagination limit in
get_docker_hub_tags with the centralized constants: use REGISTRY_REQUEST_TIMEOUT
instead of timeout=30 for all requests.get calls (including the initial call and
the paginated loop) and use MAX_TAGS_FETCH_LIMIT instead of the literal 1000
when comparing len(tag_data) to stop pagination; keep using
self._create_tag_data_dict for per-tag processing and ensure
REGISTRY_REQUEST_TIMEOUT and MAX_TAGS_FETCH_LIMIT are referenced (they are
already imported) so the function uses the shared configuration values.
---
Duplicate comments:
In `@scripts/infra-mcp/server.py`:
- Around line 29-34: The current use of getattr(logging, LOG_LEVEL) in the
logging.basicConfig call can raise AttributeError for invalid LOG_LEVEL values;
change to validate and map LOG_LEVEL to a numeric level before calling
logging.basicConfig (e.g., call logging.getLevelName(LOG_LEVEL) and if it
returns an int use that, otherwise fall back to logging.INFO) so LOG_LEVEL,
logging.getLevelName, and logging.basicConfig are used safely and invalid env
values don’t crash the server at import time.
In `@scripts/infra-mcp/tools/collections/task_tools.py`:
- Around line 86-96: The execute_task function currently only catches
subprocess.CalledProcessError and will let subprocess.TimeoutExpired propagate;
add an except subprocess.TimeoutExpired handler (mirroring get_task_list) to
catch the timeout, call logger.exception with a message like "Timeout executing
task {task_name}", and return a clear timeout string (including task_name and
timeout details or exception message) so timeouts are handled gracefully instead
of crashing.
- Line 129: The change in task_tools.py that sets tool_name =
task_name.replace(":", "-") alters the separator from the previous "--" form and
can break external consumers expecting names like namespace--taskname; revert or
explicitly preserve the old separator behavior by replacing ":" with "--"
instead of "-" (i.e., update the code that computes tool_name in task_tools.py),
or add a compatibility layer that emits both the legacy name
(namespace--taskname) and the new name or a configurable separator so downstream
MCP clients continue to resolve tools.
In `@scripts/infra-mcp/tools/get_dashboard_groups.py`:
- Around line 9-16: The except ImportError block redundantly imports os; replace
the os-based path construction with Path from pathlib (already imported) and
update the sys.path manipulation accordingly: in the except block remove the
"import os" line and change the sys.path.insert call that uses
os.path.abspath(os.path.join(os.path.dirname(__file__), "..")) to use
Path(__file__).resolve().parent.parent (or equivalent) converted to str; keep
the get_git_root import and sys.path.insert behavior but reference Path so the
block is consistent with existing imports and removes the unnecessary os usage.
---
Nitpick comments:
In `@scripts/infra-mcp/pyproject.toml`:
- Around line 11-12: Update the pyproject dependency entry for pyyaml: change
the minimum version in the requirement string "pyyaml>=6.0.2,<7" to
"pyyaml>=6.0.3,<7" so the package floor is bumped to the latest non-vulnerable
patch while keeping the upper bound; locate the pyyaml line in pyproject.toml
and replace the version specifier accordingly.
In `@scripts/infra-mcp/server.py`:
- Around line 209-223: The signal registration is happening at import time; keep
the handle_shutdown function but move the two signal.signal(...) registrations
out of module top-level and into the process entry path — either wrap them in an
if __name__ == "__main__": block or register inside the existing main() function
(or a dedicated setup_signal_handlers() helper) and call it from main(),
ensuring handle_shutdown remains available but registrations only occur when the
module is run as a program, not when imported.
In `@scripts/infra-mcp/tools/get_app_icon.py`:
- Around line 133-138: The HEAD request to default_favicon uses a hard-coded
timeout=5; replace that with the shared DEFAULT_REQUEST_TIMEOUT constant to keep
timeouts consistent (update the requests.head call in the same try block that
references default_favicon to use DEFAULT_REQUEST_TIMEOUT instead of 5); ensure
imports/namespace already provide DEFAULT_REQUEST_TIMEOUT as used in other calls
in this module (lines that use DEFAULT_REQUEST_TIMEOUT) so the change matches
existing usage.
In `@scripts/infra-mcp/tools/get_container_categories.py`:
- Around line 73-74: The except block that wraps exceptions into a RuntimeError
currently uses "from None", which suppresses the original traceback; in the
except Exception as e: handler replace "from None" with "from e" (or remove the
"from ..." clause) so the original exception chain is preserved when raising the
RuntimeError in get_container_categories.py (the except block that raises
RuntimeError(f"Error finding container categories: {str(e)}")).
In `@scripts/infra-mcp/tools/get_container_tags.py`:
- Around line 326-329: The loop currently hard-caps additional requests with the
magic number tags[:100]; update get_registry_tags to honor the provided limit
parameter (or replace 100 with a named constant from constants.py) so the slice
becomes tags[:limit] or tags[:TAG_FETCH_LIMIT], and ensure tag fetching still
calls _fetch_manifest_for_tag(registry_url, image_name, tag, architecture) and
appends to tag_data accordingly; modify any defaulting logic so limit is
validated (e.g., positive int) before slicing.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Improvements