test(e2e): add inactive Windows MXC OpenClaw qualification - #8300
Conversation
Signed-off-by: Senthil Ravichandran <senthilr@nvidia.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds an inactive Windows x64 MXC OpenClaw ChangesWindows MXC OpenClaw qualification
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in commit 99e5415 in the TypeScript / code-coverage/cliThe overall coverage in commit 99e5415 in the Show a code coverage summary of the most impacted files.
Updated |
PR Review Advisor — No blocking findings reportedAdvisor assessment: No blocking advisor findings reported Model lanes
5 terminology differences from the second opinionAdvisory only. These are normalized differences from the primary terminology receipt.
Second-opinion terminology and E2E selections are advisory. They do not change the primary assessment or E2E / PR Gate. 2 semantic terminology decisionsTerminology decisions are advisory. They affect the assessment only when a separate finding identifies concrete semantic impact.
E2E guidanceAdvisory only. E2E / PR Gate selects and runs jobs independently. Recommended E2E: 1 warning · 0 suggestionsWarningsWarnings do not block.
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (7)
test/e2e/live/windows-mxc-openclaw-process-container-helpers.ts (6)
680-702: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDerive the timeout text from the constant.
The deadline uses
COMMAND_TIMEOUT_MS, but the error message on line 701 hardcodes "30 seconds". IfCOMMAND_TIMEOUT_MSchanges, the message becomes wrong.♻️ Proposed change
- throw new Error("OpenShell gateway did not listen within 30 seconds"); + throw new Error(`OpenShell gateway did not listen within ${COMMAND_TIMEOUT_MS} ms`);🤖 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/live/windows-mxc-openclaw-process-container-helpers.ts` around lines 680 - 702, Update the timeout error in waitForPort to derive its reported duration from COMMAND_TIMEOUT_MS instead of hardcoding “30 seconds,” ensuring the message remains accurate if the constant changes.
564-577: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueClear the timeout on the output-bound rejection path.
The overflow branch calls
child.kill()andreject(...)but leavestimerscheduled. Theclosehandler normally clears it. If the killed child never closes, the timer fires later and callschild.kill()a second time. The promise is already settled, so the secondrejectis a no-op, and the impact is limited to a stray timer.Call
clearTimeout(timer)in the overflow branch.♻️ Proposed change
if (outputBytes > MAX_COMMAND_OUTPUT_BYTES) { + clearTimeout(timer); child.kill(); reject(new Error(`${path.basename(file)} output exceeded its bound`)); return; }🤖 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/live/windows-mxc-openclaw-process-container-helpers.ts` around lines 564 - 577, Update the output overflow branch in append to call clearTimeout(timer) before killing the child and rejecting, ensuring the timeout cannot fire after output-bound rejection.
493-510: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueConsider ending the host wait when the agent gives up.
The agent stops polling health after 120000 ms and then never writes
readyPath. The hostwaitForFile(readyPath, READY_TIMEOUT_MS)at line 1043 keeps waiting for the full 180000 ms. Every health failure therefore adds about 60 seconds of dead wait.Have the agent write a terminal marker (or write
readyPathwith a failure body that the host inspects) so the host stops as soon as the outcome is known. Deriving both deadlines from one value would also keep them consistent.🤖 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/live/windows-mxc-openclaw-process-container-helpers.ts` around lines 493 - 510, Update the health-polling flow around the gateway wait loop so that exhausting its deadline writes a terminal failure marker or failure body to readyPath, and ensure the host wait logic inspects that result and exits promptly instead of waiting for READY_TIMEOUT_MS. Keep successful health detection unchanged, and derive the agent and host deadlines from a shared timeout where appropriate.
269-271: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider streaming and sharing one
sha256Fileimplementation.
sha256Filereads the whole file into memory. The staged tree includesnode.exeand can include up to 100,000 files hashed through the identical private copy intools/e2e/windows-mxc-openclaw-artifact-tree.mts(lines 11-13). Export the tools-module implementation and import it here, so both digests come from one definition.Streaming with
createReadStreamwould also bound peak memory. This is optional for a local qualification target.🤖 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/live/windows-mxc-openclaw-process-container-helpers.ts` around lines 269 - 271, Replace the local sha256File implementation with the exported shared implementation from tools/e2e/windows-mxc-openclaw-artifact-tree.mts, and import that symbol here so both callers use one definition. Do not add the optional streaming change unless required by the existing API.
356-359: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueSecurity Misconfiguration (CWE-1284): Improper Validation of Specified Quantity in Input
Reachability: Internal · Exploitability: Theoretical
Reachability path
● Entry test/e2e/support/windows-mxc-openclaw-process-container.test.ts │ ▼ ● Hop tools/e2e/windows-mxc-openclaw-artifact-tree.mts:11 sha256File │ ▼ ● Sink test/e2e/live/windows-mxc-openclaw-process-container-helpers.tsAnchor the port value to its
--portflag.
assertExpectedOpenClawProcessIdentityaccepts the expected port as any command-line token. Match the ordered--portand value pair so the port check is explicit. This remains defense in depth.🤖 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/live/windows-mxc-openclaw-process-container-helpers.ts` around lines 356 - 359, Update assertExpectedOpenClawProcessIdentity so the expected port is validated as the ordered "--port" flag followed by String(expected.port), rather than as an unconstrained command-line token. Preserve the existing checks for the entry path, "gateway", and probe agent path.
768-790: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueSecurity Misconfiguration (CWE-426): Untrusted Search Path
Reachability: Internal · Exploitability: Theoretical
Reachability path
● Entry test/e2e/support/windows-mxc-openclaw-process-container.test.ts │ ▼ ● Hop tools/e2e/windows-mxc-openclaw-artifact-tree.mts:11 sha256File │ ▼ ● Sink test/e2e/live/windows-mxc-openclaw-process-container-helpers.tsMake checkout identity checks independent of the caller context.
If exact identity checks must remain independent of host state, resolve
gitthrough the same trusted executable policy and pass the checkout root throughcwd(or usegit -C). The current calls depend onPATHand the process working directory.🤖 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/live/windows-mxc-openclaw-process-container-helpers.ts` around lines 768 - 790, Update assertCurrentCheckoutIdentity to resolve git using the established trusted executable policy and execute both rev-parse and status checks against the intended checkout root via cwd or git -C. Keep the existing revision and clean-status validation behavior unchanged while removing dependence on PATH and the caller’s working directory.test/e2e/support/windows-mxc-openclaw-process-container.test.ts (1)
130-196: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider splitting this test by concern.
The test covers registry name matching, the delete-retry predicate, host process identity equality, and three cases of
assertExpectedOpenClawProcessIdentity. The title names two of them. A failure in any one of the seven assertions reports the same test name, so the cause is not visible from the report.Splitting into one test per exported function would localize failures.
🤖 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/windows-mxc-openclaw-process-container.test.ts` around lines 130 - 196, Split the combined test into separate tests for each exported function: sandboxListContainsExactName, shouldRetrySandboxDelete, sameWindowsProcessIdentity, and assertExpectedOpenClawProcessIdentity. Keep the existing assertions and scenarios unchanged, grouping the three process-identity validation cases under the latter function so failures identify their specific concern.
🤖 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/live/windows-mxc-openclaw-process-container-helpers.ts`:
- Around line 1086-1095: Update the sandboxCreateAccepted logic to document why
a non-zero create.exitCode is tolerated and replace the command-detail error
denylist with a positive success signal from the pinned OpenShell revision.
Preserve the existing health and registry prerequisites, and ensure acceptance
requires that explicit signal rather than merely the absence of known error
phrases.
- Around line 1249-1260: Add an identity-validated gateway termination fallback
to the cleanup block around gateway.kill() and gatewayStopped: after the
existing five-second wait, verify the gateway is still the expected process,
invoke the established taskkill/escalation mechanism, and wait for termination
before finalizing gatewayStopped. Reuse the same PID-validation approach used
for the OpenClaw process cleanup rather than leaving a surviving gateway
unhandled.
- Around line 472-489: Update the gateway spawn flow around spawn and
writeFileSync to capture the ChildProcess "error" event, prevent an unhandled
exception, and record the spawn failure for the probe result. Only write
openClawPidPath when gateway.pid is defined, and include the captured spawnError
in the result object written by the probe agent so the receipt reports the
actual cause.
- Around line 899-902: Update the setup around gatewayPort, openClawPort,
sandboxName, and gatewayName to allocate openClawPort with freeLoopbackPort()
instead of deriving it from runId, ensuring the host-side probe validates the
port. Derive one name from the other if they must remain equal; otherwise use
distinct prefixes so sandbox and gateway entries are distinguishable.
- Around line 1262-1283: Update the cleanup block around
sensitiveRuntimeArtifactsRemoved so each sensitive path in the cleanup list is
removed within its own try/catch, allowing subsequent paths to be attempted
after EBUSY or EPERM errors. Continue collecting removal errors in
cleanupFailures, then recompute sensitiveRuntimeArtifactsRemoved after all paths
have been processed so runRoot cleanup can follow the existing flow.
- Around line 927-949: The gatewayEnvironment construction currently forwards
the entire host environment through the environment spread. Replace that spread
with an explicit allowlist containing only the variables required by the gateway
and the existing NemoClaw/OpenShell configuration, while preserving the values
already assigned in gatewayEnvironment. Keep controlEnvironment based on the
restricted gatewayEnvironment and continue removing NEMOCLAW_MXC_E2E_TOKEN
there.
- Around line 390-391: Update the qualification receipt generation near the
pc_least_privilege configuration to record that least privilege is disabled, and
add documentation explaining why the qualification requires this relaxed
privilege posture. Keep the existing filesystem-check receipt entries and
capability configuration unchanged.
- Around line 605-615: Update the process-query result handling in the helper
that invokes Get-CimInstance so any non-zero exit code is checked and raised
before evaluating empty stdout. Preserve the explicit exit code 3 path as the
genuine no-process case returning null, while only treating empty stdout as
absent after a successful query.
In `@test/e2e/live/windows-mxc-openclaw-process-container.test.ts`:
- Around line 18-33: The declared phase timeline does not match the work
performed by runWindowsMxcOpenClawProcessContainerQualification. Align the
phases with the actual boundaries by moving sandbox deletion, registry
verification, process termination, and cleanup into a distinct third
phase—either by splitting the qualification flow or having that function call
progress.phase for the cleanup phase through its existing progress parameter—so
the receipt assertion phase is not misleadingly used for completed work.
In `@test/e2e/support/windows-mxc-openclaw-process-container.test.ts`:
- Around line 232-249: Replace the source-text assertions in the test “uses
OpenShell as the sole MXC control boundary and never pre-deletes by name
(`#8178`)” with behavior-focused assertions on the exported helpers. Add AST rules
to tools/e2e/check-semantic-phases.mts that reject direct wxc-exec invocation
and sandbox-name deletion before creation, without relying on formatting,
literal counts, or source ordering; keep this test limited to observable
public-helper outcomes.
- Line 126: Update the assertion for normalizeReportedVersion("2026.7.10\n") to
use an exact expected normalized version value rather than a negative
comparison, preserving the test’s intended verification that the full version
does not get truncated to the prefix.
---
Nitpick comments:
In `@test/e2e/live/windows-mxc-openclaw-process-container-helpers.ts`:
- Around line 680-702: Update the timeout error in waitForPort to derive its
reported duration from COMMAND_TIMEOUT_MS instead of hardcoding “30 seconds,”
ensuring the message remains accurate if the constant changes.
- Around line 564-577: Update the output overflow branch in append to call
clearTimeout(timer) before killing the child and rejecting, ensuring the timeout
cannot fire after output-bound rejection.
- Around line 493-510: Update the health-polling flow around the gateway wait
loop so that exhausting its deadline writes a terminal failure marker or failure
body to readyPath, and ensure the host wait logic inspects that result and exits
promptly instead of waiting for READY_TIMEOUT_MS. Keep successful health
detection unchanged, and derive the agent and host deadlines from a shared
timeout where appropriate.
- Around line 269-271: Replace the local sha256File implementation with the
exported shared implementation from
tools/e2e/windows-mxc-openclaw-artifact-tree.mts, and import that symbol here so
both callers use one definition. Do not add the optional streaming change unless
required by the existing API.
- Around line 356-359: Update assertExpectedOpenClawProcessIdentity so the
expected port is validated as the ordered "--port" flag followed by
String(expected.port), rather than as an unconstrained command-line token.
Preserve the existing checks for the entry path, "gateway", and probe agent
path.
- Around line 768-790: Update assertCurrentCheckoutIdentity to resolve git using
the established trusted executable policy and execute both rev-parse and status
checks against the intended checkout root via cwd or git -C. Keep the existing
revision and clean-status validation behavior unchanged while removing
dependence on PATH and the caller’s working directory.
In `@test/e2e/support/windows-mxc-openclaw-process-container.test.ts`:
- Around line 130-196: Split the combined test into separate tests for each
exported function: sandboxListContainsExactName, shouldRetrySandboxDelete,
sameWindowsProcessIdentity, and assertExpectedOpenClawProcessIdentity. Keep the
existing assertions and scenarios unchanged, grouping the three process-identity
validation cases under the latter function so failures identify their specific
concern.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 978c9180-ee62-4eb0-9e8b-02f1d41f65be
📒 Files selected for processing (6)
test/e2e/README.mdtest/e2e/live/windows-mxc-openclaw-process-container-helpers.tstest/e2e/live/windows-mxc-openclaw-process-container.test.tstest/e2e/support/windows-mxc-openclaw-process-container.test.tstools/e2e/check-semantic-phases.mtstools/e2e/windows-mxc-openclaw-artifact-tree.mts
senthilr-nv
added
chore
|
Maintainer train classification for This PR is within #8178 delivery step 5 because it adds an inactive, opt-in qualification harness and does not register, activate, or document Windows/MXC support. It is not merge-ready yet:
These are technical and security validation gaps, not approval to broaden the product surface. No check waiver or activation is proposed. |
Signed-off-by: Senthil Ravichandran <senthilr@nvidia.com>
Signed-off-by: Senthil Ravichandran <senthilr@nvidia.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
test/e2e/live/windows-mxc-openclaw-process-container-helpers.ts (1)
1143-1146: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winShort-circuit the outcome wait when
sandbox createfails.
createalready runs withREADY_TIMEOUT_MS. Ifcreateexits non-zero, no probe agent runs, sowaitForFile(outcomePath, READY_TIMEOUT_MS)waits another 180000 ms and then throws a timeout error. The recordedprimaryFailurethen reports a missing outcome file instead of thecreatefailure.Fail fast on a non-zero
createexit code and record the command detail.♻️ Proposed short-circuit
+ if (create.exitCode !== 0) { + throw new Error(`OpenShell sandbox create failed: ${commandDetail(create)}`); + } await waitForFile(outcomePath, READY_TIMEOUT_MS); const ready = fs.existsSync(readyPath);🤖 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/live/windows-mxc-openclaw-process-container-helpers.ts` around lines 1143 - 1146, Update the sandbox creation flow around the `create` result and `waitForFile(outcomePath, READY_TIMEOUT_MS)` to check for a non-zero create exit code first; skip waiting for the outcome file, fail immediately, and record the create command detail in `primaryFailure` while preserving the existing outcome handling for successful creates.test/e2e/support/e2e-semantic-phase-check.test.ts (1)
52-65: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the missing create and missing delete failures.
Both test sources contain a
sandbox createcall and asandbox deletecall. The branches atcheck-semantic-phases.mtslines 189-190 that emit"OpenShell sandbox create command is missing"and"OpenShell sandbox delete command is missing"are never exercised. A source that drops the sandbox lifecycle entirely is the case the guard exists to catch.💚 Proposed additional case
+ test("rejects a Windows MXC control flow without sandbox create or delete", () => { + const source = ` + async function qualify(cli, env, progress) { + await runCommand(cli, ["gateway", "select", "name"], env, progress, "select"); + } + `; + + expect(validateWindowsMxcControlBoundarySource(source)).toEqual([ + "OpenShell sandbox create command is missing", + "OpenShell sandbox delete command is missing", + ]); + });🤖 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-semantic-phase-check.test.ts` around lines 52 - 65, Add test coverage in the semantic phase validation tests around validateWindowsMxcControlBoundarySource for a source that omits both sandbox lifecycle commands, and assert the expected “OpenShell sandbox create command is missing” and “OpenShell sandbox delete command is missing” diagnostics. Keep the existing direct wxc-exec and delete-before-create coverage unchanged.tools/e2e/check-semantic-phases.mts (2)
2630-2637: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe Windows MXC validation is bound to one exact path string.
validateWindowsMxcControlBoundarySourceruns only whenrelativeFileequalsWINDOWS_MXC_OPENCLAW_HELPER. If the helper is renamed or moved, the traversal no longer matches, and the control-boundary guard stops running without any failure. The graph scan still succeeds, so no test reports the gap.Assert that the constant resolves to an existing file, or record a failure when the live target graph contains no matching source.
♻️ Proposed existence assertion
const relativeFile = path.relative(REPO_ROOT, file).split(path.sep).join("/"); if (relativeFile === WINDOWS_MXC_OPENCLAW_HELPER) { + // The guard is path-bound; a rename must fail loudly instead of skipping validation. childProcessAuditFailures.push( ...validateWindowsMxcControlBoundarySource(sourceFile.text).map( (failure) => `${relativeFile}: ${failure}`, ), ); }Add a repository check that
WINDOWS_MXC_OPENCLAW_HELPERexists on disk.🤖 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/check-semantic-phases.mts` around lines 2630 - 2637, Ensure the repository scan validates that WINDOWS_MXC_OPENCLAW_HELPER resolves to an existing file, recording a childProcessAuditFailures entry when it does not. Add this check near the traversal logic so validateWindowsMxcControlBoundarySource cannot silently stop running after the helper is moved or renamed.
156-197: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe ordering check proves source position, not execution order.
commandKindrecordsnode.getStart(sourceFile), sodeletePositions.some((position) => position < observedCreatePosition)compares text offsets. A delete call placed in a helper function declared above the create call fails the check even when it runs after create. A delete that runs first at runtime but appears later in the file passes.
containsWxcExecPathis also broad. It matches anywxcExecPathidentifier inside an argument subtree, including a non-executing use such aspath.dirname(inputs.openShell.wxcExecPath)in aspawnoptions object.Both behaviors are acceptable for a source-level guard. Record the intent in a comment so a later reader does not treat the check as a control-flow proof.
🤖 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/check-semantic-phases.mts` around lines 156 - 197, Add a concise comment near the source-position tracking and ordering logic in `inspect` explaining that the check is a source-level guard based on textual offsets, not runtime control-flow or execution order, and that `containsWxcExecPath` intentionally performs broad subtree matching. Do not change `commandKind`, `getStart(sourceFile)`, or the existing validation behavior.
🤖 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/support/windows-mxc-openclaw-process-container.test.ts`:
- Around line 210-230: Update the negative case in the test “requires the
OpenShell gateway path and ordered port argument pair (`#8178`)” to keep the
expected port at 17670 while constructing a command line where --port and 17670
are not adjacent or correctly ordered, then assert that
assertExpectedOpenShellGatewayProcessIdentity throws. Remove the unrelated
--other 9999 variation so the test specifically validates ordered port-argument
matching.
---
Nitpick comments:
In `@test/e2e/live/windows-mxc-openclaw-process-container-helpers.ts`:
- Around line 1143-1146: Update the sandbox creation flow around the `create`
result and `waitForFile(outcomePath, READY_TIMEOUT_MS)` to check for a non-zero
create exit code first; skip waiting for the outcome file, fail immediately, and
record the create command detail in `primaryFailure` while preserving the
existing outcome handling for successful creates.
In `@test/e2e/support/e2e-semantic-phase-check.test.ts`:
- Around line 52-65: Add test coverage in the semantic phase validation tests
around validateWindowsMxcControlBoundarySource for a source that omits both
sandbox lifecycle commands, and assert the expected “OpenShell sandbox create
command is missing” and “OpenShell sandbox delete command is missing”
diagnostics. Keep the existing direct wxc-exec and delete-before-create coverage
unchanged.
In `@tools/e2e/check-semantic-phases.mts`:
- Around line 2630-2637: Ensure the repository scan validates that
WINDOWS_MXC_OPENCLAW_HELPER resolves to an existing file, recording a
childProcessAuditFailures entry when it does not. Add this check near the
traversal logic so validateWindowsMxcControlBoundarySource cannot silently stop
running after the helper is moved or renamed.
- Around line 156-197: Add a concise comment near the source-position tracking
and ordering logic in `inspect` explaining that the check is a source-level
guard based on textual offsets, not runtime control-flow or execution order, and
that `containsWxcExecPath` intentionally performs broad subtree matching. Do not
change `commandKind`, `getStart(sourceFile)`, or the existing validation
behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 59d64e4d-d38e-45e0-b154-5b565a06db8e
📒 Files selected for processing (8)
test/e2e/README.mdtest/e2e/live/windows-mxc-openclaw-process-container-helpers.tstest/e2e/live/windows-mxc-openclaw-process-container.test.tstest/e2e/mock-parity.jsontest/e2e/support/e2e-semantic-phase-check.test.tstest/e2e/support/windows-mxc-openclaw-process-container.test.tstools/e2e/check-semantic-phases.mtstools/e2e/windows-mxc-openclaw-artifact-tree.mts
🚧 Files skipped from review as they are similar to previous changes (2)
- test/e2e/live/windows-mxc-openclaw-process-container.test.ts
- test/e2e/README.md
Signed-off-by: Senthil Ravichandran <senthilr@nvidia.com>
Signed-off-by: Senthil Ravichandran <senthilr@nvidia.com>
|
Addressed the latest review feedback in
Focused E2E support tests pass (35/35), semantic phase coverage passes (118 tests across 75 files), repository checks and docs validation pass, and the exact-head security and documentation reviews pass. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
test/e2e/live/windows-mxc-openclaw-process-container-helpers.ts (2)
930-944: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRegister the live target in mock parity.
Add this Windows MXC OpenClaw target to
test/e2e/mock-parity.json. Without registration, the live qualification flow can diverge from its required mock coverage.As per path instructions, add the target to mock-parity.
🤖 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/live/windows-mxc-openclaw-process-container-helpers.ts` around lines 930 - 944, Register the Windows MXC OpenClaw qualification target in test/e2e/mock-parity.json, using the target identity associated with runWindowsMxcOpenClawProcessContainerQualification. Preserve the existing mock-parity structure and entries while adding this live target so it has corresponding mock coverage.Source: Path instructions
905-918: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAdd direct artifact-tree boundary tests.
Add direct tests for content changes, relative-path changes, symlink rejection, unsupported entries, deterministic ordering, and traversal or file-count limits. The current qualification depends on these properties to validate the staged artifact identity.
As per path instructions, preserve direct artifact-tree tests for digest sensitivity, relative-path changes, symlink rejection, unsupported entries, deterministic ordering, and traversal/file-count limits.
🤖 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/live/windows-mxc-openclaw-process-container-helpers.ts` around lines 905 - 918, Add direct tests for the artifact-tree hashing helper used by assertExactArtifactIdentities, covering content and relative-path changes, symlink and unsupported-entry rejection, deterministic ordering, and traversal/file-count limits. Keep these as focused unit tests of the artifact-tree behavior so qualification continues validating staged artifact identity.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.
Outside diff comments:
In `@test/e2e/live/windows-mxc-openclaw-process-container-helpers.ts`:
- Around line 930-944: Register the Windows MXC OpenClaw qualification target in
test/e2e/mock-parity.json, using the target identity associated with
runWindowsMxcOpenClawProcessContainerQualification. Preserve the existing
mock-parity structure and entries while adding this live target so it has
corresponding mock coverage.
- Around line 905-918: Add direct tests for the artifact-tree hashing helper
used by assertExactArtifactIdentities, covering content and relative-path
changes, symlink and unsupported-entry rejection, deterministic ordering, and
traversal/file-count limits. Keep these as focused unit tests of the
artifact-tree behavior so qualification continues validating staged artifact
identity.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 94fdd7af-f942-443e-b794-ee8f8afab617
📒 Files selected for processing (5)
test/e2e/README.mdtest/e2e/live/windows-mxc-openclaw-process-container-helpers.tstest/e2e/support/e2e-semantic-phase-check.test.tstest/e2e/support/windows-mxc-openclaw-process-container.test.tstools/e2e/check-semantic-phases.mts
🚧 Files skipped from review as they are similar to previous changes (2)
- test/e2e/README.md
- test/e2e/support/e2e-semantic-phase-check.test.ts
Signed-off-by: Senthil Ravichandran <senthilr@nvidia.com>
cjagwani
left a comment
cjagwani
left a comment
There was a problem hiding this comment.
Security review — exact head afcd6266ce6af736a9018bcd19c68b6afabe08af against base cbabb66bfc985f3474714f95bfbc6ec1992441f9: PASS with no findings.
- Secrets and credentials — PASS. The opt-in target creates an ephemeral random readiness token, keeps it out of arguments and receipts, redacts command output, and passes host children only an explicit Windows runtime-variable allowlist. No repository secret or reusable credential is added.
- Input validation and injection resistance — PASS. Required inputs are validated as exact revisions, versions, paths, hashes, ports, and process identities. Runtime commands use argument arrays rather than shell interpolation. Files and artifact trees are resolved, type-checked, bounded, and rejected when they contain links or unsupported entries.
- Authentication and authorization — PASS. The temporary gateway is loopback-bound and token-protected. The target neither registers nor activates MXC as a supported runtime and does not widen repository or host permissions.
- Dependencies and supply chain — PASS. No dependency or lockfile changes. NemoClaw, OpenShell,
wxc-exec, Node, OpenClaw entry, and the complete OpenClaw artifact tree are identity-pinned and revalidated immediately before security-relevant use. - Error handling and information exposure — PASS. Create, query, identity, policy, and cleanup failures are fail-closed. Child output is size-bounded, diagnostics omit the token, and the qualification receipt contains only non-secret evidence.
- Cryptography and data protection — PASS.
randomBytes(32)supplies the ephemeral token. SHA-256 is used for deterministic artifact identity, not as an authentication substitute. Sensitive temporary state is removed during terminal cleanup. - Configuration and infrastructure — PASS. The generated gateway and filesystem policy are temporary and least-privilege; network access is limited to loopback. The qualification is inactive and opt-in, with bounded processes, timeouts, output, artifact traversal, and cleanup scope.
- Security testing — PASS. Support contracts cover identity drift and replacement, dirty-source and version-prefix rejection, complete process identity, token exclusion, environment filtering, tree-content/path/link/device cases, semantic phases, policy allow/deny behavior, and cleanup fallbacks. Exact GitHub Actions and protected qualification evidence remain mandatory.
- System security — PASS. Emergency process termination occurs only after re-querying and matching the recorded executable, arguments, parent/child relationship, and port identity. Cleanup targets exact generated names and run directories and reports any residual sandbox, process, registry, or sensitive path.
The current head is a signed mechanical merge of current main. Its effective eight-file patch is byte-for-byte unchanged from reviewed head 98adf64d2b1c7d5275c801dadc30c9c44ff34911 (raw patch SHA-256 285c9a4cd764c51605f45f8a8bdaa9e557ff6db3d91980bd62cf80268b779734; stable patch ID d6bf3bd8aef510d5f59dc5c5d803216d62c0d3df). No product-scope waiver or gate waiver is used.
Signed-off-by: Charan Jagwani <cjagwani@nvidia.com>
cjagwani
left a comment
cjagwani
left a comment
There was a problem hiding this comment.
Security review — exact head 99e54157678400cfda425bdc9a1f0e531749e6ea against base cbabb66bfc985f3474714f95bfbc6ec1992441f9: PASS with no findings.
The nine-category review at parent afcd6266ce6af736a9018bcd19c68b6afabe08af is carried forward without expansion. The only new change is a documentation correction in test/e2e/README.md: it now says a secret-free receipt is written for either verdict only after preflight and local setup succeed. That matches the implementation and removes an unsafe expectation that early identity or host-validation failures would always leave a receipt.
- Secrets and credentials — PASS: no secret-bearing behavior or example changes.
- Input validation and injection resistance — PASS: no executable path changes; exact identity and argument-array controls remain intact.
- Authentication and authorization — PASS: the loopback token boundary and inactive/no-support scope are unchanged.
- Dependencies and supply chain — PASS: no dependency or artifact source changes.
- Error handling and information exposure — PASS: the documentation now accurately distinguishes pre-receipt failures from verdict-bearing runs.
- Cryptography and data protection — PASS: token generation, SHA-256 identity checks, and sensitive-state cleanup are unchanged.
- Configuration and infrastructure — PASS: no policy, privilege, host, network, or workflow change.
- Security testing — PASS: the previous exact eight-file security contracts remain unchanged; Markdown lint and the repository test-file budget passed for the follow-up.
- System security — PASS: no process, cleanup, filesystem, or trusted-boundary behavior changes.
Commit 99e54157678400cfda425bdc9a1f0e531749e6ea is signed and Signed-off-by. Fresh CI, independent exact-head approval, and protected E2E remain mandatory; no prior-head result is accepted.
|
The required documentation-writer review found and fixed one exact operator-contract issue in signed, GitHub-Verified commit |
5 participants
Summary
Adds an opt-in live E2E target for the inactive Windows MXC OpenClaw
process_containercandidate. NemoClaw previously had no revision-bound qualification path for this candidate; the target now checks exact package and artifact identities, workload readiness, filesystem allow and deny behavior, sandbox deletion, registry removal, and exact OpenClaw process cleanup without registering or activating MXC.Related Issue
Refs #8178
Changes
wxc-exec, and OpenClaw artifact tree before execution, then revalidate each pinned runtime artifact immediately before its security-relevant use.Type of Change
Quality Gates
99e54157678400cfda425bdc9a1f0e531749e6ea; the target is opt-in, loopback-bound, argument-array based, least-privilege, identity-pinned and revalidated before security-relevant use, host-environment restricted, token-redacting, resource-bounded, and fail-closed on create, query, or cleanup fallback.Documentation Writer Review
passDGX Station Hardware Evidence
Verification
Signed-off-by:line and all 8 commits appear asVerifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passed, ornpm run validate:prpassed after refreshingorigin/mainwhen hooks were skipped or unavailablenpx vitest run --project e2e-support test/e2e/support/windows-mxc-openclaw-process-container.test.ts test/e2e/support/e2e-semantic-phase-check.test.tspassed 35 tests in 2 files;npm run test:e2e-phases:checkpassed 118 tests in 75 files; mock parity, repository checks, CLI build, and CLI type-check passed. The receipt-boundary follow-up passed normal Markdown lint and the repository test-file budget.npm testfor broad runtime/test-harness changes;npm run checkfor repo-wide validation/coverage changes — command/result:npm testwas attempted outside the sandbox, including a four-worker retry, but unchanged local tests failed or timed out; an isolated failure was caused by the host Python lackingyaml.npm run docsbuilds without warnings (doc changes only) — the build passed with two existing Fern warnings.GitHub Actions
Fresh exact-head CI and protected E2E evidence for
99e54157678400cfda425bdc9a1f0e531749e6eaare required. No prior-head result is carried forward and no check is waived.Signed-off-by: Senthil Ravichandran senthilr@nvidia.com
Summary by CodeRabbit
New Features
Tests
Documentation