fix(advisor): keep sandbox entrypoint process alive

[apurvvkumaria]

Outcome

The Advisor sandbox initializer keeps its real Node process alive until OpenShell sends SIGTERM or SIGINT. The supervisor relay no longer closes merely because no active event-loop handle remains after initialization.

Reason

Post-merge Advisor run 34658298309 used the #11599 code from current main, but all nine specialists still stopped before review with exec relay closed before the command reported an exit status. An unresolved promise plus signal listeners does not keep a Node process alive when no active handles remain, so the initializer still exited immediately.

Related issues

Part of #10791

Changes

• Hold one active timer handle for the initializer lifetime and release it when OpenShell terminates the process.
• Add a real subprocess regression that fails when Node exits with an unsettled termination promise and passes only when the entrypoint remains alive through SIGTERM.

Verification

• npx vitest run test/automation/pull-requests/pr-review-advisor-openshell.test.ts --testTimeout=60000 — 47 tests passed after formatting.
• Pre-commit hooks — passed.
• NODE_OPTIONS=--max-old-space-size=8192 pre-push TypeScript checks — passed after building the plugin artifact required by the local checkout.
• GitHub commit verification — commit 566e6be05dc4e17f11d92bae035878b1e48886f0 is Verified.
• Diff inspection — no secrets, API keys, or credentials.

Review notes

This fixes the entrypoint-lifetime defect exposed after #11599 merged. PR #11596 separately owns the pinned OpenShell 0.0.116 sandbox exec command compatibility change; the two root causes remain separate.

---

Signed-off-by: Apurv Kumaria akumaria@nvidia.com

Summary by CodeRabbit

• Bug Fixes

  • Improved sandbox termination handling so the process remains active while waiting for OpenShell to stop the sandbox.
  • Added cleanup when termination signals are received, allowing shutdown to complete without lingering activity or errors.

• Tests

  • Added integration coverage verifying timeout-based termination behavior in a real Node.js process.

[coderabbitai]

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: ac0ab9ec-725f-4caf-9245-ea171bd9e88a

📥 Commits

Reviewing files that changed from the base of the PR and between 4eaa7c1 and 566e6be.

📒 Files selected for processing (2)

• test/automation/pull-requests/pr-review-advisor-openshell.test.ts
• tools/pr-review-advisor/openshell.mts

Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review.

---

📝 Walkthrough

Walkthrough

The OpenShell termination wait now keeps the Node.js process alive with an interval. A subprocess integration test verifies that the process remains active during the timeout and exits cleanly.

Changes

OpenShell termination handling

Layer / File(s)	Summary
Keep-alive handling and integration validation tools/pr-review-advisor/openshell.mts, test/automation/pull-requests/pr-review-advisor-openshell.test.ts	waitForAdvisorSandboxTermination keeps the process active until SIGTERM or SIGINT, then clears the interval. The integration test verifies timeout behavior, clean output, and a zero exit status.

Priority: ⬇️ Low

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

Change: Bug fix

Merge Risk: ⚪ Minimal · up to 566e6

The keep-alive behavior and subprocess regression coverage are complete, with no merge-blocking concerns identified.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 2 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely describes the primary change: keeping the Advisor sandbox entrypoint process alive until termination.
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.

• Fix all pre-merge checks with AI

✨ Finishing Touches 💡 1

📝 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-advisor-entrypoint-hold

---

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

[github-code-quality]

Code Coverage Overview

Languages: TypeScript

TypeScript / code-coverage/plugin

The overall line coverage in commit 566e6be in the codex/fix-advisor-en... branch remains at 96%, unchanged from commit 4eaa7c1 in the main branch.

TypeScript / code-coverage/cli

The overall line coverage in commit 566e6be in the codex/fix-advisor-en... branch remains at 83%, unchanged from commit 49fea6a in the main branch.

Show a line coverage summary of the most impacted files.

File	main 49fea6a	codex/fix-advisor-en... 566e6be	+/-
src/lib/onboard...eway-process.ts	90%	89%	-1%
src/lib/actions...oy-preflight.ts	84%	83%	-1%
src/lib/actions...confirmation.ts	69%	69%	0%
src/lib/domain/...dbox/destroy.ts	97%	97%	0%
src/lib/sandbox...rce-identity.ts	82%	82%	0%
src/lib/onboard...uild-context.ts	74%	75%	+1%
src/lib/actions...-add-restart.ts	30%	31%	+1%
src/lib/domain/...ycle/options.ts	85%	87%	+2%
src/lib/actions...dbox/destroy.ts	89%	92%	+3%
src/lib/actions...oy-execution.ts	91%	94%	+3%

---

_{Updated September 12, 2026 00:26 UTC}

[apurvvkumaria]

CI classification for exact head 566e6be:

• Shard 7 timed out in src/lib/inference/serving/host-local-vllm-selection.test.ts.
• Shard 9 failed because port 18789 was unavailable in test/onboarding/onboard-messaging.test.ts.

Neither test nor its owning production path is changed by this PR. The port signature also appears on #11596, and the same shards passed in concurrent #11592 CI, so these are shared intermittent failures rather than regressions from this two-file Advisor change. I am not adding unrelated fixes to this PR or issuing an undocumented retry.

[rsliter]

Reviewed exact commit 566e6be. The fix correctly keeps Node PID 1 alive with one owned timer until SIGTERM or SIGINT, clears that handle during shutdown, and preserves the existing listener cleanup. The real subprocess regression proves the prior unresolved-promise behavior no longer exits early. Focused validation passes 47/47 and all 19 repository checks pass locally; exact-head required CI passes after the classified shared-port flake; CodeRabbit found no actionable issue; DCO and commit verification pass. Approval applies to the implementation. Merge remains separately gated on the Advisor bootstrap decision.