Handle mermaid CLI timeouts and environment errors#22
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Summary by CodeRabbit
WalkthroughCentralise mermaid CLI invocation into _run_mermaid_cli, strengthen wait_for_proc to catch asyncio.TimeoutError and return (False, b""), update _render_diagram to delegate to the runner and raise RuntimeError on CLI failure, add NoNodeEnvironmentAvailableError handling in render_block, and add/adjust unit tests for timeout and temp-file cleanup. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as render_block / _render_diagram
participant Runner as _run_mermaid_cli
participant Proc as mermaid-cli (subprocess)
participant Waiter as wait_for_proc
Caller->>Runner: provide cmd, semaphore, path, idx, timeout
Runner->>Proc: spawn subprocess (stdout/stderr piped)
Runner->>Waiter: wait_for_proc(proc, path, idx, timeout)
alt completes in time
Waiter-->>Runner: (True, stderr)
Runner-->>Caller: (success, stderr)
else timeout
Waiter->>Proc: kill and wait
Waiter-->>Runner: (False, b"")
Runner-->>Caller: (failure, b"")
Caller->>Caller: raise RuntimeError with parsed stderr
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15 minutes Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (1)**/*.py⚙️ CodeRabbit Configuration File
Files:
🔇 Additional comments (4)
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
Reviewer's GuideThis PR refines Mermaid CLI execution by catching CLI timeouts in a dedicated helper, centralizes process invocation, surfaces clear errors for missing Node.js/Bun environments, ensures temporary Puppeteer configs are cleaned up, and adds tests for timeout behavior and config cleanup. Sequence diagram for error handling when Node.js/Bun is missingsequenceDiagram
participant CLIHandler
participant Logger
participant User
CLIHandler->>CLIHandler: _render_diagram()
CLIHandler->>CLIHandler: get_mmdc_cmd()
alt No Node.js/Bun environment
CLIHandler->>Logger: LOGGER.exception("No supported node environment found...")
Logger->>User: Display error message
end
Class diagram for updated Mermaid CLI process handlingclassDiagram
class CLIHandler {
+wait_for_proc(proc, path, idx, timeout)
+_run_mermaid_cli(cmd, semaphore, path, idx, timeout)
+_render_diagram(block, path, idx, timeout)
}
class NoNodeEnvironmentAvailableError
class UnexpectedExecutableError
CLIHandler --> NoNodeEnvironmentAvailableError
CLIHandler --> UnexpectedExecutableError
CLIHandler : uses wait_for_proc
CLIHandler : uses _run_mermaid_cli
CLIHandler : uses _render_diagram
NoNodeEnvironmentAvailableError <|-- Exception
UnexpectedExecutableError <|-- Exception
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes and found some issues that need to be addressed.
Blocking issues:
- Add a test to demonstrate the bug fix for catching asyncio.TimeoutError instead of TimeoutError. (link)
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `nixie/cli.py:149` </location>
<code_context>
try:
_, stderr = await asyncio.wait_for(proc.communicate(), timeout)
- except TimeoutError:
+ except asyncio.TimeoutError: # noqa: UP041
proc.kill()
await proc.wait()
</code_context>
<issue_to_address>
Add a test to demonstrate the bug fix for catching asyncio.TimeoutError instead of TimeoutError.
You changed the exception caught from TimeoutError to asyncio.TimeoutError, which is a bug fix. You must add a test that demonstrates this fix, ensuring that the correct exception is handled and the timeout logic works as intended.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (4)
nixie/cli.py (3)
149-154: Drop the noqa, and log the timeout instead of printing to stderr.Align with project guidelines: avoid blanket/narrow suppressions unless justified with FIXME/ticket, and prefer the module logger over print for consistency and testability.
Apply:
- except asyncio.TimeoutError: # noqa: UP041 + except asyncio.TimeoutError: proc.kill() await proc.wait() - print(f"{path}: diagram {idx} timed out", file=sys.stderr) + LOGGER.warning("%s: diagram %s timed out", path, idx) return (False, b"")
169-174: Document the Semgrep suppression with a FIXME rationale.Project guidelines require inline suppressions to include a FIXME or ticket and be a last resort. Add context explaining why this subprocess call is safe (allow-list of executables and controlled args).
- # nosemgrep: python.lang.security.audit.dangerous-asyncio-create-exec-audit + # nosemgrep: python.lang.security.audit.dangerous-asyncio-create-exec-audit + # FIXME: Allowed executables are constrained via ALLOWED_EXECUTABLES/get_mmdc_cmd, + # and arguments are constructed from validated Paths. Review if inputs change.
299-301: Fix invalid isinstance() usage with a union type at runtime.
isinstance(exc, KeyboardInterrupt | SystemExit)is not valid at runtime. Pass a tuple of types.- if isinstance(exc, KeyboardInterrupt | SystemExit): + if isinstance(exc, (KeyboardInterrupt, SystemExit)): raisenixie/unittests/test_wait_for_proc.py (1)
18-41: Also assert the timeout message is emitted to stderr.Capture and assert the warning to guard against regressions in user-visible output.
-@pytest.mark.asyncio -async def test_wait_for_proc_handles_asyncio_timeout_error(tmp_path: Path) -> None: +@pytest.mark.asyncio +async def test_wait_for_proc_handles_asyncio_timeout_error( + tmp_path: Path, capfd: pytest.CaptureFixture[str] +) -> None: @@ - success, stderr = await wait_for_proc(proc, tmp_path / "dummy.md", 1, timeout=0.01) + success, stderr = await wait_for_proc(proc, tmp_path / "dummy.md", 1, timeout=0.01) assert not success assert stderr == b"" + out = capfd.readouterr() + assert "dummy.md: diagram 1 timed out" in out.err, "stderr timeout message missing"
♻️ Duplicate comments (1)
nixie/unittests/test_wait_for_proc.py (1)
18-41: Demonstrate the asyncio.TimeoutError fix and assert API behaviour.The test exercises the timeout path correctly and validates the returned (False, b"") pair, addressing the earlier review request to cover the bug fix.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- Jira integration is disabled
You can enable these settings in your CodeRabbit configuration.
📒 Files selected for processing (3)
nixie/cli.py(4 hunks)nixie/unittests/test_puppeteer_config.py(1 hunks)nixie/unittests/test_wait_for_proc.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
⚙️ CodeRabbit Configuration File
**/*.py: - Keep cyclomatic complexity ≤ 12
- Follow single responsibility and CQRS (command/query segregation)
- Docstrings must follow the
numpystyle guide. Use a single-line summary for private functions and methods, and full structured docs for all public interfaces.- Move conditionals with >2 branches to predicate/helper functions
- Avoid
eval,exec,pickle, monkey-patching,ctypes, unsafe shell- Every module must begin with a triple-quoted docstring explaining its purpose, utility, and usage, including example calls if appropriate.
- Comments and docs must follow en-GB-oxendict (-ize / -our) spelling and grammar
- Lint suppressions:
- Blanket
# noqa, file-level skips, and categories are forbidden- Only narrow in-line disables (
# noqa: XYZ) are permitted, and must be accompanied byFIXME:or a ticket link, and used only as a last resort.- Use
pytestfixtures for shared setup (conftest.pyorfixtures/)- Replace duplicate tests with
@pytest.mark.parametrize- Prefer
pytest-mockorunittest.mockfor stubs/mocks- Use
assert …, "message"over bare asserts- Reflect all API/behaviour changes in
docs/and update roadmap on completion- Files must not exceed 400 logical lines:
- Decompose large modules into subpackages
- Split large
match/caseor dispatch tables by domain and collocate with targets if appropriate- Move bulky data (fixtures, templates) to external files for parsing at runtime
- Mutable defaults and shadowed built-ins are forbidden
- All code must have clear type hints using modern style (
A | B,list[str],class Foo[A]:,type Bar = int, etc.), with ABC imports drawn from the correct stdlib module.
Files:
nixie/unittests/test_wait_for_proc.pynixie/unittests/test_puppeteer_config.pynixie/cli.py
🧬 Code Graph Analysis (2)
nixie/unittests/test_wait_for_proc.py (1)
nixie/cli.py (2)
cli(395-405)wait_for_proc(143-155)
nixie/unittests/test_puppeteer_config.py (1)
nixie/cli.py (1)
create_puppeteer_config(84-106)
🔇 Additional comments (4)
nixie/cli.py (2)
175-176: LGTM: Delegate process waiting to the central helper.Returning the awaitable directly keeps the flow simple and consolidates timeout handling in one place.
220-229: LGTM: Centralise CLI invocation and surface actionable errors.Using
_run_mermaid_clihere reduces duplication, and raisingRuntimeErrorwith a parsed message improves UX.nixie/unittests/test_puppeteer_config.py (2)
11-13: LGTM: Keep typing-only imports lightweight.Importing
Pathunder TYPE_CHECKING pairs well withfrom __future__ import annotationsand avoids runtime dependencies.
20-27: LGTM: Assert temporary Puppeteer config is cleaned up.Capturing the path out-of-context and asserting it no longer exists robustly validates cleanup semantics.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (3)
nixie/cli.py (3)
158-166: Add a private docstring to _run_mermaid_cli.Private functions must have a one-line summary docstring per the guidelines.
Apply this diff:
async def _run_mermaid_cli( cmd: list[str], sem: asyncio.Semaphore, path: Path, idx: int, timeout: float, ) -> tuple[bool, bytes]: + """Run mermaid-cli under a semaphore and return (success, stderr)."""
296-305: Fix invalid isinstance usage and unreachable re-raise; restructure except blocks.isinstance(exc, KeyboardInterrupt | SystemExit) raises at runtime; also, those exceptions are not caught by except Exception, so the check is redundant. Restructure the handlers.
Apply this diff:
- except RuntimeError: - LOGGER.exception("Runtime error while rendering diagram") - except Exception as exc: - if isinstance(exc, KeyboardInterrupt | SystemExit): - raise - LOGGER.exception( - "%s: unexpected error in diagram %s", - path, - idx, - ) + except (KeyboardInterrupt, SystemExit): + raise + except RuntimeError: + LOGGER.exception("Runtime error while rendering diagram") + except Exception: + LOGGER.exception( + "%s: unexpected error in diagram %s", + path, + idx, + )
1-410: Split this module; it exceeds the 400-line limit.Extract the rendering/subprocess helpers (_run_mermaid_cli, wait_for_proc, get_mmdc_cmd, format_cli_error, create_puppeteer_config) into a dedicated module (e.g., nixie/mermaid_cli.py) and keep CLI/glue in cli.py.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- Jira integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
nixie/cli.py(4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
⚙️ CodeRabbit Configuration File
**/*.py: - Keep cyclomatic complexity ≤ 12
- Follow single responsibility and CQRS (command/query segregation)
- Docstrings must follow the
numpystyle guide. Use a single-line summary for private functions and methods, and full structured docs for all public interfaces.- Move conditionals with >2 branches to predicate/helper functions
- Avoid
eval,exec,pickle, monkey-patching,ctypes, unsafe shell- Every module must begin with a triple-quoted docstring explaining its purpose, utility, and usage, including example calls if appropriate.
- Comments and docs must follow en-GB-oxendict (-ize / -our) spelling and grammar
- Lint suppressions:
- Blanket
# noqa, file-level skips, and categories are forbidden- Only narrow in-line disables (
# noqa: XYZ) are permitted, and must be accompanied byFIXME:or a ticket link, and used only as a last resort.- Use
pytestfixtures for shared setup (conftest.pyorfixtures/)- Replace duplicate tests with
@pytest.mark.parametrize- Prefer
pytest-mockorunittest.mockfor stubs/mocks- Use
assert …, "message"over bare asserts- Reflect all API/behaviour changes in
docs/and update roadmap on completion- Files must not exceed 400 logical lines:
- Decompose large modules into subpackages
- Split large
match/caseor dispatch tables by domain and collocate with targets if appropriate- Move bulky data (fixtures, templates) to external files for parsing at runtime
- Mutable defaults and shadowed built-ins are forbidden
- All code must have clear type hints using modern style (
A | B,list[str],class Foo[A]:,type Bar = int, etc.), with ABC imports drawn from the correct stdlib module.
Files:
nixie/cli.py
🔇 Additional comments (4)
nixie/cli.py (4)
147-155: LGTM: Correctly catch asyncio.TimeoutError and clean up the process.Catching asyncio.TimeoutError is the right exception in this context; killing and awaiting the process and returning a consistent (False, b"") is sound.
175-175: LGTM: Delegate process waiting to wait_for_proc.Returning the awaited result centralises lifecycle and timeout handling.
222-222: LGTM: Reuse _run_mermaid_cli to centralise subprocess execution.This removes duplication and keeps error handling consistent.
291-295: LGTM: Log missing Node environment as an operational error, not an exception.Avoiding a traceback for this expected case is the right UX.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
1 participant
Summary
asyncio.TimeoutErrorinwait_for_procand reuse_run_mermaid_clifrom_render_diagramTesting
make fmtmake check-fmtmake lintmake typecheckmake testhttps://chatgpt.com/codex/tasks/task_e_689d160d9b3883229ef0b3ce662304f0
Summary by Sourcery
Improve CLI error handling and resource cleanup for Mermaid rendering, and strengthen test coverage.
New Features:
Enhancements:
Tests: