Skip to content

fix(backend): a __name__ filter needs a Key, so both usage readers ra… - #12066

Merged
kodjima33 merged 1 commit into
BasedHardware:mainfrom
abunet:fix/llm-usage-name-filter-needs-a-key
Aug 26, 2026
Merged

fix(backend): a __name__ filter needs a Key, so both usage readers ra…#12066
kodjima33 merged 1 commit into
BasedHardware:mainfrom
abunet:fix/llm-usage-name-filter-needs-a-key

Conversation

@abunet

@abunet abunet commented Aug 22, 2026

Copy link
Copy Markdown

…ised on every call

get_usage_summary and get_plan_usage_report bound their scan with where("__name__", ">=", cutoff_id), where cutoff_id is a date string like 2026-01-01. Firestore requires a Key there:

google.api_core.exceptions.InvalidArgument: 400 __key__ filter value must be a Key

Measured against the Firestore emulator: both functions raise, for every user, every time. The __name__ field is fine — the value's TYPE is not.

The document ids ARE the dates, so the cutoff is simply usage_collection.document(cutoff_id). The window is unchanged; only the type is.

Worth knowing for whoever reviews this: fixing it alone makes get_usage_summary return {} rather than raise, because a second, independent defect keeps the counters from ever nesting (a sibling PR: these documents are written with set(merge=True) and dotted keys, which Firestore stores as literal field names). With both applied, the same call against the emulator returns {'chat': {'input_tokens': 5, 'output_tokens': 3, 'call_count': 1}}. Either PR stands alone; neither restores the feature by itself.

One observation not addressed here, to keep this change to one thing: once the counters do nest, _aggregate_summary also reports plan_usage as if it were a feature ({'input_tokens': 0, ...}), because it walks every top-level map. That is a separate line and a separate decision.

Verification
emulator, before: InvalidArgument 400 key filter value must be a Key
emulator, after: the query runs and streams the in-window documents
new tests/unit/test_llm_usage_name_filter_takes_a_key.py 3 passed; passing the bare string again
fails all 3
tests/unit/test_llm_usage_db.py 19 - test_llm_usage_tracker.py 18 - test_llm_usage_endpoints.py 2

What changed and why

Product invariants affected

How it was verified

Tests

Failure class (fixes)

Failure-Class: none

Failure-class transition narrative (only when needed)

New guards (only when adding a check or ratchet)

Scoped cleanups (optional)

Review in cubic

@kodjima33 kodjima33 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confidence 4/5 (clear root cause, scoped diff ~126 lines/2 files, new dedicated regression test proving the key TypeError via emulator) — but the required 'PR Metadata Preflight'/'Hygiene' check (failure-class-protocol) fails: the PR body declares both 'Failure-Class: new' and the unfilled template's 'Failure-Class: none'. Approving but NOT merging per the CI hard floor; author needs to clean up the PR body before this can land. Note: per this PR's own body, applying it alone still leaves get_usage_summary returning {} until the sibling PR #12065 (nested-write fix) also lands.

…ised on every call

Failure-Class: none

`get_usage_summary` and `get_plan_usage_report` bound their scan with
`where("__name__", ">=", cutoff_id)`, where `cutoff_id` is a date string like `2026-01-01`. Firestore
requires a Key there:

    google.api_core.exceptions.InvalidArgument: 400 __key__ filter value must be a Key

Measured against the Firestore emulator: both functions raise, for every user, every time. The
`__name__` field is fine — the value's TYPE is not.

The document ids ARE the dates, so the cutoff is simply `usage_collection.document(cutoff_id)`. The
window is unchanged; only the type is.

Worth knowing for whoever reviews this: fixing it alone makes `get_usage_summary` return `{}` rather
than raise, because a second, independent defect keeps the counters from ever nesting (a sibling PR:
these documents are written with `set(merge=True)` and dotted keys, which Firestore stores as literal
field names). With both applied, the same call against the emulator returns
`{'chat': {'input_tokens': 5, 'output_tokens': 3, 'call_count': 1}}`. Either PR stands alone; neither
restores the feature by itself.

One observation not addressed here, to keep this change to one thing: once the counters do nest,
`_aggregate_summary` also reports `plan_usage` as if it were a feature (`{'input_tokens': 0, ...}`),
because it walks every top-level map. That is a separate line and a separate decision.

