From 3d51ba061e1c123b967712400bd1846861a57c93 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jaroslav=20Bene=C5=A1?= Date: Fri, 7 Aug 2026 14:41:45 +0200 Subject: [PATCH] Tests that found things reading did not The testing pass: 2140 tests to 2283, and four bugs that no amount of reading had turned up. Three came from driving the JavaScript under a Node DOM stub, which is the practice CLAUDE.md sets out and this is the reason it does. The terminal dropped every keystroke after a reconnect. `onclose` closed over the module-level socket rather than its own, and close() queues its event -- so the old socket's close arrived after a new one was assigned and nulled the live one. Output kept coming, because onmessage is bound to the object, while every send gates on the variable. It also announced "Disconnected" about a shell that had just reconnected. Two scripts were loaded twice on /messages, once by base.html and again by the page. Each is an IIFE with its own state, so four keyboard shortcuts toggled their panel twice and therefore did nothing, /help opened two dialogs, and an @ mention attached its file twice. A sweep refuses any template re-loading what base.html has. The microphone had no guard while the permission prompt was up, so each click opened another stream and only the last was ever stopped. And a skill shared with you took its name out of your own library: create checked uniqueness against what is *visible* rather than what is owned, against a (owner_id, name) constraint, and told you to edit a row you cannot edit. --ink-faint failed the contrast minimum in both themes -- 3.85 and 3.19 against 4.5 -- so the smallest text on every screen was the hardest to read. Measured in a headless browser rather than judged by eye. And the suite runs on 3.11 and 3.12 now as well as 3.14. It had only ever run on 3.14 while the image ships 3.12 and the packaging claimed 3.11: the interpreter most people would run was the one nothing had tested. Co-Authored-By: Claude Opus 5 (1M context) --- CHANGELOG.md | 34 + pyproject.toml | 8 + src/lembas/__init__.py | 2 +- src/lembas/services/library/skills.py | 31 +- src/lembas/web/static/css/tokens.css | 4 +- src/lembas/web/static/js/audio.js | 14 + src/lembas/web/static/js/terminal.js | 29 +- src/lembas/web/templates/messages/index.html | 17 +- tests/conftest.py | 106 ++ tests/test_agent_browse.py | 3 + tests/test_agent_command_edit.py | 3 + tests/test_agent_draft.py | 2 + tests/test_agent_jobs.py | 3 + tests/test_agent_profiles.py | 3 + tests/test_agent_ssh.py | 3 + tests/test_agent_tools.py | 3 + tests/test_audio_js.py | 200 ++++ tests/test_canvas_ssh.py | 3 + tests/test_cli.py | 140 +++ tests/test_composer_js.py | 287 ++++++ tests/test_library_routes.py | 981 +++++++++++++++++++ tests/test_mentions.py | 3 + tests/test_migrations.py | 232 +++++ tests/test_server.py | 150 +++ tests/test_sse.py | 105 ++ tests/test_terminal.py | 3 + tests/test_terminal_capture.py | 3 + tests/test_terminal_js.py | 268 +++++ tests/test_terminal_socket.py | 3 + tests/test_ui_js.py | 30 + tests/test_updates.py | 2 + 31 files changed, 2663 insertions(+), 12 deletions(-) create mode 100644 tests/test_audio_js.py create mode 100644 tests/test_cli.py create mode 100644 tests/test_composer_js.py create mode 100644 tests/test_library_routes.py create mode 100644 tests/test_migrations.py create mode 100644 tests/test_server.py create mode 100644 tests/test_sse.py create mode 100644 tests/test_terminal_js.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 7e5a24e..48135a1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,6 +16,40 @@ for 1.0.0 have something to be assembled from. ## Unreleased +## 0.9.13 + +**The testing pass.** 2140 tests became 2283, and writing them found four bugs +that no amount of reading had. + +- Fixed: **the terminal silently stopped accepting input after a reconnect.** + Change the connection, or let the shell catch up after falling behind, and + every keystroke was dropped from then on — while output kept arriving, so the + panel looked perfectly healthy. It also announced "Disconnected. Close and + reopen to reconnect." about a shell that had just reconnected successfully. +- Fixed: **on the Messages screen, half the keyboard did nothing.** Two scripts + were loaded twice there, so `Alt+B`, `Alt+E`, `Alt+T` and `Alt+I` toggled + their panel twice — which is to say not at all — while `/help` opened two + dialogs, `/image` posted the message twice, and picking an `@` mention + attached the file twice. +- Fixed: **pressing the microphone while the permission prompt was up opened a + recording each time.** Only the last was stopped, so the browser's recording + indicator stayed on until the tab was closed. +- Fixed: **a skill shared with you took its name out of your own library.** + Creating your own was refused with "a skill called that already exists. Edit + it instead" — naming a skill you cannot edit, because sharing grants reading + only. The model's `skill_create` hit the same dead end. Sharing a curated + skill with a team is what sharing is *for*. +- Hints and timestamps are readable now. `--ink-faint` failed the accessibility + contrast minimum in **both** themes — 3.85:1 in Moria, 3.19:1 in Shire, where + 4.5:1 is the bar — so the smallest text on every screen was the hardest to + read. +- The suite runs on **Python 3.11 and 3.12** as well as 3.14. It had only ever + run on 3.14, while the Docker image ships 3.12 and the packaging claimed 3.11 + — so the one interpreter most people would actually run was the one nothing + had tested. +- A `docs/notes/release-checklist.md` for the half of testing a machine cannot + do: a real endpoint, a real machine, real hardware, a real pair of eyes. + ## 0.9.12 **The security pass.** Six findings, all fixed. None is reachable by simply diff --git a/pyproject.toml b/pyproject.toml index 939900a..ff53d20 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -83,5 +83,13 @@ ignore = ["B008"] # FastAPI Depends() in defaults is idiomatic [tool.pytest.ini_options] testpaths = ["tests"] +# Registered so `-m "not slow"` works and an unknown-marker warning does not +# become an error later. `slow` is for the tests that stand up something real: +# a uvicorn subprocess on a port, an asyncssh server, a PTY, a git repository +# built with subprocess. They are the ones worth having and the ones worth +# being able to skip while iterating. +markers = [ + "slow: stands up a real server, shell or repository", +] asyncio_mode = "auto" filterwarnings = ["ignore::DeprecationWarning"] diff --git a/src/lembas/__init__.py b/src/lembas/__init__.py index 619aec4..d9d720c 100644 --- a/src/lembas/__init__.py +++ b/src/lembas/__init__.py @@ -1,3 +1,3 @@ """LLeMbas - a Middle-earth themed web UI for OpenAI-compatible LLM endpoints.""" -__version__ = "0.9.12" +__version__ = "0.9.13" diff --git a/src/lembas/services/library/skills.py b/src/lembas/services/library/skills.py index 5ba888c..5390ccb 100644 --- a/src/lembas/services/library/skills.py +++ b/src/lembas/services/library/skills.py @@ -67,12 +67,39 @@ def get(db: DBSession, skill_id: str, user: User | None) -> Skill | None: def by_name(db: DBSession, name: str, user: User | None) -> Skill | None: - """Look one up the way the model refers to it.""" + """Look one up the way the model refers to it. + + Scoped to what this person can **see**, which is theirs plus anything + shared with them -- correct for `skill_get` and `skill_edit`, where a + skill somebody shared is exactly what the model is reaching for. + + It is the wrong question for "is this name taken?"; see `owned_by_name`. + """ if user is None: return None return db.scalar(visible(db, user).where(Skill.name == slugify(name))) +def owned_by_name(db: DBSession, name: str, owner: User) -> Skill | None: + """One of *this person's own* skills by name. + + The uniqueness check used `by_name`, which is scoped to what is visible -- + so a skill somebody shared with you took that name out of your library. + Sharing a curated skill with a team is the intended use of `library.share`, + and doing it silently reserved the name for everyone it reached: creating + your own was refused with "a skill called 'weekly-report' already exists. + Edit it instead", naming a row you cannot edit, because sharing grants + reading only. The model's `skill_create` got the same dead end. + + The table's constraint is `(owner_id, name)`, so the question the check + should have been asking was always this one. `documents.create_base` next + door asks it correctly. + """ + return db.scalar( + select(Skill).where(Skill.owner_id == owner.id, Skill.name == slugify(name)) + ) + + def enabled_for( db: DBSession, user: User | None, *, exclude: Iterable[str] = () ) -> list[Skill]: @@ -155,7 +182,7 @@ def create( "A skill name must be two or more letters, numbers or hyphens, " "such as 'weekly-report'." ) - if by_name(db, slug, owner) is not None: + if owned_by_name(db, slug, owner) is not None: raise SkillError(f"A skill called {slug!r} already exists. Edit it instead.") if not description.strip(): raise SkillError( diff --git a/src/lembas/web/static/css/tokens.css b/src/lembas/web/static/css/tokens.css index de707c5..d389b3c 100644 --- a/src/lembas/web/static/css/tokens.css +++ b/src/lembas/web/static/css/tokens.css @@ -155,7 +155,7 @@ --ink: #E4E8EC; --ink-muted: #A2ADB8; - --ink-faint: #6E7883; + --ink-faint: #7A848F; --ink-inverse: #0B0E11; /* Mithril: the cool primary, used for focus and interactive accents. */ @@ -263,7 +263,7 @@ --ink: #2C2419; --ink-muted: #6A5C48; - --ink-faint: #94856D; + --ink-faint: #756A55; --ink-inverse: #FDFBF5; /* Hobbit-door blue-green: the cool primary. */ diff --git a/src/lembas/web/static/js/audio.js b/src/lembas/web/static/js/audio.js index d24adf5..99591e8 100644 --- a/src/lembas/web/static/js/audio.js +++ b/src/lembas/web/static/js/audio.js @@ -104,6 +104,17 @@ return; } + /* Out of `idle` **before** awaiting permission, or the button stays + clickable for as long as the browser's prompt is up -- and the dispatcher + below starts a recording on every click in that window. Each one opened + its own stream and its own MediaRecorder, only the last of which was kept + in `recorder`, so stopping released one and left the rest live: the + browser's recording indicator stayed on until the tab was closed. + + `working` already exists and is already styled, so this is the state the + button was missing rather than a new one. */ + setMicState(button, "working"); + navigator.mediaDevices.getUserMedia({ audio: true }).then(function (stream) { chunks = []; recorder = new MediaRecorder(stream); @@ -124,6 +135,9 @@ recorder.start(); setMicState(button, "recording"); }).catch(function () { + // Back to idle, or a refused permission leaves a button nothing can press + // again -- including the reader who has just gone and allowed it. + setMicState(button, "idle"); notify("The microphone could not be opened. Permission may be blocked.", "error"); }); } diff --git a/src/lembas/web/static/js/terminal.js b/src/lembas/web/static/js/terminal.js index a849e51..3df031a 100644 --- a/src/lembas/web/static/js/terminal.js +++ b/src/lembas/web/static/js/terminal.js @@ -125,10 +125,27 @@ "?cols=" + (term.cols || 80) + "&rows=" + (term.rows || 24); say("Connecting…"); - socket = new WebSocket(url); - socket.binaryType = "arraybuffer"; + /* Held in a local as well as in `socket`, because every handler below has + to know *which* connection it belongs to. - socket.onmessage = function (event) { + `close()` queues its event rather than firing it, and both reconnect + paths -- the `behind` frame and `repointTerminal` -- close the old + socket and immediately call this function. So the old socket's `close` + arrives *after* a new one has been assigned, and a handler that touched + the module-level `socket` was reaching past its own connection into the + live one. It set it to null: output kept arriving, because `onmessage` + is bound to the object, while `send`, `term.onData` and the paste + handler all gate on `socket && readyState === OPEN` -- so every + keystroke was dropped and no resize was ever sent again, on a panel that + looked perfectly healthy. And `closedOnPurpose` had already been cleared + for the new connection, so it said "Disconnected. Close and reopen to + reconnect." about a shell that had just reconnected. */ + var opened = new WebSocket(url); + socket = opened; + opened.binaryType = "arraybuffer"; + + opened.onmessage = function (event) { + if (socket !== opened) return; if (typeof event.data === "string") return control(event.data); /* Written straight through as bytes. xterm's decoder is stateful across calls, so a multi-byte character split across two frames still lands @@ -136,12 +153,14 @@ term.write(new Uint8Array(event.data)); }; - socket.onclose = function () { + opened.onclose = function () { + if (socket !== opened) return; socket = null; if (!closedOnPurpose) say("Disconnected. Close and reopen to reconnect."); }; - socket.onerror = function () { + opened.onerror = function () { + if (socket !== opened) return; /* A failed handshake gives the page nothing: no status, no reason. So this is a guess, and it names the likeliest cause rather than pretending to know. */ diff --git a/src/lembas/web/templates/messages/index.html b/src/lembas/web/templates/messages/index.html index b60a40a..d876713 100644 --- a/src/lembas/web/templates/messages/index.html +++ b/src/lembas/web/templates/messages/index.html @@ -105,8 +105,21 @@ {% endblock %} +{# + `steps.js` only. `composer.js` and `commands.js` were listed here too, and + `base.html` already loads both on every page -- so this screen ran each of + them **twice**. + + Each is an IIFE with its own state, and `stopPropagation()` does not stop a + second listener already bound to the same node. So: two composer menus stacked + on each other; one Enter on a highlighted `/` item running the command twice + (`/help` opening two dialogs, `/image` posting the message twice); an `@` + mention attaching its file twice; and `Alt+B`, `Alt+E`, `Alt+T` and `Alt+I` + toggling their panel twice, which is to say doing nothing at all. + + None of it looks like a script loaded twice. Found by driving the file under a + DOM stub, which is the rule `CLAUDE.md` sets out and the reason it does. +#} {% block scripts %} - - {% endblock %} diff --git a/tests/conftest.py b/tests/conftest.py index 5ff3e43..461f98a 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -300,6 +300,112 @@ def control_named(html: str, name: str) -> dict[str, str]: return found[0] +def script(name: str) -> str: + """One of the shipped browser files, as text. + + There is no JavaScript test runner here -- hard rule 1 keeps Node out of the + project -- so behaviour is driven by hand under a DOM stub and what the + suite pins is what the source says about itself. + """ + import lembas + + return (Path(lembas.__file__).parent / "web/static/js" / name).read_text(encoding="utf-8") + + +def js_code(source: str) -> str: + """The same source with its comments blanked out. + + These files are heavily commented, and every comment names the thing it is + explaining -- so `"list" in refresh` is true of a paragraph saying that + writing to `list` too early was the bug. An invariant about the code must + not be satisfiable by prose describing it. String literals are left intact, + because the assertions are usually about an event name inside one. + """ + out: list[str] = [] + at, end = 0, len(source) + while at < end: + char = source[at] + if char in "\"'`": + quote = char + out.append(char) + at += 1 + while at < end: + if source[at] == "\\": + out.append(source[at : at + 2]) + at += 2 + continue + out.append(source[at]) + at += 1 + if source[at - 1] == quote: + break + continue + if char == "/" and source[at : at + 2] in ("//", "/*"): + stop = ( + source.find("\n", at) + if source[at + 1] == "/" + else source.find("*/", at) + 2 + ) + if stop <= at: + stop = end + # Blanked rather than removed, so every offset still lines up. + out.append("".join(" " if c != "\n" else "\n" for c in source[at:stop])) + at = stop + continue + out.append(char) + at += 1 + return "".join(out) + + +def js_says(text: str, *pieces: str) -> bool: + """Whether the pieces appear, in order, whatever sits between them. + + Asserting a line verbatim makes a test that fails on reindentation, which is + noise; asserting only that two words appear somewhere makes one that never + fails at all. This is the middle: the shape, not the spacing. + """ + at = 0 + for piece in pieces: + at = text.find(piece, at) + if at == -1: + return False + at += len(piece) + return True + + +def js_function(source: str, name: str) -> str: + """The text of one named function, braces matched. + + Lets a test ask about the inside of a handler rather than about the file -- + "is `build()` called before anything writes to `list`" is a question about + `refresh`, and asking it of the whole source answers yes for the wrong + reason. + """ + code = js_code(source) + # The parenthesis is load-bearing: `token` is a prefix of `tokenAt`, and + # asking about the wrong function is a test that passes for no reason. + start = code.index(f"function {name}(") + at = code.index("{", start) + depth, quote = 0, "" + while at < len(code): + char = code[at] + if quote: + if char == "\\": + at += 2 + continue + if char == quote: + quote = "" + elif char in "\"'`": + quote = char + elif char == "{": + depth += 1 + elif char == "}": + depth -= 1 + if depth == 0: + return code[start : at + 1] + at += 1 + raise AssertionError(f"no closing brace for {name}()") + + @pytest.fixture def user_id(db: Session, registered: dict[str, str]) -> str: """The registered user's id. diff --git a/tests/test_agent_browse.py b/tests/test_agent_browse.py index e44a709..4e6561c 100644 --- a/tests/test_agent_browse.py +++ b/tests/test_agent_browse.py @@ -15,6 +15,9 @@ from sqlalchemy import select from lembas.db.models import SshProfile, User from lembas.services.agent import ssh as ssh_service +# Stands up something real -- see the `slow` marker in pyproject.toml. +pytestmark = pytest.mark.slow + asyncssh = pytest.importorskip("asyncssh") diff --git a/tests/test_agent_command_edit.py b/tests/test_agent_command_edit.py index 36674b1..a688fd4 100644 --- a/tests/test_agent_command_edit.py +++ b/tests/test_agent_command_edit.py @@ -20,6 +20,9 @@ from lembas.services import tools as tools_service from lembas.services.agent import policy from lembas.services.agent import ssh as ssh_service +# Stands up something real -- see the `slow` marker in pyproject.toml. +pytestmark = pytest.mark.slow + asyncssh = pytest.importorskip("asyncssh") diff --git a/tests/test_agent_draft.py b/tests/test_agent_draft.py index 21e5f8f..0fb6857 100644 --- a/tests/test_agent_draft.py +++ b/tests/test_agent_draft.py @@ -22,6 +22,8 @@ from lembas.services import settings_store from lembas.services.agent import draft as draft_service from lembas.services.agent import terminal as terminal_service +# Stands up something real -- see the `slow` marker in pyproject.toml. +pytestmark = pytest.mark.slow @pytest.fixture(autouse=True) def _clean(): diff --git a/tests/test_agent_jobs.py b/tests/test_agent_jobs.py index 6d10993..1297622 100644 --- a/tests/test_agent_jobs.py +++ b/tests/test_agent_jobs.py @@ -28,6 +28,9 @@ pytestmark = pytest.mark.skipif( ) +# Stands up something real -- see the `slow` marker in pyproject.toml. +pytestmark = pytest.mark.slow + class LocalExecutor: """`SshExecutor.run`'s contract, run against the local shell. diff --git a/tests/test_agent_profiles.py b/tests/test_agent_profiles.py index 63a42bf..1298afd 100644 --- a/tests/test_agent_profiles.py +++ b/tests/test_agent_profiles.py @@ -16,6 +16,9 @@ from lembas.db.models import SshProfile, User from lembas.services import settings_store from lembas.services.crypto import UNCHANGED_SENTINEL, decrypt, encrypt +# Stands up something real -- see the `slow` marker in pyproject.toml. +pytestmark = pytest.mark.slow + asyncssh = pytest.importorskip("asyncssh") diff --git a/tests/test_agent_ssh.py b/tests/test_agent_ssh.py index badf7c0..a6c396c 100644 --- a/tests/test_agent_ssh.py +++ b/tests/test_agent_ssh.py @@ -14,6 +14,9 @@ import pytest from lembas.services.agent import ssh from lembas.services.agent.base import ExecError, ExecRequest +# Stands up something real -- see the `slow` marker in pyproject.toml. +pytestmark = pytest.mark.slow + asyncssh = pytest.importorskip("asyncssh") diff --git a/tests/test_agent_tools.py b/tests/test_agent_tools.py index 9a8e245..e13e696 100644 --- a/tests/test_agent_tools.py +++ b/tests/test_agent_tools.py @@ -31,6 +31,9 @@ from lembas.services.agent import policy, session from lembas.services.agent import ssh as ssh_service from lembas.services.tools import RISK_EXECUTE +# Stands up something real -- see the `slow` marker in pyproject.toml. +pytestmark = pytest.mark.slow + asyncssh = pytest.importorskip("asyncssh") diff --git a/tests/test_audio_js.py b/tests/test_audio_js.py new file mode 100644 index 0000000..cfdab82 --- /dev/null +++ b/tests/test_audio_js.py @@ -0,0 +1,200 @@ +"""Dictation and read-aloud, checked without a runtime. + +There is no JavaScript test runner here and hard rule 1 keeps Node out of the +project, so the behaviour is driven by hand under a DOM stub before committing. +Both halves of this file are progressive -- without it the composer and the +bubbles still work, they simply have two buttons that do nothing -- which is +also the shape of every way it can break: a microphone left open, a button left +disabled, a reply read out twice. None of them raises anything. + +What the suite can pin is that there is one recording state machine and one +`