From f3827706dff45d440288d033d16916c7bebe00da Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Mon, 27 Apr 2026 05:11:39 -0400 Subject: [PATCH 1/4] fix: drawer delete_turn guards event_id <= 0 (T110.1) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A stale tab or hand-crafted request posting event_id=0 to the surgical delete route would compute after_event_id=-1 and silently truncate the entire log. Now rejected with 400. SQLite assigns event_log ids starting at 1, so any legitimate id is always >= 1 — non-positive values can only indicate a client bug. Test: tests/test_drawer_phase4.py::test_delete_turn_with_event_id_zero_returns_400. --- chat/web/drawer.py | 12 ++++++++++++ tests/test_drawer_phase4.py | 23 +++++++++++++++++++++++ 2 files changed, 35 insertions(+) diff --git a/chat/web/drawer.py b/chat/web/drawer.py index 5396ae8..f0c3ddb 100644 --- a/chat/web/drawer.py +++ b/chat/web/drawer.py @@ -1278,7 +1278,19 @@ async def delete_turn( A snapshot is taken before truncation (inside ``execute_rewind``) so the user can recover via the snapshot index. + + T110.1 guards ``event_id <= 0``: a stale tab or hand-crafted request + posting ``event_id=0`` would otherwise compute ``after_event_id=-1`` + and silently truncate the entire log. ``id`` is auto-assigned by + SQLite starting at 1 so any caller's "real" id is always >= 1; a + zero or negative value can only mean a client bug, surfaced as 400. """ + if int(event_id) <= 0: + raise HTTPException( + status_code=400, + detail=f"event_id must be a positive integer, got {event_id}", + ) + chat = get_chat(conn, chat_id) if chat is None: raise HTTPException(status_code=404, detail=f"chat not found: {chat_id}") diff --git a/tests/test_drawer_phase4.py b/tests/test_drawer_phase4.py index f94f266..f4f3235 100644 --- a/tests/test_drawer_phase4.py +++ b/tests/test_drawer_phase4.py @@ -458,6 +458,29 @@ def test_t98_4_delete_invokes_rewind_and_drops_cascade(client, tmp_path): assert row is None, f"event {ev_id} should have been deleted" +def test_delete_turn_with_event_id_zero_returns_400(client, tmp_path): + """T110.1: ``event_id <= 0`` is an obvious client error and must NOT + silently rewind the entire log via ``after_event_id = -1``. The route + rejects it with 400 so the audit trail stays intact. + """ + db = tmp_path / "test.db" + _seed_chat(db) + _seed_turns(db) + + # Sanity: events present before the bad request. + with open_db(db) as conn: + before = conn.execute("SELECT COUNT(*) FROM event_log").fetchone()[0] + assert before > 0 + + response = client.post("/chats/chat_bot_a/drawer/turn/delete/0") + assert response.status_code == 400 + + # And the log was NOT truncated. + with open_db(db) as conn: + after = conn.execute("SELECT COUNT(*) FROM event_log").fetchone()[0] + assert after == before + + # --------------------------------------------------------------------------- # T98.5 — remaining v1 edits (chat narrative anchor + weather). # --------------------------------------------------------------------------- From a45a33534f0100f31dac9612691fe4bedcc17946 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Mon, 27 Apr 2026 05:12:28 -0400 Subject: [PATCH 2/4] fix: drawer delete-impact modal HTML escapes user-controllable fields (T110.2) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The delete-impact modal is built via raw f-string concatenation from the ImpactReport — item.kind / item.description / report.notes ultimately embed user-controllable content (turn prose, scene timestamps). A turn with prose like "" would reach the rendered HTML verbatim. Currently safe (the fields embedded today are bounded strings) but defense-in-depth — wrap with html.escape() so future description changes can't smuggle markup through. Test: tests/test_drawer_phase4.py::test_delete_impact_modal_escapes_user_controllable_strings. --- chat/web/drawer.py | 18 +++++++++++++++--- tests/test_drawer_phase4.py | 36 ++++++++++++++++++++++++++++++++++++ 2 files changed, 51 insertions(+), 3 deletions(-) diff --git a/chat/web/drawer.py b/chat/web/drawer.py index f0c3ddb..a29f281 100644 --- a/chat/web/drawer.py +++ b/chat/web/drawer.py @@ -27,6 +27,7 @@ one so a later inverse edit can restore state (§6.4 final paragraph). from __future__ import annotations +import html import json import uuid from pathlib import Path @@ -1238,18 +1239,29 @@ async def delete_preview( # reusing the drawer template would require a fragment include just # for this surface. Mirrors the rewind-preview style in # :func:`chat.web.turns.rewind_preview`. + # + # T110.2: ``item.kind``, ``item.description``, and the notes carry + # user-controllable content (turn prose, scene timestamps, etc.). + # Wrap them with :func:`html.escape` so a payload like + # ```` renders as inert text. ``chat_id`` + # is matched against the projected ``chats`` table at request time + # (404 above) so it isn't free-form, but we escape it for symmetry. items_html = "".join( - f"
  • {item.kind}: {item.description}
    • " + f"
  • {html.escape(item.kind)}: " + f"{html.escape(item.description)}
    • " for item in report.cascading ) - notes_html = "".join(f"
  • {note}
    • " for note in report.notes) + notes_html = "".join( + f"
  • {html.escape(note)}
    • " for note in report.notes + ) body = ( "
    " f"

    Delete event {report.target_event_id}?

    " f"

    This will discard {len(report.cascading)} events. Cascade:

    " f"
      {items_html or '
    • none
      • '}
    " f"
      {notes_html}
    " - f"
    " "" "
    " diff --git a/tests/test_drawer_phase4.py b/tests/test_drawer_phase4.py index f4f3235..0b2b473 100644 --- a/tests/test_drawer_phase4.py +++ b/tests/test_drawer_phase4.py @@ -458,6 +458,42 @@ def test_t98_4_delete_invokes_rewind_and_drops_cascade(client, tmp_path): assert row is None, f"event {ev_id} should have been deleted" +def test_delete_impact_modal_escapes_user_controllable_strings(client, tmp_path): + """T110.2: defense-in-depth — fields embedded in the modal HTML come + from event payloads (turn prose, scene timestamps, etc.) which are + ultimately user-controllable. Wrap them with ``html.escape`` so a + payload like ```` renders as inert text and + doesn't leak through into the rendered modal as actual markup. + """ + db = tmp_path / "test.db" + _seed_chat(db) + + # Seed a user_turn whose prose contains an HTML-script payload. The + # modal renders ``description = "turn N (you: )"`` so + # the prose flows verbatim into the cascade list
  • . + with open_db(db) as conn: + evil_id = append_and_apply( + conn, + kind="user_turn", + payload={ + "chat_id": "chat_bot_a", + "prose": "", + "segments": [], + }, + ) + + response = client.get( + f"/chats/chat_bot_a/drawer/turn/delete-preview/{evil_id}" + ) + assert response.status_code == 200 + body = response.text + + # Raw `` renders as inert text. ``chat_id`` - # is matched against the projected ``chats`` table at request time - # (404 above) so it isn't free-form, but we escape it for symmetry. - items_html = "".join( - f"
  • {html.escape(item.kind)}: " - f"{html.escape(item.description)}
    • " - for item in report.cascading + # T110.3: render via the ``_delete_impact_modal.html`` Jinja partial + # so HTML autoescape covers user-controllable fields (item.kind, + # item.description, notes) automatically. The prior implementation + # built the modal HTML via raw f-string concatenation and required + # explicit ``html.escape()`` calls (T110.2) on each interpolated + # field; under autoescape those calls become redundant. Mirrors the + # rewind-preview style in :func:`chat.web.turns.rewind_preview`. + return TEMPLATES.TemplateResponse( + request, + "_delete_impact_modal.html", + {"chat_id": chat_id, "impact": report}, ) - notes_html = "".join( - f"
  • {html.escape(note)}
    • " for note in report.notes - ) - body = ( - "
    " - f"

    Delete event {report.target_event_id}?

    " - f"

    This will discard {len(report.cascading)} events. Cascade:

    " - f"
      {items_html or '
    • none
      • '}
    " - f"
      {notes_html}
    " - f"
    " - "" - "
    " - "
    " - ) - return HTMLResponse(body) @router.post( diff --git a/tests/test_drawer_phase4.py b/tests/test_drawer_phase4.py index 0b2b473..20b428e 100644 --- a/tests/test_drawer_phase4.py +++ b/tests/test_drawer_phase4.py @@ -458,6 +458,35 @@ def test_t98_4_delete_invokes_rewind_and_drops_cascade(client, tmp_path): assert row is None, f"event {ev_id} should have been deleted" +def test_delete_impact_modal_uses_jinja_partial(client, tmp_path): + """T110.3: the modal HTML is rendered from a Jinja partial + (`_delete_impact_modal.html`) rather than f-string concatenation in + Python. Verify the partial-rendered shape: the wrapping + ``delete-impact-modal`` div, the cascade list, and the confirm form. + + The partial inherits Jinja2 autoescape so HTML safety follows + automatically — the explicit ``html.escape()`` calls from T110.2 + become redundant once this lands. + """ + db = tmp_path / "test.db" + _seed_chat(db) + user_id, _bot_id = _seed_turns(db) + + response = client.get( + f"/chats/chat_bot_a/drawer/turn/delete-preview/{user_id}" + ) + assert response.status_code == 200 + body = response.text + + # Markup shape that the partial produces. Double-quoted attributes + # signal Jinja rendering (the prior f-string used single quotes). + assert '
    ' in body + assert '
      ' in body + # The confirm form still posts to the same delete route. + assert f"/chats/chat_bot_a/drawer/turn/delete/{user_id}" in body + assert "Confirm delete" in body + + def test_delete_impact_modal_escapes_user_controllable_strings(client, tmp_path): """T110.2: defense-in-depth — fields embedded in the modal HTML come from event payloads (turn prose, scene timestamps, etc.) which are From 2ab8fcbdf0d46c4b53637c925cd3b3894cadfdf7 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Mon, 27 Apr 2026 05:14:59 -0400 Subject: [PATCH 4/4] feat: drawer bulk significance re-rate per chat (T110.4) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The drawer's Significance review panel previously only supported per-memory edits. Adds a bulk control: pick ``level_from`` and ``level_to``, and every memory in the chat at ``level_from`` is moved to ``level_to``. Implementation emits one ``manual_edit`` event per matching memory (not a single bulk event) so the §6.4 per-row audit trail stays intact — each affected memory carries its own ``prior_value -> new_value`` snapshot, so an inverse edit can restore an individual row without needing to inspect a bulk payload's member list. Reuses the existing ``memory_significance`` ``manual_edit`` projector branch (T25), so no state-layer changes are required. The route rejects no-op submissions (``level_from == level_to``) with 400 to avoid padding the event log with empty edits, and clamps both levels to 0..3 (matching ``edit_memory_significance``). UI: a small ``
      `` block in the Significance review section with two number inputs and a submit button. Test: tests/test_drawer_phase4.py::test_bulk_significance_re_rate_emits_manual_edit_per_memory. --- chat/templates/_drawer.html | 19 ++++++++ chat/web/drawer.py | 58 ++++++++++++++++++++++++ tests/test_drawer_phase4.py | 89 +++++++++++++++++++++++++++++++++++++ 3 files changed, 166 insertions(+) diff --git a/chat/templates/_drawer.html b/chat/templates/_drawer.html index 8cfdd5f..6bbfeeb 100644 --- a/chat/templates/_drawer.html +++ b/chat/templates/_drawer.html @@ -547,6 +547,25 @@
    {% endif %} + {# T110.4: bulk significance re-rate. Move every memory in this chat + at level_from to level_to with one manual_edit event per row, so + the audit trail stays per-memory. #} +
    + Bulk re-rate significance +
    + + + +
    +
    diff --git a/chat/web/drawer.py b/chat/web/drawer.py index b965e7a..5f94957 100644 --- a/chat/web/drawer.py +++ b/chat/web/drawer.py @@ -411,6 +411,64 @@ async def edit_memory_significance( return await drawer(chat_id, request, conn) +@router.post( + "/chats/{chat_id}/drawer/memory/significance/bulk", + response_class=HTMLResponse, +) +async def bulk_re_rate_significance( + chat_id: str, + request: Request, + level_from: int = Form(...), + level_to: int = Form(...), + conn=Depends(get_conn), +): + """T110.4: bulk re-rate every memory in this chat at ``level_from`` + to ``level_to``. + + Fans out into one ``manual_edit`` event per matching memory rather + than a single bulk event so the §6.4 audit trail stays per-row — + each affected memory carries its own ``prior_value -> new_value`` + snapshot, so an inverse edit can restore an individual row without + needing to inspect a bulk payload's member list. The drawer's + significance-distribution panel surfaces the new buckets on the + refreshed partial. + + Both levels are clamped to 0..3 (matching ``edit_memory_significance``) + and a no-op (``level_from == level_to``) is rejected with 400 so a + misclick can't pad the event log with empty edits. + """ + chat = get_chat(conn, chat_id) + if chat is None: + raise HTTPException(status_code=404, detail=f"chat not found: {chat_id}") + + lf = max(0, min(3, int(level_from))) + lt = max(0, min(3, int(level_to))) + if lf == lt: + raise HTTPException( + status_code=400, + detail=f"level_from and level_to must differ (both = {lf})", + ) + + rows = conn.execute( + "SELECT id FROM memories WHERE chat_id = ? AND significance = ? " + "ORDER BY id ASC", + (chat_id, lf), + ).fetchall() + for row in rows: + memory_id = int(row[0]) + append_and_apply( + conn, + kind="manual_edit", + payload={ + "target_kind": "memory_significance", + "target_id": memory_id, + "prior_value": lf, + "new_value": lt, + }, + ) + return await drawer(chat_id, request, conn) + + @router.post( "/chats/{chat_id}/drawer/memory/{memory_id}/pin", response_class=HTMLResponse, diff --git a/tests/test_drawer_phase4.py b/tests/test_drawer_phase4.py index 20b428e..be4d854 100644 --- a/tests/test_drawer_phase4.py +++ b/tests/test_drawer_phase4.py @@ -523,6 +523,95 @@ def test_delete_impact_modal_escapes_user_controllable_strings(client, tmp_path) assert "<script>alert" in body +def test_bulk_significance_re_rate_emits_manual_edit_per_memory(client, tmp_path): + """T110.4: bulk significance re-rate fans out into one + ``manual_edit`` event per matching memory — preserving the per-row + audit trail (and reversibility) instead of collapsing everything + into a single bulk event. + + Seed five memories at significance 0, bulk re-rate 0 -> 2, and + verify five new ``memory_significance`` ``manual_edit`` rows landed + AND every memory now sits at significance 2. + """ + db = tmp_path / "test.db" + _seed_chat(db) + + # Five memories at significance 0. + with open_db(db) as conn: + for i in range(5): + append_and_apply( + conn, + kind="memory_written", + payload={ + "owner_id": "bot_a", + "chat_id": "chat_bot_a", + "pov_summary": f"low-sig memory {i}", + "witness_you": 1, + "witness_host": 1, + "witness_guest": 0, + "significance": 0, + }, + ) + # Plus one memory at significance 1 to verify the re-rate is + # scoped to ``level_from`` and doesn't sweep the whole chat. + append_and_apply( + conn, + kind="memory_written", + payload={ + "owner_id": "bot_a", + "chat_id": "chat_bot_a", + "pov_summary": "already-rated memory", + "witness_you": 1, + "witness_host": 1, + "witness_guest": 0, + "significance": 1, + }, + ) + prior_manual_edits = conn.execute( + "SELECT COUNT(*) FROM event_log WHERE kind = 'manual_edit'" + ).fetchone()[0] + + response = client.post( + "/chats/chat_bot_a/drawer/memory/significance/bulk", + data={"level_from": "0", "level_to": "2"}, + ) + assert response.status_code == 200 + + with open_db(db) as conn: + # Five new manual_edit rows, one per matching memory. + new_manual_edits = conn.execute( + "SELECT COUNT(*) FROM event_log WHERE kind = 'manual_edit'" + ).fetchone()[0] + assert new_manual_edits - prior_manual_edits == 5 + + # Every emitted edit is a memory_significance edit with prior=0 + # and new=2. + import json as _json + + rows = conn.execute( + "SELECT payload_json FROM event_log " + "WHERE kind = 'manual_edit' " + "ORDER BY id DESC LIMIT 5" + ).fetchall() + for r in rows: + payload = _json.loads(r[0]) + assert payload["target_kind"] == "memory_significance" + assert payload["prior_value"] == 0 + assert payload["new_value"] == 2 + + # Projection caught up — five memories at sig=2, the untouched + # one stays at sig=1, none remain at sig=0. + dist = dict( + conn.execute( + "SELECT significance, COUNT(*) FROM memories " + "WHERE chat_id = 'chat_bot_a' GROUP BY significance" + ).fetchall() + ) + assert dist.get(0, 0) == 0 + assert dist.get(1, 0) == 1 + assert dist.get(2, 0) == 5 + + def test_delete_turn_with_event_id_zero_returns_400(client, tmp_path): """T110.1: ``event_id <= 0`` is an obvious client error and must NOT silently rewind the entire log via ``after_event_id = -1``. The route