feat(service): devstrap service install|uninstall|status (P4-PROD-04)#139
Merged
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (28)
📝 WalkthroughWalkthroughThis PR adds ChangesBackground Service Management
Estimated code review effort: 4 (Complex) | ~75 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Wrap run-loop in a per-user launchd LaunchAgent (com.devstrap.run-loop, modern bootstrap/bootout/print) or systemd --user service (devstrap-run-loop.service) so the workspace converges unattended. Enriched ServiceSpec/ServiceManager behind internal/platform; untagged golden-tested renderers; install refuses an unconfigured hub and ephemeral exec paths, writes units atomically 0600, bakes no secrets; doctor gains an optional service check. Live launchd dogfood caught and fixed a status misparse (nested state lines) and a reinstall race (asynchronous bootout). Dual-review fixes: label validation (path traversal via --label), fail-closed control-character gates in both renderers (systemd unit-line injection), absolutized root flags. Ledger: P4-PROD-04 -> Recently shipped; spec/14 installer rows flipped. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Reederey87
force-pushed
the
feat/p4-prod-04-service-install
branch
from
ce08940 to
0b2ff32
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
internal/cli/service_test.go (1)
67-101: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd coverage for
--root/--configabsolutizing.
TestServiceInstallBuildsRunLoopArgsonly exercises--home. The commit message specifically calls out "absolutizing propagated root flags" as a dogfooding/review fix, so--rootand--configdeserve direct assertions too (e.g. a relative--root/--configresolving to an absolute path in the baked args), not just--home.🤖 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 `@internal/cli/service_test.go` around lines 67 - 101, `TestServiceInstallBuildsRunLoopArgs` only verifies `--home`, but the install path should also cover absolutizing propagated root flags. Extend this test (or add a sibling in the same service test suite) to pass relative `--root` and `--config` values through `executeForTest`, then assert the baked `installedSpec.Args` from `fakeServiceManager` contain their absolute equivalents; use the existing `withFakeService`, `executeForTest`, and `installedSpec.Args` checks to locate the flow.internal/cli/service.go (1)
82-85: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate ErrUnsupported wrapping + label-resolution logic across subcommands.
The
errors.Is(err, platform.ErrUnsupported)wrap block is repeated verbatim in install/uninstall/status, and theresolvedLabel := label; if resolvedLabel == "" { resolvedLabel = mgr.DefaultLabel() }pattern is also repeated 3x (lines 62-65, 117-120, 160-163). Extracting small helpers would reduce drift risk if the message or fallback logic ever needs to change.♻️ Suggested helpers
+func wrapServiceErr(err error) error { + if errors.Is(err, platform.ErrUnsupported) { + return appError{code: exitGeneric, err: fmt.Errorf("background service is not supported on this platform/session: %w", err)} + } + return err +} + +func resolveServiceLabel(label string, mgr platform.ServiceManager) string { + if label == "" { + return mgr.DefaultLabel() + } + return label +}Also applies to: 125-129, 166-169
🤖 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 `@internal/cli/service.go` around lines 82 - 85, The same ErrUnsupported wrapping and label fallback logic is duplicated across the service subcommands, so extract small helpers in service.go to keep install, uninstall, and status consistent. Create one helper for resolving the label from label/mgr.DefaultLabel(), and another for converting platform.ErrUnsupported into the appError with the shared message, then replace the repeated blocks in the subcommand handlers with those helpers.internal/cli/doctor.go (1)
256-256: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winUnbounded
mgr.Statuscall inside a health check.
checkServiceshells out tolaunchctl print/systemctl statusviamgr.Status(ctx, label)with whatever contextdoctoris given, and this diff doesn't add a timeout around it. If the OS service manager ever hangs (documented as happening with launchd in some environments),devstrap doctor— which is expected to run many quick checks — could hang indefinitely on this one check.Please confirm whether
cmd.Context()upstream already carries a deadline for CLI commands; if not, consider wrapping this specific call withcontext.WithTimeout.🤖 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 `@internal/cli/doctor.go` at line 256, The checkService health check is calling mgr.Status with an unbounded context, so a hanging service manager can stall doctor indefinitely. Update checkService to use a bounded context for this specific mgr.Status call, either by confirming cmd.Context() already has a deadline or by wrapping it with context.WithTimeout before invoking mgr.Status, then canceling it promptly afterward. Refer to checkService and mgr.Status to place the fix in the right spot.internal/platform/service_darwin.go (1)
45-63: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
spec.Envmutation leaks into the caller's map.
ServiceSpecis passed by value, butEnvis amap[string]string; when the caller supplies a non-nilEnv, writingspec.Env["PATH"] = ...here mutates the same underlying map the caller holds a reference to, since map headers are copied by value but point at shared backing data. If the CLI layer reuses, inspects, or logs the original spec after callingInstall(e.g. forstatus --jsonor diagnostics), it will observe the injectedPATHit never set.Clone the map before mutating it so
Installnever has externally-visible side effects on caller-owned data.♻️ Proposed fix to avoid mutating the caller's map
- if spec.Env == nil { - spec.Env = map[string]string{} - } + env := make(map[string]string, len(spec.Env)+1) + for k, v := range spec.Env { + env[k] = v + } + spec.Env = env if spec.Env["PATH"] == "" { spec.Env["PATH"] = defaultDarwinPath(filepath.Dir(spec.ExecPath)) }🤖 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 `@internal/platform/service_darwin.go` around lines 45 - 63, The Install method in LaunchdManager is mutating the caller-owned ServiceSpec.Env map when it seeds PATH, which leaks side effects outside the function. In LaunchdManager.Install, make a shallow copy of spec.Env before any writes, then apply the PATH default on the copied map so the original spec passed by the caller is never modified. Keep the existing validation and defaultDarwinPath logic unchanged, but ensure all Env updates happen on the cloned map.
🤖 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 `@internal/cli/doctor.go`:
- Around line 253-256: `checkService` is always using `mgr.DefaultLabel()`, so
`doctor` can miss a service installed with a custom label. Update the
`checkService` flow in `doctor.go` to accept and use the same label passed at
install time (or read the persisted label) instead of hardcoding the default.
Make sure the label used by `checkService` matches the one used by the
install/status path so the status check and reinstall hint target the correct
unit name.
In `@internal/platform/service_linux.go`:
- Around line 47-50: The availability errors in service_linux.go are losing the
underlying failure because the fmt.Errorf calls in the systemd checks only wrap
ErrUnsupported and print err with %v. Update both error returns in the systemd
probing path (around the runSystemctl/systemd user manager checks) to wrap err
with %w as well so the original availability failure stays in the error chain.
---
Nitpick comments:
In `@internal/cli/doctor.go`:
- Line 256: The checkService health check is calling mgr.Status with an
unbounded context, so a hanging service manager can stall doctor indefinitely.
Update checkService to use a bounded context for this specific mgr.Status call,
either by confirming cmd.Context() already has a deadline or by wrapping it with
context.WithTimeout before invoking mgr.Status, then canceling it promptly
afterward. Refer to checkService and mgr.Status to place the fix in the right
spot.
In `@internal/cli/service_test.go`:
- Around line 67-101: `TestServiceInstallBuildsRunLoopArgs` only verifies
`--home`, but the install path should also cover absolutizing propagated root
flags. Extend this test (or add a sibling in the same service test suite) to
pass relative `--root` and `--config` values through `executeForTest`, then
assert the baked `installedSpec.Args` from `fakeServiceManager` contain their
absolute equivalents; use the existing `withFakeService`, `executeForTest`, and
`installedSpec.Args` checks to locate the flow.
In `@internal/cli/service.go`:
- Around line 82-85: The same ErrUnsupported wrapping and label fallback logic
is duplicated across the service subcommands, so extract small helpers in
service.go to keep install, uninstall, and status consistent. Create one helper
for resolving the label from label/mgr.DefaultLabel(), and another for
converting platform.ErrUnsupported into the appError with the shared message,
then replace the repeated blocks in the subcommand handlers with those helpers.
In `@internal/platform/service_darwin.go`:
- Around line 45-63: The Install method in LaunchdManager is mutating the
caller-owned ServiceSpec.Env map when it seeds PATH, which leaks side effects
outside the function. In LaunchdManager.Install, make a shallow copy of spec.Env
before any writes, then apply the PATH default on the copied map so the original
spec passed by the caller is never modified. Keep the existing validation and
defaultDarwinPath logic unchanged, but ensure all Env updates happen on the
cloned map.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f9183084-1284-4266-93ec-fa105d218e5b
📒 Files selected for processing (28)
docs/audits/README.mdinternal/cli/doctor.gointernal/cli/doctor_test.gointernal/cli/root.gointernal/cli/run_loop.gointernal/cli/service.gointernal/cli/service_test.gointernal/platform/detect_darwin.gointernal/platform/detect_linux.gointernal/platform/platform.gointernal/platform/platform_test.gointernal/platform/service_darwin.gointernal/platform/service_darwin_test.gointernal/platform/service_launchd.gointernal/platform/service_launchd_test.gointernal/platform/service_linux.gointernal/platform/service_linux_test.gointernal/platform/service_systemd.gointernal/platform/service_systemd_test.gointernal/platform/testdata/run_loop.plist.goldeninternal/platform/testdata/run_loop.service.goldenspec/00_START_HERE.mdspec/05_MAC_FIRST_IMPLEMENTATION.mdspec/06_LINUX_COMPATIBILITY.mdspec/13_CLI_DAEMON_API.mdspec/14_MVP_ROADMAP_AND_BACKLOG.mdspec/16_TEST_PLAN.mdspec/18_WORK_LOG.md
Comment on lines
+253
to
+256
| func checkService(ctx context.Context, opts *options) []checkResult { | ||
| mgr := serviceBackend() | ||
| label := mgr.DefaultLabel() | ||
| status, err := mgr.Status(ctx, label) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if the chosen service label is persisted anywhere (config/state) for doctor to read back.
rg -nP '\bservice\.label\b|serviceLabel|SavedLabel' --type=goRepository: Reederey87/DevStrap
Length of output: 157
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ast-grep outline internal/cli/doctor.go --view expanded
printf '\n---\n'
ast-grep outline internal/cli/service.go --view expanded
printf '\n---\n'
rg -n "DefaultLabel|label|install|doctor|Status\(" internal/cli internal -g '*.go'
printf '\n--- service backend files ---\n'
rg -n "type .*Backend|func .*DefaultLabel|func .*Status|label" internal -g '*.go'Repository: Reederey87/DevStrap
Length of output: 50375
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- internal/cli/doctor.go (240-275) ---'
sed -n '240,275p' internal/cli/doctor.go
echo
echo '--- internal/cli/service.go (36-200) ---'
sed -n '36,200p' internal/cli/service.go
echo
echo '--- service manager interface ---'
rg -n "type ServiceManager interface|DefaultLabel|Status\(" internal -g '*.go'Repository: Reederey87/DevStrap
Length of output: 12833
doctor should accept the service label used at install time
checkService always probes mgr.DefaultLabel(), so a devstrap service install --label <custom> can still be reported as “not installed” and the reinstall hint points at a second default-labeled instance. Add a --label path here, or persist the chosen label, so doctor checks the same unit name as install and status.
🤖 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 `@internal/cli/doctor.go` around lines 253 - 256, `checkService` is always
using `mgr.DefaultLabel()`, so `doctor` can miss a service installed with a
custom label. Update the `checkService` flow in `doctor.go` to accept and use
the same label passed at install time (or read the persisted label) instead of
hardcoding the default. Make sure the label used by `checkService` matches the
one used by the install/status path so the status check and reinstall hint
target the correct unit name.
Reederey87
added a commit
that referenced
this pull request
Jul 6, 2026
spec/14 DIRECTION bullet marking the wave complete (draft-pending quarantine #133, self-healing WCK rotation #134, squash-merge worktree GC P4-GIT-04, devstrap service P4-PROD-04) with the post-wave backlog and the Pass-7 audit checkpoint; ledger intro trail extended. Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
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.
Ships audit finding P4-PROD-04:
devstrap service install|uninstall|status— the run-loop wrapped in a per-user launchd LaunchAgent (macOS) or systemd--userservice (Linux), so the workspace converges unattended without a bespoke daemon. Ledger row moved to Recently shipped; spec/14's two installer rows flipped.Design
ServiceSpecenriched (Description, WorkingDir, launchd-only log paths, RestartOnFailure/RestartDelaySeconds);ServiceManagergainsDefaultLabel()andInstall → (notes, err)so OS-idiomatic advisories (the Linux linger note) originate in the adapter — the goos-guard keeps OS branching out of the CLI. Untagged, golden-tested render/argv/parse logic; taggedLaunchdManager(com.devstrap.run-loop, modernbootstrap/bootout/print, idempotent reinstall) andSystemdUserManager(devstrap-run-loop.service, availability probe → typedErrUnsupported, linger advisory).$TMPDIR/go-buildexec path; bakesrun-loopargs (interval,--namespace-only, absolutized--hub-file/--home/--root/--configwhen explicitly set);Envstays nil — adapters add only PATH, no secret ever enters a unit file (test-pinned).statushonors--json;doctorgains an optionalrun-loop servicecheck.Live dogfood (real launchd, this Mac) caught two real bugs pre-merge
launchctl printemits the service's top-levelstate = runningbefore nested per-endpointstate = activelines — last-match parsing misreported a live service as stopped. Parse now takes each key's first occurrence (regression-pinned with the real output).bootouttears down asynchronously; an immediate reinstall'sbootstrapraced the dying job (Bootstrap failed: 5: Input/output error). Install now polls until the label leaves the domain (bounded ~3s) before bootstrapping.Also live-verified: fresh install → real tick ("run-loop tick: scan + sync + materialize"), running status with pid, reinstall over a running service, idempotent uninstall. No residue left.
Dual-review fixes (coordinator + Codex)
validateServiceLabel):--label ../../evilpreviously wrote/deleted files outside~/Library/LaunchAgents/~/.config/systemd/userviafilepath.Joinand corrupted launchctl domain targets. Now gated at every adapter entry point.\nin an exec path/arg/env value injected arbitrary directives pastsystemdQuote; raw control bytes also make launchd reject a plist silently. Both renderers now refuse any control character.--home/--root/--configbaked into a unit would resolve against the supervisor's cwd, not the install-time cwd.atomicWriteguarantees no partial read, not crash-durability.Docs
spec/00 (inventory + not-implemented truth-up), spec/05 (shipped installer replaces the deferred
daemon install; PLAT-05 resolved; exit-78/127 troubleshooting), spec/06 (unit shape + linger + fail-closed keychain custody — noDEVSTRAP_NO_KEYCHAINauto-bake, per P6-XP-04), spec/13### service, spec/14, spec/16, ledger, work log.Validation
gofmtclean ·go builddarwin+linux ·GOOS=linux go vet·golangci-lint run0 issues ·go test -race ./...ok · linux-tagged tests green in Docker ·TestEveryCommandIsDocumented·spec-drift --base origin/mainpass · live launchd dogfood (see above) · dual review (opus implementation + coordinator line-by-line + Codex).Follow-up noted in the work log: a native-Linux golangci-lint pass before the next Linux-touching PR.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
devstrap servicewithinstall,uninstall, andstatus(macOS LaunchAgent and Linuxsystemd --user).service statussupports--json, and surfaces useful log/unit details.doctornow reports whether the background service is installed and running (when supported).Bug Fixes
Documentation
devstrap serviceworkflow.