merge: T110 drawer Phase 4.5 bundle (event_id guard + html.escape + Jinja partial + bulk re-rate)
This commit is contained in:
@@ -0,0 +1,34 @@
|
||||
{# T110.3: delete-impact modal partial.
|
||||
|
||||
Rendered from :func:`chat.web.drawer.delete_preview` via a Jinja2
|
||||
TemplateResponse so HTML autoescape covers user-controllable fields
|
||||
(item.kind, item.description, notes) automatically — the prior
|
||||
f-string assembly required explicit html.escape() calls (T110.2)
|
||||
which become redundant under autoescape.
|
||||
|
||||
Inputs:
|
||||
``chat_id`` — the URL chat id (used to build the confirm form action).
|
||||
``impact`` — an :class:`~chat.services.delete_impact.ImpactReport`.
|
||||
#}
|
||||
<div class="delete-impact-modal">
|
||||
<h3>Delete event {{ impact.target_event_id }}?</h3>
|
||||
<p>This will discard {{ impact.cascading|length }} events. Cascade:</p>
|
||||
<ul class="delete-impact-cascade">
|
||||
{% if impact.cascading %}
|
||||
{% for item in impact.cascading %}
|
||||
<li><strong>{{ item.kind }}</strong>: {{ item.description }}</li>
|
||||
{% endfor %}
|
||||
{% else %}
|
||||
<li>none</li>
|
||||
{% endif %}
|
||||
</ul>
|
||||
<ul class="delete-impact-notes">
|
||||
{% for note in impact.notes %}
|
||||
<li>{{ note }}</li>
|
||||
{% endfor %}
|
||||
</ul>
|
||||
<form hx-post="/chats/{{ chat_id }}/drawer/turn/delete/{{ impact.target_event_id }}"
|
||||
hx-target="#drawer" hx-swap="innerHTML">
|
||||
<button type="submit">Confirm delete</button>
|
||||
</form>
|
||||
</div>
|
||||
@@ -547,6 +547,25 @@
|
||||
</ul>
|
||||
</details>
|
||||
{% 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. #}
|
||||
<details class="bulk-significance">
|
||||
<summary>Bulk re-rate significance</summary>
|
||||
<form class="inline-edit"
|
||||
hx-post="/chats/{{ chat.id }}/drawer/memory/significance/bulk"
|
||||
hx-target="#drawer" hx-swap="innerHTML">
|
||||
<label>
|
||||
From:
|
||||
<input type="number" name="level_from" min="0" max="3" value="0" required>
|
||||
</label>
|
||||
<label>
|
||||
To:
|
||||
<input type="number" name="level_to" min="0" max="3" value="1" required>
|
||||
</label>
|
||||
<button type="submit">Re-rate all</button>
|
||||
</form>
|
||||
</details>
|
||||
</section>
|
||||
|
||||
<section class="drawer-section">
|
||||
|
||||
+81
-21
@@ -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,
|
||||
@@ -1234,28 +1292,18 @@ async def delete_preview(
|
||||
|
||||
report = compute_delete_impact(conn, target_event_id=int(event_id))
|
||||
|
||||
# Build the modal HTML directly — the impact report is small and
|
||||
# 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`.
|
||||
items_html = "".join(
|
||||
f"<li><strong>{item.kind}</strong>: {item.description}</li>"
|
||||
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"<li>{note}</li>" for note in report.notes)
|
||||
body = (
|
||||
"<div class='delete-impact-modal'>"
|
||||
f"<h3>Delete event {report.target_event_id}?</h3>"
|
||||
f"<p>This will discard {len(report.cascading)} events. Cascade:</p>"
|
||||
f"<ul class='delete-impact-cascade'>{items_html or '<li>none</li>'}</ul>"
|
||||
f"<ul class='delete-impact-notes'>{notes_html}</ul>"
|
||||
f"<form hx-post='/chats/{chat_id}/drawer/turn/delete/{report.target_event_id}' "
|
||||
"hx-target='#drawer' hx-swap='innerHTML'>"
|
||||
"<button type='submit'>Confirm delete</button>"
|
||||
"</form>"
|
||||
"</div>"
|
||||
)
|
||||
return HTMLResponse(body)
|
||||
|
||||
|
||||
@router.post(
|
||||
@@ -1278,7 +1326,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}")
|
||||
|
||||
@@ -458,6 +458,183 @@ 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 '<div class="delete-impact-modal">' in body
|
||||
assert '<ul class="delete-impact-cascade">' 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
|
||||
ultimately user-controllable. Wrap them with ``html.escape`` so a
|
||||
payload like ``<script>alert(1)</script>`` 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: <prose excerpt>)"`` so
|
||||
# the prose flows verbatim into the cascade list <li>.
|
||||
with open_db(db) as conn:
|
||||
evil_id = append_and_apply(
|
||||
conn,
|
||||
kind="user_turn",
|
||||
payload={
|
||||
"chat_id": "chat_bot_a",
|
||||
"prose": "<script>alert('xss')</script>",
|
||||
"segments": [],
|
||||
},
|
||||
)
|
||||
|
||||
response = client.get(
|
||||
f"/chats/chat_bot_a/drawer/turn/delete-preview/{evil_id}"
|
||||
)
|
||||
assert response.status_code == 200
|
||||
body = response.text
|
||||
|
||||
# Raw <script> must NOT survive into the rendered HTML. The escaped
|
||||
# form (<script>) is what we want to see instead.
|
||||
assert "<script>alert" not in body
|
||||
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
|
||||
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).
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
Reference in New Issue
Block a user