fix(sandbox): treat an already-active port forward as recovery success

[jason-ma-nv]

Summary

nemohermes <sandbox> recover (and any dashboard/API forward recovery) could report "the dashboard/API host forward could not be restored" even when the forward was already active and healthy. ensureSandboxPortForwardForPort trusted the openshell forward start exit code — any non-zero status returned failure — but OpenShell exits non-zero when the port is already forwarded. When recovery's stop → start ran against a still-active forward (e.g. OpenShell's forward list drifted stale while the port kept listening), forward start reported "already forwarded" and recovery failed for a healthy forward. After this change, a non-zero start with a reachable local listener settles against the authoritative live forward list and accepts an already-active, target-owned forward as idempotent success, including when owner metadata appears after a short delay.

Related Issue

Closes #7085

Changes

• src/lib/actions/sandbox/forward-recovery.ts: on a non-zero forward start, fail immediately when no local listener is reachable; otherwise use the existing bounded health-settling path and accept success only when the live forward list confirms a running, reachable forward owned by the target sandbox. This keeps the live list authoritative, handles delayed owner visibility, and preserves fail-closed behavior for absent or occupied forwards. The maintenance comment records the OpenShell 0.0.85 source boundary and removal condition.
• test/process-recovery-forward-failure.test.ts: add five focused cases for non-zero starts: immediate target ownership succeeds, an absent forward fails, a reachable listener without authoritative ownership times out and fails, delayed target ownership succeeds, and delayed ownership by another sandbox fails. The new setup uses ESM imports and linear Vitest mocks, with no DGX or real sandbox dependency.

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: internal recovery/forward reconciliation behaviour; no user-facing surface, flag, or documented contract 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: Requesting maintainer review — touches sandbox forward recovery (the recover path). The change keeps fail-closed behaviour for occupied/absent forwards and only accepts a target-owned, reachable, running forward. Prepared by an automated fix loop.
• Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue:

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
• Targeted behavior tests pass for the current change set, or tests are marked not applicable above — command/result: npx vitest run --project integration test/process-recovery.test.ts test/process-recovery-forward-failure.test.ts → 36 passed; npm run check:diff → 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)

Verification detail

• Proven: a non-zero forward start succeeds for immediate or delayed target ownership, fails fast when no listener remains, rejects a reachable but ownerless listener after the settle deadline, and rejects delayed ownership by another sandbox. The broader recovery regression set remains green.
• Remains (blocked E2E): the full nemohermes recover DGX Station repro (rebuild a Hermes sandbox with a not-running gateway, run recover, observe the already-active forward reported as success) needs a DGX/sandbox not available to this loop.

---

Signed-off-by: Jason Ma jama@nvidia.com
Signed-off-by: Julie Yaunches jyaunches@nvidia.com

Summary by CodeRabbit

• Bug Fixes

  • Improved sandbox port-forward recovery to be more tolerant of transient/non-authoritative stop/wait states, while still failing when the local listener can’t quiesce under expected conditions.
  • If the background port-forward start exits with an error, recovery now continues into forward-health verification when the local listener is reachable (to handle already-correct forwards more smoothly).
  • Maintains strict failure when no authoritative forward appears or when delayed ownership is claimed by a different sandbox.

• Tests

  • Expanded and refined coverage for idempotent already-forwarded handling, delayed ownership via repeated re-probes, and unreachable/unrecoverable ownership scenarios.

[coderabbitai]

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

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: e4075edf-28da-402e-afe2-7b575430862a

📥 Commits

Reviewing files that changed from the base of the PR and between 0bc0e60 and 73bc74d.

📒 Files selected for processing (1)

• test/process-recovery.test.ts

🚧 Files skipped from review as they are similar to previous changes (1)

• test/process-recovery.test.ts

---

📝 Walkthrough

Walkthrough

Sandbox forward recovery now tolerates non-authoritative intermediate states and re-probes forward ownership after non-zero background starts. Tests cover immediate, delayed, absent, and conflicting ownership outcomes.

Changes

Sandbox forward recovery

