From c5a14279785aa3c3e7ddfd8a2d393b2a06bb4b90 Mon Sep 17 00:00:00 2001 From: ryanbr Date: Wed, 5 Aug 2026 20:26:39 +1200 Subject: [PATCH 1/3] fix(android): Coach fixes for #1074 (crash, real errors, model scan, truncation, Disconnect UI) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Five fixes for the AI Coach + custom OpenAI-compatible (DeepSeek) provider report, all confirmed against the reporter's screenshots: 1. Crash — `error!!` read inside a `semantics {}` contentDescription closure NPE'd when the accessibility pass ran after a recomposition cleared `error`. Snapshot into a stable local first. 2. Surface the provider's real error — on a 200 with empty assistant content (a hand-set model the provider doesn't offer), some servers put the error IN the 200 body. Surface it instead of a blanket "empty reply"; all three provider paths. 3. Custom base-URL normalization — a pasted full chat URL made the model scan hit .../chat/completions/models and return nothing; strip the chat suffix so /models and /chat/completions resolve either way. 4. Truncation — max_tokens 900 cut detailed cloud replies mid-sentence (the screenshot showed it on DeepSeek). Raise to 4096 (a cap, not a target; the Gemini leg already used 4096). The cut-off note gave Ollama-specific `num_ctx` advice that is wrong for cloud providers — rewritten accurate and provider-agnostic. 5. Disconnect UI — a long model id in the provider pill squeezed the Disconnect into a one-char vertical stack (or off-screen). StatePill now ellipsizes (a pill is one line); the Disconnect keeps its intrinsic single-line width. Pure helpers unit-tested (AiCoachEmptyReplyTest, AiCoachCustomUrlTest); compileFullDebugKotlin + i18n audit green. Layout changes not device-verified (Compose won't render on this box). Android-only; the Swift AICoach twin has the empty-reply + base-URL gaps as a follow-up. --- .../app/src/main/java/com/noop/ai/AiCoach.kt | 72 +++++++++++++++---- .../src/main/java/com/noop/ui/CoachScreen.kt | 26 +++++-- .../src/main/java/com/noop/ui/Components.kt | 7 +- .../java/com/noop/ai/AiCoachCustomUrlTest.kt | 36 ++++++++++ .../java/com/noop/ai/AiCoachEmptyReplyTest.kt | 30 ++++++++ 5 files changed, 149 insertions(+), 22 deletions(-) create mode 100644 android/app/src/test/java/com/noop/ai/AiCoachCustomUrlTest.kt create mode 100644 android/app/src/test/java/com/noop/ai/AiCoachEmptyReplyTest.kt diff --git a/android/app/src/main/java/com/noop/ai/AiCoach.kt b/android/app/src/main/java/com/noop/ai/AiCoach.kt index 2c30cfcbf0..1d17d1ad50 100644 --- a/android/app/src/main/java/com/noop/ai/AiCoach.kt +++ b/android/app/src/main/java/com/noop/ai/AiCoach.kt @@ -421,7 +421,10 @@ class AiCoach(private val repo: WhoopRepository) { ?.optString("content") ?.trim() - if (content.isNullOrEmpty()) throw Exception("The provider returned an empty reply. Please try again.") + // Empty assistant content on a 200: some OpenAI-compatible servers (notably a model set by hand + // that the provider doesn't offer) return the real error INSIDE a 200 body rather than a 4xx, so + // surface it instead of a blanket "empty reply" that hides the cause (#1074). + if (content.isNullOrEmpty()) throw Exception(emptyReplyMessage(responseText)) // Local servers (notably Ollama) stop with finish_reason "length" at the context-window edge // and give NO error, keep the partial text and append the actionable notice so it isn't silent. @@ -445,7 +448,10 @@ class AiCoach(private val repo: WhoopRepository) { body.put("max_completion_tokens", 900) } else { body.put("temperature", 0.6) - body.put("max_tokens", 900) + // #1074: 900 truncated detailed coaching replies mid-sentence on cloud providers (the reporter + // hit it on a DeepSeek "pro" model). 4096 lets a full multi-section reply complete; it is a cap, + // not a target, so short answers are unaffected. Matches the Gemini leg's maxOutputTokens. + body.put("max_tokens", 4096) } val builder = Request.Builder() @@ -480,7 +486,7 @@ class AiCoach(private val repo: WhoopRepository) { } /** Base for the Custom provider, the user's URL with any trailing slashes trimmed. */ - private fun customBase(url: String): String = url.trim().trimEnd('/') + private fun customBase(url: String): String = normalizeCustomBaseUrl(url) private fun customChatUrl(url: String): String { val base = customBase(url) @@ -539,7 +545,7 @@ class AiCoach(private val repo: WhoopRepository) { val body = JSONObject() .put("model", model) - .put("max_tokens", 900) + .put("max_tokens", 4096) // #1074: 900 truncated detailed replies; a cap, not a target .put("system", systemPrompt) .put("messages", messages) .toString() @@ -561,7 +567,7 @@ class AiCoach(private val repo: WhoopRepository) { ?.optString("text") ?.trim() - if (content.isNullOrEmpty()) throw Exception("The provider returned an empty reply. Please try again.") + if (content.isNullOrEmpty()) throw Exception(emptyReplyMessage(text)) return content } @@ -620,7 +626,7 @@ class AiCoach(private val repo: WhoopRepository) { if (parts != null) for (i in 0 until parts.length()) append(parts.optJSONObject(i)?.optString("text").orEmpty()) }.trim() - if (reply.isEmpty()) throw Exception("The provider returned an empty reply. Please try again.") + if (reply.isEmpty()) throw Exception(emptyReplyMessage(text)) return reply } @@ -707,6 +713,44 @@ class AiCoach(private val repo: WhoopRepository) { companion object { private val JSON = "application/json; charset=utf-8".toMediaType() + /** + * The user-facing message for a 200 response whose assistant content is empty (#1074). Some + * OpenAI-compatible servers (e.g. a hand-set model the provider doesn't offer) return the real + * error INSIDE a 200 body rather than as a 4xx; surface that here instead of a blanket "empty + * reply" so the cause is visible. Falls back to a hint about the hand-set model otherwise. + * Pure + `internal` so it is unit-testable without a network. Same `{"error":{"message":…}}` + * shape [extractApiErrorMessage] reads. + */ + /** + * Normalise a user-entered Custom base URL: trim, drop a trailing slash, and tolerate a pasted + * FULL chat URL by stripping a trailing OpenAI-style chat path. So the derived `/chat/completions` + * and `/models` endpoints resolve whether the user pasted the API root (`https://api.deepseek.com` + * or `.../v1`) OR the whole chat URL (`.../v1/chat/completions`) — the latter otherwise made the + * model scan hit `.../chat/completions/models` and silently return nothing (#1074). Pure → tested. + */ + internal fun normalizeCustomBaseUrl(url: String): String { + var base = url.trim().trimEnd('/') + for (suffix in listOf("/chat/completions", "/completions")) { + if (base.endsWith(suffix, ignoreCase = true)) { + base = base.dropLast(suffix.length).trimEnd('/') + break + } + } + return base + } + + internal fun emptyReplyMessage(body: String): String { + val providerError = runCatching { + JSONObject(body).optJSONObject("error")?.optString("message")?.takeIf { it.isNotBlank() } + }.getOrNull() + return if (providerError != null) { + "The provider returned an error: $providerError" + } else { + "The provider returned an empty reply. If you set a custom model by hand, check that " + + "the model name is one the provider actually offers." + } + } + /** * Pure: unwrap Gemini's `{"models":[{"name":"models/…"}]}` into chat-capable ids. Strips the * `models/` prefix, keeps `gemini*` only (drops embedding / AQA models). Byte-for-byte twin of @@ -803,17 +847,15 @@ class AiCoach(private val repo: WhoopRepository) { const val MAX_HISTORY_TURNS = 10 /** - * Appended to a reply when the server stopped early because it ran out of context window. - * Local OpenAI-compatible servers (notably Ollama, which defaults to a 2048-token window and - * IGNORES `num_ctx` on the `/v1` endpoint) truncate silently, no error, the text just stops - * mid-sentence. We can't raise the window over the OpenAI wire format, so we make the cutoff - * visible and tell the user exactly how to fix it. + * Appended to a reply that stopped early with `finish_reason == "length"` — it reached the + * response-length cap (`max_tokens`, now 4096) or, on a local server (e.g. Ollama's default + * 2048-token window), the model's own limit. Either way the text just stops mid-sentence with no + * error, so make the cutoff visible. Kept provider-agnostic: the old note gave Ollama-specific + * `num_ctx` instructions that were wrong for cloud providers like the #1074 DeepSeek report. */ const val TRUNCATION_NOTE = - "\n\n---\n*Reply cut off: the model hit its context-window limit. " + - "On a local server like Ollama (default 2048 tokens), raise it - create a Modelfile " + - "with `PARAMETER num_ctx 8192` and select that model, or set " + - "`OLLAMA_CONTEXT_LENGTH=8192` and relaunch Ollama - then ask again.*" + "\n\n---\n*Reply cut off at the response-length limit. Ask a more specific question for a " + + "shorter, complete answer — or, on a local server (e.g. Ollama), raise its context window.*" /** * Pure: sliding-window the chat. Returns everything when short; otherwise the first user turn diff --git a/android/app/src/main/java/com/noop/ui/CoachScreen.kt b/android/app/src/main/java/com/noop/ui/CoachScreen.kt index cca2bedc35..5212301733 100644 --- a/android/app/src/main/java/com/noop/ui/CoachScreen.kt +++ b/android/app/src/main/java/com/noop/ui/CoachScreen.kt @@ -16,6 +16,7 @@ import androidx.compose.foundation.layout.height import androidx.compose.foundation.layout.heightIn import androidx.compose.foundation.layout.padding import androidx.compose.foundation.layout.size +import androidx.compose.foundation.layout.width import androidx.compose.foundation.layout.widthIn import androidx.compose.foundation.layout.wrapContentWidth import androidx.compose.foundation.shape.RoundedCornerShape @@ -246,13 +247,21 @@ private fun CoachChat(vm: CoachViewModel) { // Active-provider strip + reset-key affordance. NoopCard(padding = 14.dp, tint = Palette.chargeColor) { Row(verticalAlignment = Alignment.CenterVertically) { - StatePill(title = uiString(R.string.l10n_coach_screen_provider_displayname_model_8b39f761, provider.displayName, model), tone = StrandTone.Accent, showsDot = true) - Spacer(Modifier.weight(1f)) + // The pill takes the flexible space (ellipsizing a long model id); the Disconnect keeps + // its intrinsic single-line width so it can never be squeezed into a vertical stack (#1074). + StatePill( + title = uiString(R.string.l10n_coach_screen_provider_displayname_model_8b39f761, provider.displayName, model), + tone = StrandTone.Accent, showsDot = true, + modifier = Modifier.weight(1f), + ) + Spacer(Modifier.width(8.dp)) val disconnectInteraction = remember { MutableInteractionSource() } Text( uiString(R.string.l10n_coach_screen_disconnect_ed28e068), style = NoopType.caption, color = Palette.textSecondary, + maxLines = 1, + softWrap = false, modifier = Modifier .clip(RoundedCornerShape(50)) .liquidPress(disconnectInteraction) @@ -304,13 +313,18 @@ private fun CoachChat(vm: CoachViewModel) { } } - // Error line (red). - if (error != null) { + // Error line (red). Capture into a stable local first: `error` is a state-backed nullable, and + // the `semantics {}` contentDescription is a DEFERRED closure run later during the accessibility + // pass — by then a recomposition can have cleared `error`, so `error!!` inside the lambda would + // NPE (#1074). The local `errorMsg` is a fixed non-null snapshot the lambda can't null out from + // under it. + val errorMsg = error + if (errorMsg != null) { Text( - error!!, + errorMsg, style = NoopType.subhead, color = Palette.statusCritical, - modifier = Modifier.semantics { contentDescription = uiString(R.string.l10n_coach_screen_coach_error_error_ad9c8c46, error!!) }, + modifier = Modifier.semantics { contentDescription = uiString(R.string.l10n_coach_screen_coach_error_error_ad9c8c46, errorMsg) }, ) } diff --git a/android/app/src/main/java/com/noop/ui/Components.kt b/android/app/src/main/java/com/noop/ui/Components.kt index 21bc437cbd..d18f61bc51 100644 --- a/android/app/src/main/java/com/noop/ui/Components.kt +++ b/android/app/src/main/java/com/noop/ui/Components.kt @@ -394,7 +394,12 @@ fun StatePill( horizontalArrangement = Arrangement.spacedBy(6.dp), ) { if (showsDot) ConnectionDot(tone = tone, pulsing = pulsing, size = 7.dp) - Text(title, style = NoopType.overline.copy(letterSpacing = 0.4.sp), color = tone.color) + // A pill is a single line: ellipsize a long title (e.g. a long custom model id) instead of + // wrapping it, so it can't eat a whole Row and squeeze a sibling control to nothing (#1074). + Text( + title, style = NoopType.overline.copy(letterSpacing = 0.4.sp), color = tone.color, + maxLines = 1, overflow = TextOverflow.Ellipsis, + ) } } diff --git a/android/app/src/test/java/com/noop/ai/AiCoachCustomUrlTest.kt b/android/app/src/test/java/com/noop/ai/AiCoachCustomUrlTest.kt new file mode 100644 index 0000000000..39b669dd92 --- /dev/null +++ b/android/app/src/test/java/com/noop/ai/AiCoachCustomUrlTest.kt @@ -0,0 +1,36 @@ +package com.noop.ai + +import org.junit.Assert.assertEquals +import org.junit.Test + +/** + * #1074: the Custom base URL normalises so the derived /chat/completions and /models endpoints + * resolve whether the user pastes the API root or the full chat URL — the latter previously broke + * the model scan (it hit .../chat/completions/models). + */ +class AiCoachCustomUrlTest { + + @Test fun apiRootIsKeptAsIs() { + assertEquals("https://api.deepseek.com", AiCoach.normalizeCustomBaseUrl("https://api.deepseek.com")) + assertEquals("https://api.deepseek.com/v1", AiCoach.normalizeCustomBaseUrl("https://api.deepseek.com/v1")) + } + + @Test fun trailingSlashTrimmed() { + assertEquals("https://api.deepseek.com/v1", AiCoach.normalizeCustomBaseUrl("https://api.deepseek.com/v1/")) + } + + @Test fun pastedFullChatUrlIsStrippedToTheBase() { + assertEquals("https://api.deepseek.com/v1", + AiCoach.normalizeCustomBaseUrl("https://api.deepseek.com/v1/chat/completions")) + assertEquals("https://api.deepseek.com", + AiCoach.normalizeCustomBaseUrl("https://api.deepseek.com/chat/completions")) + // trailing slash on the pasted chat URL too + assertEquals("https://api.deepseek.com/v1", + AiCoach.normalizeCustomBaseUrl("https://api.deepseek.com/v1/chat/completions/")) + } + + @Test fun localServerRootUnaffected() { + assertEquals("http://192.168.1.10:11434/v1", + AiCoach.normalizeCustomBaseUrl("http://192.168.1.10:11434/v1")) + } +} diff --git a/android/app/src/test/java/com/noop/ai/AiCoachEmptyReplyTest.kt b/android/app/src/test/java/com/noop/ai/AiCoachEmptyReplyTest.kt new file mode 100644 index 0000000000..8689582237 --- /dev/null +++ b/android/app/src/test/java/com/noop/ai/AiCoachEmptyReplyTest.kt @@ -0,0 +1,30 @@ +package com.noop.ai + +import org.junit.Assert.assertTrue +import org.junit.Test + +/** + * #1074: when an OpenAI-compatible provider returns a 200 with empty assistant content, surface the + * provider's own error (some servers put it in a 200 body, e.g. a hand-set model they don't offer) + * instead of a blanket "empty reply" that hides the cause. + */ +class AiCoachEmptyReplyTest { + + @Test + fun surfacesProviderErrorFromA200Body() { + val msg = AiCoach.emptyReplyMessage("""{"error":{"message":"Model Not Exist"}}""") + assertTrue("should surface the provider's real message", msg.contains("Model Not Exist")) + } + + @Test + fun fallsBackToModelHintWhenNoProviderError() { + val msg = AiCoach.emptyReplyMessage("{}") + assertTrue("no provider error → point at the hand-set model", msg.contains("model name")) + } + + @Test + fun malformedBodyStillGivesTheFallback() { + val msg = AiCoach.emptyReplyMessage("not json at all") + assertTrue(msg.contains("model name")) + } +} From a0e69f98033567a783f227fdd612e6465df3a16e Mon Sep 17 00:00:00 2001 From: ryanbr Date: Wed, 5 Aug 2026 21:08:27 +1200 Subject: [PATCH 2/3] fix(android): raise reasoning-model reply cap to 4096 too (#1074) The modern-params leg (max_completion_tokens) fronts reasoning models, which count hidden thinking tokens against that budget - so the old 900 starved them into truncated/empty replies even more readily than the standard path. Match the standard leg's 4096 cap. A cap, not a target; short answers are unaffected. Keeps this at parity with the iOS/macOS side, which uses 4096 on both legs. --- android/app/src/main/java/com/noop/ai/AiCoach.kt | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/android/app/src/main/java/com/noop/ai/AiCoach.kt b/android/app/src/main/java/com/noop/ai/AiCoach.kt index 1d17d1ad50..82db5483e0 100644 --- a/android/app/src/main/java/com/noop/ai/AiCoach.kt +++ b/android/app/src/main/java/com/noop/ai/AiCoach.kt @@ -445,7 +445,10 @@ class AiCoach(private val repo: WhoopRepository) { .put("model", model) .put("messages", messages) if (modernParams) { - body.put("max_completion_tokens", 900) + // #1074: same 4096 cap as the standard path below. This modern-params leg fronts REASONING + // models, which count hidden thinking tokens against max_completion_tokens — so 900 starved + // them into truncated/empty replies even more readily. A cap, not a target. + body.put("max_completion_tokens", 4096) } else { body.put("temperature", 0.6) // #1074: 900 truncated detailed coaching replies mid-sentence on cloud providers (the reporter From 05cad3b5d98337897779ac249a0ab72e19080241 Mon Sep 17 00:00:00 2001 From: ryanbr Date: Wed, 5 Aug 2026 21:17:34 +1200 Subject: [PATCH 3/3] fix(android): reattach the emptyReplyMessage doc comment (#1074) The emptyReplyMessage KDoc was stranded above normalizeCustomBaseUrl's own doc block, so it documented nothing (Dokka/IDE hover would miss it) and tripped the detached-doc-comment regression gate. Move it down onto emptyReplyMessage. No code change. --- android/app/src/main/java/com/noop/ai/AiCoach.kt | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/android/app/src/main/java/com/noop/ai/AiCoach.kt b/android/app/src/main/java/com/noop/ai/AiCoach.kt index 82db5483e0..ffe72b9a8f 100644 --- a/android/app/src/main/java/com/noop/ai/AiCoach.kt +++ b/android/app/src/main/java/com/noop/ai/AiCoach.kt @@ -716,14 +716,6 @@ class AiCoach(private val repo: WhoopRepository) { companion object { private val JSON = "application/json; charset=utf-8".toMediaType() - /** - * The user-facing message for a 200 response whose assistant content is empty (#1074). Some - * OpenAI-compatible servers (e.g. a hand-set model the provider doesn't offer) return the real - * error INSIDE a 200 body rather than as a 4xx; surface that here instead of a blanket "empty - * reply" so the cause is visible. Falls back to a hint about the hand-set model otherwise. - * Pure + `internal` so it is unit-testable without a network. Same `{"error":{"message":…}}` - * shape [extractApiErrorMessage] reads. - */ /** * Normalise a user-entered Custom base URL: trim, drop a trailing slash, and tolerate a pasted * FULL chat URL by stripping a trailing OpenAI-style chat path. So the derived `/chat/completions` @@ -742,6 +734,14 @@ class AiCoach(private val repo: WhoopRepository) { return base } + /** + * The user-facing message for a 200 response whose assistant content is empty (#1074). Some + * OpenAI-compatible servers (e.g. a hand-set model the provider doesn't offer) return the real + * error INSIDE a 200 body rather than as a 4xx; surface that here instead of a blanket "empty + * reply" so the cause is visible. Falls back to a hint about the hand-set model otherwise. + * Pure + `internal` so it is unit-testable without a network. Same `{"error":{"message":…}}` + * shape [extractApiErrorMessage] reads. + */ internal fun emptyReplyMessage(body: String): String { val providerError = runCatching { JSONObject(body).optJSONObject("error")?.optString("message")?.takeIf { it.isNotBlank() }