Skip to content

Add an optional CommandModifier for backend runner processes#1003

Merged
doringeman merged 2 commits into
docker:mainfrom
mat007:command-modifier
Jul 9, 2026
Merged

Add an optional CommandModifier for backend runner processes#1003
doringeman merged 2 commits into
docker:mainfrom
mat007:command-modifier

Conversation

@mat007

@mat007 mat007 commented Jul 8, 2026

Copy link
Copy Markdown
Member

What this PR does

Adds an optional hook to customize each backend runner process before it starts.

Notes for the reviewer

Embedders sometimes need to touch the spawned runner before it starts, e.g. launch it under different credentials, adjust its environment, or apply process controls. Today only the internal modifier inside RunBackend can do that, and it isn't reachable from outside the module.

This threads an optional CommandModifier func(*exec.Cmd) from BackendsConfig down through each backend's RunnerConfig to RunBackend, where it runs on the command after the internal setup (cancellation, stdio, env) and just before the process starts. It composes with the internal modifier rather than replacing it, uses no global, and doesn't change sandbox.Create's signature. No behavior change when it's nil.

Our use case is Docker Desktop's secure mode on Windows: the backend runs as a service account and needs to launch the runner as the interactive user (attach the user's token via SysProcAttr). But the hook is generic, so any embedder can use it.

I put the seam on BackendsConfig since that's where the backends are constructed. Would you prefer it on ServiceConfig instead? And happy to add a test if you can point me at the right spot.

Thread an optional func(*exec.Cmd) from BackendsConfig down through each
backend's RunnerConfig to RunBackend, where it runs on the runner command
after the internal setup (cancellation, stdio, env) and just before the
process starts. Lets an embedder customize the spawned process, for
example its credentials, environment or working directory, without a
global or a change to sandbox.Create. No behavior change when unset.

@gemini-code-assist gemini-code-assist 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.

Code Review

This pull request introduces a CommandModifier hook across various inference backends (including diffusers, llamacpp, mlx, sglang, and vllm) to allow callers to customize backend runner processes before they start. Feedback is provided regarding a critical issue in pkg/inference/backends/runner.go where command.Env remains nil if config.Env is empty; if the CommandModifier attempts to append to this nil slice, it will silently wipe out the inherited parent environment. To prevent this, command.Env should be initialized with os.Environ() before invoking the modifier.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread pkg/inference/backends/runner.go
A nil Env means inherit the parent, but a modifier that appends to it
would start from an empty slice and drop the parent environment
(PATH, SYSTEMROOT and such). Set it to os.Environ() first when nil.
@doringeman doringeman marked this pull request as ready for review July 9, 2026 07:06
@doringeman doringeman merged commit 362c7f6 into docker:main Jul 9, 2026
8 checks passed

@sourcery-ai sourcery-ai 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.

Hey - I've found 1 issue, and left some high level feedback:

  • Now that CommandModifier is threaded through multiple packages, consider introducing a shared named type (e.g. type CommandModifier func(*exec.Cmd)) in a common package to avoid repeating the raw function signature and reduce the chance of divergence across backends.
  • In server.Run, the direct sglang.New call is hard-coded with a nil CommandModifier, unlike the backends wired via DefaultBackendDefs; if the hook is intended to be globally configurable, you may want to thread the same modifier into this manually registered backend for consistency.
Prompt for AI Agents
Please address the comments from this code review:

## Overall Comments
- Now that CommandModifier is threaded through multiple packages, consider introducing a shared named type (e.g. `type CommandModifier func(*exec.Cmd)`) in a common package to avoid repeating the raw function signature and reduce the chance of divergence across backends.
- In `server.Run`, the direct `sglang.New` call is hard-coded with a nil CommandModifier, unlike the backends wired via `DefaultBackendDefs`; if the hook is intended to be globally configurable, you may want to thread the same modifier into this manually registered backend for consistency.

## Individual Comments

### Comment 1
<location path="pkg/inference/backends/runner.go" line_range="50-58" />
<code_context>
 	// process environment. If nil or empty, the backend inherits the parent
 	// env as-is (an empty non-nil slice is treated the same as nil).
 	Env []string
+	// CommandModifier, if non-nil, is invoked on the backend command immediately
+	// before it is started, after the internal setup (cancellation, stdio, env).
+	// Embedders use it to customize process attributes such as credentials
</code_context>
<issue_to_address>
**suggestion:** Clarify and possibly adjust environment handling semantics before CommandModifier runs.

With the new logic, both nil and empty Env are materialized to os.Environ() before CommandModifier runs. This matches the Env comment but prevents CommandModifier from distinguishing between "inherit parent" vs. "start from empty and selectively add vars." Embedders who want a minimal env must now overwrite command.Env inside CommandModifier. Please either document this explicitly in RunnerConfig.CommandModifier or change the flow so CommandModifier sees the raw Env configuration and can decide how to materialize or replace it.

Suggested implementation:

```golang
	// process environment. If nil or empty, the backend inherits the parent
	// env as-is (an empty non-nil slice is treated the same as nil).
	//
	// When Runner constructs the *exec.Cmd, it materializes Env into cmd.Env
	// based on this policy before CommandModifier runs; embedders will not
	// be able to distinguish "inherit parent" from "start from empty and
	// selectively add vars" via Env alone.
	Env []string
	// CommandModifier, if non-nil, is invoked on the backend command immediately
	// before it is started, after the internal setup (cancellation, stdio, env).
	// Embedders use it to customize process attributes such as credentials
	// (SysProcAttr), environment, or working directory. It composes with, and
	// runs after, the internal modifier.
	//
	// Note: by the time CommandModifier runs, cmd.Env has already been
	// materialized from RunnerConfig.Env according to the inheritance policy
	// above. Embedders that require a minimal or custom environment should
	// overwrite cmd.Env inside CommandModifier rather than relying on a
	// nil or empty RunnerConfig.Env.
	CommandModifier func(*exec.Cmd)
}

```

```golang
			if len(config.Env) > 0 {
				command.Env = append(os.Environ(), config.Env...)
			}
			// At this point, command.Env reflects the materialized environment
			// based on RunnerConfig.Env (inherit + overlay). CommandModifier
			// may further adjust or replace command.Env if a different policy
			// is desired by the embedder.
			if config.CommandModifier != nil {

```

If there are other constructors or helpers that build *exec.Cmd using RunnerConfig.Env and CommandModifier, mirror the same documentation and behavior there so embedders consistently see a materialized environment before CommandModifier runs.
</issue_to_address>

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

Comment on lines +50 to 58
// CommandModifier, if non-nil, is invoked on the backend command immediately
// before it is started, after the internal setup (cancellation, stdio, env).
// Embedders use it to customize process attributes such as credentials
// (SysProcAttr), environment, or working directory. It composes with, and
// runs after, the internal modifier.
CommandModifier func(*exec.Cmd)
}

// Logger interface for backend logging

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.

suggestion: Clarify and possibly adjust environment handling semantics before CommandModifier runs.

With the new logic, both nil and empty Env are materialized to os.Environ() before CommandModifier runs. This matches the Env comment but prevents CommandModifier from distinguishing between "inherit parent" vs. "start from empty and selectively add vars." Embedders who want a minimal env must now overwrite command.Env inside CommandModifier. Please either document this explicitly in RunnerConfig.CommandModifier or change the flow so CommandModifier sees the raw Env configuration and can decide how to materialize or replace it.

Suggested implementation:

	// process environment. If nil or empty, the backend inherits the parent
	// env as-is (an empty non-nil slice is treated the same as nil).
	//
	// When Runner constructs the *exec.Cmd, it materializes Env into cmd.Env
	// based on this policy before CommandModifier runs; embedders will not
	// be able to distinguish "inherit parent" from "start from empty and
	// selectively add vars" via Env alone.
	Env []string
	// CommandModifier, if non-nil, is invoked on the backend command immediately
	// before it is started, after the internal setup (cancellation, stdio, env).
	// Embedders use it to customize process attributes such as credentials
	// (SysProcAttr), environment, or working directory. It composes with, and
	// runs after, the internal modifier.
	//
	// Note: by the time CommandModifier runs, cmd.Env has already been
	// materialized from RunnerConfig.Env according to the inheritance policy
	// above. Embedders that require a minimal or custom environment should
	// overwrite cmd.Env inside CommandModifier rather than relying on a
	// nil or empty RunnerConfig.Env.
	CommandModifier func(*exec.Cmd)
}
			if len(config.Env) > 0 {
				command.Env = append(os.Environ(), config.Env...)
			}
			// At this point, command.Env reflects the materialized environment
			// based on RunnerConfig.Env (inherit + overlay). CommandModifier
			// may further adjust or replace command.Env if a different policy
			// is desired by the embedder.
			if config.CommandModifier != nil {

If there are other constructors or helpers that build *exec.Cmd using RunnerConfig.Env and CommandModifier, mirror the same documentation and behavior there so embedders consistently see a materialized environment before CommandModifier runs.

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.

2 participants