Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideRemoved the unused verbose parameter from render_block, updated subprocess type annotations to use the imported alias, and renamed the test to reflect the revised logging behavior. Class diagram for updated render_block and wait_for_proc signaturesclassDiagram
class render_block {
+async render_block(path: Path, idx: int, semaphore: asyncio.Semaphore, timeout: float = 30.0) bool
}
class wait_for_proc {
+async wait_for_proc(proc: asyncio_subprocess.Process, path: Path, idx: int, timeout: float = 30.0) tuple[bool, bytes]
}
render_block --> wait_for_proc : calls
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
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
WalkthroughRefactor subprocess handling in Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant _render_diagram
participant _run_mermaid_cli
participant Semaphore
participant Subprocess
Caller->>_render_diagram: call with block, paths, sem, timeout
_render_diagram->>_run_mermaid_cli: call with cmd, sem, path, idx, timeout
_run_mermaid_cli->>Semaphore: acquire()
_run_mermaid_cli->>_run_mermaid_cli: validate executable in whitelist
_run_mermaid_cli->>Subprocess: create and run process
_run_mermaid_cli->>Semaphore: release()
_run_mermaid_cli-->>_render_diagram: return (success, output)
_render_diagram-->>Caller: complete
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15 minutes Possibly related PRs
Poem
✨ 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 comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @leynos - I've reviewed your changes and they look great!
Blocking issues:
- Detected 'create_subprocess_exec' function without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'. (link)
Prompt for AI Agents
Please address the comments from this code review:
## Security Issues
### Issue 1
<location> `nixie/cli.py:164` </location>
<issue_to_address>
**security (python.lang.security.audit.dangerous-asyncio-create-exec-audit):** Detected 'create_subprocess_exec' function without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'.
*Source: opengrep*
</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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
nixie/cli.py(4 hunks)nixie/unittests/test_verbose.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.py
📄 CodeRabbit Inference Engine (AGENTS.md)
For Python files: Pass all relevant unit and behavioral tests, pass lint checks (
ruff check), adhere to formatting standards (ruff format), and pass type checking (pyright).
**/*.py: Use snake_case.py for file names, naming files for their contents (e.g., http_client.py, task_queue.py)
Use PascalCase for class names in Python files
Use snake_case for variable and function names in Python files
Use UPPER_SNAKE_CASE for module-level constants in Python files
Prefix private/internal helpers or APIs with a single underscore (_) in Python files
Use typing everywhere and maintain full static type coverage in Python files; use Pyright for type-checking
Use TypedDict or Dataclass for structured data where appropriate; for internal-only usage, prefer @DataClass(slots=True)
Avoid Any in type annotations; use Unknown, generics, or cast() when necessary, and always document why Any is acceptable if used
Be explicit with return types in public functions and class methods (e.g., use -> None, -> str, etc.)
Favor immutability in Python files; prefer tuples over lists, and frozendict or types.MappingProxyType where appropriate
Enable Ruff for linting and formatting; use Ruff to lint for performance, security, consistency, and style issues, and let Ruff handle whitespace and formatting entirely
Enforce strict mode in Pyright and treat all Pyright warnings as CI errors; use # pyright: ignore sparingly and with explanation
Avoid side effects at import time in Python modules; modules should not modify global state or perform actions on import
Use docstrings to document public functions, classes, and modules using NumPy format
Explain tricky code with inline comments for non-obvious logic or decisions in Python files
**/*.py: Use context managers to encapsulate setup and teardown logic cleanly and safely, especially for resource management (files, locks, connections, etc.), instead of manual try/finally blocks.
Use@contextmanagerfromcontextlibfor straightforward proce...
Files:
nixie/cli.pynixie/unittests/test_verbose.py
⚙️ CodeRabbit Configuration File
**/*.py: How about the following?- Keep cyclomatic complexity ≤ 12 - Follow single responsibility and CQRS (command/query segregation) - Docstrings must follow the `numpy` style 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 by `FIXME:` or a ticket link, and used only as a last resort. - Use `pytest` fixtures for shared setup (`conftest.py` or `fixtures/`) - Replace duplicate tests with `@pytest.mark.parametrize` - Prefer `pytest-mock` or `unittest.mock` for 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/case` or 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.pynixie/unittests/test_verbose.py
**/unittests/test_*.py
📄 CodeRabbit Inference Engine (.rules/python-00.mdc)
Colocate unit tests with code using a unittests subdirectory and a test_ prefix for test files
Files:
nixie/unittests/test_verbose.py
**/test_*.py
📄 CodeRabbit Inference Engine (.rules/python-00.mdc)
**/test_*.py: Use pytest idioms in test files: prefer fixtures over setup/teardown methods, parametrize broadly, and avoid unnecessary mocks
Group related tests using class with method names prefixed by test_ in test files
Files:
nixie/unittests/test_verbose.py
🪛 Ruff (0.12.2)
nixie/cli.py
96-96: Async function definition with a timeout parameter
(ASYNC109)
96-96: Trailing comma missing
Add trailing comma
(COM812)
🔇 Additional comments (6)
nixie/cli.py (5)
6-6: LGTM!The import alias improves readability and maintains consistency with the subprocess references used elsewhere in the file.
166-167: LGTM!The subprocess pipe constants correctly use the new alias, maintaining consistency with the import.
205-205: LGTM!The docstring correctly reflects the updated behaviour where command lines are always logged at INFO level, removing the previous conditional logic.
180-189: Confirm all render_block invocations updatedAll calls to render_block now pass exactly six parameters (block, tmpdir, cfg_path, path, idx, semaphore), matching the new signature.
Check locations:
- nixie/cli.py: invocation in task list under
with tempfile.TemporaryDirectory()…- nixie/unittests/test_verbose.py: two assertions calling
await render_block(…)
309-320: Ignore removal suggestion of--verboseflag.The
--verboseflag correctly sets the global logging level (INFO vs WARNING) vialogging.basicConfig(), andrender_blockuses that level to emit or suppress its command logs. Tests innixie/unittests/test_verbose.pyverify that INFO logs the command and WARNING silences it.Likely an incorrect or invalid review comment.
nixie/unittests/test_verbose.py (1)
56-56: LGTM!The function rename accurately reflects the test's purpose, focusing on logging level behaviour rather than a verbose flag. The test logic remains correct and validates the expected logging behaviour.
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (2)
nixie/unittests/test_run_mermaid_cli.py (1)
82-89: Strengthen coverage with parametrisation and message assertion.Parametrise multiple disallowed executables and assert the offending executable appears in the message. Also assert no subprocess is spawned by guarding the factory.
Apply this diff within this test to parametrise:
-@pytest.mark.asyncio -async def test_run_mermaid_cli_rejects_unexpected_executable() -> None: - semaphore = asyncio.Semaphore(1) - path = Path("doc.md") - cmd = ["echo", "hello"] - with pytest.raises(ValueError, match="Unexpected executable"): - await _run_mermaid_cli(cmd, semaphore, path, 1, 30.0) +@pytest.mark.asyncio +@pytest.mark.parametrize("exe", ["echo", "python", "sh"]) +async def test_run_mermaid_cli_rejects_unexpected_executable(exe: str, monkeypatch: pytest.MonkeyPatch) -> None: + semaphore = asyncio.Semaphore(1) + path = Path("doc.md") + cmd = [exe, "hello"] + + # Fail the test if a subprocess is attempted (whitelist must short-circuit first). + async def should_not_be_called(*_args, **_kwargs): # pragma: no cover - defensive + raise AssertionError("create_subprocess_exec must not be called for disallowed executables") + monkeypatch.setattr(asyncio, "create_subprocess_exec", should_not_be_called) + + with pytest.raises(ValueError, match=rf"Unexpected executable: {exe}"): + await _run_mermaid_cli(cmd, semaphore, path, 1, 30.0)nixie/cli.py (1)
153-159: Document the new ValueError in the Raises section.Update the docstring to reflect the whitelist enforcement.
Apply this diff:
Raises ------ RuntimeError If the CLI exits with a non-zero status. FileNotFoundError If the CLI executable cannot be found. + ValueError + If the executable in ``cmd`` is not one of the allowed values.
♻️ Duplicate comments (2)
nixie/cli.py (2)
168-172: Resolve prior “dangerous create_subprocess_exec” audit.Invoke
create_subprocess_execwith an argument list (no shell), validate the executable via whitelist, and pipe stdio explicitly. This mitigates command injection vectors.
95-97: Add trailing comma in async signature to satisfy Ruff COM812 and keep diffs minimal.Apply this diff:
-async def wait_for_proc( - proc: asyncio_subprocess.Process, path: Path, idx: int, timeout: float = 30.0 -) -> tuple[bool, bytes]: +async def wait_for_proc( + proc: asyncio_subprocess.Process, path: Path, idx: int, timeout: float = 30.0, +) -> tuple[bool, bytes]:
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
nixie/cli.py(4 hunks)nixie/unittests/test_run_mermaid_cli.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.py
📄 CodeRabbit Inference Engine (AGENTS.md)
For Python files: Pass all relevant unit and behavioral tests, pass lint checks (
ruff check), adhere to formatting standards (ruff format), and pass type checking (pyright).
**/*.py: Use snake_case.py for file names, naming files for their contents (e.g., http_client.py, task_queue.py)
Use PascalCase for class names in Python files
Use snake_case for variable and function names in Python files
Use UPPER_SNAKE_CASE for module-level constants in Python files
Prefix private/internal helpers or APIs with a single underscore (_) in Python files
Use typing everywhere and maintain full static type coverage in Python files; use Pyright for type-checking
Use TypedDict or Dataclass for structured data where appropriate; for internal-only usage, prefer @DataClass(slots=True)
Avoid Any in type annotations; use Unknown, generics, or cast() when necessary, and always document why Any is acceptable if used
Be explicit with return types in public functions and class methods (e.g., use -> None, -> str, etc.)
Favor immutability in Python files; prefer tuples over lists, and frozendict or types.MappingProxyType where appropriate
Enable Ruff for linting and formatting; use Ruff to lint for performance, security, consistency, and style issues, and let Ruff handle whitespace and formatting entirely
Enforce strict mode in Pyright and treat all Pyright warnings as CI errors; use # pyright: ignore sparingly and with explanation
Avoid side effects at import time in Python modules; modules should not modify global state or perform actions on import
Use docstrings to document public functions, classes, and modules using NumPy format
Explain tricky code with inline comments for non-obvious logic or decisions in Python files
**/*.py: Use context managers to encapsulate setup and teardown logic cleanly and safely, especially for resource management (files, locks, connections, etc.), instead of manual try/finally blocks.
Use@contextmanagerfromcontextlibfor straightforward proce...
Files:
nixie/unittests/test_run_mermaid_cli.pynixie/cli.py
⚙️ CodeRabbit Configuration File
**/*.py: How about the following?- Keep cyclomatic complexity ≤ 12 - Follow single responsibility and CQRS (command/query segregation) - Docstrings must follow the `numpy` style 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 by `FIXME:` or a ticket link, and used only as a last resort. - Use `pytest` fixtures for shared setup (`conftest.py` or `fixtures/`) - Replace duplicate tests with `@pytest.mark.parametrize` - Prefer `pytest-mock` or `unittest.mock` for 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/case` or 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_run_mermaid_cli.pynixie/cli.py
**/unittests/test_*.py
📄 CodeRabbit Inference Engine (.rules/python-00.mdc)
Colocate unit tests with code using a unittests subdirectory and a test_ prefix for test files
Files:
nixie/unittests/test_run_mermaid_cli.py
**/test_*.py
📄 CodeRabbit Inference Engine (.rules/python-00.mdc)
**/test_*.py: Use pytest idioms in test files: prefer fixtures over setup/teardown methods, parametrize broadly, and avoid unnecessary mocks
Group related tests using class with method names prefixed by test_ in test files
Files:
nixie/unittests/test_run_mermaid_cli.py
🧬 Code Graph Analysis (1)
nixie/unittests/test_run_mermaid_cli.py (1)
nixie/cli.py (1)
_run_mermaid_cli(141-181)
🪛 Ruff (0.12.2)
nixie/cli.py
96-96: Async function definition with a timeout parameter
(ASYNC109)
96-96: Trailing comma missing
Add trailing comma
(COM812)
165-165: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (3)
nixie/unittests/test_run_mermaid_cli.py (1)
82-89: Validate unexpected executable branch (LGTM).Exercise the whitelist guard and assert the correct exception type/message. Keep.
nixie/cli.py (2)
6-6: Alias asyncio.subprocess for clearer typing (LGTM).Use
asyncio_subprocessconsistently in annotations and PIPE constants.
209-210: Clarify logging behaviour in docs (LGTM).State INFO-level logging without tying it to a function parameter.
… centralize process handling. This fixes unresolved reference in tests and ensures safer CLI invocation paths. 💘 Generated with Crush Co-Authored-By: Crush <crush@charm.land>
leynos
force-pushed
the
codex/remove-unused-verbose-parameter
branch
from
c7a837b to
b8ff0a1
Compare
Ensure the generated command is non-empty and that its first token is one of our allowed executables before logging or executing. This prevents logging unvalidated or empty commands and avoids potential IndexError. 💘 Generated with Crush Co-Authored-By: Crush <crush@charm.land>
Define ALLOWED_EXECUTABLES as a typing.Final frozenset and replace per-call set creations in _run_mermaid_cli and _render_diagram. Improves clarity and avoids redundant allocations. 💘 Generated with Crush Co-Authored-By: Crush <crush@charm.land>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
nixie/cli.py (3)
38-38: Hoist and freeze allowed executables. Resolved prior review.Define
ALLOWED_EXECUTABLESasFinal[frozenset[str]]at module scope to make policy explicit and avoid per-call allocation.
186-188: Validate before logging. Resolved prior review.Perform the whitelist check before emitting the command string; also guard empty commands. This addresses the earlier logging/validation ordering concern.
108-109: Add trailing comma for multi-line signature (ruff COM812).Comply with formatting lint and keep diffs minimal.
-async def wait_for_proc( - proc: asyncio_subprocess.Process, path: Path, idx: int, timeout: float = 30.0 +async def wait_for_proc( + proc: asyncio_subprocess.Process, path: Path, idx: int, timeout: float = 30.0, ) -> tuple[bool, bytes]:
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
nixie/cli.py(5 hunks)nixie/unittests/test_render_diagram.py(2 hunks)nixie/unittests/test_verbose.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.py
📄 CodeRabbit Inference Engine (AGENTS.md)
For Python files: Pass all relevant unit and behavioral tests, pass lint checks (
ruff check), adhere to formatting standards (ruff format), and pass type checking (pyright).
**/*.py: Use snake_case.py for file names, naming files for their contents (e.g., http_client.py, task_queue.py)
Use PascalCase for class names in Python files
Use snake_case for variable and function names in Python files
Use UPPER_SNAKE_CASE for module-level constants in Python files
Prefix private/internal helpers or APIs with a single underscore (_) in Python files
Use typing everywhere and maintain full static type coverage in Python files; use Pyright for type-checking
Use TypedDict or Dataclass for structured data where appropriate; for internal-only usage, prefer @DataClass(slots=True)
Avoid Any in type annotations; use Unknown, generics, or cast() when necessary, and always document why Any is acceptable if used
Be explicit with return types in public functions and class methods (e.g., use -> None, -> str, etc.)
Favor immutability in Python files; prefer tuples over lists, and frozendict or types.MappingProxyType where appropriate
Enable Ruff for linting and formatting; use Ruff to lint for performance, security, consistency, and style issues, and let Ruff handle whitespace and formatting entirely
Enforce strict mode in Pyright and treat all Pyright warnings as CI errors; use # pyright: ignore sparingly and with explanation
Avoid side effects at import time in Python modules; modules should not modify global state or perform actions on import
Use docstrings to document public functions, classes, and modules using NumPy format
Explain tricky code with inline comments for non-obvious logic or decisions in Python files
**/*.py: Use context managers to encapsulate setup and teardown logic cleanly and safely, especially for resource management (files, locks, connections, etc.), instead of manual try/finally blocks.
Use@contextmanagerfromcontextlibfor straightforward proce...
Files:
nixie/unittests/test_render_diagram.pynixie/unittests/test_verbose.pynixie/cli.py
⚙️ CodeRabbit Configuration File
**/*.py: How about the following?- Keep cyclomatic complexity ≤ 12 - Follow single responsibility and CQRS (command/query segregation) - Docstrings must follow the `numpy` style 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 by `FIXME:` or a ticket link, and used only as a last resort. - Use `pytest` fixtures for shared setup (`conftest.py` or `fixtures/`) - Replace duplicate tests with `@pytest.mark.parametrize` - Prefer `pytest-mock` or `unittest.mock` for 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/case` or 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_render_diagram.pynixie/unittests/test_verbose.pynixie/cli.py
**/unittests/test_*.py
📄 CodeRabbit Inference Engine (.rules/python-00.mdc)
Colocate unit tests with code using a unittests subdirectory and a test_ prefix for test files
Files:
nixie/unittests/test_render_diagram.pynixie/unittests/test_verbose.py
**/test_*.py
📄 CodeRabbit Inference Engine (.rules/python-00.mdc)
**/test_*.py: Use pytest idioms in test files: prefer fixtures over setup/teardown methods, parametrize broadly, and avoid unnecessary mocks
Group related tests using class with method names prefixed by test_ in test files
Files:
nixie/unittests/test_render_diagram.pynixie/unittests/test_verbose.py
🪛 Ruff (0.12.2)
nixie/cli.py
108-108: Async function definition with a timeout parameter
(ASYNC109)
108-108: Trailing comma missing
Add trailing comma
(COM812)
127-127: Async function definition with a timeout parameter
(ASYNC109)
130-130: Avoid specifying long messages outside the exception class
(TRY003)
187-187: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (4)
nixie/unittests/test_verbose.py (1)
56-56: Rename aligns with behaviour (INFO vs WARNING). Approve.Keep the test name focused on logging level rather than a verbose flag; it matches the current design.
nixie/unittests/test_render_diagram.py (1)
78-84: Validate executable whitelist via negative test. Approve.Assert
ValueErrorfor an unexpected executable to lock down the whitelist; this complements runtime safeguards.nixie/cli.py (2)
16-16: Alias asyncio.subprocess for clearer typing. Approve.Use
asyncio_subprocessin annotations and calls to improve clarity and Pyright compatibility.
190-191: Centralise subprocess handling in helper. Approve.Delegate process lifecycle to
_run_mermaid_clito keep_render_diagramfocused on I/O and error reporting.
| async def _run_mermaid_cli( | ||
| cmd: list[str], | ||
| sem: asyncio.Semaphore, | ||
| path: Path, | ||
| idx: int, | ||
| timeout: float, | ||
| ) -> tuple[bool, bytes]: | ||
| if not cmd or cmd[0] not in ALLOWED_EXECUTABLES: | ||
| raise ValueError(f"Unexpected executable: {cmd[0] if cmd else ''}") | ||
|
|
||
| async with sem: | ||
| proc = await asyncio.create_subprocess_exec( # nosemgrep: python.lang.security.audit.dangerous-asyncio-create-exec-audit | ||
| *cmd, | ||
| stdout=asyncio_subprocess.PIPE, | ||
| stderr=asyncio_subprocess.PIPE, | ||
| ) | ||
|
|
||
| return await wait_for_proc(proc, path, idx, timeout) | ||
|
|
There was a problem hiding this comment.
Hold the semaphore until the process completes.
Do not release the semaphore immediately after spawning the process; this defeats concurrency limiting for long-running tasks. Guard both creation and wait within the same async with block.
async def _run_mermaid_cli(
@@
- async with sem:
- proc = await asyncio.create_subprocess_exec( # nosemgrep: python.lang.security.audit.dangerous-asyncio-create-exec-audit
- *cmd,
- stdout=asyncio_subprocess.PIPE,
- stderr=asyncio_subprocess.PIPE,
- )
-
- return await wait_for_proc(proc, path, idx, timeout)
+ async with sem:
+ proc = await asyncio.create_subprocess_exec( # nosemgrep: python.lang.security.audit.dangerous-asyncio-create-exec-audit
+ *cmd,
+ stdout=asyncio_subprocess.PIPE,
+ stderr=asyncio_subprocess.PIPE,
+ )
+ return await wait_for_proc(proc, path, idx, timeout)📝 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.
| async def _run_mermaid_cli( | |
| cmd: list[str], | |
| sem: asyncio.Semaphore, | |
| path: Path, | |
| idx: int, | |
| timeout: float, | |
| ) -> tuple[bool, bytes]: | |
| if not cmd or cmd[0] not in ALLOWED_EXECUTABLES: | |
| raise ValueError(f"Unexpected executable: {cmd[0] if cmd else ''}") | |
| async with sem: | |
| proc = await asyncio.create_subprocess_exec( # nosemgrep: python.lang.security.audit.dangerous-asyncio-create-exec-audit | |
| *cmd, | |
| stdout=asyncio_subprocess.PIPE, | |
| stderr=asyncio_subprocess.PIPE, | |
| ) | |
| return await wait_for_proc(proc, path, idx, timeout) | |
| async def _run_mermaid_cli( | |
| cmd: list[str], | |
| sem: asyncio.Semaphore, | |
| path: Path, | |
| idx: int, | |
| timeout: float, | |
| ) -> tuple[bool, bytes]: | |
| if not cmd or cmd[0] not in ALLOWED_EXECUTABLES: | |
| raise ValueError(f"Unexpected executable: {cmd[0] if cmd else ''}") | |
| async with sem: | |
| proc = await asyncio.create_subprocess_exec( # nosemgrep: python.lang.security.audit.dangerous-asyncio-create-exec-audit | |
| *cmd, | |
| stdout=asyncio_subprocess.PIPE, | |
| stderr=asyncio_subprocess.PIPE, | |
| ) | |
| return await wait_for_proc(proc, path, idx, timeout) |
🧰 Tools
🪛 Ruff (0.12.2)
127-127: Async function definition with a timeout parameter
(ASYNC109)
130-130: Avoid specifying long messages outside the exception class
(TRY003)
🤖 Prompt for AI Agents
In nixie/cli.py around lines 122 to 140, the semaphore is released immediately
after starting the subprocess, which allows more concurrent tasks than intended.
To fix this, move the await wait_for_proc call inside the async with sem block
so that the semaphore is held until the subprocess completes, properly limiting
concurrency for long-running tasks.
There was a problem hiding this comment.
🧩 Analysis chain
@leynos I've created a GitHub issue to track the semaphore concurrency problem identified in the _run_mermaid_cli function. The issue has been assigned to you and includes references to this PR and comment thread.
🏁 Script executed:
#!/bin/bash
gh issue create \
--title "Semaphore released too early in _run_mermaid_cli function defeats concurrency limiting" \
--body "## Issue Description
The \`_run_mermaid_cli\` function in \`nixie/cli.py\` releases the semaphore immediately after spawning the subprocess, which defeats the concurrency limiting mechanism for long-running tasks.
## Current Behaviour
The semaphore is held only during subprocess creation but released before waiting for the process to complete:
\`\`\`python
async with sem:
proc = await asyncio.create_subprocess_exec(...)
return await wait_for_proc(proc, path, idx, timeout)
\`\`\`
## Expected Behaviour
The semaphore should be held until the subprocess completes to properly limit concurrency:
\`\`\`python
async with sem:
proc = await asyncio.create_subprocess_exec(...)
return await wait_for_proc(proc, path, idx, timeout)
\`\`\`
## Impact
This allows more concurrent tasks than intended, potentially overwhelming system resources when processing multiple diagrams.
## References
- PR: https://github.com/leynos/nixie/pull/11
- Comment: https://github.com/leynos/nixie/pull/11#discussion_r2261612240
- Requested by: @leynos" \
--assignee leynos
Length of output: 1220
@leynos Perfect! I've successfully created GitHub issue #16 for the semaphore concurrency problem. You can view it at: #16
The issue has been assigned to you and includes a detailed description of the problem, the current vs expected behaviour, impact assessment, and references back to this PR and comment thread.
🧬 Code Graph Analysis Results
Relevant snippet from nixie/unittests/test_get_mmdc_cmd.py lines 22-25:
def which(cmd: str) -> str | None:
if cmd == "bun":
return "/usr/bin/bun"
return NoneThis snippet provides a mock implementation of the which function that returns a path for the "bun" executable and None otherwise. It is relevant for understanding or testing the executable selection logic in nixie/cli.py where shutil.which is used to detect available executables (mmdc, bun, npx).
1 participant
Summary
verbosearg fromrender_blockasyncio.subprocessTesting
make fmtmake lintmake typecheckmake testhttps://chatgpt.com/codex/tasks/task_e_6894ef14e6a083229493ae05c0a8684e
Summary by Sourcery
Clean up render_block by dropping the unused verbose argument, updating subprocess type imports, and renaming the test to match logging behavior
Enhancements:
Tests: