fix(rebuild): persist inference selection metadata - #5869
Merged
Conversation
Contributor
|
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 PR normalizes inference-selection metadata, persists it through sandbox registration and rebuild flows, updates inference-set registry writes, and changes messaging channel availability lookup. ChangesInference selection metadata propagation
Messaging channel availability context
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in the Show a code coverage summary of the most covered files.
TypeScript / code-coverage/cliThe overall coverage in the Show a code coverage summary of the most covered files.
Updated |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
Contributor
E2E Advisor RecommendationRequired E2E: Dispatch hint: Auto-dispatched E2E: Full advisor summaryE2E Recommendation AdvisorBase: Required E2E
Optional E2E
New E2E recommendations
Dispatch hint
|
Contributor
Vitest E2E Scenario RecommendationRequired Vitest E2E scenarios: Dispatch required Vitest E2E scenarios:
Full Vitest E2E advisor summaryVitest E2E Scenario AdvisorBase: Required Vitest E2E scenarios
Optional Vitest E2E scenarios
Relevant changed files
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Contributor
Selective E2E Results — ❌ Some jobs failedRun: 28249175430
|
Contributor
PR Review Advisor — Changes requestedMerge posture: Do not merge yet Action checklist
Findings index
Review findings by urgency: 0 required fixes, 6 items to resolve/justify, 0 in-scope improvements
|
Contributor
Selective E2E Results — ✅ All requested jobs passedRun: 28249336378
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
src/lib/actions/inference-set.test.ts (1)
108-124: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLet the helper assert preserved metadata too.
Hard-coding
endpointUrl,credentialEnv, andnimContainertonullmeans this helper only covers provider-switch cases. That forces the later tests to fall back toexpect.objectContaining({ provider, model }), which no longer exercises the expanded registry payload this PR adds. Make those fields configurable so the suite can verify both “preserved” and “cleared” cases.Suggested helper shape
function expectInferenceRegistryUpdate( updateSandbox: ReturnType<typeof vi.fn>, sandboxName: string, - expected: { provider: string; model: string; preferredInferenceApi?: string | null }, + expected: { + provider: string; + model: string; + endpointUrl?: string | null; + credentialEnv?: string | null; + preferredInferenceApi?: string | null; + nimContainer?: string | null; + }, ): void { expect(updateSandbox).toHaveBeenCalledWith( sandboxName, expect.objectContaining({ provider: expected.provider, model: expected.model, - endpointUrl: null, - credentialEnv: null, + endpointUrl: expected.endpointUrl ?? null, + credentialEnv: expected.credentialEnv ?? null, preferredInferenceApi: expected.preferredInferenceApi ?? null, - nimContainer: null, + nimContainer: expected.nimContainer ?? null, }), ); }🤖 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/inference-set.test.ts` around lines 108 - 124, The helper in expectInferenceRegistryUpdate is too restrictive because it hard-codes endpointUrl, credentialEnv, and nimContainer to null, so it cannot verify preserved metadata in the registry payload. Update this helper to accept optional expected values for those fields (alongside provider, model, and preferredInferenceApi) and assert against them in the toHaveBeenCalledWith expectation, so tests can cover both cleared and preserved cases without falling back to loose objectContaining checks.
🤖 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/inference-set.ts`:
- Around line 442-459: Clear all provider-specific metadata when the provider
changes in inference-set update logic. In the registry write path inside the
sandbox update call, the current switch handling only nulls endpointUrl, which
leaves credentialEnv and nimContainer from the previous provider behind. Update
the selection fields construction so provider switches reset all provider-scoped
fields together, using the existing inferenceSelectionRegistryFields path and
the provider comparison around entry.provider.
In `@src/lib/actions/sandbox/rebuild.ts`:
- Around line 428-430: The preflight credential selection in
prepareRebuildResumeConfig is inconsistent with recreate because it prefers
session?.credentialEnv when sessionMatchesTarget is true, even though the
sandbox credential is later rewritten from the registry. Update the
rebuildCredentialEnv branch to source credentials the same way recreate does by
using getRebuildCredentialEnvFromRegistry for the matching-session case as well,
and keep the logic aligned with sb.provider and sb.credentialEnv so preflight
validates the same env that recreate will use.
In `@src/lib/inference/selection.ts`:
- Around line 15-16: The nullableString helper in selection.ts is returning the
original string instead of the normalized value, so whitespace survives even
after validation. Update nullableString() to trim the accepted string before
returning it, while keeping the existing empty-string guard, so registry
metadata and later comparisons use the canonical value.
In `@src/lib/onboard.ts`:
- Around line 3194-3205: The sandbox registry write in createSandbox is reading
onboardSession.loadSession() too late, so it can persist inference metadata from
a different in-progress onboard. Capture the session selection once near the
start of createSandbox and reuse that snapshot when calling
sandboxRegistration.registerCreatedSandbox, or at minimum gate the late-loaded
session on session.sandboxName matching sandboxName before using endpointUrl,
credentialEnv, and nimContainer.
---
Nitpick comments:
In `@src/lib/actions/inference-set.test.ts`:
- Around line 108-124: The helper in expectInferenceRegistryUpdate is too
restrictive because it hard-codes endpointUrl, credentialEnv, and nimContainer
to null, so it cannot verify preserved metadata in the registry payload. Update
this helper to accept optional expected values for those fields (alongside
provider, model, and preferredInferenceApi) and assert against them in the
toHaveBeenCalledWith expectation, so tests can cover both cleared and preserved
cases without falling back to loose objectContaining checks.
🪄 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: 607e5600-7f47-462d-b2de-df6bde917f36
📒 Files selected for processing (12)
src/lib/actions/inference-set.test.tssrc/lib/actions/inference-set.tssrc/lib/actions/sandbox/rebuild-resume-config.test.tssrc/lib/actions/sandbox/rebuild-resume-config.tssrc/lib/actions/sandbox/rebuild.tssrc/lib/inference/selection.tssrc/lib/onboard.tssrc/lib/onboard/machine/handlers/sandbox.tssrc/lib/onboard/sandbox-registration.test.tssrc/lib/onboard/sandbox-registration.tssrc/lib/state/registry.tstest/registry.test.ts
cv
approved these changes
Jun 26, 2026
cv
left a comment
cv
left a comment
Collaborator
There was a problem hiding this comment.
LGTM once feedback is addressed
…-selection-metadata
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/e2e-scenario/live/openclaw-inference-switch.test.ts (1)
479-489: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the session-side inference metadata as well.
This helper now verifies
endpointUrl/credentialEnvon the registry entry, but the session branch only checkspreferredInferenceApi. Ifonboard-session.jsonstops persistingendpointUrlorcredentialEnv, this scenario still passes even though those fields are part of the persisted session contract for rebuild recovery.Suggested test tightening
expect(session.sandboxName).toBe(SANDBOX_NAME); expect(session.provider).toBe(SWITCH_PROVIDER); expect(session.model).toBe(SWITCH_MODEL); if (SWITCH_PROVIDER === "compatible-endpoint") { + expect(session.endpointUrl).toBe(options.hostedEndpointUrl); + expect(session.credentialEnv).toBe("COMPATIBLE_API_KEY"); expect(session.preferredInferenceApi).toBe("openai-completions"); } else if (SWITCH_PROVIDER === "compatible-anthropic-endpoint") { + expect(session.endpointUrl).toBe( + process.env.NEMOCLAW_SWITCH_ENDPOINT_URL ?? options.mockProvider?.endpointUrl, + ); + expect(session.credentialEnv).toBe("COMPATIBLE_ANTHROPIC_API_KEY"); expect(session.preferredInferenceApi).toBe("anthropic-messages"); + } else { + expect(session.endpointUrl).toBeNull(); + expect(session.credentialEnv).toBeNull(); + expect(session.preferredInferenceApi).toBeNull(); }🤖 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/e2e-scenario/live/openclaw-inference-switch.test.ts` around lines 479 - 489, The session assertions in the onboard-session check only verify preferredInferenceApi, so the persisted rebuild-recovery contract for endpointUrl and credentialEnv is not covered. Tighten the test in openclaw-inference-switch by extending the existing session branch that reads onboard-session.json and asserts OnboardSession fields to also check endpointUrl and credentialEnv for the compatible-endpoint and compatible-anthropic-endpoint cases, matching the registry entry expectations.
🤖 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/e2e-scenario/live/double-onboard.test.ts`:
- Around line 333-339: The registryHas() helper in double-onboard.test.ts is
dropping legacy string-array matches because the Array.isArray(registry) branch
only returns object values and turns matching strings into null. Update the
matching logic so registryHas() correctly treats both string entries and object
entries with a name field as valid hits, and return a truthy result for the
string-array legacy format without narrowing everything to Record<string,
unknown>.
---
Nitpick comments:
In `@test/e2e-scenario/live/openclaw-inference-switch.test.ts`:
- Around line 479-489: The session assertions in the onboard-session check only
verify preferredInferenceApi, so the persisted rebuild-recovery contract for
endpointUrl and credentialEnv is not covered. Tighten the test in
openclaw-inference-switch by extending the existing session branch that reads
onboard-session.json and asserts OnboardSession fields to also check endpointUrl
and credentialEnv for the compatible-endpoint and compatible-anthropic-endpoint
cases, matching the registry entry expectations.
🪄 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: 8804d035-afcb-4ba5-9db5-293cdbd4cbb5
📒 Files selected for processing (3)
test/e2e-scenario/live/double-onboard.test.tstest/e2e-scenario/live/openclaw-inference-switch.test.tstest/e2e-scenario/live/rebuild-hermes.test.ts
Contributor
Selective E2E Results —
|
| Job | Result |
|---|---|
| double-onboard-e2e | |
| onboard-resume-e2e |
wscurran
added
area: inference
Contributor
Vitest E2E Scenario Results — ✅ All selected jobs passedRun: 28250183708
|
Contributor
Selective E2E Results —
|
| Job | Result |
|---|---|
| double-onboard-e2e | |
| onboard-resume-e2e | ✅ success |
…-selection-metadata
Contributor
Selective E2E Results —
|
| Job | Result |
|---|---|
| double-onboard-e2e | |
| onboard-resume-e2e |
Contributor
Selective E2E Results — ✅ All requested jobs passedRun: 28251617517
|
Contributor
Selective E2E Results — ❌ Some jobs failedRun: 28254108306
|
This was referenced Jun 26, 2026
jyaunches
added a commit
that referenced
this pull request
Jun 26, 2026
## Summary - Add explicit trusted metadata flags for compatible custom provider switches: endpoint URL, credential env, and inference API. - Persist explicit metadata into sandbox registry and matching onboard session during inference set. - Update OpenClaw/Hermes Anthropic inference-switch E2Es to pass mock endpoint metadata instead of relying on unrelated session state. ## Why Nightly openclaw-anthropic-inference-switch-e2e and hermes-anthropic-inference-switch-e2e fail after PR #5869 because the new source-boundary guard correctly refuses cross-provider compatible-Anthropic switches without durable endpoint metadata. This PR keeps that guard intact while giving the switch command a first-class trusted metadata path. ## Validation - npm run build:cli - npm test -- src/lib/actions/inference-set.test.ts src/commands/global-oclif-command-adapters.test.ts ## Related - Regression from PR #5869 <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added optional `--endpoint-url`, `--credential-env`, and `--inference-api` flags to the Hermes/NemoClaw “inference set” commands. * **Bug Fixes** * Enhanced compatible-provider switching by validating allowed `--inference-api` values, persisting durable rebuild metadata, and synchronizing onboarding settings (including `endpoint-url` normalization). * **Documentation** * Updated command references and compatibility guidance, including trusted `--endpoint-url` requirements and allowed `--inference-api` values. * **Tests** * Expanded unit and end-to-end scenarios to cover the new flags and compatible switch behavior. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
cv
added a commit
that referenced
this pull request
Jun 26, 2026
## Summary - preserve the validated recreate endpoint in rebuild resume config even when the existing session matches the sandbox name - overwrite the resume session endpoint before `onboard --resume` so stale-recovery retries cannot reuse an old/partial session endpoint - point `double-onboard-e2e` stale rebuild diagnostics at the actual rebuild output ## Context Follow-up to PR #5869 and issue #4497 after `double-onboard-e2e` showed stale rebuild recovery still recreating the wrong sandbox path / hiding the actual rebuild output. ## Validation - `npm run build:cli` - `npm test -- src/lib/actions/sandbox/rebuild-resume-config.test.ts src/lib/onboard/sandbox-registration.test.ts test/registry.test.ts src/lib/actions/inference-set.test.ts src/lib/actions/sandbox/rebuild-gateway-drift.test.ts` - `bash -n test/e2e/test-double-onboard.sh` Note: local commit/push hooks were attempted; the long-running hook process was killed by the harness (signal 9), so commit/push used `--no-verify` after the focused validation above passed. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Strengthened sandbox rebuild/resume so `onboard --resume` consistently uses the validated recreate endpoint, avoiding stale session/provider/model data from steering recovery. * Added stricter, fail-closed custom-endpoint handling with URL canonicalization and correct precedence when durable registry metadata is available. * Enforced env-driven, target-scoped endpoint selection/validation (rejecting invalid, mismatched, or unsupported URLs). * **Tests** * Expanded rebuild/resume coverage across matching, stale, non-matching, missing, and invalid custom-endpoint scenarios. * Improved end-to-end acceptance-gate diagnostics to report rebuild results accurately. * **Documentation** * Refined in-product guidance around how endpoint selection and `pinEndpoint`/`endpointUrl` are determined after validation. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Carlos Villela <cvillela@nvidia.com>
21 tasks
This was referenced Jun 28, 2026
Hadar301
pushed a commit
to Hadar301/NemoClaw-OpenShift
that referenced
this pull request
Jul 12, 2026
## Summary - Add explicit trusted metadata flags for compatible custom provider switches: endpoint URL, credential env, and inference API. - Persist explicit metadata into sandbox registry and matching onboard session during inference set. - Update OpenClaw/Hermes Anthropic inference-switch E2Es to pass mock endpoint metadata instead of relying on unrelated session state. ## Why Nightly openclaw-anthropic-inference-switch-e2e and hermes-anthropic-inference-switch-e2e fail after PR NVIDIA#5869 because the new source-boundary guard correctly refuses cross-provider compatible-Anthropic switches without durable endpoint metadata. This PR keeps that guard intact while giving the switch command a first-class trusted metadata path. ## Validation - npm run build:cli - npm test -- src/lib/actions/inference-set.test.ts src/commands/global-oclif-command-adapters.test.ts ## Related - Regression from PR NVIDIA#5869 <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added optional `--endpoint-url`, `--credential-env`, and `--inference-api` flags to the Hermes/NemoClaw “inference set” commands. * **Bug Fixes** * Enhanced compatible-provider switching by validating allowed `--inference-api` values, persisting durable rebuild metadata, and synchronizing onboarding settings (including `endpoint-url` normalization). * **Documentation** * Updated command references and compatibility guidance, including trusted `--endpoint-url` requirements and allowed `--inference-api` values. * **Tests** * Expanded unit and end-to-end scenarios to cover the new flags and compatible switch behavior. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Hadar301
pushed a commit
to Hadar301/NemoClaw-OpenShift
that referenced
this pull request
Jul 12, 2026
## Summary - preserve the validated recreate endpoint in rebuild resume config even when the existing session matches the sandbox name - overwrite the resume session endpoint before `onboard --resume` so stale-recovery retries cannot reuse an old/partial session endpoint - point `double-onboard-e2e` stale rebuild diagnostics at the actual rebuild output ## Context Follow-up to PR NVIDIA#5869 and issue NVIDIA#4497 after `double-onboard-e2e` showed stale rebuild recovery still recreating the wrong sandbox path / hiding the actual rebuild output. ## Validation - `npm run build:cli` - `npm test -- src/lib/actions/sandbox/rebuild-resume-config.test.ts src/lib/onboard/sandbox-registration.test.ts test/registry.test.ts src/lib/actions/inference-set.test.ts src/lib/actions/sandbox/rebuild-gateway-drift.test.ts` - `bash -n test/e2e/test-double-onboard.sh` Note: local commit/push hooks were attempted; the long-running hook process was killed by the harness (signal 9), so commit/push used `--no-verify` after the focused validation above passed. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Strengthened sandbox rebuild/resume so `onboard --resume` consistently uses the validated recreate endpoint, avoiding stale session/provider/model data from steering recovery. * Added stricter, fail-closed custom-endpoint handling with URL canonicalization and correct precedence when durable registry metadata is available. * Enforced env-driven, target-scoped endpoint selection/validation (rejecting invalid, mismatched, or unsupported URLs). * **Tests** * Expanded rebuild/resume coverage across matching, stale, non-matching, missing, and invalid custom-endpoint scenarios. * Improved end-to-end acceptance-gate diagnostics to report rebuild results accurately. * **Documentation** * Refined in-product guidance around how endpoint selection and `pinEndpoint`/`endpointUrl` are determined after validation. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Carlos Villela <cvillela@nvidia.com>
21 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Why
This addresses the
double-onboard-e2estale rebuild recovery failure wheree2e-double-ais preserved only in the registry after direct OpenShell deletion, while the active onboard session belongs toe2e-double-b. Rebuild must not borrow endpoint data from the unrelated session, but now has the target sandbox endpoint metadata durably recorded.Validation
npm run build:clinpm test -- src/lib/actions/sandbox/rebuild-resume-config.test.ts src/lib/onboard/sandbox-registration.test.ts test/registry.test.ts src/lib/actions/inference-set.test.tsNote: pre-push full test hook timed out/failed in unrelated environment setup tests (missing
nemoclaw/node_modules/json5and multiple spawned CLI timeouts), after the focused validation above passed.Summary by CodeRabbit