Repository navigation
feat(plugins): support plugin-owned connection fields and secrets - #629
Conversation
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
|
|
||
| if !conn.plugin_secret_keys.is_empty() { | ||
| let cache = credential_cache::CredentialCache::default(); | ||
| crate::plugin_secrets::hydrate_connection(&cache, &mut conn).map_err(|message| { |
There was a problem hiding this comment.
WARNING: Plugin secret hydration runs blocking keychain I/O on the async runtime thread.
The adjacent DB-password (line 255) and connection-URI (line 286) reads in this same function both wrap their keychain access in tokio::task::spawn_blocking. hydrate_connection → get_plugin_secret_cached → keychain_utils::get_plugin_secret performs synchronous keychain reads directly on the async worker thread, which can stall the MCP server's runtime — notably on platforms where keychain access is slow or requires an unlock prompt. Mirror the surrounding pattern and move the hydration into spawn_blocking (one read per secret key, like the URI path does).
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: No Issues Found | Recommendation: Merge The previous WARNING (blocking keychain I/O during plugin secret hydration in Files Reviewed (7 files in the latest commit)
Previous Review Summary (commit d7cb728)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit d7cb728)Status: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Files Reviewed (38 files)
Reviewed by claude-sonnet-5.5 · Input: 252 · Output: 2.2K · Cached: 383.1K Review guidance: REVIEW.md from base branch |
|
@NewtTheWolf please can you check the tabularium part? |
|
I also checked the live Tabularium registry schema: |
Driver plugins can now adapt the host connection inputs and keep their own credentials in the OS keychain. - Manifest `connection_fields`: optional hidden/label/placeholder overrides for host, port, username, password and database. A hidden database input is no longer required, and hiding host also hides the Load databases action. Declared in manifest.schema.json and in the Tabularium extension schema. - `connection-modal.extra_fields` slot: optional `secretFields` (metadata only: value typed in this session, hasStoredValue, dirty) and `setSecretField(key, value)`; an empty value clears the secret. - Secrets are stored under `<id>:plugin:<driver>:<key>` and only their names are persisted (`plugin_secret_keys`). They are resolved into `params.extra[key]` in memory for every driver call, Test Connection, Load Databases and the MCP server, and never written to connections.json or returned to the frontend. - Lifecycle: save, update (driver switch drops the old secrets), duplicate, delete, group delete, export with credentials and backups, import (a metadata-only re-import keeps the stored secrets). - Disconnect and export no longer fail when a stored plugin secret cannot be read. - PLUGIN_GUIDE documents both contracts; @tabularis/plugin-api exposes the typed slot context and `PluginSecretFieldState`. Existing manifests, saved connections and UI extensions keep their behaviour: every new field is optional.
A plugin secret listed in `plugin_secret_keys` but missing from the OS keychain (synced connections file, keychain reset, downgrade) made every driver call of the connection fail, along with DDL, dump, MCP and duplicate. - Hydration is lenient everywhere: an invalid, missing or unreadable key (including a locked keychain or denied access) is logged by name, never by value, and left unset so the plugin reports the missing credential. Driver calls, Test Connection, Load Databases, export, backups and duplicate share one read path. - Duplicate copies only the values that can be read and lists only those keys on the copy. - Import skips invalid secret keys and oversized values with a warning instead of rejecting the payload, and writes plugin secrets to the keychain only after connections.json has been saved, so a failed save leaves no orphaned entries. - PLUGIN_GUIDE documents the unset-secret behaviour and what happens after a downgrade drops `plugin_secret_keys`.
d7cb728 to
02d65fb
Compare
Preview buildThe preview build of commit |
debba
changed the title
Summary
connection_fields: hide, relabel or set a placeholder forhost,port,username,password,database).<connectionId>:plugin:<driver>:<key>).hasStoredValue,dirty) and send tri-state changes throughsetSecretField; stored values are never returned to the frontend.params.extra[key]only for runtime driver calls and never written toconnections.jsonorconfig.json.Update
The branch was rebuilt on top of current
main(it was 816 commits behind). Besides resolving the conflicts:spawn_blocking.connections.jsonis saved, so a failed save leaves no orphaned secrets.PLUGIN_GUIDE.mddocuments the contract and the downgrade behaviour (an older version that rewritesconnections.jsondropsplugin_secret_keys; setting the secret again restores it).connection_fieldsis added toplugins/tabularium-extensions.schema.json. The Tabulariumdriverkind already accepts it.Compatibility
All new manifest, model, request and UI context fields are optional or defaulted. Existing manifests, saved connections, UI extensions and Password-based plugins keep their behaviour. Verified by loading every local plugin manifest with the old and new host, and by reading
connections.json, export payloads and backups written bymain(identical result) and the reverse (no crash, other data preserved).Dependent plugin PR: TabularisDB/tabularis-bigquery-plugin#2
Tests
plugin_secrets_tests.rs,persistence_tests.rs,NewConnectionModal.test.tsx, schema contract test.cargo test --all-targets,pnpm build,vitest,pnpm lint, plugin-api build andcheck:syncpass.