fix(inference): run managed vLLM Docker with argv

[apurvvkumaria]

Summary

Replace shell-composed managed vLLM Docker startup with validated argument vectors so profile flags and sensitive environment forwarding reach Docker without shell interpolation.
This supersedes #5475 with a current-main, DCO-compliant history. Original implementation credit: Minh Vu (fallintoplace).

Changes

• Build the long-lived vLLM container invocation as validated Docker argv and reject empty or NUL-bearing tokens.
• Use typed Docker lifecycle adapters for container removal, detached startup, and shutdown instead of composing shell commands.
• Keep Hugging Face token values in the subprocess environment while forwarding only the selected variable name to Docker.
• Normalize Hugging Face cache mounts and multi-GPU flags as individual argv tokens.
• Add adversarial and lifecycle coverage for metacharacter handling, token secrecy, profile flags, and managed startup.

Type of Change

• Code change (feature, bug fix, or refactor)
• Code change with doc updates
• Doc only (prose changes, no code sample modifications)
• Doc only (includes code sample changes)

Quality Gates

• Tests added or updated for changed behavior
• Existing tests cover changed behavior — justification:
• Tests not applicable — justification:
• Docs updated for user-facing behavior changes
• Docs not applicable — justification: Independent documentation review found no command, configuration, default, output contract, or supported workflow change; this hardens the existing managed vLLM launch path.
• Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging)
• Sensitive-path review completed or maintainer-approved waiver recorded — reviewer/approval link/justification: Independent nine-category security review returned PASS with no blocking findings.
• Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue:

Verification

• PR description includes the DCO sign-off declaration and every commit appears as Verified in GitHub
• Normal pre-commit, commit-msg, and pre-push hooks passed, or npm run check:diff passed when hooks were skipped or unavailable — diff-scoped prek validation and the normal pre-push CLI type-check passed on the current commit.
• Targeted behavior tests pass for the current change set, or tests are marked not applicable above — command/result or justification: 56 focused CLI tests and 19 profile integration tests passed with one worker (75/75).
• Applicable broad gate passed — npm test for broad runtime/test-harness changes; npm run check for repo-wide validation/coverage changes — command/result: Broad gates were not run; the CLI build and type-check passed.
• Quality Gates section completed with required justifications or waivers
• No secrets, API keys, or credentials committed
• npm run docs builds without warnings (doc changes only)
• Doc pages follow the style guide (doc changes only)
• New doc pages include SPDX header and frontmatter (new pages only)

---

Signed-off-by: Apurv Kumaria akumaria@nvidia.com

Summary by CodeRabbit

• New Features

  • Improved vLLM container startup using structured Docker argv calls, including clearer Hugging Face cache mounting and GPU-related launch flags.
  • Updated token handling so inference containers receive token keys while token values are supplied via the runner environment.

• Bug Fixes

  • Eliminated shell-assembled container run strings to reliably handle special characters and invalid inputs (e.g., empty/NUL tokens).
  • Improved idempotent cleanup and container stop behavior on startup/readiness failures.

• Tests

  • Expanded coverage for vLLM install and run argument validation, including HF token forwarding and rejection of invalid Docker run flags.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: cb980920-0db0-413d-9edc-ac4d8a6a04a5

📥 Commits

Reviewing files that changed from the base of the PR and between 4fc2c9b and 3eb6b4c.

📒 Files selected for processing (2)

• src/lib/inference/vllm.test.ts
• src/lib/inference/vllm.ts

🚧 Files skipped from review as they are similar to previous changes (2)

• src/lib/inference/vllm.test.ts
• src/lib/inference/vllm.ts

---

📝 Walkthrough

Walkthrough

This PR refactors vLLM container startup in src/lib/inference/vllm.ts from shell-string Docker commands to argv-array invocations using Docker adapter functions. It adds shared cache-mount and run-flag helpers, replaces buildVllmRunCommand with buildVllmRunArgs, and updates the associated tests.

Changes

vLLM Docker argv refactor

Layer / File(s)	Summary
Shared cache mount and docker run flag helpers src/lib/inference/vllm.ts	Adds centralized Hugging Face cache helpers and vllmDockerRunFlags(); wires them into SPARK profile flags, DGX Station GPU flag builder, and hf download container invocation.
buildVllmRunArgs argv builder and validation src/lib/inference/vllm.ts	Replaces buildVllmRunCommand with validation helpers plus exported buildVllmRunArgs, which validates inputs and assembles the docker run argv list including restart policy, port mapping, entrypoint, and HF token args; updates related documentation.
Container startup wiring via docker adapters src/lib/inference/vllm.ts	Updates startContainer to remove containers via dockerForceRm, build argv via buildVllmRunArgs, launch via dockerRunDetached with forwarded env, and use dockerStop on readiness failure instead of shelling out.
Updated and new tests for argv-based launch src/lib/inference/vllm.test.ts	Updates mocks and adds tests for argv tokenization, validation, cache mount behavior, GPU flag formatting, and dockerRunDetached-based container startup with HF_TOKEN forwarding.

Estimated code review effort: 3 (Moderate) | ~30 minutes

Possibly related PRs

• NVIDIA/NemoClaw#1886: Shifts docker/curl/openshell execution to argv-array based invocation, aligning with this PR's move from shell strings to buildVllmRunArgs/dockerRunDetached.
• NVIDIA/NemoClaw#5038: Both touch installVllm's model-resolution/install flow, with this PR updating the container startup step within that same flow.

Suggested labels: security

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately summarizes the main change: managed vLLM Docker startup now uses argv construction instead of shell-composed commands.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
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.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch codex/pr5475-vllm-docker-argv

---

_{Comment @coderabbitai help to get the list of available commands.}

[github-code-quality]

Code Coverage Overview

Languages: TypeScript

TypeScript / code-coverage/plugin

The overall coverage remains at 96%, unchanged from the main branch.

TypeScript / code-coverage/cli

The overall coverage in the codex/pr5475-vllm-do... branch remains at 76%, unchanged from the main branch.

Show a code coverage summary of the most impacted files.

File	main 94af5e2	codex/pr5475-vllm-do... 3eb6b4c	+/-
src/lib/actions...e-validation.ts	90%	81%	-9%
src/lib/actions...x/mcp-bridge.ts	44%	35%	-9%
src/lib/actions...lution-probe.ts	94%	88%	-6%
src/lib/messagi.../persistence.ts	95%	92%	-3%
src/lib/credentials/store.ts	61%	59%	-2%
src/lib/actions...ridge-policy.ts	64%	62%	-2%
src/lib/actions...-add-restart.ts	14%	19%	+5%
src/lib/runner.ts	73%	80%	+7%
src/lib/sandbox...rsion-scheme.ts	73%	100%	+27%
src/lib/inference/vllm.ts	41%	76%	+35%

---

_{Updated July 09, 2026 05:27 UTC
Code Coverage is in Public Preview. Learn more and provide us with your feedback.}

[github-actions]

PR Review Advisor (Nemotron Ultra) — Changes requested

Merge posture: Do not merge yet
Primary next action: Fix PRA-1: Monolith growth: vllm.ts grew 620→669 lines (+49); then add or justify PRA-T1.
Open items: 3 required · 1 warning · 0 suggestions · 4 test follow-ups
Since last review: 0 prior items resolved · 0 still apply · 3 new items found

Action checklist

• PRA-1 Fix: Monolith growth: vllm.ts grew 620→669 lines (+49) in src/lib/inference/vllm.ts:1
• PRA-2 Fix: Monolith growth: vllm.test.ts grew 325→467 lines (+142) in src/lib/inference/vllm.test.ts:1
• PRA-3 Fix: E2E validation floor required: inference-routing and network-policy jobs must pass
• PRA-4 Resolve or justify: Missing regression test for waitForVllmReady failure path calling dockerStop in src/lib/inference/vllm.ts:574
• PRA-T1 Add or justify test follow-up: Runtime validation
• PRA-T2 Add or justify test follow-up: Runtime validation
• PRA-T3 Add or justify test follow-up: E2E validation floor required: inference-routing and network-policy jobs must pass
• PRA-T4 Add or justify test follow-up: Missing regression test for waitForVllmReady failure path calling dockerStop

Findings index

ID	Severity	Category	Location	Required action
PRA-1	Required	architecture	src/lib/inference/vllm.ts:1	Accept growth for this PR (security-context coupling justifies it). File follow-up to extract: profile table, docker argv builder, install orchestrator, validation helpers into separate modules.
PRA-2	Required	architecture	src/lib/inference/vllm.test.ts:1	Accept for this PR (new coverage is thorough and security-relevant). Follow-up: split into vllm-argv.test.ts, vllm-install.test.ts, vllm-profile.test.ts
PRA-3	Required	tests	—	Do not merge until inference-routing and network-policy CI jobs pass on this head SHA. These are required automatic jobs per riskPlan.
PRA-4	Resolve/justify	tests	src/lib/inference/vllm.ts:574	Add test: 'waitForVllmReady failure calls dockerStop and prints log tail' in vLLM run command suite, or justify omission given dockerStop is thin adapter wrapper