Layer / File(s)	Summary
Reconcile listener and forward state src/lib/actions/sandbox/forward-recovery.ts, test/process-recovery.test.ts	Pre-start recovery now fails only for occupied authoritative forwards or fail-closed listener states, and tests expect a start attempt for an unreleased unowned listener.
Re-probe failed forward starts src/lib/actions/sandbox/forward-recovery.ts, test/process-recovery-forward-failure.test.ts	Non-zero starts continue to authoritative health checks when the listener is reachable; tests cover target ownership, delayed recovery, missing ownership, conflicting ownership, retries, and command options.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Recovery as ensureSandboxPortForwardForPort
  participant CLI as runOpenshell
  participant Health as forward health probe
  Recovery->>CLI: Start forward in background
  CLI-->>Recovery: Non-zero status
  Recovery->>Health: Re-probe forward ownership
  Health-->>Recovery: Target-owned, absent, or different sandbox
  Recovery-->>Recovery: Accept, retry, or return false

Loading

Suggested labels: integration: hermes, platform: dgx-station

🚥 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 summarizes the main behavior change: recovering an already-active sandbox port forward succeeds instead of failing.
Linked Issues check	✅ Passed	The code and tests address #7085 by accepting an already-active, target-owned forward and preserving failures for absent or misowned forwards.
Out of Scope Changes check	✅ Passed	The changes stay focused on sandbox port-forward recovery and corresponding tests, with no unrelated behavior introduced.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✨ 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 fix/7085-recover-forward-idempotent

---

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

[github-code-quality]

Code Coverage Overview

Languages: TypeScript

TypeScript / code-coverage/plugin

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

TypeScript / code-coverage/cli

The overall coverage in the fix/7085-recover-for... branch remains at 80%, unchanged from the main branch.

Show a code coverage summary of the most impacted files.

File	main 0339d88	fix/7085-recover-for... 73bc74d	+/-
src/lib/core/pr...mpt-activity.ts	92%	67%	-25%
src/lib/credentials/store.ts	64%	59%	-5%
src/lib/adapter...hell/resolve.ts	100%	100%	0%
src/lib/agent/s...store-reader.ts	90%	90%	0%
src/lib/sandbox...rce-identity.ts	91%	91%	0%
src/lib/securit...p-url-target.ts	100%	100%	0%
src/lib/state/config-io.ts	93%	93%	0%
src/lib/state/registry-mcp.ts	100%	100%	0%
src/lib/actions...ard-recovery.ts	92%	94%	+2%
src/lib/state/registry.ts	83%	86%	+3%

---

_{Updated July 18, 2026 09:23 UTC
Code Coverage is in Public Preview. Learn more and provide us with your feedback.}

[coderabbitai]

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/process-recovery-forward-failure.test.ts`:
- Around line 15-17: Replace the root-level test’s requireSource-based imports
around ensureSandboxPortForwardForPort with ESM imports, and convert the
associated mocks at the referenced locations to Vitest’s ESM mocking approach.
Remove createRequire usage while preserving the existing mocked behavior and
test coverage.

🪄 Autofix (Beta)

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: bc71d9bb-2a7f-4bdf-a675-6ce68f95a6dc

📥 Commits

Reviewing files that changed from the base of the PR and between 027632a and 1091b5b.

📒 Files selected for processing (2)

• src/lib/actions/sandbox/forward-recovery.ts
• test/process-recovery-forward-failure.test.ts

[github-actions]

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: onboard-repair, onboard-resume

Workflow run details

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

[apurvvkumaria]

Exact-head review for a735921 found one production-path blocker. With the default nonzero recovery wait, a listener that is already reachable enters the pre-start settling branch in forward-recovery.ts; when live ownership metadata remains stale/ownerless, that branch returns false before forward start is attempted. The new post-start already-active tolerance is therefore unreachable for the PR’s stated list-drift scenario. The positive regression test masks this by setting NEMOCLAW_FORWARD_RECOVERY_WAIT_MS=0. Please add a default/nonzero-wait regression with a pre-existing reachable listener and stale metadata, and reconcile it without ever accepting an unowned listener. Current CI, DCO/signatures, advisor review, and protected E2E otherwise look healthy.

Addressed in verified commit 0bc0e60. The nonzero-wait ownerless-listener path now reaches idempotent start reconciliation, and success still requires authoritative exact target ownership. Wrong-owner, unavailable-metadata, and forced-restart paths remain fail-closed; 37 targeted tests and all focused structural/build checks passed.

[cv]

Approved on the refreshed exact head after the deterministic gate passed: 54 current checks green, no conflicts, no unresolved major/critical review findings, DCO present, and all 6 commits verified. The recovery path remains fail-closed for absent, foreign, and unavailable listeners, replaces only when ownership is proven, and treats the same owned active forward as idempotent success. Focused recovery suites passed 37 tests; the nine-category security review is clean.