test(e2e): migrate test-sandbox-rebuild.sh to vitest#5333
Merged
Conversation
Contributor
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughAdds a sandbox-rebuild e2e Vitest scenario with fixture rebuild/readiness helpers, state/registry/backup utilities, a live test, supporting unit and workflow tests, a CI job ChangesSandbox Rebuild E2E Testing
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Contributor
E2E Advisor RecommendationRequired E2E: Dispatch hint: Full advisor summaryE2E Recommendation AdvisorBase: Required E2E
Optional E2E
New E2E recommendations
Dispatch hint
|
Contributor
Vitest E2E Scenario RecommendationRequired Vitest E2E scenarios: Dispatch required Vitest E2E scenarios:
Full Vitest E2E advisor summaryVitest E2E Scenario AdvisorBase: Required Vitest E2E scenarios
Optional Vitest E2E scenarios
Relevant changed files
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 @.github/workflows/e2e-vitest-scenarios.yaml:
- Line 1222: The DOCKER_CONFIG env var is set to the checkout
(${GITHUB_WORKSPACE}) which exposes Docker credentials to the repo; change the
DOCKER_CONFIG assignment in the workflow (replace DOCKER_CONFIG: ${{
github.workspace }}/.docker-config-model-router-provider-routed-inference) to
point to a runner temporary dir (e.g., ${{ runner.temp }}/.docker-config-...) or
another non-workspace path, and update the boundary validator in
tools/e2e-scenarios/workflow-boundary.mts to enforce that DOCKER_CONFIG (and any
docker auth paths) are outside the checkout by rejecting or warning on paths
inside ${GITHUB_WORKSPACE} rather than blessing them. Ensure the env variable
name DOCKER_CONFIG and the validator function(s) that currently allow workspace
paths are the targets of the change.
In `@test/e2e-scenario/fixtures/phases/state-validation.ts`:
- Around line 342-349: The equality check currently ignores leading/trailing
whitespace by using actual.trim(), which hides intentional whitespace
differences; in the marker verification (where actual is read via
this.readMarkerFile and compared to expected for markerPath), remove the
actual.trim() comparison and enforce exact equality, or if necessary normalize
only a single trailing newline (e.g., allow actual === expected OR
(actual.endsWith("\n") && actual.slice(0,-1) === expected)); update the
conditional that throws the Error to use this stricter comparison and keep the
same sandboxName/error message logic.
In `@test/e2e-scenario/live/sandbox-rebuild.test.ts`:
- Around line 127-132: The cleanup uses os.homedir() directly while
snapshotRegistryAndSession() and latestRebuildBackupDir() use process.env.HOME
?? os.homedir(), causing mismatched backup paths when HOME is overridden; update
the cleanup to compute backupRoot the same way (use process.env.HOME ??
os.homedir() or call the existing latestRebuildBackupDir() helper) and then use
that value in the fs.rmSync call and the cleanup description so the test removes
the same rebuild-backup tree created during the run (also ensure
restoreRegistryAndSession(stateSnapshot) remains invoked).
In `@tools/e2e-scenarios/workflow-boundary.mts`:
- Around line 701-705: The validator for the "Install OpenShell" job
(installOpenShell) lacks checks that the workflow scrubs Docker Hub credentials;
add requireRunContains assertions for the installOpenShell step that assert the
install command unsets DOCKERHUB_USERNAME and DOCKERHUB_TOKEN (i.e., add
requireRunContains(errors, installOpenShell, "-u DOCKERHUB_USERNAME") and
requireRunContains(errors, installOpenShell, "-u DOCKERHUB_TOKEN")) alongside
the existing env -u DOCKER_CONFIG and token unsets to mirror the
sandbox-rebuild-vitest behavior and prevent re-exposing Docker creds in
scripts/install-openshell.sh.
🪄 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: 913b1e82-ec9d-4088-9066-157aaebeaeeb
📒 Files selected for processing (8)
.github/workflows/e2e-vitest-scenarios.yamltest/e2e-scenario/fixtures/phases/lifecycle.tstest/e2e-scenario/fixtures/phases/onboarding.tstest/e2e-scenario/fixtures/phases/state-validation.tstest/e2e-scenario/live/sandbox-rebuild.test.tstest/e2e-scenario/support-tests/e2e-phase-state-validation.test.tstest/e2e-scenario/support-tests/e2e-scenarios-workflow.test.tstools/e2e-scenarios/workflow-boundary.mts
Comment on lines
+342
to
+349
| const actual = await this.readMarkerFile(instance, markerPath, options); | ||
| if (actual !== expected && actual.trim() !== expected) { | ||
| const sandboxName = typeof instance === "string" ? instance : instance.sandboxName; | ||
| throw new Error( | ||
| `marker ${markerPath} in ${sandboxName} did not match expected content: got ${JSON.stringify( | ||
| actual, | ||
| )}`, | ||
| ); |
Contributor
There was a problem hiding this comment.
Don’t trim marker contents during equality checks.
actual.trim() !== expected makes leading/trailing whitespace invisible, so a rebuild that changes "token" to " token " (or drops intentional whitespace) would still pass this assertion. This helper is meant to prove exact workspace persistence, so the comparison should stay exact, or at most normalize a single trailing newline if the transport is known to add one.
Suggested fix
async expectMarkerFileContent(
instance: NemoClawInstance | string,
markerPath: string,
expected: string,
options: MarkerFileOptions = {},
): Promise<void> {
const actual = await this.readMarkerFile(instance, markerPath, options);
- if (actual !== expected && actual.trim() !== expected) {
+ if (actual !== expected) {
const sandboxName = typeof instance === "string" ? instance : instance.sandboxName;
throw new Error(
`marker ${markerPath} in ${sandboxName} did not match expected content: got ${JSON.stringify(
actual,
)}`,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const actual = await this.readMarkerFile(instance, markerPath, options); | |
| if (actual !== expected && actual.trim() !== expected) { | |
| const sandboxName = typeof instance === "string" ? instance : instance.sandboxName; | |
| throw new Error( | |
| `marker ${markerPath} in ${sandboxName} did not match expected content: got ${JSON.stringify( | |
| actual, | |
| )}`, | |
| ); | |
| const actual = await this.readMarkerFile(instance, markerPath, options); | |
| if (actual !== expected) { | |
| const sandboxName = typeof instance === "string" ? instance : instance.sandboxName; | |
| throw new Error( | |
| `marker ${markerPath} in ${sandboxName} did not match expected content: got ${JSON.stringify( | |
| actual, | |
| )}`, | |
| ); |
🤖 Prompt for 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.
In `@test/e2e-scenario/fixtures/phases/state-validation.ts` around lines 342 -
349, The equality check currently ignores leading/trailing whitespace by using
actual.trim(), which hides intentional whitespace differences; in the marker
verification (where actual is read via this.readMarkerFile and compared to
expected for markerPath), remove the actual.trim() comparison and enforce exact
equality, or if necessary normalize only a single trailing newline (e.g., allow
actual === expected OR (actual.endsWith("\n") && actual.slice(0,-1) ===
expected)); update the conditional that throws the Error to use this stricter
comparison and keep the same sandboxName/error message logic.
Comment on lines
+127
to
+132
| const stateSnapshot = snapshotRegistryAndSession(); | ||
| const backupRoot = path.join(os.homedir(), ".nemoclaw", "rebuild-backups", SANDBOX_NAME); | ||
| cleanup.add(`restore NemoClaw state files for ${SANDBOX_NAME}`, () => { | ||
| restoreRegistryAndSession(stateSnapshot); | ||
| fs.rmSync(backupRoot, { recursive: true, force: true }); | ||
| }); |
Contributor
There was a problem hiding this comment.
Align backup cleanup with the helper’s HOME resolution.
snapshotRegistryAndSession() and latestRebuildBackupDir() both key off process.env.HOME ?? os.homedir(), but this cleanup hardcodes os.homedir(). If the live runner overrides HOME for isolation, the test mutates/scans one rebuild-backup tree and deletes another, which leaves stale backups behind for reruns.
Suggested fix
const stateSnapshot = snapshotRegistryAndSession();
- const backupRoot = path.join(os.homedir(), ".nemoclaw", "rebuild-backups", SANDBOX_NAME);
+ const home = process.env.HOME ?? os.homedir();
+ const backupRoot = path.join(home, ".nemoclaw", "rebuild-backups", SANDBOX_NAME);
cleanup.add(`restore NemoClaw state files for ${SANDBOX_NAME}`, () => {
restoreRegistryAndSession(stateSnapshot);
fs.rmSync(backupRoot, { recursive: true, force: true });
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const stateSnapshot = snapshotRegistryAndSession(); | |
| const backupRoot = path.join(os.homedir(), ".nemoclaw", "rebuild-backups", SANDBOX_NAME); | |
| cleanup.add(`restore NemoClaw state files for ${SANDBOX_NAME}`, () => { | |
| restoreRegistryAndSession(stateSnapshot); | |
| fs.rmSync(backupRoot, { recursive: true, force: true }); | |
| }); | |
| const stateSnapshot = snapshotRegistryAndSession(); | |
| const home = process.env.HOME ?? os.homedir(); | |
| const backupRoot = path.join(home, ".nemoclaw", "rebuild-backups", SANDBOX_NAME); | |
| cleanup.add(`restore NemoClaw state files for ${SANDBOX_NAME}`, () => { | |
| restoreRegistryAndSession(stateSnapshot); | |
| fs.rmSync(backupRoot, { recursive: true, force: true }); | |
| }); |
🤖 Prompt for 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.
In `@test/e2e-scenario/live/sandbox-rebuild.test.ts` around lines 127 - 132, The
cleanup uses os.homedir() directly while snapshotRegistryAndSession() and
latestRebuildBackupDir() use process.env.HOME ?? os.homedir(), causing
mismatched backup paths when HOME is overridden; update the cleanup to compute
backupRoot the same way (use process.env.HOME ?? os.homedir() or call the
existing latestRebuildBackupDir() helper) and then use that value in the
fs.rmSync call and the cleanup description so the test removes the same
rebuild-backup tree created during the run (also ensure
restoreRegistryAndSession(stateSnapshot) remains invoked).
Contributor
PR Review AdvisorFindings: 2 needs attention, 7 worth checking, 1 nice ideas Review findings🛠️ Needs attention
🔎 Worth checking
🌱 Nice ideas
Consider writing more tests for
Since last review detailsCurrent findings:
This is an automated advisory review. A human maintainer must make the final merge decision. |
Contributor
Vitest E2E Scenario Results — ❌ Some jobs failedRun: 27420621638
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@test/e2e-scenario/fixtures/phases/lifecycle.ts`:
- Around line 91-95: The readiness probe in outputContainsReadySandbox currently
treats the sandboxName as a prefix so names like "e2e-x-dev" match "e2e-x";
update the check in outputContainsReadySandbox to require an exact sandbox-name
token on a single line before the Ready token (e.g., iterate over lines of
stripAnsi(`${result.stdout}\n${result.stderr}`) and for each line test a regex
that anchors the sandbox name as a separate token—such as
/^\s*<escaped>\b\s+.*\bReady\b/i—so only the exact sandbox row satisfies
readiness; reference the function outputContainsReadySandbox (and callers like
assertSandboxReadyAfterRebuild) when making the change.
🪄 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: c81314b8-df1c-452d-93f7-d91b64564f31
📒 Files selected for processing (2)
test/e2e-scenario/fixtures/phases/lifecycle.tstest/e2e-scenario/support-tests/e2e-phase-lifecycle.test.ts
Comment on lines
+91
to
+95
| function outputContainsReadySandbox(result: ShellProbeResult, sandboxName: string): boolean { | ||
| const escaped = sandboxName.replace(/[.*+?^${}()|[\]\\]/g, "\\$&"); | ||
| return new RegExp(`${escaped}.*\\bReady\\b`, "i").test( | ||
| stripAnsi(`${result.stdout}\n${result.stderr}`), | ||
| ); |
Contributor
There was a problem hiding this comment.
Require an exact sandbox-name match in the readiness probe.
Line 93 currently treats any row containing the requested name as a prefix match, so e2e-x-dev Ready would satisfy readiness for e2e-x. Because assertSandboxReadyAfterRebuild() stops polling on that condition, the live rebuild scenario can pass against the wrong sandbox row and hide a failed rebuild.
Suggested fix
function outputContainsReadySandbox(result: ShellProbeResult, sandboxName: string): boolean {
const escaped = sandboxName.replace(/[.*+?^${}()|[\]\\]/g, "\\$&");
- return new RegExp(`${escaped}.*\\bReady\\b`, "i").test(
+ return new RegExp(`^\\s*${escaped}(?:\\s|$).*\\bReady\\b`, "im").test(
stripAnsi(`${result.stdout}\n${result.stderr}`),
);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| function outputContainsReadySandbox(result: ShellProbeResult, sandboxName: string): boolean { | |
| const escaped = sandboxName.replace(/[.*+?^${}()|[\]\\]/g, "\\$&"); | |
| return new RegExp(`${escaped}.*\\bReady\\b`, "i").test( | |
| stripAnsi(`${result.stdout}\n${result.stderr}`), | |
| ); | |
| function outputContainsReadySandbox(result: ShellProbeResult, sandboxName: string): boolean { | |
| const escaped = sandboxName.replace(/[.*+?^${}()|[\]\\]/g, "\\$&"); | |
| return new RegExp(`^\\s*${escaped}(?:\\s|$).*\\bReady\\b`, "im").test( | |
| stripAnsi(`${result.stdout}\n${result.stderr}`), | |
| ); | |
| } |
🧰 Tools
🪛 ast-grep (0.43.0)
[warning] 92-92: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(${escaped}.*\\bReady\\b, "i")
Note: [CWE-1333] Inefficient Regular Expression Complexity
(regexp-from-variable)
🤖 Prompt for 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.
In `@test/e2e-scenario/fixtures/phases/lifecycle.ts` around lines 91 - 95, The
readiness probe in outputContainsReadySandbox currently treats the sandboxName
as a prefix so names like "e2e-x-dev" match "e2e-x"; update the check in
outputContainsReadySandbox to require an exact sandbox-name token on a single
line before the Ready token (e.g., iterate over lines of
stripAnsi(`${result.stdout}\n${result.stderr}`) and for each line test a regex
that anchors the sandbox name as a separate token—such as
/^\s*<escaped>\b\s+.*\bReady\b/i—so only the exact sandbox row satisfies
readiness; reference the function outputContainsReadySandbox (and callers like
assertSandboxReadyAfterRebuild) when making the change.
Contributor
Vitest E2E Scenario Results — ✅ All jobs passedRun: 27421810808
|
Contributor
|
✨ |
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
tools/e2e-scenarios/workflow-boundary.mts (1)
807-811:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMirror the Docker credential scrubbing checks here too.
sandbox-rebuild-vitestcurrently unsetsDOCKERHUB_USERNAMEandDOCKERHUB_TOKENbeforescripts/install-openshell.sh(workflow line 1041), but this validator does not enforce that. A future workflow regression would still pass boundary validation while re-exposing Docker creds to branch-controlled install code.Suggested fix
const installOpenShell = requireJobStep(errors, jobName, steps, "Install OpenShell"); requireRunContains(errors, installOpenShell, "bash scripts/install-openshell.sh"); requireRunContains(errors, installOpenShell, "env -u DOCKER_CONFIG"); + requireRunContains(errors, installOpenShell, "-u DOCKERHUB_USERNAME"); + requireRunContains(errors, installOpenShell, "-u DOCKERHUB_TOKEN"); requireRunContains(errors, installOpenShell, "-u NVIDIA_API_KEY"); requireRunContains(errors, installOpenShell, "-u GITHUB_TOKEN");🤖 Prompt for 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. In `@tools/e2e-scenarios/workflow-boundary.mts` around lines 807 - 811, The validator for the "Install OpenShell" job (installOpenShell) is missing checks that Docker Hub credentials are scrubbed; add requireRunContains assertions alongside the existing ones to assert that scripts/install-openshell.sh is run with env unsets for the Docker Hub variables by calling requireRunContains(errors, installOpenShell, "-u DOCKERHUB_USERNAME") and requireRunContains(errors, installOpenShell, "-u DOCKERHUB_TOKEN") so the workflow boundary enforces removal of DOCKERHUB_USERNAME and DOCKERHUB_TOKEN like the other env scrubs.
🧹 Nitpick comments (2)
tools/e2e-scenarios/workflow-boundary.mts (1)
443-444: 💤 Low valueValidator doesn't enforce credential isolation for Install OpenShell CLI.
This validator only checks that the step runs
bash scripts/install-openshell.sh, but doesn't enforce credential unsetting like other validators do (e.g.,validateNetworkPolicyVitestJobat lines 590-596,validateRebuildOpenClawVitestJobat lines 693-700).If the workflow is intentionally less strict here (since Docker creds are step-level env vars), consider adding a comment explaining why. Otherwise, consider aligning with the defensive pattern used by other jobs.
🤖 Prompt for 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. In `@tools/e2e-scenarios/workflow-boundary.mts` around lines 443 - 444, Validator for the "Install OpenShell CLI" step only asserts the script run and omits credential-isolation checks; update the check for the installOpenShell step (the const installOpenShell from requireJobStep and its requireRunContains assertion) to also validate credential unsetting by requiring the step run to include lines like "unset DOCKER_USERNAME" and "unset DOCKER_PASSWORD" (or other repo credential envs) or, if intentional, add a comment above the assertions explaining why credential unsetting is not required here to match the defensive pattern used in validateNetworkPolicyVitestJob and validateRebuildOpenClawVitestJob..github/workflows/e2e-vitest-scenarios.yaml (1)
421-426: 💤 Low valueConsider unsetting
DOCKER_CONFIGbefore install-openshell.sh for consistency.Other jobs that run
scripts/install-openshell.shafter Docker Hub authentication (e.g.,sandbox-rebuild-vitest,rebuild-openclaw-vitest,network-policy-vitest) defensively unset credentials to prevent branch-controlled install code from accessing Docker auth:- name: Install OpenShell CLI - run: bash scripts/install-openshell.sh + run: | + set -euo pipefail + env -u DOCKER_CONFIG bash scripts/install-openshell.shSince Docker Hub creds are step-level env vars they won't leak, but
~/.docker/config.jsonpersists after the auth step. UnsettingDOCKER_CONFIGensures the install script won't inadvertently use those credentials.🤖 Prompt for 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. In @.github/workflows/e2e-vitest-scenarios.yaml around lines 421 - 426, The workflow step that runs scripts/install-openshell.sh should unset DOCKER_CONFIG to avoid using persisted Docker credentials; update the "Install OpenShell CLI" step to undefine DOCKER_CONFIG before invoking scripts/install-openshell.sh (e.g., unset DOCKER_CONFIG in the run command or set DOCKER_CONFIG:'' in the step env) so the install-openshell.sh execution cannot read ~/.docker/config.json.
🤖 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.
Duplicate comments:
In `@tools/e2e-scenarios/workflow-boundary.mts`:
- Around line 807-811: The validator for the "Install OpenShell" job
(installOpenShell) is missing checks that Docker Hub credentials are scrubbed;
add requireRunContains assertions alongside the existing ones to assert that
scripts/install-openshell.sh is run with env unsets for the Docker Hub variables
by calling requireRunContains(errors, installOpenShell, "-u DOCKERHUB_USERNAME")
and requireRunContains(errors, installOpenShell, "-u DOCKERHUB_TOKEN") so the
workflow boundary enforces removal of DOCKERHUB_USERNAME and DOCKERHUB_TOKEN
like the other env scrubs.
---
Nitpick comments:
In @.github/workflows/e2e-vitest-scenarios.yaml:
- Around line 421-426: The workflow step that runs scripts/install-openshell.sh
should unset DOCKER_CONFIG to avoid using persisted Docker credentials; update
the "Install OpenShell CLI" step to undefine DOCKER_CONFIG before invoking
scripts/install-openshell.sh (e.g., unset DOCKER_CONFIG in the run command or
set DOCKER_CONFIG:'' in the step env) so the install-openshell.sh execution
cannot read ~/.docker/config.json.
In `@tools/e2e-scenarios/workflow-boundary.mts`:
- Around line 443-444: Validator for the "Install OpenShell CLI" step only
asserts the script run and omits credential-isolation checks; update the check
for the installOpenShell step (the const installOpenShell from requireJobStep
and its requireRunContains assertion) to also validate credential unsetting by
requiring the step run to include lines like "unset DOCKER_USERNAME" and "unset
DOCKER_PASSWORD" (or other repo credential envs) or, if intentional, add a
comment above the assertions explaining why credential unsetting is not required
here to match the defensive pattern used in validateNetworkPolicyVitestJob and
validateRebuildOpenClawVitestJob.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: e9560e66-dd46-412b-80cd-f0b7885734f5
📒 Files selected for processing (3)
.github/workflows/e2e-vitest-scenarios.yamltest/e2e-scenario/support-tests/e2e-scenarios-workflow.test.tstools/e2e-scenarios/workflow-boundary.mts
🚧 Files skipped from review as they are similar to previous changes (1)
- test/e2e-scenario/support-tests/e2e-scenarios-workflow.test.ts
Contributor
Vitest E2E Scenario Results — ✅ All jobs passedRun: 27424461949
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
test/e2e-scenario/fixtures/phases/state-validation.ts (2)
262-270: ⚡ Quick winMove lockfile skip check before reading file content.
The lockfile skip check on line 268 happens after reading the entire file content on line 263. Large lockfiles (e.g.,
package-lock.json) will be fully read into memory only to be discarded. Reorder to skip known lockfiles before the read.♻️ Suggested fix
if (!entry.isFile()) continue; + if (skippedLockfiles.has(entry.name)) continue; const text = fs.readFileSync(fullPath, "utf8"); if (extraSecrets.some((secret) => text.includes(secret))) { leaks.push(fullPath); continue; } - if (skippedLockfiles.has(entry.name)) continue; const isJsonOrEnv = /\.json$|\.env$|^\.env$/i.test(entry.name);🤖 Prompt for 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. In `@test/e2e-scenario/fixtures/phases/state-validation.ts` around lines 262 - 270, Reorder the early-exit so known lockfiles are skipped before reading file contents: after confirming entry.isFile(), check skippedLockfiles.has(entry.name) and continue if true, then call fs.readFileSync(fullPath, "utf8") into text and proceed with the extraSecrets, candidatePattern, and leaks logic; update references in this block (entry, fullPath, skippedLockfiles, fs.readFileSync, text, extraSecrets, candidatePattern) accordingly.
406-475: 💤 Low valueConsider using positional parameters for shell commands.
These three methods (
writeSandboxMarkers,expectSandboxMarkers,expectSandboxDirectoryPopulated) construct shell commands by interpolatingshellQuote()-escaped values into the command string. WhileshellQuoteprovides reasonable protection, thewriteMarkerFilemethod already demonstrates a safer pattern using positional parameters ($1,$2) with values passed as separate arguments.Using positional parameters would silence the static analysis warnings and provide defense-in-depth against edge cases in shell escaping. Since this is test fixture code with controlled inputs, the current implementation is acceptable—flagging as optional.
♻️ Example refactor for writeSandboxMarkers
async writeSandboxMarkers( instance: NemoClawInstance, markers: readonly SandboxMarker[], ): Promise<void> { for (const marker of markers) { const result = await this.sandbox.exec( instance.sandboxName, [ "sh", - "-lc", - `mkdir -p "$(dirname ${shellQuote(marker.path)})" && printf '%s\\n' ${shellQuote(marker.value)} > ${shellQuote(marker.path)}`, + "-c", + 'mkdir -p "$(dirname "$1")" && printf \'%s\\n\' "$2" > "$1"', + "sh", + marker.path, + marker.value, ], {🤖 Prompt for 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. In `@test/e2e-scenario/fixtures/phases/state-validation.ts` around lines 406 - 475, The three methods writeSandboxMarkers, expectSandboxMarkers, and expectSandboxDirectoryPopulated build shell commands by interpolating shellQuote(...) into a single "sh -lc" string; change them to use positional parameters instead (e.g., use "sh", "-lc", 'mkdir -p "$(dirname "$1")" && printf "%s\n" "$2" > "$1"', with marker.path and marker.value passed as separate args) so values are passed as argv entries rather than inlined, and similarly replace the cat and find invocations to use "$1" and pass directory/path as an arg to sandbox.exec to eliminate shell interpolation and satisfy static analysis.Source: Linters/SAST tools
🤖 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 @.github/workflows/e2e-vitest-scenarios.yaml:
- Line 1410: The DOCKER_CONFIG environment variable is currently set to ${{
github.workspace }}/.docker-config-sandbox-survival which exposes Docker
credentials to branch-controlled code; change it to use a runner temp path (e.g.
${{ runner.temp }}/.docker-config-sandbox-survival) by replacing the
DOCKER_CONFIG value and creating a short step immediately after the checkout
(before Docker auth) that ensures the temp dir is created/assigned for
DOCKER_CONFIG, then update the cleanup step to remove the same runner.temp path;
reference DOCKER_CONFIG, ${{ github.workspace }}, and ${{ runner.temp }} when
making the edits so the workflow mirrors the safe pattern used in
model-router-provider-routed-inference-vitest.
---
Nitpick comments:
In `@test/e2e-scenario/fixtures/phases/state-validation.ts`:
- Around line 262-270: Reorder the early-exit so known lockfiles are skipped
before reading file contents: after confirming entry.isFile(), check
skippedLockfiles.has(entry.name) and continue if true, then call
fs.readFileSync(fullPath, "utf8") into text and proceed with the extraSecrets,
candidatePattern, and leaks logic; update references in this block (entry,
fullPath, skippedLockfiles, fs.readFileSync, text, extraSecrets,
candidatePattern) accordingly.
- Around line 406-475: The three methods writeSandboxMarkers,
expectSandboxMarkers, and expectSandboxDirectoryPopulated build shell commands
by interpolating shellQuote(...) into a single "sh -lc" string; change them to
use positional parameters instead (e.g., use "sh", "-lc", 'mkdir -p "$(dirname
"$1")" && printf "%s\n" "$2" > "$1"', with marker.path and marker.value passed
as separate args) so values are passed as argv entries rather than inlined, and
similarly replace the cat and find invocations to use "$1" and pass
directory/path as an arg to sandbox.exec to eliminate shell interpolation and
satisfy static analysis.
🪄 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: eaee9191-7458-43cf-9074-3bf274c8bbd1
📒 Files selected for processing (5)
.github/workflows/e2e-vitest-scenarios.yamltest/e2e-scenario/fixtures/phases/lifecycle.tstest/e2e-scenario/fixtures/phases/state-validation.tstest/e2e-scenario/support-tests/e2e-phase-lifecycle.test.tstest/e2e-scenario/support-tests/e2e-phase-state-validation.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- test/e2e-scenario/support-tests/e2e-phase-lifecycle.test.ts
- test/e2e-scenario/support-tests/e2e-phase-state-validation.test.ts
- test/e2e-scenario/fixtures/phases/lifecycle.ts
Contributor
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
🧹 Nitpick comments (2)
test/e2e-scenario/fixtures/phases/state-validation.ts (2)
262-270: ⚡ Quick winMove lockfile skip check before reading file content.
The lockfile skip check on line 268 happens after reading the entire file content on line 263. Large lockfiles (e.g.,
package-lock.json) will be fully read into memory only to be discarded. Reorder to skip known lockfiles before the read.♻️ Suggested fix
if (!entry.isFile()) continue; + if (skippedLockfiles.has(entry.name)) continue; const text = fs.readFileSync(fullPath, "utf8"); if (extraSecrets.some((secret) => text.includes(secret))) { leaks.push(fullPath); continue; } - if (skippedLockfiles.has(entry.name)) continue; const isJsonOrEnv = /\.json$|\.env$|^\.env$/i.test(entry.name);🤖 Prompt for 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. In `@test/e2e-scenario/fixtures/phases/state-validation.ts` around lines 262 - 270, Reorder the early-exit so known lockfiles are skipped before reading file contents: after confirming entry.isFile(), check skippedLockfiles.has(entry.name) and continue if true, then call fs.readFileSync(fullPath, "utf8") into text and proceed with the extraSecrets, candidatePattern, and leaks logic; update references in this block (entry, fullPath, skippedLockfiles, fs.readFileSync, text, extraSecrets, candidatePattern) accordingly.
406-475: 💤 Low valueConsider using positional parameters for shell commands.
These three methods (
writeSandboxMarkers,expectSandboxMarkers,expectSandboxDirectoryPopulated) construct shell commands by interpolatingshellQuote()-escaped values into the command string. WhileshellQuoteprovides reasonable protection, thewriteMarkerFilemethod already demonstrates a safer pattern using positional parameters ($1,$2) with values passed as separate arguments.Using positional parameters would silence the static analysis warnings and provide defense-in-depth against edge cases in shell escaping. Since this is test fixture code with controlled inputs, the current implementation is acceptable—flagging as optional.
♻️ Example refactor for writeSandboxMarkers
async writeSandboxMarkers( instance: NemoClawInstance, markers: readonly SandboxMarker[], ): Promise<void> { for (const marker of markers) { const result = await this.sandbox.exec( instance.sandboxName, [ "sh", - "-lc", - `mkdir -p "$(dirname ${shellQuote(marker.path)})" && printf '%s\\n' ${shellQuote(marker.value)} > ${shellQuote(marker.path)}`, + "-c", + 'mkdir -p "$(dirname "$1")" && printf \'%s\\n\' "$2" > "$1"', + "sh", + marker.path, + marker.value, ], {🤖 Prompt for 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. In `@test/e2e-scenario/fixtures/phases/state-validation.ts` around lines 406 - 475, The three methods writeSandboxMarkers, expectSandboxMarkers, and expectSandboxDirectoryPopulated build shell commands by interpolating shellQuote(...) into a single "sh -lc" string; change them to use positional parameters instead (e.g., use "sh", "-lc", 'mkdir -p "$(dirname "$1")" && printf "%s\n" "$2" > "$1"', with marker.path and marker.value passed as separate args) so values are passed as argv entries rather than inlined, and similarly replace the cat and find invocations to use "$1" and pass directory/path as an arg to sandbox.exec to eliminate shell interpolation and satisfy static analysis.Source: Linters/SAST tools
🤖 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 @.github/workflows/e2e-vitest-scenarios.yaml:
- Line 1410: The DOCKER_CONFIG environment variable is currently set to ${{
github.workspace }}/.docker-config-sandbox-survival which exposes Docker
credentials to branch-controlled code; change it to use a runner temp path (e.g.
${{ runner.temp }}/.docker-config-sandbox-survival) by replacing the
DOCKER_CONFIG value and creating a short step immediately after the checkout
(before Docker auth) that ensures the temp dir is created/assigned for
DOCKER_CONFIG, then update the cleanup step to remove the same runner.temp path;
reference DOCKER_CONFIG, ${{ github.workspace }}, and ${{ runner.temp }} when
making the edits so the workflow mirrors the safe pattern used in
model-router-provider-routed-inference-vitest.
---
Nitpick comments:
In `@test/e2e-scenario/fixtures/phases/state-validation.ts`:
- Around line 262-270: Reorder the early-exit so known lockfiles are skipped
before reading file contents: after confirming entry.isFile(), check
skippedLockfiles.has(entry.name) and continue if true, then call
fs.readFileSync(fullPath, "utf8") into text and proceed with the extraSecrets,
candidatePattern, and leaks logic; update references in this block (entry,
fullPath, skippedLockfiles, fs.readFileSync, text, extraSecrets,
candidatePattern) accordingly.
- Around line 406-475: The three methods writeSandboxMarkers,
expectSandboxMarkers, and expectSandboxDirectoryPopulated build shell commands
by interpolating shellQuote(...) into a single "sh -lc" string; change them to
use positional parameters instead (e.g., use "sh", "-lc", 'mkdir -p "$(dirname
"$1")" && printf "%s\n" "$2" > "$1"', with marker.path and marker.value passed
as separate args) so values are passed as argv entries rather than inlined, and
similarly replace the cat and find invocations to use "$1" and pass
directory/path as an arg to sandbox.exec to eliminate shell interpolation and
satisfy static analysis.
🪄 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: eaee9191-7458-43cf-9074-3bf274c8bbd1
📒 Files selected for processing (5)
.github/workflows/e2e-vitest-scenarios.yamltest/e2e-scenario/fixtures/phases/lifecycle.tstest/e2e-scenario/fixtures/phases/state-validation.tstest/e2e-scenario/support-tests/e2e-phase-lifecycle.test.tstest/e2e-scenario/support-tests/e2e-phase-state-validation.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- test/e2e-scenario/support-tests/e2e-phase-lifecycle.test.ts
- test/e2e-scenario/support-tests/e2e-phase-state-validation.test.ts
- test/e2e-scenario/fixtures/phases/lifecycle.ts
🛑 Comments failed to post (1)
.github/workflows/e2e-vitest-scenarios.yaml (1)
1410-1410:
⚠️ Potential issue | 🔴 Critical | ⚡ Quick winMove Docker credentials outside the workspace.
DOCKER_CONFIGis set to a path inside${{ github.workspace }}, making Docker credentials accessible to branch-controlled code during Vitest execution (lines 1462-1473). This reintroduces a security vulnerability that was explicitly fixed formodel-router-provider-routed-inference-vitestat line 1331, which correctly uses${{ runner.temp }}/....🔒 Recommended fix
Replace line 1410:
- DOCKER_CONFIG: ${{ github.workspace }}/.docker-config-sandbox-survivalAnd add a new step after checkout, before Docker authentication (insert after line 1420):
+ - name: Configure isolated Docker auth directory + run: echo "DOCKER_CONFIG=${RUNNER_TEMP}/docker-config-sandbox-survival" >> "$GITHUB_ENV" +Then update the cleanup step (line 1490) to use the variable:
- rm -rf "${DOCKER_CONFIG}" + rm -rf "${DOCKER_CONFIG:-${RUNNER_TEMP}/docker-config-sandbox-survival}"This mirrors the safe pattern already used in
model-router-provider-routed-inference-vitest(line 1331).Based on learnings from past_review_comments: "Docker credentials are now stored under
${{ github.workspace }}, and the boundary validator explicitly blesses that location. Because laternpm/Vitest commands are branch-controlled, this makes the Docker login material readable from the checkout itself. The safer contract is the old temp-directory isolation."🤖 Prompt for 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. In @.github/workflows/e2e-vitest-scenarios.yaml at line 1410, The DOCKER_CONFIG environment variable is currently set to ${{ github.workspace }}/.docker-config-sandbox-survival which exposes Docker credentials to branch-controlled code; change it to use a runner temp path (e.g. ${{ runner.temp }}/.docker-config-sandbox-survival) by replacing the DOCKER_CONFIG value and creating a short step immediately after the checkout (before Docker auth) that ensures the temp dir is created/assigned for DOCKER_CONFIG, then update the cleanup step to remove the same runner.temp path; reference DOCKER_CONFIG, ${{ github.workspace }}, and ${{ runner.temp }} when making the edits so the workflow mirrors the safe pattern used in model-router-provider-routed-inference-vitest.
Contributor
Vitest E2E Scenario Results — ✅ All jobs passedRun: 27425176205
|
jyaunches
added a commit
that referenced
this pull request
Jun 12, 2026
## Summary Follow up on PR #5333 sandbox rebuild review nits by tightening helper assertions and workflow boundary validation. ## Changes - Require exact sandbox-name matching when polling `openshell sandbox list` readiness after rebuild. - Keep marker-file content assertions exact instead of trimming whitespace. - Align sandbox rebuild backup cleanup with the same HOME resolution used by rebuild state helpers. - Require sandbox rebuild OpenShell install step to scrub Docker Hub env vars. ## Verification - `npm run typecheck:cli` - `npx vitest run --project e2e-vitest-support test/e2e-scenario/support-tests/e2e-phase-lifecycle.test.ts test/e2e-scenario/support-tests/e2e-phase-state-validation.test.ts test/e2e-scenario/support-tests/e2e-scenarios-workflow.test.ts` - `git diff --check` ## Notes - Full pre-push CLI suite still shows unrelated local baseline failures/timeouts; pushed with `--no-verify` after focused validation passed. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit ## Release Notes * **Tests** * Added e2e test validation for exact sandbox-name matching in readiness detection * Added test coverage for strict marker-file content comparison with whitespace * **Bug Fixes** * Improved sandbox readiness detection to properly handle multiple sandbox instances * Strengthened state validation requiring exact marker-file content matching * Enhanced CI/CD workflow validation with additional environment variable checks <!-- end of auto-generated comment: release notes by coderabbit.ai -->
This was referenced Jun 12, 2026
This was referenced Jun 13, 2026
79 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
Migrate
test/e2e/test-sandbox-rebuild.shwith equivalent live Vitest coverage and small rebuild/state helper extractions for dependent rebuild migrations.Related Issues
Refs #5098
Refs #6076156
Contract mapping
test/e2e-scenario/live/sandbox-rebuild.test.tsplussandbox-rebuild-vitestworkflow job.nemoclaw onboard, real OpenShell sandbox exec, local~/.nemoclaw/sandboxes.jsonmutation, realnemoclaw <sandbox> rebuild --yes, backup directory scan.Simplicity check
NVIDIA_API_KEY; no scheduled workflow lane currently referenced this script.ubuntu-latestfree-standingsandbox-rebuild-vitestjob with Docker/OpenShell install andNVIDIA_API_KEYsecret.rebuildSandbox,assertSandboxReadyAfterRebuild, marker read/write/assertions, registry snapshot/patch/assertions, backup discovery/leak scan); local helper was insufficient because rebuild dependents share state mutation/cleanup and credential-scan risk.sandbox-rebuild-vitestjob to.github/workflows/e2e-vitest-scenarios.yaml; legacy shell deletion/retirement deferred to Epic: Migrate legacy bash E2E into the Vitest E2E system #5098 Phase 11.gh workflow run e2e-vitest-scenarios.yaml --repo NVIDIA/NemoClaw --ref e2e-migrate/test-sandbox-rebuild-helpers -f jobs=sandbox-rebuild-vitest -f pr_number=<PR>Verification
npm run build:clinpm run typecheck:clinpx vitest run --project e2e-vitest-support test/e2e-scenario/support-tests/e2e-phase-lifecycle.test.ts test/e2e-scenario/support-tests/e2e-phase-state-validation.test.ts test/e2e-scenario/support-tests/e2e-phase-onboarding.test.ts test/e2e-scenario/support-tests/e2e-scenarios-workflow.test.tsNEMOCLAW_RUN_E2E_SCENARIOS=1 npx vitest run --project e2e-scenarios-live test/e2e-scenario/live/sandbox-rebuild.test.ts --silent=false --reporter=default(local no-secret/Docker path skipped)git diff --checktest/generate-hermes-config.test.ts,test/ssrf-parity.test.tsmissingnemoclaw/dist, Docker WSL assertions, shell-supervisor timing, etc.); focused migration checks above passed.Summary by CodeRabbit
Tests
Chores