refactor(inference): route provider lifecycle through adapter - #10724
refactor(inference): route provider lifecycle through adapter#10724rsliter wants to merge 3 commits into
Conversation
Signed-off-by: Rebecca Sliter <571084+rsliter@users.noreply.github.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. |
|
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)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: Your plan provides up to 12 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthroughInference-set provider management now uses an asynchronous OpenShell adapter. Provider metadata includes validated revision information. Diagnostics and mutations await structured adapter results. CLI commands use a 64 KiB output limit. ChangesOpenShell provider flow
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: ⚪ Minimal · up to The change routes inference provider lifecycle operations through the typed adapter while preserving credential handling, redaction, and rollback behavior. No actionable merge-blocking risk remains after normal checks and review. Sequence Diagram(s)sequenceDiagram
participant InferenceSet
participant ProviderAdapter
participant OpenShell
InferenceSet->>ProviderAdapter: inspect provider
ProviderAdapter->>OpenShell: get provider metadata
OpenShell-->>ProviderAdapter: return metadata and revision
InferenceSet->>ProviderAdapter: prepare and commit binding
ProviderAdapter->>OpenShell: create or update provider
OpenShell-->>ProviderAdapter: return mutation result
InferenceSet->>ProviderAdapter: query registered providers on failure
ProviderAdapter->>OpenShell: list providers
OpenShell-->>ProviderAdapter: return provider inventory
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall line coverage in commit 3e37598 in the TypeScript / code-coverage/cliThe overall line coverage in commit 3e37598 in the Show a line coverage summary of the most impacted files.
Updated |
Signed-off-by: Rebecca Sliter <571084+rsliter@users.noreply.github.com>
|
@coderabbitai review |
✅ Action performedReview finished.
|
…/openshell-provider-inference-adapter
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Closing this draft so the inference migration can be rebuilt cleanly on #10895. Compatibility assessment is bound to these exact commits:
This PR is not compatible as written because it still targets the closed #10719 branch and its pre-squash history. I replayed its two unique inference commits onto the exact #10895 head. The core inference consumer changes mostly applied, but the replay conflicted in the shared provider adapter, provider metadata parser, inference diagnostics, test support, and adapter tests. The contracts also diverged. #10895 uses a common mutation result, while this draft expects state-valued create, update, delete, import, and detach results. This draft additionally introduces provider revision metadata and endpointless-profile behavior that must be reconciled deliberately with the new adapter boundary. The fresh implementation should:
The branch is being preserved as reference material. It should not be merged or cherry-picked wholesale. |
## Outcome Messaging onboarding now routes checked-in profile inspection and import, provider lookup and update, and post-update verification through typed `OpenShellProviderAdapter` results. Named gateway targets fail closed when an ambient `OPENSHELL_GATEWAY_ENDPOINT` could redirect the operation, while lookup, update, identity-mismatch, and operational failures retain distinct redacted diagnostics. ## Reason The accepted #9806 slice requires the adapter to own the OpenShell CLI boundary instead of leaving profile and provider lifecycle commands in onboarding. This is a fresh implementation from current `main`; it does not merge or cherry-pick the closed #10719 attempt. ### Related issues - Part of #9806 - Replaces the adapter foundation attempted in #10719 - Relates to #9813, which owns migration of the remaining raw CLI consumers - Provides a fresh base on which the separate #10724 and #10726 consumer slices can be restacked ## Changes - Add typed provider metadata, lookup, update, profile-import, and verification results to `OpenShellProviderAdapter`. - Move checked-in profile parsing, validation, import, export, and exact contract verification behind the CLI adapter. - Move provider metadata and diagnostic parsing to the adapter layer, retaining only the narrow exact-not-found classification needed at the CLI boundary. - Route messaging provider publication and managed-clone reconciliation through exact adapter calls. - Preserve safe redacted lookup and update failure details while keeping identity mismatch distinct from operational failure. - Preserve both provider and temporary-source cleanup failures when preparation aborts. - Reject named-target operations when ambient `OPENSHELL_GATEWAY_ENDPOINT` is present. - Bind both ordinary and Hermes portable sandbox creation to the same provider-effect boundary, including deferred post-identity effects and resume replay protection. - Add tests for exact `getProvider`, `updateProvider`, profile-import, and verification calls, including raw-command exclusion and repeatable desired-state recovery after partial publication. ## Verification - On exact candidate tree `7e3490e69`, the focused CLI suite passed 253 tests across seven files, covering adapter get/update/profile results, provider publication, ordinary creation, Hermes portable creation, and the real superseded portable transaction. The published signed candidate `364aa89d4` has that exact tree. - `npx vitest run --project integration test/onboarding/onboard-hermes-portable-provider-publication.test.ts`: four public-boundary cases passed on `364aa89d4`, covering pre-create publication, deferred post-verification publication, verified-resume suppression, superseded-path exclusion, exact named-gateway adapter calls, and isolation from ambient XDG paths. - `npm run typecheck:cli`: passed on `364aa89d4` after the final canonical `main` refresh. - Targeted Oxlint for the changed TypeScript files: passed. - `npm run validate:pr`: passed on exact signed candidate `364aa89d4` against canonical `main` `d836ccb44` in a clean isolated checkout, including repository checks, secret scanning, source-shape checks, growth guardrails, commit policy, and the CLI pre-push type check. - The full manual-stage coverage pass completed successfully. The broader all-files pre-commit sweep passed every check except existing Hadolint warnings in unchanged Dockerfiles; the identical Hadolint failure was reproduced on canonical `main` `3509b5a43` before the subsequent `main` refresh. - `git diff --check origin/main...HEAD`: passed. - `npm run review:local`: previously failed before analysis because its desktop bootstrap did not forward the active Colima `DOCKER_HOST`; no artifacts were produced. Per maintainer direction, repairing that unrelated local-review tooling is deferred. - PR Review Advisor: three full runs on `eb2be67fd` produced no specialist artifacts and were deferred as unavailable infrastructure evidence. On final head `c7217b54b`, run `33759103328` succeeded for all nine specialists. Every specialist summary and full JSONL session was read; no specialist reported a change-required finding. - Diff inspection: no secrets, API keys, or credentials are present. ## Review notes - CodeRabbit's final incremental review covered `eb2be67fd..c7217b5` and produced no actionable comments. Its generated summary retained an older Hermes portable risk sentence, but CodeRabbit rechecked commits `1abb93872` and `49bc39aef`, verified all four portable lifecycle cases, and explicitly classified that sentence as stale. All review threads are resolved. - CodeRabbit's docstring-coverage warning is a generic advisory, not a repository gate. Adding docstrings across 51 touched existing functions would broaden this migration without protecting the requested adapter behavior, so no change is required. - The remaining raw provider consumers are intentionally unchanged for #9813. - The diagnostic parser could not be removed entirely because the CLI adapter still needs exact absence classification. It is no longer an onboarding shim and now lives at the adapter boundary. - The branch includes canonical `main` through `d836ccb44`, including the remediation for the inherited `fast-uri` advisories, the package-contract npm fix from #10986, the stricter existing-profile validation protocol from #10884, and the Advisor on-demand diff-reading fix from #10952. - #10724 and #10726 are broader drafts stacked on the closed #10719 branch. This PR does not close or modify them. --- Signed-off-by: Rebecca Sliter <571084+rsliter@users.noreply.github.com> --------- Signed-off-by: Rebecca Sliter <571084+rsliter@users.noreply.github.com> Co-authored-by: Rebecca Sliter <571084+rsliter@users.noreply.github.com> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Outcome
NemoClaw inference provider inspection, mutation, and rollback now use the typed OpenShell provider adapter. CLI argv, provider metadata parsing, timeouts, credential environment transfer, and redaction stay inside the CLI adapter.
Reason
Phase 1 of the OpenShell migration requires provider consumers to stop parsing CLI output directly. This is the inference consumer portion of #9806 and builds on the provider adapter foundation in #10149 and onboarding capabilities in #10719.
Related issues
Partial #9806
Depends on #10719
Relates to #9809
Relates to #9813
Changes
Verification
npm --prefix nemoclaw run build: passednpm run build:cli: passednpm run typecheck:cli: passednpm run checks:repository: passed at 1,856 files, 5,864 edges, 0 cycles, and 2,621 test candidatesgit diff --check: passedReview notes
This is one independently reviewable inference consumer change, not the completion of #9806. The raw
openshell inference setroute operation is deliberately unchanged and remains tracked by #9809. Messaging, MCP, recovery, cleanup, and the final consumer disposition remain deferred under #9806 and #9813.docs-not-needed910c4ea1705d5240d4c5415e65cd07b12e8751bethrough merge commit3e37598e05be83e52d7397abf873ffe605a3bf87. The issue patch matches reviewed commit58bedf853bc253dc0cc30e73b4eda14c045789b4. The shared metadata file contains both the approved Phase 1 slice 11: Complete the OpenShell CLI consumer sweep #9813 comment and the reviewed private-revision boundary. No command, option, diagnostic, lifecycle, or other documented user behavior changes.npm run typecheck:clipassed;git diff --checkpassed.docs_review_9806_onboarding)Signed-off-by: Rebecca Sliter 571084+rsliter@users.noreply.github.com
Summary by CodeRabbit