ADFA-5270 | Adopt shared secret store, fix terminal-tool dedup, and cover MCP flows

[jatezzz]

Description

This PR addresses duplicated cryptography logic, fixes an agent loop bug, and improves test coverage:

• Cryptography Centralization: Delegates AES/GCM encryption for ai-agent-mcp, ai-agent-gemini, and ai-agent-openai to the host's KeystoreSecretStore. The duplicate AndroidKeyStore references have been removed, and plugin.min_ide_version is bumped to 26.35. Each plugin retains its existing distinct alias so previously stored credentials migrate seamlessly.
• Tool Deduplication: Replaces the hardcoded it.name == RESPOND_TOOL check in ChatViewModel with the isTerminalToolName helper, ensuring backend variations like "Respond" are properly filtered out and the duplicate-turn check matches correctly.
• MCP Coverage & KDoc: Adds JVM test coverage for MCP session keep-alive across calls (McpSessionLifecycleTest), McpServerStore preferences lock, and McpPlugin scope cancellation. Corrects the McpServerStore.hasHeaders KDoc to accurately reflect its boolean return type.

Details

• All AI plugins build successfully against the refreshed libs/.
• JVM test suites pass, providing safe, host-independent validation of MCP connection logic and thread-safe preference locks.

Screen_Recording_20260825_162828_Code.on.the.Go.mp4

Ticket

ADFA-5270
Parent: ADFA-5255

Observation

By eliminating the redundant AES/GCM implementations, we prevent further code drift between the three AI plugins. The deduplication fix will prevent agents from getting stuck in terminal tool loops.

[claude]

Claude Code Review

This repository is configured for manual code reviews. Comment @claude review for a one-time review, or @claude review always to subscribe this PR to a review on every future push.