🚨 Required before merge

Address these before merging unless a maintainer explicitly overrides the advisor with rationale.

PRA-1 Required — Monolith growth: vllm.ts grew 620→669 lines (+49)

• Location: src/lib/inference/vllm.ts:1
• Category: architecture
• Problem: File exceeds 20-line growth threshold. New code includes argv builder, validation helpers, HF cache mount helpers, docker flag consolidation — all tightly coupled to host→sandbox trust boundary.
• Impact: Increased cognitive load, harder to audit security boundary, future changes risk regressing argv/token/GPU logic
• Required action: Accept growth for this PR (security-context coupling justifies it). File follow-up to extract: profile table, docker argv builder, install orchestrator, validation helpers into separate modules.
• Expected follow-up: Fix before merge or get explicit maintainer override.
• Verification: wc -l src/lib/inference/vllm.ts shows 669 lines
• Missing regression test: N/A — architecture finding, not a behavior gap
• Done when: The required change is committed and verification passes: wc -l src/lib/inference/vllm.ts shows 669 lines.
• Evidence: diff shows +49 net lines; riskPlan monolithDeltas severity=blocker for this file

PRA-2 Required — Monolith growth: vllm.test.ts grew 325→467 lines (+142)

• Location: src/lib/inference/vllm.test.ts:1
• Category: architecture
• Problem: Test file exceeds 20-line growth threshold. Comprehensive new coverage for argv validation, shell metacharacter handling, GPU CSV quoting, install flow with dockerRunDetached, invalid flag rejection.
• Impact: Test file becoming hard to navigate; related tests scattered across suites
• Required action: Accept for this PR (new coverage is thorough and security-relevant). Follow-up: split into vllm-argv.test.ts, vllm-install.test.ts, vllm-profile.test.ts
• Expected follow-up: Fix before merge or get explicit maintainer override.
• Verification: wc -l src/lib/inference/vllm.test.ts shows 467 lines
• Missing regression test: N/A — architecture finding
• Done when: The required change is committed and verification passes: wc -l src/lib/inference/vllm.test.ts shows 467 lines.
• Evidence: diff shows +142 net lines; riskPlan monolithDeltas severity=blocker for this file

PRA-3 Required — E2E validation floor required: inference-routing and network-policy jobs must pass

• Location: not file-specific
• Category: tests
• Problem: RiskPlan tier 2 requires two E2E jobs for inference-policy invariants: provider reachable via advertised route, health reflects real request, network policy permits intended/denies unintended egress. Unit tests mock curl/Docker — cannot validate real sandbox network path.
• Impact: Merge without E2E validation risks undetected network policy mismatch or reachability regression at host→sandbox boundary
• Required action: Do not merge until inference-routing and network-policy CI jobs pass on this head SHA. These are required automatic jobs per riskPlan.
• Expected follow-up: Fix before merge or get explicit maintainer override.
• Verification: Check CI logs for inference-routing and network-policy job status on commit 3eb6b4c
• Missing regression test: Run inference-routing E2E job to verify provider reachability through advertised route; run network-policy E2E job to verify egress allow/deny at sandbox boundary
• Done when: The required change is committed and verification passes: Check CI logs for inference-routing and network-policy job status on commit 3eb6b4c.
• Evidence: riskPlan.requiredJobs lists inference-routing and network-policy as tier 2 automatic jobs matched to src/lib/inference/vllm.ts

Review findings by urgency: 3 required fixes, 1 item to resolve/justify, 0 in-scope improvements

⚠️ Resolve or justify before merge

Investigate these in the current review; either fix them, explain why they are not applicable, or document the accepted risk.

PRA-4 Resolve/justify — Missing regression test for waitForVllmReady failure path calling dockerStop

