feat: API reliability, permissions, and CI coverage

[JMAN730]

Summary

• Harden LLM reliability: per-provider model pairing, typed errors/retries, connection tests, and chat recovery UI (API reliability & error surfacing map (upstream issue 13) #25)
• Close CI/migration gaps: Room 6→7 schema proof, lint baseline, and lint/release assemble in CI (Test coverage gaps CI does not close: Room migration, lint, release build #42)
• Also includes prior branch work: response-body redaction, grant-all permissions onboarding, Claude catalog alignment, and crash-log device metadata

Test plan

• ./gradlew testDebugUnitTest (or focused suites for LLM/errors/migration)
• ./gradlew lintDebug
• Settings: Test connection for a configured provider
• Benchmark: Test all configured providers sequentially
• Chat: trigger an LLM failure and confirm recovery card / pause behavior
• Confirm CI runs unit tests, lint, assembleDebug, assembleRelease

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, you can upgrade your account or add credits to your account and enable them for code reviews in your settings.

[greptile-apps]

byCanonicalName has a duplicate key that overwrites the primary On-Device AI entry

Both ProviderSpec(ON_DEVICE, "gemma-4-on-device") and ProviderSpec(LEGACY_ON_DEVICE, "gemma-4-on-device", ON_DEVICE) share the canonical name "On-Device AI". associateBy { it.canonicalName } takes the last winner, so byCanonicalName["On-Device AI"] ends up pointing to the legacy spec whose displayName is "Gemma 4 (On-device)". The default model happens to be identical today, but any future code that reads displayName from byCanonicalName will see the legacy name instead of the current one, which could surface confusingly in error messages and connection-test labels.

Prompt To Fix With AI

This is a comment left during a code review.
Path: app/src/main/java/com/opendroid/ai/core/llm/ProviderCatalog.kt
Line: 43-44

Comment:
**`byCanonicalName` has a duplicate key that overwrites the primary `On-Device AI` entry**

Both `ProviderSpec(ON_DEVICE, "gemma-4-on-device")` and `ProviderSpec(LEGACY_ON_DEVICE, "gemma-4-on-device", ON_DEVICE)` share the canonical name `"On-Device AI"`. `associateBy { it.canonicalName }` takes the last winner, so `byCanonicalName["On-Device AI"]` ends up pointing to the legacy spec whose `displayName` is `"Gemma 4 (On-device)"`. The default model happens to be identical today, but any future code that reads `displayName` from `byCanonicalName` will see the legacy name instead of the current one, which could surface confusingly in error messages and connection-test labels.

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

[greptile-apps]

Confidence Score: 4/5

Safe to merge with one P1 defect to address: the stale-config window in connection testing.

The LLM retry/error layer, chat recovery card, bounded incompleteMessageIds set, and Room 6→7 migration are all well-implemented and tested. The one concrete defect is SettingsViewModel.runConnectionTest using the draft in-memory config for configurationGap while WrappedLLMProvider uses the persisted config, causing spurious auth failures within the 500 ms debounce window. This is a real user-visible defect but isolated to the Settings connection-test UX, not the primary chat path.

Files Needing Attention: app/src/main/java/com/opendroid/ai/ui/viewmodel/SettingsViewModel.kt (draft vs persisted config mismatch); app/src/main/java/com/opendroid/ai/core/llm/ProviderCatalog.kt (silent duplicate key in byCanonicalName)

Important Files Changed

Filename	Overview
app/src/main/java/com/opendroid/ai/core/llm/error/LLMError.kt	New typed error hierarchy with reference-counted secret scrubbing and bounded body reading — well-structured.
app/src/main/java/com/opendroid/ai/core/llm/LLMProviderFactory.kt	WrappedLLMProvider retry decorator correct; emitted one-way latch design is undocumented but intentional.
app/src/main/java/com/opendroid/ai/ui/viewmodel/SettingsViewModel.kt	runConnectionTest uses draft _llmConfig.value for configurationGap while actual HTTP call uses persisted config — spurious auth failures within 500 ms debounce window.
app/src/main/java/com/opendroid/ai/core/agent/AgentLoop.kt	incompleteMessageIds now bounded at 100 via evicting LinkedHashMap; NonCancellable partial-save and retryRequest() added cleanly.
app/src/main/java/com/opendroid/ai/core/agent/ChatErrorUiState.kt	Clean Phase sealed class with Idle/Retrying/WaitingUntil/Final states and fromException() factory.
app/src/main/java/com/opendroid/ai/core/llm/ProviderCatalog.kt	byCanonicalName map has duplicate key for 'On-Device AI'; last entry wins silently.
app/src/main/java/com/opendroid/ai/core/permissions/PermissionModel.kt	GrantAllState/PermissionsSnapshot/grantAllButton() onboarding flow is correct; cardStatus and isBlocked logic are sound.
app/src/test/java/com/opendroid/ai/data/db/OpenDroidDatabaseMigrationTest.kt	Robolectric MigrationTestHelper validates 6→7 incremental migration, idempotent re-run, and full 1→7 chain with data-preservation assertions.
app/src/main/java/com/opendroid/ai/core/llm/ConnectionTest.kt	ConnectionTestPlanner cleanly separates pre-flight configurationGap check from network-error mapping; no issues.
app/src/main/java/com/opendroid/ai/core/permissions/PermissionAskedStore.kt	SharedPreferences-backed asked-permission store; straightforward with no issues.

Sequence Diagram

sequenceDiagram
    participant U as User
    participant SVM as SettingsViewModel
    participant WLP as WrappedLLMProvider
    participant SR as SettingsRepository
    participant Net as Network

    U->>SVM: types new API key
    SVM->>SVM: debounce 500 ms (not yet fired)
    U->>SVM: testConnection()
    SVM->>SVM: configurationGap(_llmConfig.value) pass (draft has new key)
    SVM->>WLP: streamComplete(request)
    WLP->>SR: llmConfig.first() returns OLD key
    WLP->>Net: HTTP with stale credential
    Net-->>WLP: 401 AuthInvalid
    WLP-->>SVM: ConnectionTestState.Failed
    SVM-->>U: spurious auth failure shown
    Note over SR: 500 ms debounce fires
    SR->>SR: persists new key
    U->>SVM: testConnection() again
    WLP->>SR: llmConfig.first() returns NEW key
    WLP->>Net: HTTP with correct credential
    Net-->>WLP: 200 OK
    WLP-->>SVM: ConnectionTestState.Connected
    SVM-->>U: success

Loading

_{Reviews (2): Last reviewed commit: "Merge origin/main into JMAN730/app-not-i..." | Re-trigger Greptile}

[JMAN730]

Review

Overview: This PR replaces the provider-fallback chain with a WrappedLLMProvider that resolves per-provider model/key/endpoint, adds a bounded jittered retry loop with a typed LLMError/LLMException taxonomy and allowlist-based redaction, surfaces failures as an inline chat recovery card, adds per-provider connection tests, rewrites the permissions screen around a pure PermissionModel with a grant-all onboarding flow, adds the Room 6→7 migration with a Robolectric proof test, and tightens CI (lint abortOnError true + baseline, debug + release assembles).

The error-taxonomy/redaction design, the migration SQL (verified against 7.json — exact match), and the pure-model layers are solid and genuinely tested. The wiring around them has real gaps.

Critical

1. ChatScreen.kt has literal encoding corruption. Lines 668, 917, 941, 1126–1132: "·", "—", "→", "•" became raw invalid bytes / "?" (verified: line 668 contains a bare 0xB7; file now reports the source as ISO-8859, not UTF-8). Restore the characters and fix the tool that wrote them.
2. CI assembleRelease should fail every run. app/build.gradle has a taskGraph.whenReady guard that throws GradleException when any release package task runs without signing configured, and the workflow provides no signing secrets. Either inject secrets, add an explicit -PallowUnsignedRelease=true opt-out honored by the guard, or drop the step. Suggests the workflow change was never exercised.

Major

3. Ollama error mapping outlier (OllamaProvider.kt:74): still throws plain IOException on HTTP failure, so every Ollama 401/404/400 is classified as retryable Network and retried 3×. Route through toSafeProviderException like the other providers.
4. On-device tool-streaming ignores the selected model (HybridOnDeviceProvider.kt:147, LiteRTLMProvider, GemmaProvider): tool-calling stream paths now hardcode ProviderCatalog.defaultModel(...) where they previously read the user's selection.
5. LLMProviderFactory.getActiveProvider() hard-throws a bare IllegalArgumentException on unknown/corrupt persisted activeProvider (previously fell back to Gemini); callers get an unmapped exception. Fall back or throw a typed LLMException.
6. Error card isn't session-scoped (AgentLoop.kt:129): chatError is a global StateFlow with no session id — an error from chat A renders its recovery card in whichever chat is open. Add sessionId and filter like visibleAgentState.
7. Retry duplicates the user message (ChatViewModel.kt:70–82): retryAfterChatError goes through processQuery, which inserts a new user bubble, and it re-sends the visible chat's last user message rather than the failed request (error.requestId is never used).
8. Paused plans are a dead-end (AgentLoop.kt:948,1062): re-eval failure sets PlanStatus.PAUSED, but nothing ever reads or resumes PAUSED — the recovery card's Retry starts a fresh simple query. Implement resume, or mark FAILED honestly.
9. Stale error card (AgentLoop.kt:514): _chatError clears only on simple-query success; a successful plan/action task or a new typed query leaves the error card pinned forever. Clear at the start of each task.
10. Cancelled-task partial never persists (AgentLoop.kt:471–476): the CancellationException handler calls suspend insertMessage from the already-cancelled coroutine, so it throws at its first suspension point. Wrap in withContext(NonCancellable).
11. Batch connection test can wedge (SettingsViewModel.kt:410–415): testConnection cancels a running batch but never clears _connectionBatchProgress, leaving Benchmark stuck on "Testing X of Y" with no way to start a new batch; a mid-flight provider row also sticks at "Testing…".
12. Grant-all permissions flow (PermissionsScreen.kt:135–152): one batch requests every dangerous permission (mic, fine location, SMS send/read/receive, call, contacts, camera) with no confirmation or per-permission rationale — least-privilege regression and a Play "request in context of use" policy risk. At minimum gate behind a confirmation dialog (Benchmark's "test all" got one).

Minor

• LLMError.kt:260: quota check precedes the 429 branch, so 429 bodies mentioning "quota"/"credit" (Gemini, OpenRouter) become non-retryable QuotaExhausted; for status 429 prefer RateLimited unless it's specifically insufficient_quota.
• LLMProviderFactory.kt:228–266: shouldRetry and retryDelayMillis each draw fresh jitter, so the delay validated isn't the delay used, and retryDelayMillis can throw right after shouldRetry approved. Compute once.
• LLMConfig.kt:61,71: unresolvable Claude model silently coerces to the default — user gets a different model than selected with no signal (and with the forward-compat regex removed, brand-new Anthropic IDs are unusable until a catalog bump).
• Dead machinery: RedactedDetail.vendorStatus always null; malformed(transient = true) never called so empty streams never retry; ChatErrorUiState.Phase (Retrying/WaitingUntil) computed but never rendered, so rate-limited errors show an immediately-enabled Retry with no countdown.
• ConnectionTestPlanner.cloudProviders() re-lists provider names as string literals — drift hazard; derive from ProviderCatalog.
• ChatScreen.kt:396: EDIT_MESSAGE edits history.lastOrNull { USER } instead of the message for error.requestId.
• AgentLoop.kt:132,480: incompleteMessageIds is in-memory only and unbounded.
• PermissionsScreen.kt:139–184: first-frame snapshot uses includeRationale = false so denied permissions flash as "blocked"; markAsked persists before the dialog launches and pendingRequest/grantAll are plain remember, so process death mislabels permissions as asked/blocked.
• Schema JSONs are committed in three places; the src/main/assets copies ship in every APK (~80 KB bloat + internal-structure disclosure), nothing reads them, and they'll go stale — delete. Same for the src/test/assets copies (build already adds $projectDir/schemas to test assets).
• room-testing:2.8.4 added but MigrationTestHelper never used; the migration test instead hand-duplicates the v6 DDL and identity hash as string literals (drift hazard — MigrationTestHelper + exported 6.json exists to eliminate exactly this). Also only 6→7 is tested, not the full 1→7 chain long-time users take, and roomDb.openHelper.readableDatabase.use { } closes the support DB out from under the still-open RoomDatabase.
• lint-baseline.xml: with abortOnError true, the baseline suppresses one real MissingPermission (AdvancedControlActions.kt:399, unguarded openCamera) and one NewApi — fix those two rather than baseline them; the rest (~50 icon/ObsoleteSdkInt/UnusedResources entries) are fine.
• Network-error retry re-POSTs completions that may have been processed server-side (double billing); consider restricting to connect-phase failures.

Verified clean: MIGRATION_6_7 SQL matches 7.json exactly (columns, NOT NULL set, index); manifest change is pure permission removal with no dangling references; Claude catalog IDs are current (fable-5 / opus-5 / sonnet-5 / haiku-4-5 dated) with sane legacy alias retargeting; redaction never retains raw bodies and Gemini's key moved out of the URL; PermissionModel and the crash-log tests assert real behavior.

Suggested landing order: fix the two criticals (encoding, CI release guard) plus #3–#5 before merge; #6–#12 are user-visible state bugs worth a fast follow; minors as cleanup.