fix(sandbox): derive staged scripts from Dockerfile to prevent drift

[jason-ma-nv]

Summary

stageOptimizedSandboxBuildContext maintained a hardcoded list of scripts/ files to copy into the Docker build context. Any COPY scripts/<file> added to the Dockerfile without a matching entry in that list caused a silent "file not found in build context" failure at sandbox creation time (see #2503). Replace the explicit allowlist with a Dockerfile parser so the two can never drift.

Related Issue

Fixes #2503

Changes

• src/lib/sandbox-build-context.ts: replace hardcoded scripts/ file list with a loop that parses every COPY scripts/<src> <dest> line in the staged Dockerfile (skipping --from= inter-stage copies) and copies each source into the build context at the same relative path. The Dockerfile is now the single source of truth.
• test/sandbox-build-context.test.ts: add regression test that parses the same COPY scripts/ lines from the Dockerfile and asserts each source path exists in the staged build context produced by stageOptimizedSandboxBuildContext.

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)

Verification

• npx prek run --all-files passes
• npm test passes
• Tests added or updated for new or changed behavior
• No secrets, API keys, or credentials committed
• Docs updated for user-facing behavior changes
• make 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)

AI Disclosure

• AI-assisted — tool: Cursor

---

Signed-off-by: jama jama@nvidia.com

Made with Cursor

Summary by CodeRabbit

• Refactor

  • Build script staging now dynamically aligns staged scripts with build configuration instead of using fixed references, ensuring staged contents match declared script entries.

• Tests

  • Added a validation test that checks all scripts referenced by the build configuration are present in the staged build context.

[copy-pr-bot]

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

[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: b7d2d929-e87e-47ce-ab70-8a1585bc09d9

📥 Commits

Reviewing files that changed from the base of the PR and between ff7193a and 540d06e.

📒 Files selected for processing (1)

• test/sandbox-build-context.test.ts

---

📝 Walkthrough

Walkthrough

Parses the generated Dockerfile for COPY directives that reference scripts/<...> (ignoring inter-stage COPY --from=...) and stages each referenced script into the sandbox build context, preserving relative paths; adds a test that validates all referenced script sources are present in the staged build context.

Changes

Cohort / File(s)	Summary
Dynamic Script Staging src/lib/sandbox-build-context.ts	Replaced hardcoded script copying with Dockerfile-driven staging: parse COPY lines targeting scripts/<...>, skip --from= inter-stage copies, and copy each referenced scripts/<src> from rootDir into buildCtx preserving relative paths.
Validation Test test/sandbox-build-context.test.ts	Added test that extracts COPY scripts/<...> sources from the repo Dockerfile, runs stageOptimizedSandboxBuildContext(...), and asserts each referenced script exists in the resulting build context.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰 I scurried through Dockerfile lines,
Collected scripts and matched the signs,
I copied each path where it belongs,
No missing files, no build-time wrongs 🥕✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately and concisely describes the main change: deriving staged scripts from the Dockerfile instead of using a hardcoded allowlist to prevent drift between the Dockerfile and build context.
Linked Issues check	✅ Passed	The PR directly addresses issue #2503 by replacing hardcoded script copying with dynamic parsing of Dockerfile COPY directives, ensuring all scripts referenced in the Dockerfile are included in the build context.
Out of Scope Changes check	✅ Passed	All changes are scoped to resolving the linked issue: modifying sandbox-build-context.ts to parse the Dockerfile and adding a regression test to verify script inclusion.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ 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 fix/sandbox-build-context-dockerfile-copy-drift

---

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

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (1)

src/lib/sandbox-build-context.ts (1)

61-76: Consider adding error handling for missing source files.

If a script referenced in the Dockerfile doesn't exist at srcPath, fs.copyFileSync will throw a generic ENOENT error. A more descriptive error message would help developers diagnose the issue faster.

Additionally, the regex pattern here differs from the test's pattern (line 66 handles flags like --chown, but the test's pattern on line 24 of the test file does not). This inconsistency could cause the test to extract a different set of paths than the implementation copies, potentially missing regressions.

Proposed improvement for error handling

       const relSrc = match[1]; // e.g. "scripts/nemoclaw-start.sh"
       const srcPath = path.join(rootDir, relSrc);
       const destPath = path.join(buildCtx, relSrc);
+      if (!fs.existsSync(srcPath)) {
+        throw new Error(
+          `Dockerfile references "${relSrc}" but file not found at "${srcPath}". ` +
+          `Ensure the script exists or remove the COPY directive.`
+        );
+      }
       fs.mkdirSync(path.dirname(destPath), { recursive: true });
       fs.copyFileSync(srcPath, destPath);

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/lib/sandbox-build-context.ts` around lines 61 - 76, The code that
extracts and copies script paths from stagedDockerfile can throw opaque ENOENT
errors when a referenced source (srcPath) is missing and the regex copyPattern
may not match the same forms as tests; update the implementation by (1) changing
copyPattern to accept optional COPY flags like --chown and multiple flag forms
(e.g. /^COPY(?:\s+--\w+[^\s]*)*\s+(scripts\/\S+)\s+\S/gm or align it exactly
with the test's regex) so extraction matches tests, and (2) wrap the
fs.copyFileSync call in a try/catch (or check fs.existsSync before copying) and
throw/log a clearer error that includes the missing srcPath, the Dockerfile line
or match text (match[0]) and context (stagedDockerfile/rootDir) to make ENOENT
failures descriptive; reference symbols: dockerfileContent, copyPattern, match,
srcPath, destPath, fs.copyFileSync.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@test/sandbox-build-context.test.ts`:
- Around line 22-29: The test's COPY extraction (variable copyPattern and the
loop that fills scriptSources from dockerfileContent) must be updated to match
the implementation's behavior: allow optional COPY flags like --chown/--chmod
and skip COPY lines that include --from= (inter-stage copies). Either replace
the hardcoded regex with the same pattern used by the implementation (extract
the pattern into a shared constant and import it into the test) or update the
test's copyPattern to accept leading flags and reject lines containing --from=
so the test no longer produces false positives/negatives.

---

Nitpick comments:
In `@src/lib/sandbox-build-context.ts`:
- Around line 61-76: The code that extracts and copies script paths from
stagedDockerfile can throw opaque ENOENT errors when a referenced source
(srcPath) is missing and the regex copyPattern may not match the same forms as
tests; update the implementation by (1) changing copyPattern to accept optional
COPY flags like --chown and multiple flag forms (e.g.
/^COPY(?:\s+--\w+[^\s]*)*\s+(scripts\/\S+)\s+\S/gm or align it exactly with the
test's regex) so extraction matches tests, and (2) wrap the fs.copyFileSync call
in a try/catch (or check fs.existsSync before copying) and throw/log a clearer
error that includes the missing srcPath, the Dockerfile line or match text
(match[0]) and context (stagedDockerfile/rootDir) to make ENOENT failures
descriptive; reference symbols: dockerfileContent, copyPattern, match, srcPath,
destPath, fs.copyFileSync.

🪄 Autofix (Beta)

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: a1073aff-4f76-430c-9451-cb9d566bd6dc

📥 Commits

Reviewing files that changed from the base of the PR and between 6f7f0c6 and ff7193a.

📒 Files selected for processing (2)

• src/lib/sandbox-build-context.ts
• test/sandbox-build-context.test.ts