• Location: src/lib/inference/vllm.ts:574
• Category: tests
• Problem: New code path: on readiness failure, waitForVllmReady calls dockerStop (was runShell docker stop) and prints log tail. No dedicated test exercises this branch.
• Impact: Branch coverage gap; dockerStop failure behavior untested (though dockerStop ignores errors)
• Recommended action: Add test: 'waitForVllmReady failure calls dockerStop and prints log tail' in vLLM run command suite, or justify omission given dockerStop is thin adapter wrapper
• Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
• Verification: grep -n 'dockerStop' src/lib/inference/vllm.test.ts returns no matches
• Missing regression test: Test that when vllmEndpointReady returns false, dockerStop is called with containerName, ignoreError:true, suppressOutput:true, and log tail is printed to stderr
• Done when: The risk is fixed or explicitly justified in the PR. Verification: grep -n 'dockerStop' src/lib/inference/vllm.test.ts returns no matches.
• Evidence: diff shows dockerStop(profile.containerName, { ignoreError: true, suppressOutput: true }) in waitForVllmReady failure handler; no test covers this path

💡 In-scope improvements

These are lower-risk, not throwaway. Prefer fixing them in this PR when they are local to changed code; defer only with rationale or a linked follow-up.

• None.

Test follow-ups to resolve or justify

If these cover changed behavior, prefer adding them in this PR; otherwise state why existing coverage is enough or link the follow-up.

• PRA-T1 Runtime validation — Run inference-routing E2E job to verify provider reachability through advertised route and real /v1/models health check. Unit and mocked tests cover all changed code paths comprehensively. However, riskPlan inference-policy invariants (provider reachability, real health check, network policy) require live sandbox network validation via inference-routing and network-policy E2E jobs. These are the validation floor.
• PRA-T2 Runtime validation — Run network-policy E2E job to verify sandbox egress allow/deny at network policy boundary. Unit and mocked tests cover all changed code paths comprehensively. However, riskPlan inference-policy invariants (provider reachability, real health check, network policy) require live sandbox network validation via inference-routing and network-policy E2E jobs. These are the validation floor.
• PRA-T3 E2E validation floor required: inference-routing and network-policy jobs must pass — Do not merge until inference-routing and network-policy CI jobs pass on this head SHA. These are required automatic jobs per riskPlan.
• PRA-T4 Missing regression test for waitForVllmReady failure path calling dockerStop — Add test: 'waitForVllmReady failure calls dockerStop and prints log tail' in vLLM run command suite, or justify omission given dockerStop is thin adapter wrapper

Since last review details

Current findings, using the urgency labels above:

PRA-1 Required — Monolith growth: vllm.ts grew 620→669 lines (+49)

• Location: src/lib/inference/vllm.ts:1
• Category: architecture
• Problem: File exceeds 20-line growth threshold. New code includes argv builder, validation helpers, HF cache mount helpers, docker flag consolidation — all tightly coupled to host→sandbox trust boundary.
• Impact: Increased cognitive load, harder to audit security boundary, future changes risk regressing argv/token/GPU logic
• Required action: Accept growth for this PR (security-context coupling justifies it). File follow-up to extract: profile table, docker argv builder, install orchestrator, validation helpers into separate modules.
• Expected follow-up: Fix before merge or get explicit maintainer override.
• Verification: wc -l src/lib/inference/vllm.ts shows 669 lines
• Missing regression test: N/A — architecture finding, not a behavior gap
• Done when: The required change is committed and verification passes: wc -l src/lib/inference/vllm.ts shows 669 lines.
• Evidence: diff shows +49 net lines; riskPlan monolithDeltas severity=blocker for this file

PRA-2 Required — Monolith growth: vllm.test.ts grew 325→467 lines (+142)

• Location: src/lib/inference/vllm.test.ts:1
• Category: architecture
• Problem: Test file exceeds 20-line growth threshold. Comprehensive new coverage for argv validation, shell metacharacter handling, GPU CSV quoting, install flow with dockerRunDetached, invalid flag rejection.
• Impact: Test file becoming hard to navigate; related tests scattered across suites
• Required action: Accept for this PR (new coverage is thorough and security-relevant). Follow-up: split into vllm-argv.test.ts, vllm-install.test.ts, vllm-profile.test.ts
• Expected follow-up: Fix before merge or get explicit maintainer override.
• Verification: wc -l src/lib/inference/vllm.test.ts shows 467 lines
• Missing regression test: N/A — architecture finding
• Done when: The required change is committed and verification passes: wc -l src/lib/inference/vllm.test.ts shows 467 lines.
• Evidence: diff shows +142 net lines; riskPlan monolithDeltas severity=blocker for this file

PRA-3 Required — E2E validation floor required: inference-routing and network-policy jobs must pass

• Location: not file-specific
• Category: tests
• Problem: RiskPlan tier 2 requires two E2E jobs for inference-policy invariants: provider reachable via advertised route, health reflects real request, network policy permits intended/denies unintended egress. Unit tests mock curl/Docker — cannot validate real sandbox network path.
• Impact: Merge without E2E validation risks undetected network policy mismatch or reachability regression at host→sandbox boundary
• Required action: Do not merge until inference-routing and network-policy CI jobs pass on this head SHA. These are required automatic jobs per riskPlan.
• Expected follow-up: Fix before merge or get explicit maintainer override.
• Verification: Check CI logs for inference-routing and network-policy job status on commit 3eb6b4c
• Missing regression test: Run inference-routing E2E job to verify provider reachability through advertised route; run network-policy E2E job to verify egress allow/deny at sandbox boundary
• Done when: The required change is committed and verification passes: Check CI logs for inference-routing and network-policy job status on commit 3eb6b4c.
• Evidence: riskPlan.requiredJobs lists inference-routing and network-policy as tier 2 automatic jobs matched to src/lib/inference/vllm.ts

PRA-4 Resolve/justify — Missing regression test for waitForVllmReady failure path calling dockerStop

• Location: src/lib/inference/vllm.ts:574
• Category: tests
• Problem: New code path: on readiness failure, waitForVllmReady calls dockerStop (was runShell docker stop) and prints log tail. No dedicated test exercises this branch.
• Impact: Branch coverage gap; dockerStop failure behavior untested (though dockerStop ignores errors)
• Recommended action: Add test: 'waitForVllmReady failure calls dockerStop and prints log tail' in vLLM run command suite, or justify omission given dockerStop is thin adapter wrapper
• Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
• Verification: grep -n 'dockerStop' src/lib/inference/vllm.test.ts returns no matches
• Missing regression test: Test that when vllmEndpointReady returns false, dockerStop is called with containerName, ignoreError:true, suppressOutput:true, and log tail is printed to stderr
• Done when: The risk is fixed or explicitly justified in the PR. Verification: grep -n 'dockerStop' src/lib/inference/vllm.test.ts returns no matches.
• Evidence: diff shows dockerStop(profile.containerName, { ignoreError: true, suppressOutput: true }) in waitForVllmReady failure handler; no test covers this path

Workflow run details

This is an automated, non-binding review; it still expects maintainers and agents to respond to each required or warning item. Treat suggestions as current-PR improvements when they touch changed code; defer only with maintainer rationale or a linked follow-up. A human maintainer must make the final merge decision.

[github-actions]

E2E Advisor Recommendation

Required E2E: cloud-onboard, gpu-e2e, inference-routing
Optional E2E: credential-sanitization, spark-install

Dispatch hint: cloud-onboard,gpu-e2e,inference-routing

Workflow run

Full advisor summary

E2E Recommendation Advisor

Base: origin/main
Head: HEAD
Confidence: high

Required E2E

• cloud-onboard (high): Runs the full hosted onboarding flow through the same CLI/onboard import path that requires the vLLM module, catching module-load or onboarding integration regressions even when vLLM is not selected.
• gpu-e2e (very high): Closest existing live coverage for GPU local inference setup, Docker/GPU prerequisites, OpenShell sandbox integration, auth proxy, and inference.local behavior. It does not exercise vLLM directly but is the nearest merge-blocking confidence check for local GPU inference lifecycle changes.
• inference-routing (medium): Validates inference.local routing and provider/onboarding error boundaries. This is relevant because vLLM local installs ultimately feed the same routed inference surface used by sandboxes.

Optional E2E

• credential-sanitization (high): Optional additional confidence for credential redaction and sandbox secret exposure boundaries after changes to Hugging Face token forwarding for vLLM.
• spark-install (medium): Optional adjacent installer smoke for the Spark install lane. Existing coverage verifies standard install behavior on the Spark-targeted job but does not select or launch vLLM.

New E2E recommendations

• vLLM local inference install (high): No existing live E2E appears to select install-vllm or launch the NemoClaw-managed vLLM container. The changed code is the Docker argv launch path, HF cache mount/token forwarding, GPU device quoting, model download, readiness polling, and container cleanup, which are only partially covered by unit tests.
  • Suggested test: Add a live vllm-install E2E target/job that runs onboarding with the vLLM local provider on an NVIDIA GPU runner, verifies the vLLM container is started with argv-safe Docker invocation and no token value in process argv/artifacts, waits for /v1/models readiness, and proves a sandbox can call inference.local through the vLLM-backed route.
