Allow Windows mmdc executable - #28
Conversation
Reviewer's GuideThis PR extends the mermaid CLI runner to recognize Windows executable shims by normalizing file names (stripping .exe/.cmd/.bat suffixes and lowercasing) and updating the allow-list check, plus adding a regression test to ensure .EXE paths are accepted. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Note Reviews pausedUse the following commands to manage reviews:
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
WalkthroughAdd Windows executable suffix handling and executable-name normalisation; validate mermaid CLI names via a new allow-list check and raise UnexpectedExecutableError for disallowed executables. Add unit and integration tests covering Windows-style shims and normalization logic. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller
participant CLI as _run_mermaid_cli
participant Norm as _normalize_executable_name
participant Allow as _is_allowed_executable
participant Proc as Subprocess
Caller->>CLI: invoke with executable path
CLI->>Norm: normalise (PureWindowsPath, strip .exe/.cmd/.bat, lower-case)
Norm-->>CLI: canonical name
CLI->>Allow: check canonical name against allowlist
alt allowed
CLI->>Proc: create_subprocess_exec(...) and run
Proc-->>CLI: process result
CLI-->>Caller: return success
else not allowed
CLI-->>Caller: raise UnexpectedExecutableError / return failure
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Comment |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
nixie/cli.py(2 hunks)nixie/unittests/test_render_diagram.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Python changes must pass tests (unit and behavioral) before completion/commit
Python code must pass lint checks (make lint)
Python code must adhere to formatting standards (make check-fmt/make fmt)
Python code must pass type checking (make typecheck)
Follow core Python 3.13 style conventions per .rules/python-00.md
Apply best practices for context managers per .rules/python-context-managers.md
Follow generator and iterator patterns per .rules/python-generators.md
Follow function return conventions per .rules/python-return.md
Apply Python typing best practices per .rules/python-typing.md
**/*.py: Name Python files in snake_case (e.g., http_client.py, task_queue.py)
Classes must use PascalCase
Variables and functions must use snake_case
Module-level constants use UPPER_SNAKE_CASE
Prefix non-exported helpers or internal APIs with a single underscore
Use typing everywhere; maintain full static type coverage with Pyright
Use TypedDict or @DataClass for structured data; prefer @DataClass(slots=True) for internal-only
Avoid Any; use Unknown, generics, or cast() with justification if Any is used
Be explicit with return types for all public functions and class methods (e.g., -> None, -> str)
Favor immutability (prefer tuples to lists; use frozendict or types.MappingProxyType where appropriate)
Use# pyright: ignoresparingly and include an explanation when used
Avoid side effects at import time; modules should not modify global state or perform actions on import
Never hardcode secrets in source code
Write NumPy-style docstrings for public functions, classes, and modules
Add inline comments to explain non-obvious logic or decisions
**/*.py: Use context managers to encapsulate setup/teardown for resources (files, locks, connections) instead of manual management
Use @contextmanager from contextlib for straightforward, linear setup/teardown without persistent internal state
Implement a class-based context manager (enter/exit) when there is internal sta...
Files:
nixie/unittests/test_render_diagram.pynixie/cli.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 / -yse / -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_render_diagram.pynixie/cli.py
**/unittests/test_*.py
📄 CodeRabbit inference engine (.rules/python-00.md)
Colocate unit tests with code in an unittests/ subdirectory using files prefixed with test_
Files:
nixie/unittests/test_render_diagram.py
{**/unittests/test_*.py,tests/integration/test_*.py}
📄 CodeRabbit inference engine (.rules/python-00.md)
{**/unittests/test_*.py,tests/integration/test_*.py}: Use pytest idioms: prefer fixtures, parametrize broadly, avoid unnecessary mocks
Group related tests using classes with method names prefixed by test_
Write tests from a user's perspective; test public behaviour, not internals
Avoid excessive mocking; use doubles only for external services or non-deterministic behaviour
Files:
nixie/unittests/test_render_diagram.py
🧬 Code graph analysis (1)
nixie/unittests/test_render_diagram.py (1)
nixie/unittests/test_verbose.py (7)
fake_create_subprocess_exec(45-46)fake_create_subprocess_exec(79-80)fake_create_subprocess_exec(130-131)fake_create_subprocess_exec(164-165)fake_wait_for_proc(48-51)fake_wait_for_proc(82-85)fake_wait_for_proc(133-136)
🔍 Remote MCP Ref
Summary — review-relevant facts (concise)
-
Change set (branch codex/allow-mmdc.exe-execution-in-nixie): adds WINDOWS_EXECUTABLE_SUFFIXES (".exe", ".cmd", ".bat"); introduces _normalize_executable_name(executable) that returns the basename lowercased with Windows suffixes stripped; introduces _is_allowed_executable(executable) that checks the normalized name against ALLOWED_EXECUTABLES; _run_mermaid_cli(...) now uses _is_allowed_executable(...) and raises UnexpectedExecutableError for disallowed executables. [file: nixie/cli.py — branch]
-
Same file on main (for comparison): CLI-executable validation previously checked against ALLOWED_EXECUTABLES without normalizing Windows suffixes; the branch change is limited to private helpers and does not alter public API signatures. [file: nixie/cli.py — main]
-
Tests: PR adds a regression unit test ensuring _run_mermaid_cli accepts a Windows mmdc.EXE shim (test: test_run_mermaid_cli_accepts_windows_executable in nixie/unittests/test_render_diagram.py). (PR metadata / test summary) [PR: https://github.com//pull/28]
Sources / tool calls used
- https://raw.githubusercontent.com/leynos/nixie/codex/allow-mmdc.exe-execution-in-nixie/nixie/cli.py — inspected with Ref_ref_read_url
- https://raw.githubusercontent.com/leynos/nixie/main/nixie/cli.py — inspected with Ref_ref_read_url
- #28 — PR metadata referenced with Ref_ref_read_url
⏰ Context from checks skipped due to timeout of 120000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Sourcery review
🔇 Additional comments (2)
nixie/cli.py (2)
90-90: Define Windows suffixes for normalisation — LGTM.
351-357: Allow-list check reads cleanly — LGTM.
|
@coderabbitai have the following now been resolved?
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainBased on the code examination, I can see the current state of the implementation: @leynos, neither of the suggested improvements have been implemented yet: Issue 1:
|
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
nixie/cli.py(3 hunks)nixie/unittests/test_render_diagram.py(2 hunks)tests/integration/test_windows_executables.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Python changes must pass tests (unit and behavioral) before completion/commit
Python code must pass lint checks (make lint)
Python code must adhere to formatting standards (make check-fmt/make fmt)
Python code must pass type checking (make typecheck)
Follow core Python 3.13 style conventions per .rules/python-00.md
Apply best practices for context managers per .rules/python-context-managers.md
Follow generator and iterator patterns per .rules/python-generators.md
Follow function return conventions per .rules/python-return.md
Apply Python typing best practices per .rules/python-typing.md
**/*.py: Name Python files in snake_case (e.g., http_client.py, task_queue.py)
Classes must use PascalCase
Variables and functions must use snake_case
Module-level constants use UPPER_SNAKE_CASE
Prefix non-exported helpers or internal APIs with a single underscore
Use typing everywhere; maintain full static type coverage with Pyright
Use TypedDict or @DataClass for structured data; prefer @DataClass(slots=True) for internal-only
Avoid Any; use Unknown, generics, or cast() with justification if Any is used
Be explicit with return types for all public functions and class methods (e.g., -> None, -> str)
Favor immutability (prefer tuples to lists; use frozendict or types.MappingProxyType where appropriate)
Use# pyright: ignoresparingly and include an explanation when used
Avoid side effects at import time; modules should not modify global state or perform actions on import
Never hardcode secrets in source code
Write NumPy-style docstrings for public functions, classes, and modules
Add inline comments to explain non-obvious logic or decisions
**/*.py: Use context managers to encapsulate setup/teardown for resources (files, locks, connections) instead of manual management
Use @contextmanager from contextlib for straightforward, linear setup/teardown without persistent internal state
Implement a class-based context manager (enter/exit) when there is internal sta...
Files:
nixie/cli.pynixie/unittests/test_render_diagram.pytests/integration/test_windows_executables.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 / -yse / -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.- All path manipulation must be performed using pathlib for cross platform safety. Do not use string manipulation for extraction of path components or filename elements.
Files:
nixie/cli.pynixie/unittests/test_render_diagram.pytests/integration/test_windows_executables.py
**/unittests/test_*.py
📄 CodeRabbit inference engine (.rules/python-00.md)
Colocate unit tests with code in an unittests/ subdirectory using files prefixed with test_
Files:
nixie/unittests/test_render_diagram.py
{**/unittests/test_*.py,tests/integration/test_*.py}
📄 CodeRabbit inference engine (.rules/python-00.md)
{**/unittests/test_*.py,tests/integration/test_*.py}: Use pytest idioms: prefer fixtures, parametrize broadly, avoid unnecessary mocks
Group related tests using classes with method names prefixed by test_
Write tests from a user's perspective; test public behaviour, not internals
Avoid excessive mocking; use doubles only for external services or non-deterministic behaviour
Files:
nixie/unittests/test_render_diagram.pytests/integration/test_windows_executables.py
tests/integration/test_*.py
📄 CodeRabbit inference engine (.rules/python-00.md)
Place integration tests under tests/integration/ with files prefixed with test_
Files:
tests/integration/test_windows_executables.py
🧠 Learnings (1)
📚 Learning: 2025-09-23T06:00:13.627Z
Learnt from: leynos
PR: leynos/nixie#28
File: nixie/cli.py:340-349
Timestamp: 2025-09-23T06:00:13.627Z
Learning: In the nixie project, path manipulation must use pathlib rather than string manipulation to extract path components or filename elements.
Applied to files:
nixie/cli.py
🧬 Code graph analysis (2)
nixie/unittests/test_render_diagram.py (1)
nixie/cli.py (4)
cli(597-614)_is_allowed_executable(356-361)_normalize_executable_name(340-353)_run_mermaid_cli(364-380)
tests/integration/test_windows_executables.py (2)
nixie/cli.py (2)
cli(597-614)render_block(434-508)nixie/unittests/test_render_diagram.py (6)
fake_create_subprocess_exec(36-37)fake_create_subprocess_exec(70-71)fake_create_subprocess_exec(110-111)fake_wait_for_proc(39-42)fake_wait_for_proc(73-76)fake_wait_for_proc(113-116)
🔍 Remote MCP Ref
Summary of additional review-relevant facts
-
Files inspected:
- PR overview: #28
- Branch (changes): nixie/cli.py in branch codex/allow-mmdc.exe-execution-in-nixie — added WINDOWS_EXECUTABLE_SUFFIXES, _normalize_executable_name(executable), _is_allowed_executable(executable), and switched _run_mermaid_cli to use _is_allowed_executable (strips Windows suffixes, lowercases basename using PureWindowsPath)
- Main (base) nixie/cli.py for comparison (pre-change)
- New/updated unit tests exercising normalization and allow-list behavior: nixie/unittests/test_render_diagram.py (branch)
-
Concrete, review-relevant behaviors to check
- Normalization: the new helper takes the executable path, uses PureWindowsPath to obtain the name, strips any of the suffixes in WINDOWS_EXECUTABLE_SUFFIXES (".exe", ".cmd", ".bat"), and lowercases the result before comparing to ALLOWED_EXECUTABLES. Verify this covers mixed-case suffixes and paths with Windows separators.
- Allow-list check: _is_allowed_executable checks the normalized name against ALLOWED_EXECUTABLES (so only names like "mmdc" will pass). Confirm ALLOWED_EXECUTABLES indeed contains the expected launcher names (e.g., "mmdc").
- Error handling: _run_mermaid_cli now raises UnexpectedExecutableError if _is_allowed_executable returns False — ensure calling code treats that as a rejection and that tests/mock coverage cover the early-exit path (unit and integration tests exercise accept/reject).
- Cross-platform concerns: confirm PureWindowsPath usage does not mis-handle POSIX-only paths in non-Windows environments (tests exercise Windows-like paths via mocks).
-
Tests added/updated you should run or inspect:
- Unit tests in nixie/unittests/test_render_diagram.py validating:
- _normalize_executable_name returns lowercased basename with Windows suffix stripped
- _is_allowed_executable accepts known names and rejects unknowns
- _run_mermaid_cli accepts Windows-style shim names (mmdc.EXE/.CMD/.BAT)
- (PR includes integration tests per PR description — ensure CI runs tests that mock shutil.which and subprocess creation to validate end-to-end behavior.)
- Unit tests in nixie/unittests/test_render_diagram.py validating:
Tools / sources used
- Ref_ref_read_url: PR page and files inspected,,,
⏰ Context from checks skipped due to timeout of 120000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Sourcery review
🔇 Additional comments (8)
nixie/cli.py (5)
34-34: Adopt pathlib for cross‑platform path handling.Importing PureWindowsPath alongside Path is the right call for Windows‑style separators.
90-91: Add explicit Windows suffix allow‑list.The constant is correct and typed. This pairs well with the normalisation helper.
340-354: Normalise executable names with pathlib (POSIX + Windows).The logic is sound: prefer PureWindowsPath when backslashes are present, lower‑case, and strip known Windows suffixes.
356-362: Gate execution via allow‑listed, normalised names.Looks correct and defensive.
370-372: Reuse the computed variable in the exception.Avoid re‑indexing cmd; pass executable directly (matches prior feedback).
- executable = cmd[0] if cmd else "" - if not _is_allowed_executable(executable): - raise UnexpectedExecutableError(cmd[0] if cmd else "") + executable = cmd[0] if cmd else "" + if not _is_allowed_executable(executable): + raise UnexpectedExecutableError(executable)nixie/unittests/test_render_diagram.py (3)
164-170: Fix raw string in negative cases as well.Maintain the same correction for reject‑list tests.
- ["", "python", "./mmdc.sh", r"C:\\tools\\mermaid.cmdx"], + ["", "python", "./mmdc.sh", r"C:\tools\mermaid.cmdx"],
126-144: Correct Windows raw string literals in parametrised cases.Use single backslashes in r/rf‑strings.
- (r"C:\\Users\\runneradmin\\.bun\\bin\\mmdc.EXE", "mmdc"), + (r"C:\Users\runneradmin\.bun\bin\mmdc.EXE", "mmdc"),
100-124: Fix raw string paths; don’t double‑escape backslashes.Raw strings already treat backslashes literally. Use single backslashes for Windows‑style literals.
- cmd = [rf"C:\\Users\\runneradmin\\.bun\\bin\\mmdc{suffix.upper()}", "--version"] + cmd = [rf"C:\Users\runneradmin\.bun\bin\mmdc{suffix.upper()}", "--version"]
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
nixie/unittests/test_render_diagram.py (1)
164-171: Fix raw‑string Windows path in rejection cases.Use single backslashes in the raw string.
Apply this diff:
- ["", "python", "./mmdc.sh", r"C:\\tools\\mermaid.cmdx"], + ["", "python", "./mmdc.sh", r"C:\tools\mermaid.cmdx"],
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
nixie/cli.py(3 hunks)nixie/unittests/test_render_diagram.py(2 hunks)tests/integration/test_windows_executables.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Python changes must pass tests (unit and behavioral) before completion/commit
Python code must pass lint checks (make lint)
Python code must adhere to formatting standards (make check-fmt/make fmt)
Python code must pass type checking (make typecheck)
Follow core Python 3.13 style conventions per .rules/python-00.md
Apply best practices for context managers per .rules/python-context-managers.md
Follow generator and iterator patterns per .rules/python-generators.md
Follow function return conventions per .rules/python-return.md
Apply Python typing best practices per .rules/python-typing.md
**/*.py: Name Python files in snake_case (e.g., http_client.py, task_queue.py)
Classes must use PascalCase
Variables and functions must use snake_case
Module-level constants use UPPER_SNAKE_CASE
Prefix non-exported helpers or internal APIs with a single underscore
Use typing everywhere; maintain full static type coverage with Pyright
Use TypedDict or @DataClass for structured data; prefer @DataClass(slots=True) for internal-only
Avoid Any; use Unknown, generics, or cast() with justification if Any is used
Be explicit with return types for all public functions and class methods (e.g., -> None, -> str)
Favor immutability (prefer tuples to lists; use frozendict or types.MappingProxyType where appropriate)
Use# pyright: ignoresparingly and include an explanation when used
Avoid side effects at import time; modules should not modify global state or perform actions on import
Never hardcode secrets in source code
Write NumPy-style docstrings for public functions, classes, and modules
Add inline comments to explain non-obvious logic or decisions
**/*.py: Use context managers to encapsulate setup/teardown for resources (files, locks, connections) instead of manual management
Use @contextmanager from contextlib for straightforward, linear setup/teardown without persistent internal state
Implement a class-based context manager (enter/exit) when there is internal sta...
Files:
nixie/cli.pytests/integration/test_windows_executables.pynixie/unittests/test_render_diagram.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 / -yse / -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.- All path manipulation must be performed using pathlib for cross platform safety. Do not use string manipulation for extraction of path components or filename elements.
Files:
nixie/cli.pytests/integration/test_windows_executables.pynixie/unittests/test_render_diagram.py
tests/integration/test_*.py
📄 CodeRabbit inference engine (.rules/python-00.md)
Place integration tests under tests/integration/ with files prefixed with test_
Files:
tests/integration/test_windows_executables.py
{**/unittests/test_*.py,tests/integration/test_*.py}
📄 CodeRabbit inference engine (.rules/python-00.md)
{**/unittests/test_*.py,tests/integration/test_*.py}: Use pytest idioms: prefer fixtures, parametrize broadly, avoid unnecessary mocks
Group related tests using classes with method names prefixed by test_
Write tests from a user's perspective; test public behaviour, not internals
Avoid excessive mocking; use doubles only for external services or non-deterministic behaviour
Files:
tests/integration/test_windows_executables.pynixie/unittests/test_render_diagram.py
**/unittests/test_*.py
📄 CodeRabbit inference engine (.rules/python-00.md)
Colocate unit tests with code in an unittests/ subdirectory using files prefixed with test_
Files:
nixie/unittests/test_render_diagram.py
🧠 Learnings (1)
📚 Learning: 2025-09-23T06:00:13.627Z
Learnt from: leynos
PR: leynos/nixie#28
File: nixie/cli.py:340-349
Timestamp: 2025-09-23T06:00:13.627Z
Learning: In the nixie project, path manipulation must use pathlib rather than string manipulation to extract path components or filename elements.
Applied to files:
nixie/cli.py
🧬 Code graph analysis (2)
tests/integration/test_windows_executables.py (2)
nixie/cli.py (2)
cli(597-614)render_block(434-508)nixie/unittests/test_render_diagram.py (6)
fake_create_subprocess_exec(36-37)fake_create_subprocess_exec(70-71)fake_create_subprocess_exec(110-111)fake_wait_for_proc(39-42)fake_wait_for_proc(73-76)fake_wait_for_proc(113-116)
nixie/unittests/test_render_diagram.py (1)
nixie/cli.py (6)
cli(597-614)_is_allowed_executable(356-361)_normalize_executable_name(340-353)_render_diagram(383-431)_run_mermaid_cli(364-380)get_mmdc_cmd(266-309)
🔍 Remote MCP Ref
Summary of additional facts relevant to reviewing PR #28
-
WINDOWS_EXECUTABLE_SUFFIXES exists and equals (".exe", ".cmd", ".bat"); normalization strips these suffixes before allow-list checking.
-
_normalize_executable_name(executable: str) implementation:
- returns "" for falsy input,
- uses backslash detection to detect Windows-style paths,
- uses pathlib.Path(executable).suffix and .stem and returns stem.lower() when suffix is one of WINDOWS_EXECUTABLE_SUFFIXES, otherwise returns Path(executable).name.lower().
- therefore it handles POSIX and Windows paths and normalizes case.
-
_is_allowed_executable(executable: str) calls _normalize_executable_name and checks membership against ALLOWED_EXECUTABLES (unchanged list of allowed launcher names). [_run_mermaid_cli now uses this check and raises UnexpectedExecutableError for disallowed names.]
-
Comparison to main branch: main branch prior to this PR checked raw executable names against ALLOWED_EXECUTABLES (no Windows-suffix normalization). The change is confined to private helpers; public API signatures unchanged.
-
Integration test file for Windows executables was present on the feature branch (verifies accept/reject behavior for Windows shim executables). (Note: one attempt to read the integration-test URL returned a 402; branch CLI file and main CLI file reads succeeded.)
Relevant review implications (concise)
- Normalization uses pathlib.Path.stem — good for correctness; verify behavior for inputs with multiple suffixes (e.g., ".tar.gz") is not relevant here because only known Windows suffixes are stripped.
- Confirm ALLOWED_EXECUTABLES contains the expected launcher names (unchanged by PR).
- Ensure tests cover all WINDOWS_EXECUTABLE_SUFFIXES (PR metadata indicates parameterization over the suffixes).
Sources/tools used
- Branch nixie/cli.py (feature branch):
- Main branch nixie/cli.py:
- Integration test file (feature branch; one read attempt returned 402):
⏰ Context from checks skipped due to timeout of 120000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Sourcery review
🔇 Additional comments (10)
nixie/cli.py (6)
34-34: Use pathlib import: LGTM.Import
PathandPureWindowsPathto keep path handling robust and cross‑platform.
91-91: Expose Windows suffixes as a typed constant: LGTM.The tuple with lower‑cased suffixes aligns with the normalisation logic.
340-354: Executable normalisation is correct and cross‑platform.Choosing
PureWindowsPathon backslashes and stripping known Windows suffixes is sound.
356-362: Allow‑list check is minimal and correct.Return early for falsy inputs and compare against the canonical names.
370-373: Reuse of computed variable in the exception: LGTM.Avoid re‑indexing
cmdand pass the cachedexecutable.
340-343: Align docstring with project spelling (en‑GB, Oxford).Standardize to “normalised” and “allow‑listing” to match the codebase spelling rules.
Apply this diff:
-def _normalize_executable_name(executable: str) -> str: - """Return a normalized name for ``executable`` suitable for allow-listing.""" +def _normalize_executable_name(executable: str) -> str: + """Return a normalised name for ``executable`` suitable for allow‑listing."""nixie/unittests/test_render_diagram.py (4)
91-98: Reject unexpected executables: LGTM.The test asserts the guarded path without spawning a process.
146-162: Fix raw‑string Windows paths here as well.Ensure consistency with the previous correction.
Apply this diff:
- r"C:\\Users\\runneradmin\\.bun\\bin\\mmdc.EXE", + r"C:\Users\runneradmin\.bun\bin\mmdc.EXE",
126-144: Tighten normalisation cases; fix raw‑string backslashes.Keep the cases; correct the Windows raw‑string literals.
Apply this diff:
- (r"C:\\Users\\runneradmin\\.bun\\bin\\mmdc.EXE", "mmdc"), + (r"C:\Users\runneradmin\.bun\bin\mmdc.EXE", "mmdc"),
100-124: Fix Windows raw‑string paths: remove redundant backslash escapes.In raw strings, backslashes are literal;
\\becomes two backslashes in the value. Use single backslashes forrstrings.Apply this diff:
- cmd = [rf"C:\\Users\\runneradmin\\.bun\\bin\\mmdc{suffix.upper()}", "--version"] + cmd = [rf"C:\Users\runneradmin\.bun\bin\mmdc{suffix.upper()}", "--version"]
|
@coderabbitai pause |
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 (5)
nixie/cli.py(3 hunks)nixie/unittests/conftest.py(1 hunks)nixie/unittests/test_cli_executable_allowlist.py(1 hunks)nixie/unittests/test_render_diagram.py(4 hunks)tests/integration/test_windows_executables.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Python changes must pass tests (unit and behavioral) before completion/commit
Python code must pass lint checks (make lint)
Python code must adhere to formatting standards (make check-fmt/make fmt)
Python code must pass type checking (make typecheck)
Follow core Python 3.13 style conventions per .rules/python-00.md
Apply best practices for context managers per .rules/python-context-managers.md
Follow generator and iterator patterns per .rules/python-generators.md
Follow function return conventions per .rules/python-return.md
Apply Python typing best practices per .rules/python-typing.md
**/*.py: Name Python files in snake_case (e.g., http_client.py, task_queue.py)
Classes must use PascalCase
Variables and functions must use snake_case
Module-level constants use UPPER_SNAKE_CASE
Prefix non-exported helpers or internal APIs with a single underscore
Use typing everywhere; maintain full static type coverage with Pyright
Use TypedDict or @DataClass for structured data; prefer @DataClass(slots=True) for internal-only
Avoid Any; use Unknown, generics, or cast() with justification if Any is used
Be explicit with return types for all public functions and class methods (e.g., -> None, -> str)
Favor immutability (prefer tuples to lists; use frozendict or types.MappingProxyType where appropriate)
Use# pyright: ignoresparingly and include an explanation when used
Avoid side effects at import time; modules should not modify global state or perform actions on import
Never hardcode secrets in source code
Write NumPy-style docstrings for public functions, classes, and modules
Add inline comments to explain non-obvious logic or decisions
**/*.py: Use context managers to encapsulate setup/teardown for resources (files, locks, connections) instead of manual management
Use @contextmanager from contextlib for straightforward, linear setup/teardown without persistent internal state
Implement a class-based context manager (enter/exit) when there is internal sta...
Files:
nixie/unittests/test_cli_executable_allowlist.pynixie/unittests/test_render_diagram.pynixie/unittests/conftest.pytests/integration/test_windows_executables.pynixie/cli.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 / -yse / -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.- All path manipulation must be performed using pathlib for cross platform safety. Do not use string manipulation for extraction of path components or filename elements.
Files:
nixie/unittests/test_cli_executable_allowlist.pynixie/unittests/test_render_diagram.pynixie/unittests/conftest.pytests/integration/test_windows_executables.pynixie/cli.py
**/unittests/test_*.py
📄 CodeRabbit inference engine (.rules/python-00.md)
Colocate unit tests with code in an unittests/ subdirectory using files prefixed with test_
Files:
nixie/unittests/test_cli_executable_allowlist.pynixie/unittests/test_render_diagram.py
{**/unittests/test_*.py,tests/integration/test_*.py}
📄 CodeRabbit inference engine (.rules/python-00.md)
{**/unittests/test_*.py,tests/integration/test_*.py}: Use pytest idioms: prefer fixtures, parametrize broadly, avoid unnecessary mocks
Group related tests using classes with method names prefixed by test_
Write tests from a user's perspective; test public behaviour, not internals
Avoid excessive mocking; use doubles only for external services or non-deterministic behaviour
Files:
nixie/unittests/test_cli_executable_allowlist.pynixie/unittests/test_render_diagram.pytests/integration/test_windows_executables.py
tests/integration/test_*.py
📄 CodeRabbit inference engine (.rules/python-00.md)
Place integration tests under tests/integration/ with files prefixed with test_
Files:
tests/integration/test_windows_executables.py
🧠 Learnings (1)
📚 Learning: 2025-09-23T06:00:13.627Z
Learnt from: leynos
PR: leynos/nixie#28
File: nixie/cli.py:340-349
Timestamp: 2025-09-23T06:00:13.627Z
Learning: In the nixie project, path manipulation must use pathlib rather than string manipulation to extract path components or filename elements.
Applied to files:
nixie/cli.py
🧬 Code graph analysis (3)
nixie/unittests/test_cli_executable_allowlist.py (1)
nixie/cli.py (3)
cli(600-617)_normalize_executable_name(340-356)_is_allowed_executable(359-364)
nixie/unittests/test_render_diagram.py (1)
nixie/cli.py (4)
cli(600-617)_render_diagram(386-434)_run_mermaid_cli(367-383)get_mmdc_cmd(266-309)
tests/integration/test_windows_executables.py (2)
nixie/cli.py (2)
cli(600-617)render_block(437-511)nixie/unittests/test_render_diagram.py (6)
fake_create_subprocess_exec(32-33)fake_create_subprocess_exec(68-69)fake_create_subprocess_exec(110-111)fake_wait_for_proc(35-38)fake_wait_for_proc(71-74)fake_wait_for_proc(113-116)
🔍 Remote MCP Ref
Summary of additional facts relevant to reviewing PR #28
-
WINDOWS_EXECUTABLE_SUFFIXES: the branch introduces WINDOWS_EXECUTABLE_SUFFIXES = (".exe", ".cmd", ".bat") and uses it to strip Windows suffixes before allow-list checking.
-
_normalize_executable_name behavior (branch):
- Returns "" for falsy input.
- Detects Windows-style paths (backslash) and uses pathlib.Path to inspect suffix and stem.
- If suffix is in WINDOWS_EXECUTABLE_SUFFIXES returns Path(...).stem.lower(), otherwise returns Path(...).name.lower().
- This handles Windows shims like "mmdc.EXE" -> "mmdc".
-
_is_allowed_executable behavior (branch):
- Calls _normalize_executable_name and checks membership against ALLOWED_EXECUTABLES; _run_mermaid_cli raises UnexpectedExecutableError for disallowed executables.
-
Tests added/updated:
- Unit tests for normalization and allowlist logic added in nixie/unittests/test_cli_executable_allowlist.py covering multiple suffixes and path forms.
- Existing mermaid CLI tests updated to patch via nixie.cli namespace and parameterized over WINDOWS_EXECUTABLE_SUFFIXES (ensures .exe/.cmd/.bat are tested). (Referenced in branch test files.)
Review implications / attention points
- Confirm ALLOWED_EXECUTABLES still includes the expected launcher names (unchanged by PR) so normalized names match allowlist — verify in repo main if needed.
- Verify normalization edge cases: inputs with multiple suffixes (e.g., "mmdc.exe.bak") should not be allowed; tests already cover such rejections.
- Ensure CI runs full test suite (unit + integration) on platforms/CI matrix that simulate Windows-like paths as tests rely on path normalization and monkeypatching.
Sources
- Branch nixie/cli.py (feature branch)
- Branch nixie/unittests/test_cli_executable_allowlist.py
⏰ Context from checks skipped due to timeout of 120000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Sourcery review
🔇 Additional comments (17)
nixie/unittests/conftest.py (1)
21-21: LGTM! Necessary patch for consistent test isolation.The additional patch to
nixie.cli.Path.homeensures that both the globalPath.homeand the module-specific reference return the same temporary home directory, preventing any inconsistencies in tests that rely on the nixie.cli module's path operations.nixie/unittests/test_render_diagram.py (4)
11-16: LGTM! Proper use of publicly exposed constant.The import of
WINDOWS_EXECUTABLE_SUFFIXESfromnixie.cliis appropriate as it's now a public constant, and the test properly uses it for parametrisation.
40-44: LGTM! Correct module-scoped patching.Patching via the
nixie.clinamespace paths ensures changes are localised to the module under test, avoiding global side effects.
76-80: Maintain consistency in patching approach.Both test functions patch
nixie.cli.asyncio.create_subprocess_execandnixie.cli.shutil.whichvia the module namespace for consistency.
100-125: LGTM! Comprehensive Windows executable suffix testing.The parametrised test effectively validates all Windows executable suffixes defined in
WINDOWS_EXECUTABLE_SUFFIXES. The raw string with single backslashes is correctly formatted for Windows paths.nixie/cli.py (5)
34-34: LGTM! Appropriate import for Windows path handling.Adding
PureWindowsPathalongsidePathenables proper cross-platform Windows path normalisation without requiring Windows-specific runtime checks.
90-90: LGTM! Well-defined Windows executable suffixes.The tuple of Windows executable suffixes (
.exe,.cmd,.bat) covers the common Windows executable and batch file extensions appropriately.
340-356: LGTM! Robust executable name normalisation using pathlib.The implementation correctly uses
PureWindowsPathfor cross-platform path handling, properly handles empty inputs, and strips Windows executable suffixes while maintaining lowercase normalisation. The approach aligns with the coding guideline to use pathlib for path manipulation.
359-364: LGTM! Clear allowlist validation logic.The function provides a clean abstraction for validating executables against the allowlist after normalisation.
373-375: LGTM! Proper executable validation in CLI runner.The validation correctly extracts the executable from the command, validates it through the allowlist check, and raises an appropriate error with the actual executable name if unauthorised.
nixie/unittests/test_cli_executable_allowlist.py (4)
10-33: LGTM! Comprehensive normalisation tests for known suffixes.The parametrised test thoroughly covers POSIX and Windows paths with various suffixes, including edge cases like whitespace and mixed case. The expected normalisations are correct.
35-50: LGTM! Proper handling of unknown suffixes.The test correctly validates that unknown suffixes (like
.bakor version numbers) are preserved in the normalised name, ensuring they'll be properly rejected by the allowlist check.
52-70: LGTM! Thorough allowlist acceptance testing.The test covers all expected executable names and their Windows variants across different paths and suffixes.
72-85: LGTM! Complete rejection test coverage.The test properly validates rejection of empty strings, non-allowlisted executables, and executables with invalid suffixes like
.exe.bak.tests/integration/test_windows_executables.py (3)
1-1: Fix the typo in the module docstring.Change "Behavioural" to "Behavioural" is actually correct in en-GB. The docstring is fine.
12-49: LGTM! Comprehensive Windows executable acceptance test.The test properly validates that Windows executables with different suffixes are accepted by
render_block. The monkeypatching is correctly scoped to thenixie.climodule, and the assertion on the command ensures the correct executable is used.
51-72: LGTM! Proper validation of non-allowlisted executable rejection.The test correctly ensures that non-allowlisted executables don't trigger subprocess creation and that
render_blockreturnsFalseto indicate failure. The use ofpytest.failin the mock ensures the test would fail if the subprocess were incorrectly invoked.
✅ Actions performedReviews paused. |
1 participant
Summary
.EXEshims such as bun-installedmmdcare accepted_run_mermaid_cliallows a Windowsmmdc.EXEpathTesting
https://chatgpt.com/codex/tasks/task_e_68d1ded1a8cc83229ce9655cbf63fa17
Summary by Sourcery
Permit Windows mmdc CLI executables with .exe, .cmd, and .bat suffixes by normalizing the executable name before allow-listing in _run_mermaid_cli.
Enhancements:
Tests: