diff --git a/CHANGELOG.md b/CHANGELOG.md index 54e45a5..85e947a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,6 +16,56 @@ for 1.0.0 have something to be assembled from. ## Unreleased +## 0.9.10 + +**The second audit pass: screens that were harder to use than they needed to +be.** Checked by rendering them in a real browser and measuring, not by reading +the CSS. + +- Fixed: **the Prompts admin page put its reference material first.** The + Variables legend and the Preview run to a screen each and sat above the tabs, + so the editor — the thing the page is for — started two screens down and every + tab switch had to move the whole page to be any use. On a short tab it could + not move far enough and left the panel stranded above a screenful of nothing. + The editor comes first now, the reference after, and the tab bar stays put: + measured, it moved 385→642px between tabs before and does not move at all now. + The tab bar also sticks to the top, so a long panel does not scroll it away. +- Fixed: **custom themes were three fixed slots.** A fresh instance opened on + fifty-seven empty colour boxes under three identical headings, and a fourth + theme could not be made at all. Now: one block per theme you have, plus one + blank to add the next, with the colours behind a disclosure — so a theme is a + name and a starting point until you ask for more. Up to twelve. The page is + half the height it was. +- Fixed: **deleting a chat left every file it held on disk.** The rows went — + the message, the attachments, the generated images — and the files they named + stayed, with nothing that would ever look at them again. Four of the five ways + a chat can end had this: the delete button, a schedule's task chat, a helper's + hidden chat, and deleting an account. There is one function that deletes a + chat now, and it removes the files first. +- Fixed, and it is what made the above invisible: **a file attached before the + chat existed never learned which chat it belonged to.** Anything picked on the + new-chat screen kept an empty `chat_id` for the rest of its life. Six things + filter on that, so for those files the model was not told they were attached, + the canvas would not open them, and the cleanup could not find them. +- **Folders can be nested, which the README has always claimed.** The route has + handled it since folders existed — cycle guard, depth limit — and the sidebar + has always drawn a tree; there was simply no control that could ask for it. + Moving a folder also respects the depth limit now, which only creating one did. +- The Proxmox container installs the **update helper by default**. A container + made thirty seconds ago to run one thing is not the shared host the plain + installer has to be careful about, and an appliance you cannot update without + a shell is one nobody updates. `INSTALL_UPDATE_HELPER=0` opts out. Docker + deliberately has no equivalent: updating a container is pulling an image, and + a helper inside one would need the Docker socket, which is root on the host. +- The starting points on the new-chat screen are four new ones, aimed at + somebody who has just stood an instance up and wants to know what is behind + it. Only a fresh install gets them; an instance that has already seeded keeps + whatever its administrator has made of the list. +- `README.md` describes what this actually is again — schedules, reports, + helpers, image generation, semantic search, quotas, sharing, branding and the + updates page were all missing, and two things listed as *planned* had shipped. + It gained sections on Docker, the Proxmox container and updating. + ## 0.9.9 **The first of five audit passes before 1.0.0** — everything that landed between diff --git a/CLAUDE.md b/CLAUDE.md index ed97308..6558709 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -194,7 +194,7 @@ docs/notes/ the rest of "Things that will bite you", by topic ## Things that will bite you -**Three topics live in `docs/notes/`, and are not loaded with this file.** They +**Several topics live in `docs/notes/`, and are not loaded with this file.** They were split out because this one is read in full on every session and each of them is only wanted while you are in that corner of the code. Read the file before touching the code it names -- these are the same notes, not a summary. diff --git a/README.md b/README.md index 5de7073..60de15f 100644 --- a/README.md +++ b/README.md @@ -99,18 +99,49 @@ runtime. Clone it, `pip install -e .`, run it. - **Model settings** — searchable, filterable list with a page per model: ordering, pinned models, an instance default and a per-user default, custom names, descriptions and images. Scales to hundreds of models +- **Things that happen because time passed** — say "every Monday at nine" and a + model can set it up itself, against the same recurrence rule the manual form + uses. A run can file a **report** you read later, send you a message, or work + on in a chat of its own. The reply says the timing back in words, which is the + one moment anybody can check that Monday was understood as Monday +- **News that finds you** — a dot in the sidebar, a count in the tab title while + you are looking elsewhere, and **web push** so a schedule firing at seven in + the morning reaches a browser that is shut. Opt-in per device +- **Helpers** — a reply can hand a self-contained piece of work to another model + that runs on its own and reports back, several at once, so research fans out + instead of queueing. A helper cannot ask questions, cannot send helpers of its + own, and on a machine runs only a fixed list of read-only commands +- **Drawing** — point it at a ComfyUI and a model can make images, against + workflow templates and defaults you set: size, steps, sampler, scheduler, + checkpoint. It reviews its own result and can try again +- **Semantic search** — pick an embedding model and library search fuses keyword + and meaning, so *"how do I get paid"* finds a document that says *"invoicing"*. + Choosing none is not a degraded mode: it is byte-for-byte the keyword search + that was always there, with nothing written and no requests made - **Users, groups & permissions** — per-group grants that union rather than - override, and model access restricted to chosen groups + override, model access restricted to chosen groups, read and write split for + notes, memory and skills, and a screen that answers *"what can this account + actually do?"* by naming where each permission came from +- **Quotas** — monthly tokens, concurrent replies, agent wall clock, images a + day, helpers a reply. Resolved by maximum across a person's groups, with zero + meaning *no limit* +- **Sharing** — hand a document, a note, a skill or a report to a group or a + person, read-only, with a *Shared with me* filter in every listing +- **Make it yours** — name, tagline, logo, favicon and launcher icons; the + Middle-earth wording is editable data; custom **themes** defined as a set of + colours rather than a stylesheet, and global CSS overrides - **Accounts** — first account becomes the administrator, argon2 password hashing, revocable server-side sessions, self-service password change, admin-managed accounts -- **Admin settings** — open or close registration from the UI, stored in the - database and effective immediately -- **Two themes** — *Moria* (dark) and *Shire* (light), switchable per user +- **Admin settings** — registration, upload and extraction limits, prompt + fragments, and an **Updates** page showing what is running, what is available + and what changed between +- **Two themes and your own** — *Moria* (dark), *Shire* (light), and as many + more as you care to define **Planned** -Image generation · OCR for scanned PDFs · semantic search in the library. +OCR for scanned PDFs · conversation branching · chat export · archived chats. See [PLAN.md](PLAN.md) for what is built, what is not, and why. @@ -322,6 +353,80 @@ lembas secret-key # generate a value for LEMBAS_SECRET_KEY lembas create-admin # create or promote an administrator ``` +## Running it somewhere + +Three ways, all in this repository. + +### Docker + +```bash +export LEMBAS_SECRET_KEY="$(lembas secret-key)" # required; there is no default +docker compose up -d +``` + +One stage, no build step, non-root. The image bakes **no secret key, no data and +no `.git`** — a key inside an image is one every copy shares, and rotating it +makes stored API keys unreadable. Data lives in a named volume on `/data`. + +`docker-compose.yml` publishes on `127.0.0.1` and expects a TLS proxy in front: +the service worker and the microphone both require HTTPS or localhost, so plain +http on a LAN address is a constraint rather than a preference. One replica, and +that is deliberate — the generation registry, the terminal sessions and the +schedule ticker are all in-process, so two would mean every schedule firing +twice. + +**Updating a container is pulling a new image**, and `/admin/updates` says so +rather than offering a button: + +```bash +docker compose pull && docker compose up -d +``` + +There is deliberately no in-container update helper. The one the other install +paths use restarts a systemd service; the equivalent here would be a process +inside the container reaching the Docker socket to replace the container it is +running in — which is root on the host, granted to anybody who can administer +the web interface. The image is the unit of deployment, and that is the whole +point of it. + +### A machine of its own + +`deploy/` holds a systemd unit, an nginx vhost, and install/update scripts. Every +template is parameterised and substituted at install time, so nothing +host-specific is committed here. See [deploy/README.md](deploy/README.md). + +`deploy/lxc-install.sh` creates an unprivileged Proxmox container and runs that +same installer inside it — a wrapper around what already works rather than a +second install path: + +```bash +CTID=140 SITE_HOST=chat.example ./deploy/lxc-install.sh +``` + +The container gets the **update helper by default**, unlike a bare +`install.sh`. The installer defaults it off because it cannot know what it is +installing onto; a container this script made thirty seconds ago to run one +thing, on a hypervisor you own, is not that host — and an appliance you cannot +update without a shell is one nobody updates. `INSTALL_UPDATE_HELPER=0` opts +out. + +### Updating + +**Admin → Updates** shows the version running, what is available on the channel +this host follows, and the commits between. `stable` is the newest `vX.Y.Z` tag; +`edge` is the branch tip, which is whatever was pushed most recently. + +The button that applies an update is **opt-in**, and that is the design: the +service runs unprivileged and cannot restart itself, so the request is a file +that a systemd `.path` unit picks up and runs as root. It carries no ref and no +channel — pressing it is always "deploy the channel this host was configured +with", never "deploy something else". Install it with +`INSTALL_UPDATE_HELPER=1`; without it the page says so and prints the command to +run by hand. + +Release notes come out of the annotated tag itself, so no forge API is involved +anywhere. + ## How it fits together ``` diff --git a/deploy/lxc-install.sh b/deploy/lxc-install.sh index dd11bb7..48ad8c2 100755 --- a/deploy/lxc-install.sh +++ b/deploy/lxc-install.sh @@ -124,10 +124,25 @@ pct exec "$CTID" -- bash -lc " # The same installer this repository ships, run inside. Everything it decides -- # the service user, the prefix, the unit, the vhost, the self-signed certificate # -- it decides there, so this script has no opinions to keep in step with it. +# +# The update helper is **on by default here**, and only here. `install.sh` +# defaults it off because it cannot know what it is installing onto: on a shared +# or long-lived host, letting anybody who can administer the web interface +# deploy as root is a decision somebody should make on purpose. A container +# created by this script thirty seconds ago is not that host -- it exists to run +# LLeMbas and nothing else, whoever ran this owns the hypervisor, and an +# appliance you cannot update without a shell is an appliance nobody updates. +# +# Set INSTALL_UPDATE_HELPER=0 to opt back out. +INSTALL_UPDATE_HELPER="${INSTALL_UPDATE_HELPER:-1}" +LEMBAS_CHANNEL="${LEMBAS_CHANNEL:-stable}" + echo "== install ==" pct exec "$CTID" -- bash -lc " set -e SITE_HOST='$SITE_HOST' LEMBAS_BRANCH='$BRANCH' REPO_URL='$REPO_URL' \ + INSTALL_UPDATE_HELPER='$INSTALL_UPDATE_HELPER' \ + LEMBAS_CHANNEL='$LEMBAS_CHANNEL' \ bash /tmp/lembas-src/deploy/install.sh " diff --git a/docs/notes/audit-0.9.md b/docs/notes/audit-0.9.md index f1e7eea..eecec84 100644 --- a/docs/notes/audit-0.9.md +++ b/docs/notes/audit-0.9.md @@ -183,10 +183,43 @@ Written and never read: `Chat.compacted_at`, `User.last_login_at`, `general.require_approval` (its comment says "reserved"), and the `back` form parameter on two `admin_models` routes. +**A third, found by asking where generated images go.** `db.delete(chat)` +cascades to the message and attachment *rows* and leaves every file on disk; +`files.remove_files_for_chats` was written for exactly that and was called from +one place, the temporary-chat sweep. The delete button, a schedule's task chat, +a helper's chat and account deletion all went straight to `db.delete`. + +Underneath it, a second fault that made the first invisible: `files.claim` +bound `message_id` and never `chat_id`, so anything uploaded on the *new-chat* +screen kept an empty `chat_id` for the rest of its life. Six readers filter on +that column, so for those files the harness did not name them among the attached +documents, the canvas refused to open them, and the one caller the cleanup +helper had found nothing to remove. Both fixed, and `chat_service.delete_chats` +is now the only way a chat is deleted. + *Stated:* `Message.parent_id` and `content_parts_json` are deliberate forward -looking columns; `Chat.archived` is already recorded in `PLAN.md` as a column nothing surfaces. +## Verified by rendering, not by reading — *Stage 2* + +There is no browser extension on this machine, so `/admin/prompts` and +`/admin/customization` were checked by rendering them through `TestClient`, +rewriting the asset URLs to `file://`, and driving headless Chromium against the +result. Worth recording because the first run of that harness **measured an +unstyled page**: the templates emit absolute `http://testserver/static/…` URLs +via `url_for`, not `/static/…`, so the replacement matched nothing and no CSS +loaded. It reported all five tab panels visible at once — a dramatic finding +that was entirely an artefact. The harness now fails loudly on any unrewritten +asset URL. + +What it measured once fixed: + +- `/admin/prompts`, tab bar's on-screen position across the five tabs: + **385 → 642px before** (the page moving under the reader), **195px constant + after**, with `scrollTop` staying at 0. +- `/admin/customization`, document height: **5495px before, 2820px after**. + ## Documentation — *Stage 2* - `README.md` is stale by six phases. "Planned: Image generation · OCR · diff --git a/src/lembas/__init__.py b/src/lembas/__init__.py index e3c56de..1e526cb 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.9" +__version__ = "0.9.10" diff --git a/src/lembas/api/admin_branding.py b/src/lembas/api/admin_branding.py index 4865ded..a1e548b 100644 --- a/src/lembas/api/admin_branding.py +++ b/src/lembas/api/admin_branding.py @@ -30,6 +30,14 @@ router = APIRouter(prefix="/admin/customization", tags=["admin-branding"]) MAX_CUSTOM_CSS = 40_000 +# How many custom themes an instance may keep. Not a design limit -- there is +# nothing in `theme_css` that cares -- but the whole set lives in one settings +# row read into a process-level snapshot on every render, and the page offers a +# blank block whenever there is room, so *some* number has to say when to stop +# offering. Twelve is far past what anybody wants and small enough that the +# stylesheet stays a stylesheet. +MAX_THEMES = 12 + def _page(request: Request, db: Db, saved: str = "", error: str = "") -> Response: values = settings_store.get_group(db, branding_service.BRANDING) @@ -55,6 +63,7 @@ def _page(request: Request, db: Db, saved: str = "", error: str = "") -> Respons "tokens": branding_service.THEME_TOKENS, "custom_themes": [t for t in brand.themes if not t.built_in], "bases": [name for name, _, _ in branding_service.BUILT_IN], + "max_themes": MAX_THEMES, "saved": saved, "error": error, }, @@ -167,6 +176,11 @@ async def save_themes(request: Request, db: Db, user: AdminUser) -> Response: every **read**, so a theme written straight into the settings table by hand, or stored by an earlier version, still has to produce a stylesheet that parses. Validating only on save would put that guarantee in the wrong place. + + The indices need not be contiguous and are not renumbered. The page renders + one block per theme plus a blank one, so clearing an id in the middle leaves + a gap -- and a gap is simply an index with no id, which the loop already + skips. Renumbering would be work in aid of nothing. """ form = await request.form() themes = [] @@ -186,6 +200,10 @@ async def save_themes(request: Request, db: Db, user: AdminUser) -> Response: }, } ) + # Enforced here as well as in the template, because the template's job is to + # stop offering and this one's is to stop accepting -- a crafted POST is not + # the page. + themes = themes[:MAX_THEMES] _write(db, {"themes": themes}) log.info("%d custom theme(s) saved by %s", len(themes), user.email) return RedirectResponse( diff --git a/src/lembas/api/admin_users.py b/src/lembas/api/admin_users.py index add11f2..b1c11fb 100644 --- a/src/lembas/api/admin_users.py +++ b/src/lembas/api/admin_users.py @@ -16,6 +16,7 @@ from lembas.db.models import ( ROLE_ADMIN, ROLE_PENDING, ROLE_USER, + Chat, Group, Model, User, @@ -23,6 +24,7 @@ from lembas.db.models import ( from lembas.security import permissions from lembas.security.passwords import hash_password, validate_password from lembas.security.sessions import revoke_all_for_user +from lembas.services import chat as chat_service from lembas.services import settings_store, sharing from lembas.services import usage as usage_service from lembas.web.templating import render @@ -249,6 +251,11 @@ async def delete_user(db: Db, user: AdminUser, user_id: str) -> Response: # still there to be found. sharing.forget_owner(db, target.id) sharing.forget_principal(db, PRINCIPAL_USER, target.id) + # And the same shape a third time: the chats cascade, their attachment rows + # cascade, and every file those rows named stays on disk with nothing left + # that will ever look at it. Before the delete, while the rows still say + # which files they are. + chat_service.delete_chats(db, list(db.scalars(select(Chat).where(Chat.user_id == target.id)))) db.delete(target) db.commit() log.info("%s deleted account %s", user.email, email) diff --git a/src/lembas/api/chats.py b/src/lembas/api/chats.py index bf994f1..e6d98b9 100644 --- a/src/lembas/api/chats.py +++ b/src/lembas/api/chats.py @@ -2098,7 +2098,9 @@ async def delete_chat(db: Db, user: RequiredUser, chat_id: str) -> Response: # there would be nothing left to find it by and a shell would sit open on # somebody's machine until the idle timeout noticed. await terminal_service.close_chat(chat_id) - db.delete(chat) + # Not `db.delete(chat)`: that cascades to the attachment rows and leaves + # every file they name on disk forever. See `chat_service.delete_chats`. + chat_service.delete_chats(db, [chat]) db.commit() response = Response(status_code=status.HTTP_204_NO_CONTENT) diff --git a/src/lembas/api/folders.py b/src/lembas/api/folders.py index ac0609e..237ce99 100644 --- a/src/lembas/api/folders.py +++ b/src/lembas/api/folders.py @@ -3,6 +3,7 @@ from __future__ import annotations from fastapi import APIRouter, Depends, Form, HTTPException, Request, Response, status +from sqlalchemy import select from sqlalchemy.orm import Session as DBSession from lembas.api.deps import Db, RequiredUser, require_permission @@ -36,6 +37,62 @@ def _depth_of(db: DBSession, folder: Folder | None) -> int: return depth +def _descendants(db: DBSession, folder: Folder) -> set[str]: + """Every folder under this one, and this one. Bounded by MAX_DEPTH.""" + found = {folder.id} + frontier = [folder.id] + for _ in range(MAX_DEPTH + 1): + if not frontier: + break + children = list( + db.scalars(select(Folder).where(Folder.parent_id.in_(frontier))) + ) + frontier = [c.id for c in children if c.id not in found] + found.update(frontier) + return found + + +def _subtree_height(db: DBSession, folder: Folder) -> int: + """How many levels this folder's own subtree occupies, itself included. + + A move has to consider it: the constraint is on the *deepest leaf* after the + move, not on the folder being dragged. + """ + height = 1 + frontier = [folder.id] + for _ in range(MAX_DEPTH + 1): + children = list( + db.scalars(select(Folder.id).where(Folder.parent_id.in_(frontier))) + ) + if not children: + break + height += 1 + frontier = children + return height + + +def candidate_parents(db: DBSession, user_id: str, folder: Folder) -> list[Folder]: + """Folders this one could be moved into. + + Everything the person owns, minus the folder itself and its own subtree -- + which is the cycle guard in `update_folder` stated as a list rather than as + a refusal. A picker that offers a move the route will reject is a control + that looks like it works. + + Depth is checked at the route rather than filtered here: it depends on how + tall *this* folder's subtree is, and a select that silently omitted a folder + for that reason would be unexplainable from the screen. + """ + blocked = _descendants(db, folder) + return [ + candidate + for candidate in db.scalars( + select(Folder).where(Folder.user_id == user_id).order_by(Folder.name) + ) + if candidate.id not in blocked + ] + + def _refresh_sidebar() -> Response: """Tell the browser to reload so the tree re-renders. @@ -141,6 +198,17 @@ async def update_folder( "A folder cannot be moved inside itself.", ) cursor = db.get(Folder, cursor.parent_id) if cursor.parent_id else None + # And the depth cap, which `create_folder` has always applied and this + # path never did -- moving a three-deep subtree under a six-deep folder + # builds a tree nine deep, which is what MAX_DEPTH exists to keep out of + # the recursive sidebar template. It went unnoticed because nothing in + # the interface could submit `parent_id` at all until now. + subtree = _subtree_height(db, folder) + if new_parent is not None and _depth_of(db, new_parent) + subtree > MAX_DEPTH: + raise HTTPException( + status.HTTP_400_BAD_REQUEST, + f"Folders cannot be nested more than {MAX_DEPTH} deep.", + ) folder.parent_id = new_parent.id if new_parent else None if "collapsed" in form: diff --git a/src/lembas/api/pages.py b/src/lembas/api/pages.py index 23d5120..20cd27d 100644 --- a/src/lembas/api/pages.py +++ b/src/lembas/api/pages.py @@ -606,6 +606,12 @@ async def chat_index( ) +def _candidate_parents(db: DBSession, user_id: str, folder: Folder) -> list[Folder]: + from lembas.api.folders import candidate_parents + + return candidate_parents(db, user_id, folder) + + @router.get("/folders/{folder_id}") async def folder_settings(request: Request, db: Db, user: RequiredUser, folder_id: str): """What a folder hands to the chats started inside it. @@ -631,6 +637,10 @@ async def folder_settings(request: Request, db: Db, user: RequiredUser, folder_i { "folder": folder, "chat": None, + # Imported here rather than at module scope: `api.folders` imports + # `api.deps`, which this module is a peer of, and the pair have been + # kept apart deliberately. + "parents": _candidate_parents(db, user.id, folder), "models": chat_service.available_models(db, user), **_agent_context(db, user, None), **sidebar_context(db, user), diff --git a/src/lembas/services/chat.py b/src/lembas/services/chat.py index b2b9070..5ea1cae 100644 --- a/src/lembas/services/chat.py +++ b/src/lembas/services/chat.py @@ -621,6 +621,37 @@ async def summarise_for_compaction( return raw.strip() +def delete_chats(db: DBSession, chats) -> int: + """Delete chats, and the files their attachments point at. + + **The one way to delete a chat.** `db.delete(chat)` cascades to its messages + and to its attachment *rows*, and leaves every file on disk -- a generated + image, an uploaded PDF, a photo -- with nothing that will ever look at them + again: `sweep_orphans` only considers uploads that were never attached. + + `files_service.remove_files_for_chats` was written for exactly this and was + called from one place, the temporary sweep. The delete button, a schedule's + task chat, a helper's hidden chat and deleting an account all went straight + to `db.delete`, so four of the five ways a chat can end leaked its files. + That is `sharing.forget_principal` again: a helper that exists, is correct, + and is not called on the path that needs it. + + The order matters and is why this is a function rather than a note. The + files have to be unlinked **while the rows still say which they are**, so it + happens before the delete and in the same session. + + Does not commit -- the caller decides, because some of them are deleting + other things in the same transaction. + """ + live = [chat for chat in chats if chat is not None] + if not live: + return 0 + files_service.remove_files_for_chats(db, [chat.id for chat in live]) + for chat in live: + db.delete(chat) + return len(live) + + def sweep_temporary(db: DBSession, older_than: timedelta = TEMPORARY_LIFETIME) -> int: """Delete temporary chats nobody has touched for a day. @@ -652,9 +683,7 @@ def sweep_temporary(db: DBSession, older_than: timedelta = TEMPORARY_LIFETIME) - if not stale: return 0 - files_service.remove_files_for_chats(db, [chat.id for chat in stale]) - for chat in stale: - db.delete(chat) + delete_chats(db, stale) db.commit() log.info("swept %d temporary chat(s)", len(stale)) return len(stale) diff --git a/src/lembas/services/files.py b/src/lembas/services/files.py index 242f586..acbe81b 100644 --- a/src/lembas/services/files.py +++ b/src/lembas/services/files.py @@ -29,7 +29,7 @@ from sqlalchemy import select from sqlalchemy.orm import Session as DBSession from lembas.config import settings -from lembas.db.models import KIND_DOCUMENT, KIND_IMAGE, KIND_TEXT, Attachment +from lembas.db.models import KIND_DOCUMENT, KIND_IMAGE, KIND_TEXT, Attachment, Message log = logging.getLogger(__name__) @@ -621,6 +621,19 @@ def claim(db: DBSession, *, ids: list[str], user_id: str, message_id: str) -> li Only unclaimed attachments belonging to this user are taken, so a stray or forged id cannot pull someone else's file into a conversation. + + **`chat_id` is set here, and it was not.** `POST /api/files` takes one, and + the composer sends it -- but only once a chat exists. A file picked on the + *new-chat* screen is stored before there is a chat to name, so its + `chat_id` stayed NULL for the rest of its life even after the message it + belongs to was sent. Six places filter on that column, and every one of them + was quietly wrong about those files: the harness did not name them among the + attached documents, the canvas refused to open them, and + `remove_files_for_chats` could not find them to delete -- so the temporary + sweep, the one caller it had, was removing nothing. + + Read from the message rather than passed in, so no caller can bind an + attachment to one chat and a message in another. """ if not ids: return [] @@ -634,8 +647,11 @@ def claim(db: DBSession, *, ids: list[str], user_id: str, message_id: str) -> li ) ) ) + message = db.get(Message, message_id) for attachment in pending: attachment.message_id = message_id + if message is not None: + attachment.chat_id = message.chat_id db.commit() return pending diff --git a/src/lembas/services/schedules.py b/src/lembas/services/schedules.py index f8a10b8..d836fe7 100644 --- a/src/lembas/services/schedules.py +++ b/src/lembas/services/schedules.py @@ -221,7 +221,9 @@ def delete(db: DBSession, schedule: Schedule, *, keep_chat: bool = True) -> None if keep_chat: chat.kind = "chat" else: - db.delete(chat) + from lembas.services import chat as chat_service + + chat_service.delete_chats(db, [chat]) db.delete(schedule) db.commit() diff --git a/src/lembas/services/subagent.py b/src/lembas/services/subagent.py index 8c031c1..ca8778e 100644 --- a/src/lembas/services/subagent.py +++ b/src/lembas/services/subagent.py @@ -338,9 +338,14 @@ def _cleanup(chat_id: str, *, keep: bool) -> None: return try: with session_scope() as db: + from lembas.services import chat as chat_service + chat = db.get(Chat, chat_id) if chat is not None: - db.delete(chat) + # A writing helper can generate an image or attach a file, and + # `db.delete` would leave both on disk with the row that named + # them gone. + chat_service.delete_chats(db, [chat]) except Exception: # noqa: BLE001 - tidying up is not the result log.debug("could not remove subagent chat %s", chat_id, exc_info=True) diff --git a/src/lembas/services/suggestions.py b/src/lembas/services/suggestions.py index 26cbdea..f7f15fd 100644 --- a/src/lembas/services/suggestions.py +++ b/src/lembas/services/suggestions.py @@ -27,32 +27,46 @@ MAX_DESCRIPTION = 300 MAX_PROMPT = 4000 # A card is sent the moment it is clicked, so each of these has to work cold -- -# with nothing pasted and nothing typed. They are written to ask for what they -# need, which turns the first reply into the right question rather than a guess -# at material nobody has given yet. +# with nothing pasted and nothing typed. These four are *self-contained* rather +# than question-asking: they are what somebody clicks on an instance they have +# just stood up, to find out whether the thing works and what the model behind +# it is. That is the honest first use, and it is a different job from the task +# cards that preceded them, which opened by asking for material the reader had +# not given. +# +# The name and description are read by somebody who has never seen this +# instance; the prompt is read by the model. They are allowed to differ, and +# here they do -- "Start a session" says what the card is for, and its prompt is +# the bare line that produces a clean opening turn. +# +# Only a fresh install gets these: `SEEDED_KEY` means an instance that has +# already seeded keeps whatever its administrator has since made of the list. # # No Middle-earth flavour: this is functional UI. DEFAULTS: tuple[tuple[str, str, str], ...] = ( ( - "Explain this", - "Get something confusing back in plain language.", - "I want something explained in plain language. Ask me what it is, then " - "give me one sentence summarising it, the details that actually matter, " - "and anything I should watch out for.", + "Start a session", + "Open a fresh conversation with nothing assumed.", + "Initializing a new session.", ), ( - "Draft a reply", - "Turn a message you have received into an answer you can send.", - "I need to reply to a message. Ask me what it says, what tone I want and " - "what outcome I am after, then write a draft I could send as it stands.", + "What can you do?", + "Have the model say which tools, instructions and limits it has.", + "Who are you and what are you capable of? What are your available tools? " + "What do you already know from your instructions? Do you have any set " + "personality?", ), ( - "Find the flaw", - "Have a plan argued with before you commit to it.", - "I want a plan argued with. Ask me what the plan is, then tell me what is " - "most likely to go wrong, what I am assuming without evidence, and what " - "would change your mind. Do not soften it, and do not agree just because " - "I sound confident.", + "Show me some code", + "See how the model writes, in a language of its own choosing.", + "Pick a random programming language and produce a coding example showing " + "your coding capabilities.", + ), + ( + "Tell me something", + "A fact worth knowing, from a subject picked at random.", + "Give me a random fun fact from a random topic of your choosing. " + "Available topics are science, history, sociology, theology etc.", ), ) diff --git a/src/lembas/web/static/css/admin.css b/src/lembas/web/static/css/admin.css index b49f83e..9f9af39 100644 --- a/src/lembas/web/static/css/admin.css +++ b/src/lembas/web/static/css/admin.css @@ -80,6 +80,18 @@ flex: none; overflow-x: auto; scrollbar-width: none; + /* Sticky where the *page* is the scroller, which is the admin layout. The + Tools panel is longer than a screen, so without this the bar scrolls away + and changing tab means scrolling back up to find it. Harmless on the + settings page, where `.tabs__body` scrolls underneath a bar that never + moves anyway. + + It needs the opaque `background` above it already has, or the panel would + show through. `z-index` because a panel's own cards establish stacking + contexts and would otherwise paint over it. */ + position: sticky; + top: 0; + z-index: 1; } .tabs__tab { diff --git a/src/lembas/web/templates/admin/customization.html b/src/lembas/web/templates/admin/customization.html index de3dc6d..46580b2 100644 --- a/src/lembas/web/templates/admin/customization.html +++ b/src/lembas/web/templates/admin/customization.html @@ -123,9 +123,25 @@ {# --- Themes --------------------------------------------------------------- #} {# - Three blocks, always rendered, so adding a theme needs no JavaScript: an empty - id means that block is not a theme. Saving replaces the whole list, which is - what makes removing one a matter of clearing its id. + One block per theme that exists, plus a single blank one to add the next. + + It used to be three blocks, always rendered, whether or not anybody had made a + theme -- so a fresh instance opened on fifty-seven empty colour boxes under + three identical headings, and the fourth theme was unreachable without editing + the template. The spare-block pattern is the no-JavaScript way to do "add + another": fill the blank one, save, and the page comes back with your theme + and a new blank. There is no button to press and nothing to clone, which is + why this still works with scripting off. + + The colours live in a
so a block is a heading and three fields until + you ask for them. Nineteen inputs is the right number to *offer* and the wrong + number to *show*: everything left empty inherits from the base, so most themes + set four or five. + + Saving replaces the whole list, which is what makes removing one a matter of + clearing its id -- and why the blocks may have gaps in their numbering without + anything caring. `save_themes` skips an index with no id and counts from the + keys it was actually sent. #}
@@ -138,11 +154,14 @@ are worked out from the accent, so you do not have to.

