feat(runtime): define provider state-mutation contract - #8186
Conversation
Signed-off-by: Julie Yaunches <jyaunches@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 Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR adds a ChangesState-mutation contract and validation
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
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. 3 semantic terminology decisionsTerminology 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: 1 optional E2E recommendation
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.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/lib/onboard/runtime-provider/state-mutation.test.ts (2)
39-53: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe test title claims scope sensitivity, but the test does not assert it.
The test changes
intentandprojectionSha256only. It never changesselectorsorstateRoot, so it does not prove thatplanSha256binds the plan scope. Add a case that changes the selector set. A provider that trustsplanSha256depends on that property.♻️ Proposed additional case
const changedProjection = prepareRuntimeProviderStateMutationPlan({ ...plan(), projectionSha256: "b".repeat(64), }); + const changedScope = prepareRuntimeProviderStateMutationPlan({ + ...plan(), + selectors: [{ kind: "path", path: "scripts" }], + }); expect(protectionTransition.planSha256).not.toBe(restore.planSha256); expect(changedProjection.planSha256).not.toBe(restore.planSha256); + expect(changedScope.planSha256).not.toBe(restore.planSha256); expect(changedProjection.projectionSha256).toBe("b".repeat(64));🤖 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/state-mutation.test.ts` around lines 39 - 53, Extend the test around prepareRuntimeProviderStateMutationPlan to add a plan variant with a changed selectors set, while keeping intent and projectionSha256 unchanged. Assert that this variant’s planSha256 differs from restore.planSha256, proving selector scope is included in the digest.
91-121: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winBind each rejection case to its own error message.
failprefixes every message with "Runtime provider state-mutation plan is invalid", so/state-mutation plan is invalid/umatches every validation error. Each case in this table passes when the plan is rejected for any reason, including a reason unrelated to its label. Add an expected-message column so each case proves the cause it names.Based on path instructions: "Flag copied production algorithms, broad mocks that bypass the behavior under test, and conditionals that make a test pass without exercising its claim."
♻️ Proposed change to assert the specific cause
it.each([ - ["relative state root", () => ({ ...plan(), stateRoot: "sandbox/.hermes" })], - ["filesystem root", () => ({ ...plan(), stateRoot: "/" })], - ["system state root", () => ({ ...plan(), stateRoot: "/etc/nemoclaw" })], - ["state-root traversal", () => ({ ...plan(), stateRoot: "/sandbox/../etc" })], + ["relative state root", () => ({ ...plan(), stateRoot: "sandbox/.hermes" }), /state root/u], + ["filesystem root", () => ({ ...plan(), stateRoot: "/" }), /state root/u], + ["system state root", () => ({ ...plan(), stateRoot: "/etc/nemoclaw" }), /state root/u], + ["state-root traversal", () => ({ ...plan(), stateRoot: "/sandbox/../etc" }), /state root/u], [ "relative-path traversal", () => ({ ...plan(), selectors: [{ kind: "path", path: "scripts/../../etc" }], }), + /canonical relative path/u, ], [ "control characters", () => ({ ...plan(), selectors: [{ kind: "path", path: "scripts\u0000escape" }], }), + /bounded exact string/u, ], [ "uppercase projection digest", () => ({ ...plan(), projectionSha256: "A".repeat(64), }), + /lowercase SHA-256/u, ], - ])("rejects %s (`#7744`)", (_label, value) => { - expect(() => prepareRuntimeProviderStateMutationPlan(value())).toThrow( - /state-mutation plan is invalid/u, - ); + ])("rejects %s (`#7744`)", (_label, value, expected) => { + expect(() => prepareRuntimeProviderStateMutationPlan(value())).toThrow(expected); });🤖 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/state-mutation.test.ts` around lines 91 - 121, Update the parameterized rejection cases in the test around prepareRuntimeProviderStateMutationPlan to include an expected error-message pattern for each labeled invalid input, then assert that case-specific pattern instead of the shared /state-mutation plan is invalid/u prefix. Ensure every case verifies the validation reason it is intended to cover, including path, traversal, control-character, and projection-digest failures.Source: Path instructions
src/lib/onboard/runtime-provider/state-mutation.ts (1)
19-19: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
PREFIX_PATTERNaccepts.and..as complete prefixes.
canonicalRelativePathrejects the segments.and..at Line 121.PREFIX_PATTERNdoes not apply the same rule, so{ kind: "prefix", prefix: ".." }and{ kind: "prefix", prefix: "." }pass validation. The prefix cannot contain/or\, so it cannot compose a path escape today, and no provider consumes the surface yet. A future provider that matches directory entries belowstateRootwould match the..and.entries themselves. Reject both values in the validator so the prefix selector keeps the same traversal rules as the path selector.♻️ Proposed change to reject dot prefixes
const prefix = boundedString(selector.prefix, `selector ${String(index)} prefix`, 128); if (!PREFIX_PATTERN.test(prefix)) fail(`selector ${String(index)} prefix is not canonical`); + if (prefix === "." || prefix === "..") { + fail(`selector ${String(index)} prefix is not canonical`); + } return Object.freeze({ kind: "prefix", prefix });Also applies to: 152-156
🤖 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/state-mutation.ts` at line 19, Update PREFIX_PATTERN and its corresponding validation at the later prefix-selector path to reject the complete values "." and "..", while continuing to allow other valid alphanumeric, dot, underscore, and hyphen prefixes up to 128 characters. Keep the existing canonicalRelativePath traversal rules consistent without changing unrelated validation.
🤖 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 `@test/runtime-provider-source-shape.test.ts`:
- Around line 133-136: Update the forbidden API assertions in the runtime
provider source-shape test to also reject the provider terms kubernetes and k8s
and the process APIs exec, execSync, and fork, while preserving the existing
checks for docker, podman, hermes, mxc, child_process, execFile, spawn, shell,
command, and callback.
---
Nitpick comments:
In `@src/lib/onboard/runtime-provider/state-mutation.test.ts`:
- Around line 39-53: Extend the test around
prepareRuntimeProviderStateMutationPlan to add a plan variant with a changed
selectors set, while keeping intent and projectionSha256 unchanged. Assert that
this variant’s planSha256 differs from restore.planSha256, proving selector
scope is included in the digest.
- Around line 91-121: Update the parameterized rejection cases in the test
around prepareRuntimeProviderStateMutationPlan to include an expected
error-message pattern for each labeled invalid input, then assert that
case-specific pattern instead of the shared /state-mutation plan is invalid/u
prefix. Ensure every case verifies the validation reason it is intended to
cover, including path, traversal, control-character, and projection-digest
failures.
In `@src/lib/onboard/runtime-provider/state-mutation.ts`:
- Line 19: Update PREFIX_PATTERN and its corresponding validation at the later
prefix-selector path to reject the complete values "." and "..", while
continuing to allow other valid alphanumeric, dot, underscore, and hyphen
prefixes up to 128 characters. Keep the existing canonicalRelativePath traversal
rules consistent without changing unrelated validation.
🪄 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: 6186b180-dbc3-4b84-91cc-4cff42d801c2
📒 Files selected for processing (10)
src/lib/onboard/managed-workload-rebuild-transaction.test.tssrc/lib/onboard/runtime-provider/access.tssrc/lib/onboard/runtime-provider/contract.tssrc/lib/onboard/runtime-provider/docker.tssrc/lib/onboard/runtime-provider/registry.tssrc/lib/onboard/runtime-provider/runtime-provider-contract.test.tssrc/lib/onboard/runtime-provider/state-mutation.test.tssrc/lib/onboard/runtime-provider/state-mutation.tstest/helpers/runtime-provider-bundle.tstest/runtime-provider-source-shape.test.ts
| expect(providerContract.stateMutation).not.toMatch(/\b(?:docker|podman|hermes|mxc)\b/iu); | ||
| expect(providerContract.stateMutation).not.toMatch( | ||
| /(?:child_process|execFile|spawn|shell|command|callback)/iu, | ||
| ); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Cover all forbidden provider and process APIs.
The test accepts kubernetes, k8s, exec, execSync, and fork. A later state-mutation.ts change can add provider routing or process execution through these names and still pass.
Add these names to the forbidden patterns.
🤖 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 `@test/runtime-provider-source-shape.test.ts` around lines 133 - 136, Update
the forbidden API assertions in the runtime provider source-shape test to also
reject the provider terms kubernetes and k8s and the process APIs exec,
execSync, and fork, while preserving the existing checks for docker, podman,
hermes, mxc, child_process, execFile, spawn, shell, command, and callback.
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
1 participant
Summary
This PR defines a dormant, provider-neutral
stateMutationcontract. Docker, Kubernetes, and the MXC test fixture remain explicitly unsupported, so runtime behavior does not change.AgentDefinitionissue #8006 and implementation PR #8143, Docker implementation and first consumer #8010, shared state engine #8009, and Hermes adapter #7806.Related Issue
Related to #7744.
Changes
stateMutationfacet toRuntimeProviderBundle.AgentDefinitionprojection, with bounded selectors and stable plan/projection SHA-256 bindings.ericksoa(feat(onboard): add managed bootstrap image runtime #8045, feat(images): package and publish all-agent managed images #8047, feat(runtime): add durable Podman bootstrap authority #8052, feat(runtime): add transactional Podman bootstrap preparation #8055, feat(runtime): start exact Podman image bootstrap #8056, feat(runtime): persist engine lifecycle recovery #8058, feat(runtime): manage Podman host-local inference #8061–feat(runtime): persist host-local inference ownership #8069, test(images): add protected multiarch build contract #8075–fix(onboard): preserve durable journal compatibility #8080, and fix(onboard): retain durable cleanup recovery #8083). None provides the provider-owned exact-runtime mutation authority required here. The branch remains based onmain, and no PR was copied wholesale. The mandatory-facet and provider-neutral source-guard patterns were independently reimplemented.Type of Change
Quality Gates
d167fb83c. Two adversarial findings—inherited serialization hooks and non-scalar Unicode aliases—were fixed. All nine categories then passed with no remaining findings; the reviewed 10-file diff has SHA-256b9366ef6b8816a8f05b11537de54b4068c7c1976c1a96af6e201935134e24d42.Documentation Writer Review
no-docs-neededd167fb83c. The change defines and hardens a dormant internal provider contract; every current provider remains explicitly unsupported, and no CLI, configuration, output, default, workflow, documentation route, or supported behavior changes.DGX 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 unavailablenpm run docsbuilds without warnings (doc changes only)Signed-off-by: Julie Yaunches jyaunches@nvidia.com
Summary by CodeRabbit
New Features
Bug Fixes