• DGX Station multi-GPU vLLM profile (medium): The PR changes Station multi-GPU Docker CSV quoting ("device=0,1"); current live E2E inventory does not show Station/vLLM-specific coverage for that dynamic GPU selection path.
  • Suggested test: Add a Station-capable vLLM profile E2E or hardware-gated subcase that validates GB300 GPU detection, generated --gpus argv token shape, successful container startup, and inference.local readiness.

Dispatch hint

• Workflow: .github/workflows/e2e.yaml
• jobs input: cloud-onboard,gpu-e2e,inference-routing

[github-actions]

E2E Target Recommendation

Required E2E targets: None
Optional E2E targets: None

Workflow run

Full E2E target advisor summary

E2E Target Advisor

Base: origin/main
Head: HEAD
Confidence: high

Required E2E targets

• None. The PR changes managed vLLM inference install/container-launch code and its unit tests, but the current e2e.yaml dispatch surface has no live-supported typed target or free-standing job that specifically exercises vLLM installation. Existing GPU E2E jobs exercise Ollama/GPU sandbox paths, not vLLM, so no E2E target dispatch is recommended as proof for this change.

Optional E2E targets

• None.

Relevant changed files

• None.

[github-actions]

PR Review Advisor — Changes requested

Merge posture: Do not merge yet
Primary next action: Resolve or justify PRA-1: Add inference-policy validation evidence for the changed vLLM launch path.
Open items: 0 required · 1 warning · 0 suggestions · 4 test follow-ups
Since last review: 0 prior items resolved · 0 still apply · 1 new item found

Action checklist

• PRA-1 Resolve or justify: Add inference-policy validation evidence for the changed vLLM launch path in src/lib/inference/vllm.ts:441
• PRA-T1 Add or justify test follow-up: Runtime validation
• PRA-T2 Add or justify test follow-up: Runtime validation
• PRA-T3 Add or justify test follow-up: Runtime validation
• PRA-T4 Add or justify test follow-up: Add inference-policy validation evidence for the changed vLLM launch path

Findings index

ID	Severity	Category	Location	Required action
PRA-1	Resolve/justify	tests	src/lib/inference/vllm.ts:441	Provide behavior-specific validation evidence for the existing `inference-routing` and `network-policy` risk-plan checks, and add a focused local regression test for readiness semantics if maintainers want unit-level coverage: mock the curl capture path so empty output, invalid JSON, and `{"data":{}}` fail while `{"data":[]}` succeeds, and verify failed readiness stops the managed container.

Review findings by urgency: 0 required fixes, 1 item to resolve/justify, 0 in-scope improvements

⚠️ Resolve or justify before merge

Investigate these in the current review; either fix them, explain why they are not applicable, or document the accepted risk.

PRA-1 Resolve/justify — Add inference-policy validation evidence for the changed vLLM launch path

