feat: add petiglyph media support - #6
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe PR introduces Petiglyph as an optional terminal glyph backend and extends video rendering with quality-mode controls, playlist/folder input, and adaptive frame rendering. Petiglyph integration includes CLI registration, project parsing, artifact loading, and command dispatching. Video enhancements add mode/pixel settings to control Chafa profiles and ASCII ramp selection, support playlist and folder input with queueing, and implement live/export rendering via Pillow or Chafa based on availability. ChangesPetiglyph backend and video rendering
Sequence Diagram(s)sequenceDiagram
participant User as User (CLI)
participant Main as main()
participant ArgRewrite as _rewrite_petiglyph_argv
participant Dispatch as dispatch_tool_command
participant PetiCmd as run_petiglyph_command
participant Backend as CliPetiglyphBackend
participant Project as read_petiglyph_project
User->>Main: petiglyph <project> --json
Main->>ArgRewrite: argv with --json
ArgRewrite->>Main: argv with --petiglyph-json-output
Main->>Dispatch: args.type='petiglyph'
Dispatch->>PetiCmd: args, raw_argv
PetiCmd->>Project: resolve project.toml
Project->>PetiCmd: PetiglyphProject
PetiCmd->>Backend: decide action (preview/doctor/etc)
Backend->>Backend: run petiglyph CLI
Backend->>PetiCmd: exit code
PetiCmd->>User: rendered output or JSON
sequenceDiagram
participant User as User (CLI)
participant Dispatch as dispatch_media
participant Queue as _resolve_video_queue
participant Render as render_video
participant Media as _build_ascii_image
participant Terminal as Terminal
User->>Dispatch: --video-folder=./clips --video-mode=3 --video-pixel
Dispatch->>Queue: resolve folder input
Queue->>Dispatch: items=[{path, mode:3, pixel:true}, ...]
Dispatch->>Dispatch: validate queued items
loop for each item in queue
Dispatch->>Render: path, video_mode=3, pixel_mode=true
Render->>Render: resolve_video_mode_defaults(mode=3)
Render->>Media: per-frame ASCII build with resolved color/symbols
Media->>Terminal: render frame
Render->>Render: enforce timing
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
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.5 (+0.2) 🚨 Change risk: 9.6/10 (high)
🔥 Hotspot touched (1)
🔗 Hidden coupling (1 file)
👀 Suggested reviewers @Curry 📊 Full report · ⭐ Star Repowise · 📥 Install bot · Last updated 2026-06-13 10:13 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 introduces support for Petiglyph, an optional custom font glyph backend, and significantly enhances video playback capabilities by adding support for playlists, folder scanning, continuous looping, and various quality modes (including a Pillow-based live rendering fallback). The review feedback highlights several opportunities to improve robustness and compatibility, such as explicitly excluding boolean values from codepoint checks, validating playlist entry types to prevent crashes, sorting scanned folder contents for deterministic playback, verifying overridden binary paths, handling file-based project references gracefully, falling back to tomli on older Python versions, and utilizing standard shlex.quote instead of custom quoting functions.
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.
| def _looks_like_codepoint(value: Any) -> bool: | ||
| if isinstance(value, int): | ||
| return True | ||
| text = str(value).strip() | ||
| return len(text) == 1 or text.upper().startswith("U+") or text.lower().startswith("0x") | ||
|
|
||
|
|
||
| def _format_codepoint(value: Any) -> str: | ||
| if value is None: | ||
| return "" | ||
| if isinstance(value, int): | ||
| return f"U+{value:04X}" |
There was a problem hiding this comment.
In Python, isinstance(True, int) is True because bool is a subclass of int. This causes boolean values in the JSON mapping to be incorrectly identified as codepoints (e.g., True becomes U+0001). We should explicitly exclude bool in _looks_like_codepoint and _format_codepoint.
| def _looks_like_codepoint(value: Any) -> bool: | |
| if isinstance(value, int): | |
| return True | |
| text = str(value).strip() | |
| return len(text) == 1 or text.upper().startswith("U+") or text.lower().startswith("0x") | |
| def _format_codepoint(value: Any) -> str: | |
| if value is None: | |
| return "" | |
| if isinstance(value, int): | |
| return f"U+{value:04X}" | |
| def _looks_like_codepoint(value: Any) -> bool: | |
| if isinstance(value, bool): | |
| return False | |
| if isinstance(value, int): | |
| return True | |
| text = str(value).strip() | |
| return len(text) == 1 or text.upper().startswith("U+") or text.lower().startswith("0x") | |
| def _format_codepoint(value: Any) -> str: | |
| if value is None or isinstance(value, bool): | |
| return "" | |
| if isinstance(value, int): | |
| return f"U+{value:04X}" |
| elif isinstance(item, dict): | ||
| entry = item.get("video") or item.get("path") | ||
| mode = item.get("mode", default_mode) | ||
| pixel = item.get("pixel", default_pixel) | ||
| if "mode" in item and not isinstance(item["mode"], (int, float, str)): | ||
| print(f"ERROR:schema: playlist entry has invalid mode: {item!r}", file=sys.stderr) | ||
| return None |
There was a problem hiding this comment.
If item is a dictionary, entry is retrieved via item.get("video") or item.get("path"). If entry is not a string (e.g., a list or dictionary), calling os.path.exists(entry) later will raise a TypeError and crash the CLI. Adding a type check ensures robust handling of malformed playlist JSON files.
| elif isinstance(item, dict): | |
| entry = item.get("video") or item.get("path") | |
| mode = item.get("mode", default_mode) | |
| pixel = item.get("pixel", default_pixel) | |
| if "mode" in item and not isinstance(item["mode"], (int, float, str)): | |
| print(f"ERROR:schema: playlist entry has invalid mode: {item!r}", file=sys.stderr) | |
| return None | |
| elif isinstance(item, dict): | |
| entry = item.get("video") or item.get("path") | |
| if entry is not None and not isinstance(entry, str): | |
| print(f"ERROR:schema: playlist entry path must be a string: {item!r}", file=sys.stderr) | |
| return None | |
| mode = item.get("mode", default_mode) | |
| pixel = item.get("pixel", default_pixel) | |
| if "mode" in item and (not isinstance(item["mode"], (int, float, str)) or isinstance(item["mode"], bool)): | |
| print(f"ERROR:schema: playlist entry has invalid mode: {item!r}", file=sys.stderr) | |
| return None |
| try: | ||
| with os.scandir(folder) as it: | ||
| for entry in it: | ||
| if not entry.is_file(): | ||
| continue | ||
| if Path(entry.name).suffix.lower() not in _SUPPORTED_VIDEO_EXT: | ||
| continue | ||
| queue.append({"path": entry.path, "mode": default_mode, "pixel": bool(default_pixel)}) | ||
| except OSError as exc: | ||
| print(f"ERROR:schema: cannot scan folder {folder}: {exc}", file=sys.stderr) | ||
| return None |
There was a problem hiding this comment.
os.scandir returns directory entries in arbitrary filesystem order, which can be highly non-deterministic and scrambled (especially on Linux/ext4). To ensure a consistent and expected playback order, the queue should be sorted alphabetically by file path.
| try: | |
| with os.scandir(folder) as it: | |
| for entry in it: | |
| if not entry.is_file(): | |
| continue | |
| if Path(entry.name).suffix.lower() not in _SUPPORTED_VIDEO_EXT: | |
| continue | |
| queue.append({"path": entry.path, "mode": default_mode, "pixel": bool(default_pixel)}) | |
| except OSError as exc: | |
| print(f"ERROR:schema: cannot scan folder {folder}: {exc}", file=sys.stderr) | |
| return None | |
| try: | |
| with os.scandir(folder) as it: | |
| for entry in it: | |
| if not entry.is_file(): | |
| continue | |
| if Path(entry.name).suffix.lower() not in _SUPPORTED_VIDEO_EXT: | |
| continue | |
| queue.append({"path": entry.path, "mode": default_mode, "pixel": bool(default_pixel)}) | |
| queue.sort(key=lambda x: x["path"]) | |
| except OSError as exc: | |
| print(f"ERROR:schema: cannot scan folder {folder}: {exc}", file=sys.stderr) | |
| return None |
| def __init__(self, binary: str | None = None) -> None: | ||
| override = binary or os.environ.get("GLYPH_ARTS_PETIGLYPH", "") | ||
| resolved = shutil.which("petiglyph") or shutil.which("petiglyph.exe") | ||
| self.binary = override or resolved or "petiglyph" | ||
| self._resolved = override or resolved |
There was a problem hiding this comment.
If override (from GLYPH_ARTS_PETIGLYPH env var) is set to a non-existent path, self._resolved will still be set to that path, causing available() to return True even though the binary is missing. Using shutil.which on the override path first ensures we only mark it as available if it actually exists and is executable.
| def __init__(self, binary: str | None = None) -> None: | |
| override = binary or os.environ.get("GLYPH_ARTS_PETIGLYPH", "") | |
| resolved = shutil.which("petiglyph") or shutil.which("petiglyph.exe") | |
| self.binary = override or resolved or "petiglyph" | |
| self._resolved = override or resolved | |
| def __init__(self, binary: str | None = None) -> None: | |
| override = binary or os.environ.get("GLYPH_ARTS_PETIGLYPH", "") | |
| resolved = shutil.which(override) if override else (shutil.which("petiglyph") or shutil.which("petiglyph.exe")) | |
| self.binary = resolved or override or "petiglyph" | |
| self._resolved = resolved |
| def _resolve_project_root(project_ref: str) -> Path: | ||
| raw = Path(project_ref).expanduser() | ||
| candidates = [raw] | ||
| if not raw.is_absolute(): | ||
| candidates.append(Path.cwd() / raw) | ||
| for candidate in candidates: | ||
| if candidate.exists(): | ||
| return candidate.resolve() | ||
| return raw.resolve() |
There was a problem hiding this comment.
If project_ref points directly to a petiglyph.toml file rather than its containing directory, _resolve_project_root will return the file path itself, which subsequently causes read_petiglyph_project to fail when constructing the manifest path. Resolving to the parent directory if a file is passed improves usability.
| def _resolve_project_root(project_ref: str) -> Path: | |
| raw = Path(project_ref).expanduser() | |
| candidates = [raw] | |
| if not raw.is_absolute(): | |
| candidates.append(Path.cwd() / raw) | |
| for candidate in candidates: | |
| if candidate.exists(): | |
| return candidate.resolve() | |
| return raw.resolve() | |
| def _resolve_project_root(project_ref: str) -> Path: | |
| raw = Path(project_ref).expanduser() | |
| candidates = [raw] | |
| if not raw.is_absolute(): | |
| candidates.append(Path.cwd() / raw) | |
| for candidate in candidates: | |
| if candidate.exists(): | |
| resolved = candidate.resolve() | |
| return resolved.parent if resolved.is_file() else resolved | |
| return raw.resolve() |
| def _read_toml(path: Path) -> dict[str, Any]: | ||
| try: | ||
| import tomllib | ||
|
|
||
| with path.open("rb") as handle: | ||
| data = tomllib.load(handle) | ||
| return data if isinstance(data, dict) else {} | ||
| except Exception: | ||
| return {} |
There was a problem hiding this comment.
tomllib was introduced in Python 3.11. For compatibility with Python < 3.11, we should attempt to fall back to tomli (the official backport) if tomllib is not available, rather than silently returning an empty dictionary on ImportError.
| def _read_toml(path: Path) -> dict[str, Any]: | |
| try: | |
| import tomllib | |
| with path.open("rb") as handle: | |
| data = tomllib.load(handle) | |
| return data if isinstance(data, dict) else {} | |
| except Exception: | |
| return {} | |
| def _read_toml(path: Path) -> dict[str, Any]: | |
| try: | |
| try: | |
| import tomllib | |
| except ImportError: | |
| import tomli as tomllib # type: ignore | |
| with path.open("rb") as handle: | |
| data = tomllib.load(handle) | |
| return data if isinstance(data, dict) else {} | |
| except Exception: | |
| return {} |
| def _quote_command(command: list[str]) -> str: | ||
| return " ".join(_quote_part(part) for part in command) | ||
|
|
||
|
|
||
| def _quote_part(part: str) -> str: | ||
| if not part or any(ch.isspace() for ch in part): | ||
| return '"' + part.replace('"', '\\"') + '"' | ||
| return part |
There was a problem hiding this comment.
The custom _quote_command and _quote_part functions can be replaced entirely with the standard library's shlex.quote, which is already imported and is much more robust at handling shell-sensitive characters.
| def _quote_command(command: list[str]) -> str: | |
| return " ".join(_quote_part(part) for part in command) | |
| def _quote_part(part: str) -> str: | |
| if not part or any(ch.isspace() for ch in part): | |
| return '"' + part.replace('"', '\\"') + '"' | |
| return part | |
| def _quote_command(command: list[str]) -> str: | |
| return " ".join(shlex.quote(part) for part in command) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cli_charts/render/media_engine.py (1)
1456-1459:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHonor
max_framesduring live playback too.Lines 1456-1459 build the live ffmpeg decode command without using
max_frames, so--max-framesonly works for export and not for terminal playback/playlist runs. That breaks the CLI contract and removes the only bound on very long videos here. Add-frames:vwhenmax_frames > 0(or sliceframesimmediately after globbing).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cli_charts/render/media_engine.py` around lines 1456 - 1459, The ffmpeg command built into the ff list currently ignores max_frames, so --max-frames only limits export; update the ff construction in the live playback path to honour max_frames by appending ["-frames:v", str(max_frames)] when max_frames > 0 (or alternatively apply frames = frames[:max_frames] immediately after globbing); ensure you insert the option before the output pattern (os.path.join(tmp, "f_%05d.png")) so the ff variable used by the live decode uses the cap.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cli_charts/cmd/media_dispatch.py`:
- Around line 42-73: The playlist entry path (variable entry) must be normalized
relative to the playlist file before the existence check and before being
queued: if entry is not absolute, replace it with
Path(path).resolve().parent.joinpath(entry).resolve() (using pathlib.Path) so
relative entries like "videos/clip.mp4" are resolved against the playlist file's
directory; perform this normalization after you validate mode via
_validate_video_mode and before the os.path.exists check, and then store
str(entry) (or entry.as_posix()) in the queued dict so the queued "path" is the
resolved absolute path.
- Around line 217-218: The current code uses trim=not getattr(args, "no_trim",
True) which enables trimming for all renders (causing Pillow-live to run for
videos); change the logic so trimming is enabled only for image renders: compute
a boolean like is_image (based on the existing args flag that indicates image vs
video in this command) and set trim = is_image and not getattr(args, "no_trim",
True); apply this change to both call sites that currently set trim (the two
places using getattr(args, "no_trim", True)) so videos default to trim=False
unless a video-specific trim flag is added.
In `@cli_charts/petiglyph_support.py`:
- Around line 89-90: available() currently returns True when _resolved is a
non-empty string even if it isn't executable; change available() to check
executability (e.g., use shutil.which(self._resolved) if _resolved is set,
otherwise shutil.which(self.binary)) so it only reports available when an
executable is found. In run(), wrap the subprocess.run(cmd) call in a try/except
FileNotFoundError and handle it by returning the intended exit code 127 and the
install hint (or otherwise converting it into the same controlled error path
used for missing backends) instead of letting the exception propagate. In
_build_cli_args, catch ValueError from shlex.split(extra) and convert it into a
controlled CLI error (raise a clear ValueError or the project's CLI error type
with a message like "malformed --petiglyph-arg: <original error>") so malformed
--petiglyph-arg values produce a user-facing error rather than an uncaught
exception. Ensure references: methods available(), run(), _build_cli_args and
attributes _resolved and binary are updated accordingly.
- Around line 155-156: run_petiglyph_command currently calls _build_cli_args
which may call shlex.split on args.petiglyph_arg and raise ValueError for
malformed input; wrap the _build_cli_args(...) call in run_petiglyph_command in
a try/except ValueError block that logs a clear error (or prints to stderr) and
returns a non-zero exit code (or raises a controlled SystemExit) instead of
letting the traceback bubble up, and apply the identical ValueError handling to
the call site in _run_destructive so both code paths convert shlex.split parsing
errors into the same graceful error exit behavior; reference the functions
_build_cli_args, run_petiglyph_command, _run_destructive, and shlex.split when
locating where to add the try/except.
In `@cli_charts/render/media_engine.py`:
- Around line 1523-1554: The code currently allows chafa_format_effective
(computed by _resolve_video_chafa_format) to remain "auto" and select a graphics
protocol on certain terminals; change the logic after computing
chafa_format_effective so that if video_mode == 1 or
bool(mode_profile["force_no_color"]) is true you forcibly set
chafa_format_effective = "symbols" (before the subsequent ASCII-check and
chafa_args modification and before calling _build_chafa_cmd), ensuring
_build_chafa_cmd always receives "symbols" for text-only/force-no-color modes;
reference the local variables video_mode, mode_profile, chafa_format_effective
and the functions _resolve_video_chafa_format and _build_chafa_cmd when making
this change.
- Around line 1514-1520: The except KeyboardInterrupt blocks that currently
swallow Ctrl-C should not return 0 (which the caller treats as “continue”);
after restoring the cursor (the sys.stdout.write("\x1b[?25h") /
sys.stdout.flush() cleanup) re-raise the KeyboardInterrupt so the outer
playlist/loop in cli_charts/cmd/media_dispatch.py can terminate, or
alternatively return a distinct cancellation code that media_dispatch treats as
cancellation — update both places where you see "except KeyboardInterrupt: pass"
immediately followed by the cursor restore finally block to either re-raise the
exception (recommended) or return a documented cancellation code.
---
Outside diff comments:
In `@cli_charts/render/media_engine.py`:
- Around line 1456-1459: The ffmpeg command built into the ff list currently
ignores max_frames, so --max-frames only limits export; update the ff
construction in the live playback path to honour max_frames by appending
["-frames:v", str(max_frames)] when max_frames > 0 (or alternatively apply
frames = frames[:max_frames] immediately after globbing); ensure you insert the
option before the output pattern (os.path.join(tmp, "f_%05d.png")) so the ff
variable used by the live decode uses the cap.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b482a2d8-ebc8-4762-84c8-d1e83426c353
📒 Files selected for processing (8)
cli_charts/cmd/_helpers.pycli_charts/cmd/media_args.pycli_charts/cmd/media_dispatch.pycli_charts/cmd/parser.pycli_charts/cmd/tool_commands.pycli_charts/installers.pycli_charts/petiglyph_support.pycli_charts/render/media_engine.py
| entry = item.get("video") or item.get("path") | ||
| mode = item.get("mode", default_mode) | ||
| pixel = item.get("pixel", default_pixel) | ||
| if "mode" in item and not isinstance(item["mode"], (int, float, str)): | ||
| print(f"ERROR:schema: playlist entry has invalid mode: {item!r}", file=sys.stderr) | ||
| return None | ||
| else: | ||
| print(f"ERROR:schema: playlist entry invalid: {item!r}", file=sys.stderr) | ||
| return None | ||
|
|
||
| mode = _validate_video_mode(mode) | ||
| if mode is None: | ||
| return None | ||
| if mode == 1 and pixel: | ||
| print("ERROR:schema: --video-pixel requires --video-mode 2-5", file=sys.stderr) | ||
| return None | ||
| if mode not in {1, 2, 3, 4, 5}: | ||
| print(f"ERROR:schema: invalid --video-mode {mode}; use 1-5", file=sys.stderr) | ||
| return None | ||
| if not entry: | ||
| print(f"ERROR:schema: playlist entry missing video path: {item!r}", file=sys.stderr) | ||
| return None | ||
| if not os.path.exists(entry): | ||
| print(f"ERROR:schema: playlist entry file not found: {entry}", file=sys.stderr) | ||
| return None | ||
|
|
||
| queue.append( | ||
| { | ||
| "path": str(entry), | ||
| "mode": mode, | ||
| "pixel": bool(pixel), | ||
| } |
There was a problem hiding this comment.
Resolve playlist entries relative to the playlist file.
Lines 64-70 validate and queue entry exactly as written, so a playlist entry like videos/clip.mp4 only works when the current working directory happens to match the playlist’s directory. That makes the new playlist feature non-portable as soon as the command is launched from elsewhere. Normalize non-absolute entries against Path(path).resolve().parent before the existence check and before storing them in the queue.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cli_charts/cmd/media_dispatch.py` around lines 42 - 73, The playlist entry
path (variable entry) must be normalized relative to the playlist file before
the existence check and before being queued: if entry is not absolute, replace
it with Path(path).resolve().parent.joinpath(entry).resolve() (using
pathlib.Path) so relative entries like "videos/clip.mp4" are resolved against
the playlist file's directory; perform this normalization after you validate
mode via _validate_video_mode and before the os.path.exists check, and then
store str(entry) (or entry.as_posix()) in the queued dict so the queued "path"
is the resolved absolute path.
| trim=not getattr(args, "no_trim", True), | ||
| font_size=getattr(args, "font_size", 14), |
There was a problem hiding this comment.
Don’t apply the image-only --no-trim default to every video render.
Lines 217 and 251 turn the default args.no_trim == False into trim=True for all videos, even though --no-trim is documented as TYPE=image. In cli_charts/render/media_engine.py, trim=True unconditionally selects the Pillow-live path, so the new Chafa/video-pixel logic never runs unless the user opts out with --no-trim. Pass trim=False here unless you add a video-specific trim flag.
Also applies to: 251-252
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cli_charts/cmd/media_dispatch.py` around lines 217 - 218, The current code
uses trim=not getattr(args, "no_trim", True) which enables trimming for all
renders (causing Pillow-live to run for videos); change the logic so trimming is
enabled only for image renders: compute a boolean like is_image (based on the
existing args flag that indicates image vs video in this command) and set trim =
is_image and not getattr(args, "no_trim", True); apply this change to both call
sites that currently set trim (the two places using getattr(args, "no_trim",
True)) so videos default to trim=False unless a video-specific trim flag is
added.
| def available(self) -> bool: | ||
| return bool(self._resolved or shutil.which(self.binary)) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the availability check and subprocess path are currently unguarded.
rg -n -C3 "def available\\(|self\\._resolved|def run\\(|subprocess\\.run\\(cmd\\)" cli_charts/petiglyph_support.pyRepository: 2233admin/glyph-arts
Length of output: 1836
Fix backend availability/run to avoid crashes on invalid GLYPH_ARTS_PETIGLYPH overrides
available()(lines 89-90) returns true when_resolvedcontains a non-emptyGLYPH_ARTS_PETIGLYPHstring, even if it doesn’t point to an executable; thenrun()(line 104) unguardedly callssubprocess.run(cmd), which can raiseFileNotFoundErrorinstead of returning the intended 127 + install hint._build_cli_argsusesshlex.split(extra); ensureValueErrorfrom malformed--petiglyph-argis caught and converted into a controlled CLI error.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cli_charts/petiglyph_support.py` around lines 89 - 90, available() currently
returns True when _resolved is a non-empty string even if it isn't executable;
change available() to check executability (e.g., use
shutil.which(self._resolved) if _resolved is set, otherwise
shutil.which(self.binary)) so it only reports available when an executable is
found. In run(), wrap the subprocess.run(cmd) call in a try/except
FileNotFoundError and handle it by returning the intended exit code 127 and the
install hint (or otherwise converting it into the same controlled error path
used for missing backends) instead of letting the exception propagate. In
_build_cli_args, catch ValueError from shlex.split(extra) and convert it into a
controlled CLI error (raise a clear ValueError or the project's CLI error type
with a message like "malformed --petiglyph-arg: <original error>") so malformed
--petiglyph-arg values produce a user-facing error rather than an uncaught
exception. Ensure references: methods available(), run(), _build_cli_args and
attributes _resolved and binary are updated accordingly.
| cli_args = _build_cli_args(args, tokens, raw_argv) | ||
| return backend.run(cli_args, dry_run=getattr(args, "dry_run", False)) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify shlex splitting is currently unhandled and bubbles from _build_cli_args into run_petiglyph_command.
rg -n -C3 "def run_petiglyph_command|_build_cli_args\\(|shlex\\.split\\(" cli_charts/petiglyph_support.pyRepository: 2233admin/glyph-arts
Length of output: 1523
Handle malformed --petiglyph-arg without crashing.
_build_cli_args() calls shlex.split(extra) directly for each args.petiglyph_arg (around 460-461) and run_petiglyph_command() calls _build_cli_args() with no try/except (155-156), so a malformed value can bubble up as a ValueError/traceback. The same risk also applies to _run_destructive() (which calls _build_cli_args() around 410).
Proposed fix
def run_petiglyph_command(args: Any, raw_argv: list[str] | None = None) -> int:
@@
backend = _select_backend(args)
- cli_args = _build_cli_args(args, tokens, raw_argv)
+ try:
+ cli_args = _build_cli_args(args, tokens, raw_argv)
+ except ValueError as exc:
+ print(f"ERROR:petiglyph: invalid --petiglyph-arg: {exc}", file=sys.stderr)
+ return 2
return backend.run(cli_args, dry_run=getattr(args, "dry_run", False))
@@
def _build_cli_args(args: Any, tokens: list[str], raw_argv: list[str]) -> list[str]:
@@
for extra in getattr(args, "petiglyph_arg", None) or []:
- cli_args.extend(shlex.split(extra))
+ try:
+ cli_args.extend(shlex.split(extra))
+ except ValueError as exc:
+ raise ValueError(extra) from exc
return cli_argsMake sure _run_destructive() gets equivalent ValueError handling too so destructive mode doesn’t bypass this error-to-exit-code behavior.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cli_charts/petiglyph_support.py` around lines 155 - 156,
run_petiglyph_command currently calls _build_cli_args which may call shlex.split
on args.petiglyph_arg and raise ValueError for malformed input; wrap the
_build_cli_args(...) call in run_petiglyph_command in a try/except ValueError
block that logs a clear error (or prints to stderr) and returns a non-zero exit
code (or raises a controlled SystemExit) instead of letting the traceback bubble
up, and apply the identical ValueError handling to the call site in
_run_destructive so both code paths convert shlex.split parsing errors into the
same graceful error exit behavior; reference the functions _build_cli_args,
run_petiglyph_command, _run_destructive, and shlex.split when locating where to
add the try/except.
| except KeyboardInterrupt: | ||
| pass | ||
| finally: | ||
| if is_tty: | ||
| sys.stdout.write("\x1b[?25h") | ||
| sys.stdout.flush() | ||
| return 0 |
There was a problem hiding this comment.
Let KeyboardInterrupt terminate the outer playlist/loop.
Lines 1514-1520 and 1584-1590 swallow Ctrl-C and fall through to return 0. In cli_charts/cmd/media_dispatch.py, 0 means “success, continue”, so interrupting one item in a playlist advances to the next item, and --video-loop can keep running instead of exiting. Re-raise after cursor cleanup, or return a distinct code that the caller treats as cancellation.
Also applies to: 1584-1590
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cli_charts/render/media_engine.py` around lines 1514 - 1520, The except
KeyboardInterrupt blocks that currently swallow Ctrl-C should not return 0
(which the caller treats as “continue”); after restoring the cursor (the
sys.stdout.write("\x1b[?25h") / sys.stdout.flush() cleanup) re-raise the
KeyboardInterrupt so the outer playlist/loop in cli_charts/cmd/media_dispatch.py
can terminate, or alternatively return a distinct cancellation code that
media_dispatch treats as cancellation — update both places where you see "except
KeyboardInterrupt: pass" immediately followed by the cursor restore finally
block to either re-raise the exception (recommended) or return a documented
cancellation code.
| mode_profile = _resolve_video_mode_defaults(video_mode, pixel_mode=pixel_mode) | ||
| symbols_effective = chafa_symbols or symbols or str(mode_profile["symbols"]) | ||
| colors_effective = chafa_colors | ||
| if colors_effective == "auto": | ||
| resolved_colors = str(mode_profile["chafa_colors"]) | ||
| if resolved_colors != "auto": | ||
| colors_effective = resolved_colors | ||
| if no_color or bool(mode_profile["force_no_color"]): | ||
| colors_effective = "none" | ||
| chafa_format_effective = _resolve_video_chafa_format( | ||
| chafa_format, | ||
| symbols_effective, | ||
| pixel_mode=pixel_mode, | ||
| chat=chat, | ||
| ) | ||
| chafa_args_effective = list(chafa_args or []) | ||
| if ( | ||
| chafa_format_effective == "symbols" | ||
| and _is_ascii_video_symbols(symbols_effective) | ||
| and not any(arg == "--fg-only" or arg.startswith("--bg=") for arg in chafa_args_effective) | ||
| ): | ||
| chafa_args_effective.append("--fg-only") | ||
| chafa_cmd = _build_chafa_cmd( | ||
| w, | ||
| h, | ||
| symbols=chafa_symbols or symbols, | ||
| no_color=no_color, | ||
| chafa_format=chafa_format, | ||
| chafa_colors=chafa_colors, | ||
| chafa_args=chafa_args, | ||
| symbols=symbols_effective, | ||
| no_color=no_color or bool(mode_profile["force_no_color"]), | ||
| chafa_format=chafa_format_effective, | ||
| chafa_colors=colors_effective, | ||
| chafa_args=chafa_args_effective, | ||
| chat=chat, | ||
| ) |
There was a problem hiding this comment.
Force video mode 1 onto symbol output.
cli_charts/cmd/media_args.py advertises --video-mode 1 as “text-only by design”, but Lines 1523-1554 still let the default chafa_format="auto" flow into _build_chafa_cmd(). On Kitty/WezTerm/iTerm, that can resolve to a graphics protocol, so mode 1 becomes monochrome graphics instead of text. Pin chafa_format_effective to "symbols" when video_mode == 1 (or when force_no_color is set).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cli_charts/render/media_engine.py` around lines 1523 - 1554, The code
currently allows chafa_format_effective (computed by
_resolve_video_chafa_format) to remain "auto" and select a graphics protocol on
certain terminals; change the logic after computing chafa_format_effective so
that if video_mode == 1 or bool(mode_profile["force_no_color"]) is true you
forcibly set chafa_format_effective = "symbols" (before the subsequent
ASCII-check and chafa_args modification and before calling _build_chafa_cmd),
ensuring _build_chafa_cmd always receives "symbols" for text-only/force-no-color
modes; reference the local variables video_mode, mode_profile,
chafa_format_effective and the functions _resolve_video_chafa_format and
_build_chafa_cmd when making this change.
1 participant
Summary
glyph-arts petiglyph ...support via an optional Petiglyph CLI backendManual checks
glyph-arts petiglyph doctorglyph-arts petiglyph preview ... --chatNotes
petiglyph_runs/andpipeline_outputs/are intentionally not included