Skip to content

feat: add Builder background service#167

Merged
Nek-12 merged 11 commits into
mainfrom
feat/builder-service
Apr 27, 2026
Merged

feat: add Builder background service#167
Nek-12 merged 11 commits into
mainfrom
feat/builder-service

Conversation

@Nek-12

@Nek-12 Nek-12 commented Apr 27, 2026

Copy link
Copy Markdown
Member

Summary

  • add builder service lifecycle commands for the Builder server background service
  • implement macOS launchd, Linux systemd user, and Windows scheduled-task/startup fallback backends
  • add Homebrew service caveat/restart hook and Builder Server docs

Verification

  • go test ./...
  • GOOS=linux GOARCH=arm64 go test -c ./cli/builder -o /tmp/builder-cli-linux.test
  • GOOS=windows GOARCH=amd64 go test -c ./cli/builder -o /tmp/builder-cli-windows.test.exe
  • ./scripts/test.sh ./...
  • ./scripts/build.sh --output ./bin/builder
  • pnpm --dir docs test
  • pnpm --dir docs build

Summary by CodeRabbit

  • New Features

    • Added builder service command for installing, starting, stopping, and managing a shared Builder background server.
    • Background service auto-starts at login with per-platform supervisor support (macOS, Linux, Windows).
    • Service status available in human-readable or JSON format.
  • Documentation

    • New Builder Server documentation with setup and management guidance.
    • Updated Quickstart with optional service installation instructions.
    • Added post-install hook to Homebrew formula for service restart.

@coderabbitai

coderabbitai Bot commented Apr 27, 2026

Copy link
Copy Markdown
Contributor

Warning

Rate limit exceeded

@Nek-12 has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 15 minutes and 0 seconds before requesting another review.

To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: b9a43bc0-ae82-46fe-bba0-50f3f61a357a

📥 Commits

Reviewing files that changed from the base of the PR and between ca448f6 and 8a35071.

📒 Files selected for processing (9)
  • cli/builder/service_backend_linux.go
  • cli/builder/service_backend_windows.go
  • cli/builder/service_backend_windows_test.go
  • cli/builder/service_command.go
  • cli/builder/service_command_test.go
  • cli/builder/service_types.go
  • docs/src/content/docs/quickstart.md
  • server/serve/serve.go
  • shared/protocol/protocol.go
📝 Walkthrough

Walkthrough

This pull request introduces a platform-specific background service management system for Builder, enabling users to run a persistent local server process via builder service commands. It implements lifecycle operations (install, uninstall, start, stop, restart, status) backed by platform-specific supervisors: LaunchAgent on macOS, systemd on Linux, and scheduled tasks on Windows. The feature includes CLI routing, help documentation, conflict detection, health probing, and comprehensive tests.

Changes

Cohort / File(s) Summary
Service Backend Implementations
cli/builder/service_backend_darwin.go, cli/builder/service_backend_linux.go, cli/builder/service_backend_windows.go, cli/builder/service_backend_other.go
Platform-specific service lifecycle management: macOS via LaunchAgent plist and launchctl; Linux via ~/.config/systemd/user/ units and systemctl --user; Windows via scheduled tasks with Startup-folder fallback; unsupported platforms return error for management operations.
Service Command & Types
cli/builder/service_command.go, cli/builder/service_types.go
CLI subcommand routing for status, install, uninstall, start, stop, restart with flags (--force, --no-start, --keep-running, --if-installed, --json). Conflict detection via HTTP health probing and port availability checks. Centralized type definitions for service specs, status, and backend interface.
Service Testing
cli/builder/service_command_test.go, cli/builder/service_backend_windows_test.go
Comprehensive test suites validating service lifecycle, flag handling, conflict detection, JSON status output, and Windows command parsing.
CLI Integration
cli/builder/main.go, cli/builder/help.go
Root command dispatch to new serviceSubcommand when first arg is "service". Help text additions for service subcommands and usage examples.
Documentation
docs/src/content/docs/server.md, docs/src/content/docs/service.md, docs/src/content/docs/quickstart.md, docs/src/content/docs/headless.md, docs/astro.config.mjs
New server/service documentation explaining installation, lifecycle management, OS-specific supervisors, and JSON output format. Updated quickstart and headless guides. Sidebar navigation link.
README Updates
README.md
Fixed "Quicktart" → "Quickstart", refined "What will likely never be implemented" section with specific details on MCPs, model switching costs, and sandboxing approaches.
Sandbox & Package Scripts
scripts/sandbox-serve.sh, scripts/sandbox/builder-sandbox-entrypoint.sh, scripts/sandbox/builder-sandbox.Dockerfile, scripts/update-brew-tap.sh, scripts/scripts_test.go
Added host-port availability preflight check in sandbox serve script. Entrypoint runs privileged setup only as root, then re-executes as builder user. Removed USER builder from Dockerfile. Homebrew post-install hook to restart service. Test validating port-in-use detection.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~75 minutes

Possibly related PRs

  • Add headless subagent roles and improve CLI help #147: Modifies CLI help and command routing in cli/builder/help.go and cli/builder/main.go, overlapping with service subcommand dispatch logic.
  • Fix 1.0 regressions #150: Updates CLI command wiring and help system in cli/builder/main.go and cli/builder/help.go, touching the same files and command structure.
  • Phase 2 divergence fixes #137: Modifies the same sandbox scripts (scripts/sandbox-serve.sh, scripts/sandbox/builder-sandbox-entrypoint.sh, scripts/sandbox/builder-sandbox.Dockerfile) that are updated here for service integration.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'feat: add Builder background service' directly describes the main change: adding a new background service feature to Builder, which is the primary purpose of this comprehensive pull request.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/builder-service

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 4e2b7b3152

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread cli/builder/service_backend_windows.go Outdated

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: ca448f6a2a

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread cli/builder/service_backend_windows.go Outdated

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 6

🧹 Nitpick comments (9)
scripts/sandbox-serve.sh (1)

165-172: Optional: probe without sending a stray newline.

echo >/dev/tcp/127.0.0.1/"$host_port" opens the TCP connection and writes a newline byte to whoever owns the port. For most listeners that's harmless, but custom/binary protocols may log it as a malformed request. A read-only open is sufficient to detect listening:

♻️ Suggested refactor
 require_host_port_available() {
 	if [ "$dry_run" = "true" ]; then
 		return 0
 	fi
-	if (echo >/dev/tcp/127.0.0.1/"$host_port") >/dev/null 2>&1; then
+	if (: </dev/tcp/127.0.0.1/"$host_port") >/dev/null 2>&1; then
 		die "host port ${host_port} is already in use. Stop the owner or pass --host-port <free-port>."
 	fi
 }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@scripts/sandbox-serve.sh` around lines 165 - 172, In
require_host_port_available(), the check currently uses "echo
>/dev/tcp/127.0.0.1/$host_port" which opens the TCP socket and writes a newline;
change this to open the socket read-only to avoid sending data: replace the echo
write with a read-only redirection (for example use
"</dev/tcp/127.0.0.1/$host_port" with the same >/dev/null 2>&1 capture) so the
function still detects a listener but does not send a stray byte; update the
conditional in require_host_port_available to use the read-only open and keep
the dry_run and die behavior unchanged.
scripts/scripts_test.go (1)

104-134: Optional: a Go raw string literal makes the fake docker script easier to audit.

The escape-laden single-line bash payload is functionally fine but hard to scan/diff. A backtick raw string literal preserves shell quoting verbatim and matches how bash -c style snippets are typically written elsewhere.

♻️ Suggested refactor
-	fakeDocker := filepath.Join(binDir, "docker")
-	if err := os.WriteFile(fakeDocker, []byte("#!/usr/bin/env bash\nif [ \"${1:-}\" = info ]; then exit 0; fi\nif [ \"${1:-}\" = container ] && [ \"${2:-}\" = inspect ]; then exit 1; fi\necho unexpected docker \"$@\" >&2\nexit 1\n"), 0o755); err != nil {
+	fakeDocker := filepath.Join(binDir, "docker")
+	fakeDockerScript := `#!/usr/bin/env bash
+if [ "${1:-}" = info ]; then exit 0; fi
+if [ "${1:-}" = container ] && [ "${2:-}" = inspect ]; then exit 1; fi
+echo unexpected docker "$@" >&2
+exit 1
+`
+	if err := os.WriteFile(fakeDocker, []byte(fakeDockerScript), 0o755); err != nil {
 		t.Fatalf("write fake docker: %v", err)
 	}

The two golangci-lint hints (errcheck on defer listener.Close() and noctx on net.Listen) are standard test-file false positives and don't need addressing here unless the linter is configured to gate this package.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@scripts/scripts_test.go` around lines 104 - 134, The test
TestSandboxServeUpReportsHostPortInUse uses an escaped single-line shell payload
when writing the fakeDocker script, which is hard to read; replace the current
escaped string in the os.WriteFile call that writes fakeDocker with a Go raw
string literal (backticks) containing the same bash script so the shell quoting
is preserved and the script is easier to audit, leaving the file mode (0o755)
and the surrounding logic (listener, port, cmd setup) unchanged; keep the
function name TestSandboxServeUpReportsHostPortInUse and the fakeDocker variable
so the rest of the test continues to reference the same symbols.
README.md (1)

