Skip to content

test(e2e): accept idempotent Shields cleanup - #6838

Merged
cv merged 1 commit into
mainfrom
codex/fix-shields-idempotent-cleanup
Jul 14, 2026
Merged

cv merged 1 commit into
mainfrom
codex/fix-shields-idempotent-cleanup

Conversation

@cjagwani

@cjagwani cjagwani commented Jul 14, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

Broaden the live Shields cleanup assertion to accept the existing idempotent Lockdown is already active. success response. The cleanup still requires exit code 0, so real restore failures remain failures while a valid no-op restore is no longer misclassified.

Changes

  • Accept both successful shields up responses in the shields-config cleanup callback.
  • Keep the existing exit-code assertion as the authoritative success boundary.

Type of Change

  • 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

  • Tests added or updated for changed behavior
  • Existing tests cover changed behavior — justification:
  • Tests not applicable — justification:
  • Docs updated for user-facing behavior changes
  • Docs not applicable — justification: This changes only a live E2E assertion for two existing success messages; the documentation-writer review found no user-facing contract change.
  • 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: The assertion remains gated by exit code 0 and accepts only the two success messages already emitted by shields up; no production Shields behavior changes.
  • Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue:

Verification

  • PR description includes the DCO sign-off declaration and every commit appears as Verified in GitHub
  • Normal pre-commit, commit-msg, and pre-push hooks passed, or npm run check:diff passed when hooks were skipped or unavailable
  • Targeted behavior tests pass for the current change set, or tests are marked not applicable above — command/result or justification:
  • Applicable broad gate passed — npm test for broad runtime/test-harness changes; npm run check for repo-wide validation/coverage changes — command/result:
  • Quality Gates section completed with required justifications or waivers
  • No secrets, API keys, or credentials committed
  • npm run docs builds without warnings (doc changes only)
  • Doc pages follow the style guide (doc changes only)
  • New doc pages include SPDX header and frontmatter (new pages only)

Signed-off-by: Charan Jagwani cjagwani@nvidia.com

Summary by CodeRabbit

  • Tests
    • Improved end-to-end test reliability by accepting both valid messages when restoring shield protection during cleanup.

Signed-off-by: Charan Jagwani <cjagwani@nvidia.com>
@cjagwani cjagwani self-assigned this Jul 14, 2026
@coderabbitai

coderabbitai Bot commented Jul 14, 2026 •

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: a515237a-f771-4c27-8e28-d36ee5c3d654

📥 Commits

Reviewing files that changed from the base of the PR and between 2adc848 and c8b05e5.

📒 Files selected for processing (1)
  • test/e2e/live/shields-config.test.ts

📝 Walkthrough

Walkthrough

The live E2E shields cleanup assertion now accepts either active-lockdown status message variant using a regular expression.

Changes

Shields cleanup validation

Layer / File(s) Summary
Accept active-lockdown output variants
test/e2e/live/shields-config.test.ts
The cleanup assertion accepts both "Lockdown active" and "Lockdown is already active" output variants.

Estimated code review effort: 1 (Trivial) | ~2 minutes

Possibly related PRs

Suggested labels: bug-fix, NV QA, area: sandbox

Suggested reviewers: cv

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly reflects the E2E cleanup behavior change to allow idempotent Shields restores.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch codex/fix-shields-idempotent-cleanup

Comment @coderabbitai help to get the list of available commands.

@github-code-quality

github-code-quality Bot commented Jul 14, 2026 •

Copy link
Copy Markdown
Contributor

Code Coverage Overview

Languages: TypeScript

TypeScript / code-coverage/plugin

The overall coverage remains at 96%, unchanged from the main branch.


Updated July 14, 2026 07:58 UTC
Code Coverage is in Public Preview. Learn more and provide us with your feedback.

@cv
cv merged commit ac97fdc into main Jul 14, 2026
146 of 150 checks passed
@cv
cv deleted the codex/fix-shields-idempotent-cleanup branch July 14, 2026 07:58
@github-actions

github-actions Bot commented Jul 14, 2026 •

Copy link
Copy Markdown
Contributor

PR Review Advisor — Informational

Advisor assessment: Informational / low confidence
Primary next action: No advisor follow-up required beyond maintainer review.
Findings: 0 blockers · 0 warnings · 0 optional suggestions
Status: PR review advisor failed: PR review advisor SDK execution failed: session: tests-regressions-analysis omitted required analysis; turn: tests-regressions-analysis: tests-regressions-analysis omitted required analysis

Model lanes

  • GPT-5.6 Terra (primary): Failed
  • Nemotron 3 Ultra (non-blocking second opinion): Completed · high confidence · 0 blockers · 0 warnings · 0 suggestions

Nemotron is a non-blocking second opinion. Its prose, findings, and E2E guidance do not change the primary assessment above and remain in workflow artifacts only.

E2E guidance

Advisory only: coverage and selector recommendations are non-authoritative. E2E / PR Gate independently computes and dispatches trusted jobs without consuming this output.

Recommended coverage: cloud-onboard, credential-sanitization, security-posture, shields-config
Recommended selectors: cloud-onboard, credential-sanitization, security-posture, shields-config

  • cloud-onboard — Selected from the trusted checked-in E2E coverage inventory.

  • credential-sanitization — Selected from the trusted checked-in E2E coverage inventory.

  • security-posture — Selected from the trusted checked-in E2E coverage inventory.

  • shields-config — Selected from the trusted checked-in E2E coverage inventory.

  • cloud-onboard — Selected as a trusted checked-in E2E job.

  • credential-sanitization — Selected as a trusted checked-in E2E job.

  • security-posture — Selected as a trusted checked-in E2E job.

  • shields-config — Selected as a trusted checked-in E2E job.

Workflow run details

This is an automated, non-authoritative review. Findings are inputs to maintainer adjudication. Warnings and optional suggestions do not require a response or follow-up. A human maintainer makes the final merge decision.

@wscurran wscurran added the area: e2e End-to-end tests, nightly failures, or validation infrastructure label Jul 29, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: e2e End-to-end tests, nightly failures, or validation infrastructure

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants