fix(deployments): deliver config_files on the openshell backend (AIRCORE-999) - #1145
Merged
maxdubrinsky merged 2 commits intoAug 7, 2026
Conversation
…ORE-999) The openshell backend accepted DeploymentConfig.config_files, reached READY, and never wrote the files -- a silent drop, since only the k8s backend implemented the field. Deliver each config file into the sandbox before serve launch via ExecSandbox with the content on the RPC's stdin field (cat > path; chmod), so the bytes are opaque to the shell: no argv escaping, no single-line/size-capped env. Delivery runs as the non-root sandbox user, so a target it cannot write (e.g. /workspace, which the packaged image chowns to 'agent') fails loudly with the path and the shell's error and tears the sandbox down, rather than reaching READY with the file absent. Docker delivery and deleting the nemo-agents NAT_CONFIG_YAML workaround are deferred to a follow-up. Signed-off-by: Max Dubrinsky <mdubrinsky@nvidia.com>
maxdubrinsky
marked this pull request as ready for review
August 6, 2026 21:47
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe OpenShell backend streams declared ChangesOpenShell config delivery
Sequence Diagram(s)sequenceDiagram
participant OpenShellBackend
participant ExecSandbox
participant DeliveryScript
participant ServeProcess
OpenShellBackend->>ExecSandbox: stream config content and delivery command
ExecSandbox->>DeliveryScript: create directories and write config file
DeliveryScript-->>ExecSandbox: return completion marker and exit status
ExecSandbox-->>OpenShellBackend: return delivery result
OpenShellBackend->>ServeProcess: launch after successful delivery
OpenShellBackend->>ExecSandbox: delete sandbox after delivery failure
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@plugins/nemo-deployments/src/nemo_deployments_plugin/backends/openshell/backend.py`:
- Around line 507-514: Make the serve startup sequence in read_status atomic
across concurrent calls by protecting _serve_launched, _deliver_config_files,
and the subsequent serve launch with a sandbox-wide lock or atomic launch
marker. Ensure only one caller can probe, deliver configuration files, and
launch the serve process, while concurrent callers observe the established
launch state and skip relaunching.
- Around line 712-724: Update the config delivery validation around
_CONFIG_DELIVERED_MARKER to require exit_code == 0, rejecting exit_code is None
even when the completion marker appears in output. Add a test covering
_stream_without_exit(stdout=_CONFIG_DELIVERED_MARKER) and assert the delivery
fails without launching the serve command.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: c4f073fb-7fe7-4d90-86ed-eae04c8c17f8
📒 Files selected for processing (2)
plugins/nemo-deployments/src/nemo_deployments_plugin/backends/openshell/backend.pyplugins/nemo-deployments/tests/unit/backends/openshell/test_openshell_backend_mocked.py
15 tasks
mckornfield
approved these changes
Aug 6, 2026
mckornfield
left a comment
mckornfield
left a comment
Contributor
There was a problem hiding this comment.
wonder if tests could be DRY a bit but 🤷 maybe it will look worse
Extract the inline delivery one-liner into a pure _delivery_script(path, mode) helper and add two behavioral tests that run the exact script the backend generates through a real /bin/sh: - adversarial content on stdin lands in the file byte-for-byte while the shell-injection payloads never execute (a seeded canary is untouched), proving the stdin path keeps untrusted content inert - a read-only target makes the shell exit non-zero without the completion marker, exercising the real FAILED branch instead of a mocked stream Tighten the delivery docstrings and comments per review: drop change-history narration and the cut-off /workspace phrasing, state behavior directly. Signed-off-by: Max Dubrinsky <mdubrinsky@nvidia.com>
Contributor
|
maxdubrinsky
deleted the
mdubrinsky/aircore-999-config_files-is-accepted-and-validated-then-silently
branch
August 7, 2026 19:00
8 tasks
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Summary
DeploymentConfig.config_filesis a first-class API field (accepted, validated, given a defaultmode, echoed back), but only the k8s backend implemented it. The openshell backend accepted the field, reachedREADY, and never wrote the files: a silent drop, so a caller had every reason to believe its config was delivered. This makes openshell deliver the files, or fail loudly when it cannot.Related Issue
AIRCORE-999.
Changes
config_filesentry into the sandbox before serve launch viaExecSandbox, with the content on the RPC's first-classstdinbytes field (sh -c 'set -e; mkdir -p <dir>; cat > <path>; chmod <mode> <path>'). The content is streamed, so it never touches the argv nor the single-line, size-capped sandbox environment._exec_detachedgained astdinkwarg._advance_provisioning, immediately before launch, so files are written exactly once per deployment. Delivery runs as the non-root sandbox user (ExecSandbox has no user field; the policy pinsrun_as_user), so a target that user cannot write (e.g./workspace, which the packaged agent image chowns toagenteven though the sandbox policy lists it read-write) fails with a terminalFAILEDnaming the path and the shell's error, and the sandbox is torn down, rather than reachingREADYfile-less.set -eclears mkdir/cat/chmod, and a write is accepted only when that marker is present in the output. This closes the residual silent-drop path where anExecSandboxstream ends without an exit event, without changing the shared exec probes' "only positive evidence flaps a deployment" semantics.Type of Change
Quality Gates
Verification
Signed-off-by:traileruv run pre-commit run -apasses, or any blocked checks are identified belowTargeted validation:
pre-commit run --files <changed files>: ruff, ruff format, ty, copyright headers, plugin-import guard, and merge-conflict hooks all pass; the remaining hooks skip as not-applicable. Run scoped to the changed files rather than-abecause a full workspace sync could not be built in this worktree (a transitive dependency needsPython.h); the applicable hooks are the ones exercised above.pytest plugins/nemo-deployments/tests/unit/backends/openshell: 93 passed. Adversarial cases include stdin-streaming with delivery-before-launch, multi-file, octal mode, shell-metacharacter content that must not be interpreted, spaced-path quoting, unwritable/workspacefailing loudly and deleting the sandbox, rpc-error failing loudly, unconfirmed-delivery (no completion marker) failing loudly, multi-file partial failure, empty-list no-op, and write-exactly-once.plugins/nemo-deployments/tests/unit: passes excepttest_ports.py::test_find_available_port_excludes_pending_assignments, a pre-existing real-socket flake that reproduces identically on unmodifiedmain(host port 9000 is occupied on the dev box) and is unrelated to this change, which touches only the openshell backend.Notes
Docker delivery and deleting the nemo-agents
NAT_CONFIG_YAMLenv-var workaround are deferred to a follow-up ticket, along with a possible fileset-id delivery path for large artifacts. This PR scopes to the openshell backend, whose delivery mechanism is proven.Summary by CodeRabbit
New Features
Bug Fixes