refactor(onboard): extract inference provider flows into modules (#767) - #4774
Merged
cv merged 4 commits intoJun 4, 2026
Merged
Conversation
Contributor
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Collaborator
|
@BenediktSchackenberg thank you! Could you add a DCO to the PR description, please? |
![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: 3
🧹 Nitpick comments (1)
src/lib/onboard/inference-providers/remote.ts (1)
47-50: ⚡ Quick winMove the remaining provider-specific branches into the config contract.
setupRemoteProviderInferenceis still coupled to two provider-specific rules:"nvidia-nim"must come fromREMOTE_PROVIDER_CONFIG.build, and"compatible-endpoint"gets a special timeout. Those invariants are not expressed inRemoteProviderDeps, so a future config reshuffle can break this module while the shared interface still type-checks. I'd push alias resolution and any apply-time timeout intoRemoteProviderConfigEntry(or a small resolver helper) so this function stays fully data-driven.Example direction
- const config = - provider === "nvidia-nim" - ? REMOTE_PROVIDER_CONFIG.build - : Object.values(REMOTE_PROVIDER_CONFIG).find((entry) => entry.providerName === provider); + const config = Object.values(REMOTE_PROVIDER_CONFIG).find((entry) => + entry.providerNames.includes(provider), + ); ... - if (provider === "compatible-endpoint") { - argsv.push("--timeout", String(LOCAL_INFERENCE_TIMEOUT_SECS)); - } + if (config.applyTimeoutSecs) { + argsv.push("--timeout", String(config.applyTimeoutSecs)); + }That would need companion type/config updates in
src/lib/onboard/inference-providers/types.tsand the config definitions.As per coding guidelines, "Keep function complexity low in JavaScript and TypeScript code."
Also applies to: 107-109
🤖 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/inference-providers/remote.ts` around lines 47 - 50, The function setupRemoteProviderInference currently contains provider-specific branching (resolving "nvidia-nim" to REMOTE_PROVIDER_CONFIG.build and applying a special timeout for "compatible-endpoint"); move those rules into the provider config contract so the function is data-driven: add alias/resolve fields and an optional per-entry timeout to RemoteProviderConfigEntry (or a small resolver helper), update REMOTE_PROVIDER_CONFIG entries to include the "nvidia-nim" alias and the "compatible-endpoint" timeout, and then change setupRemoteProviderInference to always read configuration from REMOTE_PROVIDER_CONFIG (or call the resolver) and remove hard-coded provider checks so it solely consumes RemoteProviderDeps/RemoteProviderConfigEntry data.
🤖 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/onboard/inference-providers/hermes.ts`:
- Around line 11-19: The args type for the Hermes provider currently allows
sandboxName: string | null but the implementation (see usage of sandboxName in
the Hermes provider function, and the failing check at line 56) requires a
non-null sandbox; change the contract to require sandboxName: string (remove |
null) so callers must provide it, or alternatively introduce a provider-specific
subtype (e.g., HermesArgs with sandboxName: string) before dispatching to the
Hermes handler; update all references to the Hermes provider invocation to
satisfy the new non-null sandboxName requirement.
In `@src/lib/onboard/inference-providers/ollama-local.ts`:
- Around line 81-83: The code persists the Ollama proxy token too early by
calling persistAndProbeOllamaProxy(proxyToken) before upsertProvider() and
applyLocalInferenceRoute(), which means the token is stored even if provider
creation or route application fails or the user is sent back; move the
persistAndProbeOllamaProxy(proxyToken) call so it runs only after
upsertProvider(...) completes successfully and after
applyLocalInferenceRoute(...) has returned a success/continue result (i.e.,
after the provider choice is finalized), and ensure it is skipped when
applyLocalInferenceRoute(...) indicates the user backed out or an error
occurred.
In `@src/lib/onboard/inference-providers/routed.ts`:
- Line 45: The call to runOpenshell(["inference", "set", "--no-verify",
"--provider", provider, "--model", model]) should be awaited so onboarding waits
for the route configuration to finish and any errors to propagate; update the
invocation in routed.ts to await runOpenshell(...) (and if inside a non-async
function, make the enclosing function async) so the promise is resolved/rejected
before returning.
---
Nitpick comments:
In `@src/lib/onboard/inference-providers/remote.ts`:
- Around line 47-50: The function setupRemoteProviderInference currently
contains provider-specific branching (resolving "nvidia-nim" to
REMOTE_PROVIDER_CONFIG.build and applying a special timeout for
"compatible-endpoint"); move those rules into the provider config contract so
the function is data-driven: add alias/resolve fields and an optional per-entry
timeout to RemoteProviderConfigEntry (or a small resolver helper), update
REMOTE_PROVIDER_CONFIG entries to include the "nvidia-nim" alias and the
"compatible-endpoint" timeout, and then change setupRemoteProviderInference to
always read configuration from REMOTE_PROVIDER_CONFIG (or call the resolver) and
remove hard-coded provider checks so it solely consumes
RemoteProviderDeps/RemoteProviderConfigEntry data.
🪄 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: 681ac2ee-5b08-4291-a778-2aac63361a00
📒 Files selected for processing (8)
src/lib/onboard.tssrc/lib/onboard/inference-providers/hermes.tssrc/lib/onboard/inference-providers/index.tssrc/lib/onboard/inference-providers/ollama-local.tssrc/lib/onboard/inference-providers/remote.tssrc/lib/onboard/inference-providers/routed.tssrc/lib/onboard/inference-providers/types.tssrc/lib/onboard/inference-providers/vllm-local.ts
Comment on lines
+11
to
+19
| args: { | ||
| sandboxName: string | null; | ||
| model: string; | ||
| provider: string; | ||
| endpointUrl: string | null; | ||
| credentialEnv: string | null; | ||
| hermesAuthMethod: HermesAuthMethod | string | null; | ||
| hermesToolGateways: string[]; | ||
| }, |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Narrow the Hermes contract to require sandboxName.
This function can't actually handle null: Line 56 immediately hard-fails if sandboxName is missing. Keeping the arg nullable weakens the provider interface and pushes a compile-time invariant into runtime. If Hermes always needs a sandbox, make that explicit in the type here (or via a provider-specific subtype before dispatch).
♻️ Suggested shape
export async function setupHermesProviderInference(
args: {
- sandboxName: string | null;
+ sandboxName: string;
model: string;
provider: string;
endpointUrl: string | null;
credentialEnv: string | null;
hermesAuthMethod: HermesAuthMethod | string | null;
hermesToolGateways: string[];
},
deps: HermesDeps,
): Promise<SetupInferenceResult> {
@@
- const targetSandbox = requireValue(sandboxName, "Hermes Provider requires a sandbox name");
+ const targetSandbox = sandboxName;Also applies to: 56-56
🤖 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/inference-providers/hermes.ts` around lines 11 - 19, The args
type for the Hermes provider currently allows sandboxName: string | null but the
implementation (see usage of sandboxName in the Hermes provider function, and
the failing check at line 56) requires a non-null sandbox; change the contract
to require sandboxName: string (remove | null) so callers must provide it, or
alternatively introduce a provider-specific subtype (e.g., HermesArgs with
sandboxName: string) before dispatching to the Hermes handler; update all
references to the Hermes provider invocation to satisfy the new non-null
sandboxName requirement.
Comment on lines
+81
to
+83
| // Persist token now that ollama-local is confirmed as the provider. | ||
| // Not persisted earlier in case the user backs out to a different provider. | ||
| await persistAndProbeOllamaProxy(proxyToken); |
Contributor
There was a problem hiding this comment.
Move proxy-token persistence after the back-out/failure paths.
This runs before both upsertProvider() and applyLocalInferenceRoute(). If either fails—or applyLocalInferenceRoute() sends the user back to provider selection—we still persist Ollama proxy state for a choice that never stuck, which also contradicts the nearby comment.
💡 Suggested change
- let ollamaCredential = "ollama";
+ let ollamaCredential = "ollama";
+ let proxyToken: string | null | undefined;
if (frontOllamaWithProxy) {
// Skip if already started during the fallback recovery above.
if (!proxyReady) ensureOllamaAuthProxy();
- const proxyToken = getOllamaProxyToken();
+ proxyToken = getOllamaProxyToken();
if (!proxyToken) {
console.error(
" Ollama auth proxy token is not set. Re-run onboard to initialize the proxy.",
);
process.exit(1);
}
ollamaCredential = proxyToken;
- // Persist token now that ollama-local is confirmed as the provider.
- // Not persisted earlier in case the user backs out to a different provider.
- await persistAndProbeOllamaProxy(proxyToken);
}
@@
if (await applyLocalInferenceRoute("ollama-local", model)) {
return { done: true, result: { retry: "selection" } };
}
+ if (proxyToken) {
+ await persistAndProbeOllamaProxy(proxyToken);
+ }🤖 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/inference-providers/ollama-local.ts` around lines 81 - 83,
The code persists the Ollama proxy token too early by calling
persistAndProbeOllamaProxy(proxyToken) before upsertProvider() and
applyLocalInferenceRoute(), which means the token is stored even if provider
creation or route application fails or the user is sent back; move the
persistAndProbeOllamaProxy(proxyToken) call so it runs only after
upsertProvider(...) completes successfully and after
applyLocalInferenceRoute(...) has returned a success/continue result (i.e.,
after the provider choice is finalized), and ensure it is skipped when
applyLocalInferenceRoute(...) indicates the user backed out or an error
occurred.
| console.error(` ${routed.result.message}`); | ||
| process.exit(routed.result.status || 1); | ||
| } | ||
| runOpenshell(["inference", "set", "--no-verify", "--provider", provider, "--model", model]); |
Contributor
There was a problem hiding this comment.
Await the OpenShell inference-set command before returning
Line 45 should await runOpenshell(...); otherwise this function can return and advance onboarding before route configuration completes (or before command failure is observed).
Suggested fix
- runOpenshell(["inference", "set", "--no-verify", "--provider", provider, "--model", model]);
+ await runOpenshell([
+ "inference",
+ "set",
+ "--no-verify",
+ "--provider",
+ provider,
+ "--model",
+ model,
+ ]);🤖 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/inference-providers/routed.ts` at line 45, The call to
runOpenshell(["inference", "set", "--no-verify", "--provider", provider,
"--model", model]) should be awaited so onboarding waits for the route
configuration to finish and any errors to propagate; update the invocation in
routed.ts to await runOpenshell(...) (and if inside a non-async function, make
the enclosing function async) so the promise is resolved/rejected before
returning.
…DIA#767) src/lib/onboard.ts had grown to ~7.2k lines with the inference provider selection logic (hermes / ollama-local / vllm-local / remote / routed) inlined across the main onboarding flow. This pulls each provider out behind a small shared interface so the orchestrator can focus on flow control. - New src/lib/onboard/inference-providers/ module: - types.ts — shared provider interface + helper types - hermes.ts - ollama-local.ts - vllm-local.ts - remote.ts - routed.ts - index.ts — registry / dispatcher - onboard.ts is now a thin dispatcher into the provider registry (7204 → 6983 lines; -314 / +93 from the file diff) Pure refactor — no behaviour changes, no UX changes, identical persisted state. Existing tests under src/lib/onboard/ pass unchanged (1072/1072 vitest, tsc clean). Refs NVIDIA#767 NVIDIA#924 Signed-off-by: Benedikt Schackenberg <6381261+BenediktSchackenberg@users.noreply.github.com>
BenediktSchackenberg
force-pushed
the
refactor/onboard-provider-extraction-767
branch
from
June 4, 2026 17:45
e5c57cc to
24e9066
Compare
Contributor
Author
|
Done — amended with a Signed-off-by line and force-pushed. Should bring the DCO check green now. |
Signed-off-by: Benedikt Schackenberg <6381261+BenediktSchackenberg@users.noreply.github.com>
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
3 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.
Refs #767 — follow-up to the CLI migration discussed in #924.
Motivation
src/lib/onboard.tshad grown to ~7.2k lines with the inference provider selection logic for hermes / ollama-local / vllm-local / remote / routed inlined across the main onboarding flow. That made it hard to add or modify a provider without reading the whole orchestrator.Change
Pulled each inference provider behind a small shared interface so
onboard.tscan dispatch through a registry instead of branching inline.New module:
src/lib/onboard/inference-providers/types.ts— shared provider interface + helper typeshermes.tsollama-local.tsvllm-local.tsremote.tsrouted.tsindex.ts— registry / dispatcheronboard.tsis now a thin dispatcher into the registry (7204 → 6983 lines; -314 / +93 on that file).Scope
This is pure structural refactoring — no UX changes, no functional changes, identical persisted state. Aimed at quality > volume per the issue discussion; happy to follow up with the credential / sandbox provider extractions in a second PR once the pattern here gets a thumbs-up.
Verification
npx vitest run src/lib/onboard— 1072 / 1072 green (101 test files)npm run typecheck— cleanDraft on purpose — would love a sanity check on the
ProviderOnboarderinterface shape before I do the rest.Summary by CodeRabbit
Signed-off-by: Benedikt Schackenberg 6381261+BenediktSchackenberg@users.noreply.github.com