feat: Add 'doctor' command for health checks #262
Conversation
Implement a new 'graph-code doctor' CLI command that verifies all critical dependencies and configurations are properly set up. The command checks: - Docker daemon is running - Memgraph database connection and responsiveness - Required API keys (GEMINI, OPENAI, ORCHESTRATOR, CYPHER) - External tools (ripgrep for code search) Output displays clear pass/fail status with green checkmarks for passing checks and red x's for failures. The command exits with code 1 if any checks fail, enabling integration with CI/CD pipelines. Changes: - codebase_rag/tools/health_checker.py: New HealthChecker class - codebase_rag/cli.py: New doctor command with Rich formatting - codebase_rag/cli_help.py: Added doctor command documentation
Summary of ChangesHello @appyjuss, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a new Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a very useful doctor command for checking the health of the application's dependencies and configuration. The implementation is clean, with good use of the rich library for user-friendly output. My review focuses on enhancing the HealthChecker by making the failure messages more specific and actionable for the user. I've suggested explicitly handling command timeouts and providing guidance on how to fix missing API keys, aligning with best practices for robust error handling with external libraries. These changes will improve the diagnostic value of the doctor command.
Greptile OverviewGreptile SummaryThis PR implements a new What Changed:
Critical Issues Found: This PR has severe coding standards violations that must be addressed before merging:
The functionality itself appears sound, but the implementation significantly deviates from the project's strict coding standards around string constants, type annotations, and data structure selection. Confidence Score: 1/5
Important Files ChangedFile Analysis
Sequence DiagramsequenceDiagram
actor User
participant CLI as cli.py::doctor()
participant Checker as HealthChecker
participant Docker as Docker Daemon
participant Memgraph as Memgraph DB
participant OS as Operating System
participant Settings as settings (config)
User->>CLI: graph-code doctor
CLI->>Checker: HealthChecker()
CLI->>Checker: run_all_checks()
Checker->>Checker: check_docker()
Checker->>Docker: subprocess.run(docker info)
alt Docker Running
Docker-->>Checker: returncode=0, version
Checker->>Checker: HealthCheckResult(passed=True)
else Docker Not Running
Docker-->>Checker: returncode!=0
Checker->>Checker: HealthCheckResult(passed=False)
end
Checker->>Checker: check_memgraph_connection()
Checker->>Settings: Get MEMGRAPH_HOST, MEMGRAPH_PORT
Settings-->>Checker: host, port
Checker->>Memgraph: mgclient.connect(host, port)
alt Connection Success
Memgraph-->>Checker: connection
Checker->>Memgraph: execute(RETURN 1)
Memgraph-->>Checker: query result
Checker->>Checker: HealthCheckResult(passed=True)
else Connection Failed
Memgraph-->>Checker: MemgraphError
Checker->>Checker: HealthCheckResult(passed=False)
end
Checker->>Checker: check_api_keys()
loop For each API key (GEMINI, OPENAI, ORCHESTRATOR, CYPHER)
Checker->>OS: os.getenv(env_name)
OS-->>Checker: value or None
Checker->>Settings: getattr(settings, env_name)
Settings-->>Checker: value or None
Checker->>Checker: HealthCheckResult(passed)
end
Checker->>Checker: check_external_tool(ripgrep)
Checker->>OS: subprocess.run(which rg)
OS-->>Checker: returncode, path
Checker->>Checker: HealthCheckResult(passed)
Checker->>Checker: check_external_tool(cmake)
Checker->>OS: subprocess.run(which cmake)
OS-->>Checker: returncode, path
Checker->>Checker: HealthCheckResult(passed)
Checker-->>CLI: List of HealthCheckResults
CLI->>Checker: get_summary()
Checker-->>CLI: (passed_count, total_count)
CLI->>CLI: Build Rich Table with results
CLI->>User: Display Panel with checkmarks
CLI->>User: Display summary text
alt Any check failed
CLI->>User: Display failed checks details
CLI->>User: Exit with code 1
else All passed
CLI->>User: Exit with code 0
end
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
- Move HealthCheckResult to schemas.py as BaseModel - Add from __future__ import annotations - Move ~50 hardcoded strings to constants.py - Add TimeoutExpired exception handling - Add error messages to API key checks - Remove docstrings (self-documenting code) - Keep cmake check (required for pymgclient, documented in README)
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a new doctor command to perform health checks on critical dependencies and configurations. The implementation is well-structured, with a dedicated HealthChecker class and clear output formatting using rich. My review focuses on improving error handling, consistency in subprocess calls, and standardizing import practices. I've provided specific code suggestions for these improvements, including a modification to clarify the rationale for broad exception catching in resource cleanup.
|
|
||
| import typer | ||
| from loguru import logger | ||
| from rich.panel import Panel |
There was a problem hiding this comment.
For consistency and better code organization, all imports should be at the top of the file. Table should be imported here along with Panel. The local import inside the doctor function (line 384) should then be removed.
| from rich.panel import Panel | |
| from rich.panel import Panel | |
| from rich.table import Table |
| if cursor is not None: | ||
| try: | ||
| cursor.close() | ||
| except Exception: | ||
| pass | ||
| if conn is not None: | ||
| try: | ||
| conn.close() | ||
| except Exception: | ||
| pass |
There was a problem hiding this comment.
Silently swallowing exceptions in a finally block can hide underlying issues with resource cleanup, such as problems with the connection or cursor. It's better to log these exceptions at a warning level to aid in debugging potential resource leaks or other unexpected behavior. This change requires importing logger from loguru at the top of the file.
| if cursor is not None: | |
| try: | |
| cursor.close() | |
| except Exception: | |
| pass | |
| if conn is not None: | |
| try: | |
| conn.close() | |
| except Exception: | |
| pass | |
| if cursor is not None: | |
| try: | |
| cursor.close() | |
| except Exception as e: # Catching broad Exception as Memgraph's close() method can raise various, unpredictable exceptions from an external library. | |
| logger.warning(f"Failed to close Memgraph cursor: {e}") | |
| if conn is not None: | |
| try: | |
| conn.close() | |
| except Exception as e: # Catching broad Exception as Memgraph's close() method can raise various, unpredictable exceptions from an external library. | |
| logger.warning(f"Failed to close Memgraph connection: {e}") |
References
- If catching a broad
Exceptionis a pragmatic choice due to a wide and unpredictable variety of exceptions from external libraries, add a comment explaining the rationale to improve code maintainability.
| result = subprocess.run( | ||
| check_cmd, | ||
| capture_output=True, | ||
| timeout=4, | ||
| check=False, | ||
| ) | ||
| if result.returncode == 0: | ||
| path = result.stdout.decode().strip().splitlines()[0] |
There was a problem hiding this comment.
For consistency with the check_docker method and to simplify the code, it's better to use text=True in the subprocess.run call. This will automatically decode the output streams as text, removing the need for result.stdout.decode().
result = subprocess.run(
check_cmd,
capture_output=True,
text=True,
timeout=4,
check=False,
)
if result.returncode == 0:
path = result.stdout.strip().splitlines()[0]
|
@appyjuss please address the bots comments. They are minor but valid points. |
2 participants
Implement a new 'graph-code doctor' CLI command that verifies all critical dependencies and configurations are properly set up. The command checks:
Output displays clear pass/fail status with green checkmarks for passing checks and red x's for failures. The command exits with code 1 if any checks fail, enabling integration with CI/CD pipelines.
Changes: