sandbox: Make updated resource path configurable #154
Merged
Conversation
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
Signed-off-by: Piotr Stankiewicz <[email protected]>
p1-0tr
force-pushed
the
ps-updated-resources-in-sandbox
branch
from
4db1ae5 to
8565b96
Compare
Contributor
Reviewer's guide (collapsed on small PRs)Reviewer's GuideEnable configurable sandbox resource paths by extending Create() to accept an updatedBinPath, replacing hardcoded subpaths with placeholders ([UPDATEDBINPATH] and [UPDATEDLIBPATH]), and propagating the new parameter to tests and the llamaCpp backend. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- Ensure every invocation of Create is updated to supply the new updatedBinPath argument so you don’t introduce build or runtime errors from missing parameters.
- Validate or guard against empty updatedBinPath before deriving UPDATEDLIBPATH, since filepath.Dir("") yields “.” and could result in unintended sandbox paths.
- Consider refactoring the expanded Create signature into a configuration struct or builder pattern to keep the API clear as more path parameters get added.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Ensure every invocation of Create is updated to supply the new updatedBinPath argument so you don’t introduce build or runtime errors from missing parameters.
- Validate or guard against empty updatedBinPath before deriving UPDATEDLIBPATH, since filepath.Dir("") yields “.” and could result in unintended sandbox paths.
- Consider refactoring the expanded Create signature into a configuration struct or builder pattern to keep the API clear as more path parameters get added.
## Individual Comments
### Comment 1
<location> `pkg/sandbox/sandbox_other.go:39` </location>
<code_context>
// function allows for an optional callback (which may be nil) to configure the
// command before it is started.
-func Create(ctx context.Context, configuration string, modifier func(*exec.Cmd), name string, arg ...string) (Sandbox, error) {
+func Create(ctx context.Context, configuration string, modifier func(*exec.Cmd), updatedBinPath, name string, arg ...string) (Sandbox, error) {
// Look up the user's home directory.
currentUser, err := user.Current()
</code_context>
<issue_to_address>
**suggestion:** updatedBinPath parameter is unused in this implementation.
If updatedBinPath is included for cross-platform consistency, please document this in the code. Otherwise, consider removing it to avoid confusion.
```suggestion
/*
updatedBinPath is included for cross-platform consistency with other platform-specific implementations.
It may not be used in this implementation, but is retained to keep the function signature uniform.
*/
func Create(ctx context.Context, configuration string, modifier func(*exec.Cmd), updatedBinPath, name string, arg ...string) (Sandbox, error) {
```
</issue_to_address>
### Comment 2
<location> `pkg/sandbox/sandbox_windows.go:68` </location>
<code_context>
// function allows for an optional callback (which may be nil) to configure the
// command before it is started.
-func Create(ctx context.Context, configuration string, modifier func(*exec.Cmd), name string, arg ...string) (Sandbox, error) {
+func Create(ctx context.Context, configuration string, modifier func(*exec.Cmd), updatedBinPath, name string, arg ...string) (Sandbox, error) {
// Look up the user's home directory.
currentUser, err := user.Current()
</code_context>
<issue_to_address>
**suggestion:** updatedBinPath parameter is unused in this implementation.
If updatedBinPath is included only for interface consistency, please document this or refactor to prevent confusion.
Suggested implementation:
```golang
/*
Create initializes a new Sandbox instance.
configuration: Pre-defined configuration value.
modifier: Optional callback to configure the command before it is started (may be nil).
updatedBinPath: Included for interface consistency with other platforms; currently unused in this implementation.
name: Name of the command to execute.
arg: Arguments to pass to the command.
*/
func Create(ctx context.Context, configuration string, modifier func(*exec.Cmd), updatedBinPath, name string, arg ...string) (Sandbox, error) {
```
```golang
func Create(ctx context.Context, configuration string, modifier func(*exec.Cmd), updatedBinPath, name string, arg ...string) (Sandbox, error) {
// updatedBinPath is intentionally unused in this implementation.
// Parse the configuration and configure limits.
limits := []winjob.Limit{winjob.WithKillOnJobClose()}
tokens := limitTokenMatcher.FindAllString(configuration, -1)
```
</issue_to_address>
### Comment 3
<location> `pkg/sandbox/sandbox_test.go:13` </location>
<code_context>
var err error
if runtime.GOOS == "windows" {
- sandbox, err = Create(t.Context(), ConfigurationLlamaCpp, nil, "go", "version")
+ sandbox, err = Create(t.Context(), ConfigurationLlamaCpp, nil, "/some/path/bin", "go", "version")
} else {
- sandbox, err = Create(t.Context(), ConfigurationLlamaCpp, nil, "date")
</code_context>
<issue_to_address>
**suggestion (testing):** Test does not verify the effect of configurable resource paths.
Please add assertions to confirm that the sandbox configuration uses the provided path and its derived lib path as intended.
Suggested implementation:
```golang
if err != nil {
t.Fatal("unable to create sandboxed process:", err)
}
// Assert that the sandbox uses the provided bin path
if sandbox.BinPath != "/some/path/bin" {
t.Errorf("expected BinPath to be '/some/path/bin', got '%s'", sandbox.BinPath)
}
// Assert that the sandbox uses the derived lib path
expectedLibPath := "/some/path/bin/lib"
if sandbox.LibPath != expectedLibPath {
t.Errorf("expected LibPath to be '%s', got '%s'", expectedLibPath, sandbox.LibPath)
}
```
If the `Sandbox` struct does not have `BinPath` or `LibPath` fields, you will need to adjust the field names to match your implementation. If the derived lib path logic is different, update `expectedLibPath` accordingly.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| // function allows for an optional callback (which may be nil) to configure the | ||
| // command before it is started. | ||
| func Create(ctx context.Context, configuration string, modifier func(*exec.Cmd), name string, arg ...string) (Sandbox, error) { | ||
| func Create(ctx context.Context, configuration string, modifier func(*exec.Cmd), updatedBinPath, name string, arg ...string) (Sandbox, error) { |
Contributor
There was a problem hiding this comment.
suggestion: updatedBinPath parameter is unused in this implementation.
If updatedBinPath is included for cross-platform consistency, please document this in the code. Otherwise, consider removing it to avoid confusion.
Suggested change
| func Create(ctx context.Context, configuration string, modifier func(*exec.Cmd), updatedBinPath, name string, arg ...string) (Sandbox, error) { | |
| /* | |
| updatedBinPath is included for cross-platform consistency with other platform-specific implementations. | |
| It may not be used in this implementation, but is retained to keep the function signature uniform. | |
| */ | |
| func Create(ctx context.Context, configuration string, modifier func(*exec.Cmd), updatedBinPath, name string, arg ...string) (Sandbox, error) { |
| // function allows for an optional callback (which may be nil) to configure the | ||
| // command before it is started. | ||
| func Create(ctx context.Context, configuration string, modifier func(*exec.Cmd), name string, arg ...string) (Sandbox, error) { | ||
| func Create(ctx context.Context, configuration string, modifier func(*exec.Cmd), updatedBinPath, name string, arg ...string) (Sandbox, error) { |
Contributor
There was a problem hiding this comment.
suggestion: updatedBinPath parameter is unused in this implementation.
If updatedBinPath is included only for interface consistency, please document this or refactor to prevent confusion.
Suggested implementation:
/*
Create initializes a new Sandbox instance.
configuration: Pre-defined configuration value.
modifier: Optional callback to configure the command before it is started (may be nil).
updatedBinPath: Included for interface consistency with other platforms; currently unused in this implementation.
name: Name of the command to execute.
arg: Arguments to pass to the command.
*/
func Create(ctx context.Context, configuration string, modifier func(*exec.Cmd), updatedBinPath, name string, arg ...string) (Sandbox, error) {func Create(ctx context.Context, configuration string, modifier func(*exec.Cmd), updatedBinPath, name string, arg ...string) (Sandbox, error) {
// updatedBinPath is intentionally unused in this implementation.
// Parse the configuration and configure limits.
limits := []winjob.Limit{winjob.WithKillOnJobClose()}
tokens := limitTokenMatcher.FindAllString(configuration, -1)| var err error | ||
| if runtime.GOOS == "windows" { | ||
| sandbox, err = Create(t.Context(), ConfigurationLlamaCpp, nil, "go", "version") | ||
| sandbox, err = Create(t.Context(), ConfigurationLlamaCpp, nil, "/some/path/bin", "go", "version") |
Contributor
There was a problem hiding this comment.
suggestion (testing): Test does not verify the effect of configurable resource paths.
Please add assertions to confirm that the sandbox configuration uses the provided path and its derived lib path as intended.
Suggested implementation:
if err != nil {
t.Fatal("unable to create sandboxed process:", err)
}
// Assert that the sandbox uses the provided bin path
if sandbox.BinPath != "/some/path/bin" {
t.Errorf("expected BinPath to be '/some/path/bin', got '%s'", sandbox.BinPath)
}
// Assert that the sandbox uses the derived lib path
expectedLibPath := "/some/path/bin/lib"
if sandbox.LibPath != expectedLibPath {
t.Errorf("expected LibPath to be '%s', got '%s'", expectedLibPath, sandbox.LibPath)
}If the Sandbox struct does not have BinPath or LibPath fields, you will need to adjust the field names to match your implementation. If the derived lib path logic is different, update expectedLibPath accordingly.
ilopezluna
approved these changes
Sep 23, 2025
doringeman
approved these changes
Sep 23, 2025
doringeman
pushed a commit
to doringeman/model-runner
that referenced
this pull request
Oct 2, 2025
…ounting-support Add AI accelerator device mounting support in CreateControllerContainer
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Not decided yet if this is the best way to handle the issue. But this fixes sandboxing for local (dev) builds of DMR.
Summary by Sourcery
Introduce a configurable resource path for sandboxed processes by adding an updatedBinPath parameter to sandbox creation across all platforms, replacing hard-coded Docker inference paths with [UPDATEDBINPATH] and [UPDATEDLIBPATH] placeholders, and propagating the new path into the llamacpp backend run command.
New Features:
Tests: