Conversation
Add bgo_cli._notify with a zero-dep, best-effort notifier that shells out to notify-send (Linux), osascript or terminal-notifier (macOS), or a user-supplied BGO_NOTIFY_CMD override. Gating env BGO_NOTIFY chooses off | errors (default) | all. Wire _notify_errored() into cmd_watcher_loop at all three errored transitions (mode=stop, backoff exhausted, restart failure). Fix latent bug in bgo_cli/__init__.py: importlib.machinery was used without being imported, breaking dev-tree invocation.
Add bgo_cli._autostart with auto-detected backend: - Linux -> systemd user unit (~/.config/systemd/user/bgo-resurrect.service) - macOS -> LaunchAgent (~/Library/LaunchAgents/sh.discus.bgo.resurrect.plist) Public API: install(target), uninstall(target), status(). Target is 'resurrect' (default) or 'tray'. Tray on Linux uses XDG desktop autostart instead of systemd. macOS uses a second LaunchAgent plist. Idempotent: re-running install overwrites; uninstall is safe when nothing is installed. Atomic file writes (tmp + os.replace). Falls back to launchctl 'load -w' when 'bootstrap' fails on older macOS or for already-loaded plists. 28 unit tests, all platform-mocked: no real autostart entries are written by the suite.
Add bgo_cli._tray (pystray + Pillow glue) and bgo_cli._tray_install (auto-install fallback when the optional [tray] extra isn't present). _tray splits pure menu logic (load_snapshots, build_menu_spec, ProcSnapshot) from the pystray event loop so the testable surface doesn't depend on the optional deps. Actions shell out to the 'bgo' binary itself; no state-mutation logic is duplicated. _tray_install detects the installer (uv tool / pipx / pip) from sys.prefix and offers the matching dep-injection command: - uv -> uv tool install --upgrade --with pystray --with Pillow bgo-cli - pipx -> pipx inject bgo-cli pystray Pillow - pip -> python -m pip install --user pystray Pillow User must confirm unless --auto-install or BGO_TRAY_AUTOINSTALL=1. Aborts cleanly when the installer binary itself is missing. After a successful install, cmd_tray re-execs sys.argv so the freshly injected deps are picked up by a clean interpreter. 26 unit tests covering snapshot loading, menu rendering, poll interval resolution, subprocess dispatch, and installer detection. The pystray-bound _run_tray is intentionally not covered — it requires the optional extra to be installed.
…features - bgo autostart install|uninstall|status [--tray] [--json] - bgo tray [--poll N] [--auto-install] - both routed via lazy imports so missing optional deps degrade cleanly instead of failing at startup - 'autostart' and 'tray' added to known_commands so direct-mode parsing doesn't misroute them as proc names - pyproject: add [tray] optional extra (pystray>=0.19, Pillow>=10) - README: three new sections (Desktop notifications, Autostart at login, Tray icon (optional)) with env vars, install commands per installer, and Wayland caveat
- notify: parse BGO_NOTIFY_CMD with shlex.split so quoted args group
correctly (was: raw .split() lost quoting); malformed quotes now
fall through to platform detection instead of producing an
unrunnable argv.
- autostart: harden _resolve_bgo_binary to abspath via Path.resolve()
even when shutil.which returns a relative entry; _launchctl_unload
now actually surfaces failures instead of unconditionally returning
True (still idempotent on 'Could not find specified service').
- tray: 'Start' menu entry was broken because 'bgo start <name>'
without a command is rejected by the CLI. Route both Start (for
stopped) and Restart (for online) through 'bgo restart <name>',
which respawns with the stored command. _open_logs now reports
fall-through failures on stderr instead of silently no-op'ing.
- bgo: cmd_tray re-exec resolves the installed entrypoint via
shutil.which('bgo') instead of sys.argv[0], which may be the bare
script path or 'python' after a uv/pipx injection.
- tray_install: honor $UV_TOOL_DIR for non-default uv tool roots;
document the re-exec contract on ensure_installed.
- tests: +6 covering shlex quoting, malformed override fallthrough,
launchctl unload fallback + missing-agent idempotency + real
failure, and UV_TOOL_DIR detection. 144 -> 150 tests.
pystray.MenuItem._assert_action introspects the action's signature
and raises ValueError on any callable whose argspec has more than
the (icon, item) positional pair. Our previous form
lambda _icon, _it, n=snap.name: run_bgo('restart', n)
binds the loop variable via a default-kwarg, which trips that
inspector even though the call site only passes two args.
Replace every menu-action lambda with a named closure factory
(bgo_action, open_logs_action, refresh_action, quit_action). Each
returns a fresh 2-arg callable that pystray accepts.
Reproduced as 'ValueError: <function ... lambda>' on first run of
'bgo tray' under uv tool install; now starts cleanly.
…support pystray's default Xorg backend cannot dock under Wayland (it asserts on _systray_manager being non-None), and its AppIndicator backend needs distro typelibs plus a GNOME extension. Switching to PySide6 gives us a single library that speaks StatusNotifierItem natively on Linux (KDE Plasma 6, Hyprland+waybar, sway, etc.) and uses native NSStatusItem on macOS via Qt's Cocoa integration. Changes: - _tray.py rewritten: build_menu_spec / load_snapshots / ProcSnapshot / run_bgo are unchanged (still toolkit-agnostic, still tested). _run_tray now uses QApplication + QSystemTrayIcon + QMenu + QTimer. Icon is an embedded SVG rendered via QSvgRenderer at QIcon time, so we ship no binary assets. QTimer keeps polling on the GUI thread so there's no thread-unsafe menu mutation. We hold strong refs to every QAction/QMenu we create because Qt doesn't take ownership. - Detection: if QSystemTrayIcon.isSystemTrayAvailable() returns False (typical on GNOME without the AppIndicator extension or on headless sessions), we print a per-distro install hint and exit 1 instead of crashing. - _tray_install: dep list trimmed from (pystray, Pillow) to (PySide6). All three installer paths (uv tool / pipx / pip) rewritten accordingly. - pyproject [tray] extra is now just 'PySide6>=6.6'. - README: rewritten tray section with platform support matrix and an explicit GNOME prerequisite section. - Removed _select_pystray_backend / _print_appindicator_hint / _make_icon_image. Their tests are dropped — Qt handles backend selection internally and the SVG icon needs no Pillow pass. - bgo (root script): updated docstrings / help text to mention PySide6 instead of pystray/Pillow. GNOME Wayland still needs the AppIndicator shell extension — that's a GNOME limitation, no Python library can paper over it. Documented in README. Live-verified on Fedora 44 Wayland: 'bgo tray' starts cleanly, Qt event loop runs without error, no docking assertion. Test count unchanged at 150 (6 pystray-backend-selection tests removed; 6 Qt-glue paths are pragma: no cover by design).
…icon
Three tray UX improvements in one pass:
1. **Open logs** no longer opens the raw log file in $EDITOR. Instead
it spawns 'bgo logs <name> -f' in a fresh terminal window so the
user gets the project's formatted live tail. Linux probes a
curated list of emulators (kitty, alacritty, wezterm, foot,
ghostty, gnome-terminal, konsole, xfce4-terminal, tilix, xterm)
and picks the first one on PATH. macOS uses AppleScript to spawn
Terminal.app, with BGO_TERMINAL=iterm switching to iTerm2.
BGO_TERMINAL also accepts arbitrary 'binary [exec-flag]' values
for non-default emulators.
2. **Left-click activation**. By default QSystemTrayIcon only opens
the context menu on right-click; left- and middle-click did
nothing. Wired tray.activated to popup the menu at the cursor on
Trigger / MiddleClick activation reasons. Same menu, both
buttons.
3. **Gear+dot icon with status-driven dot color**:
- green (#3ddc84) when at least one proc runs and none errored
- red (#ff5252) when any proc is errored
- gray (#9e9e9e) when empty or all stopped
Icon is regenerated only when the aggregate status changes (no
wasted SVG rasterization on every 3s poll). Tooltip echoes the
status string for monochrome themes that recolor icons.
Implementation notes:
- _icon_svg(color) builds the SVG on the fly; _aggregate_status
reduces a snapshot list to one of {online, errored, idle}. Both
are pure, unit-tested without Qt.
- build_menu was renamed to build_menu_from(snapshots) so rebuild
loads procs once per tick and feeds them to both the icon-color
decision and the menu construction.
- Added 11 new tests covering terminal resolution, log-opening on
Linux/macOS, missing-terminal hint, aggregate status, and icon
SVG rendering. 150 -> 166 tests.
…ft-click Three reported issues addressed: 1. **Icon was gear teeth with blue center**. The original SVG used a single path with fill-rule=evenodd to cut the inner hole; Qt's QSvgRenderer + KDE Plasma's monochrome SNI repaint pass renders that as solid white gear, after which Plasma tints it accent-blue because it reads as monochrome. Redrawn as: - a stroked ring (the gear body), - 12 separate radial rect 'teeth', - a colored center dot. No path subtraction, no evenodd. Three explicit colors (white teeth + colored dot) means hosts no longer detect the icon as monochrome and stop recoloring it. 2. **No status colors in menu**. Qt QAction does not honor per-item foreground color across themes. Added Unicode status glyph prefix instead: '● myproc · online' / '○ myproc · stopped' / '⚠ myproc · errored'. Shape-based status communication works on every platform/theme; glyphs render reliably in KDE, GNOME, and macOS menus. 3. **Left-click didn't open the menu** on KDE Plasma. Replaced QMenu.popup() (async, gets eaten by some SNI hosts before paint) with QMenu.exec() (synchronous, force-shows). Added BGO_TRAY_DEBUG env var that logs the activation reason to stderr so users can diagnose host-specific quirks. Tests: +4 (_status_glyph matrix). 166 -> 169. README updated with full new tray behavior (icon dot colors, menu glyphs, terminal-emulator probing for Open-logs, left/middle/right click activation, BGO_TERMINAL + BGO_TRAY_DEBUG envs, autostart and 'bgo start -w' background examples). Also fixed the stale test count (54 -> 169) and Python requirement (3.9+ -> 3.10+, matching pyproject.toml since main's CI matrix dropped 3.9).
Begin modularization of the 2070-line root 'bgo' script per fu #6.
Source-of-truth for each concern moves into a focused module under
src/bgo_cli/; the root script re-imports the names so its public
namespace (and the test fixture's introspection of it) keeps working
unchanged.
Modules extracted in this pass:
- _term.py (145 LOC) — COLORS, ANSI_RE, color(), strip_ansi(),
truncate(), LEVEL_*, _detect_table_level(), GLYPHS, glyphs().
- _state.py (113 LOC) — BGO_DIR, PROCS_DIR, LOGS_DIR, init_dirs(),
proc_file(), log_path(), watcher_log_path(), watcher_log(),
load_proc(), save_proc(), delete_proc(), load_all_procs().
- _proc.py (191 LOC) — _is_zombie(), is_running(),
_BLANK_PINFO, get_process_info(), get_process_info_batch(),
_looks_like_command(), derive_name(), resolve_command(),
kill_process().
- _watcher.py (410 LOC) — WATCH_DEFAULTS, BACKOFF_SCHEDULE,
TAIL_BYTES, _notify_errored, _resolve_watch_block,
_default_watch_config, _spawn_watcher, _kill_watcher,
_tail_stderr, _restart_proc_inplace, cmd_watcher_loop.
_spawn_watcher now resolves the bgo entrypoint via shutil.which
('bgo') with sys.argv[0] fallback, instead of os.path.abspath(
__file__). The latter would point at _watcher.py once relocated;
the former hits the user's installed CLI shim consistently across
uv tool / pipx / pip / repo-checkout invocations.
conftest.py now patches BGO_DIR / PROCS_DIR / LOGS_DIR on BOTH the
loaded bgo module and bgo_cli._state (when present) so tests that
mutate either binding stay consistent.
bgo script: 2070 -> 1593 LOC. Next pass: extract cmd_* handlers,
status rendering, interactive multiselect, and the argparse +
direct-mode CLI front-end.
All 169 tests pass unchanged.
fu #5 scope: documented + strictly typed Python code. New modules
(_notify, _autostart, _tray*, _term, _state, _proc, _watcher) already
meet this bar. This commit closes the gap for the cmd_* handlers and
top-level helpers that still live in the root bgo script.
Annotations added:
cmd_start, cmd_stop, cmd_restart, cmd_watch, cmd_unwatch,
cmd_status, cmd_logs, cmd_logs_follow, cmd_clean, cmd_resurrect,
cmd_restart_stopped, cmd_restart_last, cmd_delete, cmd_autostart,
cmd_tray, main
-> every signature now (args: argparse.Namespace) -> int and
carries a docstring describing inputs, behavior, and exit-code
semantics.
_clear_screen, _print_status_table, _print_proc_detail
-> -> None return annotation and longer docstrings.
cmd_start / cmd_stop / cmd_restart / cmd_watch / cmd_status / cmd_logs
/ cmd_resurrect / cmd_delete also got expanded multi-paragraph
docstrings covering modes, side effects, and what each return code
means.
No behavior changes. 169 tests still pass.
fu #7 scope: GH-style wiki linking documentation. As lead I picked an
in-repo docs/ folder over GitHub's separate .wiki.git repo because:
- The wiki git repo is created via the GitHub web UI, can't ship with
the main branch, and isn't reviewable in PRs.
- An in-repo docs/ folder renders natively in the GitHub web UI for
navigation, supports MkDocs / Astro / GitHub Pages later, AND
participates in normal review.
Structure:
docs/README.md — index linking every page
docs/installation.md — uv / pipx / pip / install.sh / manual
docs/commands.md — full CLI reference
docs/watch-mode.md — auto-restart + fast-crash policy
docs/notifications.md — desktop notifier backends + gating
docs/autostart.md — systemd-user / launchd integration
docs/tray.md — PySide6 tray, per-platform notes
docs/architecture.md — module dep graph, storage layout,
watcher protocol
docs/faq.md — common questions and gotchas
docs/contributing.md — dev setup, tests, patches, release
Pages link to each other consistently (e.g. tray.md references
autostart.md for 'run tray at login'; commands.md links to watch-mode
.md for tunable detail).
Root README rewritten as a slim quickstart + feature highlights + a
Documentation table that points into docs/. Went 417 -> 106 lines.
Duplicated content (full command tables, install matrices, tray
caveats) lives only in docs/ now.
169 tests still pass — no code touched in this commit.
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, 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 have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (29)
📝 WalkthroughWalkthroughRefactors the single-file bgo CLI into a modular bgo_cli package (state, term, proc, watcher), adds optional tray, autostart, and notification modules, updates the bgo CLI to re-export and wire handlers, and supplies comprehensive docs and tests. ChangesCore Modularization & Optional Features
🎯 4 (Complex) | ⏱️ ~75 minutes
🚥 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 unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
bgo (1)
1191-1201:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHonor stop failure before deleting process state.
If
kill_process(...)fails, the code still deletes the proc record (and optionally logs), which can leave a live unmanaged process.Suggested fix
if is_running(pid): if not getattr(args, "yes", False): confirm = input(f"Process '{name}' is running. Stop and delete? [y/N] ") if confirm.lower() != "y": print("Cancelled.") return 0 - kill_process(pid, info.get("pgid")) + if not kill_process(pid, info.get("pgid")): + print(f"{color('red', '❌')} Failed to stop '{name}'. Aborting delete.") + return 1🤖 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 `@bgo` around lines 1191 - 1201, The current flow calls kill_process(pid, info.get("pgid")) but proceeds to delete_proc(name, ...) regardless of whether the stop succeeded; change the logic in the is_running branch to check the result or catch exceptions from kill_process (e.g., capture a boolean/raise) and only call delete_proc(name, keep_logs=keep_logs) when kill_process succeeded or the process is confirmed not running (use is_running(pid) again after kill or rely on kill_process return value); if kill fails, print an error and return non-zero without deleting the proc record. Ensure you reference the existing symbols is_running, kill_process, delete_proc, args, pid, name and info.get("pgid") when making the change.
🧹 Nitpick comments (2)
docs/tray.md (1)
56-59: 💤 Low valueAdd language identifier to fenced code block.
The code block showing the terminal emulator probe order should have a language identifier for consistency with markdown best practices. As per coding guidelines, markdownlint flagged this as MD040.
📝 Suggested fix
-``` +```text kitty → alacritty → wezterm → foot → ghostty → gnome-terminal → konsole → xfce4-terminal → tilix → xterm</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.In
@docs/tray.mdaround lines 56 - 59, The fenced code block showing the
terminal emulator probe order is missing a language identifier (MD040); locate
the triple-backtick block that contains "kitty → alacritty → wezterm → foot →
ghostty → gnome-terminal → konsole → xfce4-terminal → tilix → xterm" and add a
language tag such as "text" after the openingso the block becomestext
to satisfy markdownlint and maintain consistency.</details> </blockquote></details> <details> <summary>src/bgo_cli/_tray.py (1)</summary><blockquote> `59-66`: _💤 Low value_ **Outdated docstring references pystray.** The docstring mentions translating to "`pystray.Menu` objects" but the implementation uses PySide6/Qt. Consider updating for consistency. <details> <summary>Suggested fix</summary> ```diff `@dataclass` class MenuSpec: """Declarative description of the tray menu. - The ``run`` layer translates this into ``pystray.Menu`` objects; - tests inspect the spec directly. Keeping this layer - framework-agnostic means swapping pystray for another toolkit in - the future touches only ``_run_tray``. + The ``_run_tray`` layer translates this into Qt ``QMenu`` objects; + tests inspect the spec directly. Keeping this layer + framework-agnostic means swapping Qt for another toolkit in + the future touches only ``_run_tray``. """ ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/bgo_cli/_tray.py` around lines 59 - 66, The MenuSpec docstring still mentions translating to "pystray.Menu objects" which is outdated; update the docstring for MenuSpec to reference the actual toolkit used (PySide6/Qt) and how the run layer translates the spec into QSystemTrayIcon/QMenu (or whatever _run_tray uses), and remove or replace the pystray reference; mention the translation happens in the _run_tray function/class so readers can find the implementation. ``` </details> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>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@bgo:
- Around line 1230-1243: The status branch currently ignores a possible --tray
filter: when action == "status" and you call _autostart.status() the CLI always
prints both "tray" and "resurrect" lines and the status subparser does not
accept a --tray flag; update the status subparser to accept the same --tray
option as other actions (so args.tray exists), and update the handler for action
== "status" to respect args.tray (if args.tray is truthy only output/filter the
"tray" line, if args.tray is False only output/filter non-tray lines, and when
args.json is set still return the filtered structure from _autostart.status());
touch the code paths referencing action, args, and _autostart.status() to apply
the filter consistently (also replicate the same fix for the other occurrence
noted around lines 1616-1625).- Around line 1276-1298: The broad except ImportError around "from bgo_cli
import _tray" should be narrowed to catch ModuleNotFoundError as exc and only
handle it when exc.name indicates missing GUI deps (e.g., "PySide6" or
"shiboken6"); let other import or runtime errors in _tray propagate. In the
fallback branch that calls _tray_install.ensure_installed and re-execs, preserve
the original CLI arguments by passing [bgo_bin, *sys.argv[1:]] to os.execvp
instead of hardcoding ["bgo_bin", "tray"]. Update the except clause and the
os.execvp invocation near the imports of _tray/_tray_install and the
ensure_installed call to implement these changes.In
@docs/architecture.md:
- Around line 123-125: The sentence in docs/architecture.md contains a broken
placeholder link "(#)" referencing "fu#6partial completion" near the mention
of extractingcmd_*handlers intobgo_cli._commands; either remove the link
markup so it reads "fu#6partial completion" without "(#)" or replace "(#)"
with the actual GitHub issue/PR URL for fu#6; update the sentence accordingly
to keep the reference correct and ensurecmd_*handlers and
bgo_cli._commandsremain mentioned.In
@docs/contributing.md:
- Around line 53-56: Replace the broken placeholder link "(#)" in the "fu
#5
sweep" reference in the contributing doc: locate the sentence mentioning the
rootbgoscript andcmd_*handlers (the "fu#5sweep" phrase) and either
remove the link markup entirely or replace "(#)" with the actual GitHub issue/PR
URL; ensure the resulting text reads correctly (e.g., "fu#5sweep" without link
or "fu#5sweep (https://...)" with the real link) and keep surrounding mention
ofcmd_*signatures and docstrings unchanged.In
@src/bgo_cli/_autostart.py:
- Around line 323-327: The code currently calls _launchctl_unload(path) and then
unconditionally deletes the plist and returns success; change this so that you
capture the outcome or exception from _launchctl_unload in the block that checks
path.exists(), and only call path.unlink(missing_ok=True) and return (True, "")
if _launchctl_unload indicates success or a benign "already absent" result; if
_launchctl_unload fails for any other reason, preserve the plist, return a
failure tuple (False, "") or re-raise the error so the caller can
handle it. Update the logic around _launchctl_unload and the subsequent
path.unlink call in the _autostart removal flow to use the unload
result/exception to decide whether to delete the file and report success.In
@src/bgo_cli/_proc.py:
- Around line 87-88: The subprocess call building the ps command (where result =
subprocess.run(...), using pid_arg) uses the GNU-only "--no-headers" flag;
replace that with POSIX/BSD-compatible -o field= syntax by supplying a single -o
argument that lists each column with trailing equals (e.g., pid, %cpu, %mem,
etime) to suppress headers across platforms and remove the "--no-headers" token;
update the argument list passed to subprocess.run and ensure any downstream
parsing of result.stdout still strips/handles whitespace accordingly.In
@src/bgo_cli/_state.py:
- Around line 61-69: The loader functions (load_proc and load_all_procs)
currently accept any JSON value; update load_proc (and similarly load_all_procs)
so after parsing the file from proc_file(name) you validate the result is a
mapping (instance of dict) and return None for any non-dict JSON or errors;
i.e., if json.loads(...) returns a list/str/number/etc., treat it as corrupt and
return None instead of returning the raw value that downstream code will call
.get(...) on.- Around line 81-83: The current atomic-write uses a fixed tmp path (tmp =
pf.with_suffix(pf.suffix + ".tmp") / tmp.write_text and os.replace(tmp, pf))
which is racy for concurrent writers; change to create a unique temp file in the
same directory (e.g. via tempfile.NamedTemporaryFile or tempfile.mkstemp with
dir=pf.parent and a unique suffix), write the JSON to that unique temp path,
fsync if needed, close the handle, then call os.replace(unique_tmp, pf) to
atomically swap; ensure you reference the same variables (pf, tmp or unique_tmp)
and remove/cleanup the temp on error.In
@src/bgo_cli/_term.py:
- Around line 41-46: The color() function currently appends the reset code even
when the provided name is unknown; update it so that after the TTY check it
first verifies the color key exists in the COLORS mapping (e.g. using "if name
not in COLORS: return str(text)"), and only when the key is present wrap the
text with COLORS[name] and append COLORS['reset']; reference the color()
function, the COLORS dict, and sys.stdout.isatty() when making the change.- Around line 53-58: truncate() can produce results longer than width when width
< 3; update the function (truncate) to handle small widths explicitly: after
computing plain = strip_ansi(s) and detecting len(plain) > width, if width <= 0
return "" (or empty string), if width <= 3 return the ellipsis trimmed to width
(use "..."[:width]) so the result length never exceeds width, otherwise keep the
existing behavior of returning s[: width - 3] + "..." for wider columns;
reference truncate and strip_ansi to locate the change.In
@src/bgo_cli/_tray_install.py:
- Around line 109-111: The docstring for ensure_installed incorrectly references
"pystray + PIL" while the function actually ensures PySide6 is
importable/installed; update the ensure_installed docstring to accurately
describe that it checks for and installs PySide6 (and any other real runtime
deps used by functions in this module), mention the optional auto parameter
behavior, and remove leftover references to pystray/PIL so the docstring matches
the implementation in ensure_installed.In
@src/bgo_cli/_watcher.py:
- Around line 139-151: The code opens a log file handle (wlog via
watcher_log_path) before calling subprocess.Popen and in the except path returns
without guaranteeing the file is closed; ensure the file descriptor is always
closed on all failure paths by either using a context manager (with
open(watcher_log_path(name), "a") as wlog:) around the subprocess.Popen call or
by adding explicit wlog.close() in every except/return path; apply the same fix
to the other spawn path referenced in the file (the block around lines 202-225)
so watcher_log, _bgo_entrypoint, and any failing subprocess.Popen invocation
never leak the wlog file descriptor.In
@tests/conftest.py:
- Around line 33-57: The test imports fail during collection because the test
package path ("src/") is only added inside the bgo fixture; move the sys.path
insertion to module import/collection time by adding a top-level
sys.path.insert/append that adds the project's "src" directory before any test
code or helper functions run (i.e., before calling _load_bgo() or defining the
bgo fixture). Ensure this change happens in tests/conftest.py at the module
scope (so it runs on import), and keep references to _load_bgo(), bgo_dir,
BGO_DIR, PROCS_DIR, and LOGS_DIR unchanged so the existing fixture logic still
patches those symbols.
Outside diff comments:
In@bgo:
- Around line 1191-1201: The current flow calls kill_process(pid,
info.get("pgid")) but proceeds to delete_proc(name, ...) regardless of whether
the stop succeeded; change the logic in the is_running branch to check the
result or catch exceptions from kill_process (e.g., capture a boolean/raise) and
only call delete_proc(name, keep_logs=keep_logs) when kill_process succeeded or
the process is confirmed not running (use is_running(pid) again after kill or
rely on kill_process return value); if kill fails, print an error and return
non-zero without deleting the proc record. Ensure you reference the existing
symbols is_running, kill_process, delete_proc, args, pid, name and
info.get("pgid") when making the change.
Nitpick comments:
In@docs/tray.md:
- Around line 56-59: The fenced code block showing the terminal emulator probe
order is missing a language identifier (MD040); locate the triple-backtick block
that contains "kitty → alacritty → wezterm → foot → ghostty → gnome-terminal →
konsole → xfce4-terminal → tilix → xterm" and add a language tag such as "text"
after the openingso the block becomestext to satisfy markdownlint and
maintain consistency.In
@src/bgo_cli/_tray.py:
- Around line 59-66: The MenuSpec docstring still mentions translating to
"pystray.Menu objects" which is outdated; update the docstring for MenuSpec to
reference the actual toolkit used (PySide6/Qt) and how the run layer translates
the spec into QSystemTrayIcon/QMenu (or whatever _run_tray uses), and remove or
replace the pystray reference; mention the translation happens in the _run_tray
function/class so readers can find the implementation.</details> <details> <summary>🪄 Autofix (Beta)</summary> Fix all unresolved CodeRabbit comments on this PR: - [ ] <!-- {"checkboxId": "4b0d0e0a-96d7-4f10-b296-3a18ea78f0b9"} --> Push a commit to this branch (recommended) - [ ] <!-- {"checkboxId": "ff5b1114-7d8c-49e6-8ac1-43f82af23a33"} --> Create a new PR with the fixes </details> --- <details> <summary>ℹ️ Review info</summary> <details> <summary>⚙️ Run configuration</summary> **Configuration used**: defaults **Review profile**: CHILL **Plan**: Pro **Run ID**: `ff0bddd5-a054-4f4a-ba50-ed07d3bd7819` </details> <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 3c947b21ba532ad1431324c973a22295704aa0bd and 2fedd2aceb5ae2aac15db9b69a8f72ddefa1c10e. </details> <details> <summary>⛔ Files ignored due to path filters (1)</summary> * `.mycelium/mycelium.db` is excluded by `!**/*.db` </details> <details> <summary>📒 Files selected for processing (28)</summary> * `.gitignore` * `README.md` * `bgo` * `docs/README.md` * `docs/architecture.md` * `docs/autostart.md` * `docs/commands.md` * `docs/contributing.md` * `docs/faq.md` * `docs/installation.md` * `docs/notifications.md` * `docs/tray.md` * `docs/watch-mode.md` * `pyproject.toml` * `src/bgo_cli/__init__.py` * `src/bgo_cli/_autostart.py` * `src/bgo_cli/_notify.py` * `src/bgo_cli/_proc.py` * `src/bgo_cli/_state.py` * `src/bgo_cli/_term.py` * `src/bgo_cli/_tray.py` * `src/bgo_cli/_tray_install.py` * `src/bgo_cli/_watcher.py` * `tests/conftest.py` * `tests/test_autostart.py` * `tests/test_notify.py` * `tests/test_tray.py` * `tests/test_tray_install.py` </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
Qt's app.exec() blocks in a native select() loop, so Python signal handlers never run. Install handlers that call app.quit() and add a 200ms no-op QTimer to wake the interpreter often enough to deliver pending signals.
CI: - workflow now installs the package (`pip install -e .[dev]`) so test modules can `import bgo_cli` without a sys.path hack on disk - conftest.py also inserts `src/` at collection time as a belt-and- braces fallback for local pytest runs from the repo root Quality: - _state.save_proc: unique tmp file via tempfile.mkstemp; previous shared `*.tmp` path was racy under concurrent writers - _state.load_proc / load_all_procs: reject non-mapping JSON so a corrupt list/string can't crash callers that do `.get(...)` - _proc.get_process_info_batch: use POSIX-portable `pid=,%cpu=,...` header suppression; `--no-headers` is GNU-only and broke macOS ps - _term.color: unknown names now return the text untouched (was appending a stray reset code, violating the documented contract) - _term.truncate: clamp at narrow widths so the 3-char ellipsis can no longer overflow into width<3 columns - _watcher: open log handles get closed via `finally`, so repeated spawn failures don't leak FDs - _autostart.uninstall: propagate launchctl unload errors instead of silently deleting the plist and orphaning the agent - _tray_install.ensure_installed: docstring referenced pystray/PIL (leftover from pre-PySide6 migration) CLI: - `autostart status --tray` is now wired through (subparser flag + handler), matching the help text - `cmd_tray` re-exec forwards `sys.argv[1:]` so `--poll` survives Docs: - replace `(#)` placeholder links in architecture.md / contributing.md with real issue URLs
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@src/bgo_cli/_tray.py`:
- Around line 99-108: The function load_snapshots currently sorts JSON files by
filename before calling _load_one, which can produce the wrong order if a
ProcSnapshot's internal name differs from the filename; change load_snapshots
(and use ProcSnapshot.name) so it first loads all snapshots via _load_one into
out and then return them sorted by each snapshot.name (e.g., using sorted(out,
key=lambda s: s.name)) while still filtering out None results.
- Around line 284-297: The AppleScript string is vulnerable because `quoted`
(produced from `follow_cmd` using `shlex.quote`) is interpolated directly into
`script`; escape the AppleScript layer by escaping backslashes and double quotes
in `quoted` before interpolation (e.g. replace "\" with "\\\\" and `"` with
"\\\"") and use that escaped value in both `if use_iterm:` and `else:` branches
when building `script` (references: `follow_cmd`, `shlex.quote`, `quoted`,
`use_iterm`, and `script`). Ensure the escaping happens immediately after
`quoted = ...` so all subsequent uses are safe.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
- load_snapshots: sort after parsing so order follows the snapshot's stored `name` field (filename can drift from name after rename) - _open_logs_darwin: escape backslashes + double quotes in the shell-quoted command before interpolating into the AppleScript string literal — paths containing either would otherwise terminate the AppleScript literal early
Owner
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Highlights
Verification
Test plan
Breaking changes
None. Existing commands (`start`, `stop`, `restart`, `status`, `logs`, `watch`, etc.) and all flags / env vars / exit codes are unchanged. The new `autostart` and `tray` subcommands are purely additive.
Summary by CodeRabbit
New Features
Documentation
Requirements
Tests
Chores