• Location: src/lib/inference/vllm.ts:441
• Category: tests
• Problem: This PR changes the long-lived managed vLLM Docker launch boundary, including argv construction, port publishing, token forwarding, container replacement, and failure cleanup. The unit tests cover the argv and invalid-token paths well, but the deterministic risk invariants for the real inference boundary are only partially evidenced statically: readiness still uses `/v1/models`, and the run args still include `-p ${VLLM_PORT}:8000`, but there is no local regression evidence in this diff that the selected provider remains reachable through the route advertised to the agent or that the intended network-policy route is permitted while unintended egress is denied.
• Impact: A regression in Docker argv ordering, port exposure, local route selection, or network-policy integration could leave the managed vLLM provider unreachable from the agent or accidentally broaden/deny inference traffic even though unit tests pass against mocked Docker adapters.
• Recommended action: Provide behavior-specific validation evidence for the existing `inference-routing` and `network-policy` risk-plan checks, and add a focused local regression test for readiness semantics if maintainers want unit-level coverage: mock the curl capture path so empty output, invalid JSON, and `{"data":{}}` fail while `{"data":[]}` succeeds, and verify failed readiness stops the managed container.
• Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
• Verification: Shortest read-only check: inspect `src/lib/inference/vllm.ts` around `buildVllmRunArgs()`, `startContainer()`, `vllmEndpointReady()`, and `waitForVllmReady()` plus `src/lib/inference/vllm.test.ts` for coverage of `/v1/models` readiness and host-to-sandbox route behavior; the current tests cover argv construction but do not drive the readiness loop or network-policy boundary.
• Missing regression test: Add a test that exercises `installVllm()` or an exported/testable readiness helper with mocked `runCapture`: return `''`, malformed JSON, `{"data":{}}`, and finally `{"data":[]}`; assert readiness only succeeds for the JSON-array response, uses `http://127.0.0.1:${VLLM\_PORT}/v1/models\`, and calls `dockerStop(profile.containerName, ...)` on readiness timeout/failure. Runtime validation evidence should cover the real `inference-routing` and `network-policy` invariants for the changed launch path.
• Done when: The risk is fixed or explicitly justified in the PR. Verification: Shortest read-only check: inspect `src/lib/inference/vllm.ts` around `buildVllmRunArgs()`, `startContainer()`, `vllmEndpointReady()`, and `waitForVllmReady()` plus `src/lib/inference/vllm.test.ts` for coverage of `/v1/models` readiness and host-to-sandbox route behavior; the current tests cover argv construction but do not drive the readiness loop or network-policy boundary.
• Evidence: Risk plan marks `inference-policy` for `src/lib/inference/vllm.ts` with invariants: selected provider reachable through advertised route, health reflects a real request, and network policy permits intended route/denies unintended egress. Diff changes `buildVllmRunArgs()` and `startContainer()` to call `dockerRunDetached(runArgs, ...)`; existing tests assert argv/token behavior but do not exercise `vllmEndpointReady()` or real policy routing.

💡 In-scope improvements

These are lower-risk, not throwaway. Prefer fixing them in this PR when they are local to changed code; defer only with rationale or a linked follow-up.

• None.

Test follow-ups to resolve or justify

If these cover changed behavior, prefer adding them in this PR; otherwise state why existing coverage is enough or link the follow-up.

• PRA-T1 Runtime validation — Provide evidence from the existing `inference-routing` validation for this head SHA showing the managed vLLM provider selected by onboarding remains reachable through the route advertised to the agent after the Docker argv launch change.. Unit coverage is strong for the argv/security regression, but the deterministic `inference-policy` risk family requires evidence at the real host-to-sandbox inference boundary. Static review can see the local `/v1/models` readiness check and port mapping are preserved, but cannot prove real routing and network-policy behavior.
• PRA-T2 Runtime validation — Provide evidence from the existing `network-policy` validation for this head SHA showing the intended vLLM route is permitted and unintended egress remains denied after the Docker argv launch change.. Unit coverage is strong for the argv/security regression, but the deterministic `inference-policy` risk family requires evidence at the real host-to-sandbox inference boundary. Static review can see the local `/v1/models` readiness check and port mapping are preserved, but cannot prove real routing and network-policy behavior.
• PRA-T3 Runtime validation — Add a focused unit regression around readiness: mock curl capture responses for empty output, malformed JSON, `{"data":{}}`, and `{"data":[]}`; assert only the JSON-array `/v1/models` response marks vLLM ready and failed readiness stops the managed container.. Unit coverage is strong for the argv/security regression, but the deterministic `inference-policy` risk family requires evidence at the real host-to-sandbox inference boundary. Static review can see the local `/v1/models` readiness check and port mapping are preserved, but cannot prove real routing and network-policy behavior.
• PRA-T4 Add inference-policy validation evidence for the changed vLLM launch path — Provide behavior-specific validation evidence for the existing `inference-routing` and `network-policy` risk-plan checks, and add a focused local regression test for readiness semantics if maintainers want unit-level coverage: mock the curl capture path so empty output, invalid JSON, and `{"data":{}}` fail while `{"data":[]}` succeeds, and verify failed readiness stops the managed container.

Since last review details

Current findings, using the urgency labels above:

PRA-1 Resolve/justify — Add inference-policy validation evidence for the changed vLLM launch path

• Location: src/lib/inference/vllm.ts:441
• Category: tests
• Problem: This PR changes the long-lived managed vLLM Docker launch boundary, including argv construction, port publishing, token forwarding, container replacement, and failure cleanup. The unit tests cover the argv and invalid-token paths well, but the deterministic risk invariants for the real inference boundary are only partially evidenced statically: readiness still uses `/v1/models`, and the run args still include `-p ${VLLM_PORT}:8000`, but there is no local regression evidence in this diff that the selected provider remains reachable through the route advertised to the agent or that the intended network-policy route is permitted while unintended egress is denied.
• Impact: A regression in Docker argv ordering, port exposure, local route selection, or network-policy integration could leave the managed vLLM provider unreachable from the agent or accidentally broaden/deny inference traffic even though unit tests pass against mocked Docker adapters.
• Recommended action: Provide behavior-specific validation evidence for the existing `inference-routing` and `network-policy` risk-plan checks, and add a focused local regression test for readiness semantics if maintainers want unit-level coverage: mock the curl capture path so empty output, invalid JSON, and `{"data":{}}` fail while `{"data":[]}` succeeds, and verify failed readiness stops the managed container.
• Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
• Verification: Shortest read-only check: inspect `src/lib/inference/vllm.ts` around `buildVllmRunArgs()`, `startContainer()`, `vllmEndpointReady()`, and `waitForVllmReady()` plus `src/lib/inference/vllm.test.ts` for coverage of `/v1/models` readiness and host-to-sandbox route behavior; the current tests cover argv construction but do not drive the readiness loop or network-policy boundary.
• Missing regression test: Add a test that exercises `installVllm()` or an exported/testable readiness helper with mocked `runCapture`: return `''`, malformed JSON, `{"data":{}}`, and finally `{"data":[]}`; assert readiness only succeeds for the JSON-array response, uses `http://127.0.0.1:${VLLM\_PORT}/v1/models\`, and calls `dockerStop(profile.containerName, ...)` on readiness timeout/failure. Runtime validation evidence should cover the real `inference-routing` and `network-policy` invariants for the changed launch path.
• Done when: The risk is fixed or explicitly justified in the PR. Verification: Shortest read-only check: inspect `src/lib/inference/vllm.ts` around `buildVllmRunArgs()`, `startContainer()`, `vllmEndpointReady()`, and `waitForVllmReady()` plus `src/lib/inference/vllm.test.ts` for coverage of `/v1/models` readiness and host-to-sandbox route behavior; the current tests cover argv construction but do not drive the readiness loop or network-policy boundary.
• Evidence: Risk plan marks `inference-policy` for `src/lib/inference/vllm.ts` with invariants: selected provider reachable through advertised route, health reflects a real request, and network policy permits intended route/denies unintended egress. Diff changes `buildVllmRunArgs()` and `startContainer()` to call `dockerRunDetached(runArgs, ...)`; existing tests assert argv/token behavior but do not exercise `vllmEndpointReady()` or real policy routing.

