fix(conversations): honor a preferred summarization app the setter admitted#10078
Merged
Git-on-my-level merged 1 commit intoJul 21, 2026
Conversation
…mitted Setting a default summary template goes through PUT /v1/users/preferences/app, which admits any app get_available_app_by_id can see — existence and private-ownership only. The summary path then re-decided availability with a different check: the preferred app was honored only if it appeared in the works_with_memories()-and-enabled installed slice. An app that passed the first check but not the second — a template whose enable call failed (the create flow ignores enableAppServer failures), or one created on another device before the 30s app-slice caches converge — was accepted as the default and then silently ignored at generation time, falling through to LLM suggestions (BasedHardware#10074). The reader now resolves a preferred app that is missing from the installed slice through the setter's own authority (get_available_app_model_by_id, the new shared App-model form of the admission check) and uses it when it can summarize. A deleted or inaccessible app still falls through to suggestions, and a resolvable app without the memories capability falls through with a warning — the previously silent drop now always logs why. Regression tests extend the existing _trigger_apps preferred-app suite: an admitted-but-not-installed template runs without a suggestion LLM call; a capability-less app still falls back; the deleted-app fallthrough keeps its contract. Fixes BasedHardware#10074. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Git-on-my-level
left a comment
Git-on-my-level
left a comment
Collaborator
There was a problem hiding this comment.
Thanks for the focused fix here. I reviewed the diff and ran the targeted backend unit coverage locally:
PYTHONUTF8=1 ENCRYPTION_SECRET=*** OPENAI_API_KEY=*** BACKEND_FAST_UNIT_WARN_SECONDS=999 BACKEND_FAST_UNIT_FAIL_SECONDS=999 .venv/bin/python -m pytest -q tests/unit/test_process_conversation_usage_context.py -m 'not integration and not slow'
Result: 20 passed.
The implementation now reuses the same availability authority as the set-preferred route when a stored preferred summarization app is not present in the enabled/installed slice, while still falling back when the app is missing or does not support memories. The added regression tests cover the in-slice preferred app, missing/stale preferred app, no preferred app, out-of-slice but available preferred app, and non-summarization app fallback paths.
I do not see code blockers. I’m leaving this as a positive maintainer signal rather than a formal approval because the PR touches the repo automation ratchet baseline under .github/, so it falls under the automation/workflow-sensitive approval guard.
by AI on behalf of David — if you need David’s attention urgently, please @Git-on-my-level and escalate with need human response.
Git-on-my-level
added
the
candidate-clean-pr
Contributor
Author
|
Not still in progress — this is complete and ready for review. The branch is a single commit, currently mergeable against latest |
Contributor
Author
|
@Git-on-my-level need human response — this PR (and the same automation-guard situation on #10094 / #10095, which only bump ratchet baselines with justifications) is complete and mergeable, blocked only on the required human approval for the |
undivisible
added a commit
that referenced
this pull request
Jul 20, 2026
…ollout state (#10094) # Summary - `GET /v1/dev/user/memories` no longer returns `403 missing_rollout_state` for accounts that were never enrolled in the canonical-memory rollout: the route now serves the authoritative legacy `memories` collection for them. - Every other deny reason (missing grant, malformed state, unsupported schema, uid mismatch) keeps the fail-closed 403 contract, as does `/v1/dev/user/memories/vector/search`, which has no legacy read surface. - The mode change is recorded through the shared `record_fallback` helper (`component=other`, `reason=policy`, `outcome=recovered`) — no new one-off counter. Fixes #9892. # Root cause Two authorities disagreed about the same account, and the fail-closed one won: - **Cohort selector (authoritative):** `pin_memory_system` → `resolve_memory_system` resolves everyone outside the code-reviewed `CANONICAL_MEMORY_USERS` whitelist to `MemorySystem.LEGACY` — "the documented default — not an implicit None fallback" (its own words). For these accounts, `users/{uid}/memory_control/state` is *expected* to be absent. - **Rollout gate:** `read_default_read_rollout` treats an absent doc as `DENY_MEMORY` / `missing_rollout_state` — correct fail-closed behavior for an account that might be mid-migration, wrong for one the cohort selector already pinned LEGACY in the same request. The route's legacy read path (`memories_db.get_memories` + per-doc validation) already existed but was unreachable: only `USE_LEGACY_SAFE` reaches it, and rollout normalization never produces `USE_LEGACY_SAFE` from Firestore state — only the explicit opt-in constructor does. So every un-enrolled account lost Developer API memory reads entirely, while `/v1/dev/user/conversations` and `/v1/dev/user/action-items` (no such gate) kept working — exactly the reported symptom, breaking e.g. the Obsidian omi-sync memories feed. # Failure class Failure-Class: none No registered class covers a reader failing closed on state whose absence the authoritative cohort selector defines as normal. It rhymes with FC-split-mutation-authority (competing authorities over one decision) but that class is scoped to mutable-state transition ownership, not read-side policy divergence; flagging rather than stretching — same call as #10078. # Durable guard - The fallback is *narrow and explicit*: only `missing_rollout_state` (doc absent = never enrolled) on the already-LEGACY branch falls back. `malformed_rollout_state`, `rollout_read_failed`, grant and schema denials all stay fail-closed, because there a doc exists (or its state is unknown) and the account may be mid-migration. - The previously silent-dead `should_use_legacy_fallback: pass` branch is gone; the decision structure now states every outcome. - Mode change is observable via the shared `omi_fallback_total` metric instead of being invisible. # Product invariants affected none (verified with `scripts/pr-preflight --suggest`) # Validation - Old-code regression check (route change stashed): `test_get_memories_missing_rollout_state_falls_back_to_legacy` → **fails** (403); with the fix → **passes**. Full file: 7 passed. - New tests extend the existing grant-ordering suite through its established seams: an un-enrolled legacy-cohort account lists legacy memories with 200 (grant check still runs first); a non-missing deny reason still 403s with the actionable `developer_memory_access_not_ready` contract; the vector-search missing-rollout 403 test keeps its contract untouched. - Repo runner (file-isolated, as CI executes): `tests/unit/test_dev_api_canonical_grant_ordering.py` → 7 passed, `tests/unit/test_developer_memory_adapter.py` → 24 passed, `tests/unit/test_memories_validation.py` → 12 passed, `tests/unit/test_dev_api_folder_filters.py` → included in sweep, passed. - The pre-push bounded gate ran the wider backend selection and passed; it caught (and I updated) the static source-order tripwire in `test_developer_memory_adapter.py` that encoded the old "no legacy after deny" ordering — it now asserts the narrow un-enrolled guard precedes the legacy read, and is labeled as a static tripwire per the repo's testing guidance. - `make preflight` (local lane, this PR body) → all selected checks pass. - `black --line-length 120 --skip-string-normalization` clean on both changed files. Not exercised live: a production Developer API key against a real un-enrolled account (no such credentials here). The fix point is server-side branch selection driven directly by the regression tests; the issue reporter's account can verify live after deploy. # Out of scope, named - MCP list/search (`routers/mcp.py`, `mcp_sse.py`) degrade differently for the same accounts (silent empty list instead of 403). Same underlying divergence, different surface and files — follow-up rather than scope creep here. - The issue's alternative ask (enrolling individual accounts) is an ops action, not a code fix; this makes it unnecessary for reads. <!-- This is an auto-generated description by cubic. --> <a href="https://cubic.dev/pr/BasedHardware/omi/pull/10094?utm_source=github" target="_blank" rel="noopener noreferrer" data-no-image-dialog="true"><picture><source media="(prefers-color-scheme: dark)" srcset="https://www.cubic.dev/buttons/review-in-cubic-dark.svg"><source media="(prefers-color-scheme: light)" srcset="https://www.cubic.dev/buttons/review-in-cubic-light.svg"><img alt="Review in cubic" src="https://www.cubic.dev/buttons/review-in-cubic-dark.svg"></picture></a> <!-- End of auto-generated description by cubic. -->
kodjima33
pushed a commit
that referenced
this pull request
Jul 20, 2026
![https://www.cubic.dev/buttons/review-in-cubic-dark.svg"><source](https://www.cubic.dev/buttons/review-in-cubic-dark.svg"><source){kind=link}
![https://www.cubic.dev/buttons/review-in-cubic-light.svg"><img](https://www.cubic.dev/buttons/review-in-cubic-light.svg"><img){kind=link}
![https://www.cubic.dev/buttons/review-in-cubic-dark.svg"></picture></a](https://www.cubic.dev/buttons/review-in-cubic-dark.svg"></picture></a){kind=link}
Creating a summary template from a conversation called enableAppServer directly and ignored its result: on failure the sheet still showed 'App created and installed!', opened the summarized-apps picker, and let the user set the never-enabled template as their default — the app-side half of #10074 (the server-side honor fix landed in #10078). The sheet now installs through AppProvider.toggleApp — the existing owner of enable/disable, which already reverts prefs/app-list state and shows the failure dialog — and reports the real outcome: success keeps the current flow, failure shows 'Failed to install {app}' instead of a false success, and the created-but-not-installed template stays visible in the store to retry. toggleApp now returns whether the toggle stuck (all existing callers ignore the return, unchanged). Not run locally (no Flutter SDK on this machine): change is confined to the sheet's install branch and a Future<void>->Future<bool> provider signature; app CI lane compiles and analyzes it. Failure-Class: none Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
kodjima33
approved these changes
Jul 20, 2026
kodjima33
left a comment
kodjima33
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed by automated PR reviewer — approving; holding merge (see notes).
Collaborator
|
Thanks LGTM |
Contributor
Author
|
@Git-on-my-level @undivisible This PR is fully approved (LGTM received) and ready to merge. Could you please trigger the merge when you have a moment? Thank you! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
PUT /v1/users/preferences/appis now actually applied at generation time, even when the app never landed in the user's enabled-installed slice.get_available_app_model_by_id, the new shared App-model form ofget_available_app_by_id) and uses it when it can summarize.memoriescapability falls through with a warning.Fixes #10074.
Root cause
Split availability authority between admission and use:
routers/users.py):get_available_app_by_id— existence + private-ownership only. Accepts the template and stores it in Redis._trigger_appsinutils/conversations/process_conversation.py): honored the preferred id only if present in theworks_with_memories() AND enabledinstalled slice.Any app passing the first check but not the second was accepted as the default and silently ignored at generation, falling through to the LLM suggestion path — exactly the reported behavior ("the template can be set as standard/default, but it is not applied"). Two concrete ways to get there without any user error:
enableAppServerand ignores failure (create_template_bottom_sheet.dart—if (success)with no else), then lets the user set the never-enabled app as default.Failure class
Failure-Class: none
No registered class covers split availability authority between an admission route and its consumer. It rhymes with
FC-split-mutation-authority(one authoritative owner per decision) but that class is scoped to mutable-state transitions, not read-side policy; declaring it here would stretch its definition. Flagging rather than stretching.Durable guard
Product invariants affected
none (verified with
scripts/pr-preflight --suggest)Validation
tests/unit/test_process_conversation_usage_context.py._trigger_appspreferred-app suite through its established seams: an admitted-but-not-installed template runs with no suggestion LLM call; a capability-less preferred app falls back to suggestions; the existing deleted-app fallthrough test keeps its contract (resolver returns None).tests/unit/test_process_conversation_usage_context.py→ 20 passed,tests/unit/test_lock_bypass_fixes.py→ 61 passed,tests/unit/test_listen_process_pending_shutdown.py→ 3 passed.test_process_conversation_usage_context.pyandtest_lock_bypass_fixes.pyin one shared pytest process shows 19 cross-file failures on clean main as well — pre-existing module-isolation interaction, which is exactly whytest.shruns file-isolated; not introduced or worsened here.make preflight(local lane, this PR body) → all selected checks pass.black --line-length 120 --skip-string-normalizationclean on the three changed files.Not exercised live: the full app→backend template flow against production (no device/credentials here). The fix point is server-side selection logic driven directly by the regression tests; the app-side create flow is unchanged.
Out of scope, named
enableAppServerfailure (create_template_bottom_sheet.dart:167) still deserves a user-visible error; with this fix it no longer breaks the default template, so it's a polish follow-up.app_idparameter) keeps its existing installed-slice lookup — different admission route, untouched.