Repair the tests main is currently red on by danielhanchen · Pull Request #9192 · unslothai/unsloth · GitHub
Skip to content

Repair the tests main is currently red on - #9192

Merged
danielhanchen merged 9 commits into
mainfrom
fix-mmproj-import
Aug 19, 2026
Merged

Repair the tests main is currently red on#9192
danielhanchen merged 9 commits into
mainfrom
fix-mmproj-import

Conversation

@danielhanchen

@danielhanchen danielhanchen commented Aug 18, 2026

Copy link
Copy Markdown
Member

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.ts gained import { isTextOnlyMmprojFallback } from "./mmproj-fallback". Before #9173 that module held only import type lines, which are erased, so it had no runtime relative import at all. node --experimental-strip-types cannot resolve an extensionless specifier, so queued-model-capabilities.test.ts fails 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 mmprojFallbackMessage

test_chat_autoload_failure_gate.py slices autoLoadSmallestModel out of chat-adapter.ts and 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.py asserted the literal description: 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_args lost its old argument name

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 find() of the old spelling returned -1 and 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_controls asserts the permission pill on expect's 5s default. Four of its six assertions come straight after page.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 at aa32c1861: 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.py installs a stub into sys.modules for playwright and playwright.sync_api at collection time, defining exactly sync_playwright = None. sys.modules is session-wide, so every later test imports its harness through that stub. Harnesses that need only sync_playwright were satisfied; playwright_strip_ansi_smoke.py imports Page, expect, sync_playwright and so was the only one that broke, which is why just one of four parametrisations of test_an_empty_smoke_base_url_means_unset failed. (unknown location) in the error is the tell for a bare ModuleType, not a namespace package, and importorskip cannot 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 AttributeError so 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}/forks with 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 on CHAT_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.ts actually 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 tests step in Frontend build + bundle sanity fails on item 1, so Install Chromium for browser smokes and every Browser smoke for * step is skipped, and Repo 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 cpuFallbackReason also appears in the function signature, so it is now scoped to the description expression.

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.
@danielhanchen

Copy link
Copy Markdown
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.
@unslothai unslothai deleted a comment from chatgpt-codex-connector Bot Aug 18, 2026
@unslothai unslothai deleted a comment from chatgpt-codex-connector Bot Aug 18, 2026
@unslothai unslothai deleted a comment from chatgpt-codex-connector Bot Aug 18, 2026
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.
@danielhanchen danielhanchen changed the title Studio: give the mmproj fallback import its file extension Repair the tests main is currently red on Aug 18, 2026
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.
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.
@danielhanchen

Copy link
Copy Markdown
Member Author

@codex review

@danielhanchen

Copy link
Copy Markdown
Member Author

@codex review

@unslothai unslothai deleted a comment from chatgpt-codex-connector Bot Aug 19, 2026
@danielhanchen

Copy link
Copy Markdown
Member Author

@codex review

@unslothai unslothai deleted a comment from chatgpt-codex-connector Bot Aug 19, 2026
@danielhanchen

Copy link
Copy Markdown
Member Author

@codex review

@danielhanchen

Copy link
Copy Markdown
Member Author

@codex review

@unslothai unslothai deleted a comment from chatgpt-codex-connector Bot Aug 19, 2026
@danielhanchen

Copy link
Copy Markdown
Member Author

@codex review

@unslothai unslothai deleted a comment from chatgpt-codex-connector Bot Aug 19, 2026
@danielhanchen

Copy link
Copy Markdown
Member Author

@codex review

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Can't wait for the next one!

Reviewed commit: 0a44ee7655

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

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.
@danielhanchen

Copy link
Copy Markdown
Member Author

@codex review

@danielhanchen
danielhanchen merged commit 65bdc91 into main Aug 19, 2026
36 checks passed
@danielhanchen
danielhanchen deleted the fix-mmproj-import branch August 19, 2026 12:03
@chatgpt-codex-connector

Copy link
Copy Markdown

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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant