ci(wsl): bootstrap trusted workflow helper - #7719
Merged
Merged
Conversation
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Contributor
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in commit 92507d6 in the TypeScript / code-coverage/cliThe overall coverage in commit 92507d6 in the Updated |
cv
added
area: ci
Contributor
📝 WalkthroughWalkthroughAdds a trusted PowerShell WSL helper for execution, distro initialization, provisioning, workflow path setup, and ext4 checkout synchronization. Migrates WSL workflows to the helper and adds tests for helper behavior, trusted loading, workflow boundaries, and environment-based ChangesTrusted WSL CI orchestration
Environment-based needs handling
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Workflow
participant TrustedHelper
participant RunnerTemp
participant WSL
participant BashScript
Workflow->>TrustedHelper: Load trusted helper
TrustedHelper->>WSL: Ensure distro and provision environment
TrustedHelper->>RunnerTemp: Write temporary Bash script
TrustedHelper->>WSL: Sync checkout and invoke script
WSL->>BashScript: Execute workflow commands
BashScript-->>WSL: Return output and exit code
TrustedHelper->>RunnerTemp: Delete temporary script
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
23 tasks
Contributor
PR Review Advisor — Blocking findings reportedAdvisor assessment: Blockers require maintainer review Model lanes
Nemotron output stays in workflow artifacts and does not change the assessment above. E2E guidanceAdvisory only. E2E / PR Gate selects and runs jobs independently. Recommended E2E: None Blockers
|
![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: 1
🧹 Nitpick comments (1)
tools/wsl/ci-helper.ps1 (1)
91-102: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winCaptured output always merges stderr, unlike
Invoke-WslNative's opt-in switch.
Invoke-WslNativeOutputunconditionally does2>&1, so any stderr text (or the caller's own error records) gets interleaved intoOutputand later joined into the returned string inInvoke-WslScript(Line 139).Invoke-WslNativeinstead makes this opt-in via-MergeError. If a future caller uses-CaptureOutputto parse structured output (e.g.git status --shortfor the ext4 sync layer), stray stderr lines could corrupt the parsed result.♻️ Proposed fix: make error-stream merging opt-in for consistency
function Invoke-WslNativeOutput { param( [Parameter(Mandatory = $true)] - [string[]]$ArgumentList + [string[]]$ArgumentList, + + [switch]$MergeError ) - $output = @(& wsl `@ArgumentList` 2>&1) + $output = if ($MergeError) { @(& wsl `@ArgumentList` 2>&1) } else { @(& wsl `@ArgumentList`) } return [pscustomobject]@{ ExitCode = $LASTEXITCODE Output = $output } }🤖 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/wsl/ci-helper.ps1` around lines 91 - 102, Update Invoke-WslNativeOutput to make stderr merging opt-in, matching Invoke-WslNative’s MergeError behavior, while preserving normal stdout capture and the returned ExitCode/Output structure. Ensure callers only receive stderr entries in Output when the new or existing merge switch is explicitly enabled.
🤖 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 `@tools/wsl/ci-helper.ps1`:
- Around line 350-399: Validate $Workdir before deriving $workdirParent or
generating the destructive command: reject empty/root paths, and reject paths
equal to or containing $Checkout such that $Workdir is the checkout or an
ancestor of it. Normalize both paths consistently for comparison, throw a clear
error on invalid overlap, and preserve the existing sync behavior only for safe,
non-overlapping workspaces in Get-WslCheckoutSyncScript.
---
Nitpick comments:
In `@tools/wsl/ci-helper.ps1`:
- Around line 91-102: Update Invoke-WslNativeOutput to make stderr merging
opt-in, matching Invoke-WslNative’s MergeError behavior, while preserving normal
stdout capture and the returned ExitCode/Output structure. Ensure callers only
receive stderr entries in Output when the new or existing merge switch is
explicitly enabled.
🪄 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: 838d510c-c4ee-4879-a228-f597bc742c9c
📒 Files selected for processing (2)
test/wsl-ci-helper.test.tstools/wsl/ci-helper.ps1
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/wsl-ci-helper.test.ts (1)
117-128: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winRestore the PowerShell stub after this test.
Invoke-WslScriptis replaced but never restored or removed. IfrunPowerShellBatchreuses a PowerShell session, later cases become order-dependent and may silently call this no-op stub. Wrap the calls intry/finallyand restore the original function (or remove the stub); also confirm the batch runner’s isolation behavior.As per coding guidelines, deterministic tests must undo global stubs. Based on learnings, locally created mocks should be explicitly restored in a
finallyblock.Suggested cleanup
+$originalInvokeWslScript = + (Get-Command Invoke-WslScript -CommandType Function).ScriptBlock +try { function Invoke-WslScript { param( [string]$Distro, @@ Install-WslUbuntuDependencies -Distro Ubuntu -Packages @('curl') Install-WslUbuntuDependencies -Distro Ubuntu -Packages @('curl') -TestUser nemoclaw-ci +} finally { + Set-Item Function:\Invoke-WslScript -Value $originalInvokeWslScript +}🤖 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/wsl-ci-helper.test.ts` around lines 117 - 128, Update the test setup around the Invoke-WslScript stub to scope its use with try/finally, and in the finally block restore the original function or remove the locally created stub. Ensure runPowerShellBatch completes before cleanup and verify the cleanup preserves isolation for subsequent test cases.Sources: Coding guidelines, Learnings
🤖 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.
Nitpick comments:
In `@test/wsl-ci-helper.test.ts`:
- Around line 117-128: Update the test setup around the Invoke-WslScript stub to
scope its use with try/finally, and in the finally block restore the original
function or remove the locally created stub. Ensure runPowerShellBatch completes
before cleanup and verify the cleanup preserves isolation for subsequent test
cases.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 4b6b611d-0bad-4acd-a3a1-2a362442358e
📒 Files selected for processing (2)
test/wsl-ci-helper.test.tstools/wsl/ci-helper.ps1
🚧 Files skipped from review as they are similar to previous changes (1)
- tools/wsl/ci-helper.ps1
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
<!-- markdownlint-disable MD041 --> ## Summary This stacked PR completes #6952 by extracting the remaining repeated Windows/WSL setup and command construction. It uses the trusted PowerShell helper bootstrapped by #7719, switches both workflow consumers to that trusted boundary, and fails closed if the helper is absent. It also passes the `needs` context through environment data instead of interpolating it into GitHub Script source. ## Related Issue Fixes #6952 Fixes #6958 ## Changes - Use `tools/wsl/ci-helper.ps1`, bootstrapped by #7719, for the WSL E2E and platform Vitest workflows. Both consumers require the same distro setup, script transfer, Node.js installation, and ext4 checkout synchronization. Keeping this logic inline duplicated the security-sensitive command boundary; the helper and workflow regression tests protect the shared behavior and connection. - Load the helper through mandatory sparse checkouts at the PR base SHA or workflow SHA before the candidate checkout. Both workflows fail closed if the trusted helper is missing; the workflow boundary tests protect the trusted-source requirement. - Pass `needs` JSON through `NEEDS_JSON` and parse it inside GitHub Script. The E2E operations boundary rejects direct expression interpolation. - Reduce nonblank executable lines from 191 to 80 in `wsl-e2e.yaml` and from 228 to 129 in `platform-vitest-main.yaml`. Keep the root-only test selection inline with an adjacent rationale. ## Type of Change - [x] 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 - [x] Tests added or updated for changed behavior - [ ] Existing tests cover changed behavior — justification: - [ ] Tests not applicable — justification: - [ ] Docs updated for user-facing behavior changes - [x] Docs not applicable — justification: This changes internal CI workflows and their tests. It does not change a CLI, configuration, supported integration, or end-user contract. - [x] Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging) - [x] Sensitive-path review completed or maintainer-approved waiver recorded — reviewer/approval link/justification: At stacked PR SHA `d6662d367`, targeted boundary review and tests verify trusted helper provenance, fail-closed loading, bidirectional checkout/workdir overlap rejection, command quoting, script encoding, credential-data interpolation, and root/non-root separation. - [ ] Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue: ## Documentation Writer Review - [x] Documentation writer subagent reviewed the completed changes - Result: `no-docs-needed` - Evidence: At exact head `d6662d367`, the exact-base diff against helper head `3a5ebeb2f` contains the expected nine internal workflow, validator, trigger, and regression-test files. The reviewed overlap guard is present in both head and base, so helper files do not appear in this PR diff. No documentation sources changed; focused tests, CLI type-checking, and the exact-base diff check passed. - Agent: Codex Desktop <!-- docs-review-head-sha: d6662d3 --> <!-- docs-review-agents-blob-sha: be20a09 --> ## DGX Station Hardware Evidence - [ ] Tested on DGX Station - Tested commit: - Station profile/scenario: - Result: - Supporting evidence: ## Verification - [x] PR description includes a `Signed-off-by:` line and every commit appears as `Verified` in GitHub - [x] Normal `pre-commit`, `commit-msg`, and `pre-push` hooks passed, or `npm run check:diff` passed when hooks were skipped or unavailable - [x] Targeted behavior tests pass for the current change set, or tests are marked not applicable above — command/result or justification: The combined focused run passed 50 tests across the platform workflow, watch-trigger, and E2E boundary suites. It also discovered twelve PowerShell helper tests and skipped them locally because PowerShell is unavailable. `npm run typecheck:cli`, the exact-base diff check, normal commit hooks, and the pre-push TypeScript check passed. - [ ] Applicable broad gate passed — `npm test` for broad runtime/test-harness changes; `npm run check` for repo-wide validation/coverage changes — command/result: - [x] Quality Gates section completed with required justifications or waivers - [x] No secrets, API keys, or credentials committed - [ ] `npm run docs` builds without warnings (doc changes only) - [ ] Doc pages follow the [style guide](https://github.com/NVIDIA/NemoClaw/blob/main/docs/CONTRIBUTING.md) (doc changes only) - [ ] New doc pages include SPDX header and frontmatter (new pages only) --- Signed-off-by: Carlos Villela <cvillela@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved reliability and orchestration of WSL-based test runs with clearer conditional execution and safer setup/sync. * **Security** * Strengthened workflow-boundary enforcement so `needs` is provided via `env` and parsed as JSON (no inline interpolation). * Routed WSL provisioning/execution through a trusted helper path. * **Tests** * Expanded CI, boundary, and trusted-helper suite coverage for script generation, path handling, and failure propagation. * Updated workflow trigger mappings to include the trusted helper. * **Chores** * Consolidated WSL workflow logic into the shared trusted helper flow. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
19 tasks
<!-- markdownlint-disable MD041 --> ## Summary This replacement stacked PR completes #6952 by extracting the remaining repeated Windows and WSL setup and command construction. It uses the trusted helper from #7719 and restores the reviewed #7709 diff after #7709 merged into its staging branch. ## Related Issue Fixes #6952 Fixes #6958 Replaces staged PR #7709 Depends on #7719 ## Changes - Use `tools/wsl/ci-helper.ps1`, bootstrapped by #7719, for WSL E2E and platform Vitest workflows. - Load the helper through mandatory sparse checkouts at the PR base SHA or workflow SHA before candidate checkout, and fail closed. - Pass `needs` through `NEEDS_JSON` and parse it instead of interpolating expressions into executable code. - Reduce inline executable workflow logic while retaining root-only selection and its reason. - Reapply the exact reviewed #7709 patch after recovering #7719's helper-only scope. The binary patch is identical to `df3bcdcdd`. ## Type of Change - [x] Code - [ ] Documentation - [ ] Tests only - [ ] Other ## Quality Gates - [x] Tests added or updated for changed behavior - [x] Documentation is not applicable because this changes internal CI only - [x] Sensitive paths changed - [x] Sensitive-path review completed: the exact `c1f34675d1025b7f189938977cf6158efdc66b4c` patch is byte-for-byte identical to reviewed #7709 commit `df3bcdcdd`; targeted boundary review covers trusted provenance, fail-closed loading, bidirectional overlap, quoting, encoding, `needs` interpolation, and root/non-root separation - [ ] Non-success behavior changed ## Documentation Writer Review - [x] Documentation writer review completed Result: no-docs-needed Evidence: Reviewed exact head `c1f34675d1025b7f189938977cf6158efdc66b4c` against base `e69d9c3d94623a0cd499fb2c84f33f88c863c631`. The expected nine files match the binary patch SHA-256 for reviewed #7709 commit `df3bcdcdd`; no user-facing documentation changes are required. Focused tests passed 50 cases with 12 PowerShell cases skipped because PowerShell is unavailable locally. CLI type-check and exact-base diff checks passed. Agent surface: Codex Desktop <!-- docs-review-head-sha: c1f3467 --> <!-- docs-review-agents-blob-sha: be20a09 --> ## DGX Station Test Evidence Not applicable. ## Verification - [x] DCO declaration is included and every commit is GitHub Verified - [x] Normal commit and pre-push hooks passed - [x] Targeted tests passed: 50 passed; 12 PowerShell cases skipped locally because PowerShell is unavailable - [x] CLI type-check and exact-base diff checks passed - [ ] Broad test suite passed - [x] Quality gates above are complete - [x] No secrets, API keys, or credentials are committed - [ ] User-facing documentation was updated - [ ] Documentation build passed Signed-off-by: Carlos Villela <cvillela@nvidia.com> Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
test/e2e/support/e2e-operations-workflow-boundary.test.ts (1)
68-117: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winMake the negative tests isolate the interpolation predicate.
The first case deletes
NEEDS_JSONat Line [76]-[77], so it would fail even ifNEEDS_INTERPOLATIONwere removed. The second case tests only thetoJSON(needs)form. Keep the environment binding intact while injecting interpolation, and add a direct${{ needs.* }}regression case.Proposed test adjustment
- delete report.env?.NEEDS_JSON; - delete scorecard.env?.NEEDS_JSON; report.with!.script = String(report.with!.script).replace( "JSON.parse(process.env.NEEDS_JSON || '{}')", "${{ toJSON(needs) }}", ); @@ scorecard.with!.script = `${String(scorecard.with!.script)} const interpolatedNeeds = \${{ toJSON ( needs ) }}; +const directNeeds = \${{ needs.live.result }}; `;As per path instructions, tests should prove the boundary behavior rather than only one implementation spelling.
🤖 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/support/e2e-operations-workflow-boundary.test.ts` around lines 68 - 117, Update the negative tests in the interpolation validation cases so they preserve the existing NEEDS_JSON environment bindings and isolate only the interpolation predicate. Replace the first test’s deletion of report.env.NEEDS_JSON and scorecard.env.NEEDS_JSON with injected interpolation, and extend the second test to cover direct `${{ needs.* }}` interpolation in addition to whitespace-obscured `toJSON(needs)`.Source: Path instructions
🤖 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/wsl-e2e.yaml:
- Around line 153-170: Update the WSL invocation around Invoke-WslScript so
NVIDIA_INFERENCE_API_KEY and GITHUB_TOKEN are passed through the WSL environment
via WSLENV rather than embedded in the $exports script content. Remove those
secrets from the generated script and export/read them directly inside WSL,
while preserving the existing non-secret environment exports and test command.
In `@test/platform-vitest-main-workflow.test.ts`:
- Around line 130-133: Extend the forbidden-pattern assertions in the main and
WSL E2E workflow checks to also reject direct `wsl -d` invocations, including
the PowerShell environment-variable form used by the migrated command tail. Keep
the existing `WriteAllText`, `wslpath`, and `wsl --install` protections
unchanged, ensuring both `readRepoText` checks enforce that only the trusted
helper constructs WSL commands.
In `@tools/e2e/operations-workflow-boundary.mts`:
- Around line 51-53: Broaden NEEDS_INTERPOLATION and the
passesNeedsAsEnvironmentData check in tools/e2e/operations-workflow-boundary.mts
(lines 51-53 and 141-149) to reject direct needs.* interpolations as well as
toJSON(needs), while preserving the NEEDS_JSON environment binding. Add a
regression case in test/e2e/support/e2e-operations-workflow-boundary.test.ts
(lines 68-117) covering a direct needs.job.outputs.foo reference.
---
Nitpick comments:
In `@test/e2e/support/e2e-operations-workflow-boundary.test.ts`:
- Around line 68-117: Update the negative tests in the interpolation validation
cases so they preserve the existing NEEDS_JSON environment bindings and isolate
only the interpolation predicate. Replace the first test’s deletion of
report.env.NEEDS_JSON and scorecard.env.NEEDS_JSON with injected interpolation,
and extend the second test to cover direct `${{ needs.* }}` interpolation in
addition to whitespace-obscured `toJSON(needs)`.
🪄 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: 589a00cd-6664-45fd-9559-b860cb7a5dc4
📒 Files selected for processing (9)
.github/workflows/e2e.yaml.github/workflows/platform-vitest-main.yaml.github/workflows/wsl-e2e.yamlci/source-shape-test-budget.jsontest/e2e/support/e2e-operations-workflow-boundary.test.tstest/helpers/vitest-watch-triggers.tstest/platform-vitest-main-workflow.test.tstest/vitest-watch-triggers.test.tstools/e2e/operations-workflow-boundary.mts
Comment on lines
+153
to
+170
| . "$env:TRUSTED_WSL_HELPER" | ||
| $workdir = ConvertTo-BashLiteral -Value $env:WSL_WORKDIR | ||
| $exports = @( | ||
| 'export NVIDIA_INFERENCE_API_KEY=' + (ConvertTo-BashLiteral -Value ([string]$env:NVIDIA_INFERENCE_API_KEY)) | ||
| 'export GITHUB_TOKEN=' + (ConvertTo-BashLiteral -Value ([string]$env:GITHUB_TOKEN)) | ||
| 'export NEMOCLAW_NON_INTERACTIVE=' + (ConvertTo-BashLiteral -Value $env:NEMOCLAW_NON_INTERACTIVE) | ||
| 'export NEMOCLAW_ACCEPT_THIRD_PARTY_SOFTWARE=' + (ConvertTo-BashLiteral -Value $env:NEMOCLAW_ACCEPT_THIRD_PARTY_SOFTWARE) | ||
| 'export NEMOCLAW_RECREATE_SANDBOX=' + (ConvertTo-BashLiteral -Value $env:NEMOCLAW_RECREATE_SANDBOX) | ||
| 'export NEMOCLAW_SANDBOX_NAME=' + (ConvertTo-BashLiteral -Value $env:NEMOCLAW_SANDBOX_NAME) | ||
| ) -join "`n" | ||
| $script = @" | ||
| set -euo pipefail | ||
| cd '$env:WSL_WORKDIR' | ||
| export NVIDIA_INFERENCE_API_KEY='$env:NVIDIA_INFERENCE_API_KEY' | ||
| export GITHUB_TOKEN='$env:GITHUB_TOKEN' | ||
| export NEMOCLAW_NON_INTERACTIVE='$env:NEMOCLAW_NON_INTERACTIVE' | ||
| export NEMOCLAW_ACCEPT_THIRD_PARTY_SOFTWARE='$env:NEMOCLAW_ACCEPT_THIRD_PARTY_SOFTWARE' | ||
| export NEMOCLAW_RECREATE_SANDBOX='$env:NEMOCLAW_RECREATE_SANDBOX' | ||
| export NEMOCLAW_SANDBOX_NAME='$env:NEMOCLAW_SANDBOX_NAME' | ||
| cd $workdir | ||
| $exports | ||
| export NEMOCLAW_RUN_LIVE_E2E=1 | ||
| npx vitest run --project e2e-live test/e2e/live/full-e2e.test.ts --silent=false --reporter=default | ||
| "@ | ||
| $tmp = "$env:RUNNER_TEMP\wsl-step.sh" | ||
| [IO.File]::WriteAllText($tmp, ($script -replace "`r",""), (New-Object System.Text.UTF8Encoding $false)) | ||
| $wslTmp = wsl -d $env:WSL_DISTRO -- wslpath -u ($tmp -replace '\\','/') | ||
| wsl -d $env:WSL_DISTRO -- bash -l $wslTmp | ||
| Invoke-WslScript -Distro $env:WSL_DISTRO -Script $script |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect how the helper transfers scripts and whether it supports env passthrough.
fd -t f 'ci-helper.ps1' tools
rg -nP -C3 'WSLENV|Write-WslScriptFile|New-WslScriptArguments|nemoclaw-wsl-step' tools/wsl/ci-helper.ps1
# Any existing precedent for env passthrough into WSL in workflows?
rg -nP -C2 'WSLENV' .github toolsRepository: NVIDIA/NemoClaw
Length of output: 1015
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the relevant helper functions and workflow lines.
sed -n '1,220p' tools/wsl/ci-helper.ps1
printf '\n--- WORKFLOW ---\n'
sed -n '130,190p' .github/workflows/wsl-e2e.yaml
printf '\n--- SEARCH ---\n'
rg -n 'WSLENV|ENV|environment|pass.*env|ScriptArguments|Invoke-WslScript|Write-WslScriptFile|New-WslScriptArguments' tools/wsl/ci-helper.ps1 .github/workflows/wsl-e2e.yamlRepository: NVIDIA/NemoClaw
Length of output: 10665
Pass the secrets through WSL env instead of inlining them into the script
NVIDIA_INFERENCE_API_KEY and GITHUB_TOKEN end up in the temporary nemoclaw-wsl-step.sh file. Use WSLENV/env passthrough and read them directly inside WSL so the transferred script stays secret-free.
🤖 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/wsl-e2e.yaml around lines 153 - 170, Update the WSL
invocation around Invoke-WslScript so NVIDIA_INFERENCE_API_KEY and GITHUB_TOKEN
are passed through the WSL environment via WSLENV rather than embedded in the
$exports script content. Remove those secrets from the generated script and
export/read them directly inside WSL, while preserving the existing non-secret
environment exports and test command.
Comment on lines
+130
to
+133
| expect(readRepoText(WORKFLOW_PATH)).not.toMatch(/WriteAllText|wslpath|wsl\s+--install/u); | ||
| expect(readRepoText(WSL_E2E_WORKFLOW_PATH)).not.toMatch( | ||
| /WriteAllText|wslpath|wsl\s+--install/u, | ||
| ); |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Forbidden-pattern guard misses direct wsl -d invocation, the main path being migrated away from.
/WriteAllText|wslpath|wsl\s+--install/u catches temp-file writes, path conversion, and distro install, but a reintroduced wsl -d $env:WSL_DISTRO bash -l ... tail in either workflow would still pass. Since only the trusted helper is supposed to construct WSL commands, include the direct-invocation shape too.
As per path instructions, "Migration tests must prove the superseded path is unreachable or removed, not merely prove that the new path also works."
🔒 Proposed tightening
- expect(readRepoText(WORKFLOW_PATH)).not.toMatch(/WriteAllText|wslpath|wsl\s+--install/u);
- expect(readRepoText(WSL_E2E_WORKFLOW_PATH)).not.toMatch(
- /WriteAllText|wslpath|wsl\s+--install/u,
- );
+ const forbidden = /WriteAllText|wslpath|\bwsl(?:\.exe)?\s+(?:--install|-d\b|--distribution\b)/u;
+ expect(readRepoText(WORKFLOW_PATH)).not.toMatch(forbidden);
+ expect(readRepoText(WSL_E2E_WORKFLOW_PATH)).not.toMatch(forbidden);📝 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
| expect(readRepoText(WORKFLOW_PATH)).not.toMatch(/WriteAllText|wslpath|wsl\s+--install/u); | |
| expect(readRepoText(WSL_E2E_WORKFLOW_PATH)).not.toMatch( | |
| /WriteAllText|wslpath|wsl\s+--install/u, | |
| ); | |
| const forbidden = /WriteAllText|wslpath|\bwsl(?:\.exe)?\s+(?:--install|-d\b|--distribution\b)/u; | |
| expect(readRepoText(WORKFLOW_PATH)).not.toMatch(forbidden); | |
| expect(readRepoText(WSL_E2E_WORKFLOW_PATH)).not.toMatch(forbidden); |
🤖 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/platform-vitest-main-workflow.test.ts` around lines 130 - 133, Extend
the forbidden-pattern assertions in the main and WSL E2E workflow checks to also
reject direct `wsl -d` invocations, including the PowerShell
environment-variable form used by the migrated command tail. Keep the existing
`WriteAllText`, `wslpath`, and `wsl --install` protections unchanged, ensuring
both `readRepoText` checks enforce that only the trusted helper constructs WSL
commands.
Source: Path instructions
Comment on lines
+51
to
+53
| const NEEDS_INTERPOLATION = /\$\{\{\s*toJSON\s*\(\s*needs\s*\)\s*\}\}/iu; | ||
| const NEEDS_ENV_PARSE = | ||
| /\bJSON\.parse\(\s*process\.env\.NEEDS_JSON\s*\|\|\s*["']\{\}["']\s*\)/u; |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
set -euo pipefail
printf '\n== tools/e2e/operations-workflow-boundary.mts ==\n'
sed -n '1,240p' tools/e2e/operations-workflow-boundary.mts
printf '\n== test/e2e/support/e2e-operations-workflow-boundary.test.ts ==\n'
sed -n '1,220p' test/e2e/support/e2e-operations-workflow-boundary.test.tsRepository: NVIDIA/NemoClaw
Length of output: 19450
🏁 Script executed:
set -euo pipefail
sed -n '1,240p' tools/e2e/operations-workflow-boundary.mts
printf '\n---\n'
sed -n '1,220p' test/e2e/support/e2e-operations-workflow-boundary.test.tsRepository: NVIDIA/NemoClaw
Length of output: 19340
🏁 Script executed:
set -euo pipefail
echo 'hi'Repository: NVIDIA/NemoClaw
Length of output: 156
🏁 Script executed:
set -euo pipefail
printf '\n== workflow refs ==\n'
rg -n "NEEDS_JSON|toJSON\\(needs\\)|process\\.env\\.NEEDS_JSON|needs\\." .github/workflows/e2e.yaml test/e2e/support/e2e-operations-workflow-boundary.test.ts tools/e2e/operations-workflow-boundary.mtsRepository: NVIDIA/NemoClaw
Length of output: 4409
Broaden the needs guard in the workflow-boundary check.
passesNeedsAsEnvironmentData only rejects ${{ toJSON(needs) }}, so a direct ${{ needs.job.outputs.foo }} interpolation can still satisfy the current checks. Tighten that predicate and add a regression test for a direct needs.* reference while keeping the NEEDS_JSON env binding intact.
📍 Affects 2 files
tools/e2e/operations-workflow-boundary.mts#L51-L53(this comment)tools/e2e/operations-workflow-boundary.mts#L141-L149test/e2e/support/e2e-operations-workflow-boundary.test.ts#L68-L117
🤖 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/operations-workflow-boundary.mts` around lines 51 - 53, Broaden
NEEDS_INTERPOLATION and the passesNeedsAsEnvironmentData check in
tools/e2e/operations-workflow-boundary.mts (lines 51-53 and 141-149) to reject
direct needs.* interpolations as well as toJSON(needs), while preserving the
NEEDS_JSON environment binding. Add a regression case in
test/e2e/support/e2e-operations-workflow-boundary.test.ts (lines 68-117)
covering a direct needs.job.outputs.foo reference.
Source: Path instructions
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
23 tasks
cv
added a commit
that referenced
this pull request
Jul 28, 2026
<!-- markdownlint-disable MD041 --> ## Summary This PR completes #6952 and #6958 by moving repeated Windows and WSL executable logic into the trusted helper from #7719. It recreates the #7737 extraction directly on `main` after the helper landed. ## Related Issue Fixes #6952 Fixes #6958 ## Changes - Use `tools/wsl/ci-helper.ps1` for WSL E2E and platform Vitest setup, script transfer, Node.js installation, and ext4 checkout synchronization. The workflow regression tests protect both consumers. - Load the helper from the PR base SHA or workflow SHA before candidate checkout. Both workflows fail if the trusted helper is missing. - Pass `needs` through `NEEDS_JSON` instead of interpolating GitHub expressions into executable source. The E2E operations boundary test rejects direct interpolation. - Validate through the TypeScript AST that `needs` receives the parsed environment value. Isolated tests reject comments, unrelated assignments, and malformed wiring. - Update watch triggers and the source-shape budget for the extracted logic. ## Type of Change - [x] 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 - [x] Tests added or updated for changed behavior - [ ] Existing tests cover changed behavior — justification: - [ ] Tests not applicable — justification: - [ ] Docs updated for user-facing behavior changes - [x] Docs not applicable — justification: This PR changes internal CI workflows and tests. It does not change a CLI, configuration, supported integration, or user-facing contract. - [x] Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging) - [x] Sensitive-path review completed or maintainer-approved waiver recorded — reviewer/approval link/justification: The trusted helper is loaded from the base or workflow SHA before candidate checkout. Focused tests verify trusted provenance, failure when the helper is missing, path overlap rejection, quoting, encoding, `needs` assignment and interpolation, and root separation. - [ ] Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue: ## Documentation Writer Review - [x] Documentation writer subagent reviewed the completed changes - Result: `no-docs-needed` - Evidence: Reviewed head `b56897529e166385464c7e8ec051174f3af4f2aa` against base `c3ab0526eba41d1c681e92bbad3d174925aeaa6a`. The AST validator and its regressions change internal CI security enforcement only. No documentation source or user-facing contract changed. The review-fix suite passed 45 tests. Biome, repository checks, CLI type-checking, the diff check, source-shape budget, title check, gitleaks, and commitlint passed. - Agent: Codex Desktop <!-- docs-review-head-sha: b568975 --> <!-- docs-review-agents-blob-sha: be20a09 --> ## DGX Station Hardware Evidence - [ ] Tested on DGX Station - Tested commit: - Station profile/scenario: - Result: - Supporting evidence: ## Verification - [x] PR description includes a `Signed-off-by:` line and every commit appears as `Verified` in GitHub - [x] Normal `pre-commit`, `commit-msg`, and `pre-push` hooks passed, or `npm run check:diff` passed when hooks were skipped or unavailable - [x] Targeted behavior tests pass for the current change set, or tests are marked not applicable above — command/result or justification: The original focused suite passed 50 tests and skipped 12 PowerShell-only tests. After the review fix, the E2E operations boundary suite passed 45 tests. `npx biome check` for the changed files, `npm run checks`, `npm run typecheck:cli`, and `git diff --check` passed. - [ ] Applicable broad gate passed — `npm test` for broad runtime/test-harness changes; `npm run check` for repo-wide validation/coverage changes — command/result: - [x] Quality Gates section completed with required justifications or waivers - [x] No secrets, API keys, or credentials committed - [ ] `npm run docs` builds without warnings (doc changes only) - [ ] Doc pages follow the [style guide](https://github.com/NVIDIA/NemoClaw/blob/main/docs/CONTRIBUTING.md) (doc changes only) - [ ] New doc pages include SPDX header and frontmatter (new pages only) --- Signed-off-by: Carlos Villela <cvillela@nvidia.com> --------- Signed-off-by: Carlos Villela <cvillela@nvidia.com>
cv
pushed a commit
that referenced
this pull request
Jul 28, 2026
<!-- markdownlint-disable MD041 --> ## Summary Add the canonical dated changelog entry for NemoClaw v0.0.97 before the release plan captures `origin/main`. The entry groups the user-visible and maintainer-facing changes since v0.0.96 while preserving the Deferred dual-Station status, experimental runtime-identity boundary, and pending physical IGX validation. ## Changes - Add `docs/changelog/2026-07-28.mdx` with the parser-safe MDX SPDX comment and exact `## v0.0.97` heading. - Summarize the 43 merged PRs in the release range, omitting internal-only changes from the public entry and linking each grouped change to its most specific published documentation. - Keep the experimental Okta reference explicitly opt-in and outside normal onboarding, keep the two-Station path Deferred, and state that physical IGX Orin validation remains pending. ### Source summary - [#7440](#7440), [#7443](#7443), and [#7445](#7445) -> `docs/changelog/2026-07-28.mdx`: Document read-only host readiness reports and fail-closed platform qualification. - [#7030](#7030) -> `docs/changelog/2026-07-28.mdx`: Document the Deferred trusted two-Station vLLM evaluation. - [#7265](#7265) -> `docs/changelog/2026-07-28.mdx`: Document the bounded experimental direct-runner Okta runtime-identity reference. - [#7711](#7711) and [#7648](#7648) -> `docs/changelog/2026-07-28.mdx`: Document compatible-endpoint reasoning effort and retired NVIDIA Build model paths. - [#7746](#7746), [#7763](#7763), and [#7681](#7681) -> `docs/changelog/2026-07-28.mdx`: Document safe compatible-provider creation, replacement refusal, and narrow OpenShell bridge URL handling. - [#7641](#7641), [#7690](#7690), [#7631](#7631), and [#7710](#7710) -> `docs/changelog/2026-07-28.mdx`: Document paused-container recovery, recreation journaling, pre-mutation uninstall checks, and source-checkout OpenShell selection. - [#7624](#7624) and [#7762](#7762) -> `docs/changelog/2026-07-28.mdx`: Document Jetson release diagnostics and bounded render-device group propagation. - [#7639](#7639), [#7760](#7760), [#7721](#7721), and [#7761](#7761) -> `docs/changelog/2026-07-28.mdx`: Document Telegram, MCP media-type, Hermes image-mode, and locked-restart fixes. - [#7653](#7653) and [#7680](#7680) -> `docs/changelog/2026-07-28.mdx`: Document Deep Agents policy tasks and the bounded Claude Code OAuth path. - [#7679](#7679) -> `docs/changelog/2026-07-28.mdx`: Document the checksum-bound libssh2 and Python HTMLParser backports. - [#7655](#7655), [#7651](#7651), [#7664](#7664), [#7666](#7666), [#7670](#7670), [#7719](#7719), and [#7741](#7741) -> `docs/changelog/2026-07-28.mdx`: Document exact candidate E2E evidence, Launchable selection, diagnostic consolidation, and trusted WSL validation. ## Type of Change - [ ] Code change (feature, bug fix, or refactor) - [ ] Code change with doc updates - [x] Doc only (prose changes, no code sample modifications) - [ ] Doc only (includes code sample changes) ## Quality Gates - [ ] Tests added or updated for changed behavior - [x] Existing tests cover changed behavior — justification: `test/changelog-docs.test.ts` validates the dated changelog contract, MDX header, heading uniqueness, and release-entry structure. - [ ] Tests not applicable — justification: - [x] Docs updated for user-facing behavior changes - [ ] Docs not applicable — justification: - [ ] 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: - [ ] Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue: ## Documentation Writer Review - [x] Documentation writer subagent reviewed the completed changes - Result: `docs-updated` - Evidence: The committed `docs/changelog/2026-07-28.mdx` blob exactly matches the reviewed file. Completeness, factual accuracy, link shape, parser-safe MDX header, one-sentence-per-line style, `.docs-skip` compliance, and bounded product claims passed. - Agent: Codex Desktop documentation writer subagent <!-- docs-review-head-sha: da6aa27 --> <!-- docs-review-agents-blob-sha: be20a09 --> ## DGX Station Hardware Evidence - [ ] Tested on DGX Station - Tested commit: Not applicable; this PR changes only the dated changelog. - Station profile/scenario: Not applicable. - Result: Not applicable. - Supporting evidence: Not applicable. ## Verification - [x] PR description includes a `Signed-off-by:` line and every commit appears as `Verified` in GitHub - [x] Normal `pre-commit`, `commit-msg`, and `pre-push` hooks passed, or `npm run check:diff` passed when hooks were skipped or unavailable - [x] Targeted behavior tests pass for the current change set, or tests are marked not applicable above — `npx vitest run test/changelog-docs.test.ts` passed 6/6. - [ ] Applicable broad gate passed — `npm test` for broad runtime/test-harness changes; `npm run check` for repo-wide validation/coverage changes — not applicable to this doc-only release entry. - [x] Quality Gates section completed with required justifications or waivers - [x] No secrets, API keys, or credentials committed - [ ] `npm run docs` builds without warnings (doc changes only) — completed with 0 errors and 2 pre-existing Fern warnings. - [x] Doc pages follow the [style guide](https://github.com/NVIDIA/NemoClaw/blob/main/docs/CONTRIBUTING.md) (doc changes only) - [ ] New doc pages include SPDX header and frontmatter (new pages only) — native changelog entries use the required parser-safe MDX SPDX comment and intentionally have no frontmatter. --- Signed-off-by: Charan Jagwani <cjagwani@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added improved host readiness reporting and Jetson onboarding guidance. * Added controls for reasoning effort with compatible endpoints and enhanced managed MCP discovery. * Improved Deep Agents task publication and preset support. * **Bug Fixes** * Hardened provider switching, sandbox recovery, uninstall behavior, and Telegram connectivity. * Improved container image integrity checks, media-type handling, and checksum validation. * Enhanced vLLM evaluation behavior and release diagnostics. * **Documentation** * Added the NemoClaw v0.0.97 changelog. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
23 tasks
23 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
2 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
Add the tested WSL provisioning helper to a trusted
mainrevision before consumer workflows adopt it. This helper-only PR is the first stage of #6958 so root-capable WSL execution can fail closed until the helper is present onmain.Related Issue
Part of #6958
Parent: #6952
Previous staged extraction: #7709 (merged into this branch and reverted)
Previous replacement extraction: #7737 (merged into this branch and reverted)
Next step: recreate the extraction directly from
mainafter this PR landsChanges
tools/wsl/ci-helper.ps1for WSL path conversion, LF-safe script transfer, command construction, distro provisioning, Node.js installation, and ext4 checkout synchronization.Type of Change
Quality Gates
3a5ebeb2fpassed all nine categories, and the helper files at92507d621are byte-for-byte unchanged. The helper keeps WSL arguments separate and quotes shell values. Sync deletion is limited to one positive run-ID/attempt child under the two workflow-owned roots and fails closed for symlinked roots, bidirectional checkout overlap, traversal, and unrelated paths. Package and owner inputs are validated, pinned Node.js archives are verified, transferred scripts are deleted infinally, and direct tests protect these boundaries.Documentation Writer Review
no-docs-needed; writing review passed with no findings92507d621762bb77ac7e76baf50d9a7c6ee87f28and basebbdd85008c34503ea47d4a80d57e90074ff3c79e, the exact-base diff adds onlytools/wsl/ci-helper.ps1andtest/wsl-ci-helper.test.ts. Both match the previously reviewed helper tree. No documentation sources or user-facing contracts changed. Twelve PowerShell cases were discovered and skipped because PowerShell is unavailable locally; source architecture checks, CLI type-check, normal hooks, and diff check passed.DGX Station Hardware Evidence
Verification
Signed-off-by:line and every commit appears asVerifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passednpm run checks,npm run typecheck:cli, normal hooks, and the diff check passed.npm testfor broad runtime/test-harness changes;npm run checkfor repo-wide validation/coverage changes — command/result:npm run docsbuilds without warnings (doc changes only)Signed-off-by: Carlos Villela cvillela@nvidia.com
Summary by CodeRabbit
/mnt/..., safe bash literal escaping, and LF-only UTF-8 script writing without a byte-order mark.