Repository navigation
feat(ai): add declarative external assistants with built-in providers - #1337
Conversation
NewtTheWolf
left a comment
NewtTheWolf
left a comment
There was a problem hiding this comment.
Thanks for moving provider-specific HTTP behavior into declarative packages while keeping credentials and desktop/web dispatch in the host. I reviewed f2b1215f5744f1735aa65cd8edc36015d0d12676, including installation/recovery, legacy migration, model discovery and the settings/Plugin Center changes.
Validation on that revision: 95 targeted frontend tests and 14 targeted Rust tests passed. Frontend build, typecheck, targeted ESLint and git diff --check passed. All nine package manifests validate against the bundled schema and all nine universal ZIPs build. The runtime tests exercise local HTTP fixtures, optional credentials, model filters, migration retry and interrupted updates; they are not live-provider or desktop E2E tests.
I have no confirmed code blocker from this pass, but this is not a merge approval. GitNexus classifies the PR's 235 changed symbols and 34 affected flows as critical, with shared credentials and startup migration among the affected paths. The PR is still a draft and GitHub currently reports merge conflicts. Tabularium #35 is also open, and the package publishing/runtime-version/desktop verification checklist remains outstanding.
I will run the real-app install/select/generate/update/remove and offline-upgrade checks once the PR is rebased and the registry rollout is ready, then revisit merge readiness. Holding this open in the meantime.
debba
changed the title
|
@NewtTheWolf, here are the remaining Tabularium rollout steps for this PR. This checklist supersedes the seven-replacement-package rollout described in Tabularium #35: the seven existing providers are built in and do not need external publication.
|
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| await refreshProviders(); | ||
| await updateSetting('aiModel', ''); | ||
| await updateSetting('aiProvider', id); | ||
| if (!settings.aiEnabled) await updateSetting('aiEnabled', true); |
There was a problem hiding this comment.
WARNING: Stale settings.aiEnabled and non-atomic selection after install
settings.aiEnabled is read from the render-time closure after two awaited updateSetting calls, so it can be stale. aiProvider is also persisted before aiEnabled; if the last write fails, a provider is selected while AI stays disabled. Enable AI unconditionally or use the updater form of updateSetting.
| {t( | ||
| p.id === 'chatgpt' | ||
| p.authentication === 'account' | ||
| ? 'settings.ai.planAccess' |
There was a problem hiding this comment.
WARNING: Missing i18n keys in 9 locales
settings.ai.planAccess, keyAccess and localAccess exist only in en.json (769-771) and it.json (768-770). They are absent from de, es, fr, ja, ko, pt-BR, ru, tl and zh, so every provider tile falls back to the raw key/English there. .rules/general.md requires translating strings into packages/web-ui/src/i18n/locales.
| "providerMissing": "{{provider}} is not installed. Install it from the Plugin Center to continue using your saved configuration.", | ||
| "availableProviders": "Available providers", | ||
| "installProviderHelp": "Externe KI-Anbieter direkt in diesen Einstellungen installieren oder entfernen.", | ||
| "installAndUse": "Install and use", |
There was a problem hiding this comment.
WARNING: New provider strings are untranslated English
The new settings.ai/settings.plugins keys (installAndUse, retryMigration, builtinProvider, availableProviders, providerRegistryOffline, ...) are copied in English into de, es, fr, ja, ko, pt-BR, ru, tl and zh (only it is translated). .rules/general.md requires translating into the locale files.
| try { | ||
| await client.call('migrate_ai_providers', undefined); | ||
| } catch (failure) { | ||
| setError(String(failure)); |
There was a problem hiding this comment.
WARNING: Migration errors are never shown
setError(String(failure)) is immediately cleared by the refresh() in finally, which calls setError(null) on success. A failed migrate_ai_providers therefore leaves the retry UI with no error. Also refresh has no request-id guard, so out-of-order concurrent refreshes (mount + two events + manual) can let an older catalog win, and String(failure) yields Error: ... instead of the message.
| isTheme | ||
| ? 'update.badges.themeUpdate' | ||
| : kind === 'ai-assistant' | ||
| ? 'settings.plugins.kindAiAssistant' |
There was a problem hiding this comment.
SUGGESTION: Wrong label for the update indicator
For ai-assistant the update tooltip/aria-label resolves to settings.plugins.kindAiAssistant (a kind name, "AI assistant"), not an update message. Add an update.badges.aiAssistantUpdate key.
| let variables = json!({"settings":settings,"model":request["model"],"prompt":request["prompt"],"system_prompt":request["system_prompt"]}); | ||
| let mut builder = Client::builder() | ||
| .timeout(Duration::from_secs(30)) | ||
| .redirect(reqwest::redirect::Policy::none()); |
There was a problem hiding this comment.
WARNING: Plain http:// and unrestricted hosts allowed for credentialed requests
recipe::request_url accepts http as well as https, and nothing prevents a package manifest (including {{settings.*}} URLs) from sending the API key header (send, lines 152-159) over cleartext or to private/loopback hosts. Redirects are disabled, which is good, but consider requiring https unless the host is loopback (Ollama / local OpenAI-compatible) when an API key is attached.
| return Err(error.to_string()); | ||
| } | ||
| if existed { | ||
| std::fs::remove_dir_all(backup).map_err(|e| e.to_string())?; |
There was a problem hiding this comment.
WARNING: Installed update reported as failed and leaves recovery state
If remove_dir_all(backup) fails after the new directory is already in place, the function returns Err even though the update succeeded, and the leftover .backup-{id} makes every later install fail with "A previous AI assistant update needs recovery" (line 158) until recover_package runs. This cleanup failure should be logged and ignored rather than failing the install.
| .map(str::to_owned) | ||
| .or_else(|| value.as_u64().map(|number| number.to_string())) | ||
| .ok_or("Invalid AI assistant URL setting")?; | ||
| output.push_str(text.trim_end_matches('/')); |
There was a problem hiding this comment.
SUGGESTION: Settings URL value is not validated for characters
{{settings.*}} values are appended verbatim (only trailing / trimmed) before Url::parse, so a user setting containing ?/#/path segments changes the request shape, and a ? in a prefix later makes {{model}} interpolation fail. Fragments/credentials are rejected after parsing, but consider documenting or validating that settings used in URLs are bare origins/paths.
Code Review SummaryStatus: 8 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (key files, 70+ changed in total)
Gaps: the full patch output was truncated and some read-only commands were rejected, so the remaining frontend components, tests and generated files were reviewed by sub-agents and spot checks rather than line by line. Fix these issues in Kilo Cloud Reviewed by claude-sonnet-5.5 · Input: 282 · Output: 10.6K · Cached: 2.4M Review guidance: REVIEW.md from base branch |
NewtTheWolf
left a comment
NewtTheWolf
left a comment
There was a problem hiding this comment.
Thanks for keeping the existing providers available offline and adding the contextual install flow. I re-reviewed 33df995ade79e032aedd7d7b48e4d4281b9ae496 and checked the companion registry contract.
I reproduced a model-cache bug in ai::get_models / ai_assistants::http::execute. Seed an expired entry with a previously discovered account model, make that provider's discovery endpoint return HTTP 503, then restore the endpoint and call get_models(false) again. Both results are empty and only the first call reaches the endpoint. Discovery swallows the error into Ok(default_models), then the host overwrites the previous catalog and advances fetched_at. With cache_seconds: 86400, that failed result stays fresh for a day. The independent regression fails with left: [], right: ["last-successful-account-model"].
Please distinguish successful discovery, including a deliberately empty successful response, from a failed request. Keep the last successful catalog on transient HTTP/transport/parse failure, do not mark the failed refresh successful, and cover recovery plus overlapping refreshes in regression tests.
The branch also needs integration with current main: there are conflicts in the AI runtime, settings/tests, application/plugin paths, desktop command wrapper and events, plus the removed parity file. Preserve the API Route provider and its cache/optional-key/inline-completion behavior added since this branch's base when resolving those conflicts. pnpm lint currently fails Prettier on plugins/ai-assistants/manifest.schema.json and tabularium-kind.json.
What passed on this head: production build, typecheck, 126 focused frontend tests and 2,230 Rust library tests (13 ignored). I also built and ran the real Linux Tauri app: seven embedded providers and eight icons were present; OpenAI Compatible generated SQL through an isolated local HTTP fixture and the app executed it against SQLite, returning IT=2 and US=1. A fixture HTTP 503 displayed a sanitized error and left the SQL unchanged. No live LLM account was used.
The published Gemini 1.0.1 ZIP and standalone manifest match, and their sizes/SHA-256 match GitHub's release metadata. The manifest validates against Tabularium #35's merged core-plus-kind schema. That prerelease has no min_runtime_version. The connected registry still returns 404 for ai-assistant and an empty AI-provider list, so production install/update/remove/reinstall and native live-Gemini parity remain unverified. Your new Gemini-only rollout checklist supersedes the seven-replacement-package plan still written in Tabularium #35. Keeping this open for the cache correction, main integration and coordinated release gates.
NewtTheWolf
left a comment
NewtTheWolf
left a comment
There was a problem hiding this comment.
Updated and verified 9518ced3cb53219b1f7888836d9b5390c3972fe9 with your permission. The failed-discovery cache bug is fixed: failed refreshes retain the latest successful catalog and its age, successful empty results clear it, and overlapping updates merge per provider. I also reproduced and fixed the lifecycle path that erased unrelated catalogs on assistant removal. Both failures now have passing regressions.
Current main is integrated, including API Route, inline completion, crash analysis and plugin-settings sync. API Route is an embedded declarative provider with the same environment/credential identity and explicit-model behavior. Migration preserves existing catalogs, and both new and existing AI entry points compile in desktop and server-only builds.
Lint, typecheck, build and pnpm test:all pass: 2,527 Rust library tests (19 ignored), 8,901 frontend tests. The real Linux desktop completed install/select, original Gemini 1.0.0→1.0.1 update, remove/reinstall, failure preservation, version gating and the scoped-cache checks. A local Gemini protocol fixture generated SQL that the app executed to IT=2 / US=1; inline completion/cancellation and a one-second provider timeout also worked. No live Gemini account or signed-JWS path was tested. A non-fatal Monaco cancellation is documented in the test evidence rather than hidden as a clean-console claim.
Code approved. The production Gemini rollout stays gated: the connected registry kind/listing is still absent, and the actual first shipping runtime version must be assigned before a public successor release/listing. I updated Tabularium #35's source preset and rollout documentation without changing production.
|
Verified |
External AI providers can be added as declarative HTTP packages, with credentials, configuration, proxy routing and timeouts owned by Tabularis. Existing users retain built-in providers without a registry download.
Behavior
Validation on the updated revision
pnpm lint,pnpm typecheck, production frontend build andpnpm test:allpass: 2,527 Rust library tests (19 ignored) and 8,901 frontend tests. Ignored live-database suites remain ignored.cargo check --no-default-featuresand the native desktop build withe2e-testingpass. Existing shared web-RPC/AI/inline tests are included; Linux native UI and IPC were exercised directly.generateContentusedx-goog-api-key, thought parts were skipped, and generated SQL executed to IT=2 / US=1. Native inline completion returned text and cancelled after about 130 ms; a one-second provider override timed out at about one second.Release gates
The production registry rollout is now complete for Gemini, as recorded below. OAuth, signing, streaming, pagination and multimodal request transformations remain outside this v1 runtime contract.
Published Gemini example
Google Gemini on Tabularium is now a concrete external
ai-assistantexample. Gemini 1.0.2 ships the standalone.tabulariummanifest andgemini-1.0.2-universal.zipwith:This is an excerpt; the complete manifest also declares the
ai_assistantHTTP contract.0.32.1-4is the next nightly expected on November 13, 2026, following the currently published0.32.1-3. The plugin is published now and requires that upcoming runtime or a newer compatible release; the nightly itself has not shipped yet.The authenticated operator has applied the
ai-assistantkind preset. Gemini 1.0.2 is approved and indexed, its universal ZIP resolves through the registry, and its version, runtime floor, SHA-256, size and signed integrity metadata are present. The release workflow passed and the repository webhook is installed for future releases. Gemini 1.0.0/1.0.1 remain historical development previews without this runtime floor.To try it on a compatible runtime: Settings → AI → Available providers → Google Gemini → Install and use, then configure a Gemini API key or
GEMINI_API_KEY. This publication check does not add a live Gemini generation test to the validation reported above.