fix(cli): clean up headless gateway fallback - #8033
Conversation
Signed-off-by: Senthil Ravichandran <senthilr@nvidia.com>
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
|
Note Reviews pausedIt 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 Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe destroy flow supports headless Linux cleanup when the systemd user manager is unavailable. It stops the recorded gateway process, verifies ownership and port release, then removes runtime files and gateway resources only after validation succeeds. ChangesGateway cleanup
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant ServiceStop
participant DestroyGateway
participant GatewayProcess
participant GatewayPort
participant RuntimeFiles
ServiceStop->>DestroyGateway: Report stop result and fallback eligibility
DestroyGateway->>GatewayProcess: Stop recorded PID
DestroyGateway->>GatewayPort: Verify port is free
DestroyGateway->>RuntimeFiles: Remove PID file and runtime marker
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in commit c148261 in the TypeScript / code-coverage/cliThe overall coverage in commit c148261 in the Show a code coverage summary of the most impacted files.
Updated |
Security review — PASSReviewed PR #8033 at exact head No security findings were identified. The change is safe to proceed through CI:
Files reviewed:
|
|
🌿 Preview your docs: https://nvidia-preview-pr-8033.docs.buildwithfern.com/nemoclaw |
senthilr-nv
added
v0.0.100
bug-fix
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/lib/actions/sandbox/destroy-gateway.ts (1)
40-60: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove host-bound operations out of the action.
Lines 40-60 make the action own child-process execution and filesystem deletion.
This duplicates runtime-file cleanup insrc/lib/onboard/host-gateway-process.ts.
Make an existing adapter or host-gateway boundary the authoritative owner of the port probe and runtime cleanup.
KeepcleanupGatewayAfterLastSandboxas the orchestration consumer throughCleanupGatewayDeps.As per path instructions, “actions orchestrate” and “adapters own host/process/network boundaries.”
🤖 Prompt for 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. In `@src/lib/actions/sandbox/destroy-gateway.ts` around lines 40 - 60, Move isGatewayPortFree and clearGatewayRuntimeFiles out of the action into the existing host-gateway adapter boundary, reusing the authoritative runtime-file cleanup in host-gateway-process.ts and placing the port probe there. Update CleanupGatewayDeps and cleanupGatewayAfterLastSandbox to consume those adapter operations while retaining the action as orchestration only.Source: Path instructions
🤖 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.
Nitpick comments:
In `@src/lib/actions/sandbox/destroy-gateway.ts`:
- Around line 40-60: Move isGatewayPortFree and clearGatewayRuntimeFiles out of
the action into the existing host-gateway adapter boundary, reusing the
authoritative runtime-file cleanup in host-gateway-process.ts and placing the
port probe there. Update CleanupGatewayDeps and cleanupGatewayAfterLastSandbox
to consume those adapter operations while retaining the action as orchestration
only.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 9a0c90ae-6aa7-4b15-9071-aa7b13bd672e
📒 Files selected for processing (5)
docs/reference/commands.mdxsrc/lib/actions/sandbox/destroy-gateway.test.tssrc/lib/actions/sandbox/destroy-gateway.tssrc/lib/onboard/docker-driver-gateway-service.test.tssrc/lib/onboard/docker-driver-gateway-service.ts
Signed-off-by: Senthil Ravichandran <senthilr@nvidia.com>
Security review refresh — PASSRe-reviewed PR #8033 at new exact head All nine categories remain PASS with no findings. The follow-up moves the bind probe and exact runtime-file removal into
No secrets, dependencies, authentication, cryptography, network policy, container policy, or credential behavior changed. |
|
Addressed the CodeRabbit boundary finding in 494b2a5: the bind probe and runtime-file cleanup now live in host-gateway-process.ts, with focused boundary tests for free/occupied/inconclusive probe results and exact runtime-file removal. Focused tests: 59 passed; CLI type-check and validate:pr passed. |
PR Review Advisor — No blocking findings reportedAdvisor assessment: No blocking advisor findings reported Model lanes
Second-opinion E2E selections are advisory. They do not change the primary assessment or E2E / PR Gate. E2E guidanceAdvisory only. E2E / PR Gate selects and runs jobs independently. Recommended E2E: 1 optional E2E recommendation
1 warning · 0 suggestionsWarningsWarnings do not block.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/lib/onboard/host-gateway-process.test.ts (1)
89-102: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAssert that unrelated state files remain intact.
The test creates only the two files that should be removed. A regression that removes all of
stateDirrecursively would still pass. Add an unrelated sentinel file and assert that it remains afterclearHostGatewayRuntimeFiles.As per path instructions, verify the observable filesystem boundary rather than only the intended deletion.
[details]const markerFile = path.join(stateDir, "runtime.json"); + const unrelatedFile = path.join(stateDir, "unrelated.txt"); fs.writeFileSync(pidFile, "4242\n"); fs.writeFileSync(markerFile, "{}\n"); + fs.writeFileSync(unrelatedFile, "keep\n"); clearHostGatewayRuntimeFiles(stateDir, pidFile); expect(fs.existsSync(pidFile)).toBe(false); expect(fs.existsSync(markerFile)).toBe(false); + expect(fs.existsSync(unrelatedFile)).toBe(true);[details]
🤖 Prompt for 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. In `@src/lib/onboard/host-gateway-process.test.ts` around lines 89 - 102, Extend the test around clearHostGatewayRuntimeFiles by creating an unrelated sentinel file in stateDir before invoking it. Assert afterward that pidFile and markerFile are removed while the sentinel file still exists, verifying only the targeted runtime files are deleted.Source: Path instructions
🤖 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 `@src/lib/onboard/host-gateway-process.test.ts`:
- Around line 89-102: Update the test around clearHostGatewayRuntimeFiles to
wrap temporary-directory setup and assertions in a try/finally block, and remove
stateDir recursively in the finally clause. Preserve the existing PID and
runtime marker assertions while ensuring cleanup runs even when setup or an
assertion fails.
In `@src/lib/onboard/host-gateway-process.ts`:
- Around line 197-200: Update clearHostGatewayRuntimeFiles to remove the runtime
marker via clearDockerDriverGatewayRuntimeMarker before deleting pidFile,
ensuring the PID evidence remains available if marker cleanup fails. If cleanup
can partially succeed before an error is rethrown, restore any removed runtime
file so both artifacts survive the failure.
---
Nitpick comments:
In `@src/lib/onboard/host-gateway-process.test.ts`:
- Around line 89-102: Extend the test around clearHostGatewayRuntimeFiles by
creating an unrelated sentinel file in stateDir before invoking it. Assert
afterward that pidFile and markerFile are removed while the sentinel file still
exists, verifying only the targeted runtime files are deleted.
🪄 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: aece70e8-87ae-42d2-aa9c-1611b7a32a3e
📒 Files selected for processing (3)
src/lib/actions/sandbox/destroy-gateway.tssrc/lib/onboard/host-gateway-process.test.tssrc/lib/onboard/host-gateway-process.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/lib/actions/sandbox/destroy-gateway.ts
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/lib/onboard/host-gateway-process.test.ts (1)
107-117: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winStart cleanup before test setup.
stateDiris created at Line 107, but thetryblock starts at Line 117. If eitherfs.writeFileSyncorvi.spyOnfails, the temporary directory remains. Put setup inside the outertry, restore the spy in an innerfinally, and removestateDirin the outerfinally.🤖 Prompt for 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. In `@src/lib/onboard/host-gateway-process.test.ts` around lines 107 - 117, Move the temporary-directory setup, file writes, and fs.rmSync spy creation in the test around the existing cleanup scenario into the outer try block. Restore the spy in an inner finally, and remove stateDir in the outer finally so cleanup runs even when setup or spy creation fails.
🤖 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 `@src/lib/onboard/host-gateway-process.test.ts`:
- Around line 112-115: Update the rmSync spy setup in the test to capture
fs.rmSync.bind(fs) before calling vi.spyOn, then invoke that captured original
implementation when candidate !== markerFile instead of calling fs.rmSync
through the spy.
---
Duplicate comments:
In `@src/lib/onboard/host-gateway-process.test.ts`:
- Around line 107-117: Move the temporary-directory setup, file writes, and
fs.rmSync spy creation in the test around the existing cleanup scenario into the
outer try block. Restore the spy in an inner finally, and remove stateDir in the
outer finally so cleanup runs even when setup or spy creation fails.
🪄 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: b5177ffe-f3dd-4275-a0c5-08eb0cfab811
📒 Files selected for processing (7)
docs/reference/commands.mdxsrc/lib/actions/sandbox/destroy-gateway.test.tssrc/lib/actions/sandbox/destroy-gateway.tssrc/lib/onboard/docker-driver-gateway-service.test.tssrc/lib/onboard/docker-driver-gateway-service.tssrc/lib/onboard/host-gateway-process.test.tssrc/lib/onboard/host-gateway-process.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- src/lib/actions/sandbox/destroy-gateway.test.ts
- src/lib/actions/sandbox/destroy-gateway.ts
- docs/reference/commands.mdx
- src/lib/onboard/docker-driver-gateway-service.ts
- src/lib/onboard/docker-driver-gateway-service.test.ts
- src/lib/onboard/host-gateway-process.ts
2 participants
Summary
Final sandbox cleanup failed on headless Linux runners when the packaged OpenShell service existed but
systemctl --usercould not reach a user manager.Cleanup now permits the standalone path only for the recognized unavailable-manager condition and only when the packaged unit cannot activate automatically.
It requires PID-bound gateway identity, verifies that the port is free, and preserves runtime evidence until cleanup completes.
Related Issue
Follow-up to #7904 and #7907.
Changes
destroycommand reference.Type of Change
Quality Gates
c14826163d08d98031a583c49407783355b24592against base227e8b92b600c77684fe45befd7909ae4ad39141; all nine categories passed with no actionable findings.Documentation Writer Review
docs-updateddocs/reference/commands.mdxand every changed comment, diagnostic, and test title against the implementation, writing rules, controlled word list, and documentation contributor guide. The final test-only delta keeps the injected cleanup failure path linear and preserves its exact marker-path assertion.npm run docscompleted with 0 errors and 2 generic Fern upgrade notices.DGX Station Hardware Evidence
Verification
Signed-off-by:line and every commit appears asVerifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passed, ornpm run validate:prpassed after refreshingorigin/mainwhen hooks were skipped or unavailablenpx vitest run --project cli src/lib/actions/sandbox/destroy-gateway.test.ts src/lib/onboard/docker-driver-gateway-service.test.ts src/lib/onboard/host-gateway-process.test.ts(62 passed);npm run typecheck:clipassed.npm testfor broad runtime/test-harness changes;npm run checkfor repo-wide validation/coverage changes — command/result:npm run docsbuilds without warnings (doc changes only)Exact local evidence: head
c14826163d08d98031a583c49407783355b24592; base227e8b92b600c77684fe45befd7909ae4ad39141; complete seven-file diff reviewed;git diff --checkpassed; diff-awarepre-commit,commit-msg, andpre-pushstages passed againstupstream/main.Product scope: approved independently of GitHub merge state. This fixes the accepted final-destroy behavior from #7904 and the packaged-service lifecycle established by #7907. It adds no new integration, configuration surface, image, third-party stack, or supported workflow.
Signed-off-by: Senthil Ravichandran senthilr@nvidia.com
Summary by CodeRabbit
Bug Fixes
Tests