Skip to content

test(onboard): cover empty policy resume contracts#7254

Merged
prekshivyas merged 4 commits into
NVIDIA:mainfrom
scarab-systems:scarab-systems/nemoclaw-6042-policy-presets
Jul 25, 2026
Merged

test(onboard): cover empty policy resume contracts#7254
prekshivyas merged 4 commits into
NVIDIA:mainfrom
scarab-systems:scarab-systems/nemoclaw-6042-policy-presets

Conversation

@scarab-systems

@scarab-systems scarab-systems commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

Summary

Adds regression coverage for existing policy-resume behavior. The tests verify
that an empty preset selection is not treated as applied and that enabling Slack
adds its required preset and requests reconciliation. Production behavior is
unchanged.

Related Issue

Relates to #6042

Changes

  • Exercise the real arePolicyPresetsApplied contract for an empty selection.
  • Exercise the real policy resume-selection contract when Slack becomes required
    after an empty recorded selection.
  • Remove handler tests whose mocks represented states the production contracts
    cannot produce.

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 is a test-only change; no command,
    output, configuration, workflow, schema, or supported behavior changes.
  • 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: Codex Desktop reviewed the policy and
    onboarding test changes. Production behavior is unchanged, and the tests use
    the real policy-application and reconciliation contracts.
  • 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: No documentation paths changed. The diff adds tests for existing
    empty-selection and required-preset reconciliation behavior; it does not
    change a command, output, configuration, workflow, schema, or supported
    behavior.
  • Agent: Codex Desktop

DGX Station Hardware Evidence

  • Tested on DGX Station
  • Tested commit:
  • Station profile/scenario:
  • Result:
  • Supporting evidence:

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 check:diff passed when hooks were skipped or unavailable —
    npm run check:diff passed for 47d3546c.
  • Targeted behavior tests pass for the current change set, or tests are
    marked not applicable above —
    npx vitest run --project cli src/lib/onboard/policy-resume-selection.test.ts
    passed 13 tests; npx vitest run --project integration test/onboard.test.ts
    passed 34 tests.
  • Applicable broad gate passed — npm test for broad runtime/test-harness
    changes; npm run check for repo-wide validation/coverage changes — not
    applicable to this test-only change; focused suites and npm run check:diff
    passed.
  • 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)

Disclosure: This PR was prepared with AI assistance under human direction and
review.


Signed-off-by: ScarabSystems scarab.systems@yahoo.com
Signed-off-by: Prekshi Vyas prekshiv@nvidia.com

Signed-off-by: ScarabSystems <scarab.systems@yahoo.com>
@copy-pr-bot

copy-pr-bot Bot commented Jul 20, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@coderabbitai

coderabbitai Bot commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

handlePoliciesState now prevents resume/skip behavior when recorded policy presets are empty. Regression tests verify normal setup, expanded preset selection, and resulting applied presets.

Changes

Policy preset resume guard

Layer / File(s) Summary
Guard empty policy preset resumes
src/lib/onboard/machine/handlers/policies.ts, src/lib/onboard/machine/handlers/policies.test.ts
Resume now requires non-empty recorded presets. Tests verify empty selections proceed through setup, including expansion to required presets, without invoking skip behavior.

Estimated code review effort: 2 (Simple) | ~10 minutes

Suggested reviewers: cv, laitingsheng

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Linked Issues check ✅ Passed The fix addresses #6042 by preventing the Policy Presets step from being skipped when recorded presets are empty.
Out of Scope Changes check ✅ Passed The changes are limited to the policy resume logic and its regression tests, with no obvious unrelated additions.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title is concise and clearly related to the empty policy resume edge case covered by the PR.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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

@github-actions

github-actions Bot commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

PR Review Advisor — Informational

Advisor assessment: Informational / high confidence
Next action: No advisor follow-up needed.
Findings: 0 blockers · 0 warnings · 0 suggestions
Status: No actionable findings remain in the canonical review ledger.

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 E2E selections differ; severity counts match.

Nemotron output stays in workflow artifacts and does not change the assessment above.

E2E guidance

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

Recommended E2E: cloud-onboard

1 optional E2E recommendation
  • onboard-resume

Workflow run details

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

Signed-off-by: ScarabSystems <scarab.systems@yahoo.com>
@scarab-systems
scarab-systems marked this pull request as ready for review July 20, 2026 18:39
@wscurran wscurran added area: cli Command line interface, flags, terminal UX, or output area: onboarding Onboarding FSM, provider setup, sandbox launch, or first-run flow bug-fix PR fixes a bug or regression labels Jul 20, 2026
@wscurran

Copy link
Copy Markdown
Contributor

✨ Thanks for the fix. This addresses the resume edge case where an empty policyPresets array caused the policies step to be skipped incorrectly. The regression coverage for empty recorded preset lists is a good addition.


Related open issues:

@prekshivyas prekshivyas self-assigned this Jul 25, 2026
@prekshivyas prekshivyas added v0.0.97 Release target v0.0.96 Release target and removed v0.0.97 Release target labels Jul 25, 2026
@prekshivyas prekshivyas changed the title fix(onboard): require policy presets before resume skip test(onboard): cover empty policy resume contracts Jul 25, 2026
@prekshivyas
prekshivyas merged commit ee7a7eb into NVIDIA:main Jul 25, 2026
69 of 71 checks passed
scarab-systems added a commit to scarab-systems/scarab-field-lab that referenced this pull request Jul 25, 2026
Record NVIDIA/NemoClaw#7406 as an accepted #7387 repair after upstream merge.

Refresh NVIDIA/NemoClaw#7254 as accepted test coverage for #6042 while noting the issue remains open and the merged PR was test-only.

Refresh NVIDIA/NemoClaw#7291/#7273 as closed without merge so the Field Lab no longer reports that PR as open.

Verification: gh pr/issue views for #7254, #7406, #7291, #6042, #7387, and #7273; git diff --cached --check; public leakage scan for local/private/SDS terms.
@scarab-systems
scarab-systems deleted the scarab-systems/nemoclaw-6042-policy-presets branch July 25, 2026 20:22
@scarab-systems

Copy link
Copy Markdown
Contributor Author

Woohoo! Thank You! so pleased we could help. 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: cli Command line interface, flags, terminal UX, or output area: onboarding Onboarding FSM, provider setup, sandbox launch, or first-run flow bug-fix PR fixes a bug or regression v0.0.96 Release target

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants