From af606a7207acab3b62e5a52eb96ed1efa36fe27c Mon Sep 17 00:00:00 2001 From: John Trujillo Date: Tue, 25 Aug 2026 12:38:16 -0500 Subject: [PATCH 1/5] refactor(ai): centralize plugin crypto on the host KeystoreSecretStore Also switches ChatViewModel's terminal-tool dedup to isTerminalToolName and adds JVM coverage for MCP keep-alive, the McpServerStore lock and McpPlugin scope cancellation. --- CLAUDE.md | 24 +++ ai-agent-gemini/README.md | 14 +- ai-agent-gemini/ai-agent-gemini.html | 6 +- ai-agent-gemini/build.gradle.kts | 4 +- ai-agent-gemini/src/main/AndroidManifest.xml | 2 +- .../aiagentgemini/backend/GeminiBackend.kt | 27 ++- .../preferences/GeminiPreferences.kt | 2 +- .../security/SecureApiKeyStore.kt | 151 +------------ .../settings/GeminiSettingsFragment.kt | 35 ++- .../settings/GeminiSettingsViewModel.kt | 29 ++- .../src/main/res/values/strings.xml | 1 + ai-agent-mcp/ai-agent-mcp.html | 5 +- ai-agent-mcp/build.gradle.kts | 1 + .../aiagentmcp/client/McpConnections.kt | 17 +- .../aiagentmcp/errors/McpErrorFormatter.kt | 7 + .../plugins/aiagentmcp/plugin/McpPlugin.kt | 69 +++++- .../aiagentmcp/security/SecureTokenStore.kt | 201 +----------------- .../security/UnavailableSecretException.kt | 14 ++ .../aiagentmcp/settings/McpServerStore.kt | 34 +-- .../settings/McpSettingsFragment.kt | 3 + .../settings/McpSettingsViewModel.kt | 18 +- ai-agent-mcp/src/main/res/values/strings.xml | 3 + .../client/McpSessionLifecycleTest.kt | 122 ++++++++++- .../aiagentmcp/plugin/McpPluginScopeTest.kt | 93 ++++++++ .../settings/McpServerStoreLockTest.kt | 124 +++++++++++ .../testing/FakeSharedPreferences.kt | 98 +++++++++ ai-agent-openai/README.md | 17 +- ai-agent-openai/ai-agent-openai.html | 5 +- ai-agent-openai/src/main/AndroidManifest.xml | 2 +- .../aiagentopenai/security/ApiKeyCache.kt | 25 ++- .../security/SecureApiKeyStore.kt | 148 +------------ .../settings/OpenAiSettingsFragment.kt | 35 ++- .../settings/OpenAiSettingsViewModel.kt | 27 ++- .../src/main/res/values/strings.xml | 1 + .../plugins/aicore/viewmodel/ChatViewModel.kt | 6 +- 35 files changed, 796 insertions(+), 574 deletions(-) create mode 100644 ai-agent-mcp/src/main/kotlin/com/itsaky/androidide/plugins/aiagentmcp/security/UnavailableSecretException.kt create mode 100644 ai-agent-mcp/src/test/kotlin/com/itsaky/androidide/plugins/aiagentmcp/plugin/McpPluginScopeTest.kt create mode 100644 ai-agent-mcp/src/test/kotlin/com/itsaky/androidide/plugins/aiagentmcp/settings/McpServerStoreLockTest.kt create mode 100644 ai-agent-mcp/src/test/kotlin/com/itsaky/androidide/plugins/aiagentmcp/testing/FakeSharedPreferences.kt diff --git a/CLAUDE.md b/CLAUDE.md index 11e90634..67337db8 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -46,6 +46,30 @@ There is also **one shared Gradle wrapper at the repo root** (`gradlew` + `gradl Both jars are referenced via `../libs/*.jar`. **Always use the repo-root `libs/` jars and the repo-root Gradle wrapper — never bundle per-plugin copies.** A plugin that ships its own `libs/plugin-api.jar` / `libs/gradle-plugin.jar` (e.g. copied from another plugin) can drift out of sync with the rest of the repo; point `build.gradle.kts` (`compileOnly`) and `settings.gradle.kts` (buildscript `classpath`) at `../libs/*.jar` and delete any local `libs/`. The root `plugin-api.jar` already carries the full API surface (including `IdeTemplateService`/`CgtTemplateBuilder`), so newer sub-APIs do not justify a local copy. **A plugin folder is not standalone in isolation** — copy the root `libs/` along if you move one elsewhere. When CoGo's API changes, refresh via the script above or the **Update libs from CodeOnTheGo** GitHub Action (which also commits the refreshed jars, cuts a release, and deploys `.cgp` files to the website). +### Credentials: use the host's `KeystoreSecretStore`, never your own crypto + +A plugin that stores a credential encrypts it with `com.itsaky.androidide.plugins.security.KeystoreSecretStore` +from `plugin-api.jar` (**26.35+** — set `plugin.min_ide_version` accordingly). It is `compileOnly` +like the rest of the API, so there is one implementation in the IDE's process rather than a copy +compiled into each `.cgp`. Do not re-implement AES/GCM in a plugin; three AI plugins each grew a +copy that started to diverge, which is what ADFA-5255 removed. + +Construct it with **this plugin's own alias** (`KeystoreSecretStore(ALIAS)`) as a single +top-level `val` in a `SecureApiKeyStore.kt`/`SecureTokenStore.kt` that holds nothing but the alias; +callers use that instance directly. The store logs under its own name — it takes no log tag, and +the second constructor parameter is a `SecretKeySource` override that plugins do not pass. +`ai-agent-mcp`, `ai-agent-gemini` and `ai-agent-openai` are the reference shape. Do **not** wrap +it in an object of forwarding methods — that is just a second copy of the store's contract to keep +in step. The alias must be unique per +plugin (all plugins share the host's UID and Keystore, so a shared alias lets one plugin's +invalidated-key recovery delete another's secret) and must never change across releases. + +`readAndMigrate` returns a three-way `Stored` (`Absent` / `Value` / `Unreadable`) rather than a +nullable String on purpose: "never saved" and "saved but this device's Keystore can no longer open +it" need opposite advice, and a plugin that collapses them tells a user their credential was +refused when it was never sent. Collapse it only where the caller genuinely has one answer for +both, and say so in a comment. + ### Plugin shape A plugin is an Android *application* module (despite installing as a library) with: diff --git a/ai-agent-gemini/README.md b/ai-agent-gemini/README.md index 4be7e9d3..128acc06 100644 --- a/ai-agent-gemini/README.md +++ b/ai-agent-gemini/README.md @@ -27,10 +27,14 @@ The key is entered in **AI Core → Agent settings**, not here. It is stored encrypted (AES/GCM under a hardware-backed Android Keystore secret) and sent as an `x-goog-api-key` **header**, never in a URL query string. -`security/SecureApiKeyStore.kt` is the only copy of the crypto — this plugin owns -both the write and the read, so there are no constants to keep in sync with -another plugin. A key written under an earlier plugin id is adopted once by -`preferences/GeminiPreferences.kt` and re-encrypted here. +`security/SecureApiKeyStore.kt` holds only this plugin's Keystore alias +(`cotg_ai_gemini_key_v1`); the AES/GCM itself is the IDE's `KeystoreSecretStore` +(`plugin-api`, since **26.35** — hence this plugin's `min_ide_version`), so there +is one implementation in the process rather than a copy per plugin. The alias +stays per plugin: they all share the host's Keystore, so a shared alias would let +one plugin's invalidated-key recovery delete another's secret. A key written under +an earlier plugin id is adopted once by `preferences/GeminiPreferences.kt` and +re-encrypted here. ## Installation @@ -57,7 +61,7 @@ root of `com/itsaky/androidide/plugins/aiagentgemini/`. - `plugin/GeminiPlugin.kt` — plugin entry point; registers the backend with ai-core - `backend/GeminiBackend.kt` — the REST transport, streaming (SSE) and model catalog - `errors/GeminiErrorFormatter.kt` — turns an API failure into one translated sentence -- `security/SecureApiKeyStore.kt` — AES/GCM at rest +- `security/SecureApiKeyStore.kt` — this plugin's Keystore alias, over the IDE's `KeystoreSecretStore` - `preferences/GeminiPreferences.kt` — this plugin's settings store, plus the one-time adoption of settings written under earlier plugin ids - `prompt/GeminiSystemPrompt.kt` — the system prompt this cloud model is given diff --git a/ai-agent-gemini/ai-agent-gemini.html b/ai-agent-gemini/ai-agent-gemini.html index 9a228040..e73c45ab 100644 --- a/ai-agent-gemini/ai-agent-gemini.html +++ b/ai-agent-gemini/ai-agent-gemini.html @@ -82,9 +82,9 @@

Technical architecture

GeminiErrorFormatterClassifies a failure (retired model, quota, refused key, outage, unreachable) so it can be reported as one translated sentence. - SecureApiKeyStoreAES/GCM encryption of the API - key under a hardware-backed Android Keystore secret owned by this - plugin. + SecureApiKeyStoreBinds this plugin's Keystore + alias to the IDE's KeystoreSecretStore, which AES/GCM-encrypts + the API key under a hardware-backed Android Keystore secret. GeminiSettingsFragmentThe settings pane AI Core mounts: key entry and verification, visibility toggle, and the model picker driven by the live catalog. diff --git a/ai-agent-gemini/build.gradle.kts b/ai-agent-gemini/build.gradle.kts index d3ecedf3..ab95ea6c 100644 --- a/ai-agent-gemini/build.gradle.kts +++ b/ai-agent-gemini/build.gradle.kts @@ -71,8 +71,8 @@ dependencies { testImplementation("org.json:json:20231013") } -// SecureApiKeyStore is no longer duplicated: this plugin holds the only copy, so there is nothing -// left to drift against. The parity check that guarded the ai-assistant copy went with that plugin. +// No SecureApiKeyStore parity check any more: the AES/GCM core is the host's KeystoreSecretStore +// (plugin-api), so there is one implementation in the process rather than copies to keep in step. // AAR metadata checks are disabled by convention for these application-as-library plugins. tasks.matching { diff --git a/ai-agent-gemini/src/main/AndroidManifest.xml b/ai-agent-gemini/src/main/AndroidManifest.xml index 5c721c7b..74ba6b64 100644 --- a/ai-agent-gemini/src/main/AndroidManifest.xml +++ b/ai-agent-gemini/src/main/AndroidManifest.xml @@ -33,7 +33,7 @@ and pairs with ai-core, which requires the same release. --> + android:value="26.35" /> stored.plain.trim().takeIf { it.isNotBlank() } + KeystoreSecretStore.Stored.Absent -> null + // Reported here rather than passed on as "no key": generation fails either way, but a + // lost Keystore entry needs the key entering again, and the log is all that says so. + KeystoreSecretStore.Stored.Unreadable -> { + context.logger.warn( + "GeminiBackend: the saved API key cannot be decrypted on this device; " + + "it has to be entered again in settings" + ) + null + } + // Transient, so it returns without caching: the key is very likely intact, and caching + // this answer would freeze "no key" until the stored value itself changed. + KeystoreSecretStore.Stored.Unavailable -> { + context.logger.warn( + "GeminiBackend: the keystore could not be reached to read the saved API key; " + + "retrying on the next read" + ) + return null + } + } val raw = prefs?.getString(GeminiPreferences.KEY_API_KEY, null) keyCache = raw?.let { it to plain } return plain diff --git a/ai-agent-gemini/src/main/kotlin/com/itsaky/androidide/plugins/aiagentgemini/preferences/GeminiPreferences.kt b/ai-agent-gemini/src/main/kotlin/com/itsaky/androidide/plugins/aiagentgemini/preferences/GeminiPreferences.kt index a23b01ff..c59d33ee 100644 --- a/ai-agent-gemini/src/main/kotlin/com/itsaky/androidide/plugins/aiagentgemini/preferences/GeminiPreferences.kt +++ b/ai-agent-gemini/src/main/kotlin/com/itsaky/androidide/plugins/aiagentgemini/preferences/GeminiPreferences.kt @@ -59,7 +59,7 @@ internal object GeminiPreferences { * Copies this backend's settings out of every store in [LEGACY_FILES], once. * * The API key moves as ciphertext and stays readable: it is encrypted under a Keystore alias - * (see [SecureApiKeyStore]) rather than under anything plugin-specific, and every plugin runs + * (see [secureApiKeyStore]) rather than under anything plugin-specific, and every plugin runs * in the host's process and UID. Copies rather than moves, so downgrading still finds the old * values. Call before anything reads a setting. * diff --git a/ai-agent-gemini/src/main/kotlin/com/itsaky/androidide/plugins/aiagentgemini/security/SecureApiKeyStore.kt b/ai-agent-gemini/src/main/kotlin/com/itsaky/androidide/plugins/aiagentgemini/security/SecureApiKeyStore.kt index 198b65e0..6daa4ca2 100644 --- a/ai-agent-gemini/src/main/kotlin/com/itsaky/androidide/plugins/aiagentgemini/security/SecureApiKeyStore.kt +++ b/ai-agent-gemini/src/main/kotlin/com/itsaky/androidide/plugins/aiagentgemini/security/SecureApiKeyStore.kt @@ -1,146 +1,17 @@ package com.itsaky.androidide.plugins.aiagentgemini.security -import android.content.SharedPreferences -import android.security.keystore.KeyGenParameterSpec -import android.security.keystore.KeyPermanentlyInvalidatedException -import android.security.keystore.KeyProperties -import android.util.Base64 -import android.util.Log -import com.itsaky.androidide.plugins.aiagentgemini.logging.LOG_PREFIX -import java.security.GeneralSecurityException -import java.security.KeyStore -import javax.crypto.Cipher -import javax.crypto.KeyGenerator -import javax.crypto.SecretKey -import javax.crypto.spec.GCMParameterSpec +import com.itsaky.androidide.plugins.security.KeystoreSecretStore + +/** Unique to this plugin and fixed across releases; see [KeystoreSecretStore] for why both matter. */ +private const val ALIAS = "cotg_ai_gemini_key_v1" /** - * AES/GCM encryption for sensitive settings (currently the Gemini API key), - * keyed by a hardware-backed Android Keystore secret. Only ciphertext is - * written to SharedPreferences, so a copied prefs file (root, `adb backup`, - * forensic dump) is useless without this device's Keystore. + * This plugin's binding of [KeystoreSecretStore]: its API key, encrypted under this plugin's own + * Keystore alias. * - * The [ALIAS] must stay stable across releases: a key encrypted under one - * alias cannot be read under another, so changing it silently invalidates - * every stored key. It is also what lets a key written before the AI plugins - * were reorganised still decrypt today — every plugin runs in the host app's - * process and UID, so they all share one Android Keystore. + * The store is the IDE's, from plugin-api, and callers use it directly. A forwarding object per + * method would only be a second copy of its contract to keep in step — and one that had to pick a + * single answer for "absent" and "no longer decryptable", which callers here do not share. The + * thing this file owns is the alias. */ -object SecureApiKeyStore { - private const val TAG = "$LOG_PREFIX.SecureApiKeyStore" - private const val KEYSTORE = "AndroidKeyStore" - private const val ALIAS = "cotg_ai_gemini_key_v1" - private const val TRANSFORM = "AES/GCM/NoPadding" - private const val IV_LEN = 12 - private const val TAG_BITS = 128 - - /** Marks a stored value as ciphertext; anything without it is treated as legacy plaintext. */ - const val ENC_PREFIX = "enc:v1:" - - private fun getOrCreateKey(): SecretKey { - val ks = KeyStore.getInstance(KEYSTORE).apply { load(null) } - (ks.getEntry(ALIAS, null) as? KeyStore.SecretKeyEntry)?.let { return it.secretKey } - val generator = KeyGenerator.getInstance(KeyProperties.KEY_ALGORITHM_AES, KEYSTORE) - generator.init( - KeyGenParameterSpec.Builder( - ALIAS, - KeyProperties.PURPOSE_ENCRYPT or KeyProperties.PURPOSE_DECRYPT - ) - .setBlockModes(KeyProperties.BLOCK_MODE_GCM) - .setEncryptionPaddings(KeyProperties.ENCRYPTION_PADDING_NONE) - .build() - ) - return generator.generateKey() - } - - private fun deleteKey() { - try { - KeyStore.getInstance(KEYSTORE).apply { load(null) }.deleteEntry(ALIAS) - } catch (e: Exception) { - Log.w(TAG, "Failed to delete Keystore alias $ALIAS", e) - } - } - - private fun encryptWith(key: SecretKey, plain: String): String { - val cipher = Cipher.getInstance(TRANSFORM) - cipher.init(Cipher.ENCRYPT_MODE, key) - val iv = cipher.iv - val ciphertext = cipher.doFinal(plain.toByteArray(Charsets.UTF_8)) - val combined = ByteArray(iv.size + ciphertext.size) - System.arraycopy(iv, 0, combined, 0, iv.size) - System.arraycopy(ciphertext, 0, combined, iv.size, ciphertext.size) - return ENC_PREFIX + Base64.encodeToString(combined, Base64.NO_WRAP) - } - - /** - * Encrypt [plain] into a self-describing string: [ENC_PREFIX] + base64(iv | ciphertext). - * - * The key is not auth-bound, so a credential change does not invalidate it; an alias an - * OEM Keystore drops anyway is regenerated once before retrying. - * - * @param plain the value to encrypt - * @throws GeneralSecurityException on any other Keystore/cipher failure, so the caller can - * inform the user instead of crashing the IDE on Save - */ - @Throws(GeneralSecurityException::class) - fun encrypt(plain: String): String { - return try { - encryptWith(getOrCreateKey(), plain) - } catch (e: KeyPermanentlyInvalidatedException) { - Log.w(TAG, "Keystore key invalidated; regenerating and retrying encrypt", e) - deleteKey() - encryptWith(getOrCreateKey(), plain) - } - } - - /** - * Return the plaintext for a stored value, handling both formats transparently: - * an [ENC_PREFIX] value is decrypted; anything else is returned unchanged as - * legacy plaintext (use [readAndMigrate] to upgrade it in place). Returns - * null if a ciphertext value can't be decrypted — e.g. the Keystore key was - * lost or invalidated — in which case the user must re-enter the key. - */ - fun decrypt(stored: String?): String? { - if (stored == null) return null - if (!stored.startsWith(ENC_PREFIX)) return stored - return try { - val combined = Base64.decode(stored.removePrefix(ENC_PREFIX), Base64.NO_WRAP) - val iv = combined.copyOfRange(0, IV_LEN) - val ciphertext = combined.copyOfRange(IV_LEN, combined.size) - val cipher = Cipher.getInstance(TRANSFORM) - cipher.init(Cipher.DECRYPT_MODE, getOrCreateKey(), GCMParameterSpec(TAG_BITS, iv)) - String(cipher.doFinal(ciphertext), Charsets.UTF_8) - } catch (e: Exception) { - Log.w(TAG, "Failed to decrypt stored API key", e) - null - } - } - - /** - * Read [key] from [prefs], upgrading a legacy plaintext value to ciphertext in place. - * - * Keys written before this store existed are still plaintext on disk, and [decrypt] alone - * hands them back unchanged forever — so an install that configured its key earlier would - * never actually gain encryption. Re-encrypting on the first read closes that gap without - * making the user re-enter the key. - * - * The value is trimmed on migration, so the stored, displayed and sent forms all agree. - * - * Keystore IPC + AES/GCM, so call this off the main thread. - * - * @return the trimmed plaintext value, or null when nothing is stored or decryption failed. - */ - fun readAndMigrate(prefs: SharedPreferences?, key: String): String? { - val stored = prefs?.getString(key, null) ?: return null - if (stored.startsWith(ENC_PREFIX)) return decrypt(stored) - val plain = stored.trim() - if (plain.isEmpty()) return plain - try { - prefs.edit().putString(key, encrypt(plain)).apply() - Log.i(TAG, "Upgraded legacy plaintext value for '$key' to ciphertext") - } catch (e: Exception) { - Log.w(TAG, "Could not upgrade legacy plaintext value for '$key' to ciphertext", e) - } - return plain - } -} +val secureApiKeyStore = KeystoreSecretStore(ALIAS) diff --git a/ai-agent-gemini/src/main/kotlin/com/itsaky/androidide/plugins/aiagentgemini/settings/GeminiSettingsFragment.kt b/ai-agent-gemini/src/main/kotlin/com/itsaky/androidide/plugins/aiagentgemini/settings/GeminiSettingsFragment.kt index bec2f7bd..c9dbea4b 100644 --- a/ai-agent-gemini/src/main/kotlin/com/itsaky/androidide/plugins/aiagentgemini/settings/GeminiSettingsFragment.kt +++ b/ai-agent-gemini/src/main/kotlin/com/itsaky/androidide/plugins/aiagentgemini/settings/GeminiSettingsFragment.kt @@ -31,6 +31,7 @@ import com.itsaky.androidide.plugins.PluginContext import com.itsaky.androidide.plugins.aiagentgemini.plugin.GeminiPlugin import com.itsaky.androidide.plugins.aiagentgemini.R import com.itsaky.androidide.plugins.base.PluginFragmentHelper +import com.itsaky.androidide.plugins.security.KeystoreSecretStore import com.itsaky.androidide.plugins.services.IdeTooltipService import kotlinx.coroutines.launch import java.text.SimpleDateFormat @@ -189,20 +190,30 @@ class GeminiSettingsFragment : Fragment() { } viewLifecycleOwner.lifecycleScope.launch { - val savedApiKey = viewModel.getGeminiApiKey() + val stored = viewModel.getGeminiApiKey() + val savedApiKey = (stored as? KeystoreSecretStore.Stored.Value)?.plain val hasKey = !savedApiKey.isNullOrBlank() updateUiState(isEditing = !hasKey) if (hasKey) { statusTextView.text = savedApiKeyStatusText() } else { apiKeyInput.setText("") - // A stored-but-undecryptable key also reads as null; warn as the Edit path does. - if (viewModel.hasStoredGeminiApiKey()) { + // Only for a key that is there and will not decrypt; an empty box alone looks like + // data loss. Nothing stored at all is the ordinary first run and says nothing. + if (stored is KeystoreSecretStore.Stored.Unreadable) { Toast.makeText( requireContext(), getString(R.string.msg_api_key_unreadable), Toast.LENGTH_LONG ).show() + } else if (stored is KeystoreSecretStore.Stored.Unavailable) { + // Said differently from the above: the key is still there and intact, so this + // must not send the user off to find and type it again. + Toast.makeText( + requireContext(), + getString(R.string.msg_api_key_unavailable), + Toast.LENGTH_LONG + ).show() } } } @@ -383,20 +394,30 @@ class GeminiSettingsFragment : Fragment() { editButton.setOnClickListener { editButton.isEnabled = false viewLifecycleOwner.lifecycleScope.launch { - val apiKey = try { + val stored = try { viewModel.getGeminiApiKey() } finally { editButton.isEnabled = true } - // null = a key IS stored but won't decrypt; an empty box alone looks like data loss. - if (apiKey == null) { + // A key that is stored and will not decrypt; an empty box alone looks like data + // loss. Told apart from "nothing stored" here, which this button rarely sees but + // must not report as a lost Keystore entry when it does. + if (stored is KeystoreSecretStore.Stored.Unreadable) { Toast.makeText( requireContext(), getString(R.string.msg_api_key_unreadable), Toast.LENGTH_LONG ).show() + } else if (stored is KeystoreSecretStore.Stored.Unavailable) { + // Said differently from the above: the key is still there and intact, so this + // must not send the user off to find and type it again. + Toast.makeText( + requireContext(), + getString(R.string.msg_api_key_unavailable), + Toast.LENGTH_LONG + ).show() } - revealEditMode(apiKey.orEmpty()) + revealEditMode((stored as? KeystoreSecretStore.Stored.Value)?.plain.orEmpty()) } } diff --git a/ai-agent-gemini/src/main/kotlin/com/itsaky/androidide/plugins/aiagentgemini/settings/GeminiSettingsViewModel.kt b/ai-agent-gemini/src/main/kotlin/com/itsaky/androidide/plugins/aiagentgemini/settings/GeminiSettingsViewModel.kt index 12060510..3e47a891 100644 --- a/ai-agent-gemini/src/main/kotlin/com/itsaky/androidide/plugins/aiagentgemini/settings/GeminiSettingsViewModel.kt +++ b/ai-agent-gemini/src/main/kotlin/com/itsaky/androidide/plugins/aiagentgemini/settings/GeminiSettingsViewModel.kt @@ -10,7 +10,8 @@ import com.itsaky.androidide.plugins.PluginLogger import com.itsaky.androidide.plugins.aiagentgemini.backend.GeminiBackend import com.itsaky.androidide.plugins.aiagentgemini.logging.LOG_PREFIX import com.itsaky.androidide.plugins.aiagentgemini.preferences.GeminiPreferences -import com.itsaky.androidide.plugins.aiagentgemini.security.SecureApiKeyStore +import com.itsaky.androidide.plugins.aiagentgemini.security.secureApiKeyStore +import com.itsaky.androidide.plugins.security.KeystoreSecretStore import kotlinx.coroutines.CancellationException import kotlinx.coroutines.CoroutineDispatcher import kotlinx.coroutines.Dispatchers @@ -112,7 +113,7 @@ class GeminiSettingsViewModel( } /** - * Encrypts [apiKey] via [SecureApiKeyStore] and persists only the ciphertext to private prefs, + * Encrypts [apiKey] via [secureApiKeyStore] and persists only the ciphertext to private prefs, * off the main thread. Nothing is written on failure. Kept separate from [verifyGeminiKey]: a * rejected key never reaches here, and an unverifiable one only after the user says so. * @@ -130,7 +131,7 @@ class GeminiSettingsViewModel( return@withContext false } val encrypted = try { - SecureApiKeyStore.encrypt(apiKey.trim()) + secureApiKeyStore.encrypt(apiKey.trim()) } catch (e: Exception) { logger?.error("$TAG: failed to encrypt Gemini API key", e) return@withContext false @@ -156,19 +157,15 @@ class GeminiSettingsViewModel( * Decrypt the stored key off the main thread (Keystore IPC + AES/GCM), upgrading a * pre-encryption plaintext key to ciphertext in passing so existing installs actually * end up encrypted rather than waiting for the user to re-enter the key. + * + * @return what is on disk: nothing, the key, a key this device's Keystore can no longer open, + * or one it would not open just now. Those are not the same — a lost Keystore entry has to be + * entered again, a keystore that did not answer only retried — so the caller says which. */ - suspend fun getGeminiApiKey(): String? = withContext(ioDispatcher) { - SecureApiKeyStore.readAndMigrate(prefs(), KEY_API_KEY) + suspend fun getGeminiApiKey(): KeystoreSecretStore.Stored = withContext(ioDispatcher) { + secureApiKeyStore.readAndMigrate(prefs(), KEY_API_KEY) } - /** - * True when a key is present on disk, whether or not it can still be decrypted. Lets the UI - * tell "nothing was saved" from "the Keystore entry is gone" — [getGeminiApiKey] is null for - * both. Raw pref only, so no Keystore IPC and safe on the main thread. - */ - fun hasStoredGeminiApiKey(): Boolean = - !prefs()?.getString(KEY_API_KEY, null).isNullOrBlank() - fun getGeminiApiKeySaveTimestamp(): Long = prefs()?.getLong(KEY_API_KEY_TIMESTAMP, 0L) ?: 0L fun clearGeminiApiKey() { @@ -199,9 +196,11 @@ class GeminiSettingsViewModel( _geminiModelsLoading.postValue(true) try { - val apiKey = getGeminiApiKey()?.trim() + // The fallback list is the answer to any key it cannot read, whatever the reason, + // so this is one of the few callers that has no use for the difference. + val apiKey = (getGeminiApiKey() as? KeystoreSecretStore.Stored.Value)?.plain?.trim() if (apiKey.isNullOrBlank()) { - logger?.warn("$TAG: no Gemini API key saved; showing fallback models") + logger?.warn("$TAG: no usable Gemini API key saved; showing fallback models") _geminiModels.postValue(GeminiModelOptions(FALLBACK_MODELS, isLive = false)) return@launch } diff --git a/ai-agent-gemini/src/main/res/values/strings.xml b/ai-agent-gemini/src/main/res/values/strings.xml index 255b1997..d5345f3e 100644 --- a/ai-agent-gemini/src/main/res/values/strings.xml +++ b/ai-agent-gemini/src/main/res/values/strings.xml @@ -30,6 +30,7 @@ API Key saved on: %s API Key saved and verified on: %s The stored API key could not be read on this device. Please enter it again. + The device keystore could not be reached, so the stored API key could not be read. It is still saved — please try again in a moment. Couldn\'t save the API key on this device. Please try again. Checking this key with Google… Verified, your API key works diff --git a/ai-agent-mcp/ai-agent-mcp.html b/ai-agent-mcp/ai-agent-mcp.html index 121feb82..d45b4742 100644 --- a/ai-agent-mcp/ai-agent-mcp.html +++ b/ai-agent-mcp/ai-agent-mcp.html @@ -107,8 +107,9 @@

Technical architecture

McpToolTextSanitises server-supplied names and descriptions — untrusted remote text that would otherwise land verbatim in a prompt assembled inside a third-party backend plugin. - SecureTokenStoreAES/GCM encryption under a - Keystore alias owned by this plugin, so only ciphertext reaches disk. + SecureTokenStoreBinds this plugin's own Keystore + alias to the IDE's KeystoreSecretStore, whose AES/GCM keeps + only ciphertext on disk.

The transport is MCP's Streamable HTTP revision over HttpURLConnection. Two dependencies were deliberately not taken: diff --git a/ai-agent-mcp/build.gradle.kts b/ai-agent-mcp/build.gradle.kts index cf5a3325..b082c4ce 100644 --- a/ai-agent-mcp/build.gradle.kts +++ b/ai-agent-mcp/build.gradle.kts @@ -75,6 +75,7 @@ dependencies { testImplementation(files("../libs/plugin-api.jar")) testImplementation("junit:junit:4.13.2") + testImplementation("io.mockk:mockk:1.13.8") testImplementation("org.json:json:20240303") testImplementation("org.jetbrains.kotlinx:kotlinx-coroutines-test:1.8.1") } diff --git a/ai-agent-mcp/src/main/kotlin/com/itsaky/androidide/plugins/aiagentmcp/client/McpConnections.kt b/ai-agent-mcp/src/main/kotlin/com/itsaky/androidide/plugins/aiagentmcp/client/McpConnections.kt index ace2ea23..1f11a8a6 100644 --- a/ai-agent-mcp/src/main/kotlin/com/itsaky/androidide/plugins/aiagentmcp/client/McpConnections.kt +++ b/ai-agent-mcp/src/main/kotlin/com/itsaky/androidide/plugins/aiagentmcp/client/McpConnections.kt @@ -2,10 +2,11 @@ package com.itsaky.androidide.plugins.aiagentmcp.client import android.util.Log import com.itsaky.androidide.plugins.aiagentmcp.logging.LOG_PREFIX -import com.itsaky.androidide.plugins.aiagentmcp.security.SecureTokenStore +import com.itsaky.androidide.plugins.aiagentmcp.security.UnavailableSecretException import com.itsaky.androidide.plugins.aiagentmcp.security.UnreadableSecretException import com.itsaky.androidide.plugins.aiagentmcp.settings.McpServer import com.itsaky.androidide.plugins.aiagentmcp.settings.McpServerStore +import com.itsaky.androidide.plugins.security.KeystoreSecretStore import java.security.MessageDigest import java.util.concurrent.ConcurrentHashMap @@ -89,13 +90,21 @@ object McpConnections { * @return its token and headers; either may be empty. * @throws UnreadableSecretException when a stored credential cannot be decrypted here. Sending * the request without it would earn a 401 and tell the user their token was refused. + * @throws UnavailableSecretException when the keystore could not be reached to decrypt one, a + * failure to retry rather than to report as a lost credential. */ private fun credentialsFor(serverId: String): McpCredentials { val token = when (val stored = McpServerStore.token(serverId)) { - is SecureTokenStore.Stored.Value -> stored.plain - SecureTokenStore.Stored.Absent -> "" - SecureTokenStore.Stored.Unreadable -> + is KeystoreSecretStore.Stored.Value -> stored.plain + KeystoreSecretStore.Stored.Absent -> "" + KeystoreSecretStore.Stored.Unreadable -> throw UnreadableSecretException("The stored token for '$serverId' cannot be decrypted.") + // Not Unreadable: the token is very likely intact and the call is worth repeating, so + // the user is told to retry rather than to enter the token again. + KeystoreSecretStore.Stored.Unavailable -> + throw UnavailableSecretException( + "The stored token for '$serverId' could not be read just now." + ) } return McpCredentials(token, McpServerStore.headers(serverId)) } diff --git a/ai-agent-mcp/src/main/kotlin/com/itsaky/androidide/plugins/aiagentmcp/errors/McpErrorFormatter.kt b/ai-agent-mcp/src/main/kotlin/com/itsaky/androidide/plugins/aiagentmcp/errors/McpErrorFormatter.kt index 30bc3ddd..05081868 100644 --- a/ai-agent-mcp/src/main/kotlin/com/itsaky/androidide/plugins/aiagentmcp/errors/McpErrorFormatter.kt +++ b/ai-agent-mcp/src/main/kotlin/com/itsaky/androidide/plugins/aiagentmcp/errors/McpErrorFormatter.kt @@ -3,6 +3,7 @@ package com.itsaky.androidide.plugins.aiagentmcp.errors import android.content.Context import com.itsaky.androidide.plugins.aiagentmcp.R import com.itsaky.androidide.plugins.aiagentmcp.client.McpProtocolException +import com.itsaky.androidide.plugins.aiagentmcp.security.UnavailableSecretException import com.itsaky.androidide.plugins.aiagentmcp.security.UnreadableSecretException import com.itsaky.androidide.plugins.aiagentmcp.transport.McpHttpException import com.itsaky.androidide.plugins.aiagentmcp.transport.McpRedirectException @@ -64,6 +65,9 @@ sealed interface McpFailure { /** A stored credential cannot be decrypted on this device, so nothing was sent. */ data object SecretUnreadable : McpFailure + /** The keystore would not open a stored credential just now, so nothing was sent — retry. */ + data object SecretUnavailable : McpFailure + /** The server redirected somewhere the request cannot be repeated with its credentials. */ data object RedirectRefused : McpFailure @@ -88,6 +92,7 @@ object McpErrorFormatter { fun classify(error: Throwable): McpFailure = when (error) { // Before the IOException branches below, which it is one of. is UnreadableSecretException -> McpFailure.SecretUnreadable + is UnavailableSecretException -> McpFailure.SecretUnavailable is McpRedirectException -> McpFailure.RedirectRefused is McpHttpException -> forStatus(error.statusCode) is McpProtocolException -> McpFailure.Rejected(error.message.orEmpty()) @@ -125,6 +130,8 @@ object McpErrorFormatter { McpFailure.Cancelled -> context.getString(R.string.mcp_error_cancelled, serverName) McpFailure.SecretUnreadable -> context.getString(R.string.mcp_error_secret_unreadable, serverName) + McpFailure.SecretUnavailable -> + context.getString(R.string.mcp_error_secret_unavailable, serverName) McpFailure.RedirectRefused -> context.getString(R.string.mcp_error_redirect_refused, serverName) is McpFailure.ServerError -> diff --git a/ai-agent-mcp/src/main/kotlin/com/itsaky/androidide/plugins/aiagentmcp/plugin/McpPlugin.kt b/ai-agent-mcp/src/main/kotlin/com/itsaky/androidide/plugins/aiagentmcp/plugin/McpPlugin.kt index 2dc68346..79732e27 100644 --- a/ai-agent-mcp/src/main/kotlin/com/itsaky/androidide/plugins/aiagentmcp/plugin/McpPlugin.kt +++ b/ai-agent-mcp/src/main/kotlin/com/itsaky/androidide/plugins/aiagentmcp/plugin/McpPlugin.kt @@ -18,6 +18,7 @@ import com.itsaky.androidide.plugins.services.SharedServices import com.itsaky.androidide.plugins.services.ToolSourceRegistry import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.Job import kotlinx.coroutines.SupervisorJob import kotlinx.coroutines.cancel import kotlinx.coroutines.isActive @@ -38,15 +39,33 @@ class McpPlugin : IPlugin, SettingsExtension, DocumentationExtension { /** True once [toolSource] is registered with AI Core, so re-registration is idempotent. */ @Volatile private var registered = false + /** + * Serialises every swap of [scope]. + * + * Cancelling the old scope and installing a new one is one transition, not two: without this, + * two lifecycle calls landing together can each cancel the scope the other has already replaced, + * leaving a live refresh behind after [deactivate] or orphaning an activation's scope uncancelled. + * `@Volatile` alone would publish each write but still let the pair interleave. + */ + private val lifecycleLock = Any() + /** * Background work: listing tools is network work and never belongs on the main thread. * * Replaced on every [activate] and cancelled by [deactivate], so a refresh left running cannot * register sessions in [McpConnections] after `closeAll()` emptied the map. Volatile like - * [registered]: the host may drive the lifecycle from one thread and the next from another. + * [registered]: the host may drive the lifecycle from one thread and the next from another, + * and [scopeJob] reads it outside [lifecycleLock]. */ @Volatile private var scope = newScope() + /** + * The scope [stopScope] leaves behind: cancelled from birth, so a `launch` arriving after the + * lifecycle edge is the no-op it has always been. One per plugin, since cancellation is + * terminal and a cancelled scope carries no state a later stop could disturb. + */ + private val stoppedScope = newScope().apply { cancel() } + companion object { /** Must match `plugin.id` in AndroidManifest.xml; also this source's provider id. */ const val PLUGIN_ID = "com.itsaky.androidide.plugins.aiagentmcp" @@ -117,9 +136,10 @@ class McpPlugin : IPlugin, SettingsExtension, DocumentationExtension { } override fun activate(): Boolean = try { - // Cancelled first: a host that activates twice would otherwise orphan the running scope. - scope.cancel() - scope = newScope() + // Cancelled and replaced as one step: a host that activates twice would otherwise orphan + // the running scope, and the launch below has to use this activation's scope, not whatever + // a concurrent lifecycle call has since installed. + val active = swapScope(newScope()) toolSource = McpToolSource() McpServerStore.addChangeListener(settingsChanged) context.addPluginLifecycleListener(aiCoreLifecycle) @@ -130,7 +150,7 @@ class McpPlugin : IPlugin, SettingsExtension, DocumentationExtension { // Tool lists are answered from cache, so the cache has to be filled before the user opens // the Agent — otherwise the first cold-start session sees no MCP tools at all. - scope.launch { + active.launch { val refreshed = McpToolCatalog.refreshAll { isActive } if (refreshed > 0 && isActive) settingsChanged() } @@ -146,7 +166,7 @@ class McpPlugin : IPlugin, SettingsExtension, DocumentationExtension { unregisterToolSource() // Before the connections are closed: an in-flight refresh would otherwise repopulate the // catalogue and the session map straight after they were cleared. - scope.cancel() + stopScope() releaseConnections() true } catch (e: Exception) { @@ -158,7 +178,7 @@ class McpPlugin : IPlugin, SettingsExtension, DocumentationExtension { runCatching { context.removePluginLifecycleListener(aiCoreLifecycle) } McpServerStore.removeChangeListener(settingsChanged) unregisterToolSource() - scope.cancel() + stopScope() releaseConnections() pluginContext = null context.logger.info("McpPlugin: disposed") @@ -167,6 +187,41 @@ class McpPlugin : IPlugin, SettingsExtension, DocumentationExtension { /** A fresh scope for this activation; the previous one is cancelled, never reused. */ private fun newScope() = CoroutineScope(SupervisorJob() + Dispatchers.IO) + /** + * Installs [next] as the current scope and cancels whichever scope it displaced. + * + * @param next the scope to install. + * @return [next], so a caller can launch on the scope it installed rather than re-reading the + * field and handing its work to a later activation. + */ + private fun swapScope(next: CoroutineScope): CoroutineScope { + val previous = synchronized(lifecycleLock) { scope.also { scope = next } } + previous.cancel() + return next + } + + /** + * Ends the current activation's scope, leaving an already-cancelled one in the field. + * + * Installing [stoppedScope] rather than cancelling the field in place: an [activate] running + * alongside this has by then installed a scope of its own, and cancelling whatever the field + * happens to hold would either miss it or kill it. Swapping ends exactly the scope this call + * displaced. + */ + private fun stopScope() { + swapScope(stoppedScope) + } + + /** + * The current activation scope's job. + * + * A seam: the rule that a second [activate] orphans nothing and that [deactivate] leaves no + * refresh running is otherwise only observable on a device, where the symptom is a background + * `tools/list` repopulating a catalogue that was just cleared. + */ + internal val scopeJob: Job? + get() = scope.coroutineContext[Job] + /** * Registers this plugin's tools with AI Core, if the registry is reachable. * diff --git a/ai-agent-mcp/src/main/kotlin/com/itsaky/androidide/plugins/aiagentmcp/security/SecureTokenStore.kt b/ai-agent-mcp/src/main/kotlin/com/itsaky/androidide/plugins/aiagentmcp/security/SecureTokenStore.kt index 40e44e13..38139600 100644 --- a/ai-agent-mcp/src/main/kotlin/com/itsaky/androidide/plugins/aiagentmcp/security/SecureTokenStore.kt +++ b/ai-agent-mcp/src/main/kotlin/com/itsaky/androidide/plugins/aiagentmcp/security/SecureTokenStore.kt @@ -1,199 +1,16 @@ package com.itsaky.androidide.plugins.aiagentmcp.security -import android.content.SharedPreferences -import android.security.keystore.KeyGenParameterSpec -import android.security.keystore.KeyPermanentlyInvalidatedException -import android.security.keystore.KeyProperties -import android.util.Base64 -import android.util.Log -import com.itsaky.androidide.plugins.aiagentmcp.logging.LOG_PREFIX -import java.security.GeneralSecurityException -import java.security.KeyStore -import javax.crypto.Cipher -import javax.crypto.KeyGenerator -import javax.crypto.SecretKey -import javax.crypto.spec.GCMParameterSpec +import com.itsaky.androidide.plugins.security.KeystoreSecretStore -private const val TAG = "$LOG_PREFIX.SecureTokenStore" +/** Unique to this plugin and fixed across releases; see [KeystoreSecretStore] for why both matter. */ +private const val ALIAS = "cotg_ai_mcp_token_v1" /** - * AES/GCM encryption for the bearer tokens of configured MCP servers, keyed by a hardware-backed - * Android Keystore secret. Only ciphertext is written to SharedPreferences, so a copied prefs file - * (root, `adb backup`, forensic dump) is useless without this device's Keystore. + * This plugin's binding of [KeystoreSecretStore]: the bearer tokens and extra headers of configured + * MCP servers, encrypted under this plugin's own Keystore alias. * - * The [ALIAS] must stay stable across releases — a token encrypted under one alias cannot be read - * under another — and is deliberately this plugin's own: every plugin runs in the host's process - * and UID and therefore shares one Keystore, so a shared alias would let this plugin's recovery - * path destroy a backend plugin's stored key as a side effect. + * The store is the IDE's, from plugin-api, and callers use it directly. A forwarding object per + * method would only be a second copy of its contract to keep in step — the thing this file owns is + * the alias. */ -object SecureTokenStore { - - /** - * What was found under a preference key. - * - * Three outcomes rather than a nullable String: "nothing stored" and "stored but no longer - * readable on this device" lead to opposite advice, and collapsing them is what tells a user - * their token was refused when it was never sent. - */ - sealed interface Stored { - - /** Nothing is stored under the key. */ - data object Absent : Stored - - /** The stored value, decrypted. */ - data class Value(val plain: String) : Stored - - /** Something is stored, but this device's Keystore can no longer open it. */ - data object Unreadable : Stored - } - - private const val KEYSTORE = "AndroidKeyStore" - private const val ALIAS = "cotg_ai_mcp_token_v1" - private const val TRANSFORM = "AES/GCM/NoPadding" - private const val IV_LEN = 12 - private const val TAG_BITS = 128 - - /** Marks a stored value as ciphertext; anything without it is treated as legacy plaintext. */ - const val ENC_PREFIX = "enc:v1:" - - /** - * Encrypts [plain] into a self-describing string: [ENC_PREFIX] + base64(iv | ciphertext). - * - * The key is not auth-bound, so a credential change does not invalidate it; an alias an OEM - * Keystore drops anyway is regenerated once before retrying. - * - * @param plain the value to encrypt. - * @return the ciphertext to store. - * @throws GeneralSecurityException on any other Keystore or cipher failure, so the caller can - * tell the user instead of crashing the IDE on Save. - */ - @Throws(GeneralSecurityException::class) - fun encrypt(plain: String): String = try { - encryptWith(getOrCreateKey(), plain) - } catch (e: KeyPermanentlyInvalidatedException) { - Log.w(TAG, "Keystore key invalidated; regenerating and retrying encrypt", e) - deleteKey() - encryptWith(getOrCreateKey(), plain) - } - - /** - * Reads a stored value back. - * @param stored the stored string, ciphertext or legacy plaintext. - * @return the plaintext, or null when a ciphertext value cannot be decrypted — the Keystore key - * was lost, and the user has to enter the token again. - */ - fun decrypt(stored: String?): String? { - if (stored == null) return null - if (!stored.startsWith(ENC_PREFIX)) return stored - return try { - val combined = Base64.decode(stored.removePrefix(ENC_PREFIX), Base64.NO_WRAP) - val iv = combined.copyOfRange(0, IV_LEN) - val ciphertext = combined.copyOfRange(IV_LEN, combined.size) - val cipher = Cipher.getInstance(TRANSFORM) - cipher.init(Cipher.DECRYPT_MODE, getOrCreateKey(), GCMParameterSpec(TAG_BITS, iv)) - // Zeroed once the String is built; see the note in encryptWith about the String. - val plainBytes = cipher.doFinal(ciphertext) - try { - String(plainBytes, Charsets.UTF_8) - } finally { - plainBytes.fill(0) - } - } catch (e: Exception) { - Log.w(TAG, "Failed to decrypt a stored MCP token", e) - null - } - } - - /** - * Stores [plain] under [key], encrypted; an empty value removes the entry instead. - * - * Keystore IPC plus AES/GCM, so call this off the main thread. - * - * @param prefs where to store it. - * @param key the preference key. - * @param plain the token, or empty to forget it. - * @return true when the value was stored (or removed), false when encryption failed. - */ - fun write(prefs: SharedPreferences?, key: String, plain: String): Boolean { - val editor = prefs?.edit() ?: return false - if (plain.isBlank()) { - editor.remove(key).apply() - return true - } - return try { - editor.putString(key, encrypt(plain)).apply() - true - } catch (e: Exception) { - Log.e(TAG, "Could not encrypt a token for '$key'", e) - false - } - } - - /** - * Reads [key] from [prefs], upgrading a legacy plaintext value to ciphertext in place. - * - * @param prefs where the value lives. - * @param key the preference key. - * @return what was found: nothing, the plaintext, or a value that cannot be decrypted here. - */ - fun readAndMigrate(prefs: SharedPreferences?, key: String): Stored { - val stored = prefs?.getString(key, null) ?: return Stored.Absent - if (stored.startsWith(ENC_PREFIX)) { - // A lost Keystore alias — restore onto new hardware, an OEM reset, a re-enrolled screen - // lock — is not the same as an absent token, and must not be reported as one. - return decrypt(stored)?.let(Stored::Value) ?: Stored.Unreadable - } - val plain = stored.trim() - if (plain.isEmpty()) return Stored.Value(plain) - try { - prefs.edit().putString(key, encrypt(plain)).apply() - Log.i(TAG, "Upgraded a legacy plaintext token to ciphertext") - } catch (e: Exception) { - Log.w(TAG, "Could not upgrade a legacy plaintext token to ciphertext", e) - } - return Stored.Value(plain) - } - - private fun getOrCreateKey(): SecretKey { - val store = KeyStore.getInstance(KEYSTORE).apply { load(null) } - (store.getEntry(ALIAS, null) as? KeyStore.SecretKeyEntry)?.let { return it.secretKey } - val generator = KeyGenerator.getInstance(KeyProperties.KEY_ALGORITHM_AES, KEYSTORE) - generator.init( - KeyGenParameterSpec.Builder( - ALIAS, - KeyProperties.PURPOSE_ENCRYPT or KeyProperties.PURPOSE_DECRYPT - ) - .setBlockModes(KeyProperties.BLOCK_MODE_GCM) - .setEncryptionPaddings(KeyProperties.ENCRYPTION_PADDING_NONE) - .build() - ) - return generator.generateKey() - } - - private fun deleteKey() { - try { - KeyStore.getInstance(KEYSTORE).apply { load(null) }.deleteEntry(ALIAS) - } catch (e: Exception) { - Log.w(TAG, "Failed to delete Keystore alias $ALIAS", e) - } - } - - private fun encryptWith(key: SecretKey, plain: String): String { - val cipher = Cipher.getInstance(TRANSFORM) - cipher.init(Cipher.ENCRYPT_MODE, key) - val iv = cipher.iv - // Zeroed straight after the cipher reads it. The String itself cannot be: every API this - // token passes through — SharedPreferences, JSONObject, setRequestProperty — takes one, so - // a CharArray here would only move the immutable copy one frame away. - val plainBytes = plain.toByteArray(Charsets.UTF_8) - val ciphertext = try { - cipher.doFinal(plainBytes) - } finally { - plainBytes.fill(0) - } - val combined = ByteArray(iv.size + ciphertext.size) - System.arraycopy(iv, 0, combined, 0, iv.size) - System.arraycopy(ciphertext, 0, combined, iv.size, ciphertext.size) - return ENC_PREFIX + Base64.encodeToString(combined, Base64.NO_WRAP) - } -} +val secureTokenStore = KeystoreSecretStore(ALIAS) diff --git a/ai-agent-mcp/src/main/kotlin/com/itsaky/androidide/plugins/aiagentmcp/security/UnavailableSecretException.kt b/ai-agent-mcp/src/main/kotlin/com/itsaky/androidide/plugins/aiagentmcp/security/UnavailableSecretException.kt new file mode 100644 index 00000000..cb3355cd --- /dev/null +++ b/ai-agent-mcp/src/main/kotlin/com/itsaky/androidide/plugins/aiagentmcp/security/UnavailableSecretException.kt @@ -0,0 +1,14 @@ +package com.itsaky.androidide.plugins.aiagentmcp.security + +import java.io.IOException + +/** + * A stored credential this device's Keystore would not open just now. + * + * Its own type rather than an [UnreadableSecretException], because the two lead to opposite advice: + * the credential here is very likely intact — the Keystore was not ready, or a binder call failed — + * and the only useful thing to say is to try again, not to enter the credential over. + * + * @param detail what could not be read, for logcat; the user sees the formatted sentence instead. + */ +class UnavailableSecretException(detail: String) : IOException(detail) diff --git a/ai-agent-mcp/src/main/kotlin/com/itsaky/androidide/plugins/aiagentmcp/settings/McpServerStore.kt b/ai-agent-mcp/src/main/kotlin/com/itsaky/androidide/plugins/aiagentmcp/settings/McpServerStore.kt index 75e60950..ff8cfcc8 100644 --- a/ai-agent-mcp/src/main/kotlin/com/itsaky/androidide/plugins/aiagentmcp/settings/McpServerStore.kt +++ b/ai-agent-mcp/src/main/kotlin/com/itsaky/androidide/plugins/aiagentmcp/settings/McpServerStore.kt @@ -4,9 +4,11 @@ import android.content.SharedPreferences import android.util.Log import com.itsaky.androidide.plugins.aiagentmcp.logging.LOG_PREFIX import com.itsaky.androidide.plugins.aiagentmcp.plugin.McpPlugin -import com.itsaky.androidide.plugins.aiagentmcp.security.SecureTokenStore +import com.itsaky.androidide.plugins.aiagentmcp.security.UnavailableSecretException import com.itsaky.androidide.plugins.aiagentmcp.security.UnreadableSecretException +import com.itsaky.androidide.plugins.aiagentmcp.security.secureTokenStore import com.itsaky.androidide.plugins.aiagentmcp.transport.McpHeaders +import com.itsaky.androidide.plugins.security.KeystoreSecretStore import java.util.UUID import java.util.concurrent.CopyOnWriteArrayList import org.json.JSONArray @@ -164,10 +166,11 @@ object McpServerStore { * * @param id the server the token belongs to. * @param token the token, or blank to remove it. - * @return true when it was stored. + * @return true once it is on disk, or removed for a blank one; false when encrypting it or the + * write itself failed, which is what the pane must not report as saved. */ fun setToken(id: String, token: String): Boolean { - val stored = SecureTokenStore.write(prefs(), KEY_TOKEN_PREFIX + id, token) + val stored = secureTokenStore.write(prefs(), KEY_TOKEN_PREFIX + id, token) // Like every other mutator: a new credential has to reach the agent, or it keeps calling // with the old one until something else happens to touch the store. fireChanged() @@ -180,10 +183,11 @@ object McpServerStore { * Keystore work, so call this off the main thread. * * @param id the server. - * @return what is stored: nothing, the token, or a token this device can no longer read. + * @return what is stored: nothing, the token, a token this device can no longer read, or one + * the keystore would not open just now. */ - fun token(id: String): SecureTokenStore.Stored = - SecureTokenStore.readAndMigrate(prefs(), KEY_TOKEN_PREFIX + id) + fun token(id: String): KeystoreSecretStore.Stored = + secureTokenStore.readAndMigrate(prefs(), KEY_TOKEN_PREFIX + id) /** True when a token is stored for [id], without decrypting it. */ fun hasToken(id: String): Boolean = prefs()?.contains(KEY_TOKEN_PREFIX + id) == true @@ -200,13 +204,18 @@ object McpServerStore { * @return the headers in the order they were entered; empty when there are none. */ fun headers(id: String): Map { - val stored = SecureTokenStore.readAndMigrate(prefs(), KEY_HEADERS_PREFIX + id) - if (stored is SecureTokenStore.Stored.Unreadable) { + val stored = secureTokenStore.readAndMigrate(prefs(), KEY_HEADERS_PREFIX + id) + if (stored is KeystoreSecretStore.Stored.Unreadable) { // Same failure as an unreadable token, and reported the same way: sending the request // without them would look like the server refusing a credential that is still correct. throw UnreadableSecretException("The stored headers for '$id' cannot be decrypted.") } - val raw = (stored as? SecureTokenStore.Stored.Value)?.plain ?: return emptyMap() + if (stored is KeystoreSecretStore.Stored.Unavailable) { + // Kept apart from the above: these headers are very likely intact, so the caller is + // told to retry instead of asking the user to enter them again. + throw UnavailableSecretException("The stored headers for '$id' could not be read just now.") + } + val raw = (stored as? KeystoreSecretStore.Stored.Value)?.plain ?: return emptyMap() return try { val json = JSONObject(raw) val parsed = LinkedHashMap() @@ -225,7 +234,8 @@ object McpServerStore { * * @param id the server the headers belong to. * @param headers the headers to store; unusable pairs are dropped. - * @return true when they were stored. + * @return true once they are on disk, or removed for an empty map; false when encrypting them + * or the write itself failed. */ fun setHeaders(id: String, headers: Map): Boolean { val clean = McpHeaders.sanitize(headers) @@ -237,12 +247,12 @@ object McpServerStore { } val json = JSONObject() clean.forEach { (name, value) -> json.put(name, value) } - val stored = SecureTokenStore.write(prefs(), key, json.toString()) + val stored = secureTokenStore.write(prefs(), key, json.toString()) fireChanged() return stored } - /** How many extra headers are configured for [id], without decrypting them. */ + /** Whether any extra header is configured for [id], without decrypting them. */ fun hasHeaders(id: String): Boolean = prefs()?.contains(KEY_HEADERS_PREFIX + id) == true /** diff --git a/ai-agent-mcp/src/main/kotlin/com/itsaky/androidide/plugins/aiagentmcp/settings/McpSettingsFragment.kt b/ai-agent-mcp/src/main/kotlin/com/itsaky/androidide/plugins/aiagentmcp/settings/McpSettingsFragment.kt index 281fe248..af9e1867 100644 --- a/ai-agent-mcp/src/main/kotlin/com/itsaky/androidide/plugins/aiagentmcp/settings/McpSettingsFragment.kt +++ b/ai-agent-mcp/src/main/kotlin/com/itsaky/androidide/plugins/aiagentmcp/settings/McpSettingsFragment.kt @@ -211,6 +211,9 @@ class McpSettingsFragment : Fragment() { if (form.secretsUnreadable) { tokenField.hint = getString(R.string.mcp_hint_token_unreadable) status.text = getString(R.string.mcp_secrets_unreadable) + } else if (form.secretsUnavailable) { + tokenField.hint = getString(R.string.mcp_hint_token_unavailable) + status.text = getString(R.string.mcp_secrets_unavailable) } else if (form.hasToken) { tokenField.hint = getString(R.string.mcp_hint_token_stored) } diff --git a/ai-agent-mcp/src/main/kotlin/com/itsaky/androidide/plugins/aiagentmcp/settings/McpSettingsViewModel.kt b/ai-agent-mcp/src/main/kotlin/com/itsaky/androidide/plugins/aiagentmcp/settings/McpSettingsViewModel.kt index 6b0c41d5..09372cbb 100644 --- a/ai-agent-mcp/src/main/kotlin/com/itsaky/androidide/plugins/aiagentmcp/settings/McpSettingsViewModel.kt +++ b/ai-agent-mcp/src/main/kotlin/com/itsaky/androidide/plugins/aiagentmcp/settings/McpSettingsViewModel.kt @@ -8,9 +8,10 @@ import com.itsaky.androidide.plugins.aiagentmcp.R import com.itsaky.androidide.plugins.aiagentmcp.client.McpConnections import com.itsaky.androidide.plugins.aiagentmcp.client.McpTool import com.itsaky.androidide.plugins.aiagentmcp.errors.McpErrorFormatter -import com.itsaky.androidide.plugins.aiagentmcp.security.SecureTokenStore +import com.itsaky.androidide.plugins.aiagentmcp.security.UnavailableSecretException import com.itsaky.androidide.plugins.aiagentmcp.security.UnreadableSecretException import com.itsaky.androidide.plugins.aiagentmcp.tools.McpToolCatalog +import com.itsaky.androidide.plugins.security.KeystoreSecretStore import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.flow.MutableStateFlow import kotlinx.coroutines.flow.StateFlow @@ -48,12 +49,15 @@ class McpSettingsViewModel( * unreadable" has to count as a stored credential or the control that clears it hides. * @property secretsUnreadable whether a stored token or header cannot be decrypted on this * device, which the field has to say aloud: it looks stored, but nothing can send it. + * @property secretsUnavailable whether the keystore merely would not answer this time, which + * the field says differently: the credential is intact and the read is worth repeating. * @property headers the extra headers configured for the server. */ data class FormState( val hasToken: Boolean, val hasHeaders: Boolean, val secretsUnreadable: Boolean, + val secretsUnavailable: Boolean, val headers: Map, ) @@ -81,16 +85,26 @@ class McpSettingsViewModel( viewModelScope.launch { val state = withContext(Dispatchers.IO) { val token = McpServerStore.token(id) + var headersUnreadable = false + var headersUnavailable = false val headers = try { McpServerStore.headers(id) } catch (e: UnreadableSecretException) { + headersUnreadable = true + null + } catch (e: UnavailableSecretException) { + headersUnavailable = true null } FormState( hasToken = McpServerStore.hasToken(id), hasHeaders = McpServerStore.hasHeaders(id), secretsUnreadable = - token is SecureTokenStore.Stored.Unreadable || headers == null, + token is KeystoreSecretStore.Stored.Unreadable || headersUnreadable, + // Both can be true; the dialog shows the unreadable message first, since a + // credential that has to be entered again is the worse news. + secretsUnavailable = + token is KeystoreSecretStore.Stored.Unavailable || headersUnavailable, headers = headers.orEmpty(), ) } diff --git a/ai-agent-mcp/src/main/res/values/strings.xml b/ai-agent-mcp/src/main/res/values/strings.xml index 39a48b28..e3067541 100644 --- a/ai-agent-mcp/src/main/res/values/strings.xml +++ b/ai-agent-mcp/src/main/res/values/strings.xml @@ -24,6 +24,7 @@ Leave empty if the server needs none Stored — type to replace it Stored but unreadable — type it again + Stored, but unreadable right now — try again Connect Save Cancel @@ -42,6 +43,7 @@ The stored token and headers for this server have been removed. Still reading this server\'s stored credentials — try again in a moment. The saved token or headers for this server can no longer be read on this device. Enter them again. + The saved token or headers for this server could not be read just now. They are still stored — close this and try again in a moment. Connecting… Connected to %1$s. It listed %2$d tools below — new ones start switched off. Connected to %1$s, which offers no tools. @@ -65,6 +67,7 @@ The call to %1$s was cancelled. %1$s redirected the request to another address, so it was not sent — it carries your token. Check the endpoint URL. %1$s\'s saved token can no longer be read on this device. Open MCP server settings and enter it again. + %1$s\'s saved token could not be read just now. It is still stored — try again in a moment. Could not reach %1$s. Could not reach %1$s: %2$s \'%1$s\' is no longer offered by any configured MCP server. diff --git a/ai-agent-mcp/src/test/kotlin/com/itsaky/androidide/plugins/aiagentmcp/client/McpSessionLifecycleTest.kt b/ai-agent-mcp/src/test/kotlin/com/itsaky/androidide/plugins/aiagentmcp/client/McpSessionLifecycleTest.kt index bee9d7f7..01ecb8b7 100644 --- a/ai-agent-mcp/src/test/kotlin/com/itsaky/androidide/plugins/aiagentmcp/client/McpSessionLifecycleTest.kt +++ b/ai-agent-mcp/src/test/kotlin/com/itsaky/androidide/plugins/aiagentmcp/client/McpSessionLifecycleTest.kt @@ -1,14 +1,18 @@ package com.itsaky.androidide.plugins.aiagentmcp.client import com.itsaky.androidide.plugins.aiagentmcp.transport.McpHttpClient +import com.itsaky.androidide.plugins.aiagentmcp.transport.McpHttpException import java.net.HttpURLConnection +import org.json.JSONArray import org.json.JSONObject import org.junit.Assert.assertEquals +import org.junit.Assert.assertNull import org.junit.Assert.assertTrue import org.junit.Test /** - * Whether `notifications/initialized` is sent, which the negotiated revision alone decides. + * What a session does across calls: whether `notifications/initialized` is sent, and whether the + * handshake it paid for is then kept alive rather than repeated or silently lost. * * Reading an absent `Mcp-Session-Id` as "stateless" left a conforming 2025-06-18 server without * the notification, so its next `tools/list` answered "not initialized" and the user saw no tools. @@ -18,15 +22,38 @@ class McpSessionLifecycleTest { private companion object { const val ENDPOINT = "https://example.test/mcp" const val NOTIFICATION = "notifications/initialized" + const val INITIALIZE = "initialize" + const val LIST_TOOLS = "tools/list" + const val SESSION = "s-1" + const val TOOL = "search" } - /** Answers `initialize` with [protocolVersion], recording the methods it was asked for. */ + /** + * Answers `initialize` with [protocolVersion], recording the methods it was asked for. + * + * @param protocolVersion the revision the handshake reports back. + * @param sessionId the session the server assigns, or null for one that keeps no state. + */ private class FakeHttpClient( private val protocolVersion: String, private val sessionId: String? = null, ) : McpHttpClient() { - val methods = mutableListOf() + /** One request the client made, with the session it carried. */ + data class Request(val method: String, val sessionId: String?) + + val requests = mutableListOf() + + /** Sessions the client ended with a DELETE. */ + val deletedSessions = mutableListOf() + + /** The method to answer `404` for, standing in for a session the server forgot. */ + var expiringMethod: String? = null + + /** How many more times [expiringMethod] answers `404` before it starts working. */ + var expiriesLeft = 0 + + val methods: List get() = requests.map { it.method } override fun post( url: String, @@ -38,15 +65,34 @@ class McpSessionLifecycleTest { onConnected: (HttpURLConnection) -> Unit, ): Response { val method = body.optString("method") - methods += method - if (method != "initialize") return Response(null, this.sessionId) - val result = JSONObject().put("protocolVersion", this.protocolVersion) + requests += Request(method, sessionId) + if (method == expiringMethod && expiriesLeft > 0) { + expiriesLeft-- + throw McpHttpException(HttpURLConnection.HTTP_NOT_FOUND, "session expired") + } + val result = when (method) { + INITIALIZE -> JSONObject().put("protocolVersion", this.protocolVersion) + LIST_TOOLS -> JSONObject().put( + "tools", + JSONArray().put(JSONObject().put("name", TOOL)) + ) + else -> return Response(null, this.sessionId) + } val document = JSONObject() .put("jsonrpc", "2.0") .put("id", body.opt("id")) .put("result", result) return Response(document.toString(), this.sessionId) } + + override fun deleteSession( + url: String, + token: String, + sessionId: String, + extraHeaders: Map, + ) { + deletedSessions += sessionId + } } private fun sessionOn(http: McpHttpClient) = @@ -58,16 +104,16 @@ class McpSessionLifecycleTest { sessionOn(http).initialize() - assertEquals(listOf("initialize", NOTIFICATION), http.methods) + assertEquals(listOf(INITIALIZE, NOTIFICATION), http.methods) } @Test fun givenAStatefulRevisionAndASessionHeader_whenInitializing_thenTheNotificationIsSent() { - val http = FakeHttpClient(McpSession.PREFERRED_PROTOCOL_VERSION, sessionId = "s-1") + val http = FakeHttpClient(McpSession.PREFERRED_PROTOCOL_VERSION, sessionId = SESSION) sessionOn(http).initialize() - assertEquals(listOf("initialize", NOTIFICATION), http.methods) + assertEquals(listOf(INITIALIZE, NOTIFICATION), http.methods) } @Test @@ -76,7 +122,7 @@ class McpSessionLifecycleTest { sessionOn(http).initialize() - assertEquals(listOf("initialize"), http.methods) + assertEquals(listOf(INITIALIZE), http.methods) } @Test @@ -87,4 +133,60 @@ class McpSessionLifecycleTest { assertTrue("an unparseable revision must fail safe", NOTIFICATION in http.methods) } + + @Test + fun givenAnInitializedSession_whenMoreCallsFollow_thenTheHandshakeIsNotRepeated() { + val http = FakeHttpClient(McpSession.PREFERRED_PROTOCOL_VERSION, sessionId = SESSION) + val session = sessionOn(http) + + session.initialize() + session.listTools() + session.listTools() + + // The handshake is what a kept-alive session buys; paying it per call is the regression. + assertEquals(1, http.methods.count { it == INITIALIZE }) + assertEquals(2, http.methods.count { it == LIST_TOOLS }) + } + + @Test + fun givenAServerAssignedSession_whenACallFollows_thenItCarriesTheSessionHeader() { + val http = FakeHttpClient(McpSession.PREFERRED_PROTOCOL_VERSION, sessionId = SESSION) + + sessionOn(http).listTools() + + assertNull("the handshake itself has no session yet", http.requests.first().sessionId) + assertTrue( + "every later request must carry the assigned session", + http.requests.drop(1).all { it.sessionId == SESSION } + ) + } + + @Test + fun givenAnExpiredSession_whenTheServerAnswers404_thenItReInitializesAndRetriesOnce() { + val http = FakeHttpClient(McpSession.PREFERRED_PROTOCOL_VERSION, sessionId = SESSION).apply { + expiringMethod = LIST_TOOLS + expiriesLeft = 1 + } + + val tools = sessionOn(http).listTools() + + assertEquals( + listOf(INITIALIZE, NOTIFICATION, LIST_TOOLS, INITIALIZE, NOTIFICATION, LIST_TOOLS), + http.methods + ) + assertEquals(listOf(TOOL), tools.map { it.name }) + } + + @Test + fun givenAClosedSession_whenItIsUsedAgain_thenTheServerSessionIsEndedAndTheHandshakeRepeats() { + val http = FakeHttpClient(McpSession.PREFERRED_PROTOCOL_VERSION, sessionId = SESSION) + val session = sessionOn(http) + + session.listTools() + session.close() + session.listTools() + + assertEquals(listOf(SESSION), http.deletedSessions) + assertEquals(2, http.methods.count { it == INITIALIZE }) + } } diff --git a/ai-agent-mcp/src/test/kotlin/com/itsaky/androidide/plugins/aiagentmcp/plugin/McpPluginScopeTest.kt b/ai-agent-mcp/src/test/kotlin/com/itsaky/androidide/plugins/aiagentmcp/plugin/McpPluginScopeTest.kt new file mode 100644 index 00000000..22aa9231 --- /dev/null +++ b/ai-agent-mcp/src/test/kotlin/com/itsaky/androidide/plugins/aiagentmcp/plugin/McpPluginScopeTest.kt @@ -0,0 +1,93 @@ +package com.itsaky.androidide.plugins.aiagentmcp.plugin + +import com.itsaky.androidide.plugins.PluginContext +import com.itsaky.androidide.plugins.aiagentmcp.testing.FakeSharedPreferences +import io.mockk.every +import io.mockk.mockk +import kotlinx.coroutines.Job +import org.junit.After +import org.junit.Assert.assertFalse +import org.junit.Assert.assertNotSame +import org.junit.Assert.assertTrue +import org.junit.Before +import org.junit.Test + +/** + * Whether [McpPlugin] leaves a coroutine scope running past the lifecycle edge that ended it. + * + * The scope carries the cold-start `tools/list` refresh, which registers sessions in + * `McpConnections` and fills `McpToolCatalog`. Left running past `deactivate`, it repopulates both + * straight after they were cleared, leaving sockets nothing can reach — and a host that activates + * twice would orphan the first scope entirely. + */ +class McpPluginScopeTest { + + private val prefs = FakeSharedPreferences() + private lateinit var context: PluginContext + private lateinit var plugin: McpPlugin + + @Before + fun setUp() { + context = mockk(relaxed = true) + every { context.getPluginSharedPreferences(any()) } returns prefs + plugin = McpPlugin() + plugin.initialize(context) + } + + @After + fun tearDown() { + plugin.dispose() + } + + @Test + fun givenAnActivatedPlugin_whenActivatedAgain_thenThePreviousScopeIsCancelled() { + plugin.activate() + val first = requireNotNull(plugin.scopeJob) + + plugin.activate() + + val second = requireNotNull(plugin.scopeJob) + assertTrue("the orphaned scope must not outlive the activation", first.isCancelled) + assertNotSame("a second activation gets a scope of its own", first, second) + assertScopeUsable(second) + } + + @Test + fun givenAnActivatedPlugin_whenDeactivated_thenTheScopeIsCancelled() { + plugin.activate() + val job = requireNotNull(plugin.scopeJob) + + plugin.deactivate() + + assertTrue("an in-flight refresh must not survive deactivation", job.isCancelled) + } + + @Test + fun givenAnActivatedPlugin_whenDisposed_thenTheScopeIsCancelled() { + plugin.activate() + val job = requireNotNull(plugin.scopeJob) + + plugin.dispose() + + assertTrue("dispose must leave nothing running", job.isCancelled) + } + + @Test + fun givenADeactivatedPlugin_whenActivatedAgain_thenItGetsALiveScope() { + plugin.activate() + plugin.deactivate() + + plugin.activate() + + assertScopeUsable(requireNotNull(plugin.scopeJob)) + } + + /** + * Asserts a scope can still take work, which a cancelled one cannot. + * @param job the activation scope's job. + */ + private fun assertScopeUsable(job: Job) { + assertFalse("the current activation's scope must be live", job.isCancelled) + assertTrue("the current activation's scope must accept work", job.isActive) + } +} diff --git a/ai-agent-mcp/src/test/kotlin/com/itsaky/androidide/plugins/aiagentmcp/settings/McpServerStoreLockTest.kt b/ai-agent-mcp/src/test/kotlin/com/itsaky/androidide/plugins/aiagentmcp/settings/McpServerStoreLockTest.kt new file mode 100644 index 00000000..8e7532f2 --- /dev/null +++ b/ai-agent-mcp/src/test/kotlin/com/itsaky/androidide/plugins/aiagentmcp/settings/McpServerStoreLockTest.kt @@ -0,0 +1,124 @@ +package com.itsaky.androidide.plugins.aiagentmcp.settings + +import com.itsaky.androidide.plugins.PluginContext +import com.itsaky.androidide.plugins.aiagentmcp.plugin.McpPlugin +import com.itsaky.androidide.plugins.aiagentmcp.testing.FakeSharedPreferences +import io.mockk.every +import io.mockk.mockk +import java.util.concurrent.CountDownLatch +import java.util.concurrent.TimeUnit +import org.junit.After +import org.junit.Assert.assertEquals +import org.junit.Assert.assertFalse +import org.junit.Assert.assertTrue +import org.junit.Before +import org.junit.Test + +/** + * Whether [McpServerStore]'s lock actually serialises the read-modify-write of the server list. + * + * The list is one JSON blob, so every mutator reads it whole and puts it back whole. A refresh on + * `McpPlugin`'s scope and a toggle on the screen's dispatcher do interleave in practice, and the + * loser's write simply vanishes — the switch stays on while `enabledTools` on disk no longer holds + * it, which reads to the user as the Agent ignoring a tool they enabled. + */ +class McpServerStoreLockTest { + + private companion object { + /** Enough concurrent writers to lose one, few enough to stay fast. */ + const val WRITERS = 8 + + /** Widens the read-modify-write window; see [FakeSharedPreferences.readDelayMillis]. */ + const val READ_DELAY_MS = 2L + + const val JOIN_TIMEOUT_SECONDS = 30L + } + + private val prefs = FakeSharedPreferences() + private lateinit var plugin: McpPlugin + private lateinit var serverId: String + + @Before + fun setUp() { + // Through the plugin's own lifecycle rather than a hook on the store: `initialize` is how + // the host hands over the preferences [McpServerStore] then reads, so the test drives the + // same path the device does and the store keeps no test-only surface. + val context = mockk(relaxed = true) + every { context.getPluginSharedPreferences(any()) } returns prefs + plugin = McpPlugin() + plugin.initialize(context) + serverId = McpServerStore.saveDetails( + McpServerStore.newServer("Docs", "https://example.test/mcp") + ).id + } + + @After + fun tearDown() { + plugin.dispose() + } + + @Test + fun givenConcurrentToolToggles_whenTheyInterleave_thenNoWriteIsLost() { + val tools = (1..WRITERS).map { "tool_$it" } + McpServerStore.setKnownTools(serverId, tools) + prefs.readDelayMillis = READ_DELAY_MS + + runTogether(tools) { McpServerStore.setToolEnabled(serverId, it, true) } + + assertEquals(tools.toSet(), McpServerStore.server(serverId)?.enabledTools) + } + + @Test + fun givenAToggleAndAWholeServerSwitchAtOnce_whenTheyInterleave_thenNeitherIsLost() { + val tools = (1..WRITERS).map { "tool_$it" } + McpServerStore.setKnownTools(serverId, tools) + prefs.readDelayMillis = READ_DELAY_MS + + runTogether(tools + "disable") { work -> + if (work == "disable") { + McpServerStore.setEnabled(serverId, false) + } else { + McpServerStore.setToolEnabled(serverId, work, true) + } + } + + val stored = McpServerStore.server(serverId) + assertEquals(tools.toSet(), stored?.enabledTools) + assertFalse("the whole-server switch must survive the toggles", stored?.enabled ?: true) + } + + /** + * Runs [work] on one thread per item, all released at once. + * + * @param items one item per thread. + * @param work what each thread does with its item. + */ + private fun runTogether(items: List, work: (String) -> Unit) { + val start = CountDownLatch(1) + val done = CountDownLatch(items.size) + val threads = items.map { item -> + // Daemons, and joined in a `finally` below: a writer wedged on the store's lock must not + // outlive the assertion that noticed it and hold the Gradle test worker's JVM open. + Thread { + start.await() + try { + work(item) + } finally { + done.countDown() + } + }.apply { + isDaemon = true + start() + } + } + try { + start.countDown() + assertTrue( + "the writers did not finish", + done.await(JOIN_TIMEOUT_SECONDS, TimeUnit.SECONDS) + ) + } finally { + threads.forEach { it.join(TimeUnit.SECONDS.toMillis(JOIN_TIMEOUT_SECONDS)) } + } + } +} diff --git a/ai-agent-mcp/src/test/kotlin/com/itsaky/androidide/plugins/aiagentmcp/testing/FakeSharedPreferences.kt b/ai-agent-mcp/src/test/kotlin/com/itsaky/androidide/plugins/aiagentmcp/testing/FakeSharedPreferences.kt new file mode 100644 index 00000000..f8660738 --- /dev/null +++ b/ai-agent-mcp/src/test/kotlin/com/itsaky/androidide/plugins/aiagentmcp/testing/FakeSharedPreferences.kt @@ -0,0 +1,98 @@ +package com.itsaky.androidide.plugins.aiagentmcp.testing + +import android.content.SharedPreferences +import java.util.concurrent.ConcurrentHashMap + +/** + * An in-memory [SharedPreferences], so the settings store can be exercised off a device. + * + * The seam this plugs into exists for one reason — showing that `McpServerStore`'s lock actually + * serialises a read-modify-write — so [readDelayMillis] is here too: a real preferences read costs + * a lock and a file, and a race that needs microseconds of window is not a race a test would ever + * catch on an in-memory map. + */ +class FakeSharedPreferences : SharedPreferences { + + private val values = ConcurrentHashMap() + + /** How long a read blocks, widening the window a missing lock would lose a write in. */ + @Volatile + var readDelayMillis: Long = 0 + + override fun getAll(): MutableMap = HashMap(values) + + override fun getString(key: String, defValue: String?): String? { + if (readDelayMillis > 0) Thread.sleep(readDelayMillis) + return values[key] ?: defValue + } + + override fun getStringSet(key: String, defValues: MutableSet?): MutableSet? = + defValues + + override fun getInt(key: String, defValue: Int): Int = defValue + + override fun getLong(key: String, defValue: Long): Long = defValue + + override fun getFloat(key: String, defValue: Float): Float = defValue + + override fun getBoolean(key: String, defValue: Boolean): Boolean = defValue + + override fun contains(key: String): Boolean = values.containsKey(key) + + override fun edit(): SharedPreferences.Editor = FakeEditor() + + override fun registerOnSharedPreferenceChangeListener( + listener: SharedPreferences.OnSharedPreferenceChangeListener? + ) = Unit + + override fun unregisterOnSharedPreferenceChangeListener( + listener: SharedPreferences.OnSharedPreferenceChangeListener? + ) = Unit + + /** Batches edits and applies them at once, as the real editor does. */ + private inner class FakeEditor : SharedPreferences.Editor { + + private val puts = LinkedHashMap() + private val removals = LinkedHashSet() + private var cleared = false + + override fun putString(key: String, value: String?): SharedPreferences.Editor { + if (value == null) removals += key else puts[key] = value + return this + } + + override fun putStringSet( + key: String, + values: MutableSet? + ): SharedPreferences.Editor = this + + override fun putInt(key: String, value: Int): SharedPreferences.Editor = this + + override fun putLong(key: String, value: Long): SharedPreferences.Editor = this + + override fun putFloat(key: String, value: Float): SharedPreferences.Editor = this + + override fun putBoolean(key: String, value: Boolean): SharedPreferences.Editor = this + + override fun remove(key: String): SharedPreferences.Editor { + removals += key + return this + } + + override fun clear(): SharedPreferences.Editor { + cleared = true + return this + } + + override fun commit(): Boolean { + if (cleared) values.clear() + removals.forEach { values.remove(it) } + values.putAll(puts) + return true + } + + override fun apply() { + commit() + } + } +} diff --git a/ai-agent-openai/README.md b/ai-agent-openai/README.md index 86476f22..182ed5ca 100644 --- a/ai-agent-openai/README.md +++ b/ai-agent-openai/README.md @@ -97,14 +97,15 @@ after configuring OpenAI cannot put that bearer token on the network in the clea A key stored before the origin was recorded is still sent, since it cannot be shown to belong elsewhere. The connection test applies the same rule. -`security/SecureApiKeyStore.kt` is this plugin's **own copy**, under its own -Keystore alias (`cotg_ai_openai_key_v1`). It is deliberately not shared with -ai-agent-gemini's copy: every plugin runs in the host app's process and UID and -therefore shares one Keystore, so a shared alias would let one plugin's +`security/SecureApiKeyStore.kt` holds only this plugin's Keystore alias +(`cotg_ai_openai_key_v1`); the AES/GCM itself is the IDE's `KeystoreSecretStore` +(`plugin-api`, since **26.35** — hence this plugin's `min_ide_version`), so there +is one implementation in the process rather than a copy per plugin. The **alias** +is deliberately not shared: every plugin runs in the host app's process and UID +and therefore shares one Keystore, so a shared alias would let one plugin's invalidated-key recovery (`deleteEntry`) destroy the other backend's stored key. -The two never read each other's ciphertext, so they have no reason to share an -alias — and there is therefore nothing to keep in parity. Extracting the shared -*source* is tracked separately. +The plugins never read each other's ciphertext, so they have no reason to share +one. ## Installation @@ -135,7 +136,7 @@ root of `com/itsaky/androidide/plugins/aiagentopenai/`. - `backend/SseChunk.kt` — one line of the token stream (pure) - `backend/ChatModelFilter.kt` — keeps non-chat models out of the picker (pure) - `errors/OpenAiErrorFormatter.kt` — turns a failure into one translated sentence -- `security/SecureApiKeyStore.kt` — AES/GCM at rest +- `security/SecureApiKeyStore.kt` — this plugin's Keystore alias, over the IDE's `KeystoreSecretStore` - `preferences/OpenAiPreferences.kt` — this plugin's settings store - `prompt/OpenAiSystemPrompt.kt` — the system prompt this cloud model is given - `settings/BaseUrlPolicy.kt` — URL normalization and the cleartext rule (pure) diff --git a/ai-agent-openai/ai-agent-openai.html b/ai-agent-openai/ai-agent-openai.html index 4508ae41..8403d0f9 100644 --- a/ai-agent-openai/ai-agent-openai.html +++ b/ai-agent-openai/ai-agent-openai.html @@ -118,8 +118,9 @@

Technical architecture

OpenAiErrorFormatterClassifies a failure (unknown model, rate limit, spent balance, refused key, outage, server not running) so it can be reported as one translated sentence. - SecureApiKeyStoreAES/GCM encryption of the API - key, under this plugin's own Keystore alias. + SecureApiKeyStoreBinds this plugin's own + Keystore alias to the IDE's KeystoreSecretStore, which + AES/GCM-encrypts the API key.

No third-party HTTP SDK. Plugins run in the host IDE's classloader where okhttp3 resolves to the host's older OkHttp — a mismatch that diff --git a/ai-agent-openai/src/main/AndroidManifest.xml b/ai-agent-openai/src/main/AndroidManifest.xml index aefa3899..dd3d2814 100644 --- a/ai-agent-openai/src/main/AndroidManifest.xml +++ b/ai-agent-openai/src/main/AndroidManifest.xml @@ -38,7 +38,7 @@ carries it once that host version is known. --> + android:value="26.35" /> stored.plain.trim().takeIf { it.isNotBlank() } + KeystoreSecretStore.Stored.Absent -> null + // Reported here rather than passed on as "no key": generation fails either way, but a + // lost Keystore entry needs the key entering again, and the log is all that says so. + KeystoreSecretStore.Stored.Unreadable -> { + logger.warn( + "ApiKeyCache: the saved API key cannot be decrypted on this device; " + + "it has to be entered again in settings" + ) + null + } + // Transient, so it returns without caching: the key is very likely intact, and caching + // this answer would freeze "no key" until the stored value itself changed. + KeystoreSecretStore.Stored.Unavailable -> { + logger.warn( + "ApiKeyCache: the keystore could not be reached to read the saved API key; " + + "retrying on the next read" + ) + return null + } + } val raw = prefs?.getString(prefKey, null) cached = raw?.let { it to plain } return plain diff --git a/ai-agent-openai/src/main/kotlin/com/itsaky/androidide/plugins/aiagentopenai/security/SecureApiKeyStore.kt b/ai-agent-openai/src/main/kotlin/com/itsaky/androidide/plugins/aiagentopenai/security/SecureApiKeyStore.kt index 5f383bd4..e6ae6667 100644 --- a/ai-agent-openai/src/main/kotlin/com/itsaky/androidide/plugins/aiagentopenai/security/SecureApiKeyStore.kt +++ b/ai-agent-openai/src/main/kotlin/com/itsaky/androidide/plugins/aiagentopenai/security/SecureApiKeyStore.kt @@ -1,143 +1,17 @@ package com.itsaky.androidide.plugins.aiagentopenai.security -import android.content.SharedPreferences -import android.security.keystore.KeyGenParameterSpec -import android.security.keystore.KeyPermanentlyInvalidatedException -import android.security.keystore.KeyProperties -import android.util.Base64 -import android.util.Log -import com.itsaky.androidide.plugins.aiagentopenai.logging.LOG_PREFIX -import java.security.GeneralSecurityException -import java.security.KeyStore -import javax.crypto.Cipher -import javax.crypto.KeyGenerator -import javax.crypto.SecretKey -import javax.crypto.spec.GCMParameterSpec +import com.itsaky.androidide.plugins.security.KeystoreSecretStore + +/** Unique to this plugin and fixed across releases; see [KeystoreSecretStore] for why both matter. */ +private const val ALIAS = "cotg_ai_openai_key_v1" /** - * AES/GCM encryption for this plugin's API key, keyed by a hardware-backed Android Keystore secret. - * Only ciphertext is written to SharedPreferences, so a copied prefs file (root, `adb backup`, - * forensic dump) is useless without this device's Keystore. - * - * The [ALIAS] must stay stable across releases: a key encrypted under one alias cannot be read - * under another, so changing it silently invalidates every stored key. + * This plugin's binding of [KeystoreSecretStore]: its API key, encrypted under this plugin's own + * Keystore alias. * - * It is also deliberately **this plugin's own** alias, not the one ai-agent-gemini uses. Every - * plugin runs in the host app's process and UID and therefore shares one Keystore, so a shared - * alias would let [deleteKey] — the recovery path for an invalidated key — destroy the other - * backend's stored key as a side effect. The two plugins never read each other's ciphertext, so - * they have no reason to share. + * The store is the IDE's, from plugin-api, and callers use it directly. A forwarding object per + * method would only be a second copy of its contract to keep in step — and one that had to pick a + * single answer for "absent" and "no longer decryptable", which callers here do not share. The + * thing this file owns is the alias. */ -object SecureApiKeyStore { - private const val TAG = "$LOG_PREFIX.SecureApiKeyStore" - private const val KEYSTORE = "AndroidKeyStore" - private const val ALIAS = "cotg_ai_openai_key_v1" - private const val TRANSFORM = "AES/GCM/NoPadding" - private const val IV_LEN = 12 - private const val TAG_BITS = 128 - - /** Marks a stored value as ciphertext; anything without it is treated as legacy plaintext. */ - const val ENC_PREFIX = "enc:v1:" - - private fun getOrCreateKey(): SecretKey { - val ks = KeyStore.getInstance(KEYSTORE).apply { load(null) } - (ks.getEntry(ALIAS, null) as? KeyStore.SecretKeyEntry)?.let { return it.secretKey } - val generator = KeyGenerator.getInstance(KeyProperties.KEY_ALGORITHM_AES, KEYSTORE) - generator.init( - KeyGenParameterSpec.Builder( - ALIAS, - KeyProperties.PURPOSE_ENCRYPT or KeyProperties.PURPOSE_DECRYPT - ) - .setBlockModes(KeyProperties.BLOCK_MODE_GCM) - .setEncryptionPaddings(KeyProperties.ENCRYPTION_PADDING_NONE) - .build() - ) - return generator.generateKey() - } - - private fun deleteKey() { - try { - KeyStore.getInstance(KEYSTORE).apply { load(null) }.deleteEntry(ALIAS) - } catch (e: Exception) { - Log.w(TAG, "Failed to delete Keystore alias $ALIAS", e) - } - } - - private fun encryptWith(key: SecretKey, plain: String): String { - val cipher = Cipher.getInstance(TRANSFORM) - cipher.init(Cipher.ENCRYPT_MODE, key) - val iv = cipher.iv - val ciphertext = cipher.doFinal(plain.toByteArray(Charsets.UTF_8)) - val combined = ByteArray(iv.size + ciphertext.size) - System.arraycopy(iv, 0, combined, 0, iv.size) - System.arraycopy(ciphertext, 0, combined, iv.size, ciphertext.size) - return ENC_PREFIX + Base64.encodeToString(combined, Base64.NO_WRAP) - } - - /** - * Encrypt [plain] into a self-describing string: [ENC_PREFIX] + base64(iv | ciphertext). - * - * The key is not auth-bound, so a credential change does not invalidate it; an alias an - * OEM Keystore drops anyway is regenerated once before retrying. - * - * @param plain the value to encrypt - * @throws GeneralSecurityException on any other Keystore/cipher failure, so the caller can - * inform the user instead of crashing the IDE on Save - */ - @Throws(GeneralSecurityException::class) - fun encrypt(plain: String): String { - return try { - encryptWith(getOrCreateKey(), plain) - } catch (e: KeyPermanentlyInvalidatedException) { - Log.w(TAG, "Keystore key invalidated; regenerating and retrying encrypt", e) - deleteKey() - encryptWith(getOrCreateKey(), plain) - } - } - - /** - * Return the plaintext for a stored value, handling both formats transparently: - * an [ENC_PREFIX] value is decrypted; anything else is returned unchanged as - * legacy plaintext (use [readAndMigrate] to upgrade it in place). Returns - * null if a ciphertext value can't be decrypted — e.g. the Keystore key was - * lost or invalidated — in which case the user must re-enter the key. - */ - fun decrypt(stored: String?): String? { - if (stored == null) return null - if (!stored.startsWith(ENC_PREFIX)) return stored - return try { - val combined = Base64.decode(stored.removePrefix(ENC_PREFIX), Base64.NO_WRAP) - val iv = combined.copyOfRange(0, IV_LEN) - val ciphertext = combined.copyOfRange(IV_LEN, combined.size) - val cipher = Cipher.getInstance(TRANSFORM) - cipher.init(Cipher.DECRYPT_MODE, getOrCreateKey(), GCMParameterSpec(TAG_BITS, iv)) - String(cipher.doFinal(ciphertext), Charsets.UTF_8) - } catch (e: Exception) { - Log.w(TAG, "Failed to decrypt stored API key", e) - null - } - } - - /** - * Read [key] from [prefs], upgrading a legacy plaintext value to ciphertext in place. - * - * The value is trimmed on migration, so the stored, displayed and sent forms all agree. - * - * Keystore IPC + AES/GCM, so call this off the main thread. - * - * @return the trimmed plaintext value, or null when nothing is stored or decryption failed. - */ - fun readAndMigrate(prefs: SharedPreferences?, key: String): String? { - val stored = prefs?.getString(key, null) ?: return null - if (stored.startsWith(ENC_PREFIX)) return decrypt(stored) - val plain = stored.trim() - if (plain.isEmpty()) return plain - try { - prefs.edit().putString(key, encrypt(plain)).apply() - Log.i(TAG, "Upgraded legacy plaintext value for '$key' to ciphertext") - } catch (e: Exception) { - Log.w(TAG, "Could not upgrade legacy plaintext value for '$key' to ciphertext", e) - } - return plain - } -} +val secureApiKeyStore = KeystoreSecretStore(ALIAS) diff --git a/ai-agent-openai/src/main/kotlin/com/itsaky/androidide/plugins/aiagentopenai/settings/OpenAiSettingsFragment.kt b/ai-agent-openai/src/main/kotlin/com/itsaky/androidide/plugins/aiagentopenai/settings/OpenAiSettingsFragment.kt index fab7f2f6..0fba45d2 100644 --- a/ai-agent-openai/src/main/kotlin/com/itsaky/androidide/plugins/aiagentopenai/settings/OpenAiSettingsFragment.kt +++ b/ai-agent-openai/src/main/kotlin/com/itsaky/androidide/plugins/aiagentopenai/settings/OpenAiSettingsFragment.kt @@ -33,6 +33,7 @@ import com.itsaky.androidide.plugins.PluginContext import com.itsaky.androidide.plugins.aiagentopenai.R import com.itsaky.androidide.plugins.aiagentopenai.plugin.OpenAiPlugin import com.itsaky.androidide.plugins.base.PluginFragmentHelper +import com.itsaky.androidide.plugins.security.KeystoreSecretStore import com.itsaky.androidide.plugins.services.IdeTooltipService import kotlinx.coroutines.launch import java.text.SimpleDateFormat @@ -347,20 +348,30 @@ class OpenAiSettingsFragment : Fragment() { } viewLifecycleOwner.lifecycleScope.launch { - val savedApiKey = viewModel.getApiKey() + val stored = viewModel.getApiKey() + val savedApiKey = (stored as? KeystoreSecretStore.Stored.Value)?.plain val hasKey = !savedApiKey.isNullOrBlank() updateUiState(isEditing = !hasKey) if (hasKey) { statusTextView.text = savedApiKeyStatusText() } else { apiKeyInput.setText("") - // A stored-but-undecryptable key also reads as null; warn as the Edit path does. - if (viewModel.hasStoredApiKey()) { + // Only for a key that is there and will not decrypt; an empty box alone looks like + // data loss. Nothing stored at all is the ordinary first run and says nothing. + if (stored is KeystoreSecretStore.Stored.Unreadable) { Toast.makeText( requireContext(), getString(R.string.msg_api_key_unreadable), Toast.LENGTH_LONG ).show() + } else if (stored is KeystoreSecretStore.Stored.Unavailable) { + // Said differently from the above: the key is still there and intact, so this + // must not send the user off to find and type it again. + Toast.makeText( + requireContext(), + getString(R.string.msg_api_key_unavailable), + Toast.LENGTH_LONG + ).show() } } } @@ -566,20 +577,30 @@ class OpenAiSettingsFragment : Fragment() { editButton.setOnClickListener { editButton.isEnabled = false viewLifecycleOwner.lifecycleScope.launch { - val apiKey = try { + val stored = try { viewModel.getApiKey() } finally { editButton.isEnabled = true } - // null = a key IS stored but won't decrypt; an empty box alone looks like data loss. - if (apiKey == null) { + // A key that is stored and will not decrypt; an empty box alone looks like data + // loss. Told apart from "nothing stored" here, which this button rarely sees but + // must not report as a lost Keystore entry when it does. + if (stored is KeystoreSecretStore.Stored.Unreadable) { Toast.makeText( requireContext(), getString(R.string.msg_api_key_unreadable), Toast.LENGTH_LONG ).show() + } else if (stored is KeystoreSecretStore.Stored.Unavailable) { + // Said differently from the above: the key is still there and intact, so this + // must not send the user off to find and type it again. + Toast.makeText( + requireContext(), + getString(R.string.msg_api_key_unavailable), + Toast.LENGTH_LONG + ).show() } - revealEditMode(apiKey.orEmpty()) + revealEditMode((stored as? KeystoreSecretStore.Stored.Value)?.plain.orEmpty()) } } diff --git a/ai-agent-openai/src/main/kotlin/com/itsaky/androidide/plugins/aiagentopenai/settings/OpenAiSettingsViewModel.kt b/ai-agent-openai/src/main/kotlin/com/itsaky/androidide/plugins/aiagentopenai/settings/OpenAiSettingsViewModel.kt index 22a810f8..85e4596b 100644 --- a/ai-agent-openai/src/main/kotlin/com/itsaky/androidide/plugins/aiagentopenai/settings/OpenAiSettingsViewModel.kt +++ b/ai-agent-openai/src/main/kotlin/com/itsaky/androidide/plugins/aiagentopenai/settings/OpenAiSettingsViewModel.kt @@ -10,7 +10,8 @@ import com.itsaky.androidide.plugins.PluginLogger import com.itsaky.androidide.plugins.aiagentopenai.backend.OpenAiBackend import com.itsaky.androidide.plugins.aiagentopenai.logging.LOG_PREFIX import com.itsaky.androidide.plugins.aiagentopenai.preferences.OpenAiPreferences -import com.itsaky.androidide.plugins.aiagentopenai.security.SecureApiKeyStore +import com.itsaky.androidide.plugins.aiagentopenai.security.secureApiKeyStore +import com.itsaky.androidide.plugins.security.KeystoreSecretStore import kotlinx.coroutines.CancellationException import kotlinx.coroutines.CoroutineDispatcher import kotlinx.coroutines.Dispatchers @@ -244,7 +245,7 @@ class OpenAiSettingsViewModel( } /** - * Encrypts [apiKey] via [SecureApiKeyStore] and persists only the ciphertext, off the main + * Encrypts [apiKey] via [secureApiKeyStore] and persists only the ciphertext, off the main * thread. Nothing is written on failure. * * @param apiKey the plaintext key to store (trimmed before encryption) @@ -261,7 +262,7 @@ class OpenAiSettingsViewModel( return@withContext false } val encrypted = try { - SecureApiKeyStore.encrypt(apiKey.trim()) + secureApiKeyStore.encrypt(apiKey.trim()) } catch (e: Exception) { logger?.error("$TAG: failed to encrypt API key", e) return@withContext false @@ -288,9 +289,13 @@ class OpenAiSettingsViewModel( /** * Decrypt the stored key off the main thread (Keystore IPC + AES/GCM), upgrading a plaintext * value to ciphertext in passing. + * + * @return what is on disk: nothing, the key, a key this device's Keystore can no longer open, + * or one it would not open just now. Those are not the same — a lost Keystore entry has to be + * entered again, a keystore that did not answer only retried — so the caller says which. */ - suspend fun getApiKey(): String? = withContext(ioDispatcher) { - SecureApiKeyStore.readAndMigrate(prefs(), OpenAiPreferences.KEY_API_KEY) + suspend fun getApiKey(): KeystoreSecretStore.Stored = withContext(ioDispatcher) { + secureApiKeyStore.readAndMigrate(prefs(), OpenAiPreferences.KEY_API_KEY) } /** @@ -300,18 +305,20 @@ class OpenAiSettingsViewModel( * entered for OpenAI. A key stored before the origin was recorded is returned, matching the * backend's own rule. * - * @return the plaintext key, or null when none is stored or it belongs to another server + * @return the plaintext key, or null when none is stored, it cannot be decrypted here, or it + * belongs to another server. The connection test has the same answer — send no key — for + * every one of them, and the pane has already said so on the read that opened it. */ suspend fun getApiKeyFor(baseUrl: String): String? { val savedFor = prefs()?.getString(OpenAiPreferences.KEY_API_KEY_URL, null) if (savedFor != null && !BaseUrlPolicy.sameOrigin(savedFor, baseUrl)) return null - return getApiKey() + return (getApiKey() as? KeystoreSecretStore.Stored.Value)?.plain } /** - * True when a key is present on disk, whether or not it can still be decrypted. Lets the UI - * tell "nothing was saved" from "the Keystore entry is gone" — [getApiKey] is null for both. - * Raw pref only, so no Keystore IPC and safe on the main thread. + * True when a key is present on disk, whether or not it can still be decrypted: what the key + * block is dressed from, which must not collapse the moment a Keystore entry is lost. Raw pref + * only, so no Keystore IPC and safe on the main thread — which [getApiKey] is not. */ fun hasStoredApiKey(): Boolean = !prefs()?.getString(OpenAiPreferences.KEY_API_KEY, null).isNullOrBlank() diff --git a/ai-agent-openai/src/main/res/values/strings.xml b/ai-agent-openai/src/main/res/values/strings.xml index f13a8291..8d954ac4 100644 --- a/ai-agent-openai/src/main/res/values/strings.xml +++ b/ai-agent-openai/src/main/res/values/strings.xml @@ -69,6 +69,7 @@ API Key saved on: %s API Key saved and verified on: %s The stored API key could not be read on this device. Please enter it again. + The device keystore could not be reached, so the stored API key could not be read. It is still saved — please try again in a moment. Couldn\'t save the API key on this device. Please try again. Checking this key with the server… Verified, your API key works diff --git a/ai-core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/viewmodel/ChatViewModel.kt b/ai-core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/viewmodel/ChatViewModel.kt index eb63fa7e..59d6bcf3 100644 --- a/ai-core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/viewmodel/ChatViewModel.kt +++ b/ai-core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/viewmodel/ChatViewModel.kt @@ -25,6 +25,7 @@ import com.itsaky.androidide.plugins.aicore.tool.ToolCall import com.itsaky.androidide.plugins.aicore.tool.ToolCallExtractor import com.itsaky.androidide.plugins.aicore.tool.ToolExecutionTracker import com.itsaky.androidide.plugins.aicore.tool.ToolHandler +import com.itsaky.androidide.plugins.aicore.tool.isTerminalToolName import com.itsaky.androidide.plugins.aicore.tool.sources.ToolSourceStore import com.itsaky.androidide.plugins.aicore.tool.handlers.AddDependencyHandler import com.itsaky.androidide.plugins.aicore.tool.handlers.CreateFileHandler @@ -1004,7 +1005,10 @@ class ChatViewModel( // Per-run flag (set by executeToolCalls), not a session-wide scan. val lastToolFailed = lastToolFailedThisRun - val realCalls = toolCalls.filterNot { it.name == RESPOND_TOOL } + // Matched loosely, as everywhere else: a backend that answers `Respond` would + // otherwise leave the terminal call in `realCalls`, so an identical repeat + // never matches and the duplicate-turn bubble is never dropped. + val realCalls = toolCalls.filterNot { isTerminalToolName(it.name, RESPOND_TOOL) } if (realCalls.isNotEmpty() && realCalls == lastSucceededCalls) { viewModelScope.launch(Dispatchers.Main) { _messages.value = _messages.value.filter { it.id != agentMessageId } From 03a81e27ce1ed9ec3cb3f2480a94739fed1908fe Mon Sep 17 00:00:00 2001 From: John Trujillo Date: Tue, 1 Sep 2026 09:50:51 -0500 Subject: [PATCH 2/5] fix(ai): stop Unavailable secrets being treated as lost or overwritable MCP headers gain the token's keep-it sentinel, the OpenAI and Gemini key panes stay dressed as configured, setHeaders clears through write(), and the MCP bearer token regains its .trim(); plus tests, CLAUDE.md and stale comments. --- CLAUDE.md | 34 +++++++++----- ai-agent-gemini/src/main/AndroidManifest.xml | 2 - .../settings/GeminiSettingsFragment.kt | 29 ++++++++---- .../settings/GeminiSettingsViewModel.kt | 8 ++++ ai-agent-mcp/README.md | 19 +++++--- .../aiagentmcp/client/McpConnections.kt | 4 +- .../aiagentmcp/settings/McpServerStore.kt | 17 ++++--- .../settings/McpSettingsFragment.kt | 15 ++++-- .../settings/McpSettingsViewModel.kt | 26 +++++++++-- .../errors/McpErrorFormatterTest.kt | 10 ++++ .../testing/FakeSharedPreferences.kt | 46 +++++++++++++------ ai-agent-openai/build.gradle.kts | 4 -- ai-agent-openai/src/main/AndroidManifest.xml | 3 -- .../settings/OpenAiSettingsFragment.kt | 30 ++++++++---- .../aicore/viewmodel/AgentReplyRenderer.kt | 21 +++++++++ .../plugins/aicore/viewmodel/ChatViewModel.kt | 7 +-- .../viewmodel/AgentReplyRendererTest.kt | 39 ++++++++++++++++ 17 files changed, 237 insertions(+), 77 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 67337db8..0cfb856c 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -56,19 +56,31 @@ copy that started to diverge, which is what ADFA-5255 removed. Construct it with **this plugin's own alias** (`KeystoreSecretStore(ALIAS)`) as a single top-level `val` in a `SecureApiKeyStore.kt`/`SecureTokenStore.kt` that holds nothing but the alias; -callers use that instance directly. The store logs under its own name — it takes no log tag, and -the second constructor parameter is a `SecretKeySource` override that plugins do not pass. +callers use that instance directly. It takes no log tag — the store logs under its own name — and +that single-argument constructor is the only one a plugin can reach: the two-argument form takes an +`internal` `SecretKeySource`, which is not on the plugin's compile classpath at all. `ai-agent-mcp`, `ai-agent-gemini` and `ai-agent-openai` are the reference shape. Do **not** wrap it in an object of forwarding methods — that is just a second copy of the store's contract to keep -in step. The alias must be unique per -plugin (all plugins share the host's UID and Keystore, so a shared alias lets one plugin's -invalidated-key recovery delete another's secret) and must never change across releases. - -`readAndMigrate` returns a three-way `Stored` (`Absent` / `Value` / `Unreadable`) rather than a -nullable String on purpose: "never saved" and "saved but this device's Keystore can no longer open -it" need opposite advice, and a plugin that collapses them tells a user their credential was -refused when it was never sent. Collapse it only where the caller genuinely has one answer for -both, and say so in a comment. +in step. The alias must be unique per plugin (all plugins share the host's UID and Keystore, so a +shared alias lets one plugin's invalidated-key recovery delete another's secret) and must never +change across releases. + +`readAndMigrate` returns a four-way `Stored` rather than a nullable String on purpose — each state +needs different advice, and a plugin that collapses them tells a user their credential was refused +when it was never sent: + +- `Absent` — nothing was ever saved. The ordinary first run; say nothing. +- `Value` — the plaintext. Trim it at the call site if your credential format wants it; + `readAndMigrate` migrates verbatim. +- `Unreadable` — stored, but this device's Keystore can no longer open it (a restored backup, an + OEM Keystore reset). Permanent: the user has to enter it again. +- `Unavailable` — the Keystore would not answer this time. **Transient: the credential is intact, + so retry and never re-prompt.** In particular a pane that reads `Unavailable` must not dress + itself as never-configured, and nothing on that screen may write over the credential it could + not read — an empty field then means "not shown", not "removed". + +Handle all four; collapse them only where the caller genuinely has one answer for every state, and +say so in a comment. ### Plugin shape diff --git a/ai-agent-gemini/src/main/AndroidManifest.xml b/ai-agent-gemini/src/main/AndroidManifest.xml index 74ba6b64..fbd465d6 100644 --- a/ai-agent-gemini/src/main/AndroidManifest.xml +++ b/ai-agent-gemini/src/main/AndroidManifest.xml @@ -29,8 +29,6 @@ android:name="plugin.author" android:value="App Dev for All" /> - diff --git a/ai-agent-gemini/src/main/kotlin/com/itsaky/androidide/plugins/aiagentgemini/settings/GeminiSettingsFragment.kt b/ai-agent-gemini/src/main/kotlin/com/itsaky/androidide/plugins/aiagentgemini/settings/GeminiSettingsFragment.kt index c9dbea4b..b842c909 100644 --- a/ai-agent-gemini/src/main/kotlin/com/itsaky/androidide/plugins/aiagentgemini/settings/GeminiSettingsFragment.kt +++ b/ai-agent-gemini/src/main/kotlin/com/itsaky/androidide/plugins/aiagentgemini/settings/GeminiSettingsFragment.kt @@ -193,10 +193,18 @@ class GeminiSettingsFragment : Fragment() { val stored = viewModel.getGeminiApiKey() val savedApiKey = (stored as? KeystoreSecretStore.Stored.Value)?.plain val hasKey = !savedApiKey.isNullOrBlank() - updateUiState(isEditing = !hasKey) - if (hasKey) { + // A keystore that would not answer this time leaves the key on disk and intact, so the + // pane stays dressed as configured — status line, Edit, Remove. Opening edit mode + // instead would make it identical to a fresh install, contradicting the toast below. + val keptConfigured = + !hasKey && + stored is KeystoreSecretStore.Stored.Unavailable && + viewModel.hasStoredGeminiApiKey() + updateUiState(isEditing = !hasKey && !keptConfigured) + if (hasKey || keptConfigured) { statusTextView.text = savedApiKeyStatusText() - } else { + } + if (!hasKey) { apiKeyInput.setText("") // Only for a key that is there and will not decrypt; an empty box alone looks like // data loss. Nothing stored at all is the ordinary first run and says nothing. @@ -402,18 +410,21 @@ class GeminiSettingsFragment : Fragment() { // A key that is stored and will not decrypt; an empty box alone looks like data // loss. Told apart from "nothing stored" here, which this button rarely sees but // must not report as a lost Keystore entry when it does. - if (stored is KeystoreSecretStore.Stored.Unreadable) { + if (stored is KeystoreSecretStore.Stored.Unavailable) { + // Said differently from an unreadable key: this one is still there and intact, + // so the pane stays as it is rather than emptying the field under a status line + // that just said the key is saved — it must not be re-typed to be recovered. Toast.makeText( requireContext(), - getString(R.string.msg_api_key_unreadable), + getString(R.string.msg_api_key_unavailable), Toast.LENGTH_LONG ).show() - } else if (stored is KeystoreSecretStore.Stored.Unavailable) { - // Said differently from the above: the key is still there and intact, so this - // must not send the user off to find and type it again. + return@launch + } + if (stored is KeystoreSecretStore.Stored.Unreadable) { Toast.makeText( requireContext(), - getString(R.string.msg_api_key_unavailable), + getString(R.string.msg_api_key_unreadable), Toast.LENGTH_LONG ).show() } diff --git a/ai-agent-gemini/src/main/kotlin/com/itsaky/androidide/plugins/aiagentgemini/settings/GeminiSettingsViewModel.kt b/ai-agent-gemini/src/main/kotlin/com/itsaky/androidide/plugins/aiagentgemini/settings/GeminiSettingsViewModel.kt index 3e47a891..65dc0ac3 100644 --- a/ai-agent-gemini/src/main/kotlin/com/itsaky/androidide/plugins/aiagentgemini/settings/GeminiSettingsViewModel.kt +++ b/ai-agent-gemini/src/main/kotlin/com/itsaky/androidide/plugins/aiagentgemini/settings/GeminiSettingsViewModel.kt @@ -166,6 +166,14 @@ class GeminiSettingsViewModel( secureApiKeyStore.readAndMigrate(prefs(), KEY_API_KEY) } + /** + * True when a key is present on disk, whether or not it can still be decrypted: what the key + * block is dressed from, which must not collapse the moment the Keystore declines to answer. + * Raw pref only, so no Keystore IPC and safe on the main thread — which [getGeminiApiKey] is not. + */ + fun hasStoredGeminiApiKey(): Boolean = + !prefs()?.getString(KEY_API_KEY, null).isNullOrBlank() + fun getGeminiApiKeySaveTimestamp(): Long = prefs()?.getLong(KEY_API_KEY_TIMESTAMP, 0L) ?: 0L fun clearGeminiApiKey() { diff --git a/ai-agent-mcp/README.md b/ai-agent-mcp/README.md index 67a7fb87..57891125 100644 --- a/ai-agent-mcp/README.md +++ b/ai-agent-mcp/README.md @@ -79,7 +79,7 @@ Two dependencies were deliberately not taken: `McpToolCatalog` (what each server last advertised), `McpToolText` (sanitising) - `settings/` — server CRUD, per-tool toggles, the settings pane - `errors/` — HTTP and JSON-RPC failures reduced to one translated sentence -- `security/` — Keystore-backed token encryption +- `security/SecureTokenStore.kt` — this plugin's Keystore alias, over the IDE's `KeystoreSecretStore` ## Security notes @@ -91,11 +91,18 @@ Two dependencies were deliberately not taken: pairing — the two plugins version independently — so an `ai-core` older than the release that flattens at its `ContributedToolHandler` boundary would take raw multi-line server text into the prompt. -- Tokens are encrypted with an AES/GCM key held in the Android Keystore under - this plugin's own alias; only ciphertext is written to disk. A token that can no - longer be decrypted — a restored backup, an OEM Keystore reset — is reported as - exactly that, never sent as an absent one, which would surface as the server - refusing a token that is still stored and still correct. +- Tokens and custom headers are encrypted with an AES/GCM key held in the Android + Keystore under this plugin's own alias; only ciphertext is written to disk. + `security/SecureTokenStore.kt` holds nothing but that alias (`cotg_ai_mcp_token_v1`); + the AES/GCM itself is the IDE's `KeystoreSecretStore` (`plugin-api`, since + **26.35** — hence this plugin's `min_ide_version`), so there is one implementation + in the process rather than a copy per plugin. The alias stays per plugin: they all + share the host's Keystore, so a shared alias would let one plugin's invalidated-key + recovery delete another's secret. A token that can no longer be decrypted — a + restored backup, an OEM Keystore reset — is reported as exactly that, never sent as + an absent one, which would surface as the server refusing a token that is still + stored and still correct; a Keystore that merely would not answer is told apart + again, since that token is intact and the call is only worth retrying. - A token or a custom header is refused on an `http://` URL: encryption at rest buys nothing for a credential sent in the clear. - Redirects are never followed automatically. A 3xx is repeated only when it diff --git a/ai-agent-mcp/src/main/kotlin/com/itsaky/androidide/plugins/aiagentmcp/client/McpConnections.kt b/ai-agent-mcp/src/main/kotlin/com/itsaky/androidide/plugins/aiagentmcp/client/McpConnections.kt index 1f11a8a6..3f9b28fd 100644 --- a/ai-agent-mcp/src/main/kotlin/com/itsaky/androidide/plugins/aiagentmcp/client/McpConnections.kt +++ b/ai-agent-mcp/src/main/kotlin/com/itsaky/androidide/plugins/aiagentmcp/client/McpConnections.kt @@ -95,7 +95,9 @@ object McpConnections { */ private fun credentialsFor(serverId: String): McpCredentials { val token = when (val stored = McpServerStore.token(serverId)) { - is KeystoreSecretStore.Stored.Value -> stored.plain + // Trimmed here, not by the store: the host's readAndMigrate migrates verbatim, and a + // legacy token saved with a stray newline is one `setRequestProperty` refuses to send. + is KeystoreSecretStore.Stored.Value -> stored.plain.trim() KeystoreSecretStore.Stored.Absent -> "" KeystoreSecretStore.Stored.Unreadable -> throw UnreadableSecretException("The stored token for '$serverId' cannot be decrypted.") diff --git a/ai-agent-mcp/src/main/kotlin/com/itsaky/androidide/plugins/aiagentmcp/settings/McpServerStore.kt b/ai-agent-mcp/src/main/kotlin/com/itsaky/androidide/plugins/aiagentmcp/settings/McpServerStore.kt index ff8cfcc8..0ccfc938 100644 --- a/ai-agent-mcp/src/main/kotlin/com/itsaky/androidide/plugins/aiagentmcp/settings/McpServerStore.kt +++ b/ai-agent-mcp/src/main/kotlin/com/itsaky/androidide/plugins/aiagentmcp/settings/McpServerStore.kt @@ -240,14 +240,17 @@ object McpServerStore { fun setHeaders(id: String, headers: Map): Boolean { val clean = McpHeaders.sanitize(headers) val key = KEY_HEADERS_PREFIX + id - if (clean.isEmpty()) { - prefs()?.edit()?.remove(key)?.apply() - fireChanged() - return true + // The empty map goes through write() as a blank, which removes the entry, rather than + // through a bare remove/apply: the token's clear is synchronous and says whether it landed, + // and headers left on disk for a credential the user just forgot are the same leak. + val payload = if (clean.isEmpty()) { + "" + } else { + val json = JSONObject() + clean.forEach { (name, value) -> json.put(name, value) } + json.toString() } - val json = JSONObject() - clean.forEach { (name, value) -> json.put(name, value) } - val stored = secureTokenStore.write(prefs(), key, json.toString()) + val stored = secureTokenStore.write(prefs(), key, payload) fireChanged() return stored } diff --git a/ai-agent-mcp/src/main/kotlin/com/itsaky/androidide/plugins/aiagentmcp/settings/McpSettingsFragment.kt b/ai-agent-mcp/src/main/kotlin/com/itsaky/androidide/plugins/aiagentmcp/settings/McpSettingsFragment.kt index af9e1867..8f84f7cd 100644 --- a/ai-agent-mcp/src/main/kotlin/com/itsaky/androidide/plugins/aiagentmcp/settings/McpSettingsFragment.kt +++ b/ai-agent-mcp/src/main/kotlin/com/itsaky/androidide/plugins/aiagentmcp/settings/McpSettingsFragment.kt @@ -164,6 +164,10 @@ class McpSettingsFragment : Fragment() { // Unknown, never Absent, until the decrypt answers: guessing is what let an http:// URL // be saved over a token that was still stored and still sent. var credential = if (existing == null) Credential.ABSENT else Credential.UNKNOWN + // Whether the header rows on screen are what is stored. False until the decrypt answers, + // and for headers this device could not read: an empty list then means "never drawn", and + // saving it would delete headers the dialog was in no position to show. + var headersKnown = existing == null nameField.setText(server.name) urlField.setText(server.url) @@ -179,7 +183,11 @@ class McpSettingsFragment : Fragment() { clearButton.setOnClickListener { clearButton.isEnabled = false viewModel.clearCredential(server.id) { cleared -> - if (cleared) credential = Credential.ABSENT + if (cleared) { + credential = Credential.ABSENT + // Nothing is stored now, so the empty list on screen is the stored truth again. + headersKnown = true + } whileDialogShown { clearButton.isEnabled = !cleared clearButton.visibility = if (cleared) View.GONE else View.VISIBLE @@ -203,6 +211,7 @@ class McpSettingsFragment : Fragment() { credential = if (form.hasToken || form.hasHeaders) Credential.PRESENT else Credential.ABSENT + headersKnown = form.headersKnown } whileDialogShown { // The stored token is never shown: it is decrypted only to be sent. An empty @@ -250,7 +259,7 @@ class McpSettingsFragment : Fragment() { viewModel.save( candidate, viewModel.tokenToStore(tokenField.text.toString()), - headers, + viewModel.headersToStore(headers, headersKnown), ) { saved, failure -> server = saved if (tokenField.text.isNotBlank() || headers.isNotEmpty()) { @@ -315,7 +324,7 @@ class McpSettingsFragment : Fragment() { viewModel.save( candidate, viewModel.tokenToStore(tokenField.text.toString()), - headers, + viewModel.headersToStore(headers, headersKnown), ) { _, _ -> } dialog.dismiss() } diff --git a/ai-agent-mcp/src/main/kotlin/com/itsaky/androidide/plugins/aiagentmcp/settings/McpSettingsViewModel.kt b/ai-agent-mcp/src/main/kotlin/com/itsaky/androidide/plugins/aiagentmcp/settings/McpSettingsViewModel.kt index 09372cbb..6230b823 100644 --- a/ai-agent-mcp/src/main/kotlin/com/itsaky/androidide/plugins/aiagentmcp/settings/McpSettingsViewModel.kt +++ b/ai-agent-mcp/src/main/kotlin/com/itsaky/androidide/plugins/aiagentmcp/settings/McpSettingsViewModel.kt @@ -51,6 +51,8 @@ class McpSettingsViewModel( * device, which the field has to say aloud: it looks stored, but nothing can send it. * @property secretsUnavailable whether the keystore merely would not answer this time, which * the field says differently: the credential is intact and the read is worth repeating. + * @property headersKnown whether [headers] is what is stored. False when the decrypt failed, in + * which case an empty list on screen means "not shown" and must not be saved over them. * @property headers the extra headers configured for the server. */ data class FormState( @@ -58,6 +60,7 @@ class McpSettingsViewModel( val hasHeaders: Boolean, val secretsUnreadable: Boolean, val secretsUnavailable: Boolean, + val headersKnown: Boolean, val headers: Map, ) @@ -105,6 +108,7 @@ class McpSettingsViewModel( // credential that has to be entered again is the worse news. secretsUnavailable = token is KeystoreSecretStore.Stored.Unavailable || headersUnavailable, + headersKnown = headers != null, headers = headers.orEmpty(), ) } @@ -119,6 +123,21 @@ class McpSettingsViewModel( */ fun tokenToStore(typed: String): String? = typed.trim().takeIf { it.isNotEmpty() } + /** + * What the header rows mean when saving, given whether the stored ones could be read. + * + * The token's rule, applied to headers: an empty list is "leave them alone" whenever the dialog + * never drew what is stored, because a keystore that would not answer must not cost the user + * headers that are still intact. Rows the user actually typed are always an explicit + * replacement, and [clearCredential] is the way back to none. + * + * @param collected the header rows as they currently stand. + * @param headersKnown whether those rows reflect what is stored. + * @return the headers to store, or null to keep the stored ones. + */ + fun headersToStore(collected: Map, headersKnown: Boolean): Map? = + if (headersKnown || collected.isNotEmpty()) collected else null + /** * Stores the edited name and URL of a server and, when given, its token. * @@ -127,20 +146,21 @@ class McpSettingsViewModel( * * @param server the server to store. * @param token the token to store, or null to leave the stored one alone. - * @param headers the extra headers to store, replacing whatever was there. + * @param headers the extra headers to store, replacing whatever was there, or null to leave the + * stored ones alone — see [headersToStore]. * @param onDone receives the merged record and a status sentence, null when everything worked. */ fun save( server: McpServer, token: String?, - headers: Map = emptyMap(), + headers: Map? = emptyMap(), onDone: (McpServer, String?) -> Unit, ) { viewModelScope.launch { val outcome = withContext(Dispatchers.IO) { val merged = McpServerStore.saveDetails(server) val tokenStored = token?.let { McpServerStore.setToken(server.id, it.trim()) } ?: true - val headersStored = McpServerStore.setHeaders(server.id, headers) + val headersStored = headers?.let { McpServerStore.setHeaders(server.id, it) } ?: true // A credential change has to invalidate the session, or the old one keeps working. McpConnections.invalidate(server.id) val failure = when { diff --git a/ai-agent-mcp/src/test/kotlin/com/itsaky/androidide/plugins/aiagentmcp/errors/McpErrorFormatterTest.kt b/ai-agent-mcp/src/test/kotlin/com/itsaky/androidide/plugins/aiagentmcp/errors/McpErrorFormatterTest.kt index 2c310285..cb837d72 100644 --- a/ai-agent-mcp/src/test/kotlin/com/itsaky/androidide/plugins/aiagentmcp/errors/McpErrorFormatterTest.kt +++ b/ai-agent-mcp/src/test/kotlin/com/itsaky/androidide/plugins/aiagentmcp/errors/McpErrorFormatterTest.kt @@ -1,6 +1,7 @@ package com.itsaky.androidide.plugins.aiagentmcp.errors import com.itsaky.androidide.plugins.aiagentmcp.client.McpProtocolException +import com.itsaky.androidide.plugins.aiagentmcp.security.UnavailableSecretException import com.itsaky.androidide.plugins.aiagentmcp.security.UnreadableSecretException import com.itsaky.androidide.plugins.aiagentmcp.transport.McpHttpException import com.itsaky.androidide.plugins.aiagentmcp.transport.McpRedirectException @@ -27,6 +28,15 @@ class McpErrorFormatterTest { assertEquals(McpFailure.SecretUnreadable, failure) } + @Test + fun givenAKeystoreThatWouldNotAnswer_whenClassified_thenItIsRetryableRatherThanAReachFailure() { + // Kept apart from the arm above and from the generic one: nothing was sent, so "Could not + // reach X" sends the user hunting a network problem when the answer is to try again. + val failure = McpErrorFormatter.classify(UnavailableSecretException("keystore busy")) + + assertEquals(McpFailure.SecretUnavailable, failure) + } + @Test fun givenARefusedRedirect_whenClassified_thenItIsItsOwnFailureRatherThanAGenericOne() { // "Could not reach X: " would read as a network problem; nothing was sent, and the diff --git a/ai-agent-mcp/src/test/kotlin/com/itsaky/androidide/plugins/aiagentmcp/testing/FakeSharedPreferences.kt b/ai-agent-mcp/src/test/kotlin/com/itsaky/androidide/plugins/aiagentmcp/testing/FakeSharedPreferences.kt index f8660738..ef47fbca 100644 --- a/ai-agent-mcp/src/test/kotlin/com/itsaky/androidide/plugins/aiagentmcp/testing/FakeSharedPreferences.kt +++ b/ai-agent-mcp/src/test/kotlin/com/itsaky/androidide/plugins/aiagentmcp/testing/FakeSharedPreferences.kt @@ -13,7 +13,10 @@ import java.util.concurrent.ConcurrentHashMap */ class FakeSharedPreferences : SharedPreferences { - private val values = ConcurrentHashMap() + // Typed, not String-keyed: every getter and putter is backed by this one map, so a test that + // stores a flag or a timestamp through the fake reads back what it wrote rather than the + // default. A no-op putter would give it a green run that proved nothing. + private val values = ConcurrentHashMap() /** How long a read blocks, widening the window a missing lock would lose a write in. */ @Volatile @@ -23,19 +26,21 @@ class FakeSharedPreferences : SharedPreferences { override fun getString(key: String, defValue: String?): String? { if (readDelayMillis > 0) Thread.sleep(readDelayMillis) - return values[key] ?: defValue + return values[key] as? String ?: defValue } + @Suppress("UNCHECKED_CAST") override fun getStringSet(key: String, defValues: MutableSet?): MutableSet? = - defValues + (values[key] as? Set)?.toMutableSet() ?: defValues - override fun getInt(key: String, defValue: Int): Int = defValue + override fun getInt(key: String, defValue: Int): Int = values[key] as? Int ?: defValue - override fun getLong(key: String, defValue: Long): Long = defValue + override fun getLong(key: String, defValue: Long): Long = values[key] as? Long ?: defValue - override fun getFloat(key: String, defValue: Float): Float = defValue + override fun getFloat(key: String, defValue: Float): Float = values[key] as? Float ?: defValue - override fun getBoolean(key: String, defValue: Boolean): Boolean = defValue + override fun getBoolean(key: String, defValue: Boolean): Boolean = + values[key] as? Boolean ?: defValue override fun contains(key: String): Boolean = values.containsKey(key) @@ -52,7 +57,7 @@ class FakeSharedPreferences : SharedPreferences { /** Batches edits and applies them at once, as the real editor does. */ private inner class FakeEditor : SharedPreferences.Editor { - private val puts = LinkedHashMap() + private val puts = LinkedHashMap() private val removals = LinkedHashSet() private var cleared = false @@ -64,15 +69,30 @@ class FakeSharedPreferences : SharedPreferences { override fun putStringSet( key: String, values: MutableSet? - ): SharedPreferences.Editor = this + ): SharedPreferences.Editor { + if (values == null) removals += key else puts[key] = LinkedHashSet(values) + return this + } - override fun putInt(key: String, value: Int): SharedPreferences.Editor = this + override fun putInt(key: String, value: Int): SharedPreferences.Editor { + puts[key] = value + return this + } - override fun putLong(key: String, value: Long): SharedPreferences.Editor = this + override fun putLong(key: String, value: Long): SharedPreferences.Editor { + puts[key] = value + return this + } - override fun putFloat(key: String, value: Float): SharedPreferences.Editor = this + override fun putFloat(key: String, value: Float): SharedPreferences.Editor { + puts[key] = value + return this + } - override fun putBoolean(key: String, value: Boolean): SharedPreferences.Editor = this + override fun putBoolean(key: String, value: Boolean): SharedPreferences.Editor { + puts[key] = value + return this + } override fun remove(key: String): SharedPreferences.Editor { removals += key diff --git a/ai-agent-openai/build.gradle.kts b/ai-agent-openai/build.gradle.kts index 9fa1882e..c6822884 100644 --- a/ai-agent-openai/build.gradle.kts +++ b/ai-agent-openai/build.gradle.kts @@ -71,10 +71,6 @@ dependencies { testImplementation("org.json:json:20231013") } -// This plugin carries its own copy of SecureApiKeyStore under its own Keystore alias. No parity -// check against ai-agent-gemini's copy: the two never read each other's ciphertext, and a shared -// alias would let one plugin's invalidation recovery delete the other plugin's key. - // AAR metadata checks are disabled by convention for these application-as-library plugins. tasks.matching { it.name.contains("checkDebugAarMetadata") || diff --git a/ai-agent-openai/src/main/AndroidManifest.xml b/ai-agent-openai/src/main/AndroidManifest.xml index dd3d2814..3dec2c57 100644 --- a/ai-agent-openai/src/main/AndroidManifest.xml +++ b/ai-agent-openai/src/main/AndroidManifest.xml @@ -33,9 +33,6 @@ android:name="plugin.author" android:value="App Dev for All" /> - diff --git a/ai-agent-openai/src/main/kotlin/com/itsaky/androidide/plugins/aiagentopenai/settings/OpenAiSettingsFragment.kt b/ai-agent-openai/src/main/kotlin/com/itsaky/androidide/plugins/aiagentopenai/settings/OpenAiSettingsFragment.kt index 0fba45d2..2a229a9b 100644 --- a/ai-agent-openai/src/main/kotlin/com/itsaky/androidide/plugins/aiagentopenai/settings/OpenAiSettingsFragment.kt +++ b/ai-agent-openai/src/main/kotlin/com/itsaky/androidide/plugins/aiagentopenai/settings/OpenAiSettingsFragment.kt @@ -351,10 +351,19 @@ class OpenAiSettingsFragment : Fragment() { val stored = viewModel.getApiKey() val savedApiKey = (stored as? KeystoreSecretStore.Stored.Value)?.plain val hasKey = !savedApiKey.isNullOrBlank() - updateUiState(isEditing = !hasKey) - if (hasKey) { + // A keystore that would not answer this time leaves the key on disk and intact, so the + // pane stays dressed as configured. Opening edit mode instead would make it identical + // to a fresh install, and for a server that needs no key a blank Save from there runs + // clearApiKey() over the key this same read just called recoverable. + val keptConfigured = + !hasKey && + stored is KeystoreSecretStore.Stored.Unavailable && + viewModel.hasStoredApiKey() + updateUiState(isEditing = !hasKey && !keptConfigured) + if (hasKey || keptConfigured) { statusTextView.text = savedApiKeyStatusText() - } else { + } + if (!hasKey) { apiKeyInput.setText("") // Only for a key that is there and will not decrypt; an empty box alone looks like // data loss. Nothing stored at all is the ordinary first run and says nothing. @@ -585,18 +594,21 @@ class OpenAiSettingsFragment : Fragment() { // A key that is stored and will not decrypt; an empty box alone looks like data // loss. Told apart from "nothing stored" here, which this button rarely sees but // must not report as a lost Keystore entry when it does. - if (stored is KeystoreSecretStore.Stored.Unreadable) { + if (stored is KeystoreSecretStore.Stored.Unavailable) { + // Said differently from an unreadable key: this one is still there and intact, + // so the pane stays as it is rather than opening an empty field the user would + // Save over it — it must not send them off to find and type it again. Toast.makeText( requireContext(), - getString(R.string.msg_api_key_unreadable), + getString(R.string.msg_api_key_unavailable), Toast.LENGTH_LONG ).show() - } else if (stored is KeystoreSecretStore.Stored.Unavailable) { - // Said differently from the above: the key is still there and intact, so this - // must not send the user off to find and type it again. + return@launch + } + if (stored is KeystoreSecretStore.Stored.Unreadable) { Toast.makeText( requireContext(), - getString(R.string.msg_api_key_unavailable), + getString(R.string.msg_api_key_unreadable), Toast.LENGTH_LONG ).show() } diff --git a/ai-core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/viewmodel/AgentReplyRenderer.kt b/ai-core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/viewmodel/AgentReplyRenderer.kt index d2568d7e..a4570e26 100644 --- a/ai-core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/viewmodel/AgentReplyRenderer.kt +++ b/ai-core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/viewmodel/AgentReplyRenderer.kt @@ -12,6 +12,27 @@ import com.itsaky.androidide.plugins.aicore.tool.respondMessageOf */ object AgentReplyRenderer { + /** + * Whether this turn only repeats the calls that already succeeded, so its bubble is dropped. + * + * The terminal call is filtered out by the same loose match a handler is routed by, not by + * equality: a backend answering `Respond` would otherwise leave it among the real calls, so an + * identical repeat never compares equal and the duplicate bubble is never dropped. + * + * @param toolCalls the calls parsed out of this turn. + * @param lastSucceededCalls the calls this run last executed successfully, null when none did. + * @param terminalTool the name of the answer-carrying pseudo-tool (`respond`). + * @return true when the turn adds nothing and should not reach the transcript. + */ + fun isDuplicateTurn( + toolCalls: List, + lastSucceededCalls: List?, + terminalTool: String, + ): Boolean { + val realCalls = toolCalls.filterNot { isTerminalToolName(it.name, terminalTool) } + return realCalls.isNotEmpty() && realCalls == lastSucceededCalls + } + /** * Renders one model turn. * @param rawText the model's raw reply. diff --git a/ai-core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/viewmodel/ChatViewModel.kt b/ai-core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/viewmodel/ChatViewModel.kt index 59d6bcf3..d16e7835 100644 --- a/ai-core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/viewmodel/ChatViewModel.kt +++ b/ai-core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/viewmodel/ChatViewModel.kt @@ -25,7 +25,6 @@ import com.itsaky.androidide.plugins.aicore.tool.ToolCall import com.itsaky.androidide.plugins.aicore.tool.ToolCallExtractor import com.itsaky.androidide.plugins.aicore.tool.ToolExecutionTracker import com.itsaky.androidide.plugins.aicore.tool.ToolHandler -import com.itsaky.androidide.plugins.aicore.tool.isTerminalToolName import com.itsaky.androidide.plugins.aicore.tool.sources.ToolSourceStore import com.itsaky.androidide.plugins.aicore.tool.handlers.AddDependencyHandler import com.itsaky.androidide.plugins.aicore.tool.handlers.CreateFileHandler @@ -1005,11 +1004,7 @@ class ChatViewModel( // Per-run flag (set by executeToolCalls), not a session-wide scan. val lastToolFailed = lastToolFailedThisRun - // Matched loosely, as everywhere else: a backend that answers `Respond` would - // otherwise leave the terminal call in `realCalls`, so an identical repeat - // never matches and the duplicate-turn bubble is never dropped. - val realCalls = toolCalls.filterNot { isTerminalToolName(it.name, RESPOND_TOOL) } - if (realCalls.isNotEmpty() && realCalls == lastSucceededCalls) { + if (AgentReplyRenderer.isDuplicateTurn(toolCalls, lastSucceededCalls, RESPOND_TOOL)) { viewModelScope.launch(Dispatchers.Main) { _messages.value = _messages.value.filter { it.id != agentMessageId } } diff --git a/ai-core/src/test/kotlin/com/itsaky/androidide/plugins/aicore/viewmodel/AgentReplyRendererTest.kt b/ai-core/src/test/kotlin/com/itsaky/androidide/plugins/aicore/viewmodel/AgentReplyRendererTest.kt index fad0e4b0..30f6def2 100644 --- a/ai-core/src/test/kotlin/com/itsaky/androidide/plugins/aicore/viewmodel/AgentReplyRendererTest.kt +++ b/ai-core/src/test/kotlin/com/itsaky/androidide/plugins/aicore/viewmodel/AgentReplyRendererTest.kt @@ -2,6 +2,8 @@ package com.itsaky.androidide.plugins.aicore.viewmodel import com.itsaky.androidide.plugins.aicore.tool.ToolCall import org.junit.Assert.assertEquals +import org.junit.Assert.assertFalse +import org.junit.Assert.assertTrue import org.junit.Test /** @@ -124,4 +126,41 @@ class AgentReplyRendererTest { fun givenNothingAtAll_whenRendered_thenTheFallbackIsShown() { assertEquals(NO_RESPONSE, render(" ")) } + + @Test + fun givenARepeatedCallBesideACapitalisedRespond_whenChecked_thenTheTurnIsStillADuplicate() { + // What `it.name == RESPOND_TOOL` got wrong: a backend that answers `Respond` left the + // terminal call among the real ones, so the repeat never compared equal and the agent's + // duplicate turn reached the transcript. + val succeeded = listOf(ToolCall("read_file", mapOf("file_path" to "A.kt"))) + val repeated = succeeded + ToolCall("Respond", mapOf("message" to "Done.")) + + assertTrue(AgentReplyRenderer.isDuplicateTurn(repeated, succeeded, TERMINAL)) + } + + @Test + fun givenADifferentCall_whenChecked_thenTheTurnIsNotADuplicate() { + val succeeded = listOf(ToolCall("read_file", mapOf("file_path" to "A.kt"))) + val next = listOf(ToolCall("read_file", mapOf("file_path" to "B.kt"))) + + assertFalse(AgentReplyRenderer.isDuplicateTurn(next, succeeded, TERMINAL)) + } + + @Test + fun givenOnlyATerminalCall_whenChecked_thenTheAnswerIsNeverDroppedAsADuplicate() { + // The answer-carrying turn has no real calls left after the filter, so it must fall through + // even when the run's last real call is what it is reporting on. + val succeeded = listOf(ToolCall("read_file", mapOf("file_path" to "A.kt"))) + + assertFalse( + AgentReplyRenderer.isDuplicateTurn(respond("message" to "Done."), succeeded, TERMINAL) + ) + } + + @Test + fun givenNothingSucceededYet_whenChecked_thenTheTurnIsNotADuplicate() { + val calls = listOf(ToolCall("read_file", mapOf("file_path" to "A.kt"))) + + assertFalse(AgentReplyRenderer.isDuplicateTurn(calls, null, TERMINAL)) + } } From efe0f6040b6849cfb58d628987ee0a49feb8cecb Mon Sep 17 00:00:00 2001 From: John Trujillo Date: Tue, 1 Sep 2026 11:06:15 -0500 Subject: [PATCH 3/5] fix(ai): keep Unavailable secrets out of the test and second-save paths Test connection now reports an unreadable-just-now key instead of testing without it, a saved header set is deletable on a second save, and the 26.35 floor says why it moved again. --- ai-agent-gemini/src/main/AndroidManifest.xml | 2 ++ .../preferences/GeminiPreferences.kt | 2 ++ ai-agent-mcp/src/main/AndroidManifest.xml | 2 ++ .../settings/McpSettingsFragment.kt | 9 ++++++- ai-agent-openai/build.gradle.kts | 3 +++ ai-agent-openai/src/main/AndroidManifest.xml | 2 ++ .../settings/OpenAiSettingsFragment.kt | 24 +++++++++++++++---- .../settings/OpenAiSettingsViewModel.kt | 21 +++++++++------- .../src/main/res/values/strings.xml | 1 + 9 files changed, 52 insertions(+), 14 deletions(-) diff --git a/ai-agent-gemini/src/main/AndroidManifest.xml b/ai-agent-gemini/src/main/AndroidManifest.xml index fbd465d6..cc46e585 100644 --- a/ai-agent-gemini/src/main/AndroidManifest.xml +++ b/ai-agent-gemini/src/main/AndroidManifest.xml @@ -29,6 +29,8 @@ android:name="plugin.author" android:value="App Dev for All" /> + diff --git a/ai-agent-gemini/src/main/kotlin/com/itsaky/androidide/plugins/aiagentgemini/preferences/GeminiPreferences.kt b/ai-agent-gemini/src/main/kotlin/com/itsaky/androidide/plugins/aiagentgemini/preferences/GeminiPreferences.kt index c59d33ee..56c5c8fb 100644 --- a/ai-agent-gemini/src/main/kotlin/com/itsaky/androidide/plugins/aiagentgemini/preferences/GeminiPreferences.kt +++ b/ai-agent-gemini/src/main/kotlin/com/itsaky/androidide/plugins/aiagentgemini/preferences/GeminiPreferences.kt @@ -2,6 +2,8 @@ package com.itsaky.androidide.plugins.aiagentgemini.preferences import android.content.SharedPreferences import com.itsaky.androidide.plugins.PluginContext +// Imported for the KDoc link below: the alias the API key is encrypted under lives there. +import com.itsaky.androidide.plugins.aiagentgemini.security.secureApiKeyStore /** * This plugin's own settings store, and the one-time move of its settings out of AI Core's. diff --git a/ai-agent-mcp/src/main/AndroidManifest.xml b/ai-agent-mcp/src/main/AndroidManifest.xml index e85fb8c9..fa97893c 100644 --- a/ai-agent-mcp/src/main/AndroidManifest.xml +++ b/ai-agent-mcp/src/main/AndroidManifest.xml @@ -29,6 +29,8 @@ android:name="plugin.author" android:value="App Dev for All" /> + diff --git a/ai-agent-mcp/src/main/kotlin/com/itsaky/androidide/plugins/aiagentmcp/settings/McpSettingsFragment.kt b/ai-agent-mcp/src/main/kotlin/com/itsaky/androidide/plugins/aiagentmcp/settings/McpSettingsFragment.kt index 8f84f7cd..6dc780dd 100644 --- a/ai-agent-mcp/src/main/kotlin/com/itsaky/androidide/plugins/aiagentmcp/settings/McpSettingsFragment.kt +++ b/ai-agent-mcp/src/main/kotlin/com/itsaky/androidide/plugins/aiagentmcp/settings/McpSettingsFragment.kt @@ -256,12 +256,19 @@ class McpSettingsFragment : Fragment() { // Stored first: connecting reads the URL and credentials from the store. status.text = getString(R.string.mcp_status_connecting) button.isEnabled = false + val headersToStore = viewModel.headersToStore(headers, headersKnown) viewModel.save( candidate, viewModel.tokenToStore(tokenField.text.toString()), - viewModel.headersToStore(headers, headersKnown), + headersToStore, ) { saved, failure -> server = saved + // Connect deliberately leaves the dialog open, so there can be a second save. + // Once these rows have been written they are the stored truth, and a row + // removed afterwards has to be a deletion rather than a skipped write. + if (headersToStore != null) { + headersKnown = true + } if (tokenField.text.isNotBlank() || headers.isNotEmpty()) { credential = Credential.PRESENT } diff --git a/ai-agent-openai/build.gradle.kts b/ai-agent-openai/build.gradle.kts index c6822884..9cd03321 100644 --- a/ai-agent-openai/build.gradle.kts +++ b/ai-agent-openai/build.gradle.kts @@ -71,6 +71,9 @@ dependencies { testImplementation("org.json:json:20231013") } +// No SecureApiKeyStore parity check any more: the AES/GCM core is the host's KeystoreSecretStore +// (plugin-api), so there is one implementation in the process rather than copies to keep in step. + // AAR metadata checks are disabled by convention for these application-as-library plugins. tasks.matching { it.name.contains("checkDebugAarMetadata") || diff --git a/ai-agent-openai/src/main/AndroidManifest.xml b/ai-agent-openai/src/main/AndroidManifest.xml index 3dec2c57..6bad27a0 100644 --- a/ai-agent-openai/src/main/AndroidManifest.xml +++ b/ai-agent-openai/src/main/AndroidManifest.xml @@ -33,6 +33,8 @@ android:name="plugin.author" android:value="App Dev for All" /> + diff --git a/ai-agent-openai/src/main/kotlin/com/itsaky/androidide/plugins/aiagentopenai/settings/OpenAiSettingsFragment.kt b/ai-agent-openai/src/main/kotlin/com/itsaky/androidide/plugins/aiagentopenai/settings/OpenAiSettingsFragment.kt index 2a229a9b..7b7f69fc 100644 --- a/ai-agent-openai/src/main/kotlin/com/itsaky/androidide/plugins/aiagentopenai/settings/OpenAiSettingsFragment.kt +++ b/ai-agent-openai/src/main/kotlin/com/itsaky/androidide/plugins/aiagentopenai/settings/OpenAiSettingsFragment.kt @@ -764,12 +764,28 @@ class OpenAiSettingsFragment : Fragment() { // Tests what is on screen: a typo is worth catching before it is saved. val url = urlInput.text.toString().trim().ifEmpty { viewModel.getBaseUrl() } val typedKey = apiKeyInput.text.toString().trim() - val key = if (apiKeyLayout.visibility == View.VISIBLE && typedKey.isNotEmpty()) { + val useTyped = apiKeyLayout.visibility == View.VISIBLE && typedKey.isNotEmpty() + // Scoped to the URL under test: probing a LAN server must not hand it the key the + // user entered for OpenAI. + val stored = if (useTyped) null else viewModel.getApiKeyFor(url) + // A keystore that would not answer is not "no key stored": the key is intact and + // the pane above still reads "saved on ...". Testing without it would render the + // 401 as a refused key, or ask for one in a field that is not even shown. + if (stored is KeystoreSecretStore.Stored.Unavailable) { + showStatus( + statusText, + getString(R.string.msg_api_key_unavailable_for_test), + R.drawable.ic_key_unchecked + ) + testButton.isEnabled = true + return@launch + } + // Absent and Unreadable do share one answer here — no key to send — and the read + // that opened the pane has already said which of the two it was. + val key = if (useTyped) { typedKey } else { - // Scoped to the URL under test: probing a LAN server must not hand it the key - // the user entered for OpenAI. - viewModel.getApiKeyFor(url).orEmpty() + (stored as? KeystoreSecretStore.Stored.Value)?.plain?.trim().orEmpty() } // A server with no anonymous access can only answer 401 without a key, and // reporting that as "the server refused this key" when there is no key sends the diff --git a/ai-agent-openai/src/main/kotlin/com/itsaky/androidide/plugins/aiagentopenai/settings/OpenAiSettingsViewModel.kt b/ai-agent-openai/src/main/kotlin/com/itsaky/androidide/plugins/aiagentopenai/settings/OpenAiSettingsViewModel.kt index 85e4596b..dfee07f2 100644 --- a/ai-agent-openai/src/main/kotlin/com/itsaky/androidide/plugins/aiagentopenai/settings/OpenAiSettingsViewModel.kt +++ b/ai-agent-openai/src/main/kotlin/com/itsaky/androidide/plugins/aiagentopenai/settings/OpenAiSettingsViewModel.kt @@ -299,20 +299,23 @@ class OpenAiSettingsViewModel( } /** - * The stored key, but only when it was saved for [baseUrl]. + * What is stored for [baseUrl] — the whole four-way read, not a key-or-null. * * What the connection test sends: testing a LAN server must not hand it the key the user - * entered for OpenAI. A key stored before the origin was recorded is returned, matching the - * backend's own rule. + * entered for OpenAI, so a key saved for another origin reads as absent. A key stored before + * the origin was recorded is returned, matching the backend's own rule. * - * @return the plaintext key, or null when none is stored, it cannot be decrypted here, or it - * belongs to another server. The connection test has the same answer — send no key — for - * every one of them, and the pane has already said so on the read that opened it. + * @return the read, with a key belonging to another server reported as + * [KeystoreSecretStore.Stored.Absent] — the test's answer for it is the same as for nothing + * stored. [KeystoreSecretStore.Stored.Unavailable] is *not* that answer: the key is intact, + * so the caller retries instead of testing without one. */ - suspend fun getApiKeyFor(baseUrl: String): String? { + suspend fun getApiKeyFor(baseUrl: String): KeystoreSecretStore.Stored { val savedFor = prefs()?.getString(OpenAiPreferences.KEY_API_KEY_URL, null) - if (savedFor != null && !BaseUrlPolicy.sameOrigin(savedFor, baseUrl)) return null - return (getApiKey() as? KeystoreSecretStore.Stored.Value)?.plain + if (savedFor != null && !BaseUrlPolicy.sameOrigin(savedFor, baseUrl)) { + return KeystoreSecretStore.Stored.Absent + } + return getApiKey() } /** diff --git a/ai-agent-openai/src/main/res/values/strings.xml b/ai-agent-openai/src/main/res/values/strings.xml index 8d954ac4..daab0802 100644 --- a/ai-agent-openai/src/main/res/values/strings.xml +++ b/ai-agent-openai/src/main/res/values/strings.xml @@ -70,6 +70,7 @@ API Key saved and verified on: %s The stored API key could not be read on this device. Please enter it again. The device keystore could not be reached, so the stored API key could not be read. It is still saved — please try again in a moment. + The device keystore could not be reached, so the saved API key could not be read for this test. It is still saved — please try the test again in a moment. Couldn\'t save the API key on this device. Please try again. Checking this key with the server… Verified, your API key works From 8a06a9a9007e0fa5e0ff18799f0e97f5dac4beb2 Mon Sep 17 00:00:00 2001 From: John Trujillo Date: Tue, 1 Sep 2026 12:38:00 -0500 Subject: [PATCH 4/5] fix(ai): correct the IDE floor and stop unread headers being replaced MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit KeystoreSecretStore lands after 26.35 was tagged, so all nine sites now name 26.36. MCP header rows replace the stored set only when the dialog drew it, a four-way state gives each failure its own refusal — and forgetting headers on an unopened store is success. Plus an interleaving test that fails without McpPlugin's lifecycle lock, and the trims the edit fields lost. --- CLAUDE.md | 2 +- ai-agent-gemini/README.md | 2 +- ai-agent-gemini/src/main/AndroidManifest.xml | 7 +- .../settings/GeminiSettingsFragment.kt | 2 +- ai-agent-mcp/README.md | 2 +- ai-agent-mcp/src/main/AndroidManifest.xml | 7 +- .../plugins/aiagentmcp/plugin/McpPlugin.kt | 12 ++++ .../aiagentmcp/settings/McpServerStore.kt | 8 ++- .../settings/McpSettingsFragment.kt | 65 ++++++++++++++----- .../settings/McpSettingsViewModel.kt | 17 +++-- ai-agent-mcp/src/main/res/values/strings.xml | 2 + .../aiagentmcp/plugin/McpPluginScopeTest.kt | 57 ++++++++++++++++ .../settings/McpSettingsHeadersToStoreTest.kt | 43 ++++++++++++ ai-agent-openai/README.md | 2 +- ai-agent-openai/src/main/AndroidManifest.xml | 7 +- .../settings/OpenAiSettingsFragment.kt | 2 +- 16 files changed, 197 insertions(+), 40 deletions(-) create mode 100644 ai-agent-mcp/src/test/kotlin/com/itsaky/androidide/plugins/aiagentmcp/settings/McpSettingsHeadersToStoreTest.kt diff --git a/CLAUDE.md b/CLAUDE.md index 0cfb856c..e34fe35f 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -49,7 +49,7 @@ Both jars are referenced via `../libs/*.jar`. **Always use the repo-root `libs/` ### Credentials: use the host's `KeystoreSecretStore`, never your own crypto A plugin that stores a credential encrypts it with `com.itsaky.androidide.plugins.security.KeystoreSecretStore` -from `plugin-api.jar` (**26.35+** — set `plugin.min_ide_version` accordingly). It is `compileOnly` +from `plugin-api.jar` (**26.36+** — set `plugin.min_ide_version` accordingly). It is `compileOnly` like the rest of the API, so there is one implementation in the IDE's process rather than a copy compiled into each `.cgp`. Do not re-implement AES/GCM in a plugin; three AI plugins each grew a copy that started to diverge, which is what ADFA-5255 removed. diff --git a/ai-agent-gemini/README.md b/ai-agent-gemini/README.md index 128acc06..e5b5251d 100644 --- a/ai-agent-gemini/README.md +++ b/ai-agent-gemini/README.md @@ -29,7 +29,7 @@ an `x-goog-api-key` **header**, never in a URL query string. `security/SecureApiKeyStore.kt` holds only this plugin's Keystore alias (`cotg_ai_gemini_key_v1`); the AES/GCM itself is the IDE's `KeystoreSecretStore` -(`plugin-api`, since **26.35** — hence this plugin's `min_ide_version`), so there +(`plugin-api`, since **26.36** — hence this plugin's `min_ide_version`), so there is one implementation in the process rather than a copy per plugin. The alias stays per plugin: they all share the host's Keystore, so a shared alias would let one plugin's invalidated-key recovery delete another's secret. A key written under diff --git a/ai-agent-gemini/src/main/AndroidManifest.xml b/ai-agent-gemini/src/main/AndroidManifest.xml index cc46e585..e463434a 100644 --- a/ai-agent-gemini/src/main/AndroidManifest.xml +++ b/ai-agent-gemini/src/main/AndroidManifest.xml @@ -29,11 +29,12 @@ android:name="plugin.author" android:value="App Dev for All" /> - + + android:value="26.36" /> - + + android:value="26.36" /> ): Boolean { val clean = McpHeaders.sanitize(headers) @@ -250,7 +251,10 @@ object McpServerStore { clean.forEach { (name, value) -> json.put(name, value) } json.toString() } - val stored = secureTokenStore.write(prefs(), key, payload) + // A clear with no preferences behind it has nothing to remove, so it succeeded; only a + // write with something to store has genuinely failed. + val prefs = prefs() ?: return clean.isEmpty() + val stored = secureTokenStore.write(prefs, key, payload) fireChanged() return stored } diff --git a/ai-agent-mcp/src/main/kotlin/com/itsaky/androidide/plugins/aiagentmcp/settings/McpSettingsFragment.kt b/ai-agent-mcp/src/main/kotlin/com/itsaky/androidide/plugins/aiagentmcp/settings/McpSettingsFragment.kt index 6dc780dd..ade27415 100644 --- a/ai-agent-mcp/src/main/kotlin/com/itsaky/androidide/plugins/aiagentmcp/settings/McpSettingsFragment.kt +++ b/ai-agent-mcp/src/main/kotlin/com/itsaky/androidide/plugins/aiagentmcp/settings/McpSettingsFragment.kt @@ -164,10 +164,10 @@ class McpSettingsFragment : Fragment() { // Unknown, never Absent, until the decrypt answers: guessing is what let an http:// URL // be saved over a token that was still stored and still sent. var credential = if (existing == null) Credential.ABSENT else Credential.UNKNOWN - // Whether the header rows on screen are what is stored. False until the decrypt answers, - // and for headers this device could not read: an empty list then means "never drawn", and - // saving it would delete headers the dialog was in no position to show. - var headersKnown = existing == null + // What the header rows on screen are: the stored set, or not that. Anything but + // [Headers.KNOWN] means an empty list is "never drawn" rather than "none", and a typed row + // is no basis for replacing a set the dialog was in no position to show. + var headersState = if (existing == null) Headers.KNOWN else Headers.UNKNOWN nameField.setText(server.name) urlField.setText(server.url) @@ -186,7 +186,7 @@ class McpSettingsFragment : Fragment() { if (cleared) { credential = Credential.ABSENT // Nothing is stored now, so the empty list on screen is the stored truth again. - headersKnown = true + headersState = Headers.KNOWN } whileDialogShown { clearButton.isEnabled = !cleared @@ -211,7 +211,6 @@ class McpSettingsFragment : Fragment() { credential = if (form.hasToken || form.hasHeaders) Credential.PRESENT else Credential.ABSENT - headersKnown = form.headersKnown } whileDialogShown { // The stored token is never shown: it is decrypted only to be sent. An empty @@ -230,6 +229,14 @@ class McpSettingsFragment : Fragment() { // clear otherwise, and an unreadable secret is exactly what it is for. clearButton.visibility = if (credential == Credential.PRESENT) View.VISIBLE else View.GONE + // Recorded beside the render that draws them, not behind the credential guard + // above: these rows appear whatever that guard decided, and a deletion made + // once they are on screen must not be dropped as "never drawn". + headersState = when { + form.headersKnown -> Headers.KNOWN + form.headersUnavailable -> Headers.UNAVAILABLE + else -> Headers.UNREADABLE + } renderHeaders(view, form.headers) } } @@ -248,7 +255,7 @@ class McpSettingsFragment : Fragment() { return@setOnClickListener } val validation = - validate(candidate, tokenField.text.toString(), credential, headers) + validate(candidate, tokenField.text.toString(), credential, headers, headersState) if (validation != null) { status.text = validation return@setOnClickListener @@ -256,19 +263,12 @@ class McpSettingsFragment : Fragment() { // Stored first: connecting reads the URL and credentials from the store. status.text = getString(R.string.mcp_status_connecting) button.isEnabled = false - val headersToStore = viewModel.headersToStore(headers, headersKnown) viewModel.save( candidate, viewModel.tokenToStore(tokenField.text.toString()), - headersToStore, + viewModel.headersToStore(headers, headersState == Headers.KNOWN), ) { saved, failure -> server = saved - // Connect deliberately leaves the dialog open, so there can be a second save. - // Once these rows have been written they are the stored truth, and a row - // removed afterwards has to be a deletion rather than a skipped write. - if (headersToStore != null) { - headersKnown = true - } if (tokenField.text.isNotBlank() || headers.isNotEmpty()) { credential = Credential.PRESENT } @@ -323,7 +323,7 @@ class McpSettingsFragment : Fragment() { return@setOnClickListener } val problem = - validate(candidate, tokenField.text.toString(), credential, headers) + validate(candidate, tokenField.text.toString(), credential, headers, headersState) if (problem != null) { status.text = problem return@setOnClickListener @@ -331,7 +331,7 @@ class McpSettingsFragment : Fragment() { viewModel.save( candidate, viewModel.tokenToStore(tokenField.text.toString()), - viewModel.headersToStore(headers, headersKnown), + viewModel.headersToStore(headers, headersState == Headers.KNOWN), ) { _, _ -> } dialog.dismiss() } @@ -526,12 +526,21 @@ class McpSettingsFragment : Fragment() { typedToken: String, credential: Credential, headers: Map, + headersState: Headers, ): String? = when { server.name.isBlank() -> getString(R.string.mcp_name_required) server.url.isBlank() -> getString(R.string.mcp_url_required) !server.url.startsWith("http://") && !server.url.startsWith("https://") -> getString(R.string.mcp_url_scheme_invalid) !McpHeaders.isSendableToken(typedToken.trim()) -> getString(R.string.mcp_token_illegal) + // Saving a typed row would replace the whole stored set, so it is refused while the dialog + // cannot show what that set is — each state with the advice that actually helps it. + headers.isNotEmpty() && headersState == Headers.UNKNOWN -> + getString(R.string.mcp_credentials_loading) + headers.isNotEmpty() && headersState == Headers.UNAVAILABLE -> + getString(R.string.mcp_headers_unavailable_no_replace) + headers.isNotEmpty() && headersState == Headers.UNREADABLE -> + getString(R.string.mcp_headers_unreadable_no_replace) server.url.startsWith("http://") && credential == Credential.UNKNOWN -> getString(R.string.mcp_credentials_loading) server.url.startsWith("http://") && @@ -558,6 +567,28 @@ class McpSettingsFragment : Fragment() { ABSENT, } + /** + * What the header rows on screen are, which decides what saving them may do. + * + * The host's own four-way secret read, as this dialog sees it: only [KNOWN] rows may replace + * the stored set, and the three others each need their own sentence rather than one "could not + * be read" that sends a user with intact headers off to enter them again. + */ + private enum class Headers { + + /** The decrypt has not answered yet. */ + UNKNOWN, + + /** Stored headers this device's Keystore can no longer open. Permanent: clear and re-enter. */ + UNREADABLE, + + /** Stored headers the Keystore would not answer for this time. Transient: retry. */ + UNAVAILABLE, + + /** The rows on screen are the stored set — a new server, a successful read, or just cleared. */ + KNOWN, + } + /** * Runs [action] only while the dialog it draws on is still on screen. * diff --git a/ai-agent-mcp/src/main/kotlin/com/itsaky/androidide/plugins/aiagentmcp/settings/McpSettingsViewModel.kt b/ai-agent-mcp/src/main/kotlin/com/itsaky/androidide/plugins/aiagentmcp/settings/McpSettingsViewModel.kt index 6230b823..d1e50103 100644 --- a/ai-agent-mcp/src/main/kotlin/com/itsaky/androidide/plugins/aiagentmcp/settings/McpSettingsViewModel.kt +++ b/ai-agent-mcp/src/main/kotlin/com/itsaky/androidide/plugins/aiagentmcp/settings/McpSettingsViewModel.kt @@ -53,6 +53,8 @@ class McpSettingsViewModel( * the field says differently: the credential is intact and the read is worth repeating. * @property headersKnown whether [headers] is what is stored. False when the decrypt failed, in * which case an empty list on screen means "not shown" and must not be saved over them. + * @property headersUnavailable why the decrypt failed, when it did: true for a keystore that + * merely would not answer, so the dialog can say "try again" rather than "start over". * @property headers the extra headers configured for the server. */ data class FormState( @@ -61,6 +63,7 @@ class McpSettingsViewModel( val secretsUnreadable: Boolean, val secretsUnavailable: Boolean, val headersKnown: Boolean, + val headersUnavailable: Boolean, val headers: Map, ) @@ -109,6 +112,7 @@ class McpSettingsViewModel( secretsUnavailable = token is KeystoreSecretStore.Stored.Unavailable || headersUnavailable, headersKnown = headers != null, + headersUnavailable = headersUnavailable, headers = headers.orEmpty(), ) } @@ -126,17 +130,18 @@ class McpSettingsViewModel( /** * What the header rows mean when saving, given whether the stored ones could be read. * - * The token's rule, applied to headers: an empty list is "leave them alone" whenever the dialog - * never drew what is stored, because a keystore that would not answer must not cost the user - * headers that are still intact. Rows the user actually typed are always an explicit - * replacement, and [clearCredential] is the way back to none. + * The token's rule, applied to headers: the rows replace what is stored only when the dialog + * drew what is stored. Otherwise they mean nothing about it — [setHeaders] replaces the whole + * map, so writing one typed row over a set nobody could read would delete headers the user was + * just told were intact. The dialog refuses such a save instead, and [clearCredential] is the + * way back to none. * * @param collected the header rows as they currently stand. * @param headersKnown whether those rows reflect what is stored. - * @return the headers to store, or null to keep the stored ones. + * @return the rows to store, replacing the stored set, or null to leave it alone. */ fun headersToStore(collected: Map, headersKnown: Boolean): Map? = - if (headersKnown || collected.isNotEmpty()) collected else null + if (headersKnown) collected else null /** * Stores the edited name and URL of a server and, when given, its token. diff --git a/ai-agent-mcp/src/main/res/values/strings.xml b/ai-agent-mcp/src/main/res/values/strings.xml index e3067541..e5fc1a08 100644 --- a/ai-agent-mcp/src/main/res/values/strings.xml +++ b/ai-agent-mcp/src/main/res/values/strings.xml @@ -86,6 +86,8 @@ A header value cannot contain line breaks. That header is already listed above. Remove one of the two. The extra headers could not be encrypted, so they were not saved. + The headers already saved for this server couldn\'t be read just now, so a new one can\'t replace them. They are still stored — try again in a moment. + The headers already saved for this server can no longer be read on this device, so a new one can\'t replace them. Use Clear credential to replace them all. Fix the marked header before saving. Back diff --git a/ai-agent-mcp/src/test/kotlin/com/itsaky/androidide/plugins/aiagentmcp/plugin/McpPluginScopeTest.kt b/ai-agent-mcp/src/test/kotlin/com/itsaky/androidide/plugins/aiagentmcp/plugin/McpPluginScopeTest.kt index 22aa9231..7e2a542f 100644 --- a/ai-agent-mcp/src/test/kotlin/com/itsaky/androidide/plugins/aiagentmcp/plugin/McpPluginScopeTest.kt +++ b/ai-agent-mcp/src/test/kotlin/com/itsaky/androidide/plugins/aiagentmcp/plugin/McpPluginScopeTest.kt @@ -5,7 +5,11 @@ import com.itsaky.androidide.plugins.aiagentmcp.testing.FakeSharedPreferences import io.mockk.every import io.mockk.mockk import kotlinx.coroutines.Job +import java.util.concurrent.CyclicBarrier +import java.util.concurrent.Executors +import java.util.concurrent.TimeUnit import org.junit.After +import org.junit.Assert.assertEquals import org.junit.Assert.assertFalse import org.junit.Assert.assertNotSame import org.junit.Assert.assertTrue @@ -82,6 +86,51 @@ class McpPluginScopeTest { assertScopeUsable(requireNotNull(plugin.scopeJob)) } + /** + * The invariant [McpPlugin.lifecycleLock] exists for, which no sequential call can break. + * + * `activate` and `deactivate` each read the scope field, install their own, and cancel what + * they displaced. Let those two steps interleave and both calls can displace the *same* scope: + * the stop cancels the one the activation replaced, and the scope the activation handed its + * `tools/list` refresh to is left running with nothing holding it — a socket the plugin can no + * longer reach, repopulating a catalogue that was just cleared. + * + * So whatever order the two land in, the scope an activation launched on is either the one + * still installed (and live) or cancelled. Never a live orphan, and never a cancelled scope + * left installed as though the plugin were activated. + */ + @Test + fun givenTwoLifecycleCallsAtOnce_whenTheyInterleave_thenNoScopeIsOrphaned() { + val threads = Executors.newFixedThreadPool(2) + try { + repeat(ITERATIONS) { iteration -> + val barrier = CyclicBarrier(2) + // Both orders: which call reaches the field first is the whole question. + val activateFirst = iteration % 2 == 0 + val first = threads.submit { + barrier.await(TIMEOUT_SECONDS, TimeUnit.SECONDS) + if (activateFirst) plugin.activate() else plugin.deactivate() + } + val second = threads.submit { + barrier.await(TIMEOUT_SECONDS, TimeUnit.SECONDS) + if (activateFirst) plugin.deactivate() else plugin.activate() + } + first.get(TIMEOUT_SECONDS, TimeUnit.SECONDS) + second.get(TIMEOUT_SECONDS, TimeUnit.SECONDS) + + val installed = plugin.scopeJob + val launched = requireNotNull(plugin.activationJob) + assertEquals( + "the scope an activation launched on must be the installed one or cancelled", + installed !== launched, + launched.isCancelled, + ) + } + } finally { + threads.shutdownNow() + } + } + /** * Asserts a scope can still take work, which a cancelled one cannot. * @param job the activation scope's job. @@ -90,4 +139,12 @@ class McpPluginScopeTest { assertFalse("the current activation's scope must be live", job.isCancelled) assertTrue("the current activation's scope must accept work", job.isActive) } + + private companion object { + /** Enough interleavings to catch a lost swap; the whole loop runs in well under a second. */ + const val ITERATIONS = 400 + + /** Generous: a thread that never arrives is a deadlock, not a slow machine. */ + const val TIMEOUT_SECONDS = 10L + } } diff --git a/ai-agent-mcp/src/test/kotlin/com/itsaky/androidide/plugins/aiagentmcp/settings/McpSettingsHeadersToStoreTest.kt b/ai-agent-mcp/src/test/kotlin/com/itsaky/androidide/plugins/aiagentmcp/settings/McpSettingsHeadersToStoreTest.kt new file mode 100644 index 00000000..7e3ddb30 --- /dev/null +++ b/ai-agent-mcp/src/test/kotlin/com/itsaky/androidide/plugins/aiagentmcp/settings/McpSettingsHeadersToStoreTest.kt @@ -0,0 +1,43 @@ +package com.itsaky.androidide.plugins.aiagentmcp.settings + +import org.junit.Assert.assertEquals +import org.junit.Assert.assertNull +import org.junit.Test + +/** + * What the header rows mean when the dialog saves them. + * + * `setHeaders` replaces the whole stored map, so this predicate is the only thing standing between + * a keystore that would not answer and a user losing headers they were just told were intact: the + * rows on screen may replace what is stored only when the dialog drew what is stored. + */ +class McpSettingsHeadersToStoreTest { + + private val viewModel = McpSettingsViewModel { null } + + private val typed = mapOf("X-Api-Key" to "abc") + + @Test + fun givenTheStoredHeadersWereDrawn_whenSavingRows_thenTheyReplaceThem() { + assertEquals(typed, viewModel.headersToStore(typed, headersKnown = true)) + } + + @Test + fun givenTheStoredHeadersWereDrawn_whenSavingNoRows_thenTheyAreDeleted() { + // The user removed every row they could see, so an empty map is the intended replacement. + assertEquals(emptyMap(), viewModel.headersToStore(emptyMap(), headersKnown = true)) + } + + @Test + fun givenTheStoredHeadersCouldNotBeRead_whenSavingNoRows_thenTheyAreLeftAlone() { + // An empty list here means "never drawn", not "none": nothing may be written over them. + assertNull(viewModel.headersToStore(emptyMap(), headersKnown = false)) + } + + @Test + fun givenTheStoredHeadersCouldNotBeRead_whenSavingATypedRow_thenTheyAreStillLeftAlone() { + // The regression this pins: one typed row used to be an implicit replacement of a set the + // user never saw, which deleted the rest of it. The dialog refuses such a save instead. + assertNull(viewModel.headersToStore(typed, headersKnown = false)) + } +} diff --git a/ai-agent-openai/README.md b/ai-agent-openai/README.md index 182ed5ca..d79bb0f5 100644 --- a/ai-agent-openai/README.md +++ b/ai-agent-openai/README.md @@ -99,7 +99,7 @@ to belong elsewhere. The connection test applies the same rule. `security/SecureApiKeyStore.kt` holds only this plugin's Keystore alias (`cotg_ai_openai_key_v1`); the AES/GCM itself is the IDE's `KeystoreSecretStore` -(`plugin-api`, since **26.35** — hence this plugin's `min_ide_version`), so there +(`plugin-api`, since **26.36** — hence this plugin's `min_ide_version`), so there is one implementation in the process rather than a copy per plugin. The **alias** is deliberately not shared: every plugin runs in the host app's process and UID and therefore shares one Keystore, so a shared alias would let one plugin's diff --git a/ai-agent-openai/src/main/AndroidManifest.xml b/ai-agent-openai/src/main/AndroidManifest.xml index 6bad27a0..8684f20e 100644 --- a/ai-agent-openai/src/main/AndroidManifest.xml +++ b/ai-agent-openai/src/main/AndroidManifest.xml @@ -33,11 +33,12 @@ android:name="plugin.author" android:value="App Dev for All" /> - + + android:value="26.36" /> Date: Wed, 2 Sep 2026 09:44:32 -0500 Subject: [PATCH 5/5] fix(ai): keep the agent's answer and stop the token field speaking for headers Round-4 review: a turn carrying a terminal call is never a duplicate, the MCP form reports each credential's read separately, header rows are redrawn only when the stored set was read, Save waits for the write, and the scope test's KDoc names what it pins. --- .../settings/McpSettingsFragment.kt | 50 ++++++++++++++----- .../settings/McpSettingsViewModel.kt | 31 ++++++------ ai-agent-mcp/src/main/res/values/strings.xml | 2 +- .../aiagentmcp/plugin/McpPluginScopeTest.kt | 15 ++++-- .../aicore/viewmodel/AgentReplyRenderer.kt | 12 +++-- .../viewmodel/AgentReplyRendererTest.kt | 20 +++++--- 6 files changed, 86 insertions(+), 44 deletions(-) diff --git a/ai-agent-mcp/src/main/kotlin/com/itsaky/androidide/plugins/aiagentmcp/settings/McpSettingsFragment.kt b/ai-agent-mcp/src/main/kotlin/com/itsaky/androidide/plugins/aiagentmcp/settings/McpSettingsFragment.kt index ade27415..9cc7da16 100644 --- a/ai-agent-mcp/src/main/kotlin/com/itsaky/androidide/plugins/aiagentmcp/settings/McpSettingsFragment.kt +++ b/ai-agent-mcp/src/main/kotlin/com/itsaky/androidide/plugins/aiagentmcp/settings/McpSettingsFragment.kt @@ -215,15 +215,23 @@ class McpSettingsFragment : Fragment() { whileDialogShown { // The stored token is never shown: it is decrypted only to be sent. An empty // field on an existing server means "leave it alone", which the placeholder - // says aloud — unless nothing on this device can read it any more. - if (form.secretsUnreadable) { - tokenField.hint = getString(R.string.mcp_hint_token_unreadable) - status.text = getString(R.string.mcp_secrets_unreadable) - } else if (form.secretsUnavailable) { - tokenField.hint = getString(R.string.mcp_hint_token_unavailable) - status.text = getString(R.string.mcp_secrets_unavailable) - } else if (form.hasToken) { - tokenField.hint = getString(R.string.mcp_hint_token_stored) + // says aloud — unless nothing on this device can read it any more. The hint + // speaks for the token alone; headers that failed have their own sentence. + tokenField.hint = getString( + when { + form.tokenUnreadable -> R.string.mcp_hint_token_unreadable + form.tokenUnavailable -> R.string.mcp_hint_token_unavailable + form.hasToken -> R.string.mcp_hint_token_stored + else -> R.string.mcp_hint_token + } + ) + // The status line covers whichever credential failed. Unreadable first: a + // credential that has to be entered again is the worse news. + when { + form.tokenUnreadable || form.headersUnreadable -> + status.text = getString(R.string.mcp_secrets_unreadable) + form.tokenUnavailable || form.headersUnavailable -> + status.text = getString(R.string.mcp_secrets_unavailable) } // Offered only once something is known to be stored; there is nothing to // clear otherwise, and an unreadable secret is exactly what it is for. @@ -237,7 +245,10 @@ class McpSettingsFragment : Fragment() { form.headersUnavailable -> Headers.UNAVAILABLE else -> Headers.UNREADABLE } - renderHeaders(view, form.headers) + // Only the stored set replaces what is on screen. A failed decrypt has nothing + // to draw, and clearing the list would take with it any row typed while it was + // in flight — rows the save guard below then refuses to let be retyped. + if (form.headersKnown) renderHeaders(view, form.headers) } } } @@ -311,7 +322,8 @@ class McpSettingsFragment : Fragment() { serverDialog = dialog dialog.setOnDismissListener { serverDialog = null } dialog.setOnShowListener { - dialog.getButton(AlertDialog.BUTTON_POSITIVE)?.setOnClickListener { + val saveButton = dialog.getButton(AlertDialog.BUTTON_POSITIVE) ?: return@setOnShowListener + saveButton.setOnClickListener { val candidate = server.copy( name = nameField.text.toString().trim(), url = urlField.text.toString().trim(), @@ -328,12 +340,24 @@ class McpSettingsFragment : Fragment() { status.text = problem return@setOnClickListener } + saveButton.isEnabled = false viewModel.save( candidate, viewModel.tokenToStore(tokenField.text.toString()), viewModel.headersToStore(headers, headersState == Headers.KNOWN), - ) { _, _ -> } - dialog.dismiss() + ) { _, failure -> + // Dismissed only once the credential is stored, as Connect already does: a + // keystore that will not encrypt would otherwise lose the token behind a + // dialog that closed as though it had saved. + if (failure == null) { + whileDialogShown { dialog.dismiss() } + return@save + } + whileDialogShown { + status.text = failure + saveButton.isEnabled = true + } + } } } dialog.show() diff --git a/ai-agent-mcp/src/main/kotlin/com/itsaky/androidide/plugins/aiagentmcp/settings/McpSettingsViewModel.kt b/ai-agent-mcp/src/main/kotlin/com/itsaky/androidide/plugins/aiagentmcp/settings/McpSettingsViewModel.kt index d1e50103..9e5f1ee1 100644 --- a/ai-agent-mcp/src/main/kotlin/com/itsaky/androidide/plugins/aiagentmcp/settings/McpSettingsViewModel.kt +++ b/ai-agent-mcp/src/main/kotlin/com/itsaky/androidide/plugins/aiagentmcp/settings/McpSettingsViewModel.kt @@ -47,22 +47,25 @@ class McpSettingsViewModel( * @property hasHeaders whether headers are stored, from key presence rather than a decrypt: * [headers] comes back empty for headers this device can no longer read, and "stored but * unreadable" has to count as a stored credential or the control that clears it hides. - * @property secretsUnreadable whether a stored token or header cannot be decrypted on this - * device, which the field has to say aloud: it looks stored, but nothing can send it. - * @property secretsUnavailable whether the keystore merely would not answer this time, which - * the field says differently: the credential is intact and the read is worth repeating. + * @property tokenUnreadable whether the stored token cannot be decrypted on this device, which + * the field has to say aloud: it looks stored, but nothing can send it. + * @property tokenUnavailable whether the keystore merely would not answer for the token, which + * the field says differently: it is intact and the read is worth repeating. * @property headersKnown whether [headers] is what is stored. False when the decrypt failed, in * which case an empty list on screen means "not shown" and must not be saved over them. - * @property headersUnavailable why the decrypt failed, when it did: true for a keystore that - * merely would not answer, so the dialog can say "try again" rather than "start over". + * @property headersUnreadable why the header decrypt failed, when it did: true when nothing on + * this device can read them again. + * @property headersUnavailable why the header decrypt failed, when it did: true for a keystore + * that merely would not answer, so the dialog can say "try again" rather than "start over". * @property headers the extra headers configured for the server. */ data class FormState( val hasToken: Boolean, val hasHeaders: Boolean, - val secretsUnreadable: Boolean, - val secretsUnavailable: Boolean, + val tokenUnreadable: Boolean, + val tokenUnavailable: Boolean, val headersKnown: Boolean, + val headersUnreadable: Boolean, val headersUnavailable: Boolean, val headers: Map, ) @@ -102,16 +105,16 @@ class McpSettingsViewModel( headersUnavailable = true null } + // Reported per credential, never folded together: a token that decrypts beside + // headers that do not must not put "could not be read" on the token's own field, + // which is the mis-attribution this pane exists to avoid. FormState( hasToken = McpServerStore.hasToken(id), hasHeaders = McpServerStore.hasHeaders(id), - secretsUnreadable = - token is KeystoreSecretStore.Stored.Unreadable || headersUnreadable, - // Both can be true; the dialog shows the unreadable message first, since a - // credential that has to be entered again is the worse news. - secretsUnavailable = - token is KeystoreSecretStore.Stored.Unavailable || headersUnavailable, + tokenUnreadable = token is KeystoreSecretStore.Stored.Unreadable, + tokenUnavailable = token is KeystoreSecretStore.Stored.Unavailable, headersKnown = headers != null, + headersUnreadable = headersUnreadable, headersUnavailable = headersUnavailable, headers = headers.orEmpty(), ) diff --git a/ai-agent-mcp/src/main/res/values/strings.xml b/ai-agent-mcp/src/main/res/values/strings.xml index e5fc1a08..987321b2 100644 --- a/ai-agent-mcp/src/main/res/values/strings.xml +++ b/ai-agent-mcp/src/main/res/values/strings.xml @@ -67,7 +67,7 @@ The call to %1$s was cancelled. %1$s redirected the request to another address, so it was not sent — it carries your token. Check the endpoint URL. %1$s\'s saved token can no longer be read on this device. Open MCP server settings and enter it again. - %1$s\'s saved token could not be read just now. It is still stored — try again in a moment. + %1$s\'s saved credentials could not be read just now. They are still stored — try again in a moment. Could not reach %1$s. Could not reach %1$s: %2$s \'%1$s\' is no longer offered by any configured MCP server. diff --git a/ai-agent-mcp/src/test/kotlin/com/itsaky/androidide/plugins/aiagentmcp/plugin/McpPluginScopeTest.kt b/ai-agent-mcp/src/test/kotlin/com/itsaky/androidide/plugins/aiagentmcp/plugin/McpPluginScopeTest.kt index 7e2a542f..beeddf7e 100644 --- a/ai-agent-mcp/src/test/kotlin/com/itsaky/androidide/plugins/aiagentmcp/plugin/McpPluginScopeTest.kt +++ b/ai-agent-mcp/src/test/kotlin/com/itsaky/androidide/plugins/aiagentmcp/plugin/McpPluginScopeTest.kt @@ -87,17 +87,22 @@ class McpPluginScopeTest { } /** - * The invariant [McpPlugin.lifecycleLock] exists for, which no sequential call can break. + * What the swap onto [McpPlugin.stoppedScope] buys, under two lifecycle calls at once. * * `activate` and `deactivate` each read the scope field, install their own, and cancel what - * they displaced. Let those two steps interleave and both calls can displace the *same* scope: - * the stop cancels the one the activation replaced, and the scope the activation handed its - * `tools/list` refresh to is left running with nothing holding it — a socket the plugin can no - * longer reach, repopulating a catalogue that was just cleared. + * they displaced. Cancelling the field in place instead — the shape before this commit — leaves + * a cancelled scope installed as though the plugin were activated, so the scope an activation + * handed its `tools/list` refresh to is left running with nothing holding it: a socket the + * plugin can no longer reach, repopulating a catalogue that was just cleared. * * So whatever order the two land in, the scope an activation launched on is either the one * still installed (and live) or cancelled. Never a live orphan, and never a cancelled scope * left installed as though the plugin were activated. + * + * What this does *not* pin is [McpPlugin.lifecycleLock]: removing the `synchronized` from + * `swapScope` leaves every case here green, because the read-then-write window is too narrow + * for these threads to land inside. Reverting the swap fails it deterministically — including + * on the sequential activate-then-deactivate case above, which is the failure it really covers. */ @Test fun givenTwoLifecycleCallsAtOnce_whenTheyInterleave_thenNoScopeIsOrphaned() { diff --git a/ai-core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/viewmodel/AgentReplyRenderer.kt b/ai-core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/viewmodel/AgentReplyRenderer.kt index a4570e26..f98fe320 100644 --- a/ai-core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/viewmodel/AgentReplyRenderer.kt +++ b/ai-core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/viewmodel/AgentReplyRenderer.kt @@ -15,9 +15,11 @@ object AgentReplyRenderer { /** * Whether this turn only repeats the calls that already succeeded, so its bubble is dropped. * - * The terminal call is filtered out by the same loose match a handler is routed by, not by - * equality: a backend answering `Respond` would otherwise leave it among the real calls, so an - * identical repeat never compares equal and the duplicate bubble is never dropped. + * A turn carrying the terminal call is never a duplicate, whatever else it repeats: that bubble + * is the only place the answer is rendered. `AgentLoop` runs the real calls first and so never + * reaches `onFinalAnswer`, and dropping the turn here would end the run "completed" with + * nothing on screen. The match is the loose one a handler is routed by, so a backend answering + * `Respond` is recognised as terminal too. * * @param toolCalls the calls parsed out of this turn. * @param lastSucceededCalls the calls this run last executed successfully, null when none did. @@ -29,8 +31,8 @@ object AgentReplyRenderer { lastSucceededCalls: List?, terminalTool: String, ): Boolean { - val realCalls = toolCalls.filterNot { isTerminalToolName(it.name, terminalTool) } - return realCalls.isNotEmpty() && realCalls == lastSucceededCalls + if (toolCalls.any { isTerminalToolName(it.name, terminalTool) }) return false + return toolCalls.isNotEmpty() && toolCalls == lastSucceededCalls } /** diff --git a/ai-core/src/test/kotlin/com/itsaky/androidide/plugins/aicore/viewmodel/AgentReplyRendererTest.kt b/ai-core/src/test/kotlin/com/itsaky/androidide/plugins/aicore/viewmodel/AgentReplyRendererTest.kt index 30f6def2..dcde3399 100644 --- a/ai-core/src/test/kotlin/com/itsaky/androidide/plugins/aicore/viewmodel/AgentReplyRendererTest.kt +++ b/ai-core/src/test/kotlin/com/itsaky/androidide/plugins/aicore/viewmodel/AgentReplyRendererTest.kt @@ -128,14 +128,22 @@ class AgentReplyRendererTest { } @Test - fun givenARepeatedCallBesideACapitalisedRespond_whenChecked_thenTheTurnIsStillADuplicate() { - // What `it.name == RESPOND_TOOL` got wrong: a backend that answers `Respond` left the - // terminal call among the real ones, so the repeat never compared equal and the agent's - // duplicate turn reached the transcript. + fun givenARepeatedCallBesideACapitalisedRespond_whenChecked_thenTheAnswerIsKept() { + // The turn repeats a call that already succeeded, but it also carries the answer, and this + // bubble is the only place that text is ever rendered: dropping it ends the run + // "completed" with nothing on screen. `Respond` counts as terminal by the loose match. val succeeded = listOf(ToolCall("read_file", mapOf("file_path" to "A.kt"))) - val repeated = succeeded + ToolCall("Respond", mapOf("message" to "Done.")) + val repeated = succeeded + ToolCall("Respond", mapOf("message" to "Here is the answer")) - assertTrue(AgentReplyRenderer.isDuplicateTurn(repeated, succeeded, TERMINAL)) + assertFalse(AgentReplyRenderer.isDuplicateTurn(repeated, succeeded, TERMINAL)) + } + + @Test + fun givenOnlyRepeatedRealCalls_whenChecked_thenTheTurnIsADuplicate() { + // Nothing terminal, nothing new: the bubble would say what the last one already did. + val succeeded = listOf(ToolCall("read_file", mapOf("file_path" to "A.kt"))) + + assertTrue(AgentReplyRenderer.isDuplicateTurn(succeeded.toList(), succeeded, TERMINAL)) } @Test