fix(memories): list_banks NameError on per-bank capability check - #3381
Merged
nicoloboschi merged 1 commit intoAug 11, 2026
Merged
Conversation
list_banks consulted the per-bank capability with a bare bank_id that is not in scope inside the per-row loop (the row's id is row["bank_id"]). Since the argument is evaluated before the call, this raised NameError: name 'bank_id' is not defined for every org on the first bank — GET /banks 500'd outright — regardless of the store's capability. Introduced with per-bank store capabilities (#3350). Fix uses row["bank_id"]; adds a regression test covering the non-SQL store branch (fact_count sourced from the store's count_memories), which is where the bug lived.
nicoloboschi
added a commit
that referenced
this pull request
Aug 11, 2026
Re-applies the per-bank store-capability seam onto current main. The pluggable memories backend (#2917) is on main, but the per-bank capabilities landed later on feat/pluggable-memories-provider (#3350, plus fix #3381) while main advanced ~179 commits. A pluggable memories store may keep memory rows outside SQL and/or own the document store. The process-level flags writes_memory_rows_in_sql / owns_document_store gain per-bank forms — writes_memory_rows_in_sql_for(bank_id) and owns_document_store_for(bank_id), defaulting to the class attrs — and every bank-scoped call site in memory_engine, consolidation/consolidator, retain/* and reflect/tools consults the per-bank form. Process-level maintenance gates keep reading the class attr. Also two NameError fixes of the same class (a bare bank_id where the in-scope variable differs): list_banks (row["bank_id"], originally #3381) and get_chunk (chunk["bank_id"], newly surfaced by pyflakes while rebasing). Conflict resolution: only consolidation/consolidator.py conflicted — main added consolidation sites since the branch; all are bank_id-scoped, so all convert to the per-bank form. Validation: pyflakes on all changed engine files reports 0 undefined names; py_compile clean; per-bank + list_banks unit tests included.
nicoloboschi
added a commit
that referenced
this pull request
Aug 11, 2026
Re-applies the per-bank store-capability seam onto current main. The pluggable memories backend (#2917) is on main, but the per-bank capabilities landed later on feat/pluggable-memories-provider (#3350, plus fix #3381) while main advanced ~179 commits. A pluggable memories store may keep memory rows outside SQL and/or own the document store. The process-level flags writes_memory_rows_in_sql / owns_document_store gain per-bank forms — writes_memory_rows_in_sql_for(bank_id) and owns_document_store_for(bank_id), defaulting to the class attrs — and every bank-scoped call site in memory_engine, consolidation/consolidator, retain/* and reflect/tools consults the per-bank form. Process-level maintenance gates keep reading the class attr. Also two NameError fixes of the same class (a bare bank_id where the in-scope variable differs): list_banks (row["bank_id"], originally #3381) and get_chunk (chunk["bank_id"], newly surfaced by pyflakes while rebasing). Conflict resolution: only consolidation/consolidator.py conflicted — main added consolidation sites since the branch; all are bank_id-scoped, so all convert to the per-bank form. Validation: pyflakes on all changed engine files reports 0 undefined names; py_compile clean; per-bank + list_banks unit tests included.
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
1 participant
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.
Problem
GET /v1/default/banksreturns HTTP 500 for every org:Introduced with per-bank store capabilities (#3350). Inside
list_banks, the per-row loopvariable is
row["bank_id"]— there is no barebank_idin scope. Because the argument isevaluated before the call, this raises for every org on the very first bank, regardless of
what the store's capability would have returned. The line immediately below already uses
row["bank_id"]correctly.Found live: routing a dev org's memories to an out-of-SQL store (memlake), the bank list 500'd
while single-bank reads worked.
Fix
One line: pass
row["bank_id"].Test
Adds
tests/test_list_banks_non_sql_store.py: swaps in a store reportingwrites_memory_rows_in_sql_for -> Falseand assertslist_banks(a) doesn't raise, (b) consultsthe capability +
count_memorieswith the correct per-bank id, and (c) sourcesfact_countfromthe store. That non-SQL branch is exactly where the bug lived and was previously untested.