Skip to content

fix(services): re-invoke devbox by executable path, not hardcoded name (#1321) - #2936

Merged
mikeland73 merged 2 commits into
mainfrom
claude/focused-goldberg-v0mi31
Sep 15, 2026
Merged

mikeland73 merged 2 commits into
mainfrom
claude/focused-goldberg-v0mi31

Conversation

@mikeland73

Copy link
Copy Markdown
Collaborator

Summary

Fixes #1321.

devbox services start|stop|ls|restart (and the non-current-shell path of devbox services up) work by re-invoking devbox inside the computed environment. runDevboxServicesScript builds a command and hands it to RunScript, which writes a small script that evals the command. The command began with the hard-coded string "devbox":

// internal/devbox/services.go
func (d *Devbox) runDevboxServicesScript(ctx context.Context, cmdArgs []string) error {
	cmdArgs = append([]string{"services"}, cmdArgs...)
	return d.RunScript(ctx, devopt.EnvOptions{}, "devbox", cmdArgs)
}

So the generated script effectively runs eval "devbox services ...", which requires the binary to be named exactly devbox and be resolvable on PATH. If the binary is installed or renamed to anything else (a versioned install, a wrapper, devbox-cli, etc.), services commands fail with devbox: command not found.

This was the only real self-invocation of the binary — the process-compose manager already shells out via processComposeConfig.BinPath, and every other devbox ... occurrence in the services code is user-facing help text.

Fix

Reference the currently running binary via os.Executable() instead of the literal "devbox". The path is shell-quoted (via strconv.Quote, matching how the surrounding code already quotes arguments before they are eval'd) so it survives the eval even if it contains spaces. If the executable path can't be determined, it falls back to "devbox" — the previous PATH-lookup behavior.

func devboxBinaryForSelfInvocation() string {
	exe, err := os.Executable()
	if err != nil {
		return "devbox"
	}
	return strconv.Quote(exe)
}

This mirrors the existing use of os.Executable() in internal/devbox/pure_shell.go.

How was it tested?

  • go build ./... — clean.
  • go vet ./internal/devbox/ — clean.
  • gofmt — no diffs.
  • Added TestDevboxBinaryForSelfInvocation in internal/devbox/services_test.go, which asserts the helper returns the shell-quoted path to the running binary rather than the literal "devbox". It passes:
    --- PASS: TestDevboxBinaryForSelfInvocation (0.00s)
    

cc @mikeland73 (issue reporter)

Community Contribution License

All community contributions in this pull request are licensed to the project
maintainers under the terms of the
Apache 2 License.

By creating this pull request, I represent that I have the right to license the
contributions to the project maintainers under the Apache 2 License as stated in
the
Community Contribution License.

🤖 Generated with Claude Code

https://claude.ai/code/session_01BQ4KvbTQb7oJ2M3tc79DuG


Generated by Claude Code

#1321)

`devbox services start|stop|ls|restart|up` re-invoke devbox inside the
computed environment via a generated run script that eval's a command
beginning with the literal string "devbox". This required the binary to
be named exactly "devbox" and resolvable on PATH; any other name (e.g. a
renamed or versioned install) broke services commands with
"devbox: command not found".

Use os.Executable() to reference the currently running binary (shell-
quoted so paths with spaces survive the eval), falling back to "devbox"
only if the executable path can't be determined.

Fixes #1321.
@savil
savil marked this pull request as draft September 2, 2026 22:13
@mikeland73

Copy link
Copy Markdown
Collaborator Author

@claude please review this PR for correctness, and flag anything that would block merging.

@claude

claude Bot commented Sep 14, 2026

Copy link
Copy Markdown

Claude finished @mikeland73's task in 2m 29s —— View job


Review: fix(services): re-invoke devbox by executable path, not hardcoded name

  • Gather context (diff, related files)
  • Understand the request (correctness review)
  • Review internal/devbox/services.go changes
  • Review internal/devbox/services_test.go
  • Check build/vet/test results
  • Post final review

No blocking issues found. This is a small, correct fix.

What I checked

  • runDevboxServicesScript / devboxBinaryForSelfInvocation (internal/devbox/services.go:271-293): traced the new cmdName value through Devbox.RunScript (internal/devbox/devbox.go:263-341) into nix.RunScript (internal/nix/run.go). Since the quoted path (e.g. "/nix/store/.../devbox") won't match any entry in d.cfg.Scripts() or d.cfg.Aliases(), execution correctly falls into the "arbitrary command" branch, exactly as the old literal "devbox" did — no behavior change there, just resolved-by-path instead of resolved-by-PATH-lookup.
  • Quoting: strconv.Quote(exe) is applied once in devboxBinaryForSelfInvocation, and the result is placed directly into DEVBOX_RUN_CMD without re-quoting, matching how cmdArgs are already quoted before being eval'd in the same function. Consistent with the existing (if imperfect — Go string-escaping isn't 1:1 with POSIX shell escaping) convention already used for cmdArgs, so this isn't a new class of bug.
  • Fallback path: if os.Executable() errors, it falls back to unquoted "devbox", which is exactly the pre-PR behavior (relies on PATH). Good, preserves back-compat if the executable path can't be resolved.
  • Precedent: this mirrors the existing os.Executable() usage in internal/devbox/pure_shell.go:18 (createDevboxSymlink), which only runs for devbox shell, not for the RunScript path used by devbox services ... — confirming this bug wasn't already mitigated elsewhere for the services commands.
  • Test (services_test.go): straightforward and correctly targeted — asserts the helper returns the quoted real executable path and not the literal "devbox". testify is already a project dependency, so no new dependency introduced. The untestable error branch (os.Executable() failing) isn't covered, but that's a minor, non-blocking gap given how rarely that fails in practice.
  • Scanned for other hardcoded self-invocations of "devbox" that might have the same bug; found one unrelated one in internal/vercheck/vercheck.go:178 (auto-update launcher), but it already has its own LAUNCHER_PATH env-based mechanism and a user-facing warning on fallback — out of scope for this PR/issue [Bug][minor]: services commands rely on the binary being called "devbox". Any other name will break. #1321, just noting it as an FYI, not a blocker.

Note on verification

I wasn't able to execute go build/go vet/go test/gofmt myself in this environment (Bash tool calls required approval that wasn't available here), so I relied on careful static tracing of the code paths above instead of reproducing the PR description's reported clean results. The logic checks out from reading the code.
· branch claude/focused-goldberg-v0mi31