Workflow run details

This is an automated, non-binding review; it still expects maintainers and agents to respond to each required or warning item. Treat suggestions as current-PR improvements when they touch changed code; defer only with maintainer rationale or a linked follow-up. A human maintainer must make the final merge decision.

[apurvvkumaria]

Maintainer follow-up 4fc2c9b90 addresses the remaining exact-head advisor concern: dynamic vLLM launch flags are now resolved and validated before dockerForceRm, so invalid configuration cannot remove an existing healthy container. The negative-path test now proves neither teardown nor launch occurs.

Validation on the pushed head: 56 focused CLI tests passed; CLI type-check, pre-commit checks, pre-push checks, formatting, size checks, and secret scanning passed. Minh Vu remains credited as co-author. The prior CodeQL failures were the repository-wide action-version mismatch fixed and proven green by #6524; fresh exact-head CI is running.

[cv]

Exact-head maintainer review at 4fc2c9b90: approved.

• All 45 CI checks are green; DCO is present and all three commits are GitHub Verified.
• CodeRabbit has no unresolved major/critical finding, and the trusted PR Review Advisor recommends merge_as_is.
• Nine-category security review passed: the host-shell command boundary is replaced by Docker argv; token values stay in the subprocess environment and out of argv; invalid empty/NUL-bearing tokens fail before the existing container is removed; no new dependency, auth, crypto, or network-policy surface is introduced.
• Local focused verification passed 56 CLI vLLM tests and 26 vLLM onboarding/profile integration tests.

[github-actions]

E2E Target Results — ✅ All requested jobs passed

Run: 28995085839
Workflow ref: codex/pr5475-vllm-docker-argv
Requested targets: (default — all supported)
Requested jobs: gpu-e2e,inference-routing
Summary: 2 passed, 0 failed, 0 cancelled, 0 skipped

Job	Result
gpu-e2e	✅ success
inference-routing	✅ success

[github-actions]

E2E Target Results — ✅ All requested jobs passed

Run: 28996103959
Workflow ref: codex/pr5475-vllm-docker-argv
Requested targets: (default — all supported)
Requested jobs: cloud-onboard,gpu-e2e,inference-routing
Summary: 3 passed, 0 failed, 0 cancelled, 0 skipped

Job	Result
cloud-onboard	✅ success
gpu-e2e	✅ success
inference-routing	✅ success