fix(e2e): repair managed image gateway cleanup - #8763
Conversation
Signed-off-by: Apurv Kumaria <akumaria@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. |
📝 WalkthroughWalkthroughThe managed-image E2E flow validates required onboarding hooks, supports default exports, and exports the OpenShell helpers. Gateway cleanup now yields during shutdown, verifies process and listener termination, and includes focused contract and lifecycle tests. ChangesManaged-image OpenShell runtime
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in commit fd486b8 in the TypeScript / code-coverage/cliThe overall coverage in commit fd486b8 in the Show a code coverage summary of the most impacted files.
Updated |
PR Review Advisor — No blocking findings reportedAdvisor assessment: No blocking advisor findings reported Model lanes
Second-opinion terminology and E2E selections are advisory. Live E2E does not run automatically for pull requests. 1 semantic terminology decisionTerminology decisions are advisory. They affect the assessment only when a separate finding identifies concrete semantic impact.
E2E guidanceAdvisory only. A maintainer can dispatch the default E2E suite against this exact revision. Recommended E2E: This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/checks/run-managed-image-openshell-e2e.ts (1)
299-319: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftProve gateway ownership before sending signals.
stopProcesschecks only whether the PID exists. A reused PID can terminate an unrelated process. The port probe cannot identify the process that received the signal.Route cleanup through the existing scoped ownership checks in
src/lib/onboard/host-gateway-process.ts, or apply equivalent PID-file, runtime-marker, namespace, and command-line checks before each signal. If ownership is not proven, skip signaling and report cleanup failure. Add a decoy-process test intest/managed-image-protected-runtime-contract.test.ts.🤖 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 `@scripts/checks/run-managed-image-openshell-e2e.ts` around lines 299 - 319, Update stopProcess in scripts/checks/run-managed-image-openshell-e2e.ts to prove gateway ownership using the scoped checks from src/lib/onboard/host-gateway-process.ts, or equivalent PID-file, runtime-marker, namespace, and command-line validation before every signal; skip signaling and report cleanup failure when ownership is unproven. Add a decoy-process test covering PID reuse in test/managed-image-protected-runtime-contract.test.ts lines 46-84.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.
Outside diff comments:
In `@scripts/checks/run-managed-image-openshell-e2e.ts`:
- Around line 299-319: Update stopProcess in
scripts/checks/run-managed-image-openshell-e2e.ts to prove gateway ownership
using the scoped checks from src/lib/onboard/host-gateway-process.ts, or
equivalent PID-file, runtime-marker, namespace, and command-line validation
before every signal; skip signaling and report cleanup failure when ownership is
unproven. Add a decoy-process test covering PID reuse in
test/managed-image-protected-runtime-contract.test.ts lines 46-84.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: ae0ba23d-7d12-4818-8c5e-2f54e1809522
📒 Files selected for processing (3)
scripts/checks/run-managed-image-openshell-e2e.tssrc/lib/onboard.tstest/managed-image-protected-runtime-contract.test.ts
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
scripts/checks/run-managed-image-openshell-e2e.ts (1)
134-145: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep the onboard hook inventory canonical.
OnboardModuledeclares five required hooks, and Line 145 repeats the same inventory. A future change can update one list and leave the validator out of sync. Define one runtime hook map and add a compile-time completeness check. Add a valid direct-export fixture beside the existing default-export contract test.As per path instructions: “Derive inventories and limits from a canonical source where possible” and “Require focused tests for both detection and false-positive behavior.”
🤖 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 `@scripts/checks/run-managed-image-openshell-e2e.ts` around lines 134 - 145, Make the required-hook inventory canonical for OnboardModule by defining one runtime hook map whose keys are compile-time checked against all five OnboardModule hooks, then reuse it in resolveManagedImageOnboardModule instead of repeating the string list. Add a valid direct-export fixture alongside the existing default-export contract test, covering both missing-hook detection and acceptance of valid exports without false positives.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 `@scripts/checks/run-managed-image-openshell-e2e.ts`:
- Line 143: Update the candidate selection in the onboard-module loading logic
to use optional chaining on value before accessing default, then retain the
existing validation that checks whether the selected candidate is callable and
emits the intended missing-hook error. Ensure null or undefined inputs reach
that validation instead of causing a native TypeError.
---
Nitpick comments:
In `@scripts/checks/run-managed-image-openshell-e2e.ts`:
- Around line 134-145: Make the required-hook inventory canonical for
OnboardModule by defining one runtime hook map whose keys are compile-time
checked against all five OnboardModule hooks, then reuse it in
resolveManagedImageOnboardModule instead of repeating the string list. Add a
valid direct-export fixture alongside the existing default-export contract test,
covering both missing-hook detection and acceptance of valid exports without
false positives.
🪄 Autofix
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: e58188b1-0b33-4a6f-a2fa-b655fe0baa5e
📒 Files selected for processing (3)
scripts/checks/run-managed-image-openshell-e2e.tssrc/lib/onboard.tstest/managed-image-protected-runtime-contract.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- test/managed-image-protected-runtime-contract.test.ts
- src/lib/onboard.ts
3 participants
Summary
Repair the protected managed-image qualification after Ollama readiness by exposing and validating the onboarding hooks consumed by the harness. Cleanup now yields while reaping the owned gateway and verifies that both its process and configured listener are gone.
Related Issue
Fixes #8759
Changes
runOpenshellandsleepSecondshooks required by the protected harness.Type of Change
Quality Gates
Documentation Writer Review
no-docs-neededDGX 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 integration test/managed-image-protected-runtime-contract.test.ts(24 passed);npm run typecheck:clipassed.npm testfor broad runtime/test-harness changes;npm run checkfor repo-wide validation/coverage changes — command/result: Not applicable; this is a narrow harness contract and cleanup fix with focused regression coverage.npm run docsbuilds without warnings (doc changes only)Signed-off-by: Apurv Kumaria akumaria@nvidia.com
Summary by CodeRabbit
Bug Fixes
Tests