Skip to content

test(shields): remove external status assertion - #8277

Closed
prekshivyas wants to merge 2 commits into
mainfrom
codex/fix-shields-ci-status-assertion
Closed

test(shields): remove external status assertion#8277
prekshivyas wants to merge 2 commits into
mainfrom
codex/fix-shields-ci-status-assertion

Conversation

@prekshivyas

@prekshivyas prekshivyas commented Aug 5, 2026

Copy link
Copy Markdown
Collaborator

Summary

The Shields auto-restore unit test introduced by #8238 fails in CI because it asserts an external OpenShell process status. Its ESM helper mocks do not intercept the CommonJS coordinator. This change scopes the test to the deadline-restoration invariant that it can observe: an unchanged snapshot with no managed MCP entries does not require temporary staging.

Changes

  • Rename the test to state the no-staging invariant.
  • Remove assertions against the external process result and an ESM policy mock that the CommonJS coordinator does not consume.
  • Retain the simulated ENOSPC trap and verify that fs.mkdtempSync is not called.

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 test scope and evidence only. It does not change user-visible behavior.
  • 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: Independent Codex Desktop review of exact head e5f62afb4feb64dc29dd5a2dfdc7d1f21a17bf4f against base 15b0c553d7cd56f5dd20298a4e9d1daa2a80efc1 passed all nine required categories with no findings. The diff changes no runtime source, input boundary, credential flow, file operation, network behavior, permission, cryptography, or dependency. It retains the ENOSPC negative path and no-staging assertion.
  • Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue:

Documentation Writer Review

  • Documentation writer subagent reviewed the completed changes
  • Result: no-docs-needed
  • Evidence: Independently reviewed the complete effective diff in src/lib/shields/index.test.ts at head SHA e5f62afb4feb64dc29dd5a2dfdc7d1f21a17bf4f against base SHA 15b0c553d7cd56f5dd20298a4e9d1daa2a80efc1. The refreshed head is a merge-only update whose parents are the previously reviewed head and the exact current base. The effective diff is byte-identical to the previously reviewed change. It renames the test to describe the asserted staging invariant and removes invalid assertions against external process status and an ESM helper that the CommonJS coordinator does not consume. It retains the simulated ENOSPC trap and verifies that fs.mkdtempSync is not called. This test-only correction does not change a user-visible API, CLI, configuration, workflow, default, error, or supported behavior. npm run build:policy-boundary, the 34-test focused CLI source-test run, npm run test:titles:check, commitlint, and the pre-push TypeScript checks passed.
  • Agent: Codex Desktop

DGX Station Hardware Evidence

  • Tested on DGX Station
  • Tested commit: Not applicable; scripts/prepare-dgx-station-host.sh is unchanged.
  • Station profile/scenario: Not applicable.
  • Result: Not applicable.
  • Supporting evidence: Not applicable.

Verification

  • PR description includes a Signed-off-by: line and every commit appears as Verified in GitHub
  • Normal pre-commit, commit-msg, and pre-push hooks passed, or npm run validate:pr passed after refreshing origin/main 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: npm run build:policy-boundary passed. npx vitest run --project cli src/lib/shields/index.test.ts passed 34/34 tests. npm run test:titles:check passed.
  • 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: Prekshi Vyas prekshiv@nvidia.com

@prekshivyas prekshivyas self-assigned this Aug 5, 2026
@coderabbitai

coderabbitai Bot commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

The test now covers unchanged shield policy snapshots without managed MCP entries. It directly invokes applyShieldsPolicySnapshot and verifies that the restore path skips temporary-directory creation.

Changes

Shield snapshot restore

Layer / File(s) Summary
Restore behavior test
src/lib/shields/index.test.ts
Renames the test and replaces policy-application assertions with a check that applyShieldsPolicySnapshot does not create temporary storage for snapshots without managed MCP entries.

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

Possibly related PRs

  • NVIDIA/NemoClaw#8200: Both changes cover shield policy snapshot restore behavior and snapshots without managed MCP entries.
  • NVIDIA/NemoClaw#8238: This PR tests behavior introduced by the related shield policy application changes.

