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..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.
@@ -59,7 +61,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..cc0402ef 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,38 @@ 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) {
+ // 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("")
- // 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 +402,33 @@ 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.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_unavailable),
+ Toast.LENGTH_LONG
+ ).show()
+ return@launch
+ }
+ if (stored is KeystoreSecretStore.Stored.Unreadable) {
Toast.makeText(
requireContext(),
getString(R.string.msg_api_key_unreadable),
Toast.LENGTH_LONG
).show()
}
- revealEditMode(apiKey.orEmpty())
+ revealEditMode((stored as? KeystoreSecretStore.Stored.Value)?.plain?.trim().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..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
@@ -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,15 +157,19 @@ 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.
+ * 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()
@@ -199,9 +204,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/README.md b/ai-agent-mcp/README.md
index 67a7fb87..d2e2ab5b 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.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 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/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
McpToolText | Sanitises server-supplied names and
descriptions — untrusted remote text that would otherwise land verbatim in a
prompt assembled inside a third-party backend plugin. |
- SecureTokenStore | AES/GCM encryption under a
- Keystore alias owned by this plugin, so only ciphertext reaches disk. |
+ SecureTokenStore | Binds 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/AndroidManifest.xml b/ai-agent-mcp/src/main/AndroidManifest.xml
index e85fb8c9..5c9eeee8 100644
--- a/ai-agent-mcp/src/main/AndroidManifest.xml
+++ b/ai-agent-mcp/src/main/AndroidManifest.xml
@@ -29,9 +29,12 @@
android:name="plugin.author"
android:value="App Dev for All" />
+
+ android:value="26.36" />
stored.plain
- SecureTokenStore.Stored.Absent -> ""
- SecureTokenStore.Stored.Unreadable ->
+ // 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.")
+ // 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..e9493e56 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,11 @@ 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())
+ activationJob = active.coroutineContext[Job]
toolSource = McpToolSource()
McpServerStore.addChangeListener(settingsChanged)
context.addPluginLifecycleListener(aiCoreLifecycle)
@@ -130,7 +151,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 +167,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 +179,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 +188,52 @@ 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]
+
+ /**
+ * The job of the scope the last [activate] launched its refresh on.
+ *
+ * The second half of the seam: [scopeJob] says what the field holds, and this says what an
+ * activation actually handed work to. A stop that displaces some other scope leaves the two
+ * disagreeing — a live refresh nothing can reach — which is the orphan [lifecycleLock] exists
+ * to prevent and the only way a test can see it.
+ */
+ @Volatile internal var activationJob: Job? = null
+ private set
+
/**
* 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..e00e6b38 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,24 +234,32 @@ 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. Forgetting them on a store that was never opened is success:
+ * there is nothing on disk to remove.
*/
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())
+ // 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
}
- /** 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..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
@@ -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
+ // 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)
@@ -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.
+ headersState = Headers.KNOWN
+ }
whileDialogShown {
clearButton.isEnabled = !cleared
clearButton.visibility = if (cleared) View.GONE else View.VISIBLE
@@ -207,18 +215,40 @@ 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.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.
clearButton.visibility =
if (credential == Credential.PRESENT) View.VISIBLE else View.GONE
- renderHeaders(view, form.headers)
+ // 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
+ }
+ // 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)
}
}
}
@@ -236,7 +266,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
@@ -247,7 +277,7 @@ class McpSettingsFragment : Fragment() {
viewModel.save(
candidate,
viewModel.tokenToStore(tokenField.text.toString()),
- headers,
+ viewModel.headersToStore(headers, headersState == Headers.KNOWN),
) { saved, failure ->
server = saved
if (tokenField.text.isNotBlank() || headers.isNotEmpty()) {
@@ -292,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(),
@@ -304,17 +335,29 @@ 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
}
+ saveButton.isEnabled = false
viewModel.save(
candidate,
viewModel.tokenToStore(tokenField.text.toString()),
- headers,
- ) { _, _ -> }
- dialog.dismiss()
+ viewModel.headersToStore(headers, headersState == Headers.KNOWN),
+ ) { _, 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()
@@ -507,12 +550,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://") &&
@@ -539,6 +591,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 6b0c41d5..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
@@ -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
@@ -46,14 +47,26 @@ 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 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 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 tokenUnreadable: Boolean,
+ val tokenUnavailable: Boolean,
+ val headersKnown: Boolean,
+ val headersUnreadable: Boolean,
+ val headersUnavailable: Boolean,
val headers: Map,
)
@@ -81,16 +94,28 @@ 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
}
+ // 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 SecureTokenStore.Stored.Unreadable || headers == null,
+ tokenUnreadable = token is KeystoreSecretStore.Stored.Unreadable,
+ tokenUnavailable = token is KeystoreSecretStore.Stored.Unavailable,
+ headersKnown = headers != null,
+ headersUnreadable = headersUnreadable,
+ headersUnavailable = headersUnavailable,
headers = headers.orEmpty(),
)
}
@@ -105,6 +130,22 @@ 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: 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 rows to store, replacing the stored set, or null to leave it alone.
+ */
+ fun headersToStore(collected: Map, headersKnown: Boolean): Map? =
+ if (headersKnown) collected else null
+
/**
* Stores the edited name and URL of a server and, when given, its token.
*
@@ -113,20 +154,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/main/res/values/strings.xml b/ai-agent-mcp/src/main/res/values/strings.xml
index 39a48b28..987321b2 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 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.
@@ -83,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/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/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/plugin/McpPluginScopeTest.kt b/ai-agent-mcp/src/test/kotlin/com/itsaky/androidide/plugins/aiagentmcp/plugin/McpPluginScopeTest.kt
new file mode 100644
index 00000000..beeddf7e
--- /dev/null
+++ b/ai-agent-mcp/src/test/kotlin/com/itsaky/androidide/plugins/aiagentmcp/plugin/McpPluginScopeTest.kt
@@ -0,0 +1,155 @@
+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 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
+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))
+ }
+
+ /**
+ * 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. 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() {
+ 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.
+ */
+ 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)
+ }
+
+ 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/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/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-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..ef47fbca
--- /dev/null
+++ b/ai-agent-mcp/src/test/kotlin/com/itsaky/androidide/plugins/aiagentmcp/testing/FakeSharedPreferences.kt
@@ -0,0 +1,118 @@
+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 {
+
+ // 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
+ 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] as? String ?: defValue
+ }
+
+ @Suppress("UNCHECKED_CAST")
+ override fun getStringSet(key: String, defValues: MutableSet?): MutableSet? =
+ (values[key] as? Set)?.toMutableSet() ?: defValues
+
+ override fun getInt(key: String, defValue: Int): Int = values[key] as? Int ?: defValue
+
+ override fun getLong(key: String, defValue: Long): Long = values[key] as? Long ?: defValue
+
+ override fun getFloat(key: String, defValue: Float): Float = values[key] as? Float ?: defValue
+
+ override fun getBoolean(key: String, defValue: Boolean): Boolean =
+ values[key] as? 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 {
+ if (values == null) removals += key else puts[key] = LinkedHashSet(values)
+ return this
+ }
+
+ override fun putInt(key: String, value: Int): SharedPreferences.Editor {
+ puts[key] = value
+ return this
+ }
+
+ override fun putLong(key: String, value: Long): SharedPreferences.Editor {
+ puts[key] = value
+ return this
+ }
+
+ override fun putFloat(key: String, value: Float): SharedPreferences.Editor {
+ puts[key] = value
+ return this
+ }
+
+ override fun putBoolean(key: String, value: Boolean): SharedPreferences.Editor {
+ puts[key] = value
+ return 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..d79bb0f5 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.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
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
OpenAiErrorFormatter | Classifies a failure
(unknown model, rate limit, spent balance, refused key, outage, server not
running) so it can be reported as one translated sentence. |
- SecureApiKeyStore | AES/GCM encryption of the API
- key, under this plugin's own Keystore alias. |
+ SecureApiKeyStore | Binds 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/build.gradle.kts b/ai-agent-openai/build.gradle.kts
index 9fa1882e..9cd03321 100644
--- a/ai-agent-openai/build.gradle.kts
+++ b/ai-agent-openai/build.gradle.kts
@@ -71,9 +71,8 @@ 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.
+// 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-openai/src/main/AndroidManifest.xml b/ai-agent-openai/src/main/AndroidManifest.xml
index aefa3899..8684f20e 100644
--- a/ai-agent-openai/src/main/AndroidManifest.xml
+++ b/ai-agent-openai/src/main/AndroidManifest.xml
@@ -33,12 +33,12 @@
android:name="plugin.author"
android:value="App Dev for All" />
-
+
+ android:value="26.36" />
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..936c2038 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,39 @@ 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) {
+ // 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("")
- // 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 +586,33 @@ 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.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_unavailable),
+ Toast.LENGTH_LONG
+ ).show()
+ return@launch
+ }
+ if (stored is KeystoreSecretStore.Stored.Unreadable) {
Toast.makeText(
requireContext(),
getString(R.string.msg_api_key_unreadable),
Toast.LENGTH_LONG
).show()
}
- revealEditMode(apiKey.orEmpty())
+ revealEditMode((stored as? KeystoreSecretStore.Stored.Value)?.plain?.trim().orEmpty())
}
}
@@ -731,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 22a810f8..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
@@ -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,30 +289,39 @@ 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)
}
/**
- * 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 or it belongs to another server
+ * @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
+ if (savedFor != null && !BaseUrlPolicy.sameOrigin(savedFor, baseUrl)) {
+ return KeystoreSecretStore.Stored.Absent
+ }
return getApiKey()
}
/**
- * 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..daab0802 100644
--- a/ai-agent-openai/src/main/res/values/strings.xml
+++ b/ai-agent-openai/src/main/res/values/strings.xml
@@ -69,6 +69,8 @@
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.
+ 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
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..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
@@ -12,6 +12,29 @@ 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.
+ *
+ * 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.
+ * @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 {
+ if (toolCalls.any { isTerminalToolName(it.name, terminalTool) }) return false
+ return toolCalls.isNotEmpty() && toolCalls == 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 eb63fa7e..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
@@ -1004,8 +1004,7 @@ class ChatViewModel(
// Per-run flag (set by executeToolCalls), not a session-wide scan.
val lastToolFailed = lastToolFailedThisRun
- val realCalls = toolCalls.filterNot { 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..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
@@ -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,49 @@ class AgentReplyRendererTest {
fun givenNothingAtAll_whenRendered_thenTheFallbackIsShown() {
assertEquals(NO_RESPONSE, render(" "))
}
+
+ @Test
+ 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 "Here is the answer"))
+
+ 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
+ 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))
+ }
}