fix: let OpenShell routes converge - #9481
Conversation
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan includes up to 12 reviews per rolling hour; 0 remain after this review. 📝 WalkthroughWalkthroughSandbox inference route convergence now retries eligible HTTP 400/404 probe failures after 2 and 4 seconds. Retry logs include status and attempt details. Tests cover exhaustion, rollback, terminal failures, cleanup, and unchanged session state. ChangesSandbox route convergence
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to This PR extends sandbox route verification delays so provider switching can converge without changing terminal failure handling or rollback behavior; no actionable merge-blocking risk remains. Sequence Diagram(s)sequenceDiagram
participant inference_set
participant probe_route
participant retryUntilAsync
participant sandbox
participant session_state
inference_set->>probe_route: validate target route
probe_route->>sandbox: send inference probe
sandbox-->>probe_route: return HTTP 400 or 404
probe_route->>retryUntilAsync: wait 2s or 4s and retry
retryUntilAsync->>probe_route: run next probe
probe_route-->>inference_set: return convergence result
inference_set->>session_state: persist route or restore prior selection
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 |
|
🌿 Preview your docs: https://nvidia-preview-pr-9481.docs.buildwithfern.com/nemoclaw |
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in commit 93fd819 in the Show a code coverage summary of the most impacted files.
TypeScript / code-coverage/cliThe overall coverage in commit 93fd819 in the Updated |
PR Review Advisor — No blocking findings reportedAdvisor assessment: No blocking advisor findings reported Model lanes
3 terminology differences from the second opinionAdvisory only. These are normalized differences from the primary terminology receipt.
Second-opinion terminology and E2E selections are advisory. Live E2E does not run automatically for pull requests. 3 semantic terminology decisionsTerminology decisions are advisory. They affect the assessment only when a separate finding identifies concrete semantic impact.
E2E guidanceAdvisory only. A maintainer can dispatch the default E2E suite for the commit under review. Recommended E2E: Manual-only E2E: 2 optional E2E recommendations
This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge. |
jyaunches
left a comment
There was a problem hiding this comment.
LOC Reduction / Codebase Simplicity Review
Why this blocks
This PR is +123/-6, and 102 of those net new lines are in src/lib/actions/inference-set-compatible-provider.test.ts. The new does not retry a changed-family ... failure table adds about 99 lines for two status values, but it repeats the adjacent changed-family exhaustion case’s complete createCompatibleProviderCapture setup, createDeps state, runInferenceSet call, two-command rollback history, provider deletion check, and no-durable-write assertions. The existing does not retry a target rejection when the API family did not change and restores the prior route when sandbox-only provider verification ... cases repeat the same scenario again.
The only new dimensions are HTTP 401/500, one probe, and no sleep. Those do not require another full switch fixture.
Refactor direction
Make one rejected-compatible-switch scenario helper, or parameterize one table by API-family change, HTTP status, expected probe count, and expected delays. Have the existing same-family 400 case, the changed-family 401/500 cases, and the exhaustion case reuse the same setup and rollback/no-write assertions. Keep only the expectations that differ per row.
Expected result
The terminal-status coverage and exact rollback proof remain, but the new coverage becomes a small table rather than a copied 99-line workflow. This should remove roughly 60–80 lines and make the retry policy easier to audit as one matrix.
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
|
Addressed the LOC/simplicity review in |
Resolved at 93fd819. The rejected compatible-switch cases now share one scenario owner, preserving rollback and no-write assertions while removing 75 net test lines.
jyaunches
left a comment
There was a problem hiding this comment.
LOC Reduction / Codebase Simplicity Review
Resolved at 93fd81965a65976578cfd5c51873100ce0de5828.
runRejectedCompatibleSwitchScenario now owns the compatible-provider setup, switch, rollback command history, provider deletion, no-durable-write assertions, and restored session state. The exhausted-retry, terminal 401/500, and same-family rejection cases retain only their differing probe, delay, and log expectations.
The follow-up removes 75 net test lines from the reviewed commit while preserving the retry-policy matrix. I found no replacement LOC-reduction or codebase-simplicity blocker in the current complete diff. This closes only the prior simplicity review; it is not an approval of other review dimensions.
|
Advisor follow-up for the partial Nemotron attempt:
No source change is needed for these findings: the requested authoritative version evidence, exact live E2E, idempotence basis, and merge sequencing are now recorded. |
Summary
OpenShell 0.0.101 refreshes its sandbox route cache every 5 seconds, but the existing cross-family verification exhausted after 3 seconds.
This change keeps the same three-probe policy while moving the retries to 2 and 4 seconds, so the final sandbox probe occurs after one complete cache-refresh interval.
Related Issue
Fixes #9467
Changes
400/404sandbox route probes by 2 seconds and then 4 seconds.Acceptance Criteria
openclaw-inference-switchpasses on the exact final PR head, reproducing theInference: OpenClaw switches providers and remains responsive / no provider credentialjob that failed in run 32132319706. Its artifact records targetopenclaw-inference-switchas passed on exact head93fd81965a65976578cfd5c51873100ce0de5828in run 32162923306.BRAVE_API_KEY, a current trusted Deep Agents base-image import failure, and a Telegram baseline assertion also reproduced in another PR's full matrix.Type of Change
Quality Gates
400/404, progress logs contain no body, header, endpoint, or credential, HTTP401and500are regression-tested as terminal, and exhaustion restores the prior route and removes the uncommitted provider without durable writes.DGX Station Hardware Evidence
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 src/lib/actions/inference-set-compatible-provider.test.ts: 36 passed;npm run typecheck;npm run typecheck:cli -- --incremental;npm run lint;npm run docs.npm run docsbuilds without warnings (doc changes only)Signed-off-by: Aaron Erickson aerickson@nvidia.com
Summary by CodeRabbit
Bug Fixes
Documentation