fix(doctor): warn on incomplete lifecycle registration#7406
Conversation
Signed-off-by: ScarabSystems <scarab.systems@yahoo.com>
|
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 sandbox doctor now validates lifecycle-registration metadata and gateway bindings, reports incomplete or invalid states, integrates these checks into doctor output, documents the behavior, and adds focused and end-to-end test coverage. ChangesSandbox lifecycle registration diagnostics
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant NemoClawCLI
participant runSandboxDoctor
participant resolveSandboxGatewayName
participant collectDoctorChecks
participant buildLifecycleRegistrationCheck
participant collectLifecycleRegistrationIssues
NemoClawCLI->>runSandboxDoctor: request doctor report
runSandboxDoctor->>resolveSandboxGatewayName: resolve registered gateway binding
resolveSandboxGatewayName-->>runSandboxDoctor: gateway name or failure
runSandboxDoctor->>collectDoctorChecks: pass nullable gateway name
collectDoctorChecks->>buildLifecycleRegistrationCheck: pass registered sandbox metadata
buildLifecycleRegistrationCheck->>collectLifecycleRegistrationIssues: validate lifecycle fields
collectLifecycleRegistrationIssues-->>buildLifecycleRegistrationCheck: issues or no issues
buildLifecycleRegistrationCheck-->>collectDoctorChecks: lifecycle check status
collectDoctorChecks-->>runSandboxDoctor: gateway and sandbox checks
runSandboxDoctor-->>NemoClawCLI: render doctor report
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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 `@src/lib/actions/sandbox/doctor-lifecycle-registration.ts`:
- Around line 78-86: The lifecycle validation around hasCustomImageEvidence and
hasManagedImageEvidence currently reports the same blank nemoclawVersion as both
invalid and missing. Ensure the missing-issue branch only runs when no invalid
issue was added for that field, while preserving the existing invalid
classification for blank or otherwise invalid values and the missing
classification when metadata is truly absent.
🪄 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: 5d26c25d-4823-4fd8-805b-f1c16eac6537
📒 Files selected for processing (4)
src/lib/actions/sandbox/doctor-flow.test.tssrc/lib/actions/sandbox/doctor-lifecycle-registration.test.tssrc/lib/actions/sandbox/doctor-lifecycle-registration.tssrc/lib/actions/sandbox/doctor.ts
PR Review Advisor — InformationalAdvisor assessment: Informational / medium confidence Model lanes
Nemotron output stays in workflow artifacts and does not change the assessment above. E2E guidanceAdvisory only. E2E / PR Gate selects and runs jobs independently. Recommended E2E: 1 optional E2E recommendation
This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/lib/actions/sandbox/doctor-lifecycle-registration.ts (1)
71-71: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd public-boundary coverage for invalid
gatewayName.The new
gatewayNamevalidation is not exercised by the added tests. Add abuildLifecycleRegistrationCheck(..., sandbox({ gatewayName: " " }), ...)case assertingwarnandinvalid gatewayName.As per path instructions, tests should prove observable behavior through the public boundary.
🤖 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/doctor-lifecycle-registration.ts` at line 71, Add a public-boundary test in the lifecycle registration checks using buildLifecycleRegistrationCheck with sandbox({ gatewayName: " " }); assert that the result has warn severity and reports “invalid gatewayName.”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/actions/sandbox/doctor-lifecycle-registration.ts`:
- Around line 70-75: Move the pure lifecycle validation policy surrounding
addPresenceIssue, hasCustomImageEvidence, and the field validators into a
domain-layer module under src/lib/domain, preserving the existing mappings and
issue-collection behavior. Update doctor-lifecycle-registration.ts to call the
domain policy while retaining only doctor-specific orchestration and formatting
in the action module.
---
Nitpick comments:
In `@src/lib/actions/sandbox/doctor-lifecycle-registration.ts`:
- Line 71: Add a public-boundary test in the lifecycle registration checks using
buildLifecycleRegistrationCheck with sandbox({ gatewayName: " " }); assert that
the result has warn severity and reports “invalid gatewayName.”
🪄 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: c4c93f36-7871-4b12-8825-c1dfcdbae172
📒 Files selected for processing (3)
src/lib/actions/sandbox/doctor-flow.test.tssrc/lib/actions/sandbox/doctor-lifecycle-registration.test.tssrc/lib/actions/sandbox/doctor-lifecycle-registration.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/lib/actions/sandbox/doctor-flow.test.ts
wscurran
added
area: cli
|
✨ Thanks for the PR. This adds a read-only doctor check for incomplete sandbox lifecycle registration metadata. Maintainers will review the validation logic and CLI integration. Related open issues: Related open issues: |
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
prekshivyas
left a comment
prekshivyas
left a comment
There was a problem hiding this comment.
Reviewed at 6bc39b7. Lifecycle validation is read-only and redacted; invalid gateway bindings do not trigger probes against the wrong gateway. The CI-reproduced fixture gap is fixed and focused suites pass. Required E2E remains independently enforced.
Record NVIDIA/NemoClaw#7406 as an accepted #7387 repair after upstream merge. Refresh NVIDIA/NemoClaw#7254 as accepted test coverage for #6042 while noting the issue remains open and the merged PR was test-only. Refresh NVIDIA/NemoClaw#7291/#7273 as closed without merge so the Field Lab no longer reports that PR as open. Verification: gh pr/issue views for #7254, #7406, #7291, #6042, #7387, and #7273; git diff --cached --check; public leakage scan for local/private/SDS terms.
|
Woohoo! Thank You! very glad to help. 🙏 |
4 participants
Summary
Fixes #7387.
Validation
npm run build:clinpm run typecheck:clinpx vitest run --project cli src/lib/actions/sandbox/doctor-lifecycle-registration.test.ts src/lib/actions/sandbox/doctor-flow.test.tsnpm run test:changednpm run check:diffNo credentialed or live Brev environment was used for this PR.
Type of Change
Quality Gates
Documentation Writer Review
docs-updateddocs/reference/commands.mdx; generated OpenClaw, Hermes, and Deep Agents variants contain the lifecycle-readiness and invalid-binding behavior. Focused lifecycle tests (24), the CI-reproduced gateway-token integration tests (12), the expanded doctor suites (31), CLI/plugin builds, CLI type-checking, normal hooks, andnpm run docspassed. The final fixture-only commit did not change docs inputs.Verification
Signed-off-by:line and every commit appears asVerifiedin GitHubnpm run docspassed with 0 errorsSigned-off-by: ScarabSystems scarab.systems@yahoo.com
Disclosure: This PR was prepared with AI assistance under human direction and review.
Summary by CodeRabbit
doctorreport to validate lifecycle registration metadata for snapshot/rebuild/upgrade/recovery/reboot, including durable dashboard/gateway port requirements and actionable guidance to re-register/re-onboard when incomplete or invalid.doctorreports the failure and skips gateway probing/recovery selection.doctorcommand reference to cover lifecycle validation and dashboard scoping.Advisor infrastructure waiver
The secondary Nemotron advisor timed out during final synthesis after completing an empty finding ledger, all nine security categories as PASS, and a reconciliation verdict that the PR is safe to merge. The primary Terra advisor passed, the published review has no actionable finding, and no review thread is unresolved. Maintainer review accepts this as an advisor infrastructure timeout, not a code failure.