Files that outlived the chats that held them, and a page that led with its footnotes

The second audit pass. Four things, and the first two were reported.

The Prompts page put a screen of variables and a screen of preview above
the editor, so the tabs began two screens down and switching one had to
drag the whole page to be any use -- and on a short tab it could not drag
far enough, leaving the panel stranded above a screenful of nothing.
Editor first, reference after, bar sticky. Custom themes were three fixed
slots: fifty-seven empty colour boxes on a fresh instance and no way to
make a fourth theme. One block per theme plus a blank one, colours behind
a disclosure. Both measured rather than argued about -- rendered through
TestClient and driven under headless Chromium, where the tab bar moved
385->642px before and does not move now, and the themes page went from
5495px to 2820px.

Asking where generated images go found the other two. Deleting a chat
cascades to the attachment rows and leaves every file on disk; the helper
written for exactly that was called from one place, and it was not the
delete button, a schedule's chat, a helper's chat or deleting an account.
Underneath it, `claim` bound message_id and never chat_id, so anything
picked before a chat existed kept an empty chat_id forever -- which six
readers filter on, so those files were also unnamed in the prompt,
unopenable in the canvas, and invisible to the one caller the cleanup had.

And folders nest now. The route has handled parent_id since folders
existed, with a cycle guard and a depth cap the move path never applied;
the sidebar has always drawn a tree. Nothing could ask for one.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

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</h2>")
+    assert tabs < page.index("Preview</h2>")
+    assert tabs < page.index("Tool descriptions</h2>")
+
+
+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

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 `<style>`, which is a security property

tests/test_files.py

@@ -684,3 +684,51 @@ def test_only_an_administrator_may_change_extraction(client, registered):
     assert client.post("/admin/extraction", data={"max_upload_mb": "1"}).status_code == 403
     assert client.post("/admin/extraction/search", data={}).status_code == 403
     assert client.post("/admin/extraction/rebuild", data={}).status_code == 403
+
+
+def test_deleting_a_chat_removes_the_files_it_held(
+    client: TestClient, db, chat_with_model
+):
+    """Deleting a Chat cascades to its message and attachment *rows* and leaves
+    every file on disk -- a generated image, an uploaded PDF -- with nothing that
+    will ever look at them again, since `sweep_orphans` only considers uploads
+    that were never attached.
+
+    `remove_files_for_chats` was written for this and was called from exactly one
+    place, the temporary-chat sweep. The delete button was not it. That is
+    `sharing.forget_principal` a third time: a correct helper, absent from the
+    path that needs it.
+    """
+    client.post("/api/files", files={"file": ("a.png", png_bytes(), "image/png")})
+    attachment = db.scalar(select(Attachment))
+    path = files_service.stored_path(attachment.stored_name)
+    client.post(
+        f"/api/chats/{chat_with_model}/messages",
+        data={"content": "here", "file_ids": [attachment.id]},
+    )
+    assert path.exists()
+
+    client.delete(f"/api/chats/{chat_with_model}")
+
+    assert db.scalar(select(Attachment)) is None
+    assert not path.exists(), "the row went and the file stayed"
+
+
+def test_delete_chats_is_what_every_path_uses(client: TestClient, db, chat_with_model):
+    """One function, so the next thing that deletes a chat cannot forget. It has
+    to unlink *before* the rows go -- they are what says which files to remove."""
+    client.post("/api/files", files={"file": ("a.png", png_bytes(), "image/png")})
+    attachment = db.scalar(select(Attachment))
+    path = files_service.stored_path(attachment.stored_name)
+    client.post(
+        f"/api/chats/{chat_with_model}/messages",
+        data={"content": "here", "file_ids": [attachment.id]},
+    )
+
+    chat = db.get(Chat, chat_with_model)
+    assert chat_service.delete_chats(db, [chat]) == 1
+    db.commit()
+
+    assert not path.exists()
+    assert chat_service.delete_chats(db, []) == 0
+    assert chat_service.delete_chats(db, [None]) == 0

tests/test_folders.py

