feat(onboard): add inactive OpenShell MXC provider - #8271
Conversation
Signed-off-by: Senthil Ravichandran <senthilr@nvidia.com>
📝 WalkthroughWalkthroughThe change adds native-artifact workload contracts, parsing, validation, sandbox compatibility, and an inactive OpenShell MXC runtime provider for Windows x64 OpenClaw agents. ChangesNative-artifact and MXC provider support
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Receipt as Native-artifact receipt
participant Workload as Workload parser
participant Registry as Runtime-provider registry
participant MXC as MXC provider
Receipt->>Workload: Parse and clone receipt
Workload-->>MXC: Return validated workload receipt
MXC->>Registry: Register provider profile
Registry->>Registry: Validate native-artifact metadata
MXC-->>Receipt: Accept matching platform, agent, and contracts
Possibly related issues
Possibly related PRs
Suggested labels: 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 |
senthilr-nv
added
area: onboarding
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in commit 8239b1a in the TypeScript / code-coverage/cliThe overall coverage in commit 8239b1a in the Show a code coverage summary of the most impacted files.
Updated |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
src/lib/onboard/runtime-provider/contract.ts (1)
85-86: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider one absence representation for
nativeArtifactSupport.Line 85 declares
supportas required and nullable. Line 86 declaresnativeArtifactSupportas optional and nullable. This creates two ways to express "no native-artifact support" and forces every consumer to handle both.registry.tsalready tests!== undefined && !== nullfor this reason.If no persisted profile shape requires the optional form, declare the field required and nullable to match
support.♻️ Proposed alignment with the sibling field
export interface RuntimeProviderWorkloadProfile { readonly support: RuntimeProviderManagedImageSupport | null; - readonly nativeArtifactSupport?: RuntimeProviderNativeArtifactSupport | null; + readonly nativeArtifactSupport: RuntimeProviderNativeArtifactSupport | null; readonly hostArchitectures: readonly string[];This change requires
docker.tsand any other profile literal to setnativeArtifactSupport: nullexplicitly.🤖 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/runtime-provider/contract.ts` around lines 85 - 86, Make nativeArtifactSupport required but nullable in the runtime provider contract, matching the support field and using null as the sole absence representation. Update docker.ts and every runtime provider profile literal to explicitly set nativeArtifactSupport: null when unsupported, and adjust affected consumers to rely on the required nullable field.src/lib/onboard/runtime-provider/registry.ts (2)
230-230: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument why the native-artifact block precedes the
support === nullreturn.The placement at line 230 is load-bearing.
MXC_NATIVE_ARTIFACT_PROFILEsetssupport: null, so the early return at line 268 would skip native-artifact validation if the new block moved below it. A future reader could reorder these blocks and silently disable the check.Add a short comment that states the ordering requirement.
♻️ Proposed comment
+ // Validate native-artifact support before the managed-image early return. + // Native-artifact providers such as `mxc` set `support: null`. if (profile.nativeArtifactSupport !== undefined && profile.nativeArtifactSupport !== null) {Also applies to: 268-268
🤖 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/runtime-provider/registry.ts` at line 230, In the runtime-provider profile handling around the nativeArtifactSupport check and the support === null early return, add a short comment explaining that native-artifact validation must remain before the null-support return because MXC_NATIVE_ARTIFACT_PROFILE uses support: null; preserve the existing ordering and behavior.
237-266: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the shared support-block validator.
Lines 237-266 repeat the structure of the managed-image validator at lines 275-298: a boolean
exactDigestReferences, a non-empty unique allowlisted identity array, and a loop over version arrays that requires non-empty unique positive safe integers. The two blocks now differ only in the allowlist, the field names, and the error text.Extract one helper that takes the record, the allowlist sets, the version field names, and an error-message prefix. Both call sites then stay in sync when a rule changes.
🤖 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/runtime-provider/registry.ts` around lines 237 - 266, Extract the duplicated validation logic from the native-artifact block and the managed-image block into one shared support-block validator. Parameterize it with the support record, platform and agent allowlists, version field names, and error-message prefix, then update both call sites to use it while preserving the existing boolean, non-empty unique allowlisted arrays, and positive safe-integer version checks.src/lib/state/registry/workload.ts (1)
13-13: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffMove the native-artifact receipt contract and parser to a shared module.
src/lib/state/registry/types.tsandsrc/lib/state/registry/workload.tscurrently depend on the transitionalsrc/lib/onboard/workload/native-artifact.ts. This specific module has no path back tosrc/lib/state, but the two directories already have other bidirectional dependencies, includingstate/onboard-session.tsandonboard/machine/events.ts. Keep the persisted receipt contract outsideonboardand update its callers.🤖 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/state/registry/workload.ts` at line 13, Move the native-artifact receipt contract and parseNativeArtifactWorkloadReceiptV1 implementation out of the onboard/workload/native-artifact module into a shared state-level module, then update registry/types.ts and registry/workload.ts imports and callers to use the new location. Preserve the existing persisted receipt shape and parser behavior while removing their dependency on the transitional onboard module.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/runtime-provider/mxc.ts`:
- Line 27: Replace the local STARTUP_PROFILE_CONTRACT_VERSION declaration in the
runtime provider profile with the canonical
MANAGED_STARTUP_PROFILE_SCHEMA_VERSION import, confirming and using the existing
export path from native-artifact.ts. Update startupProfileContractVersions to
reference that imported constant so the declared support matches the parser’s
enforced contract.
In `@src/lib/onboard/sandbox-recreate-transaction.ts`:
- Line 111: Update the replacement matching logic around
workloadReference(replacement.workload) so native-artifact replacements ignore
imageTag and cannot be classified as "image-reused" solely from a stale tag when
workloadReference() returns null. Prefer enforcing imageTag: null during
registration or add an explicit native-artifact guard, and include a regression
test verifying the owned source workload is deleted.
---
Nitpick comments:
In `@src/lib/onboard/runtime-provider/contract.ts`:
- Around line 85-86: Make nativeArtifactSupport required but nullable in the
runtime provider contract, matching the support field and using null as the sole
absence representation. Update docker.ts and every runtime provider profile
literal to explicitly set nativeArtifactSupport: null when unsupported, and
adjust affected consumers to rely on the required nullable field.
In `@src/lib/onboard/runtime-provider/registry.ts`:
- Line 230: In the runtime-provider profile handling around the
nativeArtifactSupport check and the support === null early return, add a short
comment explaining that native-artifact validation must remain before the
null-support return because MXC_NATIVE_ARTIFACT_PROFILE uses support: null;
preserve the existing ordering and behavior.
- Around line 237-266: Extract the duplicated validation logic from the
native-artifact block and the managed-image block into one shared support-block
validator. Parameterize it with the support record, platform and agent
allowlists, version field names, and error-message prefix, then update both call
sites to use it while preserving the existing boolean, non-empty unique
allowlisted arrays, and positive safe-integer version checks.
In `@src/lib/state/registry/workload.ts`:
- Line 13: Move the native-artifact receipt contract and
parseNativeArtifactWorkloadReceiptV1 implementation out of the
onboard/workload/native-artifact module into a shared state-level module, then
update registry/types.ts and registry/workload.ts imports and callers to use the
new location. Preserve the existing persisted receipt shape and parser behavior
while removing their dependency on the transitional onboard module.
🪄 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: 41e47d43-a40f-49d8-bee1-71e24ec26d67
📒 Files selected for processing (11)
src/lib/onboard/runtime-provider/contract.tssrc/lib/onboard/runtime-provider/docker.tssrc/lib/onboard/runtime-provider/mxc.test.tssrc/lib/onboard/runtime-provider/mxc.tssrc/lib/onboard/runtime-provider/registry.tssrc/lib/onboard/sandbox-recreate-transaction.tssrc/lib/onboard/sandbox-workload-authority.test.tssrc/lib/state/registry/types.tssrc/lib/state/registry/workload.tstest/helpers/runtime-provider-bundle.tstest/runtime-provider-source-shape.test.ts
PR Review Advisor — InformationalAdvisor assessment: Informational / low confidence Model lanes
Second-opinion terminology and 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: 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
🤖 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/sandbox-recreate-transaction.test.ts`:
- Around line 182-184: Update the replacement fixture in the test around
SOURCE_ENTRY so its native-artifact workload uses an existing valid receipt
fixture or includes all required artifact, launch, profile, and digest fields.
Remove the type cast, preserve the intentionally stale imageTag, and ensure the
test exercises the real native-artifact receipt contract.
🪄 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: 0c9d04c4-5048-4e5a-a0c1-888f89aaceb4
📒 Files selected for processing (4)
src/lib/onboard/runtime-provider/mxc.tssrc/lib/onboard/sandbox-recreate-transaction.test.tssrc/lib/onboard/sandbox-recreate-transaction.tssrc/lib/onboard/workload/native-artifact.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/lib/onboard/sandbox-recreate-transaction.ts
- src/lib/onboard/runtime-provider/mxc.ts
Signed-off-by: Senthil Ravichandran <senthilr@nvidia.com>
apurvvkumaria
left a comment
apurvvkumaria
left a comment
There was a problem hiding this comment.
Approve — reviewed exact head d10c2ba against base 15b0c55. No blocking correctness, security, regression, or compatibility defect found. MXC remains absent from production selection; the native-artifact parser enforces the bounded identity, digest, path, environment-name, and startup-profile contracts; Docker rejects this receipt kind; and every unqualified lifecycle or mutation surface fails closed. The stale imageTag replacement case is covered without weakening source-workload authority. Focused review verification passed 64/64 tests. Security checklist: PASS across secrets, input validation, authorization, dependencies, logging, cryptography, configuration, tests, and holistic posture. Current required CI failures are in unchanged npm-link-or-shim, shields, and tunnel-service tests (including a timeout), so they do not demonstrate a regression from this patch; required checks should still be green before merge.
Signed-off-by: Senthil Ravichandran <senthilr@nvidia.com>
2 participants
Summary
Adds an inactive OpenShell MXC runtime-provider candidate for native Windows/OpenClaw work. The candidate consumes the merged host-qualification and native-artifact contracts, but remains absent from production selection and fails closed for every unqualified lifecycle or mutation surface.
Related Issue
Related to #8178.
Changes
mxcprovider bundle that reports candidate host facts through feat(onboard): add Windows MXC host qualification #8236 and accepts only validated native-artifact receipts.Type of Change
Quality Gates
CURRENT_RUNTIME_PROVIDER_BUNDLES, has no production import or selection path, and exposes no CLI, configuration, workflow, or supported behavior.8e8a32b73bbc9938018a0980d2a99f0f74720f4eagainst base962f1c3bf3a1354a8cfdc2056e04a27828be0dc0. The effective binary diff SHA-256 remains157885993ef0731db6b77653e778b296d1f15b786431b64832e9da4e3aff42ca, identical to the previously reviewed patch. MXC remains unregistered and unselectable; the strict native-receipt parser and Docker rejection are unchanged; lifecycle and mutation operations fail closed. The merge adds only current-main history outside the effective diff. No secrets, dependencies, network calls, privilege paths, credential handling, authentication, or cryptography changed.Documentation Writer Review
no-docs-needed0385c0423125abbb29aa6028877c3e5c34ef8b46through head8239b1ad312d3e8278f4945537b6264dba405dc2. The immutable compare contains 14 source and test files with 469 insertions and 11 deletions; it contains no documentation or Fern files.CURRENT_RUNTIME_PROVIDER_BUNDLESstill registers only Docker and Kubernetes, so MXC remains unregistered and unselectable. Issue [Epic] Support native Windows through OpenShell MXC #8178 explicitly sequences feat(onboard): add inactive OpenShell MXC provider #8271 as an inactive provider slice and keeps activation and support documentation gated on later package contracts and protected Windows/MXC/OpenClaw live E2E. Changed comments, diagnostics, and behavior-oriented test titles have no writing findings. No user-facing command, configuration, workflow, default, error, or supported behavior changed. A docs build is not applicable.DGX Station Hardware Evidence
scripts/prepare-dgx-station-host.shis unchanged.Verification
Signed-off-by:line and every published commit appears asVerifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passed, ornpm run validate:prpassed after refreshingorigin/mainwhen hooks were skipped or unavailable8e8a32b73: 4 focused files and 64 tests passed.npm run build:cli,npm run typecheck:cli,npm run validate:pr, andgit diff --checkpassed. The effective patch is byte-identical to the previously reviewed head.npm testfor broad runtime/test-harness changes;npm run checkfor repo-wide validation/coverage changes — not applicable because the provider is inactive and unregistered; protected CI is authoritative for the complete repository matrix.npm run docsbuilds without warnings (doc changes only)Signed-off-by: Senthil Ravichandran senthilr@nvidia.com