Skip to content

test(sandbox): cover gateway restart forward recovery and marker precedence - #8163

Merged
prekshivyas merged 2 commits into
NVIDIA:mainfrom
Dreamstick9:test/gateway-restart-classification-forward-coverage
Aug 4, 2026
Merged

test(sandbox): cover gateway restart forward recovery and marker precedence#8163
prekshivyas merged 2 commits into
NVIDIA:mainfrom
Dreamstick9:test/gateway-restart-classification-forward-coverage

Conversation

@Dreamstick9

@Dreamstick9 Dreamstick9 commented Aug 3, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Summary

Adds regression tests for two risky gateway restart paths in src/lib/actions/sandbox/gateway-restart.ts: host-forward recovery after a successful restart and failure-layer classification when supervisor markers co-occur. Behavior is unchanged; the tests lock the current contracts so a future edit to marker ordering or forward recovery fails fast.

Changes

  • Test the combined auxiliary forward failure detail: three failed auxiliary forwards are reported comma-joined in declaration order, and recovered or not-enabled forwards are omitted from the detail.
  • Test the per-forward contract that null means not enabled and only false fails the restart, including the full success result when no auxiliary forward is enabled.
  • Test that a failed primary dashboard/API forward is reported ahead of failed auxiliary forwards.
  • Test classifyGatewayRestartFailure precedence for co-occurring markers: each specific marker ahead of the generic health timeout it causes, a stopped supervisor ahead of the generic control markers, the replacement-stage layer with a generic control marker present, MCP drift ahead of a co-reported config hash mismatch, and the same precedence when markers split across stdout and stderr.

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: tests-only change; no user-visible behavior, CLI, configuration, default, or error text changes.
  • Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging)
  • Sensitive-path review completed — three independent exact-diff reviews (correctness, conventions, adversarial) confirmed the diff adds tests only, modifies no production sandbox code path, and locks orderings that source comments document as intentional.
  • 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: The diff changes only src/lib/actions/sandbox/gateway-restart.test.ts (+154 test lines). Every asserted layer name and detail string pre-exists in src/lib/actions/sandbox/gateway-restart.ts, and that file is unchanged. Docs pages that mention gateway restart (docs/manage-sandboxes/gateway-lifecycle-control.mdx, docs/reference/troubleshooting.mdx, docs/manage-sandboxes/runtime-controls.mdx) do not enumerate the tested failure layers or detail strings, so no page claim is affected. A change with no user-visible surface requires no pre-tag changelog entry.
  • Agent: Claude Code

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 validate:pr passed after refreshing origin/main when hooks were skipped or unavailable — pre-commit and commit-msg ran on the local commit, and npm run validate:pr passed after refreshing origin/main.
  • Targeted behavior tests pass for the current change set, or tests are marked not applicable above — command/result: npx vitest run --project cli src/lib/actions/sandbox/gateway-restart.test.ts → 32 passed (18 pre-existing + 14 new); npm run test:changed → 32 passed; npm run typecheck:cli → clean. Each of the 14 new tests was also observed to fail under a deliberate implementation break before the break was reverted (seven mutations: reversed join order, null treated as failed, health-timeout check hoisted first, auxiliary failures checked before the primary forward, stopped-supervisor check demoted, replacement-stage check demoted, config-hash check moved ahead of MCP drift).
  • 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: Kushagar Garg dreamstick909@gmail.com

@Dreamstick9
test(sandbox): cover gateway restart forward recovery and marker prec…
042ad6b 
…edence

Add regression tests for restartSandboxGateway forward recovery:
combined auxiliary forward failure detail in declaration order, the
null-means-not-enabled contract for each auxiliary forward, and the
primary forward failure reported ahead of auxiliary failures.

Add classification precedence tests for co-occurring supervisor
markers: each specific marker ahead of the health timeout it causes,
a stopped supervisor ahead of the generic control markers, the
replacement-stage layer with a generic marker present, MCP drift ahead
of the co-reported config hash mismatch, and precedence across stdout
and stderr.

Signed-off-by: Kushagar Garg <dreamstick909@gmail.com>
@copy-pr-bot

copy-pr-bot Bot commented Aug 3, 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 Aug 3, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

The PR adds gateway restart tests for failure-classification precedence and forward-recovery result reporting. The tests cover combined output streams, auxiliary-forward aggregation and filtering, successful recovery, and primary-forward failure precedence.

Changes

Gateway restart recovery

Layer / File(s) Summary
Failure classification precedence
src/lib/actions/sandbox/gateway-restart.test.ts		 Tests verify specific markers override generic health-timeout and control-unavailable markers. They also cover replacement-stage classification, MCP drift precedence, hash mismatch handling, and stdout/stderr combinations.
Forward recovery result precedence
src/lib/actions/sandbox/gateway-restart.test.ts		 Tests verify auxiliary failure aggregation and declaration order, exclusion of recovered or disabled forwards, agent-declared host-forward failures, no-forward recovery, and primary dashboard/API failure precedence.

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

Possibly related PRs

Suggested labels: chore, area: sandbox

Suggested reviewers: ericksoa, cv

🚥 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 summarizes the added tests for gateway restart forward recovery and failure-marker precedence.
✨ 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 Aug 3, 2026 •
edited
Loading

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.
1 additional E2E selection from the second opinion

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

  • sandbox-operations: 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.

2 semantic terminology decisions

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

  • established — auxiliary forward at src/lib/actions/sandbox/gateway-restart.test.ts:460: Retain "auxiliary forward" for non-primary forwards in these tests.
  • justified — replacement-stage at src/lib/actions/sandbox/gateway-restart.test.ts:76: Retain the modifier because it identifies the marker condition that selects the established supervisor-unavailable layer.

E2E guidance

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

Recommended E2E: None

1 optional E2E recommendation
  • gateway-guard-recovery

Workflow run details

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

@prekshivyas prekshivyas self-assigned this Aug 3, 2026
apurvvkumaria
apurvvkumaria approved these changes Aug 3, 2026
View reviewed changes

@apurvvkumaria apurvvkumaria left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed exact head 042ad6b. This test-only change accurately exercises existing gateway restart marker precedence and forward-recovery result contracts without changing production behavior. I found no blocking defect. The failed primary advisor lane published an informational assessment with zero findings; the second opinion completed cleanly.

@prekshivyas
prekshivyas merged commit a37c0c9 into NVIDIA:main Aug 4, 2026
41 checks passed
@github-actions github-actions Bot added the v0.0.102 Release target label Aug 4, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@apurvvkumaria apurvvkumaria apurvvkumaria approved these changes

Assignees

@prekshivyas prekshivyas

Labels

v0.0.102 Release target

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@Dreamstick9 @apurvvkumaria @prekshivyas