Verification
  emulator, before: InvalidArgument 400 __key__ filter value must be a Key
  emulator, after:  the query runs and streams the in-window documents
  new tests/unit/test_llm_usage_name_filter_takes_a_key.py 3 passed; passing the bare string again
    fails all 3
  tests/unit/test_llm_usage_db.py 19 - test_llm_usage_tracker.py 18 - test_llm_usage_endpoints.py 2
@abunet
abunet force-pushed the fix/llm-usage-name-filter-needs-a-key branch from 3b06590 to c8ec7fb Compare August 23, 2026 15:55

@kodjima33 kodjima33 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fix(backend): name filter requires a Key not a string — confirmed still broken on main (raises 400 on every call). Root cause proven against emulator, scoped diff (126 lines), new regression tests. Not merging yet: 'Backend Hermetic Merge Gate' shows failure (downstream of a cancelled 'Detect Hermetic Backend Scope' run, likely a concurrency artifact from re-running CI on this branch) and 'Backend unit suite'/'Hygiene' are still mid-run. Confidence would be 5/5 once CI is unambiguously green — re-check before merging.

abunet added a commit to abunet/omi that referenced this pull request Aug 24, 2026
No conflicts. 214 files, +11706/-4903, mostly desktop (63 macOS, 30 Windows). No new
database/ module, which is the recurring cost of these merges and the first thing checked.

Three fixes we sent upstream were merged on 2026-08-24 and arrive here as upstream's own code:

  BasedHardware#12090  Enter mid-IME-composition no longer sends the chat message. The merge creates
          web/app/src/lib/chatComposerKey.ts — our file, now theirs.
  BasedHardware#12092  the app-store page's two store buttons, both 404 until now
  BasedHardware#12095  a failed account-deletion wipe backs off instead of being re-selected every tick

That is the loop working as intended: a fix sent upstream returns by itself and leaves no
local delta behind. Our BasedHardware#12065 and BasedHardware#12066 are still open.

Post-merge audit (ADR-0029/0030): nothing to resolve.

  ports        untouched — the only match under database/store, utils/{auth,vector,object_store},
               _client.py or the notification transport is a test file that matched on its name
  fidelity     no raw persistence and no firebase_admin.auth in any backend file upstream changed
  guards       9/9 exit 0

Verification
  contract lane 55 files, 1083 passed, 78 skipped, 0 failed
  full sweep 1021 files in 2m36s: 1000 pass, 21 fail — 20 are the testing/e2e tier with no live
    services (torn down for unrelated cleanup) and the 21st is the inherited
    sys_modules_hermeticity residual. No regression.
  the sweep helper and its baselines lived in /tmp and were lost to a host reboot; both were
    rebuilt from the committed recipe in deploy/onprem/SELFHOST_NOTES.md — which is the reason
    that recipe is committed
abunet added a commit to abunet/omi that referenced this pull request Aug 25, 2026
…back and closed a pinned divergence

Conflicts, all resolved hunk by hunk (never `--theirs` on a whole file — L58):

  backend/main.py            both sides added an import in the same place. Union.
  backend/routers/referrals.py  upstream widened `claim_referral_trial` to return `(claimed, reason)`
                             and reads the account age through `firebase_admin.auth`. Took their
                             product change, kept our neutral auth port (ADR-0034): the creation time
                             is already a neutral scalar on UserProfile, and reading the Firebase
                             UserRecord here would make the referral trial Firebase-only.
  backend/utils/other/storage.py  eleven hunks. Upstream's BasedHardware#11992 makes the GCS client pointable at a
                             local dev harness — the same problem our object-store port already
                             solves one level up, so we keep ours. But it also carries a real product
                             fix we did take: `upload_sdcard_audio` used to write the object at
                             `file_path` while minting a URL for `sdcard/{file_path}`. We had noted
                             that asymmetry and left it; upstream repaired it, so now the object goes
                             where the URL says it is.

L58 check after resolving: our deltas are intact (66 `_object_store()`, 26 `_signed_url`), and the one
thing of theirs that auto-merged into storage.py was an import of `create_storage_client` /
`local_public_url`, which nothing in our tree calls. Removed rather than left dead.