_{Tip: disable this comment in your organization's Code Review settings.}

[itsaky-adfa]

Review of the shared-secret-store adoption

Verified against head af606a7, with the host side read from CodeOnTheGo#1757 (ADFA-5269) rather than assumed.

Two things block this. First, it does not compile: KeystoreSecretStore is imported by twelve files and is in neither libs/plugin-api.jar at this head nor CodeOnTheGo@origin/stage. check-toolchain.yml is the only workflow that runs on a PR here, so the green check never built anything.

Second, a theme rather than a single bug: the fourth Stored state was wired through every read path and none of the write paths. Three separate places now tell the user their credential is intact and transient, then destroy it on the next tap - the MCP header dialog, the OpenAI key pane, and (as a durability mismatch) setHeaders' clear branch. Worth fixing as one pass rather than three.

What checked out. The three aliases are unchanged from main and the host writes the same enc:v1: + base64(12-byte IV | ciphertext) AES/GCM-128 format the deleted copies wrote, so "migrate seamlessly" holds - I diffed both formats rather than taking the claim. The four-arm whens match the host's Stored exactly. executeToolCalls only ever receives terminal-filtered calls, so lastSucceededCalls and the new realCalls predicate agree. McpPlugin's swap-under-lock is correct through every activate/deactivate interleaving I traced, including the shared already-cancelled stoppedScope. UnavailableSecretException is thrown, caught and classified at every site, with no catch (UnreadableSecretException) left to miss it and no McpFailure consumer outside the formatter. The new McpSessionLifecycleTest cases pin behaviour McpSession actually has - the 404 re-init retry, close(), deleteSession. min_ide_version is 26.35 on all three; ai-agent-mcp was already there.

Not verified. No build and no JVM suite run: the missing host class makes all three modules uncompilable here. Everything above is from reading the code, the jars, and the host PR's API dump.

Verdict. Requesting changes. This repo has no written approve/request-changes rule - no REVIEW.md, CONTRIBUTING.md or PR template, and CLAUDE.md covers build and libs conventions only - so the default applied: any confirmed CRITICAL or IMPORTANT blocks; the five MINORs and two NITPICKs do not.

No earlier inline review comments existed to re-check, only the claude[bot] boilerplate.

[itsaky-adfa]

Requesting changes on one blocker and one theme.

Blocker: the branch does not compile. KeystoreSecretStore is not in libs/plugin-api.jar at this head, nor on CodeOnTheGo@origin/stage. Land CodeOnTheGo#1757 (ADFA-5269), then commit the refreshed libs/ here alongside the code.

Theme: the new Unavailable state reached every read path and no write path. Three places tell the user the credential is intact and then destroy it on the next tap - the MCP header dialog, the OpenAI key pane, and setHeaders' clear branch. One pass, not three fixes.

The five MINORs and two NITPICKs are not blocking; the CLAUDE.md one is worth taking with the rest, since it is what the next plugin author reads.

The refactor itself is right: aliases preserved, ciphertext format identical, Stored handled exhaustively, McpPlugin's scope swap correct under concurrency.

[itsaky-adfa]

Round 2 - re-check of the eleven findings, plus one new one

Verified at head 03a81e2 by reading the code, not the replies. The host side was read from
CodeOnTheGo@origin/stage (which now carries #1757) rather than assumed.

Nine of eleven are genuinely fixed. This was a thorough round of work and most of it holds up
under re-derivation - in particular the headersKnown sentinel, the keptConfigured pair applied
to both OpenAI and Gemini, and the extracted isDuplicateTurn with a test that really does fail
against it.name == RESPOND_TOOL.

#	Prior finding	Status at head
1	CRITICAL KeystoreSecretStore not on the classpath	Still open - see the thread
2	IMPORTANT Save deletes headers it said were safe	Fixed - FormState.headersKnown + headersToStore, both save sites
3	IMPORTANT MCP token lost its trim	Fixed - stored.plain.trim() in credentialsFor
4	IMPORTANT Unavailable dresses the pane as never-configured	Fixed - keptConfigured on load, Edit toasts and returns; applied to Gemini too
5	MINOR CLAUDE.md documents a three-way Stored	Fixed - four-item list, Unavailable called out as transient
6	MINOR setHeaders empty-map branch overclaims	Fixed - routed through write(); confirmed against the host's write, which does editor.remove(key).commit() on a blank
7	MINOR SecretUnavailable arm untested	Fixed - givenAKeystoreThatWouldNotAnswer_... added beside its sibling
8	MINOR comment sweep	Partly fixed - see the thread
9	MINOR terminal-tool dedup untested	Fixed - AgentReplyRenderer.isDuplicateTurn + 4 tests, wired at ChatViewModel.kt:1011
10	NITPICK two-argument constructor	Fixed - confirmed: the two-arg form is internal, the one-arg is a public secondary constructor
11	NITPICK FakeSharedPreferences no-op getters	Fixed - one ConcurrentHashMap behind every typed accessor

The two still-open ones are replies in their existing threads, and I have reopened those two
threads. The other nine stay resolved.

Claims in the description I checked

• "Each plugin retains its existing distinct alias so previously stored credentials migrate
  seamlessly" - holds, and it is the claim most worth checking. All three aliases are
  byte-identical to the deleted code (cotg_ai_mcp_token_v1, cotg_ai_gemini_key_v1,
  cotg_ai_openai_key_v1), the token_/headers_ pref prefixes are unchanged, and the host's
  wire format matches the deleted one exactly: enc:v1: + Base64 NO_WRAP of a 12-byte IV
  followed by the ciphertext, AES/GCM/NoPadding, 128-bit tag. Existing ciphertext decrypts.
• "plugin.min_ide_version is bumped to 26.35" - holds, all three manifests.
• "Corrects the McpServerStore.hasHeaders KDoc" - holds.
• "All AI plugins build successfully against the refreshed libs/" and "JVM test suites pass" -
  true only of a local refresh that is not in this diff. That is finding 1.

Coverage of this review

I did not execute any build or test suite: ../libs/plugin-api.jar is both the compileOnly
and the testImplementation source, and the class 12 files import is not in it, so nothing in
ai-agent-* compiles in this worktree. The test logic was verified by reading. ai-core has no
dependency on the missing package and its suites would run given an SDK path, but I did not set
one up. Device verification (what this repo's CLAUDE.md calls the only real verification for a
plugin) has not happened here either.

This repo has no REVIEW.md or CONTRIBUTING.md and its CLAUDE.md states no
approve/request-changes rule, so the skill's default table applied: a confirmed CRITICAL or
IMPORTANT means REQUEST_CHANGES.

[itsaky-adfa]

Requesting changes on the one blocker that survived, plus one new defect.

Nine of the eleven findings from round 1 are genuinely fixed, and I re-derived each rather than reading the replies. The headersKnown sentinel, the keptConfigured pair across both OpenAI and Gemini, and the extracted isDuplicateTurn with a test that really does fail against it.name == RESPOND_TOOL are all good work.

Blocker (unchanged): the branch still does not compile as merged. KeystoreSecretStore is in none of the five jars in libs/, on this branch or on origin/main, and the PR does not touch libs/. CodeOnTheGo#1757 landed at 14:18 today, so this is now one dispatch away: run Update libs from CodeOnTheGo, rebase, and confirm the class is in the refreshed jar.

New: headersKnown is never set true after a successful save, so a second save in the same dialog silently skips a header deletion and the removed header keeps being sent.

Partly fixed: the manifest and build.gradle.kts comments were deleted rather than corrected, so all three plugins now carry a bare 26.35 with no record of why the floor moved.

I ran no build or test suite - the missing class makes that impossible in a clean worktree - so everything above is from reading the code, the host source at CodeOnTheGo@origin/stage, and the jars. Device verification has not happened either.

[itsaky-adfa]

Supplement: one more sibling site of the Unavailable bug

A second pass over the round-1 commit turned up one more instance of exactly the collapse this
PR set out to remove - the OpenAI Test connection path, which the keptConfigured fix did not
reach. Details in the inline comment. The verdict is unchanged and already recorded; this adds a
finding rather than revising the decision.

For the record, three other candidates did not survive checking and are deliberately not posted:

• getApiKeyFor returning an untrimmed key - a false alarm. verifyConnection does
  catalogGateway.listModels(apiKey.trim(), normalized) at OpenAiSettingsViewModel.kt:232, and
  that is the function's only caller, so nothing untrimmed reaches
  setRequestProperty("Authorization", "Bearer $apiKey").
• The host's readAndMigrate purging a blank legacy plaintext where the deleted store returned
  Value("") - a real behaviour change, but a correcting one. The old result made hasToken()
  true for an empty token; reporting Absent is the better answer, and it needs a blank
  pre-encryption value to be reachable at all.
• headersKnown being assigned inside the credential == UNKNOWN guard while renderHeaders below
  it is unconditional - not reachable today: the Clear credential button is gone until the load
  callback makes it visible, so nothing can race the late loadForm. The save-path defect on the
  same variable is already filed.

[itsaky-adfa]

Round 3 - the eleven-plus-two are fixed, two new blockers

Verified at head 3dfeabf by reading the code and the host repo, not the replies. libs/ was
read from the jar this branch actually merges, and the release history from CodeOnTheGo
tags.

The round-1 blocker is genuinely gone. 3dfeabf merges main, which carries the
refreshed jars: unzip -l libs/plugin-api.jar now lists
com/itsaky/androidide/plugins/security/ with KeystoreSecretStore, all four Stored
subclasses, and SecretKeySource. I checked the shape rather than the filename -
javap gives KeystoreSecretStore(String), write(SharedPreferences, String, String) and
readAndMigrate(SharedPreferences, String), and neither write nor readAndMigrate emits a
checkNotNullParameter for the SharedPreferences, so the nullable prefs() call sites
compile. The branch builds as merged.

All five findings left open after round 2 are fixed. Re-derived, not read off the replies:

• headersKnown after a save - the Connect callback now sets it, and the positive button
  dismisses immediately so no second save reaches it. Fixed, but see the new finding below:
  it is set on a write that failed.
• getApiKeyFor folding Unavailable - now returns the four-way Stored, with another
  origin's key reported as Absent and Unavailable returning early under
  msg_api_key_unavailable_for_test. Fixed.
• The deleted manifest comments - restored in all three manifests, and ai-agent-openai/build.gradle.kts
  matches gemini's wording. Fixed. I chased the caveat the openai comment dropped
  (ConfigurableBackend / getSystemPrompt): ADFA-5095 is contained in tags 26.34 and
  26.35, so the bump answers that open question rather than losing it. Correctly deleted.
• The [secureApiKeyStore] KDoc link - imported, resolves. Fixed.
• The FakeSharedPreferences no-op getters, the McpServerStore @return, the
  SecretUnavailable test, and the extracted isDuplicateTurn were all confirmed in round 2
  and still hold. givenARepeatedCallBesideACapitalisedRespond does fail against
  it.name == RESPOND_TOOL, as claimed.

Two new findings, both IMPORTANT, both in the round-2 fix commit efe0f60. One is a
release-sequencing error that the whole PR turns on; the other defeats the very guard the
round-2 fix added. Details inline.

Sibling sweep, so you know what was and was not checked: McpServerStore.headers() and
token() have exactly two call sites each and both handle the two exception types;
credentialsFor, ApiKeyCache.refresh and GeminiBackend.refreshKeyCache all trim
consistently; Gemini has no stored-key "test connection" path, so the OpenAI fix has no
Gemini sibling to miss; fetchGeminiModels collapses Unavailable deliberately and says so.

This repo has no written approve/request-changes rule - CLAUDE.md covers build, git flow
and verification but not review outcomes - so the default applied: any confirmed IMPORTANT
blocks.

[itsaky-adfa]

Supplement: five more, from a second pass over the header-save path and the new tests

Same head 3dfeabf. These are separate from the two above and each was re-derived here
rather than taken on trust; the scope-test one was checked by actually reverting the
production change.

One of them, the headersToStore non-empty branch, is the same class of defect as the
26.35 finding: the pane says the credential is intact and the next save destroys it. It is
worth reading against the paragraph this PR itself adds to CLAUDE.md - "nothing on that
screen may write over the credential it could not read". That rule is right, and this is the
last place the code does not follow it.

[itsaky-adfa]

Requesting changes on three findings. Everything from rounds 1 and 2 is genuinely fixed.

I re-derived each prior finding rather than reading the replies, and the compile blocker is gone for real: 3dfeabf merges main, the refreshed plugin-api.jar carries plugins/security/, and javap confirms the constructor and both method signatures the plugins call - including that write/readAndMigrate take a nullable SharedPreferences, so the prefs() call sites compile. I also chased the caveat the OpenAI manifest comment dropped and found it correctly deleted: ConfigurableBackend (ADFA-5095) is contained in tags 26.34 and 26.35, so the bump answers that open question rather than losing it. This was a thorough round of fixes.

What blocks:

1. The 26.35 floor names a release that does not contain KeystoreSecretStore. ADFA-5269 merged to CoGo stage on 2026-09-01 and is not on main; tag 26.35 was cut 2026-08-24 and is still the newest release. Nine sites now assert otherwise, CLAUDE.md among them - which is what the next plugin author will copy. The Action that refreshes libs/ also deploys .cgp files to the website, so merging and running it publishes three plugins that cannot run on any released IDE.

2. An added header deletes every stored header the dialog could not read. headersToStore returns the typed rows whenever they are non-empty, even with headersKnown = false, so the user adds one header to a pane that just told them their headers are still stored and the rest are replaced away. This is the one place the code does not follow the rule this PR itself adds to CLAUDE.md.

3. headersKnown is set on a header write that failed, which defeats the guard the round-2 fix added. The keystore that answers Unavailable on load is the same one that fails the write, so it is one event rather than two, and the next save deletes headers the dialog never drew.

Two MINORs and two NITPICKs alongside them, including one worth a look on its own: the four McpPluginScopeTest cases pass unchanged against the pre-PR McpPlugin, so they pin none of the locking this commit added. I verified that by reverting the production code, not by reading the tests.

None of the three is large. The first is a number and a sentence in nine places plus a release-ordering decision; the other two are a branch and a condition in the header-save path.

[itsaky-adfa]

Round 4 - the seven round-3 findings are fixed; one new IMPORTANT

Verified at head 8a06a9a by reading the code and running the tests, not by reading the replies.

All seven from round 3 hold up. The 26.36 floor now names a real release - git tag --contains b4b318dd7 returns 26.36, so the caveat in your reply is resolved rather than outstanding. headersKnown's post-save promotion is gone entirely rather than gated. headersToStore is if (headersKnown) collected else null, with a four-way Headers state behind it and four tests, the last of which pins the exact regression. The load's state is recorded beside renderHeaders, outside the credential guard. setHeaders returns clean.isEmpty() for a null prefs. Both edit fields trim, and every Stored.Value read site now does except the two hasKey reads and the headers JSON blob, which are right not to.

I checked the migration claim rather than taking it: javap on the merged plugin-api.jar gives enc:v1:, AES/GCM/NoPadding and a 12-byte IV - byte-identical to the deleted plugin code - and all three aliases survive. All four modules compile against the merged libs/, and 653 JVM tests pass (mcp 86, ai-core 397, openai 150, gemini 20).

What blocks: AgentReplyRenderer.isDuplicateTurn deletes the turn carrying the agent's answer. The loosened filter reduces [read_file, Respond("...")] to [read_file], the bubble is removed, and nothing else renders the respond text - so the run ends "completed" with no answer on screen. Lowercase respond already did this on main, so this widens an existing defect rather than creating one; it widens it onto exactly the backends the change was written for, and the new test pins the behaviour as correct. Details inline.

The five MINORs and the NITPICK do not block.

Finding without a diff anchor

MINOR: the PR description still says plugin.min_ide_version is bumped to 26.35. That is the value round 3 blocked on; head says 26.36 at all nine sites. QA works from this text, and a description that was true at commit 1 is false by commit 4.

Checked and deliberately not posted

• The change listener has no dedupe, so a double activate() leaks a settingsChanged registration. Real, but pre-existing (5723a6b) and untouched here.
• setHeaders skipping fireChanged() on the null-prefs return is asymmetric with setToken, which fires unconditionally. Nothing changed on disk, so there is nothing to notify - your reasoning on this one holds.