fix(backend): give the chat test stubs the names their real modules export - #12292
fix(backend): give the chat test stubs the names their real modules export#12292aryanorastar wants to merge 5 commits into
Conversation
…xport
Five unit files fail to import on main. Each installs a hand-written stand-in
for a real module, and the stand-ins never gained names the production code
started importing:
ImportError: cannot import name 'get_byok_uid' from 'utils.byok' (unknown location)
ImportError: cannot import name 'get_current_context' from 'utils.llm.usage_tracker' (unknown location)
ImportError: cannot import name 'CHAT_AGENT_ROUTE_DIRECT' from 'utils.llm.gateway_client' (unknown location)
ImportError: cannot import name 'get_chat_agent_route' from 'utils.llm.gateway_client' (unknown location)
ModuleNotFoundError: No module named 'utils.llm.gateway_client'; 'utils.llm' is not a package
"(unknown location)" is the tell: the module in sys.modules is a stub with no
spec origin, so the name is missing from the stand-in rather than from the
package. Every real symbol exists -- utils/byok.py, usage_tracker.py:175,
gateway_client.py:36 and :158.
Four sites, all adding what the real module already exports:
- _chat_router_test_harness.py: usage_tracker.get_current_context. Real
signature returns Optional[UsageContext]; None is the no-active-context
value these router tests run under.
- test_chat_file_upload_unsupported.py: gateway_client.CHAT_AGENT_ROUTE_DIRECT
and get_chat_agent_route. Importers read them at module import time, so the
stub carries them even though this test never routes.
- test_chat_quota.py: byok get_byok_uid and get_cached_byok_state, which
utils.subscription imports at module scope.
- test_desktop_transcribe.py: utils.llm.gateway_client in the stub list.
utils.llm is a MagicMock, not a package, so an unlisted submodule fails to
resolve instead of falling back to the stub.
before: 5 files, 0 tests run -- collection failed
after: 108 passed
Not fixed here: tests/unit/test_byok_security.py, 8 failures across four
classes. Those patch utils.subscription.get_byok_keys, which
request_has_llm_byok_key no longer calls -- it moved to get_byok_uid /
get_cached_byok_state / get_byok_key. Repointing the patch would bind it to a
function the code under test does not use, so those need new assertions
written against the current contract rather than a rename. Tracked in BasedHardware#12289.
Failure-Class: FC-mirrored-model-omits-new-member
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
…ocked
Fixing the stubs moved these files from "collection error, never ran" to
"runs and is measured", which exposed 15 node IDs to the fast-unit duration
guard for the first time:
Backend fast unit duration guard failures (CPU time)
0.33s > 0.30s test_generate_reply_never_returns_a_staged_error_answer_as_reply_text
Every test passes; only the CPU budget fails. This is the amortization the
allowlist header already documents -- each file runs in its own pytest process,
so the first call pays the FastAPI app and router graph import. Sibling node IDs
from these same files are already grandfathered for exactly that reason.
Appended as its own commented group rather than merged into the sorted set: the
file is grouped by rationale, not sorted, so re-sorting it would have produced
107 insertions and 115 deletions for 15 real additions.
Verified with the repo runner rather than bare pytest -- bare pytest omits the
timing guard, which is why my first pass reported these files green when the
suite still failed them.
Failure-Class: none
|
Verified each of the four stub additions against current
CI on this head backs the fix: all four touched files pass, and the shared-harness consumers ( Surgical, well-commented fix with a clearly drawn boundary — leaving #12289's semantic BYOK question out of scope was the right call. From my side this is ready for maintainer merge. by AI on behalf of David — if you need David’s attention urgently, please @Git-on-my-level and escalate with |
Git-on-my-level
added
positive-signal
kodjima33
left a comment
kodjima33
left a comment
There was a problem hiding this comment.
Root cause and fix are correct (test stubs missing names their real modules now export) and CI-red items aside, this is a scoped 15-line test-only fix. Holding merge: Backend unit suite check is currently failing on this PR.
…-exports # Conflicts: # backend/tests/unit/test_chat_file_upload_unsupported.py # backend/tests/unit/test_desktop_transcribe.py
Refreshed on current
|
|
Fresh CI is complete on |
…o fix/test-stub-missing-exports # Conflicts: # backend/tests/fast_unit_duration_allowlist.txt
|
Verified the refreshed head
Backend CI is green on this head (Backend unit suite, Backend Hermetic Merge Gate, Hermetic Backend E2E all passing). The only commits after the approval are merges of by AI on behalf of David — if you need David’s attention urgently, please @Git-on-my-level and escalate with |
|
Closing — main fixed this without me. #12339 landed the stub names, and the harness on main now carries Verified against a pristine Worth recording one mistake from this PR: I first verified with bare pytest and reported the files green when the suite still failed them. |
|
Hey @aryanorastar 👋 Thank you so much for taking the time to contribute to Omi! We truly appreciate you putting in the effort to submit this pull request. After careful review, we've decided not to merge this particular PR. Please don't take this personally — we genuinely try to merge as many contributions as possible, but sometimes we have to make tough calls based on:
Before your next PR, please skim:
If this was declined for direction or taste, maintainers should cite an invariant ID or open a proposed one — ask if that citation is missing. Your contribution is still valuable to us, and we'd love to see you contribute again in the future! If you'd like feedback on how to improve this PR or want to discuss alternative approaches, please don't hesitate to reach out. Thank you for being part of the Omi community! |
3 participants
What changed and why
Two files, both test-side. The scope narrowed as
mainabsorbed the rest — see the note at the bottom.1.
_chat_router_test_harness.py— one missing stub name.The shared chat-router harness installs a stand-in for
utils.llm.usage_trackercarryingset_usage_context,reset_usage_contextandFeatures. The production code also importsget_current_context, so every suite using the harness died at collection:(unknown location)is the tell — the module insys.modulesis a stub with no spec origin, so the name is missing from the stand-in rather than the package. The real symbol isusage_tracker.py:175and returnsOptional[UsageContext];Noneis the no-active-context value these router tests run under.2.
fast_unit_duration_allowlist.txt— 15 node IDs.Fixing the stub moved these files from "collection error, never ran" to "runs and is measured", which exposed them to the fast-unit duration guard for the first time:
Every test passes; only the CPU budget fails. This is exactly the amortization the allowlist header already documents — each file runs in its own pytest process, so the first call pays the FastAPI app and router graph import — and sibling node IDs from these same files are already grandfathered for that reason.
Appended as its own commented group rather than merged into the existing set: the file is grouped by rationale, not sorted, so re-sorting would have produced 107 insertions and 115 deletions for 15 real additions.
How it was verified
With the repo's own runner, not bare pytest:
Correcting my own first pass on this PR: I originally verified with bare pytest and reported these files green. They were not —
test.shapplies a timing guard bare pytest omits, and three files still failed the suite. The runner says so directly ("Do not use bare pytest for fast-unit timing failures; it omits test.sh's guard settings") and I should have used it from the start. The allowlist commit exists because of that miss.Scope note
This PR opened covering five files. #12339 landed the stub fixes for
test_chat_quota.py,test_chat_file_upload_unsupported.pyandtest_desktop_transcribe.pyonmainin the meantime, so those are gone from the diff — what remains is the harness name and the allowlist entries, neither of which is onmain.Still not fixed, and not mine to guess at
test_byok_security.py(8 failures),test_chat_session_app_identity.py(1) andtest_paywall_reconnect_gate.py(2) all fail on pristinemainwith BYOK expectation drift —assert True is Falseagainst a reshapedrequest_has_llm_byok_key, which no longer callsget_byok_keys. Those need assertions rewritten against the current contract, which is a semantic question about intended BYOK behaviour. Traced in #12289.Product invariants affected
none
Failure-Class: FC-mirrored-model-omits-new-member