Add deployment provider credential routing - #4557
Conversation
…-runtime # Conflicts: # apps/daemon/src/routes/chat.ts # apps/web/src/components/ProjectView.tsx
|
Hey @dredozubov — the rationale and validation notes here are helpful. One quick PR-body follow-up before pool review: could you add a short What users will see section from the user's point of view, even if the answer is mainly "existing deployment-backed API flows work without client-side credentials or repeated endpoint setup"? Also, this PR touches behavior that will need a manual QA pass before merge, so please hold off self-merging for now — we'll loop QA in once it's merge-ready (and after the required product sign-off on this head). |
nettee
left a comment
There was a problem hiding this comment.
Two blocking issues are still present in the deployment-provider path on the current head.
🔁 Powered by Looper · runner=reviewer · agent=codex · An autonomous AI dev team for your GitHub repos.Location: apps/daemon/src/chat-routes.ts RIGHT line 150
deploymentProviderRunMetadata() now sits directly on the /api/proxy/openai/stream request path, but this bootstrap call still does a raw fetch() with no abort signal, no bounded timeout, and no proxyDispatcherRequestInit. That means a slow run-session service can stall the chat request before SSE starts, and deployments that require the configured outbound proxy for provider egress can fail here even though the actual provider call would succeed. Please thread request cancellation and a timeout into this helper, send it through the same proxy dispatcher used for provider egress, and add coverage for a stalled or proxy-routed run-session endpoint.
Inline comment could not be anchored: inline anchor is outside the PR diff anchorable ranges
|
Hey @dredozubov — the latest current-head blocker from @nettee looks narrower than the earlier deployment-provider passes: the Kimi direct I'd drive that review thread next before anything else. |
|
Pushed What changed:
Validation run locally under the available Node 26 shell (repo warns it wants Node ~24):
Read-only cross-review found no remaining accepted blocker in the pushed diff. |
nettee
left a comment
There was a problem hiding this comment.
@dredozubov I re-checked the current head with focus on the changed ranges on b97dc14ab0187a4c692257bdf29dba2885114d0d: the Kimi direct-mode runtime follow-up in apps/daemon/src/runtimes/defs/kimi.ts, the matching MCP/prompt-budget/runtime coverage, and the surrounding deployment-provider routing, contract, and web follow-through that remained in scope on this head. Those deltas line up with the earlier review feedback, and I didn’t find any new actionable issues in the current changed ranges. I wasn’t able to execute the local Vitest or typecheck slices in this worktree because node_modules / vitest are not installed here, but the code and targeted test updates are internally consistent. Nice work closing out the Kimi follow-up without re-opening the deployment-provider regressions.
|
Maintenance pass on current head
Remaining blockers look external to the branch diff: GitHub still reports |
|
Hey @dredozubov — thanks for the state check. I re-checked the current PR state on this head: product is already signed off, and the remaining gate on this branch is still design ( I've re-routed the design check on this head. If this branch stays the intended merge path once design clears, QA can pick it up from there. |
|
Hey @dredozubov — @nettee’s latest current-head review is the one to drive next. From the scoped onboarding/settings diff on this head, the blocker looks narrowly focused on the config-persist path: failed deployment-provider saves still need to surface as failures instead of letting onboarding continue as if the write succeeded. |
nettee
left a comment
There was a problem hiding this comment.
@dredozubov I re-checked the current head with focus on the deployment credential-routing changes in the daemon, the redacted /api/provider-orchestrator/config plus od provider config surfaces, the run-session plumbing across model discovery / connection test / proxy / finalize, and the onboarding/settings/model-switcher follow-through in the web app. The changed ranges line up with the prior review history and the added regression coverage, and I didn’t find any new actionable issues on 068a69753c4274c76e5b1b95a58a96b9571ea612. I wasn’t able to execute the local Vitest or typecheck slices in this worktree because node_modules are not installed here, but the current code and targeted tests are internally consistent. Nice work closing out the deployment-provider flow while keeping browser-visible state redacted.
nettee
left a comment
There was a problem hiding this comment.
@dredozubov I re-checked the current head with focus on the changed deployment credential-routing surface: the daemon-side deployment provider resolution and optional run-session bootstrap, the redacted /api/provider-orchestrator/config plus od provider config mirror, the model-discovery / connection-test / proxy / finalize plumbing, and the onboarding/settings/model-switcher follow-through in the web app. The current changed ranges line up with the prior review history and the added regression coverage, and I didn’t find any new actionable issues on b4712fa6c294c08986c1ae96edf96e7536d44bea. Nice work closing out the deployment-provider flow while keeping browser-visible state redacted.
|
Closing as superseded by #5257. The compatible #4557 follow-ups are now in #5257 at 197eaf4 (deployment onboarding save-failure gate, topbar model switcher visibility fix, and test stabilization). #5257 deliberately retains the deployment credential routing and Kimi ACP runtime policy, so the direct-Kimi divergence from this branch is not carried forward. |
|
Thanks for closing the loop and for linking #5257. We'll treat this branch as superseded and follow the compatible deployment-provider follow-ups there. |
Why
Some self-host deployments manage provider credentials and provider policy server-side through an OpenAI-compatible gateway. Open Design already supports direct BYOK, but deployments with shared credential custody need one daemon-owned credential source that behaves consistently across model discovery, connection tests, proxy chat, finalize, headless inspection, and first-run setup.
Support first-run setup when an administrator has preconfigured a deployment provider. The browser can read only redacted provider availability metadata; credentials remain daemon-side.
This keeps AMR, local CLI, and direct BYOK unchanged. When no deployment provider environment is configured, existing users keep the current behavior.
What users will see
Existing deployment-backed API, CLI, and web provider flows work without client-side credentials or repeated endpoint setup. Self-hosted operators can route OpenAI-compatible credentials and optional run-session metadata through the daemon.
On first run, if an administrator has configured a deployment provider, the onboarding Connect step can offer that provider as a ready-to-use option. The user sees the configured label/host and chooses a model, but never sees or enters the deployment credential.
What changed
credentialSourcecontract field. The default isuser, preserving the existing direct BYOK request shape.credentialSource: "deployment"for daemon-resolved OpenAI-compatible provider credentials./api/provider-orchestrator/configstatus endpoint andod provider configCLI mirror. They report availability, label, display host, default model, and failure kind, but never return the credential.apiKeyandbaseUrlwhen deployment credentials are selected; the daemon resolves and validates the configured endpoint instead.Surface area
apps/weborapps/desktop(including Electron menu bar)odsubcommand or flag, newtools-dev/tools-pack/tools-prflag, or newOD_*env var/api/*endpoint, new SSE event, or changed shape inpackages/contractsskills/,design-systems,design-templates, orcraft/, or change to the skills protocolTRANSLATIONS.mdfor the locale workflow)package.json(dependenciesordevDependencies)Screenshots
Deployment-provider UI screenshots were added in the PR thread:
Non-goals
Validation
Ran with Node 24.12.0:
pnpm exec vitest run -c vitest.config.ts tests/finalize-design.test.ts -t "ignores caller baseUrl validation"fromapps/daemonpnpm exec vitest run -c vitest.config.ts tests/api-token-guard.test.ts -t "opens only redacted deployment provider discovery"fromapps/daemonpnpm exec vitest run -c vitest.config.ts tests/components/EntryShell.onboarding.test.tsx -t "continues with an available deployment provider"fromapps/webpnpm --filter @open-design/web typecheckpnpm --filter @open-design/daemon typecheckpnpm guardEarlier validation on this PR also covered:
pnpm --filter @open-design/web exec vitest run tests/providers/api-proxy.test.ts tests/lib/resolve-finalize-request.test.ts tests/components/InlineModelSwitcher.test.tsxpnpm --filter @open-design/daemon exec vitest run tests/cli-startup.test.ts tests/connection-test.test.ts tests/proxy-routes.test.ts tests/finalize-design.test.tspnpm --filter @open-design/contracts typecheckpnpm typecheck