Suggested labels: area: sandbox

Suggested reviewers: cv, jyaunches

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
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.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main test change: removal of the external status assertion.
✨ 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-ci-status-assertion

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

@github-code-quality

github-code-quality Bot commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Code Coverage Overview

Languages: TypeScript

TypeScript / code-coverage/plugin

The overall coverage in commit e5f62af in the codex/fix-shields-ci... branch remains at 96%, unchanged from commit 5fe2319 in the main branch.

TypeScript / code-coverage/cli

The overall coverage in commit e5f62af in the codex/fix-shields-ci... branch remains at 81%, unchanged from commit 4e49bf1 in the main branch.

Show a code coverage summary of the most impacted files.
File main 4e49bf1 codex/fix-shields-ci... e5f62af +/-
src/lib/shields...sive-runtime.ts 96% 86% -10%
src/lib/actions...e-validation.ts 84% 80% -4%
src/lib/inferen...ompatibility.ts 94% 94% 0%
src/lib/shields/timer.ts 80% 80% 0%
src/lib/actions...l-validation.ts 95% 95% 0%
src/lib/sandbox...rce-identity.ts 87% 87% 0%
src/lib/credentials/store.ts 55% 56% +1%
src/lib/shields/index.ts 71% 73% +2%
src/lib/actions...ridge-policy.ts 66% 71% +5%
src/lib/shields...y-transition.ts 0% 99% +99%

Updated August 05, 2026 03:48 UTC

@github-actions

github-actions Bot commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

PR Review Advisor — No blocking findings reported

Advisor assessment: No blocking advisor findings reported
Next action: No advisor follow-up needed.
Findings: 0 blockers · 0 warnings · 0 suggestions

Model lanes

  • GPT-5.6 Terra (primary): Completed · high confidence · 0 blockers · 0 warnings · 0 suggestions
  • Nemotron 3 Ultra (second opinion): Completed · high confidence · 0 blockers · 0 warnings · 0 suggestions
  • Model comparison: normalized findings match; normalized terminology decisions differ; normalized E2E selections differ; severity counts match.
5 terminology differences from the second opinion

Advisory only. These are normalized differences from the primary terminology receipt.

  • unchanged at src/lib/shields/index.test.ts:595: selected only by the second-opinion lane as justified.
  • stage at src/lib/shields/index.test.ts:595: selected only by the second-opinion lane as justified.
  • snapshot at src/lib/shields/index.test.ts:595: selected only by the second-opinion lane as established.
  • auto-restore at src/lib/shields/index.test.ts:595: selected only by the second-opinion lane as established.
  • managed MCP at src/lib/shields/index.test.ts:595: selected only by the second-opinion lane as established.
1 additional E2E selection from the second opinion

Advisory only. The primary lane did not select these E2E jobs or targets.

  • hermes-shields-config: The completed second-opinion lane identified E2E coverage that the primary lane omitted.

Second-opinion terminology and E2E selections are advisory. They do not change the primary assessment or E2E / PR Gate.

1 semantic terminology decision

Terminology decisions are advisory. They affect the assessment only when a separate finding identifies concrete semantic impact.

  • justified — unchanged snapshot at src/lib/shields/index.test.ts:595: Keep "unchanged snapshot" for this test invariant because it distinguishes the no-staging path.

E2E guidance

Advisory only. E2E / PR Gate selects and runs jobs independently.

Recommended E2E: None

1 optional E2E recommendation
  • shields-config

Workflow run details

This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge.

@prekshivyas

Copy link
Copy Markdown
Collaborator Author

Closing as superseded by merged #8293 (head bdf9d11; merge commit 5fe2319). #8293 covers the accepted outcome more directly: it removes the same invalid external OpenShell status and ESM-helper assertions, then tests empty managed-MCP snapshot reuse through buildDeadlineRuntimeManagedMcpPolicy while proving that temporary staging is not invoked. It also preserves Carlos Villela as author of the Shields commits and as a co-author of the squash result. No attribution from #8277 needs to be substituted for that contributor history.

@prekshivyas prekshivyas closed this Aug 5, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant