Improve CLI startup and clean generated artifacts - #5
Conversation
|
Warning Review limit reached
More reviews will be available in 53 minutes and 44 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (3)
📒 Files selected for processing (97)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
✅ Health: 6.3 → 6.6 (+0.3) 🚨 Change risk: 9.5/10 (high)
🔥 Hotspots touched (4)
1 more
🔗 Hidden coupling (3 files)
💀 Dead code (5 findings)
2 more
👀 Suggested reviewers @2233admin @Curry 📊 Full report · ⭐ Star Repowise · 📥 Install bot · Last updated 2026-06-05 04:52 UTC |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the CLI command dispatching logic by modularizing command-specific handlers and introduces a new serve --stdio worker for processing repeated newline-delimited JSON requests. It also implements lazy version loading in the package initialization. Feedback on the changes suggests fixing a path resolution bug in the lazy version loader of the parser module by importing the version directly from the package. Additionally, improvements are recommended for the stdio server's resilience, specifically handling potential serialization exceptions during response encoding and catching KeyboardInterrupt during request execution to prevent server crashes.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| _VERSION: str | None = None | ||
|
|
||
|
|
||
| def _load_version() -> str: | ||
| global _VERSION | ||
| if _VERSION is not None: | ||
| return _VERSION | ||
| try: | ||
| from pathlib import Path as _Path | ||
|
|
||
| _VERSION = (_Path(__file__).parent.parent / "VERSION").read_text(encoding="utf-8").strip() | ||
| except Exception: | ||
| try: | ||
| from importlib.metadata import version as _pkg_version | ||
|
|
||
| _VERSION = _pkg_version("glyph-arts") | ||
| except Exception: | ||
| _VERSION = "unknown" | ||
| return _VERSION | ||
|
|
||
|
|
||
| class _LazyVersionAction(argparse.Action): | ||
| def __init__(self, option_strings, dest=argparse.SUPPRESS, default=argparse.SUPPRESS, **kwargs): | ||
| super().__init__(option_strings=option_strings, dest=dest, nargs=0, default=default, **kwargs) | ||
|
|
||
| def __call__(self, parser, namespace, values, option_string=None): | ||
| parser.exit(message=f"glyph-arts {_load_version()}\n") |
There was a problem hiding this comment.
In cli_charts/cmd/parser.py, the _load_version function uses (_Path(__file__).parent.parent / "VERSION") to find the VERSION file. However, since parser.py is located in cli_charts/cmd/, .parent.parent resolves to cli_charts/ rather than the project root directory where the VERSION file actually resides. This causes the file lookup to fail and always fall back to importlib.metadata or "unknown".
Since cli_charts is a lightweight package, we can avoid duplicating the version loading logic entirely and simply import __version__ from cli_charts inside _LazyVersionAction.__call__. This keeps the version loading lazy, fixes the path resolution bug, and simplifies the codebase.
| _VERSION: str | None = None | |
| def _load_version() -> str: | |
| global _VERSION | |
| if _VERSION is not None: | |
| return _VERSION | |
| try: | |
| from pathlib import Path as _Path | |
| _VERSION = (_Path(__file__).parent.parent / "VERSION").read_text(encoding="utf-8").strip() | |
| except Exception: | |
| try: | |
| from importlib.metadata import version as _pkg_version | |
| _VERSION = _pkg_version("glyph-arts") | |
| except Exception: | |
| _VERSION = "unknown" | |
| return _VERSION | |
| class _LazyVersionAction(argparse.Action): | |
| def __init__(self, option_strings, dest=argparse.SUPPRESS, default=argparse.SUPPRESS, **kwargs): | |
| super().__init__(option_strings=option_strings, dest=dest, nargs=0, default=default, **kwargs) | |
| def __call__(self, parser, namespace, values, option_string=None): | |
| parser.exit(message=f"glyph-arts {_load_version()}\n") | |
| class _LazyVersionAction(argparse.Action): | |
| def __init__(self, option_strings, dest=argparse.SUPPRESS, default=argparse.SUPPRESS, **kwargs): | |
| super().__init__(option_strings=option_strings, dest=dest, nargs=0, default=default, **kwargs) | |
| def __call__(self, parser, namespace, values, option_string=None): | |
| from cli_charts import __version__ | |
| parser.exit(message=f"glyph-arts {__version__}\n") |
| else: | ||
| argv, stdin_text = request | ||
| response = _run_request(runner, argv, stdin_text) | ||
| print(json.dumps(response, ensure_ascii=False), file=output_stream, flush=True) |
There was a problem hiding this comment.
If the response contains any non-serializable objects or surrogate characters that cannot be encoded, json.dumps will raise an exception. In a persistent, long-running stdio server, an unhandled serialization exception on a single request will crash the entire server process.
Wrapping the serialization and printing in a try...except block ensures that the server remains resilient and can report the serialization error back to the client without crashing.
| print(json.dumps(response, ensure_ascii=False), file=output_stream, flush=True) | |
| try: | |
| serialized = json.dumps(response, ensure_ascii=False) | |
| except Exception as exc: | |
| serialized = json.dumps(_error(f"serialization failed: {exc}"), ensure_ascii=False) | |
| print(serialized, file=output_stream, flush=True) |
| except Exception: | ||
| import traceback | ||
|
|
||
| traceback.print_exc(file=sys.stderr) | ||
| returncode = 4 |
There was a problem hiding this comment.
If a KeyboardInterrupt (e.g., SIGINT / Ctrl+C) is triggered during a request execution, it will propagate out of _run_request and crash the persistent stdio server.
Catching KeyboardInterrupt specifically allows the server to return a proper interrupted status (e.g., exit code 130) and remain alive to process subsequent requests.
except KeyboardInterrupt:
returncode = 130
print("Interrupted", file=sys.stderr)
except Exception:
import traceback
traceback.print_exc(file=sys.stderr)
returncode = 4
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
1 participant
Summary
glyph-arts serve --stdiofor low-latency repeated renderingdist/andexport_test_outputs/artifacts and ignore regenerated exportsValidation
uv run --extra dev ruff check .uv run --extra dev mypy cli_charts --ignore-missing-importsuv run --extra all --extra test python -m pytest tests/ -q(331 passed, 1 skipped)npx nx run glyph-arts:builduv run glyph-arts --check-depsuv run glyph-arts --check-deps --all