feat: modifications for provider configuration - #209
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
skill-check — worker0 verified, 13 skipped (no docs/).
Note 17 stale rendered artifact(s) detected on main, unrelated to this PR. This PR is fine; the drift was already there. A maintainer should open a chore PR to re-render these.
|
|
Warning Review limit reached
More reviews will be available in 3 minutes and 45 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughConsolidates provider credentials/settings into a harness-owned ChangesHarness provider registry and console integration
Sequence Diagram(s)sequenceDiagram
autonumber
participant Console as Console UI
participant Harness as harness::provider::list/resolve
participant Providers as provider::<id>::refresh_models
participant Config as configuration::get/set/register
Console->>Harness: list providers
Console->>Providers: refresh_models (parallel)
Providers-->>Harness: models::register (via discovery)
Console->>Harness: resolve(provider) for selection
Harness->>Config: get('harness') (composed schema/value)
Config-->>Harness: providers + permissions
Harness-->>Console: {credential, api_url, max_tokens}
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
harness/src/provider-lmstudio/auth.ts (1)
120-141:⚠️ Potential issue | 🟠 Major | ⚡ Quick winEnsure the
lm-studiofallback token cannot be sent asAuthorizationto non-loopback hosts.
Inharness/src/provider-lmstudio/auth.ts(lines 120-141), whenselectAuthKey(...)returnsnull,effectiveis still set to{ type: 'api_key', key: FALLBACK_API_KEY }, soconfigFromCredential(...)will carry"lm-studio"intoChatCompletionsConfig.api_key. Butharness/docs/workers/provider-lmstudio.mdstates that when auth is missing, the worker falls back to the literallm-studiotoken so theAuthorizationheader is always present—contradicting the inline claim that the null-key case omitsAuthorization. Update/align the implementation so the fallback is only used for loopback (or otherwise guaranteeAuthorizationis omitted for non-loopback when no credential is available).🤖 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 `@harness/src/provider-lmstudio/auth.ts` around lines 120 - 141, The code currently injects FALLBACK_API_KEY into effective when selectAuthKey(...) returns null, which causes configFromCredential(...) to carry an api_key and may send Authorization to remote hosts; change the logic so the FALLBACK_API_KEY is only applied for loopback/local URLs: add or use a small helper (e.g., isLoopbackApiUrl or isLoopbackHost) to detect loopback origins from apiUrl (normalizeChatCompletionsUrl output), and only set effective = { type: 'api_key', key: FALLBACK_API_KEY } when key === null AND isLoopbackApiUrl(apiUrl) is true; otherwise set effective to a sentinel that causes no Authorization (e.g., null/undefined or a { type: 'none' } value that configFromCredential/buildAuthHeaders treat as no auth), keeping references to selectAuthKey, normalizeChatCompletionsUrl, configFromCredential, and FALLBACK_API_KEY to locate the change.
🧹 Nitpick comments (7)
.github/workflows/_publish-registry.yml (1)
375-380: 💤 Low valueStale
iii-databasereferences left behind.With the database pre-start removed (per the new comment at Lines 105-108), no step writes
iii-database.pidoriii-database.loganymore, so the "Stop dependency workers" step here and theiii-database.logdump at Lines 362-366 are now dead no-ops. They're harmless (both file-existence guarded) but contradict the new comment; consider removing them for clarity.🤖 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 @.github/workflows/_publish-registry.yml around lines 375 - 380, Remove the now-dead "Stop dependency workers" step and the separate `iii-database.log` dump step from the GitHub Actions workflow, and delete any remaining checks or commands that reference `iii-database.pid` or `iii-database.log` (these include the kill guarded by `if [[ -f iii-database.pid ]]; then ...` and the cat/dump of `iii-database.log`), so the workflow no longer contains stale `iii-database` references that are never created.console/web/src/pages/Configuration/tabs/ConsoleSettingsTab.tsx (1)
22-24: ⚡ Quick winDuplicate
HARNESS_CONFIG_HASHconstant.This constant is also defined in
ModelPicker.tsx(line 15). Consider extracting it to a shared location (e.g., a routes/constants module) to avoid drift if the hash format changes.♻️ Example extraction
Create a shared constant:
// e.g., in `@/lib/routes.ts` or similar export const HARNESS_CONFIG_HASH = '`#/configuration/workers/harness`'Then import in both files.
🤖 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 `@console/web/src/pages/Configuration/tabs/ConsoleSettingsTab.tsx` around lines 22 - 24, The HARNESS_CONFIG_HASH constant is duplicated (defined in ConsoleSettingsTab and ModelPicker); extract it into a shared module (e.g., a routes/constants file) and replace the local definitions with an import of that single exported constant (update ConsoleSettingsTab and ModelPicker to import HARNESS_CONFIG_HASH). Ensure the new module exports the exact string and update both files to remove their local HARNESS_CONFIG_HASH definitions and reference the shared symbol instead.console/web/src/lib/conversations-context.tsx (1)
64-81: 💤 Low valueStale
presentProvidersreference in dependency array could cause unnecessary re-renders.The
refreshModelscallback capturespresentProvidersin its closure but also lists it as a dependency. This meansrefreshModelsgets a new identity every time the provider list changes (e.g., after every successful fetch), which may cause downstreamuseEffector memoization dependencies to re-trigger unnecessarily.If this is intentional (to always use the latest providers), consider using a ref to hold
presentProvidersto avoid re-creating the callback. However, if the current behavior is acceptable for your use case, this is a minor consideration.🤖 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 `@console/web/src/lib/conversations-context.tsx` around lines 64 - 81, The callback refreshModels closes over presentProviders and lists it in the dependency array, causing refreshModels to be re-created whenever provider list updates; change it to read presentProviders from a stable ref instead: create a useRef (e.g., presentProvidersRef), update presentProvidersRef.current whenever presentProviders changes, then inside refreshModels read ids from presentProvidersRef.current and remove presentProviders from the dependency array; ensure other real dependencies like refresh, refreshProviderModels and backend.id remain in the array so refreshModels identity is stable while still using the latest provider list.console/web/src/lib/models-catalog.ts (1)
55-64: 💤 Low valueRedundant
.catch()when usingPromise.allSettled.
Promise.allSettledalready handles rejections without throwing, so the inner.catch(() => undefined)is redundant. Either approach works, but using both adds unnecessary complexity.♻️ Suggested simplification
export async function refreshProviderModels( providers: readonly string[], ): Promise<void> { const client = await getIiiClient() await Promise.allSettled( - providers.map((p) => - client.call(`provider::${p}::refresh_models`, {}).catch(() => undefined), - ), + providers.map((p) => client.call(`provider::${p}::refresh_models`, {})), ) }🤖 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 `@console/web/src/lib/models-catalog.ts` around lines 55 - 64, The inner .catch(() => undefined) within refreshProviderModels is redundant because Promise.allSettled already captures rejections; remove the per-call catch and let Promise.allSettled handle outcomes. Update refreshProviderModels to await Promise.allSettled over providers.map(p => client.call(`provider::${p}::refresh_models`, {})) and keep getIiiClient and client.call usage unchanged so failures are represented in the settled results instead of being swallowed by the inner catch.harness/src/runtime/models-discovery.ts (1)
32-32: ⚡ Quick winUse a template literal here.
The lint check flagged this string concatenation; align it to avoid a CI lint failure.
♻️ Proposed fix
- return apiUrl.replace(/\/+$/, '') + '/models'; + return `${apiUrl.replace(/\/+$/, '')}/models`;🤖 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 `@harness/src/runtime/models-discovery.ts` at line 32, Replace the string concatenation that builds the models endpoint by using a template literal: instead of returning apiUrl.replace(/\/+$/, '') + '/models', return a template string that interpolates the trimmed apiUrl and appends /models (e.g., `${apiUrl.replace(/\/+$/, '')}/models`) so the lint rule is satisfied; update the return in the function that uses the apiUrl variable accordingly.harness/src/provider-lmstudio/auth.ts (2)
26-76: 💤 Low value
EMPTY_RESOLVE+resolveTolerantare duplicated verbatim across local-first providers.This block is identical to the one in
harness/src/provider-llamacpp/auth.ts(only the log message/code string differ). Consider hoisting a shared tolerant resolver helper (parameterized by provider id and log code) intoruntime/provider-resolve.jsto avoid drift.🤖 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 `@harness/src/provider-lmstudio/auth.ts` around lines 26 - 76, The EMPTY_RESOLVE constant and resolveTolerant function (used in harness/src/provider-lmstudio/auth.ts) are duplicated across local-first providers; extract a shared helper into runtime/provider-resolve.js that exports a factory or function like createTolerantResolver(providerId: string, warnCode: string) or resolveProviderTolerant(iii, providerId, warnCode) and replace the local EMPTY_RESOLVE and resolveTolerant usages (which reference PROVIDER_ID and call logger.warn) with calls to that shared helper so each provider only passes its PROVIDER_ID and log code string to avoid drift.
120-128: ⚖️ Poor tradeoffRedundant provider resolution on the request path.
buildConfigcallsresolveTolerantonce, but the actual headers are produced bybuildAuthHeaders→fetchCredential→resolveTolerantagain, so each request triggersharness::provider::resolve(a cross-worker bus call) at least twice. Consider resolving once and threading the result (or the selected key) through to header construction.🤖 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 `@harness/src/provider-lmstudio/auth.ts` around lines 120 - 128, The code calls resolveTolerant twice per request (once in buildConfig and again via buildAuthHeaders → fetchCredential → resolveTolerant), causing redundant cross-worker resolution; fix this by resolving once in buildConfig and threading the resolved result (or the selected key) into header construction instead of calling resolveTolerant again — update buildConfig to pass the resolved object or selected auth key (from selectAuthKey) down into buildAuthHeaders/fetchCredential (or change fetchCredential signature to accept the resolved credential), remove the second resolveTolerant call, and ensure overrideUrl/apiUrl and maxTokens are derived from the already-resolved object (resolved) so fetchCredential/buildAuthHeaders use the passed-in values.
🤖 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 `@harness/docs/architecture.md`:
- Line 27: The architecture doc line for models-catalog is stale: it incorrectly
mentions an “embedded fallback.” Update harness/docs/architecture.md to remove
the “embedded fallback” wording and describe models-catalog as state-first with
no embedded seed/fallback (consistent with harness/src/models-catalog/state.ts
and the provider::<name>::refresh_models behavior); also ensure any linked
worker doc reference (workers/models-catalog.md) matches the corrected phrasing
so docs and code (state.ts handlers) are consistent.
In `@harness/src/harness/providers/register.ts`:
- Around line 1-71: This file's formatting doesn't match the repo formatter; run
the project formatter (e.g. pnpm format or prettier --write) and commit the
resulting changes so CI passes. Locate the module that defines
DeclarationSchema, ResolveSchema and the export async function
registerProviderRegistry (which constructs ProviderRegistry and calls
iii.registerFunction for 'harness::provider::register',
'harness::provider::resolve', and 'harness::provider::list') and let the
formatter reflow spacing/line breaks in this file (and the companion registry
implementation) before committing the formatted files.
In `@harness/src/provider-anthropic/discover.ts`:
- Around line 50-55: The discovery code is sending OAuth credentials as
x-api-key instead of the proper Authorization header; update the header
construction in the block that calls deriveModelsUrl and fetchModelsJson to use
the existing authHeaderFor helper (from types.ts) for the credential (cred) and
merge it with the 'anthropic-version' header (and any other required headers)
before passing to fetchModelsJson; keep using deriveModelsUrl(resolved?.api_url
?? worker.default_api_url) and ANTHROPIC_VERSION but replace the hardcoded
{'x-api-key': key} with the result of authHeaderFor(cred) so OAuth tokens are
sent as Authorization: Bearer … and API keys still work.
In `@harness/src/provider-kimi/register.ts`:
- Around line 19-28: The fire-and-forget call to declareProvider(...) (the call
that uses PROVIDER_ID and worker defaults) returns a promise that's currently
discarded with void and has no rejection handler; attach a .catch to that
promise (similar to how discoverAndRegister is guarded) to handle transient
startup failures — e.g., call declareProvider(...).catch(err =>
processLogger.error(`Failed to declare provider ${PROVIDER_ID}`, err)) so the
error is logged (include the error details and PROVIDER_ID) instead of causing
an unhandled rejection.
In `@harness/src/provider-lmstudio/register.ts`:
- Around line 26-32: The call to declareProvider(iii, { id: PROVIDER_ID, ... })
is fire-and-forget and can cause unhandled promise rejections; change it to
handle promise failures by either awaiting the call or appending a .catch
handler that logs the error (consistent with the setImmediate discovery call
below), e.g. await declareProvider(...) inside an async initializer or
declareProvider(...).catch(err => /* log or handle error */), referencing the
declareProvider invocation and PROVIDER_ID so the rejection is handled.
In `@harness/tests/harness/providers/registry.test.ts`:
- Around line 106-108: The finally block currently sets
process.env.III_TEST_PROVIDER_KEY = undefined which leaves a string "undefined"
in the env; replace that assignment with using the delete operator to remove
process.env.III_TEST_PROVIDER_KEY so the environment variable is truly unset and
cannot leak into other tests (locate the finally block in the test teardown
where process.env.III_TEST_PROVIDER_KEY is manipulated).
---
Outside diff comments:
In `@harness/src/provider-lmstudio/auth.ts`:
- Around line 120-141: The code currently injects FALLBACK_API_KEY into
effective when selectAuthKey(...) returns null, which causes
configFromCredential(...) to carry an api_key and may send Authorization to
remote hosts; change the logic so the FALLBACK_API_KEY is only applied for
loopback/local URLs: add or use a small helper (e.g., isLoopbackApiUrl or
isLoopbackHost) to detect loopback origins from apiUrl
(normalizeChatCompletionsUrl output), and only set effective = { type:
'api_key', key: FALLBACK_API_KEY } when key === null AND
isLoopbackApiUrl(apiUrl) is true; otherwise set effective to a sentinel that
causes no Authorization (e.g., null/undefined or a { type: 'none' } value that
configFromCredential/buildAuthHeaders treat as no auth), keeping references to
selectAuthKey, normalizeChatCompletionsUrl, configFromCredential, and
FALLBACK_API_KEY to locate the change.
---
Nitpick comments:
In @.github/workflows/_publish-registry.yml:
- Around line 375-380: Remove the now-dead "Stop dependency workers" step and
the separate `iii-database.log` dump step from the GitHub Actions workflow, and
delete any remaining checks or commands that reference `iii-database.pid` or
`iii-database.log` (these include the kill guarded by `if [[ -f iii-database.pid
]]; then ...` and the cat/dump of `iii-database.log`), so the workflow no longer
contains stale `iii-database` references that are never created.
In `@console/web/src/lib/conversations-context.tsx`:
- Around line 64-81: The callback refreshModels closes over presentProviders and
lists it in the dependency array, causing refreshModels to be re-created
whenever provider list updates; change it to read presentProviders from a stable
ref instead: create a useRef (e.g., presentProvidersRef), update
presentProvidersRef.current whenever presentProviders changes, then inside
refreshModels read ids from presentProvidersRef.current and remove
presentProviders from the dependency array; ensure other real dependencies like
refresh, refreshProviderModels and backend.id remain in the array so
refreshModels identity is stable while still using the latest provider list.
In `@console/web/src/lib/models-catalog.ts`:
- Around line 55-64: The inner .catch(() => undefined) within
refreshProviderModels is redundant because Promise.allSettled already captures
rejections; remove the per-call catch and let Promise.allSettled handle
outcomes. Update refreshProviderModels to await Promise.allSettled over
providers.map(p => client.call(`provider::${p}::refresh_models`, {})) and keep
getIiiClient and client.call usage unchanged so failures are represented in the
settled results instead of being swallowed by the inner catch.
In `@console/web/src/pages/Configuration/tabs/ConsoleSettingsTab.tsx`:
- Around line 22-24: The HARNESS_CONFIG_HASH constant is duplicated (defined in
ConsoleSettingsTab and ModelPicker); extract it into a shared module (e.g., a
routes/constants file) and replace the local definitions with an import of that
single exported constant (update ConsoleSettingsTab and ModelPicker to import
HARNESS_CONFIG_HASH). Ensure the new module exports the exact string and update
both files to remove their local HARNESS_CONFIG_HASH definitions and reference
the shared symbol instead.
In `@harness/src/provider-lmstudio/auth.ts`:
- Around line 26-76: The EMPTY_RESOLVE constant and resolveTolerant function
(used in harness/src/provider-lmstudio/auth.ts) are duplicated across
local-first providers; extract a shared helper into runtime/provider-resolve.js
that exports a factory or function like createTolerantResolver(providerId:
string, warnCode: string) or resolveProviderTolerant(iii, providerId, warnCode)
and replace the local EMPTY_RESOLVE and resolveTolerant usages (which reference
PROVIDER_ID and call logger.warn) with calls to that shared helper so each
provider only passes its PROVIDER_ID and log code string to avoid drift.
- Around line 120-128: The code calls resolveTolerant twice per request (once in
buildConfig and again via buildAuthHeaders → fetchCredential → resolveTolerant),
causing redundant cross-worker resolution; fix this by resolving once in
buildConfig and threading the resolved result (or the selected key) into header
construction instead of calling resolveTolerant again — update buildConfig to
pass the resolved object or selected auth key (from selectAuthKey) down into
buildAuthHeaders/fetchCredential (or change fetchCredential signature to accept
the resolved credential), remove the second resolveTolerant call, and ensure
overrideUrl/apiUrl and maxTokens are derived from the already-resolved object
(resolved) so fetchCredential/buildAuthHeaders use the passed-in values.
In `@harness/src/runtime/models-discovery.ts`:
- Line 32: Replace the string concatenation that builds the models endpoint by
using a template literal: instead of returning apiUrl.replace(/\/+$/, '') +
'/models', return a template string that interpolates the trimmed apiUrl and
appends /models (e.g., `${apiUrl.replace(/\/+$/, '')}/models`) so the lint rule
is satisfied; update the return in the function that uses the apiUrl variable
accordingly.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0ba00f62-b981-445a-bdd2-747e99d81731
📒 Files selected for processing (120)
.github/workflows/_publish-registry.ymlREADME.mdconsole/web/src/components/chat/ModelPicker.tsxconsole/web/src/components/providers/ProviderRow.tsxconsole/web/src/components/providers/ProviderSettingsDialog.tsxconsole/web/src/components/providers/StatusBadge.tsxconsole/web/src/components/providers/provider-registry.tsconsole/web/src/hooks/use-model-picker-source.tsconsole/web/src/hooks/use-providers.tsconsole/web/src/lib/conversations-context.tsxconsole/web/src/lib/models-catalog.tsconsole/web/src/lib/providers.test.tsconsole/web/src/lib/providers.tsconsole/web/src/pages/Configuration/tabs/ConsoleSettingsTab.tsxconsole/web/src/types/chat.tsharness/README.mdharness/docs/architecture.mdharness/docs/storage.mdharness/docs/workers/auth-credentials.mdharness/docs/workers/harness.mdharness/docs/workers/models-catalog.mdharness/docs/workers/provider-anthropic.mdharness/docs/workers/provider-config.mdharness/docs/workers/provider-kimi.mdharness/docs/workers/provider-lmstudio.mdharness/docs/workers/provider-openai.mdharness/engine.config.yamlharness/iii.worker.yamlharness/package.jsonharness/src/approval-gate/main.tsharness/src/approval-gate/settings/default-mode.tsharness/src/approval-gate/settings/store.tsharness/src/auth-credentials/config.tsharness/src/auth-credentials/handlers/delete-token.tsharness/src/auth-credentials/handlers/get-token.tsharness/src/auth-credentials/handlers/list-providers.tsharness/src/auth-credentials/handlers/set-token.tsharness/src/auth-credentials/handlers/status.tsharness/src/auth-credentials/iii.worker.yamlharness/src/auth-credentials/main.tsharness/src/auth-credentials/register.tsharness/src/auth-credentials/resolve.tsharness/src/auth-credentials/store.tsharness/src/auth-credentials/types.tsharness/src/harness/iii.worker.yamlharness/src/harness/providers/register.tsharness/src/harness/providers/registry.tsharness/src/harness/register.tsharness/src/index.tsharness/src/models-catalog/catalog.tsharness/src/models-catalog/handlers/get.tsharness/src/models-catalog/handlers/list.tsharness/src/models-catalog/handlers/supports.tsharness/src/models-catalog/models.jsonharness/src/models-catalog/register.tsharness/src/models-catalog/state.tsharness/src/models-catalog/types.tsharness/src/provider-anthropic/auth.tsharness/src/provider-anthropic/discover.tsharness/src/provider-anthropic/iii.worker.yamlharness/src/provider-anthropic/refresh-fn.tsharness/src/provider-anthropic/register.tsharness/src/provider-anthropic/types.tsharness/src/provider-config/config.tsharness/src/provider-config/handlers/clear.tsharness/src/provider-config/handlers/get.tsharness/src/provider-config/handlers/list.tsharness/src/provider-config/handlers/set.tsharness/src/provider-config/iii.worker.yamlharness/src/provider-config/main.tsharness/src/provider-config/register.tsharness/src/provider-config/store.tsharness/src/provider-config/types.tsharness/src/provider-kimi/auth.tsharness/src/provider-kimi/discover.tsharness/src/provider-kimi/iii.worker.yamlharness/src/provider-kimi/refresh-fn.tsharness/src/provider-kimi/register.tsharness/src/provider-kimi/types.tsharness/src/provider-llamacpp/auth.tsharness/src/provider-llamacpp/iii.worker.yamlharness/src/provider-llamacpp/register.tsharness/src/provider-llamacpp/types.tsharness/src/provider-lmstudio/auth.tsharness/src/provider-lmstudio/iii.worker.yamlharness/src/provider-lmstudio/register.tsharness/src/provider-lmstudio/types.tsharness/src/provider-openai/auth.tsharness/src/provider-openai/discover.tsharness/src/provider-openai/iii.worker.yamlharness/src/provider-openai/refresh-fn.tsharness/src/provider-openai/register.tsharness/src/provider-openai/types.tsharness/src/runtime/configuration.tsharness/src/runtime/database-store.tsharness/src/runtime/fetch-overrides.tsharness/src/runtime/harness-config.tsharness/src/runtime/models-discovery.tsharness/src/runtime/provider-resolve.tsharness/src/runtime/storage-config.tsharness/src/web/register.tsharness/tests/auth-credentials/env-map-kimi.test.tsharness/tests/auth-credentials/env-map-llamacpp.test.tsharness/tests/auth-credentials/env-map-lmstudio.test.tsharness/tests/auth-credentials/resolve.test.tsharness/tests/auth-credentials/store.test.tsharness/tests/harness/policy.test.tsharness/tests/harness/providers/registry.test.tsharness/tests/models-catalog/catalog.test.tsharness/tests/models-catalog/seed-kimi.test.tsharness/tests/models-catalog/seed-llamacpp.test.tsharness/tests/models-catalog/seed-lmstudio.test.tsharness/tests/provider-config/handlers.test.tsharness/tests/provider-config/store.test.tsharness/tests/provider-llamacpp/auth.test.tsharness/tests/provider-lmstudio/auth.test.tsharness/tests/runtime/database-store.test.tsharness/tests/runtime/fake-database-sdk.tsharness/tests/runtime/storage-config.test.tsiii-permissions.yaml
💤 Files with no reviewable changes (50)
- harness/src/auth-credentials/handlers/get-token.ts
- harness/docs/workers/auth-credentials.md
- harness/src/auth-credentials/iii.worker.yaml
- harness/src/provider-config/types.ts
- harness/tests/models-catalog/seed-kimi.test.ts
- harness/src/provider-config/iii.worker.yaml
- harness/src/auth-credentials/handlers/set-token.ts
- harness/tests/provider-config/store.test.ts
- harness/src/auth-credentials/handlers/list-providers.ts
- harness/src/auth-credentials/register.ts
- harness/src/models-catalog/models.json
- harness/tests/models-catalog/seed-llamacpp.test.ts
- harness/src/auth-credentials/types.ts
- harness/src/auth-credentials/main.ts
- harness/src/auth-credentials/handlers/status.ts
- harness/src/provider-config/handlers/list.ts
- harness/src/auth-credentials/resolve.ts
- harness/tests/auth-credentials/env-map-kimi.test.ts
- console/web/src/components/providers/ProviderSettingsDialog.tsx
- harness/src/runtime/storage-config.ts
- harness/src/runtime/fetch-overrides.ts
- harness/tests/runtime/storage-config.test.ts
- harness/tests/runtime/fake-database-sdk.ts
- harness/tests/models-catalog/seed-lmstudio.test.ts
- harness/tests/auth-credentials/env-map-llamacpp.test.ts
- harness/src/runtime/database-store.ts
- harness/src/provider-config/handlers/clear.ts
- harness/tests/provider-config/handlers.test.ts
- harness/tests/auth-credentials/resolve.test.ts
- harness/src/provider-config/config.ts
- harness/src/models-catalog/catalog.ts
- console/web/src/hooks/use-providers.ts
- console/web/src/components/providers/StatusBadge.tsx
- harness/src/provider-config/register.ts
- harness/src/provider-config/handlers/set.ts
- harness/docs/workers/provider-config.md
- harness/src/auth-credentials/config.ts
- harness/tests/runtime/database-store.test.ts
- harness/tests/models-catalog/catalog.test.ts
- harness/src/provider-config/store.ts
- harness/src/provider-config/handlers/get.ts
- harness/tests/auth-credentials/store.test.ts
- harness/package.json
- harness/src/auth-credentials/store.ts
- harness/tests/auth-credentials/env-map-lmstudio.test.ts
- harness/src/auth-credentials/handlers/delete-token.ts
- console/web/src/components/providers/ProviderRow.tsx
- console/web/src/lib/providers.test.ts
- harness/src/provider-config/main.ts
- console/web/src/components/providers/provider-registry.ts
Comment on lines
+50
to
+55
| const key = cred.type === 'api_key' ? cred.key : cred.access_token; | ||
| const url = deriveModelsUrl(resolved?.api_url ?? worker.default_api_url); | ||
| const json = await fetchModelsJson(url, { | ||
| 'x-api-key': key, | ||
| 'anthropic-version': ANTHROPIC_VERSION, | ||
| }); |
There was a problem hiding this comment.
Discovery sends the wrong auth header for OAuth credentials.
For an oauth_bearer credential the access token is sent as x-api-key, but Anthropic expects Authorization: Bearer … for OAuth (see authHeaderFor in types.ts). The /v1/models request will then fail auth and discovery silently returns [], so OAuth-authenticated users never get a live model list.
🐛 Proposed fix
- const key = cred.type === 'api_key' ? cred.key : cred.access_token;
const url = deriveModelsUrl(resolved?.api_url ?? worker.default_api_url);
- const json = await fetchModelsJson(url, {
- 'x-api-key': key,
- 'anthropic-version': ANTHROPIC_VERSION,
- });
+ const authHeader: Record<string, string> =
+ cred.type === 'api_key'
+ ? { 'x-api-key': cred.key }
+ : { authorization: `Bearer ${cred.access_token}` };
+ const json = await fetchModelsJson(url, {
+ ...authHeader,
+ 'anthropic-version': ANTHROPIC_VERSION,
+ });🤖 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 `@harness/src/provider-anthropic/discover.ts` around lines 50 - 55, The
discovery code is sending OAuth credentials as x-api-key instead of the proper
Authorization header; update the header construction in the block that calls
deriveModelsUrl and fetchModelsJson to use the existing authHeaderFor helper
(from types.ts) for the credential (cred) and merge it with the
'anthropic-version' header (and any other required headers) before passing to
fetchModelsJson; keep using deriveModelsUrl(resolved?.api_url ??
worker.default_api_url) and ANTHROPIC_VERSION but replace the hardcoded
{'x-api-key': key} with the result of authHeaderFor(cred) so OAuth tokens are
sent as Authorization: Bearer … and API keys still work.
Comment on lines
+19
to
+28
| void declareProvider(iii, { | ||
| id: PROVIDER_ID, | ||
| display_name: 'kimi (moonshot)', | ||
| credential_env_var: 'MOONSHOT_API_KEY', | ||
| defaults: { | ||
| api_url: worker.default_api_url, | ||
| max_tokens: worker.default_max_tokens, | ||
| }, | ||
| supports_model_listing: true, | ||
| }); |
There was a problem hiding this comment.
Attach a rejection handler to the fire-and-forget declareProvider call.
void declareProvider(...) discards the returned promise without a .catch. A transient failure during startup self-registration would surface as an unhandled rejection (process termination on current Node defaults), crashing the worker at boot. The neighboring discoverAndRegister call already guards with .catch; mirror that here.
🛡️ Proposed fix
- void declareProvider(iii, {
- id: PROVIDER_ID,
- display_name: 'kimi (moonshot)',
- credential_env_var: 'MOONSHOT_API_KEY',
- defaults: {
- api_url: worker.default_api_url,
- max_tokens: worker.default_max_tokens,
- },
- supports_model_listing: true,
- });
+ void declareProvider(iii, {
+ id: PROVIDER_ID,
+ display_name: 'kimi (moonshot)',
+ credential_env_var: 'MOONSHOT_API_KEY',
+ defaults: {
+ api_url: worker.default_api_url,
+ max_tokens: worker.default_max_tokens,
+ },
+ supports_model_listing: true,
+ }).catch((err) => {
+ logger.warn('kimi declareProvider failed', { err: String(err) });
+ });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| void declareProvider(iii, { | |
| id: PROVIDER_ID, | |
| display_name: 'kimi (moonshot)', | |
| credential_env_var: 'MOONSHOT_API_KEY', | |
| defaults: { | |
| api_url: worker.default_api_url, | |
| max_tokens: worker.default_max_tokens, | |
| }, | |
| supports_model_listing: true, | |
| }); | |
| void declareProvider(iii, { | |
| id: PROVIDER_ID, | |
| display_name: 'kimi (moonshot)', | |
| credential_env_var: 'MOONSHOT_API_KEY', | |
| defaults: { | |
| api_url: worker.default_api_url, | |
| max_tokens: worker.default_max_tokens, | |
| }, | |
| supports_model_listing: true, | |
| }).catch((err) => { | |
| logger.warn('kimi declareProvider failed', { err: String(err) }); | |
| }); |
🤖 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 `@harness/src/provider-kimi/register.ts` around lines 19 - 28, The
fire-and-forget call to declareProvider(...) (the call that uses PROVIDER_ID and
worker defaults) returns a promise that's currently discarded with void and has
no rejection handler; attach a .catch to that promise (similar to how
discoverAndRegister is guarded) to handle transient startup failures — e.g.,
call declareProvider(...).catch(err => processLogger.error(`Failed to declare
provider ${PROVIDER_ID}`, err)) so the error is logged (include the error
details and PROVIDER_ID) instead of causing an unhandled rejection.
Comment on lines
+26
to
+32
| void declareProvider(iii, { | ||
| id: PROVIDER_ID, | ||
| display_name: 'lm studio', | ||
| credential_env_var: 'LMSTUDIO_API_KEY', | ||
| defaults: { max_tokens: worker.default_max_tokens }, | ||
| supports_model_listing: true, | ||
| }); |
There was a problem hiding this comment.
void declareProvider(...) can produce an unhandled promise rejection.
Unlike the setImmediate discovery below (which has a .catch), this fire-and-forget call discards the returned promise with void and has no rejection handler. If declareProvider rejects (e.g. the configuration worker isn't ready yet — the same race the discovery comment warns about), it surfaces as an unhandled rejection. Attach a .catch (or await it).
🛡️ Proposed fix
- void declareProvider(iii, {
- id: PROVIDER_ID,
- display_name: 'lm studio',
- credential_env_var: 'LMSTUDIO_API_KEY',
- defaults: { max_tokens: worker.default_max_tokens },
- supports_model_listing: true,
- });
+ declareProvider(iii, {
+ id: PROVIDER_ID,
+ display_name: 'lm studio',
+ credential_env_var: 'LMSTUDIO_API_KEY',
+ defaults: { max_tokens: worker.default_max_tokens },
+ supports_model_listing: true,
+ }).catch((err) => {
+ logger.warn('lmstudio declareProvider failed', { err: String(err) });
+ });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| void declareProvider(iii, { | |
| id: PROVIDER_ID, | |
| display_name: 'lm studio', | |
| credential_env_var: 'LMSTUDIO_API_KEY', | |
| defaults: { max_tokens: worker.default_max_tokens }, | |
| supports_model_listing: true, | |
| }); | |
| declareProvider(iii, { | |
| id: PROVIDER_ID, | |
| display_name: 'lm studio', | |
| credential_env_var: 'LMSTUDIO_API_KEY', | |
| defaults: { max_tokens: worker.default_max_tokens }, | |
| supports_model_listing: true, | |
| }).catch((err) => { | |
| logger.warn('lmstudio declareProvider failed', { err: String(err) }); | |
| }); |
🤖 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 `@harness/src/provider-lmstudio/register.ts` around lines 26 - 32, The call to
declareProvider(iii, { id: PROVIDER_ID, ... }) is fire-and-forget and can cause
unhandled promise rejections; change it to handle promise failures by either
awaiting the call or appending a .catch handler that logs the error (consistent
with the setImmediate discovery call below), e.g. await declareProvider(...)
inside an async initializer or declareProvider(...).catch(err => /* log or
handle error */), referencing the declareProvider invocation and PROVIDER_ID so
the rejection is handled.
Comment on lines
+106
to
+108
| } finally { | ||
| process.env.III_TEST_PROVIDER_KEY = undefined; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Node.js, does assigning undefined to a process.env property delete it or coerce it to the string "undefined"?
💡 Result:
Assigning undefined to a process.env property does not delete it; it gets coerced to the string value "undefined" (i.e., the property remains present with the literal string value). [1] To actually remove/unset an environment variable from process.env, use delete, e.g. delete process.env.MY_VAR. [1]
Citations:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '90,125p' harness/tests/harness/providers/registry.test.tsRepository: iii-hq/workers
Length of output: 1335
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '90,125p' harness/tests/harness/providers/registry.test.tsRepository: iii-hq/workers
Length of output: 1335
Use delete to unset the env var in finally.
process.env.III_TEST_PROVIDER_KEY = undefined leaves the variable set to the string "undefined" (truthy), so it can leak into subsequent tests. Replace it with delete.
💚 Proposed fix
- } finally {
- process.env.III_TEST_PROVIDER_KEY = undefined;
- }
+ } finally {
+ delete process.env.III_TEST_PROVIDER_KEY;
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| } finally { | |
| process.env.III_TEST_PROVIDER_KEY = undefined; | |
| } | |
| } finally { | |
| delete process.env.III_TEST_PROVIDER_KEY; | |
| } |
🤖 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 `@harness/tests/harness/providers/registry.test.ts` around lines 106 - 108, The
finally block currently sets process.env.III_TEST_PROVIDER_KEY = undefined which
leaves a string "undefined" in the env; replace that assignment with using the
delete operator to remove process.env.III_TEST_PROVIDER_KEY so the environment
variable is truly unset and cannot leak into other tests (locate the finally
block in the test teardown where process.env.III_TEST_PROVIDER_KEY is
manipulated).
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@harness/docs/workers/authoring-a-provider.md`:
- Around line 40-41: The markdown in authoring-a-provider.md uses incorrect
relative links like harness/docs/storage.md and
harness/docs/workers/models-catalog.md that will 404 from this file’s directory;
update those links to correct relative paths (e.g., change
harness/docs/storage.md → ../storage.md, harness/docs/workers/models-catalog.md
→ ./models-catalog.md or ../models-catalog.md as appropriate) and similarly fix
other targets referenced (e.g., provider-openai.md → ./provider-openai.md, any
src references → ../../src/...) so all links resolve from
authoring-a-provider.md; apply the same path corrections to the other reported
occurrences in this file.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 15e8dc69-4345-46e6-926b-34c5815de1d3
📒 Files selected for processing (5)
harness/docs/architecture.mdharness/docs/workers/authoring-a-provider.mdharness/src/harness/providers/register.tsharness/src/harness/providers/registry.tsharness/tests/harness/providers/registry.test.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- harness/src/harness/providers/register.ts
- harness/tests/harness/providers/registry.test.ts
- harness/docs/architecture.md
- harness/src/harness/providers/registry.ts
Comment on lines
+40
to
+41
| [storage.md](harness/docs/storage.md). Models come exclusively from provider | ||
| registration — see [models-catalog.md](harness/docs/workers/models-catalog.md). |
There was a problem hiding this comment.
Fix relative links; several targets are likely broken from this file’s location.
From harness/docs/workers/authoring-a-provider.md, links written as harness/... resolve relative to the current directory and likely 404. Please switch to correct relative paths (e.g., ../storage.md, ./provider-openai.md, ../../src/..., etc.) or repo-root absolute paths if your renderer supports them.
Suggested patch pattern
-[storage.md](harness/docs/storage.md)
+[storage.md](../storage.md)
-[provider-openai](harness/docs/workers/provider-openai.md)
+[provider-openai](./provider-openai.md)
-[runtime/provider-resolve.ts](harness/src/runtime/provider-resolve.ts)
+[runtime/provider-resolve.ts](../../src/runtime/provider-resolve.ts)Also applies to: 44-47, 97-97, 130-130, 135-135, 256-256, 262-262, 330-330, 341-341, 346-346, 353-353, 366-366, 369-369
🤖 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 `@harness/docs/workers/authoring-a-provider.md` around lines 40 - 41, The
markdown in authoring-a-provider.md uses incorrect relative links like
harness/docs/storage.md and harness/docs/workers/models-catalog.md that will 404
from this file’s directory; update those links to correct relative paths (e.g.,
change harness/docs/storage.md → ../storage.md,
harness/docs/workers/models-catalog.md → ./models-catalog.md or
../models-catalog.md as appropriate) and similarly fix other targets referenced
(e.g., provider-openai.md → ./provider-openai.md, any src references →
../../src/...) so all links resolve from authoring-a-provider.md; apply the same
path corrections to the other reported occurrences in this file.
andersonleal
approved these changes
Jun 1, 2026
4 tasks
This was referenced Jul 3, 2026
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
2 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 by CodeRabbit
Architecture & Configuration
Console Improvements
Admin & Configuration
Documentation