Repository navigation
test(e2e): keep EXDEV forwards on canonical OpenShell - #11552
Conversation
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (6)
Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review. 📝 WalkthroughWalkthroughThe EXDEV test wrapper now intercepts only ChangesEXDEV command routing
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix Sequence Diagram(s)sequenceDiagram
participant OpenClawCLI
participant NodePreloadInterceptor
participant FixtureExecutable
participant CanonicalOpenShellCLI
OpenClawCLI->>NodePreloadInterceptor: run sandbox create
NodePreloadInterceptor->>FixtureExecutable: redirect create command
OpenClawCLI->>CanonicalOpenShellCLI: run forward or list
Suggested reviewers: Merge Risk: ⚪ Minimal · up to The EXDEV test routing change preserves canonical OpenShell handling outside sandbox creation, with the changed flows covered by focused validation. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 4 files. (2 skipped: 2 unsupported.)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall line coverage in commit 855d5fc in the Show a line coverage summary of the most impacted files.
TypeScript / code-coverage/cliThe overall line coverage in commit 855d5fc in the Updated |
|
Verification evidence for commit
The change keeps the canonical OpenShell executable for forward ownership verification. Only sandbox creation uses the fixture's image and tmpfs wrapper. Production code is unchanged. The E2E dispatch receipt identifies the tested commit above, base Advisor review remains pending. Attempt 1 failed before producing any of the nine specialist reviews. Each job reported |
cjagwani
left a comment
cjagwani
left a comment
There was a problem hiding this comment.
Approved: the create-only preload keeps image/tmpfs rewriting on sandbox creation while forward and list operations retain canonical OpenShell identity. Focused E2E-support validation passed 27/27, and the exact trusted EXDEV lifecycle passed onboarding, restart, recreation, canonical listener ownership, and cleanup. Required checks and commit verification are green.\n\n
<!-- markdownlint-disable MD041 --> ## Outcome This is the first architecture slice toward #11547. PR #11552 fixed the then-current EXDEV forward-ownership failure, and the target passed on `main` after that merge. This pull request does not claim another current test failure. The live target retains all seven behavior phases and framework cleanup. It still proves distinct filesystems, a real `openclaw plugins install --force`, restart persistence, recreation with the v2 fixture image, restored inference, and complete cleanup. The target falls from 658 to 585 lines. Its target-specific live surface falls from 1,202 to 1,140 lines. Direct assertion points fall from 17 to 16, transitive assertion points fall from 32 to 25, and generated probe blocks fall from three to zero. ## Reason The target was green after #11552, but it still owned deterministic command interception, parsing, validation, and generated probes. Focused support tests can prove those contracts faster and with less environmental noise. The live target should own only behavior that requires Docker, OpenShell, process, sandbox, and cross-filesystem boundaries. ### Related issues Refs #11547. Follow-up to #11552. This pull request intentionally leaves #11547 open for later reductions. ## Changes - Replace sandbox-create interception and tmpfs rewriting with a stable read-only host mount. Focused tests protect image-ID validation, extraction safety, mount construction, recreation command shape, and cleanup order. - Use one canonical OpenShell component set throughout the EXDEV lifecycle. The exact-main driver tests protect executable resolution and component composition. - Retain the proven recreation precondition. The target verifies listener ownership, terminates only the owned process, and requires port 18789 to release. - Remove the stale gateway-stop hook, generated probes, and incidental terminal assertions. Update selection ownership, mock parity, the assertion budget, and E2E documentation. - Canonicalize temporary fixture roots so symlinked temporary-directory aliases cannot fail source-path validation before Docker runs. ## Verification - `NODE_OPTIONS=--max-old-space-size=8192 npm run validate:pr` passed on commit `6f449a8d6b219385c2f7c9ee6b7d627013cca85a`. - Four focused E2E-support suites passed 134 tests. The EXDEV prebuild suite also passed all 30 tests with `TMPDIR` set to a symlink alias. - Mock-parity and PR risk-planning integration suites passed 226 tests. - `npm run e2e:assertions:check` passed with 1,787 direct `expect` calls across 85 live test files and target budget `[9,16,9,25,0]`. - [PR CI run 34878473841](https://github.com/NVIDIA/NemoClaw/actions/runs/34878473841) passed all 12 CLI shards, aggregate CLI tests, static checks, builds, type checks, plugin tests, installer integration, package checks, and audit checks. - Exact-commit CodeRabbit review completed with no unresolved substantive thread. - [PR Review Advisor run 34878570389](https://github.com/NVIDIA/NemoClaw/actions/runs/34878570389) verified all nine specialist artifacts. Every specialist reported clear, with no P0 or P1 finding and no additional E2E recommendation. - The required [manual PR E2E run 34879686547](https://github.com/NVIDIA/NemoClaw/actions/runs/34879686547) tested the same commit. The [EXDEV job](https://github.com/NVIDIA/NemoClaw/actions/runs/34879686547/job/104096644869) passed. Cloud inference, cloud onboarding, OpenClaw security posture, the credential window, and the OpenClaw and Deep Agents MCP jobs also passed. - GitHub reports all 22 branch commits as verified. - The diff contains no secrets, API keys, or credentials. ## Review notes The sensitive path `tools/e2e/workflow-boundary.mts` changes only job ownership for the new helper and wrapper paths. Independent security review found no credential, command-injection, cleanup, or selector issue. CodeRabbit's valid canonical-temporary-path finding was fixed before the latest review. `npm run review:local` could not start a specialist because its OpenShell gateway refused the configure connection. The exact-commit remote Advisor run completed all nine independent reviews instead. The manual E2E aggregate is red because two Hermes lanes hit inherited gateway readiness failures. The [Hermes security job](https://github.com/NVIDIA/NemoClaw/actions/runs/34879686547/job/104096645230) exhausted its 90-second readiness window before any security assertion. The exact-base [security job](https://github.com/NVIDIA/NemoClaw/actions/runs/34875021241/job/104081009481) passed with the same managed image. The [Hermes MCP job](https://github.com/NVIDIA/NemoClaw/actions/runs/34879686547/job/104096644736) quarantined gateway relaunch after planned restarts were counted as crashes. The exact-base first attempt [failed with the same signature](https://github.com/NVIDIA/NemoClaw/actions/runs/34875021241/job/104081001845), and its second attempt [passed](https://github.com/NVIDIA/NemoClaw/actions/runs/34875021241/job/104090873918). The candidate does not change Hermes, MCP runtime, installer, gateway supervisor, or security-posture paths. Cleanup removed every sandbox, state volume, tunnel, relay, and Docker credential. Artifact scans found no credential exposure. No unchanged rerun was requested because this signature has no checked-in retry policy. Issue #10977 tracks this timing-sensitive gateway-health family. Removing the final listener ownership and termination boundary requires a supported direct `ForwardTcp` stop or a separately accepted lifecycle repair under #11547. --- Signed-off-by: Rebecca Sliter <571084+rsliter@users.noreply.github.com> --------- Signed-off-by: Rebecca Sliter <571084+rsliter@users.noreply.github.com> Co-authored-by: Rebecca Sliter <571084+rsliter@users.noreply.github.com>
2 participants
Outcome
The custom-plugin EXDEV test can verify dashboard forward ownership during onboarding and recreation. Its image and tmpfs wrapper runs only for sandbox creation; forwarding uses the canonical OpenShell executable.
Reason
Main run 34587100109 failed onboarding after #11427 added forward ownership verification. The fixture selected a wrapper as its OpenShell executable, but the listener ran the real binary. Existing wrapper tests checked arguments without exercising that executable selection.
Related issues
Refs #6108. Regression from #11427.
Changes
sandbox createspawn through the existing image and tmpfs wrapper. A global executable override cannot preserve forward identity.Verification
npm run e2e:assertions:check: passed; existing live assertion budget unchanged.NODE_OPTIONS=--max-old-space-size=8192 npm run validate:prpassed on855d5fce999acab6b21260902a580a4aa8826888, using canonical main41c5625e8b831ed213cd5c381385973adc58659c. The larger heap is required by this host’s TypeScript check.855d5fce999acab6b21260902a580a4aa8826888, including all 12 CLI shards, coverage, static checks, builds, and type checks.passreceipt were verified against the unchanged PR.NVIDIA/NemoClaw(ownerNVIDIA, organization); candidate855d5fce999acab6b21260902a580a4aa8826888; basee6068115cc5e02e0d05abdb46ea4509138847617; trusted workflow70cfff5f946a9bb31d1147f78ffbda914a2efaa2. Selector:jobs=openclaw-plugin-runtime-exdev, empty targets, mock inference. Correlation:e40a0daf-e1bb-4a56-bfe0-711faf7da239./sandbox/.profile: Permission deniedfollowed byexec relay closed before the command reported an exit status; none produced review artifacts. The same startup failure occurs in the independent PR #11212 Advisor run. Keep this fixture fix unchanged; a maintainer decision is needed for the shared runtime blocker and subsequent full Advisor rerun.Signed-off-by: San Dang sdang@nvidia.com
Summary by CodeRabbit