Use shellescape.Quote (already used in shell.go) instead of strconv.Quote
for the devbox executable path. The path is eval'd by a POSIX sh run
script, and strconv.Quote emits Go-style \uXXXX escapes that the shell
does not interpret inside double quotes, so a binary living under a
non-ASCII path would break. shellescape.Quote produces POSIX single-
quoted output that round-trips correctly for spaces, unicode, and shell
metacharacters.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BQ4KvbTQb7oJ2M3tc79DuG

Copy link
Copy Markdown
Collaborator Author

Reviewed for correctness. Summary: I believe this is correct and I don't see anything that blocks merging. Details below, including one behavioral note worth a maintainer's eye and one hardening change I just pushed.

Correctness

  • Coverage is complete. Every non---run-in-current-shell service subcommand — start, stop, ls, restart, and up (via StartProcessManager) — routes through runDevboxServicesScript, which is the single place devbox re-invokes itself. That was the only real self-invocation: the process-compose manager already execs via processComposeConfig.BinPath, and every other devbox … string in the services code is user-facing help text.
  • The generated Dockerfile/devcontainer templates were intentionally left alone. Those ["devbox", …] entries run inside a container where devbox on PATH is the right reference, not the host's os.Executable().
  • os.Executable() is an established pattern here (already used in pure_shell.go, telemetry, nixcache/setup, setup).

Behavioral note (not a blocker)

Re-invoking os.Executable() runs the currently-executing binary directly, bypassing a PATH-based devbox launcher wrapper if one is installed. For a self-invocation this is the desired behavior — it guarantees the same version that's already running (no redundant launcher re-resolution), and RunScript passes the inherited environment (launcher env vars included) through to the nested call. Since the launcher has already selected the correct (possibly project-pinned) binary before we reach this code, there's no version regression. Flagging it only so it's a conscious choice.

Hardening pushed (eeed220)

My first commit quoted the path with strconv.Quote. Since the path is eval'd by a POSIX sh run script, strconv.Quote's Go-style \uXXXX escapes wouldn't be interpreted by the shell inside double quotes — so a devbox binary under a non-ASCII path would have broken. Switched to shellescape.Quote (already used in shell.go), which emits POSIX single-quoted output that round-trips correctly for spaces, unicode, and shell metacharacters. Test updated accordingly.

Verification

go build ./..., go vet ./internal/devbox/, and gofmt are clean; TestDevboxBinaryForSelfInvocation passes.

One pre-existing item I did not touch (out of scope): the cmdArgs in RunScript are still strconv.Quoted and share the same theoretical unicode limitation, but those are service names/flags rather than filesystem paths.

Note: the PR is currently in draft (converted by @savil). I haven't flipped it back to ready — leaving that to you.


Generated by Claude Code

@mikeland73 mikeland73 added the ready for human review Automated review passed; needs a maintainer to review and merge label Sep 14, 2026
@mikeland73
mikeland73 marked this pull request as ready for review September 14, 2026 16:19
@mikeland73
mikeland73 merged commit 94498d6 into main Sep 15, 2026
29 checks passed
@mikeland73
mikeland73 deleted the claude/focused-goldberg-v0mi31 branch September 15, 2026 16:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready for human review Automated review passed; needs a maintainer to review and merge

Development

Successfully merging this pull request may close these issues.

[Bug][minor]: services commands rely on the binary being called "devbox". Any other name will break.

2 participants