- {% for index in range(3) %} + {# The spare block is offered until the cap, so the form cannot grow without + end and the page cannot stop offering one while there is room. #} + {% set slots = custom_themes | length + (1 if custom_themes | length < max_themes else 0) %} + {% for index in range(slots) %} {% set existing = custom_themes[index] if index < custom_themes | length else none %}

- {{ existing.label if existing else "A theme of your own" }} + {{ existing.label or existing.id if existing else "Add a theme" }}

@@ -152,7 +171,11 @@ value="{{ existing.id if existing else '' }}" maxlength="24" pattern="[a-z][a-z0-9-]*" placeholder="dusk">

- Lowercase letters, digits and hyphens. Clear it to remove the theme. + {% if existing %} + Lowercase letters, digits and hyphens. Clear it to remove this theme. + {% else %} + Lowercase letters, digits and hyphens. Give it one to make a theme. + {% endif %}

@@ -172,23 +195,42 @@
-
- {% for name, description in tokens %} -
- - -

--{{ name }}

+ {# Open on a theme that has set something, so an existing override is never + hidden behind a disclosure somebody has to know to open. #} + + + Colours + {% if existing and existing.tokens %} + {{ existing.tokens | length }} set + {% else %} + all inherited + {% endif %} + +
+ {% for name, description in tokens %} +
+ + +

--{{ name }}

+
+ {% endfor %}
- {% endfor %} -
+
{% endfor %} + + {% if custom_themes | length >= max_themes %} +

+ That is {{ max_themes }} themes, which is the limit. Clear one's id and + save to make room for another. +

+ {% endif %}
diff --git a/src/lembas/web/templates/admin/prompts.html b/src/lembas/web/templates/admin/prompts.html index b13c7d7..a1caaba 100644 --- a/src/lembas/web/templates/admin/prompts.html +++ b/src/lembas/web/templates/admin/prompts.html @@ -18,6 +18,77 @@
{{ icon("check", "icon--sm") }} Prompts saved.
{% endif %} +{# + The editor first, the reference after, and that ordering is the fix rather + than a preference. + + The Variables legend and the Preview both run to a screen each, and they used + to sit *above* the tabs -- so the thing this page exists for started two + screens down. Every tab switch then had to move the viewport to be any use, + and `ui.js` did: it put the tab bar at the top of the scroller. On a short + panel the scroller cannot go that far, so the browser clamped to the maximum + and left the panel stranded above a screen of nothing, which reads as a page + that failed to load. That is the empty space; the scrolling was the cause and + not the cure. + + With the tabs near the top there is nothing to scroll past: switching panels + changes what is below the bar and leaves the bar where it is. The reference + cards keep their place in the reading order -- after the work, where you go + to look something up -- and the preview still watches the form through + `hx-include`, which does not care about document order. +#} + +
+
+ {% for key, label, fragments in groups %} + + + {% endfor %} +
+ +
+ {% for key, label, fragments in groups %} +
+ {% for fragment in fragments %} + {% with value = values[fragment.key], overridden = fragment.key in overridden %} + {% include "admin/_prompt_field.html" %} + {% endwith %} + {% endfor %} +
+ {% endfor %} +
+
+ +
+

Length

+
+ + +

+ Everything above is cut off past this. 0 means the built-in + {{ default_harness_chars }}. It is a backstop against a large skill index + or memory list quietly eating the context window, not a budget to tune. +

+
+
+ +
+ + {# + data-confirm-button, not data-confirm: this button acts on its own through + formaction, and confirming the whole form would also catch plain Save. + #} + +
+ + +

Variables

@@ -134,57 +205,6 @@ keyup changed delay:700ms from:#prompt-form">

-
-
-
- {% for key, label, fragments in groups %} - - - {% endfor %} -
- -
- {% for key, label, fragments in groups %} -
- {% for fragment in fragments %} - {% with value = values[fragment.key], overridden = fragment.key in overridden %} - {% include "admin/_prompt_field.html" %} - {% endwith %} - {% endfor %} -
- {% endfor %} -
-
- -
-

Length

-
- - -

- Everything above is cut off past this. 0 means the built-in - {{ default_harness_chars }}. It is a backstop against a large skill index - or memory list quietly eating the context window, not a budget to tune. -

-
-
- -
- - {# - data-confirm-button, not data-confirm: this button acts on its own through - formaction, and confirming the whole form would also catch plain Save. - #} - -
-
-

Tool descriptions

diff --git a/src/lembas/web/templates/folders/edit.html b/src/lembas/web/templates/folders/edit.html index a8dac2a..c0d57da 100644 --- a/src/lembas/web/templates/folders/edit.html +++ b/src/lembas/web/templates/folders/edit.html @@ -64,6 +64,33 @@ For you, not for any model. It is never sent anywhere.

+ {# + Nesting has worked at the route since folders existed -- with a cycle + guard and a depth cap -- and the sidebar template has always + recursed to draw it. Nothing anywhere submitted `parent_id`, so the + README advertised "arbitrarily nested" folders that could not be + nested. This is that control. + + The options are `candidate_parents`, which is the cycle guard stated + as a list rather than as a refusal: a picker offering a move the + route will reject is a control that looks like it works. + #} +
+ + +

+ A folder inside another inherits its system prompt where it has + none of its own. +

+
diff --git a/tests/test_admin_prompts.py b/tests/test_admin_prompts.py index 8d819e8..001b420 100644 --- a/tests/test_admin_prompts.py +++ b/tests/test_admin_prompts.py @@ -2,6 +2,8 @@ from __future__ import annotations +import pathlib + import pytest from fastapi.testclient import TestClient from sqlalchemy import select @@ -49,6 +51,39 @@ def test_the_preview_controls_are_inside_what_the_preview_includes( assert f'name="{name}"' in controls, name +def test_the_editor_comes_before_the_reference(client: TestClient, registered): + """The Variables legend and the Preview run to a screen each and used to sit + above the tabs, so the thing this page exists for started two screens down. + Every tab switch then had to move the viewport to be any use -- and on a + short panel the scroller cannot reach the tab bar, so it clamped to the + bottom and left the panel stranded above a screenful of nothing. + + Asserted as an ordering rather than by reading the CSS, because the position + is the fix: with the tabs near the top there is nothing to scroll past. + """ + page = client.get("/admin/prompts").text + tabs = page.index('class="tabs__bar"') + assert tabs < page.index("Variables") + assert tabs < page.index("Preview") + assert tabs < page.index("Tool descriptions") + + +def test_the_tab_bar_sticks_to_the_top_of_the_scroller(): + """The Tools panel is longer than a screen, so a bar that scrolls away means + changing tab is a scroll back up to find it.""" + css = ( + pathlib.Path(__file__).resolve().parents[1] + / "src/lembas/web/static/css/admin.css" + ).read_text() + bar = css.split(".tabs__bar {", 1)[1].split("}", 1)[0] + assert "position: sticky" in bar + assert "top: 0" in bar + # Opaque, or the panel shows through it; stacked, or a panel's own cards + # paint over it. + assert "background:" in bar + assert "z-index:" in bar + + def test_the_page_is_refused_to_a_plain_user(client: TestClient, plain_user): assert client.get("/admin/prompts").status_code == 403 assert client.post("/admin/prompts", data={}).status_code == 403 diff --git a/tests/test_branding.py b/tests/test_branding.py index e9377d7..206d746 100644 --- a/tests/test_branding.py +++ b/tests/test_branding.py @@ -232,6 +232,80 @@ def test_clearing_an_id_removes_the_theme(db, client, registered): assert "dusk" not in branding.snapshot().theme_ids +def test_the_page_offers_one_blank_theme_and_no_more(db, client, registered): + """It rendered three blocks whether or not anybody had made a theme, so a + fresh instance opened on fifty-seven empty colour boxes under three + identical headings -- and the fourth theme was unreachable without editing + the template. One block per theme plus a single blank one is the + no-JavaScript way to say "add another".""" + page = client.get("/admin/customization").text + assert page.count('name="theme_0_id"') == 1 + assert 'name="theme_1_id"' not in page + + _save_theme(client, theme_0_bg="#123456") + page = client.get("/admin/customization").text + # The saved one, and a fresh blank below it. + assert 'name="theme_1_id"' in page + assert 'name="theme_2_id"' not in page + + +def test_a_fourth_theme_is_reachable(db, client, registered): + """Three was a template constant, and the page is what made it a limit.""" + data = {} + for index, name in enumerate(("one", "two", "three", "four")): + data[f"theme_{index}_id"] = name + data[f"theme_{index}_base"] = "moria" + client.post("/admin/customization/themes", data=data, follow_redirects=False) + + ids = branding.snapshot().theme_ids + assert {"one", "two", "three", "four"} <= set(ids) + + +def test_the_page_stops_offering_at_the_cap(db, client, registered): + from lembas.api.admin_branding import MAX_THEMES + + data = {} + for index in range(MAX_THEMES): + data[f"theme_{index}_id"] = f"t{index}" + data[f"theme_{index}_base"] = "moria" + client.post("/admin/customization/themes", data=data, follow_redirects=False) + + page = client.get("/admin/customization").text + assert f'name="theme_{MAX_THEMES}_id"' not in page + assert "which is the limit" in page + + +def test_the_cap_is_enforced_on_save_as_well_as_offered(db, client, registered): + """The template's job is to stop offering; the route's is to stop accepting. + A crafted POST is not the page.""" + from lembas.api.admin_branding import MAX_THEMES + + data = {} + for index in range(MAX_THEMES + 5): + data[f"theme_{index}_id"] = f"t{index}" + data[f"theme_{index}_base"] = "moria" + client.post("/admin/customization/themes", data=data, follow_redirects=False) + + custom = [t for t in branding.snapshot().themes if not t.built_in] + assert len(custom) == MAX_THEMES + + +def test_a_gap_in_the_middle_does_not_disturb_the_rest(db, client, registered): + """Clearing an id leaves a hole in the numbering, and nothing renumbers.""" + data = {} + for index, name in enumerate(("one", "two", "three")): + data[f"theme_{index}_id"] = name + data[f"theme_{index}_base"] = "moria" + client.post("/admin/customization/themes", data=data, follow_redirects=False) + + data["theme_1_id"] = "" + client.post("/admin/customization/themes", data=data, follow_redirects=False) + + ids = set(branding.snapshot().theme_ids) + assert "one" in ids and "three" in ids + assert "two" not in ids + + # --- The stylesheet ------------------------------------------------------------- def test_custom_css_is_served_as_a_stylesheet(db, client, registered): """A route rather than an inline `