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. |
📝 WalkthroughWalkthroughThis PR adds a ChangesState-mutation contract and validation
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant prepareRuntimeProviderStateMutationPlan
participant Canonicalizer
Caller->>prepareRuntimeProviderStateMutationPlan: untrusted plan value
prepareRuntimeProviderStateMutationPlan->>Canonicalizer: validate and canonicalize plan
Canonicalizer-->>prepareRuntimeProviderStateMutationPlan: frozen plan and digests
prepareRuntimeProviderStateMutationPlan-->>Caller: prepared plan
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Comment |
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in commit d167fb8 in the TypeScript / code-coverage/cliThe overall coverage in commit d167fb8 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. 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 warning · 0 suggestionsWarningsWarnings do not block.
|
![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>
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/lib/onboard/runtime-provider/state-mutation.test.ts (2)
241-249: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winTest the UTF-8 byte boundary.
The oversized fixture uses only ASCII. An implementation that counts JavaScript characters instead of UTF-8 bytes still rejects this input.
Add valid multibyte selectors whose serialized character length is within the transport budget but whose UTF-8 byte length exceeds it. Assert the bounded-transport rejection.
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/runtime-provider/state-mutation.test.ts` around lines 241 - 249, Add multibyte UTF-8 selector paths to the existing prepareRuntimeProviderStateMutationPlan test so their JavaScript character count remains within the transport budget while their encoded byte length exceeds it, and assert the same bounded-transport rejection through this public API.Source: Path instructions
29-37: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winProve source-plan isolation.
The PR contract requires accepted plans to be cloned and frozen.
prepared.plan !== sourceonly proves that the outer object differs. A faulty implementation can reuse and freezesource.selectorsor a nested selector while these assertions still pass.After preparation, mutate both selector variants and append an item to
source.selectors. Assert thatprepared.planremains unchanged. Also assert that theprefixselector is frozen.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/runtime-provider/state-mutation.test.ts` around lines 29 - 37, The test currently only proves the outer object is cloned by checking prepared.plan !== source, but does not verify deep isolation of nested structures. After the existing frozen assertions, add mutation tests that modify both selector variants within the source object and append an item to source.selectors, then assert that prepared.plan remains unchanged to prove true isolation from the original source. Also add an assertion that the prefix selector variant is frozen in addition to the existing selector[0] check.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.
Nitpick comments:
In `@src/lib/onboard/runtime-provider/state-mutation.test.ts`:
- Around line 241-249: Add multibyte UTF-8 selector paths to the existing
prepareRuntimeProviderStateMutationPlan test so their JavaScript character count
remains within the transport budget while their encoded byte length exceeds it,
and assert the same bounded-transport rejection through this public API.
- Around line 29-37: The test currently only proves the outer object is cloned
by checking prepared.plan !== source, but does not verify deep isolation of
nested structures. After the existing frozen assertions, add mutation tests that
modify both selector variants within the source object and append an item to
source.selectors, then assert that prepared.plan remains unchanged to prove true
isolation from the original source. Also add an assertion that the prefix
selector variant is frozen in addition to the existing selector[0] check.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: aa8913ba-d332-4929-8554-9b1643e76c06
📒 Files selected for processing (2)
src/lib/onboard/runtime-provider/state-mutation.test.tssrc/lib/onboard/runtime-provider/state-mutation.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/lib/onboard/runtime-provider/state-mutation.ts
wscurran
added
area: architecture
2 participants
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