From 9c59a04b41cff4b0593688f903ffefa9d6b048c9 Mon Sep 17 00:00:00 2001 From: Aryan Date: Mon, 20 Jul 2026 11:32:52 +0530 Subject: [PATCH] fix(dev): serve legacy memories when a legacy-cohort account has no rollout state MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit GET /v1/dev/user/memories fails closed with 403 missing_rollout_state for any account whose users/{uid}/memory_control/state doc is absent — which is the expected state for every account outside the canonical-memory cohort whitelist. The route already resolved the authoritative cohort via pin_memory_system (LEGACY) and already carries a legacy read path, but that path was unreachable: rollout normalization never yields USE_LEGACY_SAFE from Firestore state, so un-enrolled accounts lost Developer API memory reads entirely (conversations and action items, which have no such gate, kept working) (#9892). The reader now treats missing_rollout_state on the legacy branch as the un-enrolled case and serves the authoritative legacy memories collection, recording the mode change through the shared record_fallback helper. Every other deny reason (grant missing, malformed state, schema, uid mismatch) keeps the fail-closed 403 contract, as does the vector search endpoint, which has no legacy read surface. Regression tests: an un-enrolled legacy-cohort account lists legacy memories with 200; a non-missing deny reason still 403s with the actionable error contract. Fixes #9892. Co-Authored-By: Claude Fable 5 Failure-Class: none --- .../backend-routers.json | 3 +- backend/routers/developer.py | 19 ++++-- .../test_dev_api_canonical_grant_ordering.py | 66 +++++++++++++++---- .../unit/test_developer_memory_adapter.py | 12 +++- 4 files changed, 78 insertions(+), 22 deletions(-) diff --git a/.github/scripts/product_file_line_count_ratchet_baseline/backend-routers.json b/.github/scripts/product_file_line_count_ratchet_baseline/backend-routers.json index 9390f843905..ce7fb984a4d 100644 --- a/.github/scripts/product_file_line_count_ratchet_baseline/backend-routers.json +++ b/.github/scripts/product_file_line_count_ratchet_baseline/backend-routers.json @@ -2,12 +2,13 @@ "files": { "backend/routers/apps.py": 2335, "backend/routers/chat.py": 1577, - "backend/routers/developer.py": 2184, + "backend/routers/developer.py": 2195, "backend/routers/mcp_sse.py": 1857, "backend/routers/sync.py": 2024, "backend/routers/users.py": 2046 }, "raise_justifications": { + "backend/routers/developer.py": "GET /v1/dev/user/memories serves the authoritative legacy memories collection for legacy-cohort accounts with no rollout state (#9892), keeping the narrow deny/fallback decision in the route that owns the read contract.", "backend/routers/chat.py": "PTT stereo rejection keeps the serving STT provider boundary explicit in the established chat admission owner.", "backend/routers/sync.py": "Fresh Sync admission and final Cloud Tasks ledger recovery retain their coupled task, lock, ledger, and staged-blob lifecycle in the existing ingestion owner rather than a new module.", "backend/routers/users.py": "GET /v1/users/subscription serializes the new mobile plus/max plans as `unlimited` for clients whose plan enum predates them, so day-one buyers read as paid instead of Free (mirrors the existing operator remap); real limits/grandfather are computed from the true plan first." diff --git a/backend/routers/developer.py b/backend/routers/developer.py index d7060aff0a4..760497b350c 100644 --- a/backend/routers/developer.py +++ b/backend/routers/developer.py @@ -80,6 +80,7 @@ guard_legacy_memory_write, read_default_read_rollout, ) +from utils.observability import record_fallback import logging logger = logging.getLogger(__name__) @@ -393,11 +394,21 @@ def get_memories( if memory_result.read_decision == MemoryReadDecision.USE_MEMORY: return [CleanerMemory.model_validate(memory) for memory in memory_result.memories] if memory_result.read_decision in {MemoryReadDecision.DENY_MEMORY, MemoryReadDecision.SHADOW_ONLY}: - raise HTTPException( - status_code=403, detail=_developer_memory_access_not_ready_detail(memory_result.fallback_reason) + if memory_result.fallback_reason != 'missing_rollout_state': + raise HTTPException( + status_code=403, detail=_developer_memory_access_not_ready_detail(memory_result.fallback_reason) + ) + # pin_memory_system above already resolved this account to LEGACY, so an absent + # memory_control/state doc is the expected un-enrolled state and the legacy + # `memories` collection is the authoritative read surface — not a fail-closed + # migration condition (#9892). + record_fallback( + component='other', + from_mode='memory_default_read', + to_mode='legacy_memories', + reason='policy', + outcome='recovered', ) - if memory_result.should_use_legacy_fallback: - pass memories = memories_db.get_memories(uid, limit, offset, [c.value for c in category_list]) # Validate each record individually so a single malformed/legacy doc (e.g. missing a required diff --git a/backend/tests/unit/test_dev_api_canonical_grant_ordering.py b/backend/tests/unit/test_dev_api_canonical_grant_ordering.py index 0a19b1d344b..c768fe06c7d 100644 --- a/backend/tests/unit/test_dev_api_canonical_grant_ordering.py +++ b/backend/tests/unit/test_dev_api_canonical_grant_ordering.py @@ -375,21 +375,60 @@ def test_get_memories_allowed_grant_canonical_lists(): assert resp.json()[0]['id'] == 'canon-1' -def test_get_memories_missing_rollout_state_has_actionable_contract(): - """A valid memory-read key must not look invalid when account rollout is absent.""" +def _denied_memory_result(fallback_reason): + return type( + 'DeniedMemoryResult', + (), + { + 'read_decision': developer_module.MemoryReadDecision.DENY_MEMORY, + 'memories': [], + 'fallback_reason': fallback_reason, + 'should_use_legacy_fallback': False, + }, + )() + + +def test_get_memories_missing_rollout_state_falls_back_to_legacy(): + """A legacy-cohort account with no rollout doc reads legacy memories, not 403 (#9892). + + pin_memory_system already resolved the account to LEGACY, so an absent + memory_control/state doc is the expected un-enrolled state — the route must + serve the authoritative legacy `memories` collection instead of failing closed. + """ client = _build() developer_module.search_memory_default_developer_memories = MagicMock( - return_value=type( - 'DeniedMemoryResult', - (), - { - 'read_decision': developer_module.MemoryReadDecision.DENY_MEMORY, - 'memories': [], - 'fallback_reason': 'missing_rollout_state', - 'should_use_legacy_fallback': False, - }, - )() + return_value=_denied_memory_result('missing_rollout_state') + ) + + legacy_memory = { + 'id': 'legacy-1', + 'content': 'a legacy memory', + 'category': _VALID_CATEGORY, + 'visibility': 'private', + 'tags': [], + 'manually_added': False, + 'reviewed': False, + 'edited': False, + } + with __import__('unittest.mock', fromlist=['patch']).patch.object( + developer_module.memories_db, 'get_memories', return_value=[legacy_memory] + ): + resp = client.get('/v1/dev/user/memories') + + assert resp.status_code == 200 + body = resp.json() + assert len(body) == 1 + assert body[0]['id'] == 'legacy-1' + assert developer_module.authorize_memory_external_default_memory_read.called + + +def test_get_memories_other_deny_reason_still_403(): + """Deny reasons other than missing_rollout_state keep the fail-closed contract.""" + client = _build() + + developer_module.search_memory_default_developer_memories = MagicMock( + return_value=_denied_memory_result('missing_developer_default_memory_grant') ) resp = client.get('/v1/dev/user/memories') @@ -397,9 +436,8 @@ def test_get_memories_missing_rollout_state_has_actionable_contract(): assert resp.status_code == 403 detail = resp.json()['detail'] assert detail['code'] == 'developer_memory_access_not_ready' - assert detail['reason'] == 'missing_rollout_state' + assert detail['reason'] == 'missing_developer_default_memory_grant' assert 'key can be valid and correctly scoped' in detail['message'] - assert developer_module.authorize_memory_external_default_memory_read.called def test_search_memories_vector_missing_rollout_state_has_actionable_contract(): diff --git a/backend/tests/unit/test_developer_memory_adapter.py b/backend/tests/unit/test_developer_memory_adapter.py index b761ff38d32..4c73b5799dc 100644 --- a/backend/tests/unit/test_developer_memory_adapter.py +++ b/backend/tests/unit/test_developer_memory_adapter.py @@ -185,16 +185,22 @@ def test_developer_update_route_checks_split_brain_guard_before_reads_and_legacy def test_developer_routes_only_reach_legacy_after_explicit_legacy_safe_decision(): + # Static tripwire (source order, not behavior): the list route may reach the + # legacy read only through the deny branch's narrow un-enrolled guard (#9892); + # the vector route still requires an explicit legacy-safe decision. developer_py = Path(__file__).resolve().parents[2] / 'routers' / 'developer.py' contents = developer_py.read_text(encoding='utf-8') denied_check = 'if memory_result.read_decision in {MemoryReadDecision.DENY_MEMORY, MemoryReadDecision.SHADOW_ONLY}:' - legacy_safe_check = 'if memory_result.should_use_legacy_fallback:' + unenrolled_guard = "if memory_result.fallback_reason != 'missing_rollout_state':" legacy_call = 'memories_db.get_memories(uid, limit, offset, [c.value for c in category_list])' assert denied_check in contents - assert legacy_safe_check in contents + assert unenrolled_guard in contents assert legacy_call in contents - assert contents.index(denied_check) < contents.index(legacy_safe_check) < contents.index(legacy_call) + assert contents.index(denied_check) < contents.index(unenrolled_guard) < contents.index(legacy_call) vector_route_source = _function_source_for_route('/v1/dev/user/memories/vector/search', 'get') + legacy_safe_check = 'if memory_result.should_use_legacy_fallback:' + assert denied_check in vector_route_source + assert legacy_safe_check in vector_route_source assert vector_route_source.index(denied_check) < vector_route_source.index(legacy_safe_check)