Repository navigation
fix(e2e): approve admin scope before portable readiness - #12760
Merged
Merged
Conversation
Signed-off-by: Rebecca Sliter <571084+rsliter@users.noreply.github.com>
|
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. |
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (6)
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthroughThe approval fixture now runs its verified script through ChangesAdmin approval test fixture
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix Suggested reviewers: Merge Risk: ⚪ Minimal · up to The approval path retains exact-request selection, and no issue requiring a fix before merge was identified. 🚥 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 4 functions across 5 files. (1 skipped: 1 unsupported.)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
Contributor
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall line coverage in commit 2446ac9 in the Show a line coverage summary of the most impacted files.
TypeScript / code-coverage/cliThe overall line coverage in commit 2446ac9 in the Show a line coverage summary of the most impacted files.
Updated |
Signed-off-by: Rebecca Sliter <571084+rsliter@users.noreply.github.com>
Contributor
|
PR Review Advisor finished for commit Request review only when Require no Advisor blockers is green. |
rsliter
marked this pull request as ready for review
October 7, 2026 16:52
prekshivyas
approved these changes
Oct 7, 2026
prekshivyas
added a commit
that referenced
this pull request
Oct 7, 2026
Coverage counts native CommonJS and Vitest calls against the same original TypeScript locations. The existing V8 collector can merge incompatible generated offsets and lose executed native calls. This prevents the protected E2E controller prerequisite for #12376 from meeting its unchanged coverage floor despite passing its behavioral tests. Instrument original source before either loader transforms it, share counters across loaders, and merge only matching counter maps. Preserve reporting, shard transport, coverage floors, and uncovered code. Extract the existing native compiler/cache so instrumentation can be prepared before test timers without evaluating application code. Added dependencies are development-only. The review repair addresses: - Provider loading: externalize the TypeScript CommonJS collector in the root configuration as well as the project configuration. Exercise the real root/project configuration, shard merging, and conflicting maps. - Source selection: match include and exclude patterns independently against relative and absolute paths, reject node_modules, and retain external-file and changed-file selection rules. Remove substring matching that incorrectly included excluded paths. - Test isolation: restore the reservation fixture's isolated HOME before each of its eight existing cases. Install the warm-cache instrumenter guard before importing the loader and verify its cache remains empty. - Lifecycle fixtures: distinguish exited zombies from live gateway processes and close the synthetic listener before exiting. Replace same-size model files atomically so filesystem-identity tests cannot depend on timestamp granularity. Production lifecycle behavior, existing assertions, deadlines, and cleanup requirements are unchanged. Validation evidence for the repair: - Focused regressions: 53 cases pass, covering 32 source-coverage, 13 loader, and eight reservation cases. Root-provider loading also has an observed failing reproduction followed by a 23-case passing run. - Gateway fixture: 20 repeated lifecycle trials pass; final process-state handling passes 12 cases, all seven growth regressions, 18 repository checks, lint, and formatting. The preceding full run passed this gateway fixture. - Model identity fixture: reuse the exact previously validated repair, with 68 file tests, 300 repetitions, and an omitted-replacement negative control. - Full normal `npm run check` passed for the exact 16-file proposal on canonical main `bced57c`: all-file pre-commit hooks, full CLI/plugin suites, repository checks, growth checks and unchanged coverage floors. All 79 inputs (including incorporated main changes relative to the isolated snapshot) remained unchanged. CLI coverage: 85.74% lines, 84.04% statements, 87.82% functions, 78.26% branches. Plugin: 96.66%, 95.91%, 99.53%, 89.31%, respectively. Successful hooks suppress individual test totals, so no new case count is inferred. The latest two-file repair shares each coverage fixture's existing test deadline across subprocesses and reports subprocess errors directly. No timeout was increased. The E2E host fixture now imports the same canonical gateway endpoint function directly, avoiding an unrelated lifecycle import graph before listener identity checks; ownership and executable assertions remain intact. Validation for this repair: all 32 coverage tests and six real subprocess deadline probes pass; 190 combined host/gateway/growth tests, all 18 repository checks, lint, and formatting pass. A controlled diagnostic passed all 134 host cases with each import path, reducing the first identity case from 3.55 seconds to 27 milliseconds. This supports the import repair but is not a claim that the CI timeout was reproduced locally. The full check above covers the preceding exact 16-file proposal; these bounded repairs have focused validation and normal commit/publication hooks. The prior candidate `e2784a5` completed all scheduled feedback: [core CI](https://github.com/NVIDIA/NemoClaw/actions/runs/37582908985) had one underlying failure (the first mocked listener identity case exceeded its existing 5-second deadline); the other 11 CLI shards passed. [Managed activation](https://github.com/NVIDIA/NemoClaw/actions/runs/37582908922) passed on Docker (28 turns, three agents, 13 cleanup actions) and rootless Podman (19 turns, three agents, nine cleanup actions), with no build commands or cleanup failures. Docker exercised external images; that Docker-only path was unrun on Podman. CodeRabbit's remaining fixture deadline finding is addressed by this repair and awaits independent review. Current candidate `39164ed5501f12260326d2d63a8937aecb428408` has [green core CI](https://github.com/NVIDIA/NemoClaw/actions/runs/37586950071), including all 12 CLI shards and the merged coverage gate. All nine specialists and the no-blockers gate in the [full Advisor review](https://github.com/NVIDIA/NemoClaw/actions/runs/37588790596) passed. Actual CodeRabbit review completed; the reviewer withdrew both deadline findings and resolved [the current thread](#12713 (comment)) and [the older thread](#12713 (comment)). All four actionable review threads are resolved by CodeRabbit. Optional performance and diagnostic suggestions remain documented. [Managed validation](https://github.com/NVIDIA/NemoClaw/actions/runs/37586950142) passed on Docker (28 turns, three agents, 13 cleanup actions) and rootless Podman (19 turns, three agents, nine cleanup actions), with no build commands or cleanup failures. Docker exercised external-image onboarding, identity-drift refusal, rebuild, and destruction; that Docker-only path was unrun on Podman. Public restarts and native OpenClaw approval passed. The previous candidate `39164ed5501f12260326d2d63a8937aecb428408` completed [full E2E run 37590927253](https://github.com/NVIDIA/NemoClaw/actions/runs/37590927253), including four attempts on that unchanged head and base `bced57c38baef60e74e0142378a74ae8a7b7ff73`. The final attempt had 78 successful jobs, one executed leaf failure ([OpenClaw provider switching](https://github.com/NVIDIA/NemoClaw/actions/runs/37590927253/job/112926435295)), one aggregate failure, and 16 skipped jobs. Its targeted [Hermes GPU compatibility job](https://github.com/NVIDIA/NemoClaw/actions/runs/37590927253/job/112926429726) passed all ten runtime assertions and teardown. The OpenClaw leaf ran before merged [#12720](#12720), whose focused 12-phase, six-cleanup validation passed separately. The approved Launchable opt-out remains a skip, not a passing test. Earlier attempt failures are retained in the run history; the final attempt does not waive them for a different head. The preceding candidate `d9dd577460faa972fc2c7feaadc30f749b822b94` completed [managed validation](https://github.com/NVIDIA/NemoClaw/actions/runs/37666130863): Docker and rootless Podman both passed. Its [core CI](https://github.com/NVIDIA/NemoClaw/actions/runs/37666130821) remained red. In attempt 2, 11 of 12 CLI shards passed; shard 8 produced no output after starting the pinned Pi search-tool `apt-get update` and was cancelled at the existing 30-minute job limit. The coverage test never ran in that shard. This log identifies the stalled step, not its underlying network or lock cause. The earlier shard 6 deadline failure cleared on attempt 2. No green-core claim is made for `d9dd577`. Previous candidate `c315c921f2359597670b33296fc6a7e5a2d4a7ad` is a signed, GitHub-verified merge of `d9dd577` with main `bdac003d725b5a21f3c8ce61ba39ebbf6d3c3ff7`, incorporating merged #12720 and #12760. It bounds the existing pinned search-tool package-index update to three 120-second attempts with 20-second transport timeouts, and the pinned install to 180 seconds. Package versions, binary assertions, coverage floors, test deadlines, and the 30-minute shard limit are unchanged. Local checks passed: 18 repository checks, YAML and Bash syntax, a retry/fail-closed apt command probe, CLI build and typecheck, the normal signed commit hooks, and the normal pre-push publication gate. Eight focused source-coverage cases fail on local macOS with identical results on both `d9dd577` and the current-main integration; they are not claimed to pass. [Gate-true core CI run 37676044587](https://github.com/NVIDIA/NemoClaw/actions/runs/37676044587) then failed in CLI shard 9 because the workflow contract fixture still expected one unbounded `apt-get update` call and exit 86. The new action retries three times and exits 1 after its final error. Other substantive shards passed except shard 7, which was cancelled during apt setup. [Human review](#12713 (review)) requested this contract test update and another exact-head CI and Advisor pass. Current candidate `3b52b9ae4bf127bc327389fd4b944eedb5a84052` updates only that fixture: it verifies three bounded, identical update attempts, both retry notices, the final fail-closed error, and no install after update exhaustion. The separate Advisor runtime case still expects its single update call and original exit status. Focused integration tests pass (55 cases), selected repository checks pass (18), lint and formatting pass, and the normal `npm run validate:pr` and pre-push gates pass. The commit is signed and GitHub-verified. [Gate-true core CI run 37696446245](https://github.com/NVIDIA/NemoClaw/actions/runs/37696446245) passed on its second attempt at the unchanged head. The first attempt's sole originating failure was a 5,022.84 ms timeout against an existing 5,000 ms limit in an unchanged route-reservation test in shard 6; the other 11 CLI shards, including the repaired contract test in shard 9, passed. A user-authorized targeted rerun of only shard 6 (plus GitHub-required dependencies) passed, as did merged CLI coverage and the final required `checks` gate. No test deadline or coverage floor changed. [Managed-image validation](https://github.com/NVIDIA/NemoClaw/actions/runs/37696446254) passed both Pi architectures, direct managed startup, and Docker and rootless Podman all-agent activation. [Self-hosted PR qualification](https://github.com/NVIDIA/NemoClaw/actions/runs/37696449223) passed, including the generic NVIDIA GPU llama.cpp test. [CodeRabbit's exact-head review](#12713 (review)) found no actionable inline issue; its argument-order suggestion is a non-blocking nitpick. The [exact-head Advisor run](https://github.com/NVIDIA/NemoClaw/actions/runs/37702278216) passed with all nine specialists clear, zero findings, and a successful no-blockers gate; its [receipt](#12713 (comment)) is published. All required checks pass. Human review approval remains required; the PR is not approved or merged. This prerequisite does not establish that the protected controller or original #12376 full E2E passes. Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Gateway mutation locks now reject blank gateway names and invalid router ports before running mutation callbacks. * **Tests** * Expanded checks for gateway validation, lifecycle behavior, approval flows, and source coverage, including cached modules, report merging, coverage toggling, and thresholds. Improved test setup and coverage reporting checks to better detect regressions. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com> Signed-off-by: Rebecca Sliter <571084+rsliter@users.noreply.github.com> Co-authored-by: Rebecca Sliter <sliterrm@gmail.com> Co-authored-by: Rebecca Sliter <571084+rsliter@users.noreply.github.com>
wscurran
added
area: e2e
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Outcome
Managed-image admin approval now reaches its prepared shell when portable OpenClaw has the expected pending scope upgrade. The fixture uses the trusted OpenShell shell transport directly, so NemoClaw readiness cannot reject the intentionally unsettled request before approval.
Reason
PR #12538 added exact-request filtering but still opened the approval shell through
nemoclaw connect. Portable readiness correctly rejects an unsettled admin request, so the rootless main workflow failed before the prepared approval ran.Related issues
Refs #12538.
Changes
nemoclaw connectpath remains unchanged for other consumers.nemoclaw connectcannot be its entry point during an intentionally pending approval.The live contract remains in
test/e2e/live/managed-image-activation-e2e.test.ts. Its real boundary is the OpenShell interactive shell with portable pairing pending. It proves that the captured admin request is approved and the downstream OpenClaw operation succeeds. Deterministic payload and selector behavior stays intest/security/admin-approval-helper.test.tsandtest/e2e/support/managed-image-activation-diagnostics.test.ts.Verification
npx vitest run --project integration test/security/admin-approval-helper.test.ts— 31 tests passed.npx vitest run --project e2e-support test/e2e/support/managed-image-activation-diagnostics.test.ts— 28 tests passed.npm run typecheck:cli— passed.npm run validate:pr— passed.Signed-off-by: Rebecca Sliter 571084+rsliter@users.noreply.github.com