Skip to content

ADFA-5403 | Fix LlmInferenceService resolution for Voice-to-Code - #93

Open
jatezzz wants to merge 5 commits into
mainfrom
fix/ADFA-5403-resolve-llm-from-shared-services
Open

ADFA-5403 | Fix LlmInferenceService resolution for Voice-to-Code#93
jatezzz wants to merge 5 commits into
mainfrom
fix/ADFA-5403-resolve-llm-from-shared-services

Conversation

@jatezzz

@jatezzz jatezzz commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

Description

Updated SpeechToTextPlugin to resolve the LlmInferenceService from the correct global registry. Previously, the plugin failed to find the service in the local context, causing Voice-to-Code to insert raw transcribed text instead of generated code. The service is now resolved from SharedServices first, with the local context.services used as a fallback. Resolution is dynamically retried on every use to account for parallel loading delays when the AI Core activates.

Details

  • Added resolveLlmService() to cache successful lookups and retry upon failure.
  • Bound the code generation request to a 60-second timeout (GENERATION_TIMEOUT_SECONDS) to prevent UI freezes.
  • Implemented stripCodeFences() to remove markdown from the generated output so the editor receives raw code.
  • Migrated logging from Android Log to the plugin's context.logger.
Screen_Recording_20260903_155612_Code.on.the.Go.mp4

Ticket

ADFA-5403
Parent: ADFA-5402

…(ADFA-5403)

context.services is a per-plugin registry that never holds it, so Voice-to-Code always inserted the raw transcript. Resolve per use, bound generation with a timeout, strip markdown fences, and target the open file's language, not Kotlin.

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@hal-eisen-adfa hal-eisen-adfa left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated review (Claude Code), high and medium findings only. Low-severity notes (duplicate fence stripper, LinkageError on SharedServices, no unit tests) are left out.

F09 (Medium) — line 52, outside the diff, so no inline comment is possible.
private var llmService: LlmInferenceService? = null is a plain non-volatile var, but this PR turns it into a lazily-populated cache. activate() writes it on the main thread; handleTranscript reads it from scope.launch on Dispatchers.IO. There is no happens-before edge, so the IO thread can keep seeing null and re-resolve on every transcript. recordingState in this same class is already @Volatile.

Resolve the LLM service through getPluginService as well, and drop the
cached reference when AI Core unloads. Tune the generation request
(system prompt, temperature, maxTokens) instead of scraping fences off
the reply, size the timeout to that token budget, and await the future
cancellably so plugin teardown unwinds it. Rewrite the fence stripper to
handle a lead-in line, an unclosed fence and a one-line fenced reply.
Guard logging against an uninitialized context during teardown.
@jatezzz

jatezzz commented Sep 9, 2026

Copy link
Copy Markdown
Contributor Author

@hal-eisen-adfa F09: fixed. llmService is now @Volatile, like recordingState in the same class, so the IO threads in scope see the value activate() publishes.
dispose() and the new AI Core lifecycle listener (F03) are the only other writers.

@hal-eisen-adfa hal-eisen-adfa left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Second automated review pass (Claude Code, xhigh). All thirteen findings from the first pass are addressed in code and the plugin builds clean; thanks. Five new issues below, four of them in the code this round introduced.

F16 (the Speech-to-Text max_ide_version 26.30 vs ai-core min_ide_version 26.35 gap) is dropped — we do not enforce those ranges.

Restore IDLE state and the raw-transcript fallback when AI Core cancels the shared future, release listener/scope/service in deactivate(), and stop the fence stripper from inserting prose (F17) or eating real code (F18, F20).

@hal-eisen-adfa hal-eisen-adfa left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Third automated review pass (Claude Code, high). The core of the PR is correct. Six new findings, two of them in the code the second pass added.