48-50: Optional: minor wording fixes flagged by LanguageTool.

  • Line 48: consider "On-the-fly changing of toolsets or models" (hyphenate the prenominal compound).
  • Line 50: "java etc" → "java etc." (period after the abbreviation).
📝 Proposed wording tweaks
-- On the fly changing of toolsets or models. Changing models at runtime hurts model performance and invalidates caches, which can cost up to 10x more per invalidation.
+- On-the-fly changing of toolsets or models. Changing models at runtime hurts model performance and invalidates caches, which can cost up to 10x more per invalidation.
@@
-- Sandboxing - Codex's sandbox is annoying, doesn't work with many tools (gradle, java etc), junie's sandbox can be bypassed, claude code's sandbox is brittle and can also be bypassed. Builder can be easily sandboxed in a true, fully isolated Docker container with remote connection (no ssh hacks).
+- Sandboxing - Codex's sandbox is annoying, doesn't work with many tools (gradle, java, etc.), junie's sandbox can be bypassed, claude code's sandbox is brittle and can also be bypassed. Builder can be easily sandboxed in a true, fully isolated Docker container with remote connection (no ssh hacks).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@README.md` around lines 48 - 50, Update the README phrase "On the fly
changing of toolsets or models" to the hyphenated prenominal compound
"On-the-fly changing of toolsets or models" and add a period to the abbreviation
in "java etc" so it reads "java etc."; locate these phrases in the block
containing "On the fly changing of toolsets or models" and "Sandboxing - Codex's
sandbox..." and make the two small wording edits.
cli/builder/service_backend_windows.go (1)

54-59: Use errors.Join so both errors stay unwrappable.

fmt.Errorf("%w; startup fallback failed: %v", err, fallbackErr) only makes the schtasks error unwrappable. errors.Join(err, fmt.Errorf("startup fallback failed: %w", fallbackErr)) preserves both for callers that may want to introspect either failure (e.g., to surface a clearer hint when only the fallback path is broken).

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@cli/builder/service_backend_windows.go` around lines 54 - 59, Replace the
composite error formatting so both errors remain unwrappable: in the block where
runServiceCommand(...) returns an error and you call
installWindowsStartupItem(...), use errors.Join(err, fmt.Errorf("startup
fallback failed: %w", fallbackErr)) instead of fmt.Errorf("%w; startup fallback
failed: %v", err, fallbackErr) and ensure the errors package is imported; this
keeps both the schtasks error and the fallback error available for callers
inspecting runServiceCommand and installWindowsStartupItem.
cli/builder/service_types.go (2)

132-149: Prefer os.Executable() over os.Args[0] for service-registration paths.

os.Args[0] is the string the parent process passed in the argv vector and is not guaranteed to be the path of the running binary (it can be set to anything via exec/syscall.Exec, and isn't resolved against PATH until you do it explicitly here). For something that gets persisted into a launchd plist / systemd unit / scheduled-task script and re-executed at every login, os.Executable() is the more reliable boundary value, matching this file's coding-guideline expectation of validating invariants at boundaries.

♻️ Suggested approach
 func defaultServiceExecutablePath() (string, error) {
-	raw := strings.TrimSpace(os.Args[0])
-	if raw == "" {
-		return "", errors.New("resolve executable path: argv[0] is empty")
-	}
-	if strings.ContainsAny(raw, `/\`) {
-		abs, err := filepath.Abs(raw)
-		if err != nil {
-			return "", fmt.Errorf("resolve executable path: %w", err)
-		}
-		return abs, nil
-	}
-	path, err := exec.LookPath(raw)
-	if err != nil {
-		return "", fmt.Errorf("resolve executable path: %w", err)
-	}
-	return path, nil
+	exe, err := os.Executable()
+	if err != nil {
+		return "", fmt.Errorf("resolve executable path: %w", err)
+	}
+	resolved, err := filepath.EvalSymlinks(exe)
+	if err != nil {
+		return exe, nil
+	}
+	return resolved, nil
 }

As per coding guidelines: "Validate invariants at boundaries (input, filesystem, process execution, API responses) in Go code".

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@cli/builder/service_types.go` around lines 132 - 149, The function
defaultServiceExecutablePath currently uses os.Args[0]; replace that logic to
call os.Executable() to obtain the actual running binary path, check the
returned value for empty and return a wrapped error, then use filepath.Abs (and
optionally filepath.EvalSymlinks) on the executable path and return the resolved
absolute path; update defaultServiceExecutablePath to remove the os.Args[0]
branches and ensure all errors from os.Executable, filepath.EvalSymlinks (if
used), and filepath.Abs are wrapped with the same "resolve executable path: %w"
formatting used elsewhere.

89-104: Prefer errors.As over a raw type assertion for *exec.ExitError.

Idiomatic Go uses errors.As so a wrapped *exec.ExitError (e.g., from a future helper) still unwraps correctly.

♻️ Proposed refactor
-	if exitErr, ok := err.(*exec.ExitError); ok {
+	var exitErr *exec.ExitError
+	if errors.As(err, &exitErr) {
 		result.Stderr = string(exitErr.Stderr)
 		result.Code = exitErr.ExitCode()
 		return result, serviceCommandError{Name: name, Args: args, Result: result}
 	}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@cli/builder/service_types.go` around lines 89 - 104, The runServiceCommand
function currently uses a raw type assertion to detect *exec.ExitError; replace
that with errors.As to correctly unwrap wrapped errors: in runServiceCommand,
import the errors package and use var exitErr *exec.ExitError and if
errors.As(err, &exitErr) { set result.Stderr = string(exitErr.Stderr),
result.Code = exitErr.ExitCode(), and return serviceCommandError{Name: name,
Args: args, Result: result} } while keeping the fallback that sets result.Stderr
= err.Error() and result.Code = 1 and returns serviceCommandError; keep all uses
of serviceCommandResult and serviceCommandError unchanged.
cli/builder/service_command.go (1)

268-297: Replace the "ok" magic string with a protocol constant.

The health status "ok" is hardcoded in server/serve/serve.go:148 and compared directly in cli/builder/service_types.go:204 and service_command.go:274. Since protocol.HealthPath is already used to define the health endpoint path, define a corresponding constant (e.g., protocol.HealthStatusOK) to keep the client-server contract type-safe and prevent drift if the status value ever changes.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@cli/builder/service_command.go` around lines 268 - 297, Replace the hardcoded
"ok" health string with a typed constant: add a constant (e.g., HealthStatusOK)
to the protocol package alongside protocol.HealthPath and use that constant
wherever health responses are compared; specifically update
ensureNoUnmanagedServerConflict to set healthRunning := healthStatus ==
protocol.HealthStatusOK and update any other comparisons in service_types.go
(where "ok" is checked). Ensure the new protocol constant is documented,
exported, and imported where needed so the client/server contract uses
protocol.HealthStatusOK instead of the magic "ok" string.
cli/builder/service_backend_linux.go (2)

24-55: Consider rolling back the written unit file if daemon-reload/enable/restart fails.

If os.WriteFile succeeds but a subsequent systemctl step fails (line 43, 46, or 50), the unit file is left on disk while the install is reported as failed. A subsequent Install without --force will then succeed silently when content matches, or hit the “already installed” path even though no service is actually enabled. Removing the freshly-written file on error (or only writing it after a dry-run/validation) keeps install state consistent.

Optional — happy to defer if the current behavior is intentional given users can always re-run with --force.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@cli/builder/service_backend_linux.go` around lines 24 - 55, The Install
method on systemdServiceBackend currently writes the unit file
(os.WriteFile(renderSystemdUnit)) before running systemctl steps, so if
runServiceCommand("systemctl", "--user", "daemon-reload"/"enable"/"restart")
fails the file is left behind; update Install to rollback the written unit on
error by either (a) deferring deletion of path and removing it if any subsequent
runServiceCommand call returns an error, or (b) move the write to after a
successful daemon-reload/enable/restart validation step; reference the Install
function, the os.WriteFile call that writes renderSystemdUnit(spec), the
systemdUnitPath()/path variable, runServiceCommand invocations for
"daemon-reload"/"enable"/"restart", and the serviceSystemdUnitName so the
cleanup targets the correct unit file when rolling back.

92-95: Restart does not validate that the unit is installed (inconsistent with Start).

Start (lines 73–82) returns a friendly “not installed; run builder service install error when the unit file is missing, but Restart skips the check and lets systemctl restart produce a generic failure instead. For UX consistency, mirror the same precondition.

🔧 Suggested change
 func (systemdServiceBackend) Restart(ctx context.Context, spec serviceSpec) error {
+	path, err := systemdUnitPath()
+	if err != nil {
+		return err
+	}
+	if _, err := os.Stat(path); err != nil {
+		if errors.Is(err, os.ErrNotExist) {
+			return errors.New("Builder background service is not installed; run `builder service install`")
+		}
+		return fmt.Errorf("stat systemd unit: %w", err)
+	}
 	_, err := runServiceCommand(ctx, "systemctl", "--user", "restart", serviceSystemdUnitName)
 	return err
 }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@cli/builder/service_backend_linux.go` around lines 92 - 95, The Restart
method in systemdServiceBackend should perform the same "unit installed"
precondition check as Start to return the friendly "not installed; run `builder
service install`" error instead of letting systemctl produce a generic failure;
update Restart (in systemdServiceBackend) to call the same installation-check
logic used by Start before invoking runServiceCommand("systemctl", "--user",
"restart", serviceSystemdUnitName) and return the same explanatory error when
the unit is missing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@cli/builder/service_backend_linux.go`:
- Around line 73-82: The code calls mustSystemdUnitPath() in Start which hides
UserHomeDir errors and returns "" leading to a misleading "not installed"
message; replace calls to mustSystemdUnitPath() with systemdUnitPath() and
propagate its error (i.e., check err from systemdUnitPath() and return
fmt.Errorf("determine systemd unit path: %w", err) before calling os.Stat or
runServiceCommand), and remove the mustSystemdUnitPath helper if unused; apply
the same change to the other service methods that currently call
mustSystemdUnitPath (e.g., the methods around the Status/Stop/Enable logic) so
all callers properly surface home-dir resolution errors.
- Around line 84-90: Replace the substring error check in
systemdServiceBackend.Stop with a structural pre-check: call systemctl --user
show <serviceSystemdUnitName> --property=LoadState --value (via
runServiceCommand or a small helper) to read the unit's LoadState, and if the
LoadState indicates the unit is not present/not loaded (e.g. "not-found" or any
value other than "loaded") return nil; only invoke runServiceCommand(ctx,
"systemctl", "--user", "stop", serviceSystemdUnitName) when the LoadState shows
the unit is loaded (or otherwise eligible to be stopped), and propagate any
error from that stop invocation. Ensure you add/rename helper(s) if needed so
the logic is in Stop (and reference serviceSystemdUnitName and
runServiceCommand).
- Around line 97-127: The Status method in systemdServiceBackend incorrectly
derives Loaded from the is-active call; change it to query LoadState separately
by calling runServiceCommand(ctx, "systemctl", "--user", "show",
serviceSystemdUnitName, "--property=LoadState", "--value"), parse and trim the
output and set Loaded = (loadState == "loaded") (keep Running derived from
is-active as before), while still using the existing show call for MainPID via
parsePositiveInt; update references in systemdServiceBackend.Status and ensure
any error handling around runServiceCommand for the LoadState call mirrors the
pattern used for other systemctl calls.

In `@cli/builder/service_backend_windows.go`:
- Around line 111-135: The Status method uses a brittle substring check
(strings.Contains on taskOutput) to detect "running" which can produce false
positives; update scheduledTaskServiceBackend.Status to stop using
strings.Contains(..., "running") and instead rely solely on the reliable
PID-based liveness from windowsTaskScriptPIDs (already called) to set Running
and PID (i.e., Running = len(taskScriptPIDs) > 0 and PID = first PID when
present), and remove the taskOutput-based running determination (or if you
prefer to keep schtasks parsing, replace the /FO LIST usage with /FO CSV and
parse the Status column deterministically); change the code in Status around the
variables taskOutput, running, pid and the call sites using
windowsTaskScriptPIDs and windowsScheduledTaskInstalled accordingly.
- Around line 249-263: In windowsTaskScriptPIDs, the PowerShell -like pattern
uses unescaped user-controlled needle from windowsTaskScriptPath causing
failures if the path contains [ or ]; change the generated script (created
before calling runServiceCommand) to perform a literal, case-insensitive
substring match instead of -like — e.g., keep the needle assignment but replace
the Where-Object clause with a containment check using IndexOf/Contains with
[StringComparison]::OrdinalIgnoreCase (or call .ToLower() on both sides) and
ensure you still select ProcessId in the ForEach-Object; this ensures literal
matching even when spec.Config.PersistenceRoot includes brackets.

In `@cli/builder/service_types.go`:
- Around line 213-235: The function probeServiceHealth currently returns the
parsed health.Status and PID even for non-2xx responses; change its behavior so
that after decoding the JSON (the block using json.NewDecoder on resp.Body) you
check resp.StatusCode and if it's not in the 200..299 range return empty string
and 0 (discarding health and PID), otherwise return
strings.TrimSpace(health.Status) and health.PID; update the branches in
probeServiceHealth (around http.NewRequestWithContext, serviceHTTPClient.Do and
the Decode call) so non-2xx responses yield "", 0 to avoid false-positive health
detection used by ensureNoUnmanagedServerConflict and applyHealthProbe.

---

Nitpick comments:
In `@cli/builder/service_backend_linux.go`:
- Around line 24-55: The Install method on systemdServiceBackend currently
writes the unit file (os.WriteFile(renderSystemdUnit)) before running systemctl
steps, so if runServiceCommand("systemctl", "--user",
"daemon-reload"/"enable"/"restart") fails the file is left behind; update
Install to rollback the written unit on error by either (a) deferring deletion
of path and removing it if any subsequent runServiceCommand call returns an
error, or (b) move the write to after a successful daemon-reload/enable/restart
validation step; reference the Install function, the os.WriteFile call that
writes renderSystemdUnit(spec), the systemdUnitPath()/path variable,
runServiceCommand invocations for "daemon-reload"/"enable"/"restart", and the
serviceSystemdUnitName so the cleanup targets the correct unit file when rolling
back.
- Around line 92-95: The Restart method in systemdServiceBackend should perform
the same "unit installed" precondition check as Start to return the friendly
"not installed; run `builder service install`" error instead of letting
systemctl produce a generic failure; update Restart (in systemdServiceBackend)
to call the same installation-check logic used by Start before invoking
runServiceCommand("systemctl", "--user", "restart", serviceSystemdUnitName) and
return the same explanatory error when the unit is missing.

In `@cli/builder/service_backend_windows.go`:
- Around line 54-59: Replace the composite error formatting so both errors
remain unwrappable: in the block where runServiceCommand(...) returns an error
and you call installWindowsStartupItem(...), use errors.Join(err,
fmt.Errorf("startup fallback failed: %w", fallbackErr)) instead of
fmt.Errorf("%w; startup fallback failed: %v", err, fallbackErr) and ensure the
errors package is imported; this keeps both the schtasks error and the fallback
error available for callers inspecting runServiceCommand and
installWindowsStartupItem.

In `@cli/builder/service_command.go`:
- Around line 268-297: Replace the hardcoded "ok" health string with a typed
constant: add a constant (e.g., HealthStatusOK) to the protocol package
alongside protocol.HealthPath and use that constant wherever health responses
are compared; specifically update ensureNoUnmanagedServerConflict to set
healthRunning := healthStatus == protocol.HealthStatusOK and update any other
comparisons in service_types.go (where "ok" is checked). Ensure the new protocol
constant is documented, exported, and imported where needed so the client/server
contract uses protocol.HealthStatusOK instead of the magic "ok" string.

In `@cli/builder/service_types.go`:
- Around line 132-149: The function defaultServiceExecutablePath currently uses
os.Args[0]; replace that logic to call os.Executable() to obtain the actual
running binary path, check the returned value for empty and return a wrapped
error, then use filepath.Abs (and optionally filepath.EvalSymlinks) on the
executable path and return the resolved absolute path; update
defaultServiceExecutablePath to remove the os.Args[0] branches and ensure all
errors from os.Executable, filepath.EvalSymlinks (if used), and filepath.Abs are
wrapped with the same "resolve executable path: %w" formatting used elsewhere.
- Around line 89-104: The runServiceCommand function currently uses a raw type
assertion to detect *exec.ExitError; replace that with errors.As to correctly
unwrap wrapped errors: in runServiceCommand, import the errors package and use
var exitErr *exec.ExitError and if errors.As(err, &exitErr) { set result.Stderr
= string(exitErr.Stderr), result.Code = exitErr.ExitCode(), and return
serviceCommandError{Name: name, Args: args, Result: result} } while keeping the
fallback that sets result.Stderr = err.Error() and result.Code = 1 and returns
serviceCommandError; keep all uses of serviceCommandResult and
serviceCommandError unchanged.

In `@README.md`:
- Around line 48-50: Update the README phrase "On the fly changing of toolsets
or models" to the hyphenated prenominal compound "On-the-fly changing of
toolsets or models" and add a period to the abbreviation in "java etc" so it
reads "java etc."; locate these phrases in the block containing "On the fly
changing of toolsets or models" and "Sandboxing - Codex's sandbox..." and make
the two small wording edits.

In `@scripts/sandbox-serve.sh`:
- Around line 165-172: In require_host_port_available(), the check currently
uses "echo >/dev/tcp/127.0.0.1/$host_port" which opens the TCP socket and writes
a newline; change this to open the socket read-only to avoid sending data:
replace the echo write with a read-only redirection (for example use
"</dev/tcp/127.0.0.1/$host_port" with the same >/dev/null 2>&1 capture) so the
function still detects a listener but does not send a stray byte; update the
conditional in require_host_port_available to use the read-only open and keep
the dry_run and die behavior unchanged.

In `@scripts/scripts_test.go`:
- Around line 104-134: The test TestSandboxServeUpReportsHostPortInUse uses an
escaped single-line shell payload when writing the fakeDocker script, which is
hard to read; replace the current escaped string in the os.WriteFile call that
writes fakeDocker with a Go raw string literal (backticks) containing the same
bash script so the shell quoting is preserved and the script is easier to audit,
leaving the file mode (0o755) and the surrounding logic (listener, port, cmd
setup) unchanged; keep the function name TestSandboxServeUpReportsHostPortInUse
and the fakeDocker variable so the rest of the test continues to reference the
same symbols.
🪄 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: bf854a06-6f83-41f4-9afe-38706dce1533

📥 Commits

Reviewing files that changed from the base of the PR and between 69a024d and ca448f6.

📒 Files selected for processing (21)
  • README.md
  • cli/builder/help.go
  • cli/builder/main.go
  • cli/builder/service_backend_darwin.go
  • cli/builder/service_backend_linux.go
  • cli/builder/service_backend_other.go
  • cli/builder/service_backend_windows.go
  • cli/builder/service_backend_windows_test.go
  • cli/builder/service_command.go
  • cli/builder/service_command_test.go
  • cli/builder/service_types.go
  • docs/astro.config.mjs
  • docs/src/content/docs/headless.md
  • docs/src/content/docs/quickstart.md
  • docs/src/content/docs/server.md
  • docs/src/content/docs/service.md
  • scripts/sandbox-serve.sh
  • scripts/sandbox/builder-sandbox-entrypoint.sh
  • scripts/sandbox/builder-sandbox.Dockerfile
  • scripts/scripts_test.go
  • scripts/update-brew-tap.sh
💤 Files with no reviewable changes (1)
  • scripts/sandbox/builder-sandbox.Dockerfile

Comment thread cli/builder/service_backend_linux.go
Comment thread cli/builder/service_backend_linux.go
Comment thread cli/builder/service_backend_linux.go
Comment thread cli/builder/service_backend_windows.go
Comment thread cli/builder/service_backend_windows.go
Comment thread cli/builder/service_types.go

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 677e9318ca

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread cli/builder/service_command.go
Comment thread cli/builder/service_backend_windows.go

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 041583a1df

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread cli/builder/service_backend_windows.go Outdated
Comment thread cli/builder/service_backend_linux.go
Comment thread cli/builder/service_types.go Outdated

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 62cdeafbf6

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread cli/builder/service_backend_windows.go Outdated
Comment thread cli/builder/service_backend_windows.go Outdated

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 8a350718e6

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread cli/builder/service_backend_linux.go
@Nek-12 Nek-12 merged commit 8550c29 into main Apr 27, 2026
5 checks passed
@Nek-12 Nek-12 deleted the feat/builder-service branch April 27, 2026 16:40
Nek-12 added a commit that referenced this pull request May 1, 2026
feat: add Builder background service
@coderabbitai coderabbitai Bot mentioned this pull request May 5, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant