feat(onboard): define native artifact workload contract - #8243
Conversation
Signed-off-by: Senthil Ravichandran <senthilr@nvidia.com>
senthilr-nv
added
integration: openclaw
📝 WalkthroughWalkthroughThis change adds a native Windows/x64 OpenClaw workload receipt contract. It defines public types and constants, validates artifact and launch metadata, verifies startup-profile integrity, returns normalized receipts, and adds rejection tests. ChangesNative artifact receipt validation
Estimated code review effort: 4 (Complex) | ~45 minutes 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)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (4)
src/lib/onboard/workload/native-artifact.test.ts (2)
70-86: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the complete normalized receipt.
toMatchObjectdoes not verifykind, artifact digest and version, source revision, startup-profile fields, or the required ownership flags. A return-path regression can alter or omit these fields while this test passes.parseNativeArtifactWorkloadReceiptV1returns all of them as public contract data insrc/lib/onboard/workload/native-artifact.tslines 178-337.Compare the parsed result with the complete expected receipt. As per path instructions, prefer observable outcomes 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/onboard/workload/native-artifact.test.ts` around lines 70 - 86, Update the test around parseNativeArtifactWorkloadReceiptV1 to compare parsed against the complete expected normalized receipt using a full equality assertion instead of toMatchObject. Include kind, artifact digest and version, source revision, startup-profile fields, ownership flags, and all existing receipt fields, while asserting only through the public parser result.Source: Path instructions
108-118: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover
workingDirectorypath validation.This table only mutates
launch.executable.relativePath. The parser also validateslaunch.workingDirectorywithrequireRelativePath, so a regression that accepts../agent,agent/../work, or repeated separators in the working directory will pass this suite.Add equivalent rejection cases that mutate
launch(value).workingDirectory. As per path instructions, tests must provide behavioral confidence at 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/onboard/workload/native-artifact.test.ts` around lines 108 - 118, Extend the canonical-path rejection coverage in the parameterized test around parseNativeArtifactWorkloadReceiptV1 by adding equivalent cases that assign each invalid path to launch(value).workingDirectory. Keep the assertions at the public parser boundary and verify every non-canonical working-directory value throws the canonical relative path error.Source: Path instructions
src/lib/onboard/workload/native-artifact.ts (2)
289-302: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueNarrow the try block to the decode call.
decodeManagedStartupProfileis the only realistic throwing path here. The agent-ownership check sits inside the same try, so the code then needs theinstanceofre-throw at Line 297 to escape its own catch. Move the check after the try to remove that round trip.♻️ Proposed restructure
- try { - const profile = decodeManagedStartupProfile(contract.encodedProfile); - if (profile.agent !== agent) { - throw new NativeArtifactWorkloadContractError( - `contract.encodedProfile belongs to '${profile.agent}', not '${agent}'`, - ); - } - } catch (error) { - if (error instanceof NativeArtifactWorkloadContractError) throw error; - throw new NativeArtifactWorkloadContractError( - "contract.encodedProfile failed closed validation", - { cause: error }, - ); - } + let profile; + try { + profile = decodeManagedStartupProfile(contract.encodedProfile); + } catch (error) { + throw new NativeArtifactWorkloadContractError( + "contract.encodedProfile failed closed validation", + { cause: error }, + ); + } + if (profile.agent !== agent) { + throw new NativeArtifactWorkloadContractError( + `contract.encodedProfile belongs to '${profile.agent}', not '${agent}'`, + ); + }🤖 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/workload/native-artifact.ts` around lines 289 - 302, In the validation flow around decodeManagedStartupProfile, narrow the try/catch to only the decode call, preserving the existing wrapped error behavior for decode failures. Move the profile.agent ownership check after the catch so NativeArtifactWorkloadContractError is no longer re-thrown from its own catch.
119-139: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winPath Traversal (CWE-41)
Reachability: Unreachable · Exploitability: Theoretical
Reject Windows-normalizing path segments in
requireRelativePath.The current check already rejects backslashes. Use
bin/claw.exe.as the forward-slash example. Reject trailing dots, trailing spaces, and Windows reserved device names with extensions. Add focused tests. Existing fixturesruntime/node.exeand.remain valid. This is contract hardening for the future staging authority, not a current traversal path.🤖 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/workload/native-artifact.ts` around lines 119 - 139, The requireRelativePath validation must also reject Windows-normalizing segments: trailing dots, trailing spaces, and Windows reserved device names even when followed by extensions, while continuing to accept runtime/node.exe and "." when allowDot is true. Update the segment validation in requireRelativePath using the bin/claw.exe. example and add focused tests covering each rejected form and the preserved valid fixtures.
🤖 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/onboard/workload/native-artifact.test.ts`:
- Around line 70-86: Update the test around parseNativeArtifactWorkloadReceiptV1
to compare parsed against the complete expected normalized receipt using a full
equality assertion instead of toMatchObject. Include kind, artifact digest and
version, source revision, startup-profile fields, ownership flags, and all
existing receipt fields, while asserting only through the public parser result.
- Around line 108-118: Extend the canonical-path rejection coverage in the
parameterized test around parseNativeArtifactWorkloadReceiptV1 by adding
equivalent cases that assign each invalid path to
launch(value).workingDirectory. Keep the assertions at the public parser
boundary and verify every non-canonical working-directory value throws the
canonical relative path error.
In `@src/lib/onboard/workload/native-artifact.ts`:
- Around line 289-302: In the validation flow around
decodeManagedStartupProfile, narrow the try/catch to only the decode call,
preserving the existing wrapped error behavior for decode failures. Move the
profile.agent ownership check after the catch so
NativeArtifactWorkloadContractError is no longer re-thrown from its own catch.
- Around line 119-139: The requireRelativePath validation must also reject
Windows-normalizing segments: trailing dots, trailing spaces, and Windows
reserved device names even when followed by extensions, while continuing to
accept runtime/node.exe and "." when allowDot is true. Update the segment
validation in requireRelativePath using the bin/claw.exe. example and add
focused tests covering each rejected form and the preserved valid fixtures.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: f92ff21b-4239-48ad-904b-2d18a08e1dc4
📒 Files selected for processing (2)
src/lib/onboard/workload/native-artifact.test.tssrc/lib/onboard/workload/native-artifact.ts
Signed-off-by: Senthil Ravichandran <senthilr@nvidia.com>
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in commit e7bde8d in the TypeScript / code-coverage/cliThe overall coverage in commit e7bde8d 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. They do not change the primary assessment or E2E / PR Gate. 1 semantic terminology decisionTerminology decisions are advisory. They affect the assessment only when a separate finding identifies concrete semantic impact.
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. |
|
Maintainer decision on |
…e-workload-contract
Signed-off-by: Senthil Ravichandran <senthilr@nvidia.com>
Signed-off-by: Senthil Ravichandran <senthilr@nvidia.com>
3 participants
Summary
Defines an inactive, versioned receipt for staging the pinned OpenClaw Windows workload without an OCI image. The parser rejects mutable identity, non-canonical paths, literal environment assignments, and mismatched startup intent; it does not register or activate an MXC provider.
Related Issue
Related to #8178.
Changes
Type of Change
Quality Gates
Documentation Writer Review
no-docs-neededDGX Station Hardware Evidence
scripts/prepare-dgx-station-host.shis unchanged.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/onboard/workload/native-artifact.test.tspassed 29/29;npm run typecheck:cli,npm run build:cli,npm --prefix nemoclaw run build,npm exec -- biome checkfor both changed files, andnpm run checks:repositorypassed.npm testfor broad runtime/test-harness changes;npm run checkfor repo-wide validation/coverage changes — not applicable because this isolated parser is not imported by production code or the runtime registry.npm run docsbuilds without warnings (doc changes only)Signed-off-by: Senthil Ravichandran senthilr@nvidia.com
Summary by CodeRabbit
New Features
Tests