docker: close four holes the review found

docker-publish.yml: the llama.cpp tag resolver was the one step in the prepare
job still using `curl | sed` without pipefail. The runner's default `bash -e`
shell takes sed's exit status, so an unreachable github.com left TAG empty and
the step published the mutable `latest`. Both arch legs re-resolve that through
fetch_llama_prebuilt.py and Dockerfile.studio resolves it a third time, so a
release cut mid-run can put different llama.cpp bundles under one manifest.
Capture the redirect first and fail the job when it is missing or does not land
on a release tag, matching the three ref resolvers below it.

unsloth_nb_strip_colab.py: strip_notebook read, parsed and then unconditionally
os.replace'd. The refresh child re-arms finalize after the entrypoint has execed
the container command, so JupyterLab is already serving the tree and a save
landing in that window was destroyed, after which migrate recorded the cleaned
hash and marked the notebook pristine forever. Re-read the hash once the staged
copy is complete and drop it when the file moved, the same rule the refresh
publish in unsloth_sync_notebooks.sh already follows.

unsloth_nb_view.py: ownership for the view teardown accepted any symlink target
under DEST, but every link the tool creates points at DEST/nb. A shortcut the
user made in the landing dir to their own file elsewhere in the checkout was
therefore classified as ours and deleted on the next boot. Key ownership on
DEST/nb instead.

cellNav.ts: the edit-mode boundary test compared the cursor line against
editor.lineCount, both logical, while JupyterLab wraps markdown and raw editors
by default (StaticNotebook.defaultEditorConfig). A one-line markdown header
renders as several visual rows, so every arrow left the cell and the wrapped
rows could not be reached. Ask CodeMirror whether it can still move one visual
line (EditorView.moveVertically, compared by coordsAtPos top) and keep the
logical test as the fallback for a non-CodeMirror editor.

New tests: 12 passed / 8 failed before, 20 passed / 0 failed after.
This commit is contained in:
Daniel Han 2026-07-27 16:06:56 +00:00
commit 5906a9feb6
8 changed files with 497 additions and 24 deletions

View file

@ -0,0 +1,73 @@
# SPDX-License-Identifier: AGPL-3.0-only
# Copyright 2026-Present the Unsloth team. See /studio/LICENSE.AGPL-3.0
"""Colab-style arrow navigation must not swallow wrapped-line movement.
`cellNav.ts` owns ArrowUp/ArrowDown in the capture phase and jumps to the
previous/next cell when the cursor sits on the first/last line of the editor.
That test used `editor.getCursorPosition().line` against `editor.lineCount`,
both of which are LOGICAL (JupyterLab's CodeMirrorEditor: `get lineCount() {
return this.doc.lines }`), while JupyterLab wraps markdown and raw cell editors
by default (`StaticNotebook.defaultEditorConfig` -> `markdown: { lineWrap: true
}`, `raw: { lineWrap: true }`; the image's `docker/jupyter/overrides.json` only
sets `autoClosingBrackets`).
So for a one-line markdown header -- what every Unsloth notebook opens with --
`lineCount === 1`, the cursor is on line 0 == lineCount - 1 from every visual
row, and BOTH arrows leave the cell: the wrapped rows in between cannot be
reached at all. Measured in Chromium with CodeMirror 6 + EditorView.lineWrapping
at the notebook's editor width: 1 logical line renders as 7 visual rows and the
logical test hijacks the arrows on 7 of 7 rows, in both directions. The same
measurement on an unwrapped code cell shows the visual test agreeing with the
logical one on every row, so the Colab-style jump is unchanged there.
CodeMirror's own answer is `EditorView.moveVertically(range, forward)`, which
moves "to the next line (including wrapped lines)"; it returns the unchanged
head only at offset 0 / doc.length, so a move that stays on the same visual row
(same `coordsAtPos().top`) is the real editor edge.
Static source guard: the labextension is only built inside Dockerfile.studio
(`jlpm install && jlpm build:prod`), so there is no TS test runner in-repo.
"""
from __future__ import annotations
import re
from pathlib import Path
import pytest
REPO_ROOT = Path(__file__).resolve().parents[2]
CELL_NAV = REPO_ROOT / "docker" / "jupyter" / "unsloth_labext" / "src" / "cellNav.ts"
@pytest.fixture(scope = "module")
def source() -> str:
assert CELL_NAV.is_file(), f"missing {CELL_NAV}"
return CELL_NAV.read_text(encoding = "utf-8")
def test_the_edit_mode_boundary_test_asks_codemirror_for_a_visual_line(source: str):
assert "moveVertically" in source, (
"the edit-mode boundary check must ask CodeMirror whether it can still "
"move one VISUAL line (EditorView.moveVertically); a logical lineCount "
"test makes the wrapped rows of a markdown cell unreachable"
)
def test_the_visual_check_compares_screen_rows(source: str):
assert "coordsAtPos" in source, (
"moveVertically clamps to the document edge instead of returning the "
"same head, so the two positions have to be compared by visual row"
)
def test_the_logical_line_test_is_only_a_fallback(source: str):
body = source[source.index("const editing = notebook.mode === 'edit'") :]
logical = re.search(r"editor\.lineCount - 1", body)
assert logical, "the non-CodeMirror fallback should still exist"
visual = re.search(r"moveVertically", body)
assert visual and visual.start() < logical.start(), (
"the visual-line test has to run first; the logical one is only for an "
"editor that is not a CodeMirrorEditor"
)

View file

@ -0,0 +1,139 @@
# SPDX-License-Identifier: AGPL-3.0-only
# Copyright 2026-Present the Unsloth team. See /studio/LICENSE.AGPL-3.0
"""The Colab-intro cleanup must not overwrite a save it did not see.
`unsloth_sync_notebooks.sh` forks the GitHub refresh into a DETACHED child before
the entrypoint execs the container command, so JupyterLab is already serving
$DEST while that child runs. When the refresh copied anything the child re-arms
`finalize()`, which runs `unsloth_nb_strip_colab.py --state ... --dest ...`, i.e.
`migrate()` -> `strip_notebook()` over every owned+unedited notebook.
`strip_notebook` read the file, parsed it, serialised the cleaned copy and then
`os.replace`d it unconditionally. A user save that landed in that window was
destroyed, and `migrate` then recorded the cleaned file's hash, so the state
machine treats the notebook as pristine forever after -- the same
check-then-write hole that was closed in the refresh loop itself (the publish
there now re-reads the hash immediately before the rename).
Behavioural: the save is injected inside the window, while the helper serialises
the cleaned copy (the widest part of it: json parse + dump of a notebook that is
often megabytes). No docker, no network.
"""
from __future__ import annotations
import copy
import importlib.util
import json
from pathlib import Path
import pytest
REPO_ROOT = Path(__file__).resolve().parents[2]
STRIP_PATH = REPO_ROOT / "docker" / "unsloth_nb_strip_colab.py"
INTRO = 'To run this, press "*Runtime*" and press "*Run all*" on a **free** Tesla T4 Google Colab instance!\n'
@pytest.fixture(scope = "module")
def strip():
assert STRIP_PATH.is_file(), f"missing {STRIP_PATH}"
spec = importlib.util.spec_from_file_location("unsloth_nb_strip_race", STRIP_PATH)
mod = importlib.util.module_from_spec(spec)
spec.loader.exec_module(mod)
return mod
def notebook(*sources):
return {
"cells": [
{"cell_type": "markdown", "metadata": {}, "source": list(src)} for src in sources
],
"metadata": {},
"nbformat": 4,
"nbformat_minor": 5,
}
def write(path: Path, nb) -> None:
path.write_text(json.dumps(nb, indent = 1, ensure_ascii = False) + "\n", encoding = "utf-8")
@pytest.fixture
def racing(strip, tmp_path: Path):
"""Fire a user save inside the window: after strip_notebook read the file,
while it is serialising the cleaned copy."""
real_dump = strip.json.dump
state = {"save": None, "path": None, "fired": 0}
def dump(obj, fp, *args, **kwargs):
out = real_dump(obj, fp, *args, **kwargs)
if state["save"] is not None and state["fired"] == 0:
state["fired"] = 1
Path(state["path"]).write_text(state["save"], encoding = "utf-8") # Ctrl+S
return out
strip.json.dump = dump
try:
yield state
finally:
strip.json.dump = real_dump
def test_a_save_during_the_cleanup_is_not_overwritten(strip, racing, tmp_path: Path):
path = tmp_path / "Llama.ipynb"
write(path, notebook([INTRO, "\n", "# Llama\n"]))
edited = notebook([INTRO, "\n", "# Llama\n", "\n", "my own notes, saved from JupyterLab\n"])
racing["save"] = json.dumps(edited, indent = 1, ensure_ascii = False) + "\n"
racing["path"] = str(path)
strip.strip_notebook(str(path))
on_disk = json.loads(path.read_text(encoding = "utf-8"))
assert on_disk == edited, (
"the user's save landed after strip_notebook read the file and was "
"overwritten by the cleaned copy of the OLD content; the sync contract "
"is that user edits always win"
)
def test_the_recorded_hash_still_matches_the_file_after_a_racing_save(strip, racing, tmp_path: Path):
# migrate() rewrites STATE with the post-strip hash. If the write above is
# allowed to clobber a save, the state ALSO says "pristine", so every later
# refresh happily overwrites the notebook again.
dest = tmp_path / "unsloth-notebooks"
dest.mkdir()
path = dest / "Llama.ipynb"
write(path, notebook([INTRO, "\n", "# Llama\n"]))
before = strip._sha256(str(path))
state = tmp_path / ".unsloth_sync_state"
state.write_text(f"{before} Llama.ipynb\n", encoding = "utf-8")
edited = notebook([INTRO, "\n", "# Llama\n", "\n", "my own notes\n"])
racing["save"] = json.dumps(edited, indent = 1, ensure_ascii = False) + "\n"
racing["path"] = str(path)
strip.migrate(str(state), str(dest))
recorded = state.read_text(encoding = "utf-8").split(" ", 1)[0]
on_disk = strip._sha256(str(path))
assert json.loads(path.read_text(encoding = "utf-8")) == edited
assert recorded != on_disk, (
"a file the user saved during the cleanup must NOT end up recorded as "
"managed-and-pristine, or the next refresh overwrites it too"
)
def test_the_normal_no_race_cleanup_still_strips_and_rewrites(strip, tmp_path: Path):
# Guard the fix from over-reaching: with nobody else writing, the cleanup
# must still strip the Colab sentence and publish the result.
path = tmp_path / "Llama.ipynb"
original = notebook([INTRO, "\n", "# Llama\n"])
write(path, copy.deepcopy(original))
assert strip.strip_notebook(str(path)) is True
cleaned = json.loads(path.read_text(encoding = "utf-8"))
assert cleaned["cells"][0]["source"] == ["# Llama\n"]
assert strip.strip_notebook(str(path)) is False # idempotent

View file

@ -0,0 +1,118 @@
# SPDX-License-Identifier: AGPL-3.0-only
# Copyright 2026-Present the Unsloth team. See /studio/LICENSE.AGPL-3.0
"""The categorized notebook VIEW may only delete the links it created.
`unsloth_nb_view.py` rebuilds "/workspace/Unsloth Notebooks" on every boot, and
that directory is also JupyterLab's landing dir, so `_clear_view()` promises to
remove only the tool's own symlinks. Every link the tool creates points at
DEST/nb/<file>, but the ownership predicate accepted ANY target under DEST, so a
user's own symlink into the notebooks checkout -- e.g. a shortcut to their own
notebook saved beside it, which the sync script explicitly supports ("kept
existing user file" / "In DEST but never recorded") -- was classified as
tool-owned and deleted on the next boot.
Behavioural: builds a real DEST/VIEW pair on disk and runs build_view twice.
No docker, no network.
"""
from __future__ import annotations
import importlib.util
import os
from pathlib import Path
import pytest
REPO_ROOT = Path(__file__).resolve().parents[2]
VIEW_PATH = REPO_ROOT / "docker" / "unsloth_nb_view.py"
README = (
"### Main Notebooks\n"
"[Llama](nb/Llama3_2_%281B_and_3B%29_Conversational.ipynb)\n"
"### Gemma\n"
"[Gemma](nb/Gemma3_%284B%29.ipynb)\n"
)
@pytest.fixture(scope = "module")
def view_mod():
assert VIEW_PATH.is_file(), f"missing {VIEW_PATH}"
spec = importlib.util.spec_from_file_location("unsloth_nb_view_under_test", VIEW_PATH)
mod = importlib.util.module_from_spec(spec)
spec.loader.exec_module(mod)
return mod
@pytest.fixture
def tree(tmp_path: Path):
dest = tmp_path / "unsloth-notebooks"
view = tmp_path / "Unsloth Notebooks"
(dest / "nb").mkdir(parents = True)
view.mkdir()
for name in ("Llama3_2_(1B_and_3B)_Conversational.ipynb", "Gemma3_(4B).ipynb"):
(dest / "nb" / name).write_text("{}", encoding = "utf-8")
(dest / "README.md").write_text(README, encoding = "utf-8")
# The user's own notebook, saved inside the checkout (supported by the sync
# script), plus their own folder of shortcuts in the landing dir.
(dest / "my_work").mkdir()
(dest / "my_work" / "experiment.ipynb").write_text("{}", encoding = "utf-8")
return dest, view
def link(target: Path, at: Path) -> None:
at.parent.mkdir(parents = True, exist_ok = True)
os.symlink(os.path.relpath(target, at.parent), at)
def test_a_user_link_to_their_own_file_in_the_checkout_survives(view_mod, tree):
dest, view = tree
own = view / "00 My favourites" / "experiment.ipynb"
link(dest / "my_work" / "experiment.ipynb", own)
view_mod.build_view(str(dest), str(view))
assert os.path.islink(own), (
"a symlink the user created in the landing dir, pointing at their own "
"file inside the notebooks checkout, was deleted by _clear_view"
)
assert os.path.realpath(own) == os.path.realpath(dest / "my_work" / "experiment.ipynb")
def test_a_user_link_outside_the_checkout_survives(view_mod, tree, tmp_path: Path):
dest, view = tree
outside = tmp_path / "datasets"
outside.mkdir()
own = view / "datasets"
link(outside, own)
view_mod.build_view(str(dest), str(view))
assert os.path.islink(own)
def test_the_tools_own_stale_links_are_still_cleaned_up(view_mod, tree):
dest, view = tree
view_mod.build_view(str(dest), str(view))
generated = view / "02 Gemma" / "Gemma3_(4B).ipynb"
assert os.path.islink(generated)
# Upstream drops the notebook: its generated link (now stale, and pointing
# into DEST/nb) has to go, and the emptied folder with it.
(dest / "nb" / "Gemma3_(4B).ipynb").unlink()
(dest / "README.md").write_text(
"### Main Notebooks\n[Llama](nb/Llama3_2_%281B_and_3B%29_Conversational.ipynb)\n",
encoding = "utf-8",
)
view_mod.build_view(str(dest), str(view))
assert not os.path.islink(generated) and not os.path.exists(generated)
assert not (view / "02 Gemma").exists()
def test_a_rebuild_is_stable_for_the_links_it_owns(view_mod, tree):
dest, view = tree
view_mod.build_view(str(dest), str(view))
first = sorted(str(p.relative_to(view)) for p in view.rglob("*"))
view_mod.build_view(str(dest), str(view))
assert sorted(str(p.relative_to(view)) for p in view.rglob("*")) == first

View file

@ -97,6 +97,96 @@ def test_an_unreachable_remote_never_emits_a_mutable_ref(steps: dict, step_id: s
assert res.returncode != 0
# --- the llama.cpp prebuilt tag ----------------------------------------------
# Same hole, same job, different resolver: the tag step is
#
# TAG="$(curl -fsSL -o /dev/null -w '%{url_effective}' .../releases/latest \
# | sed -n 's#.*/releases/tag/##p')"
# echo "tag=${TAG:-latest}" >> "$GITHUB_OUTPUT"
#
# `bash -e` without pipefail takes the exit status of `sed`, so an unreachable
# github.com made the step emit `tag=latest`. That value is NOT a pin: both
# matrix legs pass it to docker/fetch_llama_prebuilt.py, whose main() re-resolves
# "latest" per build, and Dockerfile.studio re-resolves it a third time, so a
# release published mid-run can put two different llama.cpp bundles under one
# multi-arch manifest -- with `:latest` moved onto it, because the stable-tag
# gates key off the dispatch inputs, not off whether resolution worked.
@pytest.fixture(scope = "module")
def llama_step() -> str:
doc = yaml.safe_load(WORKFLOW.read_text(encoding = "utf-8"))
for step in doc["jobs"]["prepare"]["steps"]:
if step.get("id") == "llama":
return step["run"]
raise AssertionError("the llama tag resolver step is missing from the prepare job")
def test_an_unresolvable_llama_release_fails_the_step(llama_step: str, tmp_path: Path):
res = _run_llama_step(llama_step, tmp_path, curl_exit = 6)
assert res.returncode != 0, (
"a failed /releases/latest lookup must fail the prepare job:\n"
f"stdout={res.stdout}\nstderr={res.stderr}"
)
def test_an_unresolvable_llama_release_never_emits_a_mutable_tag(llama_step: str, tmp_path: Path):
res = _run_llama_step(llama_step, tmp_path, curl_exit = 6)
emitted = (tmp_path / "github_output").read_text(encoding = "utf-8")
assert "latest" not in emitted, (
f"the step published {emitted.strip()!r}; every consumer resolves that "
"mutable tag again, so the two arch legs and Studio can bake different "
"llama.cpp versions under one manifest"
)
assert res.returncode != 0
def test_a_resolved_llama_release_is_forwarded_verbatim(llama_step: str, tmp_path: Path):
# The fix must not break the normal path.
res = _run_llama_step(llama_step, tmp_path, curl_exit = 0)
assert res.returncode == 0, f"stdout={res.stdout}\nstderr={res.stderr}"
assert (tmp_path / "github_output").read_text(encoding = "utf-8").strip() == (
"tag=b10107-mix-1911198"
)
def _run_llama_step(script: str, tmp_path: Path, *, curl_exit: int):
bin_dir = tmp_path / "bin"
bin_dir.mkdir(parents = True, exist_ok = True)
stub = bin_dir / "curl"
if curl_exit:
# How curl reports an unreachable github.com: nothing on stdout, non-zero.
stub.write_text(
"#!/usr/bin/env bash\n"
'echo "curl: (6) Could not resolve host: github.com" >&2\n'
f"exit {curl_exit}\n",
encoding = "utf-8",
)
else:
stub.write_text(
"#!/usr/bin/env bash\n"
"printf '%s' "
"'https://github.com/unslothai/llama.cpp/releases/tag/b10107-mix-1911198'\n",
encoding = "utf-8",
)
stub.chmod(0o755)
out = tmp_path / "github_output"
out.write_text("", encoding = "utf-8")
env = dict(os.environ)
env["PATH"] = f"{bin_dir}{os.pathsep}" + env["PATH"]
env["GITHUB_OUTPUT"] = str(out)
env["INPUT_TAG"] = "" # the default (push / schedule) trigger
path = tmp_path / "llama_step.sh"
path.write_text(_expand(script), encoding = "utf-8")
return subprocess.run(
["bash", "-e", str(path)],
capture_output = True,
text = True,
env = env,
timeout = 60,
)
def _expand(run: str) -> str:
"""Replace the `${{ ... }}` expressions with the empty string the default
(push to main, no dispatch inputs) trigger produces."""