ADFA-5403 | Fix LlmInferenceService resolution for Voice-to-Code - #93
Conversation
…(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.
There was a problem hiding this comment.
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
left a comment
There was a problem hiding this comment.
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.
|
@hal-eisen-adfa F09: fixed. |
hal-eisen-adfa
left a comment
There was a problem hiding this comment.
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
left a comment
There was a problem hiding this comment.
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.
Reset state on activate, reject re-entrant captures, drop trailing prose, accept any fence tag, split null responses from timeouts, clear services.
itsaky-adfa
left a comment
There was a problem hiding this comment.
Fourth review pass (/pr-review high). Two IMPORTANT findings; the rest are advisory.
IMPORTANT
SpeechToTextPlugin.kt:536- an unlisted fence tag (dart,kts,rust, ...) is inserted as a line of codeSpeechToTextPlugin.kt:144-min_ide_version26.17 predates the three host APIs this PR adds, which shipped in 26.29
MINOR
SpeechToTextPlugin.kt:678-max_ide_version26.30 excludes every host AI Core runs onSpeechToTextPlugin.kt:700- the description's 60 s timeout is 132 s in the codeSpeechToTextPlugin.kt:521-isProsedeletes real content lines, not just lead-insSpeechToTextPlugin.kt:157-resolveLlmServicecan write a service back afterteardown()cleared it
NITPICK - 1 inline, not listed
Stack
This is the bottom layer: main <- #93 (c1e9b9b) <- #95 (000cc97). Comments are anchored to #93's head, but every claim above was verified against the tip 000cc97, since the stack merges atomically and the tip is the state that reaches main.
One finding was dropped for that reason, and it is worth naming. The new recordingState != RecordingState.IDLE guard in startVoiceCapture removes main's only recovery from a wedged SpeechRecognizer: on main, a second tap ran destroyRecognizer() and started fresh, so a recognizer that never delivers a terminal callback left the toolbar stuck on the waves icon with every tap answered by stt_busy. #95 repairs it in the same condition by adding elapsed < STALE_CAPTURE_MS (SpeechToTextPlugin.kt:318 at the tip), freeing the button after 147 s. It cannot reach main on its own, so it is not posted - but this layer does not stand alone on that point.
Re-check of the earlier rounds
All 26 prior findings (F01-F26) are fixed at c1e9b9b. Verified in the code, not from the replies:
- F01/F02/F18 fence parsing - I re-implemented
stripCodeFencesand ran the original repros:```println("hi")```yieldsprintln("hi"), an unclosed fence keeps everything that arrived, and code on the opening line survives a multi-line block. - F20/F24
stripLanguageInfo-```c = a + b```and```bash -c "..."are left intact, so the over-deletion is gone. The allowlist that replaced thelanguage` comparison is what the first IMPORTANT above is about - a new hole in the same function, not a regression of F20. - F03/F19 teardown -
deactivate()anddispose()both route through one idempotentteardown()that removes the listener, clears the three services and cancels the scope. - F04/F14 cancellation - the
CancellationExceptioncatch rethrows only when the job is genuinely cancelled, and thefinallypostssetState(IDLE)throughrunOnMainrather than suspending. - F05
languageis a required parameter read on the main thread. F06/F07await()replaces the blockingget, and thecancel(true)claim is gone from the comment. F08 the timeout is derived frommaxTokens(see the MINOR above). F09llmServiceis@Volatile. F10loggeris guarded by::context.isInitialized. F12systemPrompt/temperature/maxTokensare set on the request. F13 thegetPluginServicerung is present. F17/F23dropProsefilters the whole reply (see the MINOR above). F21/F22 state reset on activate, plus the busy guard. F25completedWithoutResponseseparates a null response from a timeout. F26editorService/uiServiceare cleared in teardown.
Build
../../gradlew assemblePlugin at c1e9b9b succeeds in a clean worktree and produces speech-to-text.cgp, with no Kotlin warnings. Per CLAUDE.md that is not verification: both IMPORTANT findings are device-visible and neither shows up in a build.
Rules applied
This repo has no REVIEW.md, CONTRIBUTING.md or PR template, and the cogo-plugin-review skill states no approve/request-changes rule, so CLAUDE.md is the only governing document and the default verdict rule applied.
itsaky-adfa
left a comment
There was a problem hiding this comment.
Requesting changes on the two IMPORTANT findings from the review above; the four MINOR ones and the nitpick do not block.
SpeechToTextPlugin.kt:536- gate theLANGUAGE_TAGSallowlist on the inline```tag code```form. On a plain opening fence line the remainder is an info string by definition, so drop it whatever the language, anddart,kts,rust,html,yamlandcmakestop being written into the file as code.plugins/Speech-to-Text/src/main/AndroidManifest.xml:34- raiseplugin.min_ide_versionto at least 26.29, whereSharedServices,getPluginServiceandaddPluginLifecycleListenerfirst shipped. 26.35 is the better value, since that is the lowest host AI Core supports.
Both are device-visible and neither shows up in assemblePlugin, which passes at c1e9b9b. Worth exercising the fence path on a non-Kotlin file (a .dart or build.gradle.kts) on a device before the next round, per the verification note in CLAUDE.md.
Drop an unlisted fence tag from the opening line, limit prose filtering to the reply's leading and trailing runs, guard the cached LLM router against teardown, and widen the IDE window to 26.29-26.99 to match the APIs used.
itsaky-adfa
left a comment
There was a problem hiding this comment.
Fourth pass, against head 96067fb.
Stack. This is the lower layer of a two-PR stack: main <- #93 <- #95 (ADFA-5404). Every claim below was checked against the stack tip e2df93b rather than this PR's own head, because the stack merges atomically. #95 touches this same file but not one line of stripCodeFences, dropProse, isProse, stripLanguageInfo, resolveLlmService or forgetLlmService, so nothing below is already repaired higher up.
My CHANGES_REQUESTED of 2026-09-14 should be lifted - both findings it named are fixed. Re-check of all seven prior findings, read at the tip rather than taken from the replies:
| Prior finding | State at head |
|---|---|
IMPORTANT stripCodeFences - a tag outside LANGUAGE_TAGS inserted as code |
Fixed. The !closesInline && !opener.contains(' ') gate drops a lone opener whatever it names. Re-ran my implementation: dart, kts, rust, html, yaml, cmake and the unlisted jsx all now yield only the body. |
IMPORTANT min_ide_version 26.17 |
Fixed - 26.29, where SharedServices, getPluginService and addPluginLifecycleListener first shipped. |
MINOR max_ide_version 26.30 excluded every AI Core host |
Fixed - 26.99 now covers ai-core's 26.35 floor. |
| MINOR PR description's "60-second timeout" | Fixed - the description states 132 s and shows the arithmetic. |
MINOR isProse deletes interior content lines |
Partly fixed - dropWhile/dropLastWhile saves interior lines; the first and last still go. Reply in thread. |
MINOR resolveLlmService can resurrect a cleared service |
Partly fixed - teardown() is ordered by serviceLock; forgetLlmService still writes unlocked. Reply in thread. |
NITPICK withContext(Dispatchers.IO) no-op |
Fixed - removed, with a comment saying why. |
This round - all MINOR, nothing above it:
MINOR
SpeechToTextPlugin.kt:509- an inline-closed fence absorbs the model's trailing explanation into the fileSpeechToTextPlugin.kt:286- the new busy guard removes the only recovery from a stuckRECORDINGstateSpeechToTextPlugin.kt:557(thread reply) - a multi-word fence info string still reaches the fileSpeechToTextPlugin.kt:530(thread reply) -dropProsestill drops a real first or last line, silentlySpeechToTextPlugin.kt:182(thread reply) -forgetLlmServicewrites outsideserviceLock
MINOR is by definition safe to merge, so this review is a COMMENT.
One candidate dropped. The asymmetry between the unguarded addPluginLifecycleListener at line 116 and the runCatching around removePluginLifecycleListener at line 204 does not survive checking: the asymmetry is reasoned - teardown() runs on paths where add never ran - and min_ide_version 26.29 is exactly what keeps that call off a host without the method.
Verification. Toolchain versions is green at head and the branch compiles, but per CLAUDE.md neither is verification for this plugin, and nothing here was exercised on a device this round. The fence and prose paths are only really checkable by speaking into a real editor against a real backend. This repo has no written approve/request-changes rule - CLAUDE.md governs, and it speaks to device verification rather than merge gating - so the default severity rule applied.
itsaky-adfa
left a comment
There was a problem hiding this comment.
Approving, which lifts my CHANGES_REQUESTED of 2026-09-14. Both findings it named are fixed at 96067fb: the fence gate is now language-agnostic rather than allowlist-gated, and min_ide_version is 26.29.
The five findings from this round are all MINOR - safe to merge, worth a look before or after:
SpeechToTextPlugin.kt:509- an inline-closed fence absorbs the model's trailing explanation into the file. A one-line suggestion is attached to that comment.SpeechToTextPlugin.kt:286- the new busy guard leaves no recovery from a stuckRECORDINGstate.- Three reopened threads: the multi-word fence info string,
dropProsesilently dropping a real first or last line, andforgetLlmServicewriting outsideserviceLock.
Nothing here was exercised on a device. Per CLAUDE.md that is not verification for this plugin, so the fence and prose paths are still worth one run against a real backend - a .dart or .md file is the interesting case.
Stop an inline-closed fence and a multi-word info string from reaching the file, keep prose-shaped languages out of the prose filter and log any drop, let a tap during RECORDING restart the capture, and close the forgetLlmService cache race with an epoch.
Description
Updated
SpeechToTextPluginto resolve theLlmInferenceServicefrom 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 fromSharedServicesfirst, then from AI Core's per-plugin export viacontext.getPluginService(), with the localcontext.servicesas the last fallback. Resolution is dynamically retried on every use to account for parallel loading delays when the AI Core activates.Details
resolveLlmService()to cache successful lookups and retry upon failure.GENERATION_TIMEOUT_SECONDS=MAX_GENERATION_TOKENS / SLOWEST_TOKENS_PER_SECOND + PROMPT_EVAL_SECONDS= 512 / 5 + 30 = 132 s) to prevent UI freezes.stripCodeFences()to remove markdown from the generated output so the editor receives raw code.PluginLifecycleListener, so a disabled plugin no longer pins its ClassLoader.SharedServices,PluginContext.getPluginService()andaddPluginLifecycleListener()all first ship in 26.29, and the previous 26.30 ceiling excluded every host AI Core runs on.Logto the plugin'scontext.logger.Screen_Recording_20260903_155612_Code.on.the.Go.mp4
Ticket
ADFA-5403
Parent: ADFA-5402