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..ffe72b9a8f 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. @@ -442,10 +445,16 @@ 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) - 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 +489,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 +548,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 +570,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 +629,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 +716,44 @@ class AiCoach(private val repo: WhoopRepository) { companion object { private val JSON = "application/json; charset=utf-8".toMediaType() + /** + * 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 + } + + /** + * 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() } + }.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 +850,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")) + } +}