{{ message }}
Repair the tests main is currently red on - #9192
Merged
Merged
Conversation
node --experimental-strip-types resolves relative specifiers literally, so the extensionless import added in #9173 fails at import time and takes queued-model-capabilities.test.ts down with it. Before that commit this module had only import type lines, which are erased, so it carried no runtime relative import at all and the test passed. 55 other modules already spell the extension.
This was referenced Aug 18, 2026
Member
Author
The mmproj fallback landing added a mmprojFallbackMessage import to chat-adapter.ts. Two repo tests read that file's source: - test_chat_autoload_failure_gate slices autoLoadSmallestModel out of the adapter and runs it, and its own guard fails the whole file when the slice references a name the harness preamble does not define. Add the stub. - test_model_picker_contracts pinned the success toast's description as the literal 'description: cpuFallbackReason'. The mmproj branch now sits ahead of the CPU one, so pin the description expression instead of which fallback is tested first. 71 failures on main, none of them a real regression.
test_tensor_split_abort_raises_early_to_layer_fallback pins the order of two branches in load_model by their source offsets. #9173 renamed the variable handed to _strip_mmproj_args, so the find() of the old spelling returned -1 and the ordering assert compared against it. Pin the call itself and fail loudly when it is no longer reachable.
exercise_permission_mode_controls asserts the permission pill visible on the 5s expect default, immediately after the composer mounts. The composer can mount and then drop back behind the root Suspense fallback while the route chunk loads, so the assertion lands on 'Loading...' and gives up. Reproduced on main at aa32c18: same step, same line, aria snapshot '- text: Loading...'. It is Windows-only in practice because the mac smoke wraps the script in a 3-attempt retry and the Windows one does not, so a slow 4-vCPU runner surfaces the race raw. Wait the pill out on the same budget the composer already uses.
The macOS smoke hit the same Suspense race at the second assertion, which the first commit did not cover. Four of the six assertions in this block follow page.reload(wait_until = domcontentloaded), which returns long before the lazy chat chunk resolves, so asserting on expect's 5s default races it by construction rather than only on a slow runner. Route all six through one helper that waits on the composer's budget. Seen on both the Windows and macOS smokes as 'Aria snapshot: - text: Loading...' at playwright_chat_ui.py:135 and :189.
This was referenced Aug 18, 2026
The CPU test job has no playwright, so tests/studio/test_heavy_thread_measurement_integrity.py
puts a stub `playwright.sync_api` into sys.modules at collection time rather than skipping the
harness arithmetic along with the browser. That stub defined one name, `sync_playwright`, which
is all its own harness imports.
sys.modules is session-wide, so the stub is not read only by the file that installs it. Three of
the four harnesses import `sync_playwright` alone and were satisfied by it; playwright_strip_ansi_smoke
also imports `Page` and `expect`, and got
ImportError: cannot import name 'Page' from 'playwright.sync_api' (unknown location)
from a stub two files away. `(unknown location)` is the stub itself: a bare ModuleType has no
__file__. The two tests that import that harness, test_the_ansi_dump_survives_a_vite_server_that_is_
still_talking and test_an_empty_smoke_base_url_means_unset[playwright_strip_ansi_smoke], failed on
it. pytest.importorskip("playwright") does not save them, because by then the stub package is
importable.
This is pre-existing on main and predates this branch, which never touched either file. It was
masked: the same job was failing with 71 other errors, which this PR fixes, so these two were
never the reported cause. They belong here because this is the PR that makes the job green.
The stub now answers any public name with a callable that raises, so a harness can satisfy its
imports without quietly measuring a browser that is not there, and adding an import to a harness
cannot break a different file again. Dunders still raise AttributeError, since pytest and inspect
probe those and answering them makes the stub look like a package. The install probe also imports
off the submodule now: a partial install leaves `playwright` importable while `playwright.sync_api`
resolves to an empty namespace, which fails the same way.
Verified by blocking playwright in a subprocess to reproduce the CPU job: 2 failed, 104 passed
before, 106 passed after, with the two files alone skipping cleanly when the stub is not collected.
The heavy-thread browser smoke fails its own harness check on this branch:
HARNESS-BROKEN chromium at 25000 chars let 2 /api/ requests reach the
network during the measured actions; the timings include a round trip
per request
Both leaked requests are GET /api/chat/threads/__LOCALID_.../forks, one during
seeding and two more inside the measured actions. The smoke page answers a small
allowlist of endpoints in-page so no round trip lands in a timed region, and the
fork-count entry in that allowlist still matches the per-message endpoint,
/threads/{id}/messages/{id}/forks. #8992 replaced it with one per-thread request
built by getThreadForkCounts, and the allowlist was not moved with it, so every
fork-count GET went to the dev server. fork-count-store refreshes on
CHAT_HISTORY_UPDATED_EVENT, which the delete action fires, which is why two of
them land inside the measurement rather than only at load.
Match the URL the client builds and answer the body the endpoint returns:
getThreadForkCounts reads data.counts into a Map, so {"counts":{}} is "no
message has forks" and renders no badge. The fixture is unchanged by this: the
census at both sizes is identical to the failing run, 3676 and 14285 DOM nodes,
20 and 80 messages, 3216 and 12804 highlighted tokens. Only the round trips are
gone.
This has been broken repo-wide since #8992 and invisible. On every other open PR
the earlier Unit tests step in this job fails on a broken import, so Install
Chromium and every Browser smoke step is skipped; this branch is the first job
in which they run at all. Compare step outcomes on #9081, Unit tests failure and
all smokes skipped, with this branch, Unit tests success and the smokes running.
Two things so the next drift costs less. The stray-request failure now names the
first three distinct URLs, because it fires in CI where nobody can attach a
listener afterwards, and finding this one meant re-running the smoke locally with
a print statement patched in. And a unit test now checks the allowlist against
the URL chat-api.ts builds, rather than against a string someone remembered to
update, so the same drift fails in Dev-server lifecycle tests instead of waiting
for a job where the browser smokes get to run.
The self-check itself is untouched: still zero tolerated strays, still failing
the run.
Verified locally on chromium at 25000 and 100000 chars: before, 2 stray requests
at each size and exit 1; after, 0 stray requests at each size, 9 stubbed rather
than 6, and exit 0. The three smokes the aborted job skipped, settings tab
panels, a settings panel that cannot load, and chat stream pacing, all pass
locally as well, so nothing further is hiding behind this one.
Member
Author
|
@codex review |
Member
Author
|
@codex review |
Member
Author
|
@codex review |
Member
Author
|
@codex review |
Member
Author
|
@codex review |
Member
Author
|
@codex review |
Member
Author
|
@codex review |
|
Codex Review: Didn't find any major issues. Can't wait for the next one! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Main landed its own repairs for several of the same tests. Took main's side wholesale in every conflict: its picker contract delegates to loadFallbackNotice and catches a bug mine did not (the nested ternary drops the CPU message when both fallback reasons are set), its chat UI smoke wraps the reload in reload_and_wait_for_pill, and its tp_vision and heavy-thread changes supersede mine.
Member
Author
|
@codex review |
danielhanchen
added a commit
that referenced
this pull request
Aug 20, 2026
Three conflicts, all small. The llama_cpp.py one is two independent additions at the same spot, the projector pin state and the Metal context refusal; both are kept. The mmproj-fallback test file is likewise both sides' tests, main's loadFallbackNotice coverage alongside the wording assertion here. image-input-support.ts takes main's line. The missing .ts on that runtime import is what made the frontend suite red, and #9192 has since fixed it upstream, so the commit carrying it here is redundant and only the explanatory comment differed. Dropped rather than re-added as diff noise. The cpu_offload reword is still only on this branch, so the test that pins it stays.
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
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.

Main is red on four counts right now, none of them caught before landing because each is a test that reads source text or a shape rather than behaviour. Every open PR that runs these jobs inherits all four, and merging main into a branch cannot clear them.
1. The mmproj fallback import has no file extension
chat/utils/image-input-support.tsgainedimport { isTextOnlyMmprojFallback } from "./mmproj-fallback". Before #9173 that module held onlyimport typelines, which are erased, so it had no runtime relative import at all.node --experimental-strip-typescannot resolve an extensionless specifier, soqueued-model-capabilities.test.tsfails on a pristine checkout. Adds the.ts.This is the one that makes Frontend build + bundle sanity and Frontend unit tests (Windows) fail on every PR.
2. The autoload harness has no stub for
mmprojFallbackMessagetest_chat_autoload_failure_gate.pyslicesautoLoadSmallestModelout ofchat-adapter.tsand runs it. Its own guard fails the whole file when the slice references a name the preamble does not define, and #9173 added such a name. 71 failures.3. The picker contract pinned one spelling of the success toast
test_model_picker_contracts.pyasserted the literaldescription: cpuFallbackReason. The mmproj branch now sits ahead of the CPU one in that ternary. Pinned on the description expression instead of which fallback is tested first, so the next branch to land does not break it again.2 and 3 are what make Repo tests (CPU) fail.
4.
_strip_mmproj_argslost its old argument nametest_tensor_split_abort_raises_early_to_layer_fallbackpins the order of two branches inload_modelby their source offsets. #9173 renamed the variable handed to_strip_mmproj_args, sofind()of the old spelling returned-1and the ordering assert quietly compared against it. Pins the call itself and fails loudly when it is no longer reachable. This is (Python 3.13).5. The chat UI smoke races the lazy route's Suspense fallback
Separate root cause, same symptom of an unreliable red.
exercise_permission_mode_controlsasserts the permission pill on expect's 5s default. Four of its six assertions come straight afterpage.reload(wait_until = "domcontentloaded"), which returns long before the lazy chat chunk resolves, so those race by construction rather than only on a slow runner. Reproduced on main ataa32c1861:Aria snapshot: - text: Loading....It shows up on both smokes, not one: Windows fails at line 135 (the first assertion, after the post-change-password route transition) and macOS at line 189 (the second, after a reload). Routes all six through one helper that waits on the same budget the composer already uses.
6. The playwright stub answers only one name
test_heavy_thread_measurement_integrity.pyinstalls a stub intosys.modulesforplaywrightandplaywright.sync_apiat collection time, defining exactlysync_playwright = None.sys.modulesis session-wide, so every later test imports its harness through that stub. Harnesses that need onlysync_playwrightwere satisfied;playwright_strip_ansi_smoke.pyimportsPage, expect, sync_playwrightand so was the only one that broke, which is why just one of four parametrisations oftest_an_empty_smoke_base_url_means_unsetfailed.(unknown location)in the error is the tell for a bareModuleType, not a namespace package, andimportorskipcannot help because the stub is importable by then.The stub now answers any public name with a callable that raises, so nothing can silently pretend to drive a browser, and dunders still raise
AttributeErrorso it does not masquerade as a package. Both victims keep asserting exactly what they did before; nothing is skipped.7. The heavy-thread smoke stubs a fork-count URL the app stopped requesting
#8992 replaced the per-message
/api/chat/threads/{id}/messages/{id}/forkswith one per-thread/api/chat/threads/{id}/forks. The smoke's in-page allowlist was not moved with it, so every fork-count GET reached the dev server and the harness failed its own self-check:HARNESS-BROKEN ... let 2 /api/ requests reach the network during the measured actions. The store refreshes onCHAT_HISTORY_UPDATED_EVENT, which the delete action fires, so two of the three landed inside the measurement. Harness bug, not a frontend one: the app is right to make one request per thread.The allowlist entry is corrected, the failure message now names the offending URLs, and a new contract test pins the stub against the URL
chat-api.tsactually builds so this cannot drift again. The self-check keeps zero tolerance for strays.Why these two were invisible until now. On every other open PR the
Unit testsstep inFrontend build + bundle sanityfails on item 1, soInstall Chromium for browser smokesand everyBrowser smoke for *step is skipped, andRepo tests (CPU)reported the 71 failures of items 2 and 3 rather than these. Fixing the import is what let the job reach them. With 6 and 7 fixed, the three smokes that were queued behind the heavy-thread abort (settings tab panels,a settings panel that cannot load,chat stream pacing) were run locally and are green, so nothing further is hiding.Verification
All test-only except the one-line import fix. 255 pass across the two repo-test files, 189 across the two backend files, and the frontend suite is green again. Both test changes were mutation-checked: reverting the source behaviour they guard makes them fail. My first attempt at 3 was too weak and passed under mutation because
cpuFallbackReasonalso appears in the function signature, so it is now scoped to the description expression.