Post-merge audit (ADR-0029/0030): one finding, from the guards.

  check_oss_object_store_boundary   `utils/other/local_storage.py` is new, and talks raw GCS (3 hits)
      against an empty baseline. It is upstream's dev-harness storage client — a stand-in shaped like
      `google.cloud.storage.Client` (bucket/blob/upload_from_filename/generate_signed_url), i.e. they
      abstracted at the vendor-SDK level where we abstracted at a neutral port. In our tree its GCS
      paths are unreachable: only `local_storage_root_from_env` and `LocalStorageClient` are imported,
      and production `storage.py` goes through the port. Recorded in the baseline rather than ported —
      a filesystem adapter would be a third way to do what the S3/RustFS adapter already does — but
      recorded, so wiring it into a real path shows up.

Four tests re-expressed on our seams rather than deleted:

  test_referrals.py           upstream's new case patched `routers.referrals.firebase_admin`, absent
                              here. Now uses the `_bind_profile` helper the port migration added.
  test_local_storage.py (x2)  upstream asserts the production helpers return harness URLs. Ours go
                              through the port and return SIGNED urls (ADR-0087). The property kept is
                              the same, including their good one: deriving a static URL must not build
                              a credentialed client — true here too, because the GCS adapter's client
                              is lazy and `public_url` composes from the endpoint.
  test_llm_usage_contract.py  this one is the story. It pinned two Firestore divergences; BasedHardware#12065 —
                              ours, merged upstream in this range — closed the first, so both backends
                              now nest alike and the assertion is inverted to prove it. The second
                              (`__name__` filtered with a string) is still open as BasedHardware#12066 and stays
                              pinned. Renamed accordingly.

Verification
  full sweep 1026 files in 2m39s: 1021 pass, 5 fail — FAIL set identical to the pre-merge baseline
  contract lane 55 files, 1083 passed, 78 skipped, 0 failed
  guards 9/9, black 24.4.2 clean
  the test image, backend image and Firestore emulator were rebuilt from the committed recipes after
  a Docker GC reclaimed them mid-session; `deploy/onprem/Dockerfile.emulator` is the ANDROID emulator,
  the Firestore one is `deploy/onprem/firestore-emulator/Dockerfile` — a 10GB detour worth writing down

@kodjima33 kodjima33 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Backend bug fix, 5/5 confidence: stated root cause with reproduced Firestore error, fix(backend) title, scoped diff (124 add/2 del, 2 files), CI green (Backend unit suite + Hermetic Merge Gate pass), regression tests added (3 new, verified fail on revert). Confirmed still broken on origin/main (bare-string name filter in both get_usage_summary and get_plan_usage_report), not superseded by sibling PR #12065's separate nesting fix.

@kodjima33
kodjima33 merged commit e8d15c0 into BasedHardware:main Aug 26, 2026
34 of 36 checks passed
abunet added a commit to abunet/omi that referenced this pull request Aug 27, 2026
359 files, one conflict: our test stubbed two names on utils.byok where upstream now stubs one and
returns a MagicMock from it. Took theirs, and dropped our second stub — upstream's import cleanup
(BasedHardware#11303) removed `get_firestore_client` from subscription.py, so stubbing it would only describe a
dependency that no longer exists.

BasedHardware#12066 came home. `e8d15c06fe` is our fix for the `__name__` filter needing a Key. The contract test we
wrote for that divergence said, in its own docstring, "proposed upstream as BasedHardware#12066; pinned here until it
lands" — it landed, Firestore stopped raising, and the test went red with DID NOT RAISE. That is the pin
working, not a regression. Both reasons it recorded are now closed, so its assertions are symmetric
across the two backends and its name says so.

ADR-0030 audit: no finding. Nine guards at 0, port markers intact (120 `_object_store()`,
264 `_store()`, 183 `_signed_url`, 58 `get_auth_provider`).

The sweep first reported 272 red files. That was one missing dependency, not 272 defects:
`langchain-anthropic==1.1.0` is new in this merge and the test image predated it. Rebuilt base and test
images from the committed compose — 272 to 16.

Of those 16, ten fail IDENTICALLY on a pristine upstream/main worktree, test for test and count for
count — all in the chat/BYOK/quota cluster their own commits touched. Their CI does not see them because
it is diff-scoped (`run-unit-ci.sh --changed-files` runs only what the PR diff touches) while our sweep
runs everything. The known-residual set therefore grows from 5 to 15 through no change of ours, and it
is written down here so a later session does not read them as regressions.

Verification: contract lane dual-backend 1311 passed / 0 failed; Dart analyzer ratchet passed with 1538
Flutter tests green; sweep 1030 PASS / 16 FAIL = the residual set above; pyright 1.1.403 0 errors;
guard 0 x9; black clean across all 422 changed backend files.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants