Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
77 changes: 61 additions & 16 deletions android/app/src/main/java/com/noop/ai/AiCoach.kt
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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()
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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()
Expand All @@ -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
}

Expand Down Expand Up @@ -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
}

Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
26 changes: 20 additions & 6 deletions android/app/src/main/java/com/noop/ui/CoachScreen.kt
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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) },
)
}

Expand Down
7 changes: 6 additions & 1 deletion android/app/src/main/java/com/noop/ui/Components.kt
Original file line number Diff line number Diff line change
Expand Up @@ -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,
)
}
}

Expand Down
36 changes: 36 additions & 0 deletions android/app/src/test/java/com/noop/ai/AiCoachCustomUrlTest.kt
Original file line number Diff line number Diff line change
@@ -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"))
}
}
30 changes: 30 additions & 0 deletions android/app/src/test/java/com/noop/ai/AiCoachEmptyReplyTest.kt
Original file line number Diff line number Diff line change
@@ -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"))
}
}