override fun activate(): Boolean {
Log.i(TAG, "SpeechToTextPlugin activating...")
// deactivate() cancels the scope, so a re-enabled plugin needs a fresh one.
if (!scope.isActive) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

F21 (Medium) — activate() rebuilds the scope but never resets recordingState.

The field is not part of the scope, so it keeps the value it held when the plugin was disabled. Disable the plugin while a generation runs — the F08 timeout leaves a 132 s window — and re-enable it: the same instance still reads PROCESSING, currentIconRes() returns ic_processing, and the toolbar shows the spinner permanently.

No path can clear it. The coroutine whose finally posts setState(IDLE) was cancelled by teardown(), and startVoiceCapture() does not reset the state before it sets RECORDING.

Add recordingState = RecordingState.IDLE beside the scope rebuild.

} finally {
// Posted, not dispatched: a cancelled coroutine can no longer suspend, and the
// toolbar must leave the spinner even then.
runOnMain { setState(RecordingState.IDLE) }

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

F22 (Medium) — no re-entrancy guard between a running generation and a new capture.

isEnabledProvider = { hasOpenFile() } (line 219) ignores recordingState, and startVoiceCapture() (line 262) does not reject a non-IDLE state. So the mic button stays live while the spinner is up.

Tap the mic, speak, then tap it again during generation #1:

The user gets two insertions and a toolbar that does not match the state. Guard startVoiceCapture() on recordingState == RecordingState.IDLE, and reset here only when the state is still PROCESSING.

* @param lines the trimmed reply, split into lines
* @return the reply from its first code line on, or empty when it holds none
*/
private fun dropLeadingProse(lines: List<String>): String {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

F23 (Low) — dropLeadingProse drops leading prose only, but the KDoc promises more.

The doc says "a lead-in or a refusal is never written into the open file". The scan stops at the first code line and keeps everything after it, so trailing prose survives. For

Here is the code:
val x = 1
This adds one.

the return value is val x = 1\nThis adds one., and the English sentence goes into the .kt file.

SuggestionProvider.sanitizeCompletion avoids this because it keeps the first code line only. Either filter prose over the whole reply or narrow the KDoc claim.

// With code behind it the tag may be code itself, so require the tag we asked for and a
// remainder that starts a name - `c = a + b` and `bash -c "..."` are code, not info.
val startsName = rest.first().isLetter() || rest.first() == '_' || rest.first() == '@'
return if (startsName && tag.equals(language, ignoreCase = true)) rest else fenceLine

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

F24 (Low) — the F20 fix compares a fence tag against a host editor id, so it rarely matches.

language comes from IdeEditorService.getCurrentLanguageId(), which returns the host's editor id (kt, java). The model writes its own info string (kotlin, java). The two agree only by chance.

For a one-line reply ```kotlin println("hi")``` in a Kotlin file, tag is kotlin and language is kt, so equals fails and the whole line is returned. The literal word kotlin is inserted into the user's file — the defect F20 asked you to prevent, with a new trigger.

The multi-line case is safe, because rest.isEmpty() returns "" whatever the language. Map the editor id to its tag aliases, or accept any LANGUAGE_TAGS member here as the sibling does and keep the startsName test as the guard.

private suspend fun <T> CompletableFuture<T>.await(): T =
suspendCancellableCoroutine { cont ->
whenComplete { value, error ->
if (error == null) cont.resume(value) else cont.resumeWithException(error)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

F25 (Low) — a null future result is reported as a timeout.

generateCompletion returns a platform-typed CompletableFuture<LlmResponse!>, and cont.resume(value) passes a null through unguarded. withTimeoutOrNull also yields null on a real timeout, so both states reach the same if (response == null) at line 405 and log "Code generation timed out after 132s".

A backend that breaks its contract therefore reads as a slow model, and the 142 s budget looks wrong when it is not. Separate the two — check the future result inside the withTimeoutOrNull block.

* would still write into the user's file, host callbacks to a dead instance, and the
* cached router that pins AI Core's ClassLoader. Idempotent - dispose() follows deactivate().
*/
private fun teardown() {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

F26 (Low) — teardown() keeps the host UI service, which is then used after deactivate() returned.

It clears llmService but leaves editorService and uiService set. The finally in handleTranscript posts runOnMain { setState(IDLE) }, and setState calls uiService?.refreshToolbarActions().

Disable the plugin during a generation: scope.cancel() runs the finally, the post lands after deactivate() returned, and the call reaches into the host toolbar from a disabled plugin. runOnMain has no try/catch, so a host that rejects the call crashes the main thread.

Null editorService and uiService in teardown() beside llmService.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants