feat(cli): add gptme service init scaffold for headless agents - #3574
Conversation
Greptile SummaryThe PR adds a
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains. Important Files Changed
Reviews (9): Last reviewed commit: "fix(service): escape systemd unit values..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
Pushed
All 5 existing tests still pass. |
|
@greptileai review |
|
Pushed On-demand timer deletion now respects Previously, reinitializing with
Added two regression tests covering both branches. All 7 tests pass. |
|
@greptileai review |
|
@greptileai review |
|
@greptileai review |
|
Pushed Root cause: Fix: call New test: |
|
@greptileai review |
| click.echo( | ||
| f" Disabling loaded timer unit {name}.timer (on-demand mode, --force)" | ||
| ) | ||
| subprocess.run( |
There was a problem hiding this comment.
When systemctl --user disable --now returns a nonzero result, this branch ignores the failure and unconditionally deletes the timer file, leaving the loaded timer active while making subsequent cleanup harder.
There was a problem hiding this comment.
Fixed in b26c46b — stale_timer.unlink() is now inside the else: branch and only executes when result.returncode == 0. A failed disable prints a warning but leaves the file in place so the loaded unit remains resolvable for manual cleanup. New test test_on_demand_force_preserves_timer_when_disable_fails covers this path.
🤖 AI code reviewThis pull request introduces a new CLI subcommand Confidence Score: 5/5 — No findings on this head✅ No findings. The diff looks correct to me on this pass. Files changed (3) — the diff as I read it
Previous review passes
Reviewed Maintainer commands
|
|
@greptileai review |
| result = subprocess.run( | ||
| ["systemctl", "--user", "disable", "--now", f"{name}.timer"], | ||
| check=False, # best-effort; unit may not be enabled | ||
| ) |
There was a problem hiding this comment.
If systemctl is unavailable, running on-demand initialization with --force and an existing timer raises an uncaught FileNotFoundError after writing the service unit but before generating the remaining workspace files, leaving a partial scaffold and the stale timer intact.
There was a problem hiding this comment.
Fixed in b26c46b — the subprocess.run call is now wrapped in try/except FileNotFoundError. When systemctl is unavailable the code emits a clear warning and continues generating all workspace files (service unit, gptme.toml, AGENTS.md, startup script) without crashing. New test test_on_demand_force_handles_missing_systemctl covers this path.
|
Ready for review — all Greptile P1 threads addressed (2 real bugs fixed, 7 false positives with replies), CI running on b26c46b. Bugs fixed in b26c46b:
3 new tests cover these paths (disable success → file removed; disable fails → file preserved; missing systemctl → file preserved, warning emitted). Self-merge blocked: |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
🔍 Convergence adjudication — #3574Two real bugs fixed in c00a92e (pushed just now; thread replies posted): Fixed this session
2 new tests: Previously fixed (b26c46b, stale Greptile threads)
Greptile threads 3816589459 and 3816645954 were raised against older commits and have been replied to with the fix SHA; they haven't been formally resolved (Greptile hasn't re-reviewed c00a92e). CIPending on c00a92e. Lint + typecheck passed on prior commits. Domain riskNew module ( Self-mergeBlocked — new module outside the self-merge allowlist. Needs maintainer review. Convergence: detector reports |
|
CI completed on c00a92e — 2 failures, neither caused by this PR:
The PR changes ( |
|
@greptileai review |
|
@greptileai review |
Adds a 'gptme service init' subcommand (registered as the gptme-service console script) that scaffolds a persistent headless gptme agent as a systemd user service, following Bob's autonomous-agent pattern. Generates: - systemd service unit (configurable model, work-dir) - optional systemd timer (hourly/daily/weekly/on-demand) - gptme-agent-run.sh startup script - skeleton gptme.toml and AGENTS.md The command only writes files; installation (systemctl enable --now) is left to the operator. Includes integration tests validating generated Bash syntax, systemd unit parse, and file generation. Refs #1294
- Remove Requires= from timer unit (prevented immediate service start on --now) - Fix weekly schedule (was identical to daily; correct: Mon *-*-* 00:00:00) - Quote ExecStart path to handle work-dirs with spaces - Remove stale timer file when reinitializing with --timer-schedule on-demand - Gate startup script chmod behind write check (skip mode must not mutate perms)
…emand mode When switching to --timer-schedule on-demand with --force, the old code deleted the timer file before issuing the systemctl disable --now guidance, which meant systemd could no longer resolve the unit name to stop it. Fix: call subprocess.run systemctl --user disable --now best-effort before unlinking the file, matching systemd's established cleanup order (disable -> unlink). The call is check=False so a non-enabled or absent unit does not abort the command. Add a mock-based test that verifies disable fires while the timer file still exists, covering the previously unreachable failure path.
…oval When --force removes a stale timer for on-demand reinit, systemctl disable --now may return non-zero (e.g. unit not enabled). Previously the failure was silently swallowed. Now emit a warning so the user knows periodic runs may continue until the next daemon-reload. Also strengthens the test to assert the warning is printed when the mocked disable call fails (returncode=1).
…mctl - Skip timer file removal when systemctl disable returns nonzero, so the loaded unit remains resolvable for manual cleanup - Catch FileNotFoundError from subprocess.run when systemctl is unavailable, emit a clear warning, and let scaffolding continue without crashing - Expand tests: success path (disable→remove), failure path (preserve+warn), and missing-systemctl path (scaffold continues)
…e charset User= is not supported in systemd user units — the user manager rejects any unit containing it with a load error, making the generated service unloadable. Removed User=%u from the service template; user units already run as the invoking user by default. --name values were inserted verbatim into the generated startup script, allowing shell-metacharacter injection. Added _NAME_RE validation (letters, digits, hyphens, underscores only) that rejects unsafe names at the CLI layer. Also use shlex.quote for work_dir in the startup script template to handle paths with unusual characters. Two new tests: test_service_unit_has_no_user_directive and test_invalid_name_rejected (4 bad-name cases).
… unit-parse test - Escape work_dir for the systemd unit template: double literal '%' to prevent specifier expansion, reject embedded newlines that could inject arbitrary unit directives (P1 finding). - Add a 30s timeout to the 'systemctl --user disable --now' subprocess call so a hung user manager can't block the CLI indefinitely (P2 finding). - test_systemd_unit_parses now requires returncode == 0 instead of accepting (0, 1), which let any 'systemd-analyze verify' error pass silently (P2 finding). - New tests: work-dir '%' escaping, work-dir newline rejection.
3d0aa7d to
bc2fbc6
Compare
|
Rebased the branch onto current |
|
Pushed
Added regression coverage for each path. Local verification: 18 service-command tests pass; Ruff and targeted mypy pass. Fresh CI and the AI reviewer are running automatically on the new head. |
|
Pushed
Added regression tests for portability and permission repair. Local verification: 20 service-command tests pass, Ruff passes, Bash syntax passes, and pre-commit passes. The prior CI failure was unrelated to this PR: |
|
Reran only the failed |
|
Pushed
Added regression tests for both cases. Local verification: 22 service-command tests pass, Ruff passes, targeted pre-commit (including mypy) passes, |
|
The current AI-review P1 is a false positive, not a code defect. Python No additional product-code change or Greptile trigger was made. The PR remains outside the self-merge allowlist because it adds a new product module and changes |
|
@TimeToBuildBob See the pre-existing Should also clearly reference gptme-agent-template as the "full" version. |
|
Thanks for merging, and noted on the follow-up. I checked
I'll open the follow-up PR in a separate branch. If you have a specific file in gptme-agent-template that already encodes the canonical launchd plist (so I'm not starting from scratch), let me know and I'll reuse it. |
* feat(service-init): add launchd support for macOS agents + reference gptme-agent-template Add platform-aware scaffolding: - New --platform flag (linux/macos/auto) to select target OS - Auto-detect platform on macOS/Linux - launchd plist generation for macOS with StartInterval scheduling - Logs directory creation for plist output redirection - Updated docstring to reference gptme-agent-template as the full version - Tests for launchd plist validity, XML structure, and scheduling Addresses Erik's feedback on #3574 to improve code reuse by referencing the gptme-agent-template examples and adding macOS support. * fix(service-init): xml-escape plist values; fix inverted RunAtLoad for on-demand * fix(service-init): scope systemd escaping to linux branch only Move _escape_systemd_value/_escape_systemd_exec_value calls inside the else (systemd) branch. They were running unconditionally before the platform check, raising click.BadParameter for valid macOS paths containing apostrophes, backslashes, or quotes — which systemd forbids but launchd allows. Add regression test: macOS scaffolding with apostrophe in work-dir path must succeed without BadParameter. * fix(service-init): strip XML 1.0-forbidden control chars before plist interpolation saxutils.escape handles entity refs but preserves control chars that XML 1.0 forbids (U+0000-U+0008, U+000B, U+000C, U+000E-U+001F, U+FFFE, U+FFFF). Add _xml_safe() helper that strips those first, then switch _generate_launchd_plist to use it for name/model/work_dir. Also add a regression test. * fix(service-init): extend XML forbidden-char filter to include lone surrogates UTF-16 surrogates (U+D800-U+DFFF) are also invalid in XML 1.0 and survive saxutils.escape unchanged. Add them to _XML10_FORBIDDEN and extend the regression test to verify they are stripped from plist output. * fix(service-init): validate work-dir for XML-forbidden chars on macOS instead of silently sanitizing Silently stripping chars from work_dir in the plist would create a path mismatch (the startup script is written to the original path). Reject at input instead, with a clear error message. model continues to use silent stripping since it is only an env-var string, not a path reference. * fix(service-init): use \uFFFE/\uFFFF escapes in regex to avoid mypy INTERNAL ERROR Literal U+FFFE/U+FFFF noncharacters in the source file caused mypy to crash with INTERNAL ERROR. Replace them with \uFFFE\uFFFF escape sequences — Python's re module handles \uXXXX in raw strings, so the filter remains correct. * fix(service-init): address AI review P2 findings — platform, model validation, macOS note - _detect_platform: raise UsageError for non-Linux/non-Darwin systems (Windows, FreeBSD, etc.) instead of silently falling back to 'linux' - Validate model consistently before both systemd and launchd branches: reject newlines/quotes/backslashes so a malicious model value is caught on macOS just as it is on Linux (previously it was silently truncated) - Emit an informative note on stderr when --platform auto resolves to macOS so users aren't surprised by the switch from systemd → launchd - Add 3 new tests covering all three fixes (64 → 34 total in file) * fix(service-init): validate model/work-dir before creating dirs; fix vacuous test - Move model validation before directory creation so invalid inputs don't leave behind side-effect directories (logs dir, output dir, work dir). - Strip mkdir from _resolve_work_dir (now pure path resolution); call work.mkdir() explicitly only after all validation passes. - On macOS, XML-forbidden work-dir check also runs before mkdir. - test_macos_autodetect_emits_warning: drop --output-dir so the launchd note path (fires only when output_dir is None) is actually exercised; tighten assertion to 'launchd plist' to prevent vacuous pass via scaffold line. * fix(service-init): fix test hermeticity, quote launchctl path, pre-validate Linux work-dir - test_macos_autodetect_emits_warning: pass env HOME=tmp_path so the default '~/Library/LaunchAgents' expands into tmp, not the real FS - quote the plist path in 'launchctl load' instruction so paths with spaces copy-paste correctly - call _escape_systemd_value(str(work)) before work.mkdir on the Linux path so BadParameter fires before any directory side-effects Co-Authored-By: Bob <bob@superuserlabs.org> * fix(service-init): reject model with XML-forbidden chars instead of silently stripping Silent stripping via _xml_safe could cause the agent to run with a different (or nonexistent) model than the user requested. Now raises click.BadParameter on macOS when the model contains XML 1.0-forbidden control chars or lone surrogates, consistent with work-dir validation. _generate_launchd_plist now uses _xml_escape (not _xml_safe) for model and work_dir, since both are pre-validated by the caller. Updates test_launchd_plist_strips_control_characters → renamed to test_launchd_model_with_forbidden_chars_rejected (expects exit_code != 0).
… reference (#3614) Completes the follow-up to #3574 (launchd support). This refactoring addresses the overlap between cmd_service.py templates and gptme-agent-template examples: **Changes:** - Remove the _xml_safe() helper; consolidate to _xml_escape() for consistency (1.2) - Update AGENTS.md template to clearly state the minimal-vs-full-template boundary - Update CLI help and generated scaffold to reference gptme-agent-template as the batteries-included version (1.4) - Add tests verifying help text mentions launchd and template reference - Add tests verifying generated AGENTS.md mentions launchd and template **Work items completed:** - 1.1: Audited overlap (templates in agent-template are more advanced; cmd_service creates minimal scaffold by design — no refactor needed, just clearer docs) - 1.2: Consolidated _xml_safe() into direct _xml_escape() calls - 1.3: Launchd plist generation already shipped in #3586 - 1.4: Updated help text and generated files (done) - 1.5: Tests added and exist (test-cmd-service.py) - 1.6: Ready for PR This unblocks the 13 parallel sessions waiting on this follow-up work. Closes: #3574 (follow-up)
2 participants
Adds a
gptme service initsubcommand that scaffolds a persistent headless gptme agent as a systemd user service — the pattern Bob uses to run 100+ autonomous sessions/day, without forcing operators to reverse-engineer an agent workspace.What it generates
Design
gptme-serviceconsole script; dispatched via the existinggptme CMD → gptme-CMDplugin mechanism.systemctl enable --nowis left to the operator.Testing
ruff,mypy, and pre-commit all clean.Refs #1294 (headless-agent demand).