test(e2e): validate #8304 failed-startup recovery - #8874
Conversation
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Signed-off-by: Apurv Kumaria <akumaria@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:
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe E2E recovery scenario now uses the supported ChangesShields-down recovery
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
E2E Target Results — ❌ Some tests failedRun: 31578289319
|
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in commit 79b41c3 in the TypeScript / code-coverage/cliThe overall coverage in commit 79b41c3 in the Show a code coverage summary of the most impacted files.
Updated |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@scripts/checks/openshell-policy-mutation-read.mts`:
- Around line 522-526: Update POLICY_READ_RUNNERS handling in
policyReadFailureHandling so the generic identifier run is accepted only when
symbol resolution confirms it refers to src/lib/runner.ts run; keep the
explicitly named policy-read runners unchanged. Add focused tests covering the
real repository runner and a shadowed local run, ensuring guardrails avoid false
positives.
In `@src/lib/shields/index.ts`:
- Around line 4918-4924: The deferred policy flow around deferredPolicyConfirmed
must not enter config-unlock recovery when the direct sandbox is absent. In
test/helpers/shields-flow-harness.ts, update deferredPolicyContainerMissing so
every direct command fails; in src/lib/shields/policy-transition.test.ts, add
the deferred completion coverage asserting the supported fail-closed result and
no follow-up direct mutation. Update the src/lib/shields/index.ts flow as needed
so absence detection does not invoke direct privileged execution or
unlockAgentConfig.
In `@src/lib/shields/relock-reconfirm.test.ts`:
- Around line 46-78: Extend the parameterized rejection cases in
confirmDeferredPermissivePolicyForStoppedSandbox to include an accepted active
receipt state alongside pending, and add output-level fixtures containing
duplicate Version, Hash, and Status receipt fields. Assert each case rejects
while preserving the existing behavioral test structure.
🪄 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: 377e4233-227e-48cd-92eb-8027c1ac0781
📒 Files selected for processing (8)
scripts/checks/openshell-policy-mutation-read.mtssrc/lib/sandbox/privileged-exec.test.tssrc/lib/sandbox/privileged-exec.tssrc/lib/shields/index.tssrc/lib/shields/policy-transition.test.tssrc/lib/shields/relock-reconfirm.test.tssrc/lib/shields/relock-reconfirm.tstest/helpers/shields-flow-harness.ts
| it.each([ | ||
| ["a live sandbox", false, policyReceipt(), timedOutPolicySet()], | ||
| ["a mismatched hash", true, policyReceipt("aaaaaaaaaaaa"), timedOutPolicySet()], | ||
| ["a mismatched version", true, policyReceipt(SUBMITTED_POLICY_HASH, "12"), timedOutPolicySet()], | ||
| [ | ||
| "a rejected receipt", | ||
| true, | ||
| policyReceipt(SUBMITTED_POLICY_HASH, "11", "rejected"), | ||
| timedOutPolicySet(), | ||
| ], | ||
| [ | ||
| "submission without a timeout", | ||
| true, | ||
| policyReceipt(), | ||
| timedOutPolicySet({ stderr: "policy rejected\n" }), | ||
| ], | ||
| [ | ||
| "duplicate submission evidence", | ||
| true, | ||
| policyReceipt(), | ||
| timedOutPolicySet({ | ||
| stdout: `Policy version 11 submitted (hash: ${SUBMITTED_POLICY_HASH})\nPolicy version 11 submitted (hash: ${SUBMITTED_POLICY_HASH})\n`, | ||
| }), | ||
| ], | ||
| ])("rejects %s (#8304)", (_name, stopped, receipt, policySetResult) => { | ||
| expect( | ||
| confirmDeferredPermissivePolicyForStoppedSandbox(policySetResult, { | ||
| proveNoRunningDirectSandbox: () => stopped, | ||
| readPolicyReceipt: () => receipt, | ||
| parsePolicy: () => "version: 1\nnetwork_policies: {}", | ||
| }), | ||
| ).toBe(false); | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Cover both permitted receipt states and duplicate receipt fields.
The success case covers pending, but the helper also accepts active. The rejection cases do not cover duplicate Version, Hash, or Status receipt fields. Add output-level cases for these conditions.
As per path instructions, tests must give behavioral confidence.
🤖 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 `@src/lib/shields/relock-reconfirm.test.ts` around lines 46 - 78, Extend the
parameterized rejection cases in
confirmDeferredPermissivePolicyForStoppedSandbox to include an accepted active
receipt state alongside pending, and add output-level fixtures containing
duplicate Version, Hash, and Status receipt fields. Assert each case rejects
while preserving the existing behavioral test structure.
Source: Path instructions
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
PR Review Advisor — No blocking findings reportedAdvisor assessment: No blocking advisor findings reported Model lanes
3 terminology differences from the second opinionAdvisory only. These are normalized differences from the primary terminology receipt.
1 additional E2E selection from the second opinionAdvisory only. The primary lane did not select these E2E jobs or targets.
Second-opinion terminology and E2E selections are advisory. Live E2E does not run automatically for pull requests. 3 semantic terminology decisionsTerminology decisions are advisory. They affect the assessment only when a separate finding identifies concrete semantic impact.
E2E guidanceAdvisory only. A maintainer can dispatch the default E2E suite for the commit under review. Recommended E2E: None Manual-only E2E: This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge. |
cv
left a comment
cv
left a comment
There was a problem hiding this comment.
src/lib/shields/index.ts:4918-4994confirms the deferred case only after proving that no direct container runs, but then unconditionally callsunlockAgentConfig, whose OpenClaw preflight requires privileged direct-container execution. Production therefore throws the same missing-container error and rolls back, soshields downstill cannot recover #8304.test/helpers/shields-flow-harness.ts:321-333hides this behavior by failing only the/usr/bin/trueprobe and fabricating argv for later unlock calls; thereforesrc/lib/shields/policy-transition.test.ts:179-195is invalid regression evidence. Implement a stopped-sandbox completion that does not require direct-container execution after absence is proved, or defer the unlock behind a verified later container lifecycle. Make missing-container mode fail every direct command and assert that no impossible follow-up mutation occurs. 2.scripts/checks/openshell-policy-mutation-read.mts:522-560accepts every identifier namedrunas a policy runner without resolving its symbol. A local or shadowedruncan be misclassified and weaken the exhaustive policy-mutation error-handling audit. Resolveruntosrc/lib/runner.tsand add regression tests for the repository runner and a shadowed local runner. 3.src/lib/shields/relock-reconfirm.test.ts:46-78does not test a nonzero receipt read, the acceptedactivestate, or duplicate receipt fields. This security admission boundary could regress to accepting failed or ambiguous evidence without a focused test failing. Add positive and negative receipt tests; the PR claim that failed reads and duplicate malformed evidence are covered is not supported by the current tests.
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
apurvvkumaria
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/e2e/live/shields-config.test.ts`:
- Around line 1146-1163: The live-child recovery scenario must exercise the
supported CLI refusal contract instead of calling
runInstalledFailedStartupUnlock directly. Update the live-child case to invoke
runNemoclaw with the shields down command and its relevant arguments, then
assert the expected refusal result and message while preserving the existing
childless recovery coverage.
🪄 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: 1aad73ec-c742-4950-9509-e08867c693cf
📒 Files selected for processing (1)
test/e2e/live/shields-config.test.ts
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
wscurran
added
area: e2e
|
Security review: PASS — no findings. The current PR diff is limited to one live E2E file that exercises the supported Shields recovery command. The earlier production-code review threads are outdated because those changes are now in
Validation: required checks, DCO, documentation receipt, and commit verification pass. Repository hooks, the test-title gate, and the test-size gate also pass. The earlier human changes-requested review remains the only external gate and needs reviewer re-evaluation. No reviewer-routing action was taken. |
The latest PR commit removes the reviewed production workaround and related synthetic evidence. The remaining E2E exercises the supported recovery path. Dismissing this resolved review before a full current-commit review.
…cle-followup Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
|
Readiness update after refreshing from current
Fresh repository checks are running. The outstanding human review needs re-evaluation against the current one-file diff; no reviewer-routing action was taken. |
rsliter
left a comment
rsliter
left a comment
There was a problem hiding this comment.
I reviewed the effective one-file test diff and linked issue. All 50 current checks pass on latest PR commit 79b41c381e, GitHub reports MERGEABLE, contributor compliance passes, and the security and documentation reviews have no findings.
5 participants
Summary
The live Shields acceptance now exercises the supported failed-startup recovery path through
nemoclaw <sandbox> shields downwhile the managed container still exists. It no longer unlocks the container directly and then asks the host to reconcile a stopped-container policy transition.The reported Shields-up stop/start lifecycle already passes on current
mainwith the merged lifecycle fixes. This PR keeps that regression coverage, makes the later failed-startup phase prove the public atomic policy-and-config transition before restart and relock, and gives the post-start health assertion a bounded convergence window without weakening its required healthy state.Related Issue
Fixes #8304
Changes
shields downcommand.policy set --wait, then pauses the supervisor and terminates the exact startup child immediately before the guarded config transition. This preserves real policy acknowledgement while making the terminal failed-startup boundary deterministic.statusresult within a fixed bound so transient inference convergence does not obscure a successful lifecycle recovery; the assertion still requires exit 0 andPhase: Ready.Type of Change
Quality Gates
Documentation Writer Review
no-docs-neededtest/e2e/live/shields-config.test.tsat reviewed blob5ecf9810521b757583c066bf7ead1a3f99982eb8; the fixture changes trusted E2E timing after real policy acknowledgement but does not change production commands, output, flags, defaults, schemas, health definitions, or the documented recovery workflow.DGX Station Hardware Evidence
Verification
Signed-off-by:line and every commit appears asVerifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passednpx vitest run --project cli src/lib/shields/openclaw-transition.test.ts src/lib/shields/policy-transition.test.ts— 55 passednpx vitest run --project e2e-support test/e2e/support/shields-failed-startup.test.ts— 10 passednpm run typecheck:clinpm run test:projects:checknpm run test-size:checknpx prek run --files test/e2e/live/shields-config.test.ts83f680b6fpassed all applicable jobs.83f680b6fpassed with recommendationmerge_as_is; both model lanes completed with high confidence and reported no blockers, warnings, or suggestions.f0c7bf79epassedshields-config, including the real policy acknowledgement, supported childless Shields recovery, stop/start, bounded healthy-status convergence, and relock. The signed merge-only refresh to83f680b6fpreserves the reviewed one-file effective diff byte-for-byte (SHA-2560aa00af5f3bcbcde2617ff31a5466836882fa3ca8921d563fee968c820b76b84). The broad matrix completed with unrelated failures outside this one-file test diff; its protected managed-image GPU failure reproduced with the same phase, probe, and assertion on unchanged-current-main run 31585922181.shields-configas a selective exact-revision target. The exact-head advisor independently recommends no E2E and classifiesshields-configas manual-only from reviewed code onmain; no rerun of the known-noisy broad matrix was warranted.test/install-station-pair-preparation.test.ts; its complete 58-test file passes locally, and CI run 31603167697 passed on the one allowed failed-job rerun.npm run docsbuilds without warnings (doc changes only)Signed-off-by: Apurv Kumaria akumaria@nvidia.com