Skip to content

fix(sandbox): ignore stable zombies during gateway discovery - #7029

Merged
cv merged 4 commits into
mainfrom
codex/fix-managed-controller-zombies
Jul 16, 2026
Merged

fix(sandbox): ignore stable zombies during gateway discovery#7029
cv merged 4 commits into
mainfrom
codex/fix-managed-controller-zombies

Conversation

@prekshivyas

@prekshivyas prekshivyas commented Jul 16, 2026

Copy link
Copy Markdown
Collaborator

Summary

Hermes gateway lifecycle control can return SUPERVISOR_UNAVAILABLE forever when an otherwise healthy sandbox contains a persistent zombie process, because Linux exposes an empty /proc/<pid>/cmdline for zombies. This change excludes only a descriptor-pinned, twice-observed zombie while preserving terminal refusal for empty command lines on live processes.

Observed in the post-fix full E2E run: https://github.com/NVIDIA/NemoClaw/actions/runs/29516191099

Changes

  • Parse cmdline together with the process state captured from the same pinned procfs descriptor.
  • Accept an empty cmdline only for state Z; the existing two-capture stable key must still match before the process can be ignored.
  • Extend the managed controller regression harness with a stable zombie and a live empty-command-line process.
  • Document the narrow zombie exclusion in the trusted-computing-boundary process proof.

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:
  • 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: Author review confirmed the stable key includes zombie state and command line, both descriptor-pinned captures must agree, and live empty command lines remain terminal; the regression test covers both outcomes.
  • 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 — npx vitest run --project integration test/managed-gateway-control.test.ts (3 passed)
  • Applicable broad gate passed — not applicable; this is a narrow procfs parsing change covered by the managed controller integration test
  • 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) — passed with 0 errors; Fern reported only the pre-existing unauthenticated redirect-check and light-mode contrast warnings
  • Doc pages follow the style guide (doc changes only)
  • New doc pages include SPDX header and frontmatter (new pages only)

Additional checks: python3 -m py_compile scripts/managed-gateway-control.py; npm run typecheck:cli; npm run docs.


Signed-off-by: Prekshi Vyas prekshiv@nvidia.com

Summary by CodeRabbit

  • Bug Fixes
    • Improved process identity verification by validating process thread count and strengthening process stat parsing.
    • Empty command lines are now accepted only for zombie processes with exactly one thread; otherwise discovery fails closed with an unavailable-supervisor error.
  • Documentation
    • Clarified the precise criteria for when empty command lines are acceptable during process identity verification.
  • Tests
    • Expanded the test harness and assertions to cover zombie-related scenarios, including new fail-closed cases.

Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
@coderabbitai

coderabbitai Bot commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

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: c9d03657-4be5-4dbe-94ad-4c77c67cb3e7

📥 Commits

Reviewing files that changed from the base of the PR and between 3474cf2 and 198543e.

📒 Files selected for processing (1)
  • test/managed-gateway-control.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
  • test/managed-gateway-control.test.ts

📝 Walkthrough

Walkthrough

Empty command-line handling is now state-aware: descriptor-pinned zombie captures remain acceptable only for single-thread zombies, while other empty command lines fail with SUPERVISOR_UNAVAILABLE. Tests cover these outcomes and document the verification rule.

Changes

Zombie command-line verification

Layer / File(s) Summary
State-aware command-line capture
scripts/managed-gateway-control.py, docs/security/tcb-boundary.mdx
_parse_stat validates and returns thread counts; _parse_cmdline accepts empty argv only for single-thread zombies; and ProcReader.capture applies the rule to both captures. Documentation reflects the updated identity-verification criteria.
Zombie and live-process coverage
test/managed-gateway-control.test.ts
The harness models stable single-thread zombies, zombie leaders with live siblings, and empty-command-line processes, asserting the corresponding capture and discovery results.

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

Suggested reviewers: cv

🚥 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 matches the main change: gateway discovery now ignores only stable zombie processes.
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.
✨ 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-managed-controller-zombies

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

@github-code-quality

github-code-quality Bot commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

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 codex/fix-managed-co... branch remains at 80%, unchanged from the main branch.

Show a code coverage summary of the most impacted files.
File main 24c7334 codex/fix-managed-co... 21aedbb +/-
src/lib/inferen...lama/process.ts 100% 50% -50%
src/lib/domain/.../connect-env.ts 97% 89% -8%
src/lib/inferen...er-lifecycle.ts 71% 65% -6%

Updated July 16, 2026 19:50 UTC
Code Coverage is in Public Preview. Learn more and provide us with your feedback.

@github-actions

Copy link
Copy Markdown
Contributor

@github-actions

github-actions Bot commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

PR Review Advisor — Informational

Advisor assessment: Informational / high confidence
Next action: Review the warnings below.
Findings: 0 blockers · 1 warning · 0 suggestions
Status: Canonical ledger: 0 blocker(s), 1 warning(s), 0 suggestion(s).

Model lanes

  • GPT-5.6 Terra (primary): Completed · high confidence · 0 blockers · 1 warning · 0 suggestions
  • Nemotron 3 Ultra (second opinion): Completed · high confidence · 0 blockers · 0 warnings · 0 suggestions
  • Model comparison: normalized findings differ; normalized E2E selections differ; Nemotron reported the same number of blockers, 1 fewer warning, the same number of suggestions.

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: None

2 optional E2E recommendations
  • gateway-guard-recovery
  • sandbox-operations
1 warning · 0 suggestions

Warnings

Warnings do not block.

PRA-1 Warning — Record the lifecycle and removal condition for the zombie parsing exception

  • Location: docs/security/tcb-boundary.mdx:95
  • Category: docs
  • Problem: The new trusted-boundary exception permits an otherwise invalid empty command line when procfs reports a single-thread zombie, but the accompanying documentation only states the rule. It does not identify the kernel/procfs condition being accommodated, why its source cannot be changed in this PR, or when the exception should be reevaluated or removed.
  • Impact: A future maintainer cannot distinguish this narrowly justified tolerance from a general weakening of process-identity proof, making it easier to retain or broaden the exception after its source assumption changes.
  • Recommendation: Add a concise source-of-truth note naming the Linux procfs zombie behavior being handled and a concrete reevaluation/removal condition for the single-thread empty-cmdline exception.
  • Verification: Inspect the Process and Listener Proofs section and the `_parse_cmdline` comment together; confirm the documentation identifies the source condition and a removal/review trigger rather than only restating acceptance behavior.
  • Test coverage: The existing focused harness already proves stable single-thread zombie tolerance and rejection of a threaded zombie leader; retain those cases as the contract test when documenting the exception's lifecycle.
  • Evidence: `scripts/managed-gateway-control.py:629-637` adds tolerant parsing for empty cmdline only when state is `Z` and thread count is one. `docs/security/tcb-boundary.mdx:95` documents the allowed and rejected observations but provides no source rationale or removal/review condition for this newly introduced exception.

Workflow run details

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

@apurvvkumaria apurvvkumaria added bug-fix PR fixes a bug or regression area: sandbox OpenShell sandbox lifecycle, runtime, config, or recovery labels Jul 16, 2026
@apurvvkumaria apurvvkumaria self-assigned this Jul 16, 2026
@apurvvkumaria apurvvkumaria added area: security Security controls, permissions, secrets, or hardening integration: hermes Hermes integration behavior v0.0.85 labels Jul 16, 2026
Co-authored-by: Prekshi Vyas <prekshiv@nvidia.com>
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>

@cv cv 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.

Security-test blocker on exact head 3474cf20e7a27ca602c104bf2b9014484a50cb9b:

test/managed-gateway-control.test.ts:398 still writes the pre-change /proc/<pid>/stat fixture shape (["S", "40"] + 17 zeroes + ["999"]). With the new parser, suffix[17] is therefore 0, so _parse_stat rejects thread_count < 1 before _terminate_gateway can compare the changed start identity. The reused: "SUPERVISOR_UNAVAILABLE" assertion passes for malformed-stat rejection and no longer proves that a valid reused PID is not signalled. I reproduced the current inline record as SUPERVISOR_UNAVAILABLE, while the valid shape with num_threads=1 parses as ("S", 40, "999", 1).

Please update this inline rewrite to emit a valid num_threads=1 field (or route it through the shared stat fixture builder), then rerun npx vitest run --project integration test/managed-gateway-control.test.ts. Because this is the process-identity security boundary, preserving the PID-reuse regression is a merge gate.

Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
@apurvvkumaria

Copy link
Copy Markdown
Collaborator

The requested PID-reuse fixture correction is now on exact head 198543ef069e063723a2dbc0a65e17b5a235a824, authored and signed by the original contributor. I independently reproduced the correction locally, confirmed the remote diff is byte-for-byte identical, and the focused process-identity test plus full exact-head CI are green. The commit is GitHub Verified. @cv please re-review this exact head and clear the outdated change request if satisfied.

@cv
cv enabled auto-merge (squash) July 16, 2026 19:44
@cv
cv merged commit 4cab939 into main Jul 16, 2026
53 checks passed
@cv
cv deleted the codex/fix-managed-controller-zombies branch July 16, 2026 19:51
cv pushed a commit that referenced this pull request Jul 17, 2026
<!-- markdownlint-disable MD041 -->
## Summary

Add the canonical dated changelog entry required before the v0.0.85
release plan can be generated.
The entry summarizes the user-visible OpenShell, DGX Station, inference,
MCP, onboarding, and recovery changes merged since v0.0.84 and links to
their owning guides.

## Changes

- Add `docs/changelog/2026-07-16.mdx` with the exact `## v0.0.85`
heading, parser-safe SPDX comment, release summary, and detailed
bullets.
- Link every documented theme to its most specific published OpenClaw
guide routes.
- Reconcile the release entry with these merged source PRs:
- #6726 -> `docs/changelog/2026-07-16.mdx`: Document the supported
OpenShell v0.0.85 upgrade, immutable consumed artifacts, multiline exec,
credential rewrite diagnostics, and child-process TLS boundary.
- #6986 -> `docs/changelog/2026-07-16.mdx`: Document managed MCP
behavior shared across supported agents.
- #6991 and #7045 -> `docs/changelog/2026-07-16.mdx`: Document qualified
DGX Station host preparation and the interactive-terminal boundary for
`--station-deepseek`.
- #6992, #7001, #7006, and #7044 -> `docs/changelog/2026-07-16.mdx`:
Document managed-model reasoning behavior, safe inference route
mutation, and verified vLLM served aliases.
- #6865, #7010, and #7028 -> `docs/changelog/2026-07-16.mdx`: Document
onboarding DNS recovery, explicit notice acceptance, and upgrades with
user-local OpenShell.
- #7005, #7021, #7029, and #7049 -> `docs/changelog/2026-07-16.mdx`:
Document rebuild backup safety, no-dashboard state, managed gateway
discovery, and Hermes shields topology checks.

## Type of Change

- [ ] Code change (feature, bug fix, or refactor)
- [ ] Code change with doc updates
- [x] Doc only (prose changes, no code sample modifications)
- [ ] Doc only (includes code sample changes)

## Quality Gates

- [ ] Tests added or updated for changed behavior
- [x] Existing tests cover changed behavior — justification:
`test/changelog-docs.test.ts` validates the canonical heading,
parser-safe SPDX comment, and detailed entry structure; the docs build
validates published routes.
- [ ] Tests not applicable — justification:
- [x] Docs updated for user-facing behavior changes
- [ ] Docs not applicable — justification:
- [ ] 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:
- [ ] Non-success, skipped, or missing CI check accepted by maintainer —
check name, approval link, and follow-up issue:

## Verification

- [x] PR description includes a `Signed-off-by:` line and every commit
appears as `Verified` in GitHub
- [x] Normal `pre-commit`, `commit-msg`, and `pre-push` hooks passed, or
`npm run check:diff` passed when hooks were skipped or unavailable
- [x] Targeted behavior tests pass for the current change set, or tests
are marked not applicable above — `npx vitest run
test/changelog-docs.test.ts` passed 6/6.
- [ ] 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 doc-only entry.
- [x] Quality Gates section completed with required justifications or
waivers
- [x] No secrets, API keys, or credentials committed
- [ ] `npm run docs` builds without warnings (doc changes only) — passed
with 0 errors and 2 pre-existing Fern warnings.
- [x] Doc pages follow the [style
guide](https://github.com/NVIDIA/NemoClaw/blob/main/docs/CONTRIBUTING.md)
(doc changes only)
- [x] New doc pages include SPDX header and frontmatter (new pages only)
— native changelog entries use the required parser-safe MDX SPDX comment
instead of frontmatter.

---
Signed-off-by: Charan Jagwani <cjagwani@nvidia.com>


<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->

## Summary by CodeRabbit

* **Documentation**
  * Added release notes for NemoClaw v0.0.85.
* Documented improvements to compatibility, credential handling, setup
validation, recovery workflows, endpoint configuration, gateway
discovery, and runtime validation.
  * Added links to relevant user-guide sections.

<!-- end of auto-generated comment: release notes by coderabbit.ai -->

Signed-off-by: Charan Jagwani <cjagwani@nvidia.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: sandbox OpenShell sandbox lifecycle, runtime, config, or recovery area: security Security controls, permissions, secrets, or hardening bug-fix PR fixes a bug or regression integration: hermes Hermes integration behavior

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants