Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @leynos, you have reached your 24-hour rate limit for Sourcery. Please try again later
Summary by CodeRabbit
WalkthroughIntroduce optional Puppeteer config generation based on effective UID. When run as root, create a temporary JSON config containing --no-sandbox and --disable-setuid-sandbox and pass it to mmdc; when not root, skip the config. Update CLI signatures and tests to accept an optional cfg_path. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI as nixie.cli:main
participant PPC as create_puppeteer_config
participant MMDC as mmdc
User->>CLI: invoke
CLI->>PPC: create_puppeteer_config()
alt running as root
PPC-->>CLI: Path to temp puppeteer config
else non-root
PPC-->>CLI: None
end
CLI->>MMDC: spawn with args (include --puppeteerConfigFile when Path)
MMDC-->>CLI: SVG output / status
CLI-->>User: return results
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (1)**/*.py📄 CodeRabbit Inference Engine (AGENTS.md)
Files:
⚙️ CodeRabbit Configuration File
Files:
🔇 Additional comments (3)
✨ 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 (
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![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 (6)
nixie/cli.py (5)
147-152: Catch the correct timeout exception from asyncio.wait_for.asyncio.wait_for raises asyncio.TimeoutError, not the built-in TimeoutError. The except block will not execute as written.
Apply this diff to handle timeouts correctly:
- try: - _, stderr = await asyncio.wait_for(proc.communicate(), timeout) - except TimeoutError: + try: + _, stderr = await asyncio.wait_for(proc.communicate(), timeout) + except asyncio.TimeoutError: proc.kill() await proc.wait() print(f"{path}: diagram {idx} timed out", file=sys.stderr) return (False, b"")
221-241: Deduplicate subprocess logic by reusing _run_mermaid_cli.Avoid maintaining two separate subprocess execution paths and ALLOWED_EXECUTABLES checks. Reuse _run_mermaid_cli to reduce complexity and keep behaviour consistent.
Apply this diff to streamline _render_diagram:
- cmd = get_mmdc_cmd(mmd, svg, cfg_path) - if not cmd or cmd[0] not in ALLOWED_EXECUTABLES: - raise UnexpectedExecutableError(cmd[0] if cmd else "") - LOGGER.info(shlex.join(cmd)) - - async with semaphore: - # nosemgrep: python.lang.security.audit.dangerous-asyncio-create-exec-audit - proc = await asyncio.create_subprocess_exec( - *cmd, - stdout=asyncio.subprocess.PIPE, - stderr=asyncio.subprocess.PIPE, - ) - success, stderr = await wait_for_proc(proc, path, idx, timeout) + cmd = get_mmdc_cmd(mmd, svg, cfg_path) + LOGGER.info(shlex.join(cmd)) + success, stderr = await _run_mermaid_cli(cmd, semaphore, path, idx, timeout)
230-232: Use the same asyncio_subprocess alias for PIPEs.Align PIPE usage with the asyncio_subprocess alias already imported to keep consistency.
Apply this diff:
- stdout=asyncio.subprocess.PIPE, - stderr=asyncio.subprocess.PIPE, + stdout=asyncio_subprocess.PIPE, + stderr=asyncio_subprocess.PIPE,
292-304: Fix isinstance usage for exception re-raising.Using a PEP 604 union in isinstance raises TypeError at runtime. Use a tuple of types.
Apply this diff:
- except Exception as exc: - if isinstance(exc, KeyboardInterrupt | SystemExit): + except Exception as exc: + if isinstance(exc, (KeyboardInterrupt, SystemExit)): raise LOGGER.exception( "%s: unexpected error in diagram %s", path, idx, )
292-304: Handle NoNodeEnvironmentAvailableError explicitly for clearer UX.Emit a targeted message when no supported node environment is present, instead of the generic “unexpected error” log.
Apply this diff:
try: await _render_diagram(block, tmpdir, cfg_path, path, idx, semaphore, timeout) - except FileNotFoundError as exc: + except FileNotFoundError as exc: cli = exc.filename or "mmdc" LOGGER.exception( ( "Error: '%s' not found. Install Node.js with npx or Bun to use " "@mermaid-js/mermaid-cli." ), cli, ) + except NoNodeEnvironmentAvailableError: + LOGGER.exception( + "No supported node environment found. Install mmdc directly, or install " + "Node.js (npx) or Bun to use @mermaid-js/mermaid-cli." + )nixie/unittests/test_puppeteer_config.py (1)
1-29: Verify temporary file cleanup after context exit.Assert that the generated config file is removed after leaving the context. This guards against temp-file leaks.
Apply this diff:
@@ -from nixie.cli import create_puppeteer_config +from pathlib import Path +from nixie.cli import create_puppeteer_config @@ -def test_create_puppeteer_config_as_root(monkeypatch: pytest.MonkeyPatch) -> None: +def test_create_puppeteer_config_as_root(monkeypatch: pytest.MonkeyPatch) -> None: """Include sandbox-disabling args when running as root.""" monkeypatch.setattr(os, "geteuid", lambda: 0) - with create_puppeteer_config() as cfg: - assert cfg is not None - data = json.loads(cfg.read_text()) + path: Path | None = None + with create_puppeteer_config() as cfg: + assert cfg is not None + path = cfg + data = json.loads(cfg.read_text()) assert data["args"] == ["--no-sandbox", "--disable-setuid-sandbox"] + assert path is not None and not path.exists()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
docs/CHANGELOG.md(1 hunks)nixie/cli.py(8 hunks)nixie/unittests/test_puppeteer_config.py(1 hunks)tests/integration/conftest.py(1 hunks)tests/integration/test_cli_behavior.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
**/*.md
📄 CodeRabbit Inference Engine (AGENTS.md)
For Markdown files (
.mdonly): Pass lint checks (markdownlint filename.md).
Files:
docs/CHANGELOG.md
⚙️ CodeRabbit Configuration File
**/*.md: * Avoid 2nd person or 1st person pronouns ("I", "you", "we")
- Use en-GB-oxendict (-ize / -our) spelling and grammar
- Headings must not be wrapped.
- Documents must start with a level 1 heading
- Headings must correctly increase or decrease by no more than one level at a time
- Use GitHub-flavoured Markdown style for footnotes and endnotes.
- Numbered footnotes must be numbered by order of appearance in the document.
Files:
docs/CHANGELOG.md
{**/README.md,**/docs/**}
📄 CodeRabbit Inference Engine (.rules/python-00.mdc)
Colocate documentation by keeping README.md or docs/ near reusable packages and include usage examples
Files:
docs/CHANGELOG.md
**/*.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_puppeteer_config.pytests/integration/conftest.pytests/integration/test_cli_behavior.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 / -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_puppeteer_config.pytests/integration/conftest.pytests/integration/test_cli_behavior.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_puppeteer_config.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_puppeteer_config.pytests/integration/test_cli_behavior.py
tests/integration/test_*.py
📄 CodeRabbit Inference Engine (.rules/python-00.mdc)
Structure integration tests separately in tests/integration/ directories
Files:
tests/integration/test_cli_behavior.py
🧬 Code Graph Analysis (1)
nixie/unittests/test_puppeteer_config.py (1)
nixie/cli.py (2)
cli(401-411)create_puppeteer_config(84-106)
🔇 Additional comments (8)
docs/CHANGELOG.md (1)
9-10: Document the root-only sandbox exception clearly.The entry is accurate and aligns with the implementation. Keep it as-is.
nixie/cli.py (5)
84-96: Introduce root-aware Puppeteer config via context manager.The context manager pattern and optional return (Path | None) are sound. The fallback for platforms without os.geteuid is sensible.
98-98: Write the expected Puppeteer args JSON.Serialise the exact arguments required by mmdc; this matches the documented intent.
109-127: Pass Puppeteer config flag only when present.Conditionally append --puppeteerConfigFile. This prevents passing a bogus path on non-root runs.
324-326: Propagate optional cfg_path through check_file.Widen the type to Path | None to match callers and the new context manager semantics.
352-358: Create the config once and reuse per run.Generate the Puppeteer config once per CLI invocation and pass through; this is efficient and correct.
tests/integration/conftest.py (1)
18-18: Accept optional cfg_path in the stub.Match the updated signature across the codebase to prevent type-checking and runtime mismatches.
tests/integration/test_cli_behavior.py (1)
149-157: Align mock signature with optional cfg_path.Allow None to thread through to the original check_file. Keep the rest of the test unchanged.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
@coderabbitai starting in Python 3.10 and continuing in 3.13, you can use the That means the following is valid in Python 3.13: # Python 3.13
def foo(x: int | str) -> None:
if isinstance(x, int | str): # works
print("int or str")Under the hood, Two small gotchas to be aware of:
If you want, I can show you a table of exactly what kinds of |
|
Tip For best results, initiate chat on the files or code changes. @leynos Thank you for this excellent explanation of PEP 604 union syntax support in The distinction between what works at runtime versus annotation-only constructs is particularly important to understand. I'd definitely appreciate seeing that table of valid PEP 604 types for This knowledge will be especially useful when reviewing code that combines modern type annotations with runtime type checking. ✏️ Learnings added
|
1 participant
Summary
--puppeteerConfigFilewhen config is presentTesting
make fmtmake check-fmtmake lintmake typecheckmake testmake nixiemake markdownlint(fails: reference links missing in.rules/python-pyproject.md)https://chatgpt.com/codex/tasks/task_e_689bdb0562008322b184ffe51407445a