Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 2 additions & 7 deletions .github/workflows/cli-validate.yml
Original file line number Diff line number Diff line change
Expand Up @@ -29,16 +29,11 @@ jobs:
outputs:
matrix: ${{ steps.generate.outputs.matrix }}
steps:
-
name: Checkout
uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd
-
name: Generate matrix
id: generate
uses: docker/bake-action/subaction/matrix@a66e1c87e2eca0503c343edf1d208c716d54b8a8
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.

any reason why you change this ?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Yes. The reason was the failing Validate model-cli / prepare check.

So , previously before new commit , it showed as gvien below . further , I understood to do changes in CI as per asked by ericcurtin

The failure log showed:

actions/github-script@v8 is not allowed in docker/model-runner because all actions must be pinned to a full-length commit SHA

That check comes from this workflow (Validate model-cli) and specifically the prepare job. The direct docker/bake-action/subaction/matrix reference is pinned, but the subaction path was still causing GitHub to resolve actions/github-script@v8, which is blocked by the repo/org full-SHA action policy.

GitHub documents that repositories/organizations can require actions to be pinned to a full-length commit SHA:
https://docs.github.com/en/repositories/managing-your-repositorys-settings-and-features/enabling-features-for-your-repository/managing-github-actions-settings-for-a-repository

Since cmd/cli/docker-bake.hcl currently has only two validation targets under the validate group (validate-docs and validate-tests), I replaced the dynamic matrix generation with an explicit matrix output while keeping the existing prepare and validate job structure.

If you prefer to keep the bake matrix subaction and handle this CI policy another way, I can revert the workflow change and keep this PR scoped to the install-runner behavior.

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.

I think we should not do this change in the CI. I think what @ericcurtin mentioned was to fix reason if any changes done from this PR breaks the CI.

Copy link
Copy Markdown
Author

@YasharthPanwar-2003 YasharthPanwar-2003 May 26, 2026

Choose a reason for hiding this comment

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

I was thinking same ealier . Thanks for feedback , I will undo it and keep the relevant ones .

Updated based on the review feedback.

I reverted the CI workflow change and kept this PR scoped to the install-runner behavior only.

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.

All these changes to this file should be reverted like @sathiraumesh said... I meant the build was failing in this PR, we cannot merge until it's green

with:
files: ./cmd/cli/docker-bake.hcl
target: validate
run: |
echo 'matrix={"include":[{"target":"validate-docs"},{"target":"validate-tests"}]}' >> "$GITHUB_OUTPUT"

validate:
runs-on: ubuntu-24.04
Expand Down
36 changes: 36 additions & 0 deletions cmd/cli/commands/install-runner.go
Original file line number Diff line number Diff line change
Expand Up @@ -245,6 +245,39 @@ type runnerOptions struct {
tlsKey string
}

func commandFlagChanged(cmd *cobra.Command, name string) bool {
if cmd == nil {
return false
}
flag := cmd.Flags().Lookup(name)
return flag != nil && flag.Changed
}

func existingRunnerOptionsHint(cmd *cobra.Command, opts runnerOptions) string {
backendChanged := commandFlagChanged(cmd, "backend")
gpuChanged := commandFlagChanged(cmd, "gpu")

if !backendChanged && !gpuChanged {
return ""
}

reinstallArgs := []string{"docker", "model", "reinstall-runner"}

if backendChanged && opts.backend != "" {
reinstallArgs = append(reinstallArgs, "--backend", fmt.Sprintf("%q", opts.backend))
}
if gpuChanged && opts.gpuMode != "" {
reinstallArgs = append(reinstallArgs, "--gpu", fmt.Sprintf("%q", opts.gpuMode))
}
Comment on lines +266 to +271
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.

security-critical critical

Critical

  • The suggested command string is constructed using unvalidated and unescaped user input from flags. This poses a security risk (command injection if a user copy-pastes a malicious string) and a correctness issue if values contain spaces. Values should be properly quoted/escaped for the shell.

Following the Security principle (Repository Style Guide, line 12), input must be handled safely when crossing boundaries, including when displayed back to the user as a suggested command.

Suggested change
if backendChanged && opts.backend != "" {
reinstallArgs = append(reinstallArgs, "--backend", opts.backend)
}
if gpuChanged && opts.gpuMode != "" {
reinstallArgs = append(reinstallArgs, "--gpu", opts.gpuMode)
}
if backendChanged && opts.backend != "" {
reinstallArgs = append(reinstallArgs, "--backend", fmt.Sprintf("%q", opts.backend))
}
if gpuChanged && opts.gpuMode != "" {
reinstallArgs = append(reinstallArgs, "--gpu", fmt.Sprintf("%q", opts.gpuMode))
}


return fmt.Sprintf(
"\nThe requested runner options were not applied because the Model Runner container is already running.\n"+
"To recreate the runner with the requested options, run:\n\n"+
" %s\n",
strings.Join(reinstallArgs, " "),
)
}

// runInstallOrStart is shared logic for install-runner and start-runner commands
func runInstallOrStart(cmd *cobra.Command, opts runnerOptions, debug bool) error {
// On macOS ARM64, the vllm backend requires deferred installation
Expand Down Expand Up @@ -343,6 +376,9 @@ func runInstallOrStart(cmd *cobra.Command, opts runnerOptions, debug bool) error
} else {
cmd.Printf("Model Runner container %s is already running\n", ctrID[:12])
}

cmd.Print(existingRunnerOptionsHint(cmd, opts))

return nil
}
}
Expand Down
139 changes: 139 additions & 0 deletions cmd/cli/commands/install-runner_test.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package commands

import (
"strings"
"testing"

"github.com/docker/model-runner/pkg/inference/backends/llamacpp"
Expand Down Expand Up @@ -154,3 +155,141 @@ func TestInstallRunnerValidArgsFunction(t *testing.T) {
t.Error("Expected ValidArgsFunction to be set")
}
}

func TestExistingRunnerOptionsHintNoExplicitOptions(t *testing.T) {
cmd := newInstallRunner()

// Default install-runner options should not print a reinstall hint.
got := existingRunnerOptionsHint(cmd, runnerOptions{
backend: "",
gpuMode: "auto",
})

if got != "" {
t.Fatalf("expected no hint when backend/gpu flags are not explicitly changed, got %q", got)
}
}

func TestExistingRunnerOptionsHintWithBackendOnly(t *testing.T) {
cmd := newInstallRunner()

if err := cmd.Flags().Set("backend", vllm.Name); err != nil {
t.Fatal(err)
}

// A backend-only request should preserve only the explicit backend flag.
got := existingRunnerOptionsHint(cmd, runnerOptions{
backend: vllm.Name,
gpuMode: "auto",
})

if !strings.Contains(got, `docker model reinstall-runner --backend "vllm"`) {
t.Fatalf("expected backend-only reinstall hint, got %q", got)
}
if strings.Contains(got, "--gpu") {
t.Fatalf("did not expect gpu flag in backend-only hint, got %q", got)
}
}

func TestExistingRunnerOptionsHintWithCUDA(t *testing.T) {
cmd := newInstallRunner()

if err := cmd.Flags().Set("gpu", "cuda"); err != nil {
t.Fatal(err)
}

// This fakes a user explicitly requesting CUDA without requiring local GPU hardware.
got := existingRunnerOptionsHint(cmd, runnerOptions{
gpuMode: "cuda",
})

if !strings.Contains(got, `docker model reinstall-runner --gpu "cuda"`) {
t.Fatalf("expected cuda reinstall hint, got %q", got)
}
if strings.Contains(got, "--backend") {
t.Fatalf("did not expect backend flag in cuda-only hint, got %q", got)
}
}

func TestExistingRunnerOptionsHintWithBackendAndCUDA(t *testing.T) {
cmd := newInstallRunner()

if err := cmd.Flags().Set("backend", vllm.Name); err != nil {
t.Fatal(err)
}
if err := cmd.Flags().Set("gpu", "cuda"); err != nil {
t.Fatal(err)
}

// This covers the WSL2/vLLM issue path: the existing runner needs reinstall-runner.
got := existingRunnerOptionsHint(cmd, runnerOptions{
backend: vllm.Name,
gpuMode: "cuda",
})

expectedFragments := []string{
"The requested runner options were not applied",
`docker model reinstall-runner --backend "vllm" --gpu "cuda"`,
}

for _, fragment := range expectedFragments {
if !strings.Contains(got, fragment) {
t.Fatalf("expected hint to contain %q, got %q", fragment, got)
}
}
}

func TestExistingRunnerOptionsHintWithNoGPU(t *testing.T) {
cmd := newInstallRunner()

if err := cmd.Flags().Set("gpu", "none"); err != nil {
t.Fatal(err)
}

// An explicit CPU/no-GPU request should be preserved in the reinstall command.
got := existingRunnerOptionsHint(cmd, runnerOptions{
gpuMode: "none",
})

if !strings.Contains(got, `docker model reinstall-runner --gpu "none"`) {
t.Fatalf("expected no-gpu reinstall hint, got %q", got)
}
if strings.Contains(got, "--backend") {
t.Fatalf("did not expect backend flag in no-gpu hint, got %q", got)
}
}

func TestExistingRunnerOptionsHintQuotesFlagValues(t *testing.T) {
cmd := newInstallRunner()

if err := cmd.Flags().Set("gpu", "cuda; echo bad"); err != nil {
t.Fatal(err)
}

// Suggested command values should be quoted before being shown to the user.
got := existingRunnerOptionsHint(cmd, runnerOptions{
gpuMode: "cuda; echo bad",
})

if !strings.Contains(got, `docker model reinstall-runner --gpu "cuda; echo bad"`) {
t.Fatalf("expected quoted gpu reinstall hint, got %q", got)
}
if strings.Contains(got, "--gpu cuda;") {
t.Fatalf("expected gpu value to be quoted in reinstall hint, got %q", got)
}
}

func TestCommandFlagChangedDefensiveCases(t *testing.T) {
cmd := newInstallRunner()

// Missing commands and flags should be treated as unchanged.
if commandFlagChanged(nil, "gpu") {
t.Fatal("expected nil command to report unchanged flag")
}
if commandFlagChanged(cmd, "missing") {
t.Fatal("expected missing flag to report unchanged")
}
if commandFlagChanged(cmd, "gpu") {
t.Fatal("expected default gpu flag to report unchanged")
}
}
Loading