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 +`