@@ -361,3 +361,78 @@ def test_the_settings_form_posts_at_a_route_that_serves_patch(
     folder = _folder(db, "Errands")
     assert client.post(f"/api/folders/{folder.id}", data={"name": "x"}).status_code == 405
     assert client.patch(f"/api/folders/{folder.id}", data={"name": "x"}).status_code == 204
+
+
+# --- Nesting, which had no control ---------------------------------------------
+def folders_service_create(client, name: str, *, parent: str = "") -> str:
+    """Make a folder through the route and return its id."""
+    data = {"name": name}
+    if parent:
+        data["parent_id"] = parent
+    client.post("/api/folders", data=data)
+
+    from lembas.db.session import session_scope
+
+    with session_scope() as db:
+        return db.scalar(select(Folder).where(Folder.name == name)).id
+
+
+def test_a_folder_can_be_put_inside_another(client: TestClient, db, registered):
+    """`parent_id` has been handled at the route since folders existed, with a
+    cycle guard and a depth cap, and `partials/_folder.html` has always recursed
+    to draw the tree. Nothing anywhere submitted it, so the README advertised
+    "arbitrarily nested" folders that could not be nested."""
+    outer = folders_service_create(client, "Outer")
+    inner = folders_service_create(client, "Inner")
+
+    client.patch(f"/api/folders/{inner}", data={"parent_id": outer})
+
+    assert db.get(Folder, inner).parent_id == outer
+
+
+def test_the_settings_page_offers_the_move(client: TestClient, db, registered):
+    """And offers only moves the route will accept -- a picker listing a folder
+    that would be refused is a control that looks like it works."""
+    outer = folders_service_create(client, "Outer")
+    inner = folders_service_create(client, "Inner")
+    client.patch(f"/api/folders/{inner}", data={"parent_id": outer})
+
+    page = client.get(f"/folders/{outer}").text
+    assert 'name="parent_id"' in page
+    # Its own child is not offered: that move is the cycle the route refuses.
+    assert f'value="{inner}"' not in page
+    assert f'value="{outer}"' not in page, "a folder cannot be its own parent"
+
+
+def test_a_folder_cannot_be_moved_into_its_own_subtree(client: TestClient, db, registered):
+    outer = folders_service_create(client, "Outer")
+    inner = folders_service_create(client, "Inner")
+    client.patch(f"/api/folders/{inner}", data={"parent_id": outer})
+
+    refused = client.patch(f"/api/folders/{outer}", data={"parent_id": inner})
+    assert refused.status_code == 400
+    assert db.get(Folder, outer).parent_id is None
+
+
+def test_moving_respects_the_depth_cap(client: TestClient, db, registered):
+    """`create_folder` has always applied MAX_DEPTH; the move path never did, so
+    a three-deep subtree could be dropped under a six-deep folder and build a
+    tree the recursive sidebar template was never meant to draw. It went
+    unnoticed because nothing could submit `parent_id` at all."""
+    from lembas.api.folders import MAX_DEPTH
+
+    chain = []
+    parent = ""
+    for index in range(MAX_DEPTH):
+        made = folders_service_create(client, f"L{index}", parent=parent)
+        chain.append(made)
+        parent = made
+
+    loose = folders_service_create(client, "Loose")
+    child = folders_service_create(client, "LooseChild", parent=loose)
+    assert db.get(Folder, child).parent_id == loose
+
+    # `loose` is two tall; the deepest folder is already at the cap.
+    refused = client.patch(f"/api/folders/{loose}", data={"parent_id": chain[-1]})
+    assert refused.status_code == 400
+    assert db.get(Folder, loose).parent_id is None

tests/test_suggestions.py

@@ -76,7 +76,10 @@ def test_every_default_prompt_stands_on_its_own():
     model has to guess at."""
     for name, _description, prompt in suggestions_service.DEFAULTS:
         assert prompt == prompt.strip(), f"{name} has stray whitespace"
-        assert prompt.endswith("."), f"{name} does not end as a complete sentence"
+        # Any terminal punctuation, not a full stop. The point is that the
+        # prompt finishes rather than trails off, and a card that asks the model
+        # a direct question ends with "?" -- which this asserted was malformed.
+        assert prompt[-1] in ".?!", f"{name} does not end as a complete sentence"
 
 
 # --- What is shown ------------------------------------------------------------