Configure and enable ruff formatter, simplify code - #268
Conversation
WalkthroughEnables ruff-format hook and relaxes Ruff limits; normalizes string quoting and trailing commas across many scripts; migrates several utilities to pathlib and typed signatures; adds masking and GitHub-link utilities; and performs a substantial refactor of scripts/labctl.py (Docker options, env/network handling, command orchestration). Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI (user)
participant Labctl as labctl.py
participant FS as Filesystem (stack files)
participant Docker as Docker daemon (subprocess)
CLI->>Labctl: invoke service command (service, action, DockerOptions)
Labctl->>FS: locate stack_dir / compose file (DOCKER_STACKS_DIR, get_compose_file)
Labctl->>Labctl: validate options, compute env-file args, ensure networks exist
Labctl->>Docker: run docker subprocess with env / env-file args / flags
Docker-->>Labctl: return status / output
Labctl-->>CLI: emit logs / exit status
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/proxy-request-log.py (1)
44-59: Unreachable code after early return - is this intentional?The
returnstatement on line 45 makes the proxy logging-only. All code from lines 47-59 is now unreachable dead code and the proxy will never actually forward requests to the target server.If this is intentional for debugging purposes, consider either:
- Removing the dead code entirely
- Adding a comment explaining this is a logging-only mode
If this was unintentional, remove the early return to restore proxy functionality.
If logging-only mode is intentional, remove the dead code:
print(f"\nSending request to: https://{target_host}:{target_port}{parsed_url.path}") - return - - # Create the connection to the target server - conn = http.client.HTTPConnection(target_host, target_port) - - # Make the request to the target server - conn.request(self.command, urlunparse(parsed_url._replace(scheme="", netloc="")), body=post_data, headers=headers) - target_response = conn.getresponse() - - # Send the response back to the client - self.send_response(target_response.status, target_response.reason) - for header, value in target_response.getheaders(): - self.send_header(header, value) - self.end_headers() - self.wfile.write(target_response.read()) + return # Logging-only mode - proxy forwarding disabled
🤖 Fix all issues with AI agents
In `@scripts/update-example-env.py`:
- Around line 52-53: The unpacked but unused `value` in the block that checks
`if "=" in line:` should be renamed to indicate it's intentionally unused;
update the split in that block (the `variable, value = line.strip().split("=",
1)` line) to either `variable, _ = ...` or assign only the left side (e.g., take
index 0) so static analysis no longer flags `value` as unused.
🧹 Nitpick comments (1)
scripts/infra-mcp/tools/get_container_tags.py (1)
190-192: Consider silencing the unusedlimitparameter warning.The
limitparameter is documented as "unused in fetching, used by caller" which is intentional. To satisfy Ruff's ARG002 check, you could either prefix with underscore or add a noqa comment.Option 1: Prefix with underscore
def get_docker_hub_tags( - self, image_name: str, limit: int = 10, architecture: str = "linux/amd64", sort_by: str = "version" + self, image_name: str, _limit: int = 10, architecture: str = "linux/amd64", sort_by: str = "version" ) -> list[dict[str, Any]]:Option 2: Add noqa comment
def get_docker_hub_tags( - self, image_name: str, limit: int = 10, architecture: str = "linux/amd64", sort_by: str = "version" + self, image_name: str, limit: int = 10, architecture: str = "linux/amd64", sort_by: str = "version" # noqa: ARG002 ) -> list[dict[str, Any]]:
d02073b to
af7aaf2
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@scripts/infra-mcp/utils/git.py`:
- Around line 27-29: The current logic always sets cwd =
Path(reference_path).resolve().parent which treats reference_path as a file;
change it to detect whether reference_path is a directory or a file and set cwd
accordingly (if Path(reference_path).resolve().is_dir() then cwd should be the
resolved path itself, else use .parent), and update any docstring/comment for
the function that uses reference_path to clarify that the function accepts
either a file or directory and will run from the appropriate directory;
reference the variables reference_path and cwd in your change.
In `@scripts/labctl.py`:
- Around line 141-152: The long RuntimeError message in the docker function
should be replaced by raising a dedicated exception class: define a new
DockerNotFoundError (subclassing RuntimeError) with a sensible default message
like "Docker executable not found on PATH." and then change the docker(...)
implementation to raise DockerNotFoundError() when shutil.which("docker")
returns None; keep the existing behavior of locating docker_bin and calling
subprocess.run otherwise.
In `@scripts/update-example-env.py`:
- Around line 61-78: The mask_line function currently preserves whitespace on
the left-hand variable so inputs like "PASSWORD = secret" yield variable names
with trailing spaces and bypass masking; update mask_line to strip whitespace
from the variable name immediately after splitting (e.g., set variable =
variable.strip()) before calling get_generalized_value or contains_any_substring
and use that trimmed variable when building the returned string so sensitive
names (SENSITIVE_VARS_TO_MASK) and generalized matches are detected and replaced
with MASKED_VALUE as intended.
🧹 Nitpick comments (2)
scripts/infra-mcp/tools/get_container_tags.py (1)
190-192: Consider prefixing unused parameters with underscore.The
limitparameter is documented as "unused in fetching, used by caller" but static analysis flags it (ARG002). While the current approach is acceptable given the docstring explanation, you could prefix it with an underscore (_limit) to signal intentional non-use, or add a# noqa: ARG002comment if you prefer keeping the current naming.Also applies to: 238-245
scripts/github-extract-links.py (1)
13-35: Stabilize link ordering for deterministic output.
set→listmakes output order unpredictable; this can cause noisy diffs in CI or scripts. Consider sorting before returning.♻️ Proposed change
- return list(github_links) + return sorted(github_links)
af7aaf2 to
8f70b41
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@scripts/labctl.py`:
- Around line 81-100: The create_localhost_link function uses
socket.gethostname() raw casing which can mismatch get_host_config_dir() that
lowercases hostnames; change create_localhost_link to normalize the hostname
(e.g., hostname = socket.gethostname().lower()) before constructing hostname_dir
and the symlink target so the directory lookup and os.symlink target match
get_host_config_dir's lowercase convention; update all uses of hostname within
create_localhost_link (hostname_dir and the f"{hostname}/" symlink target) to
the lowercased value and keep the existing existence/symlink checks and error
handling.
🧹 Nitpick comments (2)
scripts/github-extract-links.py (2)
27-30: Consider specifying explicit UTF-8 encoding.
read_text()without an encoding argument uses the system's locale-preferred encoding, which may not be UTF-8 on all platforms. While theUnicodeDecodeErrorcatch provides a fallback, being explicit improves cross-platform consistency.♻️ Suggested change
try: - content = file_path.read_text() + content = file_path.read_text(encoding="utf-8") except (OSError, UnicodeDecodeError): continue
43-48: Consider adding directory validation for better user feedback.If the provided path doesn't exist or isn't a directory, the script silently produces no output. Adding a check could improve usability.
♻️ Suggested change
def main() -> None: """Extract and print GitHub links from the specified directory.""" directory = Path(sys.argv[1]) if len(sys.argv) > 1 else Path.cwd() + if not directory.is_dir(): + print(f"Error: '{directory}' is not a valid directory", file=sys.stderr) + sys.exit(1) + for link in extract_github_links(directory): print(f"https://github.com/{link}")
8f70b41 to
2658f8d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@scripts/infra-mcp/tools/get_app_icon.py`:
- Around line 83-121: The favicon check using requests.head on default_favicon
misses redirects because requests.head defaults to allow_redirects=False; in the
get_app_icon logic update the requests.head call for default_favicon to include
allow_redirects=True and replace the strict status_code==200 check with
favicon_response.ok (or equivalent truthy success check); keep the same headers
and timeout, and ensure any exceptions from requests (e.g., in the surrounding
function retrieving homepage_url/default_favicon) are handled consistently with
the existing error handling in the get_app_icon flow.
🧹 Nitpick comments (3)
scripts/infra-mcp/utils/git.py (1)
25-25: Optional: Extract exception messages to improve Ruff TRY003 compliance.Ruff flags long messages in
RuntimeErrorraises (TRY003). You could define message constants or a small custom exception class, but this is purely stylistic and low priority given the code passes linting.Also applies to: 40-40, 42-42
scripts/infra-mcp/tools/get_container_tags.py (1)
190-192: Consider removing or using the unusedlimitparameter.Ruff flags that
limitis unused in bothget_docker_hub_tagsandget_registry_tags. The docstrings note it's "unused in fetching, used by caller," but having an unused parameter can be misleading to callers who might expect limiting to happen internally.Options:
- Remove the parameter and document that limiting should be done by the caller
- Actually use
limitto cap results before returning (e.g.,return tag_data[:limit])- If intentionally keeping for API consistency, prefix with underscore:
_limitscripts/github-extract-links.py (1)
13-35: Return a stable ordering for extracted links.
setiteration order is arbitrary, so output order can change run to run. Returning a sorted list makes CLI output deterministic and easier to diff.✅ Suggested change
- return list(github_links) + return sorted(github_links)
2658f8d to
a44ac8d
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@scripts/labctl.py`:
- Around line 108-115: The function has_build_directive currently assumes
yaml_content["services"] is a dict and will raise AttributeError if it's not;
update has_build_directive to check that yaml_content.get("services") is a dict
before iterating—if it's missing or not a dict, return False; then only call
.values() and any("build" in svc ...) when services is confirmed to be a mapping
to avoid runtime errors on malformed compose files.
- Around line 275-279: The loop reading config["services"] must skip non-dict
entries to avoid AttributeError when accessing keys(); change the validation
around category_entry (in scripts/labctl.py) to first check
isinstance(category_entry, dict) and len(category_entry) == 1, otherwise
logger.warning and continue; additionally validate that the sole value (the
services list) is a list (using isinstance(value, list)) before processing and
warn+continue if not, so the code using category_entry.keys()/values() won't
crash on strings/lists.
In `@scripts/update-example-env.py`:
- Around line 61-79: The mask_line function is currently doing case-sensitive
checks which can miss secrets; modify it to preserve the original variable key
but use an uppercase-normalized version for lookups: after extracting variable
from the line, create a normalized_variable = variable.upper() and pass that
into get_generalized_value(normalized_variable) and
contains_any_substring(normalized_variable, SENSITIVE_VARS_TO_MASK) (and any
other matching calls), while still returning f"{variable}={...}" so the original
key casing remains in the output; ensure
SENSITIVE_VARS_TO_GENERALIZE/SENSITIVE_VARS_TO_MASK comparisons expect uppercase
or normalize them once at module load if needed.
🧹 Nitpick comments (1)
scripts/github-extract-links.py (1)
35-47: Make output deterministic.
setiteration order is nondeterministic, so the printed list can change between runs. Sorting avoids churn in CI logs and diffs.♻️ Suggested change
- return list(github_links) + return sorted(github_links)
a44ac8d to
a0cd835
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@scripts/infra-mcp/utils/git.py`:
- Line 25: The inline long error messages in raises (e.g., the RuntimeError at
the raise in git.py and the raises around lines 40-42) trigger Ruff TRY003;
define small custom exception classes (for example GitNotFoundError and
GitCommandError) with default messages and raise those classes without passing
the long string inline. Update the locations that currently do `raise
RuntimeError("...long message...")` to instead `raise GitNotFoundError()` (or
the appropriate new exception) and ensure the new exception classes inherit from
RuntimeError and include the long message as their default message in their
class definition or __str__; reference the existing raise sites and function
names in git.py to replace the inline messages accordingly.
In `@scripts/labctl.py`:
- Around line 264-304: The process_services function lacks type guards for
malformed config entries and services; add checks so if config is not a dict or
"services" is not a list you log an error and return, and inside the loop ensure
each category_entry is a dict with a single key and that the category value is a
list (already partially done), and before reading service.get validate that
service is a dict—if not, logger.warning and continue. Also ensure
state_override/service.get usage remains safe by only calling service.get when
service is a dict, and keep using DockerOptions, ALLOWED_OPERATIONS, and
docker_command as-is.
🧹 Nitpick comments (3)
scripts/github-extract-links.py (2)
9-10: Comment is misleading about allowed characters.The comment says usernames can only contain alphanumeric and dashes, but the regex pattern
[\w.\-_]+also matches dots and underscores. While the pattern is intentionally permissive to also match repository names (which allow periods, underscores, and hyphens), the comment should reflect this.Additionally,
_is redundant in the character class since\walready includes underscores.📝 Suggested fix
-# Usernames for user accounts on GitHub can only contain alphanumeric characters and dashes ( - ). -GITHUB_REPO_PATTERN = re.compile(r"https://github\.com/([\w.\-_]+/[\w.\-_]+)") +# GitHub URLs: owner (alphanumeric + hyphens) and repo (alphanumeric + hyphens, underscores, periods) +GITHUB_REPO_PATTERN = re.compile(r"https://github\.com/([\w.\-]+/[\w.\-]+)")
43-48: Consider adding directory validation.If an invalid or non-existent path is provided, the script silently produces no output. For better UX, consider validating the directory exists.
📝 Suggested validation
def main() -> None: """Extract and print GitHub links from the specified directory.""" directory = Path(sys.argv[1]) if len(sys.argv) > 1 else Path.cwd() + if not directory.is_dir(): + print(f"Error: '{directory}' is not a valid directory", file=sys.stderr) + sys.exit(1) for link in extract_github_links(directory): print(f"https://github.com/{link}")scripts/update-example-env.py (1)
61-80: Preserve inline comments when masking (optional).Masking currently rebuilds the line without any trailing inline comment (e.g.,
VAR=value # note). If those comments matter in example files, consider retaining them when masking/generalizing.♻️ Optional tweak to preserve inline comments
- variable, _ = line.strip().split("=", 1) - variable = variable.strip() + variable, remainder = line.strip().split("=", 1) + variable = variable.strip() + comment_match = re.search(r"\s+#.*$", remainder) + comment_suffix = comment_match.group(0) if comment_match else "" normalized_variable = variable.upper() generalized_value = get_generalized_value(normalized_variable) if generalized_value is not None: - return f"{variable}={generalized_value}" + return f"{variable}={generalized_value}{comment_suffix}" if contains_any_substring(normalized_variable, SENSITIVE_VARS_TO_MASK): - return f"{variable}={MASKED_VALUE}" + return f"{variable}={MASKED_VALUE}{comment_suffix}"
a0cd835 to
058afe9
Compare
Summary by CodeRabbit
New Features
Improvements
Style
Chores
✏️ Tip: You can customize this high-level summary in your review settings.